Here is something that looks like a bug in TrueCrypt, is this a bug in your implementation?

[Anonymous]

http://stackoverflow.com/questions/22122509/truecrypt-bug-in-serpent-implementation

Please send email to margelatu227@gmail.com

Thanks,
M

[Mounir IDRASSI]

Thank you for reporting this. I have studied the issue and I posted my analysis to the stackoverflow forum : http://stackoverflow.com/a/26072218/707093

As quick summary, while this is indeed a mistake in the code, this is not a bug and it doesn't affect the security or the correctness of the implementation.

I'll commit a correction to this mistake in VeraCrypt.

Here is a my answer posted on stackoverflow :

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.