Using this XOR encryption in C++

[qwerz]

Hello,

I am currently using a byte encryption in C# which gets decrypted in my C++ client.

It is XOR, but I can't get it to work. Is there anything that Im doing wrong?
I already wrote the C++ XOR in C#, which look pretty much the same.

C#

        
public byte[] XOR(byte[] strng, byte[] key)
        {
            int string_len = strng.Length;
            int key_length = key.Length;

            int i, position;

            for (i = 0; i < string_len; i++)
            {
                position = i % key_length;
                strng[i] = Convert.ToByte(strng[i] ^ key[position]);
            }
            return strng;
        }

C++

char *encrypt(char *string, char *key)
{
	int string_len = strlen(string);
	int key_length = strlen(key);

	int i,position;

	for(i = 0; i < string_len; i++)
	{
		position = i % key_length;
		string[i] = (char)((int)string[i] ^ (int)key[position]);
	}
	return string;
}

In C# the bytes get encrypted like:

byte[] bytestocrypt = //here my source of the bytes
byte[] b = new byte[] { 4, 5, 3, 0 };
bytestocrypt = XOR(bytestocrypt, b);

in C++

LPVOID pBuffer = //here source of decrypted bytes
char b[4] = {4, 5, 3, 0};
char *decrypted = encrypt((char *)pBuffer, b);
pBuffer = decrypted;

Thanks in advance.

[Paul McKenzie]

Hello,

I am currently using a byte encryption in C# which gets decrypted in my C++ client.

It is XOR, but I can't get it to work. Is there anything that Im doing wrong?
I already wrote the C++ XOR in C#, which look pretty much the same.

Pointers are not arrays. The code you wrote is not the same as the C# code. The C# code actually uses arrays. Pointers are just that in C++ -- pointers. They are not arrays (although you use pointers to point to the beginning of arrays). You can pass arrays to C# and they remain arrays, not so in C++, so the real solution is to use constructs that maintain their "arrayness" when passed around.

Second, post the main() program. Your attempt of using pointers is highly dependent on how you called that function. For example, this tells us nothing:

LPVOID pBuffer = //here source of decrypted bytes

Everything, and I mean everything depends on how you initialized that pBuffer.

Third, you posted another question concerning this, and my response to you is the same -- ditch the pointers and use classes. Your C++ code not only is not the same, it is dangerous as heck. I can easily break it like this:

char *encrypt(char *string, char *key);
//..
encrypt("abc123", "123456");  // writing to a string literal as the first argument -- possible crash
encrypt(NULL, NULL);  // calling strlen() on a NULL pointer -- undefined behaviour

Both of those lines compile. Now what happens when you run it? The first one has a string-literal as the first argument. The second has a NULL as the first argument. Providing those values as arguments makes your encrypt function go haywire.

I could go on and on about how that function is crash-prone. You're using C++, not 'C' -- there is absolutely no excuse to make crash-prone functions like this. To make it more bullet-proof, use what I mentioned in the other thread, and that is container classes.

#include <vector>
typedef std::vector<char> ByteArray;
//...
ByteArray& XOR(ByteArray& strng,  const ByteArray& key)
{
    size_t string_len = strng.size();
    size_t key_length = key.size();
    int position;

    for (size_t i = 0; i < string_len; i++)
    {
           position = i &#37; key_length;
           strng[i] = (ByteArray::value_type)(strng[i] ^ key[position]);
    }
    return strng;
}

This code is much more safer to use. Also, here is a radically different way in C++ to do the same code.

#include <algorithm>
#include <vector>

typedef std::vector<char> ByteArray;

struct XORTransformer
{
    int curPos, m_size;
    const ByteArray& m_key;
    XORTransformer(const ByteArray& key) : m_key(key), curPos(-1), m_size(key.size()) { }
    ByteArray::value_type operator()(ByteArray::value_type val) 
    {    
        ++curPos;   
        return (ByteArray::value_type)(val ^ m_key[curPos % m_size]);
    }
};

template <typename Transformer>
ByteArray&  Transform(ByteArray& strng, const ByteArray& key)
{
    std::transform(strng.begin(), strng.end(), strng.begin(),  Transformer(key));
    return strng;
}

int main()
{
    char bytes[] = "xabcde54";
    char keys[] = "12345678";
    ByteArray key(keys, keys + sizeof(keys) / sizeof(keys[0]));
    ByteArray barr(bytes, bytes + sizeof(bytes)/sizeof(bytes[0]));
    Transform<XORTransformer>(barr, key);
}

The utility of doing it this way may not be apparant, but basically, I created a function where you can plug-in the transformation you want to use. If you come up with another way to transform the source string, just call:

Transform<AnotherWay>(barr, key);

Where AnotherWay is a class that handles the key and transforms the source string in whatever way it sees fit.

Regards,

Paul McKenzie

[qwerz]

Thank you for your reply.

That is indeed right, I could not get the other version working. So this time, instead of translating C# to C++, I chose a snippet which was posted online.

