Closed
Bug 867795
Opened 8 years ago
Closed 8 years ago
Fix problems in the new code in libSSL found during code review
Categories
(NSS :: Libraries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.15
People
(Reporter: wtc, Assigned: wtc)
Details
Attachments
(5 files, 4 obsolete files)
1.36 KB,
patch
|
ryan.sleevi
:
review+
KaiE
:
superreview+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
1.30 KB,
patch
|
ryan.sleevi
:
review+
KaiE
:
superreview+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
2.63 KB,
patch
|
KaiE
:
review+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
3.17 KB,
patch
|
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
1006 bytes,
patch
|
wtc
:
review+
briansmith
:
checked-in+
|
Details | Diff | Splinter Review |
This bug report fixes the problems Ryan Sleevi and I found during code review of the new code in libSSL. The first patch improves the test for the existence of an OCSP response on the server side. In addition to testing the ss->certStatusArray pointer, we should also test that the SECItemArray it points to is non-empty.
Attachment #744309 -
Flags: superreview?(kaie)
Attachment #744309 -
Flags: review?(ryan.sleevi)
Updated•8 years ago
|
Attachment #744309 -
Flags: review?(ryan.sleevi) → review+
Assignee | ||
Comment 1•8 years ago
|
||
ekr: you just need to look at the last change in this patch (the XXX comment I added). I looked at ssl3_HandleHandshakeMessage because Ryan complained about the unbraced-else with braced-switch. When writing a patch to avoid that, I discovered and fixed some bugs. 1. If we are in the wait_certificate_status state but the received message is not certificate_status, we will call ssl3_AuthCertificate but fail to handle the received message. 2. If we received an unexpected certificate_status, we should also send a fatal unexpected_message alert. 3. In DTLS, we may fail to increment ss->ssl3.hs.recvMessageSeq if we received a certificate or certificate_request message and the ss->authCertificate() or ss->getClientAuthData() user callback returns SECWouldBlock.
Attachment #744316 -
Flags: superreview?(kaie)
Attachment #744316 -
Flags: review?(bsmith)
Attachment #744316 -
Flags: feedback?(ekr)
Assignee | ||
Comment 2•8 years ago
|
||
I attached the wrong patch. This is the correct patch.
Attachment #744316 -
Attachment is obsolete: true
Attachment #744316 -
Flags: superreview?(kaie)
Attachment #744316 -
Flags: review?(bsmith)
Attachment #744316 -
Flags: feedback?(ekr)
Attachment #744318 -
Flags: superreview?(kaie)
Attachment #744318 -
Flags: review?(bsmith)
Attachment #744318 -
Flags: feedback?(ekr)
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 744318 [details] [diff] [review] Fix bugs in ssl3_HandleHandshakeMessage (correct patch) Ryan, this patch ended up being more than fixing the unbraced-else with braced-switch, so I'm not sure if you want to review it.
Attachment #744318 -
Flags: review?(ryan.sleevi)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #744321 -
Flags: review?(ryan.sleevi)
Updated•8 years ago
|
Attachment #744321 -
Flags: review?(ryan.sleevi) → review+
Comment 5•8 years ago
|
||
Comment on attachment 744318 [details] [diff] [review] Fix bugs in ssl3_HandleHandshakeMessage (correct patch) Review of attachment 744318 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/ssl/ssl3con.c @@ +9672,5 @@ > > + /* XXX should this be rv != SECFailure? ssl3_HandleCertificate and > + * ssl3_HandleCertificateRequest may call a user callback, which may > + * return SECWouldBlock. > + */ Seems to be, since we should never block for any other reason (ssl3_HandleHandshakeMessage should only be called with a fully-gathered record)
Attachment #744318 -
Flags: review?(ryan.sleevi) → review+
Comment 6•8 years ago
|
||
Comment on attachment 744318 [details] [diff] [review] Fix bugs in ssl3_HandleHandshakeMessage (correct patch) Review of attachment 744318 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/ssl/ssl3con.c @@ +9671,5 @@ > } > > + /* XXX should this be rv != SECFailure? ssl3_HandleCertificate and > + * ssl3_HandleCertificateRequest may call a user callback, which may > + * return SECWouldBlock. I'm not sure. What happens when this functions complete? Do we reenter with the original message or move on to the next one? If the former, I think it's OK. If the latter, then probably we need to do something along these lines.
Comment 7•8 years ago
|
||
Comment on attachment 744318 [details] [diff] [review] Fix bugs in ssl3_HandleHandshakeMessage (correct patch) Review of attachment 744318 [details] [diff] [review]: ----------------------------------------------------------------- > If we are in the wait_certificate_status state but the received > message is not certificate_status, we will call ssl3_AuthCertificate > but fail to handle the received message. In other words, we did not correctly handle the case where the server negotiated the certificate status extension, but did not send the CertificateStatus message. ::: lib/ssl/ssl3con.c @@ +9558,5 @@ > rv = ssl3_AuthCertificate(ss); /* sets ss->ssl3.hs.ws */ > + > + if (ss->ssl3.hs.msg_type == certificate_status) { > + return rv; > + } Shouldn't this be if (ss->ssl3.hs.msg_type == certificate_status || rv == SECFailure)? Otherwise, if ssl3_AuthCertificate fails, then we will ignore the failure because we will replace the value of rv with the return value of a ssl3_Handle*** call. We should never call the ssl3_Handle*** call in the first place if certificate validation failed. @@ +9672,5 @@ > > + /* XXX should this be rv != SECFailure? ssl3_HandleCertificate and > + * ssl3_HandleCertificateRequest may call a user callback, which may > + * return SECWouldBlock. > + */ Why not just: if (IS_DTLS(ss)) { if rv == SECFailure, then the value of ss->ssl3.hs.recvMessageSeq no longer matters since the connection can no longer be used. This would also match the addition you made above.
Attachment #744318 -
Flags: review?(bsmith) → review-
Assignee | ||
Comment 8•8 years ago
|
||
In addition to fixing variable declaration formatting, I also fix two problems. 1. Pass ss->pkcs11PinArg instead of |arg| as the "void *pwArg" argument to CERT_CacheOCSPResponseFromSideChannel. 2. ss->sec.peerCert is the server's leaf cert, so it matches only &certStatusArray->items[i] for i = 0. I replaced the for loop with an if statement.
Attachment #744321 -
Attachment is obsolete: true
Attachment #744336 -
Flags: superreview?(kaie)
Attachment #744336 -
Flags: review?(ryan.sleevi)
Updated•8 years ago
|
Attachment #744336 -
Flags: review?(ryan.sleevi) → review+
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 744309 [details] [diff] [review] Also check ss->certStatusArray points to a non-empty SECItemArray. https://hg.mozilla.org/projects/nss/rev/4f4190cc5f9f
Attachment #744309 -
Flags: checked-in+
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 744336 [details] [diff] [review] Fix problems in SSL_AuthCertificate https://hg.mozilla.org/projects/nss/rev/44a46d334735
Attachment #744336 -
Flags: checked-in+
Updated•8 years ago
|
Attachment #744309 -
Flags: superreview?(kaie) → superreview+
Updated•8 years ago
|
Attachment #744318 -
Flags: superreview?(kaie)
Comment 11•8 years ago
|
||
Comment on attachment 744336 [details] [diff] [review] Fix problems in SSL_AuthCertificate >diff --git a/lib/ssl/sslauth.c b/lib/ssl/sslauth.c > >- for (i = 0; i < certStatusArray->len; ++i) { >+ if (certStatusArray->len) { > CERT_CacheOCSPResponseFromSideChannel(handle, ss->sec.peerCert, Well, ok. The old code somehow played the role of a reminder, but I agree this should be rather done correctly in bug 611836.
Attachment #744336 -
Flags: superreview?(kaie) → superreview+
Assignee | ||
Comment 12•8 years ago
|
||
https://hg.mozilla.org/projects/nss/rev/96330ae13a3f
Attachment #744867 -
Flags: review?(kaie)
Attachment #744867 -
Flags: checked-in+
Updated•8 years ago
|
Attachment #744867 -
Flags: review?(kaie) → review+
Assignee | ||
Comment 13•8 years ago
|
||
Brian: I made the fix you suggested. I will deal with the ss->ssl3.hs.recvMessageSeq increment at the end of ssl3_HandleHandshakeMessage in a separate bug.
Attachment #744318 -
Attachment is obsolete: true
Attachment #744318 -
Flags: feedback?(ekr)
Attachment #745307 -
Flags: superreview?(ryan.sleevi)
Attachment #745307 -
Flags: review?(bsmith)
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 745307 [details] [diff] [review] Fix bugs in ssl3_HandleHandshakeMessage, v2 Review of attachment 745307 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/ssl/ssl3con.c @@ +9558,5 @@ > rv = ssl3_AuthCertificate(ss); /* sets ss->ssl3.hs.ws */ > + > + if (rv == SECFailure || ss->ssl3.hs.msg_type == certificate_status) { > + return rv; > + } Brian: this ssl3_AuthCertificate() may return SECWouldBlock. We will proceed to handle the received handshake message in that case. I assume this is the correct behavior. If the ssl3_Handle<Foo> call returns SECSuccess, should ssl3_HandleHandshakeMessage return SECWouldBlock or SECSuccess? With this patch, ssl3_HandleHandshakeMessage will return SECSuccess.
Comment 15•8 years ago
|
||
(In reply to Wan-Teh Chang from comment #14) > ::: lib/ssl/ssl3con.c > @@ +9558,5 @@ > > rv = ssl3_AuthCertificate(ss); /* sets ss->ssl3.hs.ws */ > > + > > + if (rv == SECFailure || ss->ssl3.hs.msg_type == certificate_status) { > > + return rv; > > + } > > Brian: this ssl3_AuthCertificate() may return SECWouldBlock. We will proceed > to handle the received handshake message in that case. I assume this is the > correct behavior. ssl3_AuthCertificate will not return SECWouldBlock, AFAICT. If the auth certificate hook returns SECWouldBlock then ssl3_AuthCertificate will return SECSuccess: ... if (rv == SECWouldBlock) { if (ss->sec.isServer) { errCode = SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_SERVERS; rv = SECFailure; goto loser; } ss->ssl3.hs.authCertificatePending = PR_TRUE; rv = SECSuccess; ... > If the ssl3_Handle<Foo> call returns SECSuccess, should > ssl3_HandleHandshakeMessage return SECWouldBlock or SECSuccess? With this > patch, ssl3_HandleHandshakeMessage will return SECSuccess. ssl3_Handle<Foo> will return SECWouldBlock when needed. This will happen in ssl3_HandleServerHelloDone (indirectly via ssl3_SendClientSecondRound) and ssl3_HandleFinished. The idea is that a function should only return SECWouldBlock when it can't make progress. But, we can still make progress even when we are waiting for the certificate to be authenticated, up until the handshake is over; that is, we complete the handshake while cert authentication is pending but we won't send/receive application data until both the handshake is finished (or false start conditions are met) and the certificate has been successfully authenticated.
Comment 16•8 years ago
|
||
Comment on attachment 745307 [details] [diff] [review] Fix bugs in ssl3_HandleHandshakeMessage, v2 Review of attachment 745307 [details] [diff] [review]: ----------------------------------------------------------------- Did you find out if there are any ssl tests that test the case where the certificate authentication failed? If not, we should file a follow-up bug for adding some. ::: lib/ssl/ssl3con.c @@ +9558,5 @@ > rv = ssl3_AuthCertificate(ss); /* sets ss->ssl3.hs.ws */ > + > + if (rv == SECFailure || ss->ssl3.hs.msg_type == certificate_status) { > + return rv; > + } See my comment above. (I should have put my comment in splinter). I suggest that we change this to: if (rv != SECSuccess || ss->ssl3.hs.msg_type == certificate_status) { PORT_Assert(rv == SECFailure); /* Not SECWouldBlock */ return rv; } This should clear up some of the confusion.
Attachment #745307 -
Flags: review?(bsmith) → review+
Assignee | ||
Comment 17•8 years ago
|
||
Brian, thank you for answering my questions. I made the change you suggested and checked this in. I forgot to tell you that NSS test suite passed with my old patch. So most likely we don't have a test with both an untrusted server certificate and a missing certificate_status message. https://hg.mozilla.org/projects/nss/rev/76a4d93de110
Attachment #745307 -
Attachment is obsolete: true
Attachment #745307 -
Flags: superreview?(ryan.sleevi)
Attachment #745353 -
Flags: checked-in+
Assignee | ||
Updated•8 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 18•8 years ago
|
||
Wan-Teh, I made an obvious mistake in my suggestion to you:
> if (rv != SECSuccess || ss->ssl3.hs.msg_type == certificate_status) {
> PORT_Assert(rv == SECFailure); /* Not SECWouldBlock */
We forgot to account for the case where rv == SECSuccess && ss->ssl3.hs.msg_type == certificate_status.
See the attached patch.
Attachment #745472 -
Flags: review?(wtc)
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 745472 [details] [diff] [review] Fix assertion of return value of ssl3_AuthCertificate Review of attachment 745472 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc. Could you check this in? Thanks.
Attachment #745472 -
Flags: review?(wtc) → review+
Comment 20•8 years ago
|
||
Comment on attachment 745472 [details] [diff] [review] Fix assertion of return value of ssl3_AuthCertificate https://hg.mozilla.org/projects/nss/rev/4d21439eba16
Attachment #745472 -
Flags: checked-in+
You need to log in
before you can comment on or make changes to this bug.
Description
•