C++ XOR encryption

Question

After reading several white papers on cryptography and runtime PE crypters, I decided to write my own. It's very simple and only for educational purposes.

Here is the GitHub repo: https://github.com/Jyang772/XOR_Crypter

I have two questions.

• First, why do I have to keep changing my file permissions to start every outputted .exe (File created by Builder.exe not the compiler)? It creates a file that is Shared. I have to right click it and select share with Nobody. Does this have something to do with the File Access and Security Rights? I am using CreateFile() and Readfile to read and write the input and output files.

http://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).aspx

• Second, I can't seem to get XOR encryption to work. It seems pretty straight forward for what I have done. The byte sizes are the same. While I was investigating, I had the Builder and the Stub each output a file with the file data unencrypted. They are the same. Then I tried with the data encrypted. There is no doubt the data is encrypted with the cipher, however it shows up blank when it is decrypted by the stub later on. I'm confused.

Here is my XOR implementation:

fs = byte size Rsize = byte size Should be the same.

Builder:

 char cipher[] ="penguin";
      for (int i = 0; i < fs; i++)
        {   
                FB[i] ^= cipher[i % strlen(cipher)]; // Simple Xor chiper
        }

Stub:

char cipher[] = "penguin";
for (int i = 0; i < Rsize; i++)
    {
        RData[i] ^= cipher[i % strlen(cipher)];
    }

If I were to comment out the encryption function in the Builder and Stub, the crypted file runs fine. Uhh, except with the permissions error.

I'm also trying to include a options menu where the user can select the encryption method used. Perhaps I might have done something wrong there? The Builder.exe adds one byte containing the user's choice to the end of FB buffer. Stub.exe reads that and determines which encryption method is used to decrypt the data.

Solution 1

First off, with XOR "encryption", your "encrypt" and "decrypt" functions should be the same:

void xor_crypt(const char *key, int key_len, char *data, int data_len)
{
    for (int i = 0; i < data_len; i++)
        data[i] ^= key[ i % key_len ];
}

You should be able to use this same function in both the "XOR Crypter" program as well as your "Stub" program.

It's not a very C++ style; ordinarily you'd use std::string or std::vector. For example:

void xor_crypt(const std::string &key, std::vector<char>& data)
{
    for (size_t i = 0; i != data.size(); i++)
        data[i] ^= key[ i % key.size() ];
}

Then in the program that calls this, you'd declare:

std::string key = "penguin";

and you'd read your file in like so:

std::vector<char> file_data;  // With your current program, make this a global.

fs = GetFileSize(efile, NULL);
file_data.resize(fs);    // set vector length equal to file size

// Note:  Replace &( file_data[0] ) with file_data.data() if you have C++11 support
ReadFile(efile, (LPVOID)( &( file_data[0] )), fs, &bt, NULL);   

if (fs != bt)
    // error reading file:  report it here.

Then you would simply encrypt with xor_crypt( key, file_data );. To write the XOR-crypted data to your resource, I believe you'd call your existing function with:

// replace &( file_data[0] ) with file_data.data() if C++11
WriteToResources(output, 1, (BYTE *)&( file_data[0] ), file_data.size() ); 

I suspect the real issue is with the Windows APIs you're using. Does LoadResource give you mutable data, or are you required to copy it? I don't know the Windows API, but I wouldn't be surprised if LoadResource gives you a read-only copy.

If you do need to make your own copy in order to modify the resource, then in your "Stub" program recovering the XOR-crypted resource should look something like this:

std::vector<char> RData;

void Resource(int id)
{
    size_t Rsize;

    HRSRC hResource = FindResource(NULL, MAKEINTRESOURCE(1), RT_RCDATA);
    HGLOBAL temp = LoadResource(NULL, hResource);
    Rsize = SizeofResource(NULL, hResource);
    RData.resize(RSize);
    memcpy( (void*)&(RData[0]), temp, RSize );  // replace &RData[0] with RData.data() if C++11
}

and the decryption in your "Stub" should just be xor_crypt( key, RData );.

I have one last thought. The biggest bug I see in your "Stub" program is this line:

    switch (RData[strlen(RData)-1])

Once you've XOR-crypted your data, some of the bytes will become zero. The strlen() function will not return the index of the last byte in your RData as a result. And, there's a different, more subtle error: This returns the last byte of the string, not the last byte of the resource. I can't really see how this line was ever correct; rather, I suspect your program was working when encryption was disabled in spite of itself, by falling through to the default of the switch-case.

If you really intend to distinguish between different types of data based on the last byte of the resource payload, then you really should just use the size returned by the Windows API to find that byte.

If you switch to using vector<char> as I suggest above, then you can find that with RData.back(). Otherwise, if you continue using char *, then that byte would be RData[RSize - 1].

Solution 2

Depending on your content data, you write char option in allocated memory pointed by FB or after it (buffer overrun) in "C++ Builder/main.cpp" when calling strcat(FB, choice).

Fix: allocate enough space in FB for data + option char. As you are dealing with binary data you should not use string functions (ex: strcat).

FB = new char[fs + 1];
memcpy(FB +fs, option, 1); // copy the option at end

Comments

[Quaxton Hale]

Thank you for taking the time to read through my code and giving me a very detailed answer. I have taken your advice and commented out my C-style functions with the ones you suggested. The only part that I needed to fix was making sure file_data and data in the xor function were the same. Otherwise I'm just XOR crypting a copy of file_data

[Quaxton Hale]

Thanks for all the help, I included your name in my project. The only problem I have to fix now is figuring out why the crypted file is always being outputted as a shared file.

[Admin]

Here is a slight variation that uses a 64-bit integer as a key. _UINT8 *XorCipher(_UINT8 *arr, size_t arrLen, _UINT64 key) { // Allocate Output buffer _UINT8 *outArr = new _UINT8[arrLen]; // Refer to the 64-bit key as a 8 byte byte array _UINT8 *keyArr = (_UINT8*)&key; for (size_t i = 0; i < arrLen; i++) outArr[i] = arr[i] ^ keyArr[i % 8]; return outArr; }

[Joe Z]

One thing to watch out for with that approach, @IngeHenriksen, is endianness. In this case, the same type of computer will be used to encrypt and decrypt. If two different machines are sharing information, then they both have to understand the endianness of the key.