I translated this into C# because that is the language that I normally use to code, and understand.

Your solution seems to be right, but I would like to use an algorithm instead of these libraries to XOR.

This XOR snippet was posted online and seemed to work for the people.

And thanks for pointing out the problems.

---

Btw I used your above snippet and there still seems to be an error on the first line, passing the first argument.

And the source of the Buffer is just a bytearray from RC DATA embedded resources. (FindResource(0, "0", "RT_RCDATA");

[Paul McKenzie]

Your solution seems to be right, but I would like to use an algorithm instead of these libraries to XOR.

Where did I not use the algorithm you posted? It's in both the examples I posted -- you just don't recognize it.

Btw I used your above snippet and there still seems to be an error on the first line, passing the first argument.

The full example I posted works correctly. I don't know what you're doing or what data you're using to have such an error.

And the source of the Buffer is just a bytearray from RC DATA embedded resources. (FindResource(0, "0", "RT_RCDATA");

OK, so what's the problem? You can take any byte array and place it in the vector.

That's why I mentioned to you in the other thread to learn the interface of the vector class. If you did that, you wouldn't see that there is any problem.

Regards,

Paul McKenzie

[qwerz]

Where did I not use the algorithm you posted? It's in both the examples I posted -- you just don't recognize it.

Sorry I did recognize it of course, but I just wanted to say that the second example that you coded was not an option for me.

Well if it is working for you, then I will be making a mistake?
This is the full code that I am talking about.

#include <windows.h>
#include "extra.h"
#include <vector>
typedef std::vector<char> ByteArray;

HINSTANCE instance;
WPARAM wParam ;


ByteArray& XOR(ByteArray& strng,  const ByteArray& key)
{
    size_t string_len = strng.size();
    size_t key_length = key.size();
    int position;

    for (size_t i = 0; i < string_len; i++)
    {
           position = i &#37; key_length;
           strng[i] = (ByteArray::value_type)(strng[i] ^ key[position]);
    }
    return strng;
}

LPVOID noel()
{
    LPVOID pBuffer = NULL;

    DWORD dwSize;
    LPBYTE lpData;
    HRSRC hRsrc = FindResource(0, "0", "RT_RCDATA");

    HGLOBAL hGlobal = LoadResource(0, hRsrc);
    dwSize = SizeofResource(0, hRsrc);

    lpData = (LPBYTE)LockResource(hGlobal);
    pBuffer = lpData;

    char b[4] = {4, 5, 3, 0};
    char decrypted[] = XOR(pBuffer, b);
    pBuffer = decrypted;

    return pBuffer;
}

Maybe you can see the error?

|45|error: invalid initialization of reference of type 'ByteArray& {aka std::vector<char>&}' from expression of type 'char*'|
|13|error: in passing argument 1 of 'ByteArray& XOR(ByteArray&, const ByteArray&)'|

Could be beginners mistake , excuse me for that.

[Paul McKenzie]

Sorry I did recognize it of course, but I just wanted to say that the second example that you coded was not an option for me.

Well, it is the same as the first example, except it's wrtten in a much more flexible way.

Well if it is working for you, then I will be making a mistake?
This is the full code that I am talking about.

Even if you didn't use ByteArray, there are problems with your code.

First, All of those Windows API calls are off-topic in this forum. Assume I know nothing about them or what they do.

    LPVOID pBuffer = NULL;
    //...
    pBuffer = lpData;

So is pBuffer NULL terminated array of char? If not, then your original XOR function using char* would not have worked properly, since calling strlen() on data you aren't sure is NULL terminated leads to strlen() going off the deep-end trying to find the NULL.

If it is NULL terminated, then you need to create ByteArray's from this data. You can't just send it to the function, as raw char data is not a ByteArray. Remember that ByteArray is a vector -- a full-blown class that wraps an array, and isn't just an array. You have to get the raw data into the ByteArray object somehow. That's why I mentioned to you to study the interface of vector, so that you know how to convert from a raw type to a vector:

    pBuffer = lpData;
    ByteArray b1(pBuffer, pBuffer + strlen(pBuffer));
    ByteArray b2(b, b + sizeof(b));
    XOR(b1, b2);

Then you use b1 and b2 from there, as pBuffer is no longer needed at that point.

There is a constructor for vector that takes two arguments, where both arguments are iterators to starting and beginning of the sequence of values. In the example, you want to create a vector that consists of characters from pBuffer and from b.

The b1 vector is created by supplying the start of pBuffer and the end of pBuffer. The same with b2 -- the start of b and the end of b are provided. Then presto, you have the vector of characters.

Again, I'm assuming you know that pBuffer is NULL terminated. If it isn't, then you need to know the number of bytes so as to set up the b1 vector correctly (you can't use strlen()).

    ByteArray b1(pBuffer, pBuffer + whatever_number_of_bytes);

Regards,

Paul McKenzie

[monarch_dodra]

...

Nice design, but your XORFunction object carries a variable state, which makes it implicitly dangerous, especially when mixed with the STL (transform does not necessarily guarantee the range will be transformed in-order if you provide it with a random access iterator, for example).

IMO, the Encoder should take an entire input range directly anyways, as some encoders will need the entire input to function correctly anyways.

About this line:

template <typename Transformer>
ByteArray&  Transform(ByteArray& strng, const ByteArray& key)

Did you purposefully omit to pass a Transformer object? I suppose it was to prevent the re-use of the XORFunction (which would be problematic, given the whole state thing)?

Function-like parameters parameters are usually also passed as an argument, in case they can be parametrized (as far as I have seen anyways). Plus, it avoids having to specify the parameterization of the function when calling it.

[Paul McKenzie]

Nice design, but your XORFunction object carries a variable state, which makes it implicitly dangerous, especially when mixed with the STL (transform does not necessarily guarantee the range will be transformed in-order if you provide it with a random access iterator, for example).

True. I was assuming the iterator was random access and did an in-order traversal (from begin() to end() in that order).

template <typename Transformer>
ByteArray&  Transform(ByteArray& strng, const ByteArray& key)

Did you purposefully omit to pass a Transformer object? I suppose it was to prevent the re-use of the XORFunction (which would be problematic, given the whole state thing)?

Not on purpose, no. I could have rewritten it to take a Transformer as a parameter also. I was thinking more of the lines of a policy-based design.

All you're saying is true however.

Edit:

Regards,

Paul McKenzie

[qwerz]

Hmm alright thanks. Ill read into that vectors and such.

But the thing that I dont get is why even a Char encryption fails for me. I must be doing something wrong.

I did a simple test with a Char array by Xoring it twice (which would result in the input)

char *XOR(char *str, char *cipher)
{
    for (size_t i = 0; i < strlen(str); i++)
    {
        str[i] ^= cipher[i &#37; strlen(cipher)];
    }

    return str;
}

    char *CPA = XOR((char*)"CreateProcess", (char*)"Sddd");
    CPA = XOR(CPA, (char*)"Sddd");

This failed too, the CPA output was not the original that I entered. Pretty much tried with alot of different XOR encryptions and variations.
Failed in all for some reason.

[Paul McKenzie]

But the thing that I dont get is why even a Char encryption fails for me. I must be doing something wrong.

I already mentioned what you're doing wrong:

char *XOR(char *str, char *cipher)
{
    for (size_t i = 0; i < strlen(str); i++)
    {
        str[i] ^= cipher[i &#37; strlen(cipher)];
    }

    return str;
}

    char *CPA = XOR((char*)"CreateProcess", (char*)"Sddd");
    CPA = XOR(CPA, (char*)"Sddd");

Didn't I already mention that you cannot write on a string-literal? You are passing a string-literal, and writing to a string-literal is undefined behaviour. All of that casting doesn't turn a string-literal into something else.

You must pass a writable buffer, i.e. a true array of chars, not a quoted string-literal.

Regards,

Paul McKenzie

[qwerz]

You must pass a writable buffer, i.e. a true array of chars, not a quoted string-literal.

Regards,

Paul McKenzie

Oh I thought the string woulc become an array if I added (char*).
Like
arraybit[0] = C
arraybit[1] = r

Then how do you suggest to encrypt that string with a XORencryption?

Regards,

[Paul McKenzie]

If you want quick proof of how dangerous this is:

int main()
{
    char *p = "abc123";
    p[0] = 'x';
}

What do you think will happen when you run this program? Most of the time, the program will crash as soon as you change p[0]. Maybe it won't crash, and you have the false sense that the program is OK when it really isn't. This is what your first call to XOR was doing.

This is what I mean by writing to a string-literal as invoking undefined behaviour. Unlike languages such as C#, where if you do something illegal, you always get the runtime to let you know, C++ doesn't work that way. You make a mistake like this, and you don't know what will happen. Maybe it will work, maybe it will crash, maybe it works on your machine but crash on another, etc.

The following is the correction:

int main()
{
    char p[] = "abc123";
    p[0] = 'x';
}

This program now is perfectly fine. See the difference?

Regards,

Paul McKenzie

[Paul McKenzie]

Then how do you suggest to encrypt that string with a XORencryption?

You must provide a writable buffer, an actual char array for example. You were not doing that in your attempt.

That's the danger of doing this, and why I mentioned back in my second post on this thread the many ways I can break your code. You still didn't address the NULL first argument -- that hole is still there.

Regards,

Paul McKenzie

[qwerz]

Uhm yes I see the difference, but I have no idea why it would be dangerous.

Are you saying that the difference between
char p[] and char *p is that char p[] is really a char array while the first one is a string-literal and not writable?

[Paul McKenzie]

Uhm yes I see the difference, but I have no idea why it would be dangerous.

Are you saying that the difference between
char p[] and char *p is that char p[] is really a char array while the first one is a string-literal and not writable?

Yes. Why not try the small program and see what happens? Depending on the compiler you're using, the code that uses the string-literal will immediately crash on the assignment to p[0].

Regards,

Paul McKenzie