truecrypt bug in serpent implementation

Question

I was browsing the Serpent implementation in TrueCrypt (7.1a) and something does not look right! Here is the interface of this algorithm:

void serpent_set_key(const unsigned __int8 userKey[], int keylen, unsigned __int8 *ks);
void serpent_encrypt(const unsigned __int8 *inBlock, unsigned __int8 *outBlock, unsigned __int8 *ks);
void serpent_decrypt(const unsigned __int8 *inBlock,  unsigned __int8 *outBlock, unsigned __int8 *ks);

The function of interest here is serpent_set_key. The user key is 32 bytes in length, the keylen should be its size and the ks is the output key to be used for encryption/decryption. The issue is the implementation. Here is the relevant fragment at the beginning:

unsigned __int32 a,b,c,d,e;
unsigned __int32 *k = (unsigned __int32 *)ks;
unsigned __int32 t;
int i;

for (i = 0; i < keylen / (int)sizeof(__int32); i++)
    k[i] = LE32(((unsigned __int32*)userKey)[i]);

The for loop, is actually copying the data from the user key to the implementation key. It is done by 'looking' at the data as 4 bytes integers. Now, all is ok if the key len is sent in as bytes (32 is the correct value) but...

In all the implementation of trueCrypt this is called in two places. Here is the first one: In CipherInit is called like this:

case SERPENT:
    serpent_set_key (key, CipherGetKeySize(SERPENT) * 8, ks);
    break;

CipherGetKeySize(SERPENT) will return 32 (bytes) so the passed in param will have a value of 256! That is correct as the length of the key is concerned but, NOT for this implementation! This will cause buffer overflow in 'serpent_set_key' because the for loop will be run 64 times instead of just 8! The other place where this is called is in EAInit like this:

serpent_set_key (key, 32 * 8, ks);

Here is it obvious that the passed in param will be 256.

I am curious what other people think about this? Can anyone else confirm this bug?

Answer 1

As VeraCrypt main developer, a user has redirected me to this post since VeraCrypt is based on TrueCrypt source .

After studying the issue you raised, I can confirm that this is indeed a mistake in the code and that calls to serpent_set_key should pass 32 instead of 256 as a parameter.

Luckily, this mistake has no effect on the correctness or security during the execution of the program, this is why no body discovered it before you. Thus, we can NOT qualify this as a bug.

Let me explain this in three points :

1. let's look at the Serpent algorithm implementation of serpent_set_key : the parameter keylen is used only for copying user key into the ks buffer which is guaranteed to have a minimum size of 560 (look at the SERPENT_KS define in crypt.h). Thus, even if keylen is 256 instead of 32, we will never write beyond the allocated memory of ks.
2. the internal key expansion that comes after this loop will build the expanded Serpent key using the first 32 bytes of userKey only as in the Serpent algorithm specification. Thus, all bytes coming after the first 32 ones will be discarded and they will be never used. This explains why the calculation result is correct even if 256 bytes are passed to it instead of the expected 32 bytes.
3. if we list all the runtime calls the leads to serpent_set_key we'll notice that, except for the case of auto tests, all the calls use a 256 bytes buffer for the userKey parameter even if its first 32 bytes are filled (look at MASTER_KEYDATA_SIZE in crypto.h). So, during runtime, we'll never read beyond the allocate buffer space. It remains the case of the auto tests (e.i. in Tests.c or CipherTestDialogProc in Dlgcode.c) where a 32 bytes buffer is used for userKey : here we will read beyond the allocated space but in practice it doesn't cause any harm because the memory around this buffer is readable.

I hope this clarifies why this mistake is harmless. That being said, it has to be corrected and that's what we'll do in VeraCrypt.

For the record, it appears that this mistake was caused by mix-up between twofish_set_key and serpent_set_key : the declaration of the two functions have the same type of parameters but twofish_set_key expects the user key length in bits whereas serpent_set_key expect it in bytes! Clearly, we should have the same convention for the size of a key.