Memory error - Simple XOR encryption in C

Question

I am having some memory issues with printing hex in the following format: \xAA\xAB\xDC using my encryption routine.

I did some modifications, using snprintf() and strcat() in an attempt to fix the output and it worked to some degree.

This is the function I originally started out with, which is probably better than my modified version.

char *encrypt(char key, const char *a) {
    char *output = malloc(strlen(a)+1);
    bzero(output, strlen(a)+1);
    strcpy(output, a);
    char *tmp = output;
    int i;
    for (i = 0; tmp[i] != 0; i++) {
        tmp[i] = key ^ tmp[i];
    }
    return output;
}

My current progress is as follows:

char *encrypt(char key, const char *a) 
{
    char buf[256];
    char *tmp = a;
    int i;
    int *k;

    for (i = 0; tmp[i] != 0; i++)
    {
        char temp[10];
        k = key ^ tmp[i];
        snprintf(temp, sizeof(temp), "\\x%s", k);
        strcat(buf, temp);
    }
    return buf;
}

int main(int argc, char **argv)
{
    if (argv[1] == NULL){
        printf("Usage: %s <string>\n", argv[0]);
    }
    else printf("Encrypted string: %s\n", encrypt(0xEB, argv[1]));
    return 0;
}

If anyone could point me in the right direction on how to fix the memory issue, and if the code can be improved I would appreciate that a lot.

Answer 1

The primary issue, in your code, buf is local to the function encrypt(). So you may not return the array from the function. Once the function finishes, the array will cease to exist and the returned address will be invalid. If the returned value is ued in the caller, it will invoke undefined behavior.

You need to define buf as a pointer and allocate dynamic memory using malloc() or family. Also, you need to free() the memory, once the usage is over.

That said,

• you have defined k to be a pointer but did not allocate memory to it.
• k = key ^ tmp[i]; seems meaningless, maybe you meant *k = key ^ tmp[i];
• %s expects a pointer to char array (null-terminated) as argument. From that point, snprintf(temp, sizeof(temp), "\\x%s", k); also looks wrong. What you need is snprintf(temp, sizeof(temp), "\\x%d", *k); to print the int value.

Answer 2

Instead of:

int *k;
k = key ^ tmp[i];
snprintf(temp, sizeof(temp), "\\x%s", k);

use this:

unsigned char k;
k = key ^ tmp[i];
snprintf(temp, sizeof temp, "\\x%02X", k);

Note that you also have other changes to make regarding buf. Firstly you never initialize it, so you are appending to junk. And you never check that you didn't overflow it.

Also you attempt to return this from a function, however, since it is a local variable, it ceases to exist when the function returns.

See this thread for some suggestions of how to get freshly-written string out of a function. You could use malloc(256) in the same vein as your first attempt (and remember to replace sizeof buf with the mallocated length, in the snprintf call).

---

It'd be more robust to use unsigned char instead of char for both the key and the message. An example of the issues is that on x86 or x64, char has a range of -128 to 127 so when you supply 0xEB (i.e. 235) this is an out-of-range assignment which is not well-defined.

But on common systems you will get away with using char because they tend to define out-of-range assignment by using 2's complement and truncating excess bits, which works in your situation.