If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Handling of IV for AES-CBC is incorrect

RESOLVED FIXED in 3.4

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: Terry Hayes, Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

3.2.1
x86
Windows NT

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

17 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.
(Assignee)

Comment 1

17 years ago
Created attachment 38181 [details] [diff] [review]
proposed fix, also fixes leak of iv buffer, uses symbolic constants

Comment 2

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

Ian, could you please review Nelson's patch?
Assignee: wtc → nelsonb
(Assignee)

Comment 3

17 years ago
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)
(Assignee)

Comment 4

17 years ago
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.
Status: NEW → ASSIGNED

Comment 5

17 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) );
(Assignee)

Comment 6

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

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

Updated

17 years ago
Blocks: 55048

Comment 8

17 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
(Assignee)

Comment 9

17 years ago
The fix for this is now checked in on the trunk.
More testing is desirable, and will be done for bug 55048.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.