Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27254004:
Make SSL False Start work with asynchronous certificate validation (Closed)
|
Created:
7 years, 2 months ago by wtc Modified:
7 years ago Reviewers:
briansmith CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
Description[This CL was for code review only.]
Make SSL False Start work with asynchronous certificate validation
(SSL_AuthCertificateComplete).
Patch by Brian Smith, attachment 814807, dated 2013-10-09 02:25 PDT.
Upstream NSS bug: https://bugzilla.mozilla.org/show_bug.cgi?id=713933
Patch Set 1 #Patch Set 2 : Resolve merge conflict in ssl3_SendClientSecondRound #
Total comments: 50
Patch Set 3 : New patch from Brian Smith on 2013-10-24 13:52 PDT #
Total comments: 42
Patch Set 4 : New patch from Brian Smith on 2013-10-29 11:03 PDT #
Total comments: 2
Patch Set 5 : What was checked in to NSS upstream #
Total comments: 1
Messages
Total messages: 17 (0 generated)
Review comments on patch set 2: https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ssl.h File net/third_party/nss/ssl/ssl.h (left): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl.h:744: ** Set the callback on a particular socket that gets called when we finish Just wondering: did you delete "on a particular socket" because it's superfluous? https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ssl.h File net/third_party/nss/ssl/ssl.h (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl.h:747: ** Set the callback that gets called when the TLS handshake is complete. The Nit: perhaps "the TLS handshake" should be changed to "a TLS handshake is complete" to allow for renegotiations? https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl.h:749: ** before processing incoming application data. If the connection false started Is this also true for the server side? Also, this isn't accurate for renegotiation. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl.h:768: ** minimum requirement. 1. As I noted before, unless we document exactly what SSL_RecommendedCanFalseStart does, an application cannot use SSL_RecommendedCanFalseStart as the minimum requirement and perform additional checks in its custom callback. I agree that it is difficult to document the behavior of SSL_RecommendedCanFalseStart. Therefore, I think SSL_RecommendedCanFalseStart can only be used as the custom callback, rather than being called by the custom callback. 2. I think it is useful to document that NSS will require 80 bits of security, so that people know their custom callback only need to check the bits of security if they require an even higher level of security. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl.h:771: ** done. The SSL_ENABLE_FALSE_START option should be mentioned somewhere in this comment block. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl.h:779: /* This function sets *canFalseStart according to the NSS team's recommended Nit: remove "NSS team's". Perhaps you can rename this function back to SSL_DefaultCanFalseStart to avoid the issue of "who recommended these criteria". https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl.h:780: ** criteria for false start. This criteria may change from release to release Nit: "criteria" is plural. The singular form is "criterion". https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:7387: return SECSuccess; Nit: it seems better to handle the "no false start callback" case first because it has much fewer lines of code. But I see that the "have false start callback" case may also fall through here. So the SSL_TRC message on line 7383 needs to be updated to say "weak cipher suite or no false start callback ..." https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:10326: } If |target| is ssl3_SendClientSecondRound here, does that also mean certificate authentication won the race with peer's finished message? Or does that mean we canceled the race (when we set ss->ssl3.hs.restartTarget to ssl3_SendClientSecondRound)? https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:11639: PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss) ); Why do we need to call ssl3_HandleRecord while holding SSL3HandshakeLock? Did you discover that we are already calling ssl3_HandleRecord with SSL3HandshakeLock held? https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3gthr.c (left): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3gthr.c:378: ss->ssl3.hs.ws == wait_new_session_ticket) && Note that ss->ssl3.hs.ws == wait_finished is not part of the check. This means the original code will not set canFalseStart to true if we have already received the server's ChangeCipherSpec but not Finished. I am not sure whether this is intentional or an oversight. I suspect it doesn't matter. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3gthr.c (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3gthr.c:336: PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) ); Is this assertion necessary? We already checked this on line 281. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3gthr.c:367: cText.type == content_application_data); Can you explain this assertion with a comment? https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/sslsecur.c (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:104: ssl_ReleaseRecvBufLock(ss); Why do we only need to set ss->gs.recordLen to 0 for SSL 2.0? Can these three lines (lines 102-104) be moved into ssl_FinishHandshake? https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:128: ssl_FinishHandshake(sslSocket *ss) Should we move this function to sslcon.c, since it is SSL 2.0 only? https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:385: SSL_DBG(("%d: SSL[%d]: bad socket in SSL_DefaultCanFalseStart", SSL_DefaultCanFalseStart => SSL_RecommendedCanFalseStart https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:602: ssl_Get1stHandshakeLock(ss); Why does DoRecv() need to get the 1stHandshakeLock? https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:1273: if (ss->opt.enableFalseStart && Why do you check ss->opt.enableFalseStart? I guess ss->opt.enableFalseStart can be checked without getting the SSL3HandshakeLock, so you can check it first as an optimization? https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:1313: ssl_ReleaseSSL3HandshakeLock(ss); falseStart is set here but never used again. So lines 1307-1313 can all be deleted. Perhaps this code is incomplete? https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/sslsock.c (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsock.c:2466: ss->ssl3.hs.canFalseStart)) { I think we can remove the ss->version >= SSL_LIBRARY_VERSION_3_0 check because ss->ssl3.hs.canFalseStart can only be true if ss->version >= SSL_LIBRARY_VERSION_3_0.
I will attach the interdiff for my changes as a patch to the Bugzilla bug as I am in a rush today and I don't have time to figure out how to use this tool. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ssl.h File net/third_party/nss/ssl/ssl.h (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl.h:747: ** Set the callback that gets called when the TLS handshake is complete. The On 2013/10/17 00:10:53, wtc wrote: > > Nit: perhaps "the TLS handshake" should be changed to "a TLS handshake is > complete" to allow for renegotiations? Done. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl.h:749: ** before processing incoming application data. If the connection false started On 2013/10/17 00:10:53, wtc wrote: > > Is this also true for the server side? > > Also, this isn't accurate for renegotiation. I tried to make this clearer in the updated patch. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl.h:768: ** minimum requirement. On 2013/10/17 00:10:53, wtc wrote: > > 1. As I noted before, unless we document exactly what > SSL_RecommendedCanFalseStart does, an application cannot use > SSL_RecommendedCanFalseStart as the minimum requirement and perform additional > checks in its custom callback. > > I agree that it is difficult to document the behavior of > SSL_RecommendedCanFalseStart. Therefore, I think SSL_RecommendedCanFalseStart > can only be used as the custom callback, rather than being called by the custom > callback. > > 2. I think it is useful to document that NSS will require 80 bits of security, > so that people know their custom callback only need to check the bits of > security if they require an even higher level of security. I think we should definitely improve this, including even improving the criteria that SSL_RecommendedCanFalseStart actually uses. But, I think we should land this as-is now and make those improvements later, after we've agreed on exactly what we want to do. Many existing things are much less well-documented than this and we do just fine. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl.h:771: ** done. On 2013/10/17 00:10:53, wtc wrote: > > The SSL_ENABLE_FALSE_START option should be mentioned somewhere in this comment > block. Done. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl.h:779: /* This function sets *canFalseStart according to the NSS team's recommended On 2013/10/17 00:10:53, wtc wrote: > > Nit: remove "NSS team's". Perhaps you can rename this function back to > SSL_DefaultCanFalseStart to avoid the issue of "who recommended these criteria". Done. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl.h:780: ** criteria for false start. This criteria may change from release to release On 2013/10/17 00:10:53, wtc wrote: > > Nit: "criteria" is plural. The singular form is "criterion". Done. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:7387: return SECSuccess; On 2013/10/17 00:10:53, wtc wrote: > > Nit: it seems better to handle the "no false start callback" case first because > it has much fewer lines of code. > > But I see that the "have false start callback" case may also fall through here. > So the SSL_TRC message on line 7383 needs to be updated to say "weak cipher > suite or no false start callback ..." Done. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:10326: } On 2013/10/17 00:10:53, wtc wrote: > > If |target| is ssl3_SendClientSecondRound here, does that also mean certificate > authentication won the race with peer's finished message? > > Or does that mean we canceled the race (when we set ss->ssl3.hs.restartTarget to > ssl3_SendClientSecondRound)? if target == ssl3_SendClientSecondRound then that means we haven't sent our client key exchange message yet so there is no race since the server is waiting for us to send our client key exchange and other "client second round" stuff. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:11639: PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss) ); On 2013/10/17 00:10:53, wtc wrote: > > Why do we need to call ssl3_HandleRecord while holding SSL3HandshakeLock? > Did you discover that we are already calling ssl3_HandleRecord with > SSL3HandshakeLock held? ssl3_GatherCompleteHandshake is the function that calls ssl3_HandleRecord. In ssl3_GatherCompleteHandshake, we were acquiring and releasing the ssl_HaveSSL3HandshakeLock over and over again. Also, we were checking ss->ssl3.hs.ws != idle_handshake at the end of the do { } while loop without holding the SSL3HandshakeLock, which seemed wrong. So, I fixed that function by holding the SSL3HandshakeLock for the duration of that loop. The updates to this function are a consequence of the updates to ssl3_GatherCompleteHandshake. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3gthr.c (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3gthr.c:336: PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) ); On 2013/10/17 00:10:53, wtc wrote: > > Is this assertion necessary? We already checked this on line 281. Done. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3gthr.c:367: cText.type == content_application_data); On 2013/10/17 00:10:53, wtc wrote: > > Can you explain this assertion with a comment? Done. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/sslsecur.c (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:104: ssl_ReleaseRecvBufLock(ss); On 2013/10/17 00:10:53, wtc wrote: > > Why do we only need to set ss->gs.recordLen to 0 for SSL 2.0? recordLen is only used for SSL 2.0: unsigned int recordLen; /* ssl2 only */ I verified that that is the case. > Can these three lines (lines 102-104) be moved into ssl_FinishHandshake? They could, but I did not do that because we should keep the SSL-2.0-specific functionality separated so that it is clear what to remove when we remove SSL 2.0 support later. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:128: ssl_FinishHandshake(sslSocket *ss) On 2013/10/17 00:10:53, wtc wrote: > > Should we move this function to sslcon.c, since it is SSL 2.0 only? This is not SSL-2.0-only. This is the functionality that is common to both SSL 2.0 and SSL 3.0+. It is called by ssl3_FinishHandshake too. I think we should leave it in this file because it deals with fields of ss that the rest of the functions in this file deal with. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:385: SSL_DBG(("%d: SSL[%d]: bad socket in SSL_DefaultCanFalseStart", On 2013/10/17 00:10:53, wtc wrote: > > SSL_DefaultCanFalseStart => SSL_RecommendedCanFalseStart Done. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:602: ssl_Get1stHandshakeLock(ss); On 2013/10/17 00:10:53, wtc wrote: > > Why does DoRecv() need to get the 1stHandshakeLock? It calls ssl3_GatherAppDataRecord and ssl3_GatherAppDataRecord requires the 1stHandshakeLock to be held. Otherwise selfserv fails in the false start case in the sslauth.txt tests. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:1273: if (ss->opt.enableFalseStart && On 2013/10/17 00:10:53, wtc wrote: > > Why do you check ss->opt.enableFalseStart? > > I guess ss->opt.enableFalseStart can be checked without getting the > SSL3HandshakeLock, so you can check it first as an optimization? Yes https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:1313: ssl_ReleaseSSL3HandshakeLock(ss); On 2013/10/17 00:10:53, wtc wrote: > > falseStart is set here but never used again. So lines 1307-1313 can all be > deleted. Perhaps this code is incomplete? You are right that this complicated code can be deleted. I had originally not realized that this code can only be reached in the case that !ss->firstHsDone if we are false starting, so I had made things more complicated than necessary. I didn't simplify the code enough when I cleaned it up. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/sslsock.c (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsock.c:2466: ss->ssl3.hs.canFalseStart)) { On 2013/10/17 00:10:53, wtc wrote: > > I think we can remove the ss->version >= SSL_LIBRARY_VERSION_3_0 check because > ss->ssl3.hs.canFalseStart can only be true if ss->version >= > SSL_LIBRARY_VERSION_3_0. I don't know if we can rely on ss->ssl3.hs being initialized correctly for the SSL 2.0 case though, since ssl3_init was not called.
https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ssl.h File net/third_party/nss/ssl/ssl.h (left): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl.h:744: ** Set the callback on a particular socket that gets called when we finish On 2013/10/17 00:10:53, wtc wrote: > > Just wondering: did you delete "on a particular socket" because it's > superfluous? Yes
https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3gthr.c (left): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3gthr.c:378: ss->ssl3.hs.ws == wait_new_session_ticket) && On 2013/10/17 00:10:53, wtc wrote: > > Note that ss->ssl3.hs.ws == wait_finished is not part of the check. This means > the original code will not set canFalseStart to true if we have already received > the server's ChangeCipherSpec but not Finished. > > I am not sure whether this is intentional or an oversight. I suspect it doesn't > matter. The false start code, before this patch, was inconsistent in this respect. I agree that it doesn't matter.
Brian: thank you for your quick reply. You can just upload your new patch to Bugzilla. I will upload the new patch to this code review tool, and the tool will show interdiffs. High-level comments: The patch looks good except for the two locking issues. 1. We should fix the SSL3HandshakeLock issue in ssl3_GatherCompleteHandshake and ssl3_HandleRecord. 2. We should at least document the 1stHandshakeLock issue in DoRecv. I don't understand the issue, so I can't offer a suggested fix. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ssl.h File net/third_party/nss/ssl/ssl.h (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl.h:768: ** minimum requirement. On 2013/10/17 01:42:40, briansmith wrote: > But, I think we should land > this as-is now and make those improvements later, ... Agreed. > Many existing things are much less well-documented than this > and we do just fine. I was sharing my experience writing a custom callback. I checked the SSL_RecommendedCanFalseStart code to verify I didn't duplicate its checks. But you are right. The most similar default callback function is probably SSL_AuthCertificate, and its comment simply says "An implementation of the certificate authentication hook". https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:11639: PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss) ); On 2013/10/17 01:42:40, briansmith wrote: > > ssl3_GatherCompleteHandshake is the function that calls ssl3_HandleRecord. ... I fixed [ssl3_GatherCompleteHandshake] by > holding the SSL3HandshakeLock for the duration of that loop. The updates to this > function are a consequence of the updates to ssl3_GatherCompleteHandshake. I see. We should reconsider this change. This function makes no apparent use of ss->ssl3.hs (which I think is protected by the SSL3HandshakeLock) for about 350 lines of code until the very end. I will also discuss this issue in ssl3_GatherCompleteHandshake. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3gthr.c (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3gthr.c:323: rv = dtls_GatherData(ss, &ss->gs, flags); I agree we should be holding the SSL3HandshakeLock most of the time in this function, at least whenever it accesses ss->ssl3.hs. But when this function calls out to other functions, we may need to release the SSL3HandshakeLock. A good example is here. We need to read more data from the underlying TCP or UDP socket. If the socket is in blocking mode, ssl3_GatherData and dtls_GatherData may block indefinitely. It is bad to hold a lock for a long time. So, please try to release the lock before line 311 and re-acquire the lock before line 373. You may need to change to goto statements back to return statements in this block of code. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/sslsecur.c (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:602: ssl_Get1stHandshakeLock(ss); On 2013/10/17 01:42:40, briansmith wrote: > > It calls ssl3_GatherAppDataRecord and ssl3_GatherAppDataRecord requires the > 1stHandshakeLock to be held. Otherwise selfserv fails in the false start case in > the sslauth.txt tests. I'm sorry that I still don't understand this. Could you provide more detail, such as the call stack of the selfserv fails (I assume it's a lock assertion failure)? I traced ssl3_GatherAppDataRecord -> ssl3_GatherCompleteHandshake but couldn't find where they require the 1stHandshakeLock to be held. Perhaps you were referring to some SSL_XXX function called from the handshakeCallback? In any case, we should at least add a comment to explain why we need to get the 1stHandshakeLock and outline a possible solution. (Perhaps only call ssl_Get1stHandshakeLock if ss->firstHsDone is false?) https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:1280: (ss->handshake || ss->nextHandshake || ss->securityHandshake)) { Hmm... If we false started, ss->handshake etc. should all be NULL, so we don't need to check ss->ssl3.hs.canFalseStart, right? https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/sslsock.c (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsock.c:2466: ss->ssl3.hs.canFalseStart)) { On 2013/10/17 01:42:40, briansmith wrote: > > I don't know if we can rely on ss->ssl3.hs being initialized correctly for the > SSL 2.0 case though, since ssl3_init was not called. I see. I missed that.
https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/sslsecur.c (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:130: PORT_Assert( ss->opt.noLocks || ssl_Have1stHandshakeLock(ss) ); Wan-Teh, do you agree that this assertion is correct? That is, do you agree that we must be holding the 1stHandshakeLock when we update ss->firstHsDone? If so, then we must grab the 1stHandshakeLock before we call ssl3_HandleRecord. That in turn means we must be holding the 1stHandshakeLock before we call ssl3_GatherCompleteHandshake. That in turn means that we must be holding the lock before we call ssl3_GatherAppDataRecord. That means we must acquire the 1stHandshakeLock in DoRecv. Note that the 1stHandshakeLock must be acquired before acquiring the RecvBufLock, according to the comment for ssl_Get1stHandshakeLock in sslimpl.h This is why I acquire the 1stHanshakeLock in DoRecv. It seems I should add this here too: PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) );
https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/sslsecur.c (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/sslsecur.c:130: PORT_Assert( ss->opt.noLocks || ssl_Have1stHandshakeLock(ss) ); On 2013/10/17 19:09:01, briansmith wrote: > Wan-Teh, do you agree that this assertion is correct? That is, do you agree that > we must be holding the 1stHandshakeLock when we update ss->firstHsDone? This seems very reasonable, but I didn't find this documented in sslimpl.h. See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssli... So I don't really know :-) Also, this code checks ss->firstHsDone without holding the 1stHandshakeLock: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssls... > This is why I acquire the 1stHanshakeLock in DoRecv. Thank you very much for the explanation. > It seems I should add this here too: > PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) ); I did notice that we should get the RecvBufLock when modifying ss->gs.writeOffset and ss->gs.readOffset.
https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3gthr.c (left): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3gthr.c:378: ss->ssl3.hs.ws == wait_new_session_ticket) && On 2013/10/17 01:44:46, briansmith wrote: > On 2013/10/17 00:10:53, wtc wrote: > > > > Note that ss->ssl3.hs.ws == wait_finished is not part of > > the check. This means the original code will not set > > canFalseStart to true if we have already received the > > server's ChangeCipherSpec but not Finished. > > > > I am not sure whether this is intentional or an oversight. > > I suspect it doesn't matter. > > The false start code, before this patch, was inconsistent > in this respect. I agree that it doesn't matter. I found a reason why we should probably care about this. See the comment in sslimpl.h for ssl3_WaitingForStartOfServerSecondRound.
Brian: patch set 3 is the new patch you uploaded yesterday. Please go ahead and annotate the changes you made. Thanks.
I hope the new comments I added to the code also help clarify things. https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3gthr.c (right): https://codereview.chromium.org/27254004/diff/9001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3gthr.c:367: cText.type == content_application_data); On 2013/10/17 01:42:40, briansmith wrote: > On 2013/10/17 00:10:53, wtc wrote: > > > > Can you explain this assertion with a comment? > > Done. Making this clearer, and fixing the locking according to your feedback, motivated me to rearrange this code this way, to make it clearer. The conditions for ending the loop are the same as before; they are just organized in a clearer way. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3con.c:10373: ss->ssl3.hs.ws == wait_finished); This change is just putting the states in chronological order instead of alphabetical order, to make things a little clearer. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3con.c:10383: * and we want to process them all at the same time. This comment addition is redundant with the documentation comment for ssl3_WaitingForStartOfServerSecondRound and I forgot to remove it when I added the comment for ssl3_WaitingForStartOfServerSecondRound. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3con.c:11676: ssl_ReleaseSSL3HandshakeLock(ss); This function was reverted to the way it was before on your request. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/ssl3gthr.c (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3gthr.c:289: for (;;) { The locking in this function was changed based on your request, so that it does not hold the SSL3HandshakeLock during I/O. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3gthr.c:390: if (ss->ssl3.hs.ws == idle_handshake) { Note that ss->ssl3.hs.ws needs to have the SSl3HandshakeLock held when it is being accessed (AFAICT). Before this bug, ss->ssl3.hs.ws was being accessed without the lock being held. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3gthr.c:416: } I added this because we should not try to false start once we've received the NewSessionTicket or change_cipher_spec, as explained in ssl3_WaitingForStartOfServerSecondRound.
Review comments on patch set 3: This seems good. I will review it again tomorrow. It's a shame that we still hold the 1stHandshakeLock across the DoRecv call, but I don't have time to come up with a solution. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/ssl.h (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl.h:753: ** sent before the handshake callback is called. If the connection has not Nit: should we also change this "connection" to "handshake"? https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3con.c:7361: /* An attacker can control the selected ciphersuite so we only wish to Nit: you can still declare maybeFalseStart in this block because it is only used in this block. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3con.c:7608: PORT_Assert(ssl3_WaitingForStartOfServerSecondRound(ss)); Nit: this assertion will never fail. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3con.c:10382: * ChangeCipherSoec, and Finished mesage were sent in the same packet Nit: these three lines of comments should be reformatted. Line 10380 seems too long. Line 10381 is too short. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3con.c:12069: Delete this blank line. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/ssl3gthr.c (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3gthr.c:289: for (;;) { This loop should be while (keepGoing) { https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/sslimpl.h (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslimpl.h:1439: * the case that the NewSessionTicket, ChangeCipherSoec, and Finished mesage Typo: mesage => messages (two s'es and plural).
Patch set 3 LGTM. I went over the patch twice, although I was not as focused as I would like. I have some more comments. I will experiment with removing the !falseStart check from ssl_SecureSend, and adjusting the ssl_Get1stHandshakeLock in ssl_SecureRecv. You don't need to worry about these two issues unless I can demonstrate they actually work. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/ssl.h (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl.h:752: ** SSL_ENABLE_FALSE_START), then application data may already have already been Nit: "already" is used twice. Remove one of them. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3con.c:10379: * bother false starting. See the false-start-ws comment in Nit: "false-start-ws" should be clarified. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/ssl3gthr.c (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3gthr.c:377: * completing any renegotiation handshake we may be doing. Unless you have seen this case in practice, I think this optimization for an uncommon case is not worth the code complexity. On the other hand, this case is specifically allowed by the comment on lines 258-259. It also seems to be in the original code. So I guess you are just documenting an existing behavior? https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3gthr.c:408: * progress. IMPORTANT: Chromium won't call SSL_ForceHandshake again after SSL_ForceHandshake returns SECSuccess. Chromium will start calling PR_Send to send application data. So it doesn't seem necessary to mention SSL_ForceHandshake in this comment. Moreover, if doing this check at the beginning of the loop allows you to simplify the code, it may be worth doing. Otherwise, I don't think we need to mention this. It seems to bring up an unimportant issue. I can confirm we didn't deliberately put the check here. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/sslimpl.h (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslimpl.h:1438: * round then we don't botehr trying to false start, since it is almost always 1. Typo: botehr => bother 2. If the server sends the NewSessionTicket, ChangeCipherSoec, and Finished messages in the same packet, I think NSS will handle the three messages in one shot. So it seems that we will not try to false start in the middle of the server's second round. It seems that this function is only useful in the uncommon case that NSS cannot read the three messages in the server's second round in one shot. If you agree, then we should delete this function to simplify our code. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/sslsecur.c (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslsecur.c:350: void *client_data) Nit: client_data => arg https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslsecur.c:603: ssl_Get1stHandshakeLock(ss); Please add a comment to explain why we need to get 1stHandshakeLock. It is only needed for the false start case, when we need to finish the first handshake inside DoRecv(). https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslsecur.c:1211: rv = ssl_Do1stHandshake(ss); I think the key to avoiding getting 1stHandshakeLock for DoRecv is to add a version of ssl_Do1stHandshake that doesn't use the ss->handshake function pointer for the false start case. Unfortunately I don't have time to implement this. Another solution is to get 1stHandshakeLock for DoRecv only if !ss->firstHsDone. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslsecur.c:1280: if (!falseStart && As I mentioned before, I believe this !falseStart check is not necessary because the ss->handshake, ss->nextHandshake, ss->securityHandshake pointers should all be null in the false start case. Otherwise, ssl_SecureRecv would not finish the handshake inside DoRecv().
https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/ssl.h (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl.h:753: ** sent before the handshake callback is called. If the connection has not On 2013/10/29 01:21:51, wtc wrote: > Nit: should we also change this "connection" to "handshake"? I reworded it to avoid the issue. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3con.c:7361: /* An attacker can control the selected ciphersuite so we only wish to On 2013/10/29 01:21:51, wtc wrote: > > Nit: you can still declare maybeFalseStart in this block because it is only used in this block. Done. Also did the same for rv. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3con.c:7608: PORT_Assert(ssl3_WaitingForStartOfServerSecondRound(ss)); On 2013/10/29 01:21:51, wtc wrote: > > Nit: this assertion will never fail. Ideally no assertions will fail. This is mostly for documentation's sake and to sanity-check the implementation of the ssl3_WaitingForStartOfServerSecondRound function itself. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3con.c:10382: * ChangeCipherSoec, and Finished mesage were sent in the same packet On 2013/10/29 01:21:51, wtc wrote: > Nit: these three lines of comments should be reformatted. I removed this part of the comment since it is redundant with the documentation for ssl3_WaitingForStartOfServerSecondRound anyway. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3con.c:12069: On 2013/10/29 01:21:51, wtc wrote: > > Delete this blank line. Done. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/ssl3gthr.c (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3gthr.c:377: * completing any renegotiation handshake we may be doing. On 2013/10/29 19:54:22, wtc wrote: > > Unless you have seen this case in practice, I think this optimization for an > uncommon case is not worth the code complexity. > > On the other hand, this case is specifically allowed by the comment on lines > 258-259. It also seems to be in the original code. So I guess you are just > documenting an existing behavior? I am just documenting existing behavior. I don't want to change the behavior of this in this patch. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3gthr.c:408: * progress. On 2013/10/29 19:54:22, wtc wrote: > simplify the code, it may be worth doing. Otherwise, I don't think we > need to mention this. It seems to bring up an unimportant issue. I > can confirm we didn't deliberately put the check here. I am just trying to explain why, based on my explanation, things are the way they are (the way they were before my patches). If we want to change how this works, let's do that in another bug. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/sslimpl.h (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslimpl.h:1438: * round then we don't botehr trying to false start, since it is almost always On 2013/10/29 19:54:22, wtc wrote: > > 1. Typo: botehr => bother Done. > 2. If the server sends the NewSessionTicket, ChangeCipherSoec, and > Finished messages in the same packet, I think NSS will handle the three messages in one shot. So it seems that we will not try to false start in the middle of the server's second round. It seems that this function is only useful in the uncommon case that NSS cannot read > the three messages in the server's second round in one shot. The ChangeCipherSpec message is always in a record of type change_cipher_spec, whereas NewSessionTicket and Finished are always in a record of type handshake. So, these three messages can never appear together in one TLS record. The libssl record parser only parses one record at a time. So, I think this function is still useful. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslimpl.h:1439: * the case that the NewSessionTicket, ChangeCipherSoec, and Finished mesage On 2013/10/29 01:21:51, wtc wrote: > > Typo: mesage => messages (two s'es and plural). Done. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/sslsecur.c (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslsecur.c:350: void *client_data) On 2013/10/29 19:54:22, wtc wrote: > > Nit: client_data => arg Done. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslsecur.c:603: ssl_Get1stHandshakeLock(ss); On 2013/10/29 19:54:22, wtc wrote: > > Please add a comment to explain why we need to get 1stHandshakeLock. > It is only needed for the false start case, when we need to finish the > first handshake inside DoRecv(). Done. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslsecur.c:1211: rv = ssl_Do1stHandshake(ss); On 2013/10/29 19:54:22, wtc wrote: > Another solution is to get 1stHandshakeLock for DoRecv only if !ss->firstHsDone. The main question is whether we need to hold the lock when reading/writing firstHsDone. It seems like the current code isn't taking into consideration that memory barriers may be needed if firstHsDone is set on a thread different than the threads that are reading it. In general, I don't understand why it is so important to avoid holding 1stHandshakeLock. Also, I think the locking in libssl is unjustifiably and unnecessarily complicated and error-prone. Let's have a separate discussion about what to do about the locking stuff in libssl. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslsecur.c:1280: if (!falseStart && On 2013/10/29 19:54:22, wtc wrote: > > As I mentioned before, I believe this !falseStart check is > not necessary because the ss->handshake, ss->nextHandshake, > ss->securityHandshake pointers should all be null in the false > start case. Otherwise, ssl_SecureRecv would not finish the > handshake inside DoRecv(). With the current state of the code, I am almost 100% sure that the canFalseStart variable is not needed at all because it is redundant with ss->handshake/nextHandshake/securityHandshake like you mention. I noticed that last week, and I spent a little time experimenting with removing the canFalseStart variable. However, I would very much like to remove the SSL 2.0 code very soon, and when SSL 2.0 is removed, ss->handshake/nextHandshake/securityHandshake should be removed. Testing the canFalseStart variable is much clearer than testing ss->handshake. So, I would like to resolve the temporary redundancy during the removal of SSL 2.0.
https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/ssl3gthr.c (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3gthr.c:408: * progress. On 2013/10/29 21:00:13, briansmith wrote: > I am just trying to explain why, based on my explanation, things are the way > they are (the way they were before my patches). If we want to change how this > works, let's do that in another bug. I mean "based on my *investigation*." Here is the the comment as I will check it in: /* Prioritize sending application data over trying to complete * the handshake if we're false starting. * * If we were to do this check at the beginning of the loop instead * of here, then this function would become be a no-op after * receiving the ServerHelloDone in the false start case. By doing * this check here, we ensure that we always try to make at least a * little progress. (XXX: It isn't clear whether is is actually * necessary for us to "make at least a little progress.") */
Patch set 4 LGTM. Some of the minor comment changes you marked as "Done" are not in patch set 4. I assume you have made the changes in your work area. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/ssl3gthr.c (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3gthr.c:408: * progress. On 2013/10/29 21:11:38, briansmith wrote: > > * little progress. (XXX: It isn't clear whether is is actually > * necessary for us to "make at least a little progress.") is is => it is But I think you should remove the "XXX" from the comment. In the false start case, the handshake will be completed in ssl_SecureRecv (the DoRecv function), so it is necessary for this function to call ssl3_GatherData and ssl3_HandleRecord when ss->ssl3.hs.canFalseStart is true. So, it seems that we will still exit this loop after making a little progress in that case, but DoRecv -> ssl3_GatherAppDataRecord will immediately call this function again. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/sslsecur.c (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslsecur.c:1211: rv = ssl_Do1stHandshake(ss); On 2013/10/29 21:00:13, briansmith wrote: > > In general, I don't understand why it is so important to avoid holding > 1stHandshakeLock. Since the locking in libssl is not well documented, any new lock we acquire will set a precedent and will be extremely difficult to remove even if it's actually unnecessary to acquire the lock. This is why I don't want us to acquire 1stHandshakeLock when the first handshake is known to be finished. Isn't it bad to hold a lock across a long function such as DoRecv(), just so that we can check ss->firstHsDone? If we want to check ss->firstHsDone in a thread-safe manner, there must be a better solution. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslsecur.c:1280: if (!falseStart && On 2013/10/29 21:00:13, briansmith wrote: > > With the current state of the code, I am almost 100% sure that the canFalseStart > variable is not needed at all because it is redundant with > ss->handshake/nextHandshake/securityHandshake like you mention. Hmm... did you mean ss->ssl3.hs.canFalseStart is not needed because it is equivalent to the following condition? !ss->firstHsDone && !ss->handshake && !ss->nextHandshake && !ss->securityHandshake https://codereview.chromium.org/27254004/diff/163002/net/third_party/nss/ssl/... File net/third_party/nss/ssl/ssl.h (right): https://codereview.chromium.org/27254004/diff/163002/net/third_party/nss/ssl/... net/third_party/nss/ssl/ssl.h:752: ** SSL_ENABLE_FALSE_START), then application data may already have already been Nit: remember to remove one "already" from this line. https://codereview.chromium.org/27254004/diff/163002/net/third_party/nss/ssl/... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/27254004/diff/163002/net/third_party/nss/ssl/... net/third_party/nss/ssl/ssl3con.c:7608: PORT_Assert(ssl3_WaitingForStartOfServerSecondRound(ss)); I guess my comment was not clear. To someone familiar with ssl3_WaitingForStartOfServerSecondRound, the fact that this assertion will never fail is almost as obvious as: x = 1; PORT_Assert(IsEqualToOne(x)); In any case, this is just a nit.
https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/ssl3gthr.c (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/ssl3gthr.c:408: * progress. On 2013/10/30 01:01:20, wtc wrote: > > On 2013/10/29 21:11:38, briansmith wrote: > > > > * little progress. (XXX: It isn't clear whether is is actually > > * necessary for us to "make at least a little progress.") > > is is => it is > > But I think you should remove the "XXX" from the comment. In the false start > case, the handshake will be completed in ssl_SecureRecv (the DoRecv function), > so it is necessary for this function to call ssl3_GatherData and > ssl3_HandleRecord when ss->ssl3.hs.canFalseStart is true. So, it seems that we > will still exit this loop after making a little progress in that case, but > DoRecv -> ssl3_GatherAppDataRecord will immediately call this function again. Done. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... File net/third_party/nss/ssl/sslsecur.c (right): https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslsecur.c:1211: rv = ssl_Do1stHandshake(ss); On 2013/10/30 01:01:20, wtc wrote: > On 2013/10/29 21:00:13, briansmith wrote: > Since the locking in libssl is not well documented, any new lock we acquire will > set a precedent and will be extremely difficult to remove even if it's actually > unnecessary to acquire the lock. This is why I don't want us to acquire > 1stHandshakeLock when the first handshake is known to be finished. > > Isn't it bad to hold a lock across a long function such as DoRecv(), just so > that we can check ss->firstHsDone? If we want to check ss->firstHsDone in a > thread-safe manner, there must be a better solution. Let's discuss this separately and do the fix before NSS 3.15.3 RTM. I filed NSS bug 933686 for this. https://codereview.chromium.org/27254004/diff/33001/net/third_party/nss/ssl/s... net/third_party/nss/ssl/sslsecur.c:1280: if (!falseStart && On 2013/10/30 01:01:20, wtc wrote: > On 2013/10/29 21:00:13, briansmith wrote: > Hmm... did you mean ss->ssl3.hs.canFalseStart is not needed > because it is equivalent to the following condition? > > !ss->firstHsDone && !ss->handshake && !ss->nextHandshake && > !ss->securityHandshake && ssl3_WaitingForStartOfServerSecondRound(ss). Yes, I think that is correct.
Review comments on patch set 5: https://codereview.chromium.org/27254004/diff/273001/net/third_party/nss/ssl/... File net/third_party/nss/ssl/ssl3gthr.c (right): https://codereview.chromium.org/27254004/diff/273001/net/third_party/nss/ssl/... net/third_party/nss/ssl/ssl3gthr.c:382: } Brian: to match the original code better, it seems that this if statement (lines 374-382) should be moved after line 386, so that it also handles the result of the ssl3_HandleRecord call on line 325. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
