3
\$\begingroup\$

I'm still new to C++ and I am very open to any kind of suggestion on how write proper and understandable C++ code. I decided to create a class for the code to have everything closely tided together. Don't know if that was a good idea.

About signature scanners (aka. pattern matchers), see see wiki. Signature scanners are commonly used in anti-virus. Basically a signature scanner can be used to identify a sequence of bytes that matches a sequence of bytes one already identified.

Masks are used to identify which bytes represent wildcards in the signature. If the pattern is "ff45b3" and the mask is "ff??b3" then the second byte, the "45" is a wildcard and should be skipped by the pattern scan function.

Main.cpp:

int main()
{
    SigScan scan("C:\\SimplePayload.dll"); // could be any dll right now.

    // Find the MZ DOS header: "MZ", but with a masked '??'
    scan.FindSignature("4d5a", "4d??", true);
    scan.PrintDictionary();

    // Find the PE file header: "PE".
    scan.FindSignature("5045", "5045", true);
    scan.PrintDictionary();
}

SignatureScan.h:

#pragma once

#include <iostream>
#include <fstream>
#include <windows.h>
#include <sstream>
#include <iomanip>
#include <map>

class SigScan {

private:
    std::string DllFile;                    // path to dll file. e.g.: "C:\\File.dll"
    std::string Sig;                        // full signature: e.g.: "4d5a90"
    std::string Mask;                       // full signature incl. mask: e.g.: "4d??90"
    std::string FirstSigByte;               // we start by comparing each byte with the signature's initial bytes.
    std::string Buffer;                     // holds the signature found byte by byte.

    unsigned int i;                         // is iterating over all bytes.
    unsigned int j;                         // is starting to iterate when the initial byte signature is found.
    unsigned int currentAddress;            // the current address of where the currentByte is at.
    unsigned int fileSize;

    BYTE* byteData;                         // contains the binary data

    std::map<int, std::string> Dictionary;  // will be used if "fullscan" is enabled. In case there are more signatures.


    // Convert byte data to readable string (hex)
    std::string hexStr(BYTE*, int);

    // Get current byte
    std::string CurrentByte();


    void CountAddress(unsigned int);

    // Read file
    void ReadFile();

    // Print address in uppercase hex format with 8 digits.
    void PrintCurrentAddress();

public:

    // Prints at what address the signature was found in binary
    void PrintDictionary();

    // Constructor
    SigScan(std::string);

    void FindSignature(std::string, std::string, bool);
};

SignatureScan.cpp:

#include "SignatureScan.h"

// Converts bytes to a readable string (hex representation).
std::string SigScan::hexStr(BYTE* data, int len)
{
    std::stringstream ss;
    ss << std::hex;

    for (int i(0); i < len; ++i)
        ss << std::setw(2) << std::setfill('0') << (int)data[i];

    return ss.str();
}

// Reads binary data byte by byte. 
std::string SigScan::CurrentByte() {
    // Wrapper around hexStr, which can otherwise also be used to print
    //  - i and j are adjusting the placement (see the function 'FindSignature').
    return hexStr(byteData + i + j, 1);
}

// Bytes per row. We count for every 16th bytes
void SigScan::CountAddress(unsigned int count)
{
    if (count % 16 == 0) {
        currentAddress = count;
    }
}

// Read file
void SigScan::ReadFile()
{
    std::ifstream File(DllFile, std::ios::binary | std::ios::ate);

    auto FileSize = File.tellg();

    fileSize = (unsigned int)FileSize;

    byteData = new BYTE[static_cast<UINT_PTR>(FileSize)];

    File.seekg(0, std::ios::beg);
    File.read(reinterpret_cast<char*>(byteData), FileSize);
    File.close();
}

void SigScan::PrintCurrentAddress() {

    // Print address in uppercase hex format with 8 digits.
    std::cout << std::uppercase << std::hex << std::setw(8) << std::setfill('0') << currentAddress << std::endl;
}


// public:
void SigScan::PrintDictionary() {
    for (auto& x : Dictionary)
    {
        std::cout << "[ Address: " << std::uppercase << std::hex << std::setw(8) << std::setfill('0') << x.first << " | Signature: " << x.second << " ]" << std::endl;
    }
}

// Constructor
SigScan::SigScan(std::string InDllFile) {

    DllFile = InDllFile; // saves dll

    ReadFile();          // takes dll path and store binary data in 'byteData'
}

