libSSL doesn't detect zero-length cert or CA names

RESOLVED FIXED in 3.9

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
15 years ago
14 years ago

People

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

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

The SSL and TLS specs forbid any certificate in the certificate message from 
being zero length.  They also forbid any CA name in the certificate request
message from being zero length.  libSSL does not check this explicitly.  
It passes the received length to PORT_ArenaAlloc, and tests the result for
NULL, apparently expecting that PORT_ArenaAlloc will return NULL if asked
for a zero size allocation. 

Also, it appears that ssl3_HandleCertificate makes an unnecessary allocation
and copy of each received DER certificate.  It calls PORT_ArenaAlloc to 
allocate space, then calls ssl3_ConsumeHandshake to copy the certificate 
into the allocated space, then calls CERT_NewTempCertificate which copies
the DER cert AGAIN, into the CERTCertificate's arena.  The allocation and
copy done in ssl3_HandleCertificate appears to be completely superfluous.
(Assignee)

Comment 1

15 years ago
Created attachment 122640 [details] [diff] [review]
patch v1, passes all.sh

This patch should reduce the number of allocations and memcpy calls 
on clients, and on servers that do client auth.  Do we have any stats for
those?
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 3.9

Comment 2

15 years ago
I only have numbers for non-client auth servers.  In that case, there are
currently about 1200 memcpys per SSL request (I don't have malloc numbers), but
a couple of other patches reduces that to about 990.  This fix would reduce it
by a few more in the client auth case, so every little bit helps ;)
(Assignee)

Comment 3

15 years ago
Comment on attachment 122640 [details] [diff] [review]
patch v1, passes all.sh

Ian and Wan-Teh, please confirm that your organization is OK with eliminating
the BSAFE-dependent version of freebl.
Attachment #122640 - Flags: superreview?(wtc)
Attachment #122640 - Flags: review?(ian.mcgreer)
(Assignee)

Comment 4

15 years ago
Comment on attachment 122640 [details] [diff] [review]
patch v1, passes all.sh

Sorry, I edited the wrong patch
Attachment #122640 - Flags: superreview?(wtc)
Attachment #122640 - Flags: review?(ian.mcgreer)

Comment 5

14 years ago
Comment on attachment 122640 [details] [diff] [review]
patch v1, passes all.sh

r=wtc.

There seems to be unecessary copying of the CA names, too.
Nelson will look at that.
Attachment #122640 - Flags: review+
(Assignee)

Comment 6

14 years ago
Patch v1 checked in.  I will work on eliminating the mem allocation and copies
for CA names next.  Patch forthcoming.
(Assignee)

Comment 7

14 years ago
Created attachment 132562 [details] [diff] [review]
supplemental patch
(Assignee)

Comment 8

14 years ago
Comment on attachment 132562 [details] [diff] [review]
supplemental patch

This patch eliminates unnecessasry copying of cert names.  It also improves on
the previous patch.  It detects a potential error situation that was left
undetected  by the previous patch, where the SSL message said it was longer
than it actually was.
Attachment #132562 - Flags: superreview?(wchang0222)

Comment 9

14 years ago
Comment on attachment 132562 [details] [diff] [review]
supplemental patch

r=wtc.

It seems that you should be able to do the equivalent of

+    if ((PRUint32)remaining > length)
+	goto alert_loser;

in the ssl3_ConsumeHandshakeNumber function, so that this
test is always performed.
Attachment #132562 - Flags: superreview?(wchang0222) → superreview+
(Assignee)

Comment 10

14 years ago
I checked in the above patch.  

/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.57; previous revision: 1.56

The suggestion in comment 9 that the check for numbers <= length
could be moved into ssl3_ConsumeHandshakeNumber would be correct only
if it is true that the number being consumed is a length of data the
follows in all cases.  However, that is not always the case.  For example,
sometimes that function is used to "consume" a number that is the protocol
version number, which has no relation to message/record length.  
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

Updated

14 years ago
Summary: libSSL doesn't detect zero-lenth cert or CA names → libSSL doesn't detect zero-length cert or CA names
You need to log in before you can comment on or make changes to this bug.