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

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: 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?

Comment 2

[Ian McGreer]

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

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

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

Sorry, I edited the wrong patch

Comment 5

[Wan-Teh Chang]

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.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Patch v1 checked in.  I will work on eliminating the mem allocation and copies
for CA names next.  Patch forthcoming.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: supplemental patch

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 9

[Wan-Teh Chang]

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.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

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.