Closed Bug 571796 Opened 11 years ago Closed 11 years ago

ssl3_HandleRecord should check all the padding bytes, not just the first and last

Categories

(NSS :: Libraries, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.7

People

(Reporter: briansmith, Assigned: briansmith)

References

()

Details

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/533.4 (KHTML, like Gecko) Chrome/5.0.375.70 Safari/533.4
Build Identifier: 

ssl3_HandleRecord checks the first padding byte and the last padding byte but it does not check any of the bytes in the middle. AFAIK, NSS is the only TLS implementation that takes this shortcut. If there is a justification for the shortcut, then it should be clearly documented. Otherwise, it should check all the padding bytes. Especially, the shortcut seems worrisome given that TLS allows padding longer than the block size.

Reproducible: Always
Just remember that SSL 3.0 does not define the contents of the padding, 
so in SSL 3.0, the contents of the padding cannot be checked at all.
But for TLS, feel free to check all the padding bytes.  I would be good
to use an efficient checking algorithm, rather than simply checking each 
and every byte in succession, especially when the size of the padding 
exceeds (say) 2*sizeof(long).
Assignee: nobody → brian
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → trunk
It is very, very rare for a TLS implementation to add padding beyond the minimum necessary. I believe the way I wrote this patch is good for the extremely common (likely 99.9%) case where we received correct padding that is less than 15 bytes or less long.
Attachment #451098 - Flags: review?(nelson)
> It is very, very rare for a TLS implementation to add padding beyond the
> minimum necessary.

Agreed.  Let's be leaders!  Let's adopt a strategy to pad to bring the total
padded record size up to a multiple of (say) 128 bytes, so that all records 
are multiples of 128 bytes.  Strike a blow against traffic analysis!  
(In a later patch.  I'll review this one without that. :)
Comment on attachment 451098 [details] [diff] [review]
Patch to check all the padding bytes

r=nelson
Attachment #451098 - Flags: review?(nelson) → review+
Priority: -- → P2
Target Milestone: --- → 3.12.7
Checking in ssl3con.c; new revision: 1.140; previous revision: 1.139
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.