void SigScan::FindSignature(std::string Sig, std::string Mask, bool fullscan)
{
    FirstSigByte = Sig.substr(0, 2); // Get the first byte from Sig.
    Dictionary.clear();              // Clear the dictionary for patterns before initiation.

    for (i = 0; i < fileSize; i++)
    {
        CountAddress(i); // Counts every 16th byte

        // If first byte of signature is equal to current byte, we may have a pattern.
        // (e.g.: FirstSigByte: "4d", CurrentByte(): "4d" 
        if (FirstSigByte.compare(CurrentByte()) == 0)
        {
            // We compare pair-wise, so we only need half of the iterations
            for (j = 0; j < (Sig.length() / 2); j++)
            {
                // Success if the next byte in signature is equal to current byte
                if (Sig.substr(j * 2, 2).compare(CurrentByte()) == 0)
                {
                    // Append "??" if it's mask
                    if (Mask.substr(j * 2, 2).compare("??") == 0)
                    {
                        Buffer.append("??");
                    }
                    // Append CurrentByte if it's not a mask.
                    else
                    {
                        Buffer.append(CurrentByte());
                    }
                }
                else
                {
                    // No match anyway, clear buffer and reset
                    Buffer.clear();
                    break;
                }
            }

            // If mask and buffer are equal (e.g.: "4d??90" == "4d??90" 
            if (Mask.compare(Buffer) == 0)
            {
                // If we want to find all patterns
                if (fullscan)
                {
                    // Appends address and buffer (holding the signature), then clear buffer and continue.
                    Dictionary.insert(std::pair<int, std::string>(currentAddress, Buffer));
                    Buffer.clear();
                }
                else
                {
                    // If we are fine with stopping when one signature is found, break loop.
                    break;
                }
            }
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

Overall Observations

It is important to remember when writing code professionally that you may not be the only one writing the code or maintaining and debugging the code. If the project is a high priority then there may be a team of programmers working on it. If the code is shipped, it may have a life span of a decade or more and you may not still be at the company (think winning the lottery or getting a better paying job with another company). Code should be easy to read, write, and be maintained by others.

The file extension .dll has a very specific meaning in the Microsoft Windows world, it is a Dynamically Loaded Library (DLL), it would be less confusing for anyone who has to maintain the code if the signature data file had a different file extension. By definition on Windows platforms you are linking your code to .dll files such as the C++ STL files so that it can run. There are special functions for loading DLL files at runtime. If you are scanning for virus signatures, then this program should also be able to search other types of files such as .exe, .doc, .docx, etc. and not just .dll.

Generally in when editing C++ programs one uses an Interactive Development Environment (IDE) such as Visual Studio, eclipse or CLion. These IDEs provide wizards for creating classes, and will automatically add header files and source files to the program that match the exact name of the class. For the files to have the same name as the class makes it easier for people who have to maintain the code to find the source code for the class. In this code the name of the header and source files are different from the name of the class and that can be confusing.

Private Versus Public in C++ Classes

Given the current organization of the file, where the private variables and functions precede the public variables and functions the keyword private is not required because by default all variables and functions are private in a C++ Class, this is different from a struct where all variables and functions are public by default.

That said, in object oriented programming the public interfaces in an object declaration are generally listed first so that the users of the class (other developers that may be working in parallel to you) can find the public interfaces quickly. In most of the C++ code I have seen the constructors and destructors are listed first (when they exist).

The function organization in SignatureScan.cpp should list Constructors first, then destructors (when needed), then the public functions, and finally the private functions.

Very short public or private functions that probably won't be modified don't need to be in the .cpp file, they can be in the header file. Examples of these kinds of functions std::string SigScan::CurrentByte() and void SigScan::CountAddress(unsigned int count). Doing this will allow an optimizing compiler to decide what should be inlined so that the code will run faster.

Header Files

Within header files, only include header files that are necessary for the code to compile, this will decrease the compile / build time for those source files that include the header file. In the code presented, there are 6 header files included but only 3 of these files are necessary for the code to compile in a source file that includes the header file (windows.h, string and map). Include the other headers necessary in the source file SignatureScan.h.

Variable Names

There are 2 private variables declared in the header file that have questionable names, i and j. This forced added comments in both the header file and the source file. Write self documenting code as much as possible using more descriptive variable names so that comments are not as necessary. The problem with comments is that they also need to be maintained, and therefore add cost to the maintenance of the software.

Based on my earlier comment about DLLs, the variable name could be changed to fileToScan.

The variable names in the function prototypes are important, especially in the public function prototypes. These variable names will give the users of the functions an idea of what the variable is to be used for.

Use C++ Container Classes Rather Than Old Style C Arrays or Pointers

The class definition of SigScan contains the variable declarations

    unsigned int fileSize;

    BYTE* byteData;                         // contains the binary data

While fileSize may be needed for multiple reasons, the most important reason seems to be that it is the size of byteData. There are 2 different ways these 2 variables could be combined into 1 complex variable, the first would be to use the C++ type array and the second would be to use the C++ type vector. On of the values of using a C++ container type is being able to pass both variables in a single parameter. A second value of using a C++ container type is that you can use a range based for loop that would reduce the code necessary in std::string SigScan::hexStr(BYTE* data, int len) and possibly run faster because it is using iterators.

std::string SigScan::hexStr(std::vector<BYTE> data)
{
    std::stringstream ss;
    ss << std::hex;

    for (BYTE byte: data)
    {
        ss << std::setw(2) << std::setfill('0') << (int)byte;
    }

    return ss.str();
}

Note that there is no need to specify the index i in the above loop.

Another reason is that raw pointers are frowned upon in modern C++ because they lead to bugs.

I may have time to review void FindSignature(std::string, std::string, bool) later, but there is enough information now.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you so much, @pacmaninbw. I noted everything you said. Also, it makes sense. If you have time for FindSignature I'd appreciate it, but there's more than enough to get me started right now. Thanks again \$\endgroup\$
    – Hummas
    May 22 '20 at 17:01
  • \$\begingroup\$ The paragraph about the missing destructor is misleading up to wrong: If not specified otherwise, every class has a destructor. For classes which do not deal with ownership, it is encouraged to use the rule of zero. Explicitly defaulting the destructor even implicitly deletes the move-constructors (see same link), which is very bad for a class holding something large like a map! Please correct your answer accordingly. \$\endgroup\$
    – Emma X
    May 22 '20 at 19:41
  • \$\begingroup\$ @EmmaX anything else? \$\endgroup\$
    – pacmaninbw
    May 22 '20 at 20:32
  • \$\begingroup\$ @pacmaninbw I’m not aware of any reason why iterators would be faster than indexes in cases of contiguous memory, but one should prefer iterators nonetheless due to being more generic. Other than that I’m quite fond of your review. \$\endgroup\$
    – Emma X
    May 22 '20 at 23:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.