Handling of IV for AES-CBC is incorrect



18 years ago
18 years ago


(Reporter: thayes0993, Assigned: nelson)


Windows NT

Firefox Tracking Flags

(Not tracked)



(1 attachment)



18 years ago
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

Comment 2

18 years ago
Thanks, Nelson, for coming up with a patch in an hour.

Ian, could you please review Nelson's patch?
Assignee: wtc → nelsonb
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.

Comment 5

18 years ago
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 );

Comment 7

18 years ago
ugh.  bad math on my part.  you're right :)


18 years ago
Blocks: 55048

Comment 8

18 years ago
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.
Priority: -- → P1
Target Milestone: --- → 3.4
The fix for this is now checked in on the trunk.
More testing is desirable, and will be done for bug 55048.
Last Resolved: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.