The encryption and decryption routines for AES-CBC do not update the iv value in the context. This means that data in the second and following calls will reuse the initial IV rather than using the last cipherblock from the previous call. NOTE: testing will work if only one call is made to the encryption routines. Interoperabillity will also work in this case. A better test is to perform encryption and decryption with multiple calls using different buffer sizes so that the IV chaining will break if it's not updated properly.
Created attachment 38181 [details] [diff] [review] proposed fix, also fixes leak of iv buffer, uses symbolic constants
Thanks, Nelson, for coming up with a patch in an hour. Ian, could you please review Nelson's patch?
The patch I attached to this bug also fixes several warnings about const, and about set-but-unused variables. It also makes some loops more efficient by removing if-the-else tests from them. Ian's CBC decrypt code is brilliant (should be quite fast), but it doesn't work properly if the input and output buffers overlap with the output buffer starting at a lower address than the input buffer. E.g. consider the case where out = in - 32 and length is 1024. I think there should be an assertion here that detects the condition that doesn't work. e.g. PORT_Assert( out - in >= 0 || in - out >= inputlen)
BTW, I should add that Ian and I discussed this CBC decrypt code before he checked it in. We agreed that it is OK for this code to require that the buffers not overlap in a certain fashion. I just think the code should have an assertion to detect this case.
I reviewed the patch. Thanks for taking the time to fix everything, Nelson. I agree that we should assert if the input and output buffers overlap. However, is that the right set of conditions? Specifically, I don't think (out - in >=0) is correct. That requires the output buffer to always follow the input buffer in memory, but couldn't the output buffer be before the input buffer (at a lower address) as long as they do not overlap? I think the assert should be: PORT_Assert( (out > in && out - in >= inputLen) || (in > out && in - out >= inputLen) );
The second half of my assert expression allows for the case where the output buffer comes entirely before the input buffer, does it not? Actually, I corrected it slightly by casting inputLen to int to force a signed comparison. PORT_Assert(output - input >= 0 || input - output >= (int)inputLen );
ugh. bad math on my part. you're right :)
This bug only needs to be fixed before the TLS/AES work (bug #55048) starts, so I am setting the target milestone to NSS 3.4.
The fix for this is now checked in on the trunk. More testing is desirable, and will be done for bug 55048.