85538 - Handling of IV for AES-CBC is incorrect

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Terry Hayes]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: proposed fix, also fixes leak of iv buffer, uses symbolic constants

Comment 2

[Wan-Teh Chang]

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

Ian, could you please review Nelson's patch?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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)

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

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

[Ian McGreer]

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) );

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

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

[Ian McGreer]

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

Comment 8

[Wan-Teh Chang]

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.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

The fix for this is now checked in on the trunk.
More testing is desirable, and will be done for bug 55048.