CODEWITHSUNDEEP
c++
Fluffy
Fluffy

Reputation: 761

String encryption function works with char[], but not a plain string

I'm using version xtea encryption from wikipedia that's written in C++. I wrote a function to encrypt a string

const char* charPtrDecrypt(const char* encString, int len, bool encrypt)
    {

        /********************************************************
        * This currently uses a hard-coded key, but I'll implement
        * a dynamic key based on string length or something.
        *********************************************************/
        unsigned int key[4] = { 0xB5D1, 0x22BA, 0xC2BC, 0x9A4E };

        int n_blocks=len/BLOCK_SIZE;
        if (len%BLOCK_SIZE != 0)
            ++n_blocks;

        for (int i = 0; i < n_blocks; i++)
        {
            if (encrypt)
                xtea::Encrypt(32, (uint32_t*)(encString + (i*BLOCK_SIZE)), key);
            else
                xtea::Decrypt(32, (uint32_t*)(encString + (i*BLOCK_SIZE)), key);
        }
        return encString;
    }

It works when I supply a const char encString[] = "Hello, World!", but when I supply a raw string e.g. const char* a = charPtrDecrypt("Hello, World!", 14, true) It crashes.

Upvotes: 2

Views: 471

Answers (2)

Jerry Coffin
Jerry Coffin

Reputation: 490108

There's an old saying (I know it's old, because I first posted it to Usenet around 1992 or so) that: "If you lie to the compiler, it will get its revenge." That's what's happening here.

Here:

const char* charPtrDecrypt(const char* encString, int len, bool encrypt)

...you promise that you will not modify the characters that encString points at. That's what the const says/means/does.

Here, however:

xtea::Encrypt(32, (uint32_t*)(encString + (i*BLOCK_SIZE)), key);

...you cast away that constness (cast to uint32_t *, with no const qualifier), and pass the pointer to a function that modifies the buffer it points at.

Then the compiler gets its revenge: it allows you to pass a pointer to data you can't modify, because you promise not to modify it--but then when you turn around and try to modify it anyway, your program crashes and burns because you try to modify read-only data.

This can be avoided in any number of ways. One would be to get away from the relatively low-level constructs you're using now, and pass/return std::strings instead of pointers to [const] char.

The code has still more problems than just that though. For one thing, it treats the input as a block of uint32_t items, and rounds its view of the length up to the next multiple of the size of a uint32_t (typically 4). Unfortunately, it doesn't actually change the size of the buffer, so even when the buffer is writable, it doesn't really work correctly--it still reads and writes beyond the end of the buffer.

Here again, std::string will be helpful: it lets us resize the string up to the correct size instead of just reading/writing past the end of the fixed-size buffer.

Along with that, there's a fact the compiler won't care about, but you (and any reader of this code) will (or at least should): the name of the function is misleading, and has parameters whose meaning isn't at all apparent--particularly the Boolean that governs whether to encrypt or decrypt. I'd advise using an enumeration instead, and renaming the function to something that can encompass either encryption or decryption:

Finally, I'd move the if statement that determines whether to encrypt or decrypt outside the loop, since we aren't going to change from one to the other as we process one input string.

Taking all those into account, we could end up with code something like this:

enum direction { ENCRYPT, DECRYPT };

std::string xtea_process(std::string enc_string, direction d) {

    unsigned int key[4] = { 0xB5D1, 0x22BA, 0xC2BC, 0x9A4E };

    size_t len = enc_string.size();

    len += len % BLOCK_SIZE;    // round up to next multiple of BLOCK_SIZE

    enc_string.resize(len);     // enlarge the string to that size, if necessary

    if (direction == DECRYPT)
        for (size_t i = 0; i < len; i+=BLOCK_SIZE)
            xtea::Decrypt(32, reinterpret_cast<uint32_t *>(&encString[i]), key);
    else
        for (size_t i = 0; i < len; i += BLOCK_SIZE)
            xtea::Encrypt(32, reinterpret_cast<uint32_t *>(&encString[i]), key);
    }
    return encString;
}

This does still leave (at least) one point that I haven't bothered to deal with: some machines may have stricter alignment requirements for a uint32_t than for char, and it's theoretically possible that the buffer used in a string won't meet those stricter alignment requirements. You could run into a situation where you need to copy the data out of the string, into a buffer that's properly aligned for uint32_t access, do the encryption/decryption, then copy the result back.

Upvotes: 4

m3tikn0b
m3tikn0b

Reputation: 1318

You pass a constant const char* to the function but cast it to a non-constant uint32_t*. I guess that xtea::Encrypt modifies the string buffer in place.

In the first version const char encString[] = "Hello, World!" the variable --while being const-- most likely lies on the stack which is modifiable. So it's "not nice" to remove the const, but it works.

In the second version you string most likely lies in a read-only data segment. So casting away const let's you call the Encrypt function, but crashes as soon as the function really tries to modify the string.

Upvotes: 2

Related Questions