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.
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?
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 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 on attachment 122640 [details] [diff] [review] patch v1, passes all.sh Sorry, I edited the wrong patch
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.
Patch v1 checked in. I will work on eliminating the mem allocation and copies for CA names next. Patch forthcoming.
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 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.
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.