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

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Patch to check all the padding bytes

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.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

> 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 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 451098 [details] [diff] [review]
Patch to check all the padding bytes

r=nelson

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Checking in ssl3con.c; new revision: 1.140; previous revision: 1.139