|
|
Created:
8 years, 9 months ago by wtc Modified:
8 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, agl Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd DTLS support to NSS, contributed by Eric Rescorla.
R=rsleevi@chromium.org
BUG=120938
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129778
Patch Set 1 #
Total comments: 34
Patch Set 2 : Made one pass, dtls1con.c not reviewed yet #
Total comments: 18
Patch Set 3 : Made a second pass, reviewed dtls1con.c only for coding style #
Total comments: 48
Patch Set 4 : Made changes based on comments from EKR #
Total comments: 8
Patch Set 5 : Made changes suggested by rsleevi #
Total comments: 6
Patch Set 6 : Made the second round of changes suggested by rsleevi #
Total comments: 2
Patch Set 7 : Third round of changes suggested by ekr and rsleevi #Patch Set 8 : Remove an extra 'the' #
Total comments: 15
Patch Set 9 : Add the dtls.patch file #Patch Set 10 : Update AUTHORS #
Messages
Total messages: 35 (0 generated)
ekr: Patch set 1 is your original NSS patch. I have reviewed everything except dtls1con.c (not at all) and ssl3con.c (only cursively). I have some questions for you. https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... File net/third_party/nss/ssl/ssl3con.c (right): https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/ssl3con.c:2219: cipherBytes += ivBytes; This is a merging error. cipherBytes already includes the IV length. See line 2113 above. Your ivBytes variable is always 0. https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/ssl3con.c:2889: ss->sec.uncache(ss->sec.ci.sid); What is this change for? Did you run into a crash? https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/ssl3con.c:7683: are cases where you want to have no CAs offered. */ Are you sure this is good practice? https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/ssl3con.c:7948: if (rv != SECSuccess || (client_version != ss->clientHelloVersion)) { I think this rv != SECSuccess || was added incorrectly. At this point, rv must be equal to SECSuccess because of the test on line 7938 above. https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... File net/third_party/nss/ssl/sslimpl.h (right): https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslimpl.h:201: #define DTLS_FINISHED_TIMER 120000 /* Time to wait in FINISHED state */ DTLS_FINISHED_TIMER is larger than MAX_DTLS_TIMEOUT_MS. Will that be a problem? https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslimpl.h:201: #define DTLS_FINISHED_TIMER 120000 /* Time to wait in FINISHED state */ Nit: it would be nice to standardize on one of "TIMEOUT" and "TIMER". Also, include _MS in the macro's name. https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslimpl.h:896: DtlsTimerCb rtTimerCb; /* The timer to call on expiry */ Nit: this comment should read "The function to call on expiry", right? https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslimpl.h:899: PRUint32 rtRetries; /* The retry counter */ In the names of these members, "rt" stands for "retransmission", right? https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... File net/third_party/nss/ssl/sslproto.h (right): https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslproto.h:52: /* Note: this is the external format, not the wire format */ You meant "internal format" rather than "external format", right? https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslproto.h:59: #define SSL_LIBRARY_VERSION_DTLS_1_0_WIRE ((~0x0100) & 0xffff) Nit: would it be faster to do (0x0100 ^ 0xffff)? https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... File net/third_party/nss/ssl/sslsecur.c (right): https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslsecur.c:618: PORT_Assert(ss->gs.readOffset <= ss->gs.writeOffset); Did you mean to add this assertion, or did you add it for debugging and forget to remove it?
Thanks for the careful review and catching this stuff. My comments below. What's the best way to proceed? Should I generate a new patch that fixes these? Wait for the rest of your comments? https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... File net/third_party/nss/ssl/ssl3con.c (right): https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/ssl3con.c:2219: cipherBytes += ivBytes; On 2012/03/21 01:22:07, wtc wrote: > > This is a merging error. > > cipherBytes already includes the IV length. See line 2113 > above. Your ivBytes variable is always 0. Oops. This merge got ugly and I thought I caught all the errors, but I guess not. https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/ssl3con.c:2889: ss->sec.uncache(ss->sec.ci.sid); On 2012/03/21 01:22:07, wtc wrote: > > What is this change for? Did you run into a crash? IIRC I was seeing crashes when noCache was set. I have some vague memory that there is an existing bug on this, but I should have filed one. Sorry. https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/ssl3con.c:7683: are cases where you want to have no CAs offered. */ On 2012/03/21 01:22:07, wtc wrote: > > Are you sure this is good practice? So, it's explicitly permitted by RFC 5246, and it's how we want DTLS-SRTP to behave because the certs are self-signed, so there's no sensible value to put here. https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/ssl3con.c:7948: if (rv != SECSuccess || (client_version != ss->clientHelloVersion)) { On 2012/03/21 01:22:07, wtc wrote: > > I think this > rv != SECSuccess || > was added incorrectly. At this point, rv must be equal to > SECSuccess because of the test on line 7938 above. You're right. This was an error introduced when I changed the semantics of dtls_DTLSVersion...() so it could never fail and did not update this code. https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... File net/third_party/nss/ssl/sslimpl.h (right): https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslimpl.h:201: #define DTLS_FINISHED_TIMER 120000 /* Time to wait in FINISHED state */ On 2012/03/21 01:22:07, wtc wrote: > > DTLS_FINISHED_TIMER is larger than MAX_DTLS_TIMEOUT_MS. > Will that be a problem? No. MAX_DTLS_TIMEOUT_MS is the biggest timer we back-off to for retransmission, but this is a different case. Wonder if we should relable or comment these to make clear. Honestly, MAX_DTLS_TIMEOUT_MS is probably too big... https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslimpl.h:201: #define DTLS_FINISHED_TIMER 120000 /* Time to wait in FINISHED state */ On 2012/03/21 01:22:07, wtc wrote: > > Nit: it would be nice to standardize on one of "TIMEOUT" and > "TIMER". Also, include _MS in the macro's name. Good point. https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslimpl.h:896: DtlsTimerCb rtTimerCb; /* The timer to call on expiry */ On 2012/03/21 01:22:07, wtc wrote: > > Nit: this comment should read "The function to call on expiry", > right? Agreed. https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslimpl.h:899: PRUint32 rtRetries; /* The retry counter */ On 2012/03/21 01:22:07, wtc wrote: > > In the names of these members, "rt" stands for "retransmission", > right? Correct. https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... File net/third_party/nss/ssl/sslproto.h (right): https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslproto.h:52: /* Note: this is the external format, not the wire format */ On 2012/03/21 01:22:07, wtc wrote: > > You meant "internal format" rather than "external format", > right? Yes. My bad. https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslproto.h:59: #define SSL_LIBRARY_VERSION_DTLS_1_0_WIRE ((~0x0100) & 0xffff) On 2012/03/21 01:22:07, wtc wrote: > > Nit: would it be faster to do (0x0100 ^ 0xffff)? Probably, but shouldn't the compiler make this a constant anyway? https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... File net/third_party/nss/ssl/sslsecur.c (right): https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslsecur.c:618: PORT_Assert(ss->gs.readOffset <= ss->gs.writeOffset); On 2012/03/21 01:22:07, wtc wrote: > > Did you mean to add this assertion, or did you add it for > debugging and forget to remove it? I suspect that I added it for debugging and forgot to remove it, but OTOH, it seems like in fact it should be an invariant, right?
On 2012/03/21 01:36:40, ekr wrote: > What's the best way to proceed? Should I generate a new patch that fixes > these? Wait for the rest of your comments? I will take care of fixing these and generating a new patch. When I upload a new patch, I will ask you to review the diffs between your original patch and my patch. Rietveld makes it easy to do that. Then you can apply the diffs to your NSS git tree.
https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... File net/third_party/nss/ssl/ssl3con.c (right): https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/ssl3con.c:2889: ss->sec.uncache(ss->sec.ci.sid); On 2012/03/21 01:36:40, ekr wrote: > > IIRC I was seeing crashes when noCache was set. I have some vague memory that > there is an existing bug on this, but I should have filed one. Sorry. Thank you for the confirmation. We have fixed crashes like this before. I found an open bug about a variant of this crash: https://bugzilla.mozilla.org/show_bug.cgi?id=540535 Here are the closed bugs of variants of this crash: https://bugzilla.mozilla.org/show_bug.cgi?id=727691 https://bugzilla.mozilla.org/show_bug.cgi?id=237724 Your fix is similar to Nelson's fix for bug 237724 except that you don't set SSL_ERROR_SERVER_CACHE_NOT_CONFIGURED if ss->opt.noCache is false but the cache hasn't been configured. https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... File net/third_party/nss/ssl/sslproto.h (right): https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslproto.h:59: #define SSL_LIBRARY_VERSION_DTLS_1_0_WIRE ((~0x0100) & 0xffff) On 2012/03/21 01:36:40, ekr wrote: > > Probably, but shouldn't the compiler make this a constant anyway? Yes, you're right. I thought we may be performing this operation elsewhere on a variable, which is why I asked for a more efficient way to compute this. But I found that you do the DTLS-to-TLS version conversion using the wire version directly, so it is not necessary to compute one's complement of the wire version. https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... File net/third_party/nss/ssl/sslsecur.c (right): https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/sslsecur.c:618: PORT_Assert(ss->gs.readOffset <= ss->gs.writeOffset); On 2012/03/21 01:36:40, ekr wrote: > > I suspect that I added it for debugging and forgot to remove it, but OTOH, it > seems like in fact it should be an invariant, right? I am not familiar with this code, but I suspect this should be an invariant because the code above does available = ss->gs.writeOffset - ss->gs.readOffset; in two places and appears to assume available is >= 0. Is this the best place to check this invariant?
https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... File net/third_party/nss/ssl/ssl3con.c (right): https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/ssl3con.c:2889: ss->sec.uncache(ss->sec.ci.sid); I found that this crash was reported in the NSS newsgroup before: https://bugzilla.mozilla.org/show_bug.cgi?id=540535#c2
ekr: I uploaded patch set 2. You can click the underlined "1" links to review the diffs against patch set 2. Use the "k" and "j" keys to move between files. You will see that most of my changes are coding style fixes. I also undid unnecessary whitespace changes to make your patch as small as possible (to facilitate code review). Tomorrow I will go over ssl3con.c to convince myself that the changes are a no-op if DTLS is disabled.
OK. I'm in the air most of today but I'll try to find some wireless during some time when I'm sitting around an airport. Unless there's some way to do it offline? -Ekr On Tue, Mar 20, 2012 at 7:40 PM, <wtc@chromium.org> wrote: > ekr: I uploaded patch set 2. > > You can click the underlined "1" links to review the diffs > against patch set 2. Use the "k" and "j" keys to move > between files. > > You will see that most of my changes are coding style fixes. > I also undid unnecessary whitespace changes to make your > patch as small as possible (to facilitate code review). > > Tomorrow I will go over ssl3con.c to convince myself that > the changes are a no-op if DTLS is disabled. > > https://chromiumcodereview.appspot.com/9764001/
On 2012/03/21 13:55:53, ekr wrote: > OK. I'm in the air most of today but I'll try to find some wireless during > some time when I'm sitting around an airport. Unless there's some > way to do it offline? No, I don't think we can use the code review tool offline. The changes I have made are all minor, so you don't need to review them today. I will send you questions if necessary. Have a good flight!
ekr: I have some questions for you. If you are busy, just answer the first "IMPORTANT" question about the code that sets read and write sequence numbers. It seems that the code has been moved to ssl3_InitPendingCipherSpec. But I don't understand why that is safe. http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl.h File net/third_party/nss/ssl/ssl.h (right): http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl.... net/third_party/nss/ssl/ssl.h:955: SSL_IMPORT SECStatus DTLS_GetTimeout(PRFileDesc *socket, Nit: DTLS_GetTimeout => DTLS_GetHandshakeTimeout? http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... File net/third_party/nss/ssl/ssl3con.c (left): http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3con.c:2819: pwSpec->write_seq_num.low = 0; IMPORTANT: Where did this go? Same question for line 2881 below. http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3con.c:1797: IMPORTANT: I think we need to add if (rv != SECSuccess) { goto done; } here to skip the new code that sets the sequence numbers if something already failed. http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3con.c:1808: rv = SECFailure; IMPORTANT: Should we add 'goto done' here? http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3con.c:1812: * cwSpec at different times. Did you mean prSpec or cwSpec here? cwSpec is not pointing to a "pending" spec object. http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3con.c:5236: } Lines 5228-5230 and 5232-5236 have been removed by the SSL version range API patch. http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3con.c:9703: plaintext->len = 0; /* filled in by decode call below. */ I moved this block of code (lines 9693-9703) back to its original location. This seems to be a merging error. http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssls... File net/third_party/nss/ssl/sslsecur.c (right): http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssls... net/third_party/nss/ssl/sslsecur.c:618: PORT_Assert(ss->gs.readOffset <= ss->gs.writeOffset); Nit: should we assert the invariant before the return statement? Or is it only safe to asser the invariant when the function returns successfully? I can see here is a good place to assert because we just updated ss->gs.readOffset.
Just have a few minutes on the ground in Newark. Will get this either on the flight or when I get settled in europe do should be ready for you tomorrow am On Mar 21, 2012, at 19:12, wtc@chromium.org wrote: > ekr: I have some questions for you. If you are busy, just > answer the first "IMPORTANT" question about the code > that sets read and write sequence numbers. It seems that > the code has been moved to ssl3_InitPendingCipherSpec. > But I don't understand why that is safe. > > > http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl.h > File net/third_party/nss/ssl/ssl.h (right): > > http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl.... > net/third_party/nss/ssl/ssl.h:955: SSL_IMPORT SECStatus > DTLS_GetTimeout(PRFileDesc *socket, > > Nit: DTLS_GetTimeout => DTLS_GetHandshakeTimeout? > > http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... > File net/third_party/nss/ssl/ssl3con.c (left): > > http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... > net/third_party/nss/ssl/ssl3con.c:2819: pwSpec->write_seq_num.low = 0; > > IMPORTANT: Where did this go? Same question for line 2881 below. > > http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... > File net/third_party/nss/ssl/ssl3con.c (right): > > http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... > net/third_party/nss/ssl/ssl3con.c:1797: > > IMPORTANT: I think we need to add > if (rv != SECSuccess) { > goto done; > } > here to skip the new code that sets the sequence numbers > if something already failed. > > http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... > net/third_party/nss/ssl/ssl3con.c:1808: rv = SECFailure; > > IMPORTANT: Should we add 'goto done' here? > > http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... > net/third_party/nss/ssl/ssl3con.c:1812: * cwSpec at different times. > > Did you mean prSpec or cwSpec here? cwSpec is not pointing to > a "pending" spec object. > > http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... > net/third_party/nss/ssl/ssl3con.c:5236: } > > Lines 5228-5230 and 5232-5236 have been removed by the SSL > version range API patch. > > http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... > net/third_party/nss/ssl/ssl3con.c:9703: plaintext->len = 0; /* filled in > by decode call below. */ > > I moved this block of code (lines 9693-9703) back to its > original location. This seems to be a merging error. > > http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssls... > File net/third_party/nss/ssl/sslsecur.c (right): > > http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssls... > net/third_party/nss/ssl/sslsecur.c:618: PORT_Assert(ss->gs.readOffset <= > ss->gs.writeOffset); > > Nit: should we assert the invariant before the return > statement? Or is it only safe to asser the invariant when > the function returns successfully? > > I can see here is a good place to assert because we just > updated ss->gs.readOffset. > > http://codereview.chromium.org/9764001/
rsleevi: please review this CL. agl: just FYI. This is a big patch. I have reviewed the actual DTLS code. In your review, you only need to verify that the patch does no harm when DTLS is not used. 1. Assume IS_DTLS(ss) and isDTLS are false when checking the code used by both DTLS and non-DTLS. 2. Skip dtls1con.c because it is all DTLS code. 3. Review ssl3con.c last because it has many changes. I annotated the changes I am not sure about in this file. 4. ssl3_SendRecord has a new 'epoch' argument (the second argument). Verify that we pass a dummy epoch=0 in non-DTLS code. 5. ssl3_SendClientHello has a new 'resending' argument. Verify that we pass resending=PR_FALSE in non-DTLS code. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (left): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:7385: } Removing this check allows an SSL server to request a self-signed client certificate. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:1820: pwSpec->read_seq_num.low = pwSpec->write_seq_num.low = 0; rsleevi: this block of code is originally in - ssl3_SendChangeCipherSpecs: lines 2818-2819 on the left - ssl3_HandleChangeCipherSpecs: line 2881 on the left I am not sure about moving the code here. I've asked EKR for clarification. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:2859: ss->sec.uncache(ss->sec.ci.sid); rsleevi: this is a fix for the crash described in NSS bug https://bugzilla.mozilla.org/show_bug.cgi?id=540535#c2 http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:6495: ss->securityHandshake = NULL; rsleevi: I don't fully understand this change. I believe this change is related to the || IS_DTLS(ss) change in ssl_GatherRecord1stHandshake at sslcon.c:1257. Here I think EKR's comment is referring to the following code in sslgathr.c:ssl2_HandleV3HandshakeRecord: /* Clearing these handshake pointers ensures that * ssl_Do1stHandshake won't call ssl2_HandleMessage when we return. */ ss->nextHandshake = 0; ss->securityHandshake = 0; If necessary, we can put this code inside if (IS_DTLS(ss)) { } http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslcon.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslcon.c:1257: if ((ss->version >= SSL_LIBRARY_VERSION_3_0) || IS_DTLS(ss)) { rsleevi: see my comment at ssl3con.c:6495 about this change.
http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl.h File net/third_party/nss/ssl/ssl.h (right): http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl.... net/third_party/nss/ssl/ssl.h:955: SSL_IMPORT SECStatus DTLS_GetTimeout(PRFileDesc *socket, On 2012/03/21 23:12:16, wtc wrote: > > Nit: DTLS_GetTimeout => DTLS_GetHandshakeTimeout? That would be fine with me. http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... File net/third_party/nss/ssl/ssl3con.c (left): http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3con.c:2819: pwSpec->write_seq_num.low = 0; On 2012/03/21 23:12:16, wtc wrote: > > IMPORTANT: Where did this go? Same question for line 2881 below. I claim that I moved these to ssl_InitPendingSpec:1803-1826. You asked why this is safe. My argument here is that a cipher suite cannot be initialized without calling ssl_InitPendingCipherSpec() [the evidence of this is that that is the only call site for ssl3_DeriveConnectionKeysPKCS11(). And thus it doesn't matter whether we zero the values at the time when we create the cipher spec or when we set it to current. But I could have misunderstood this, so I would be happy to be double-checked. http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3con.c:1797: On 2012/03/21 23:12:16, wtc wrote: > > IMPORTANT: I think we need to add > if (rv != SECSuccess) { > goto done; > } > here to skip the new code that sets the sequence numbers > if something already failed. I concur. http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3con.c:1808: rv = SECFailure; On 2012/03/21 23:12:16, wtc wrote: > > IMPORTANT: Should we add 'goto done' here? Yes, I agree. http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3con.c:1812: * cwSpec at different times. On 2012/03/21 23:12:16, wtc wrote: > > Did you mean prSpec or cwSpec here? cwSpec is not pointing to > a "pending" spec object. I assume we're just talking about the comment here? If so, I agree that it's misleading and it should say prSpec. http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3con.c:9703: plaintext->len = 0; /* filled in by decode call below. */ On 2012/03/21 23:12:16, wtc wrote: > > I moved this block of code (lines 9693-9703) back to its > original location. This seems to be a merging error. It may be an error but it was intentional. :) The issue is that there are a number of conditions below in which packets get rejected but we return SECSuccess. We want to make sure that there is no way for the caller to end up with data in the plaintext buffers that hasn't been integrity checked. One would hope that databuf is already empty at this point, but moving this assignment up makes sure. Can you sugest a better approach? http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssls... File net/third_party/nss/ssl/sslsecur.c (right): http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssls... net/third_party/nss/ssl/sslsecur.c:618: PORT_Assert(ss->gs.readOffset <= ss->gs.writeOffset); On 2012/03/21 23:12:16, wtc wrote: > > Nit: should we assert the invariant before the return > statement? Or is it only safe to asser the invariant when > the function returns successfully? > > I can see here is a good place to assert because we just > updated ss->gs.readOffset. I'm not sure about the first question, but your last statement seems plausible.
ekr: thank you for answering my questions! http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... File net/third_party/nss/ssl/ssl3con.c (left): http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3con.c:2819: pwSpec->write_seq_num.low = 0; On 2012/03/22 15:07:17, ekr wrote: > > I claim that I moved these to ssl_InitPendingSpec:1803-1826. Thank you for confirming this. > You asked why this is safe. My argument here is that a cipher suite cannot be > initialized without calling ssl_InitPendingCipherSpec() [the evidence of this is > that that is the only call site for ssl3_DeriveConnectionKeysPKCS11(). And thus > it doesn't matter whether we zero the values at the time when we create the > cipher spec or when we set it to current. But I could have misunderstood this, > so I would be happy to be double-checked. I studied the code to understand how read_seq_num and write_seq_num are set and used. I came to the same conclusion that it is fine to zero them in ssl3_InitPendingCipherSpec(). Your proof using the ssl3_DeriveConnectionKeysPKCS11() call is also very convincing. Thanks. http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3con.c:1812: * cwSpec at different times. Yes, I was just asking about the comment here. I decided to remove the comment because this function already asserts that property at line 1762 above. PORT_Assert(ss->ssl3.prSpec == ss->ssl3.pwSpec); http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3con.c:9703: plaintext->len = 0; /* filled in by decode call below. */ On 2012/03/22 15:07:17, ekr wrote: > > The issue is that there are a number of conditions below in which packets get > rejected but we return SECSuccess. We want to make sure that there is no way for > the caller to end up with data in the plaintext buffers that hasn't been > integrity checked. One would hope that databuf is already empty at this point, > but moving this assignment up makes sure. Can you sugest a better approach? I see. I suggest that we set databuf->len to 0 before returning SECSuccess, like you do on lines 9867-9869 below. Note: there we should also set databuf->len instead of plaintext->len to 0 because plaintext points to temp_buf if we need to decompress.
rsleevi: I have done enough checking to convince myself that this patch is safe if DTLS is disabled. ekr: I have three more questions for you. http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:1257: dtls_InitRecvdRecords(&pwSpec->recvdRecords); ekr: can we also move this dtls_InitRecvdRecords(&pwSpec->recvdRecords); call to ssl3_InitPendingCipherSpec, next to where it sets pwSpec->epoch? http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:6497: } ekr: I put this code inside IS_DTLS(ss) to reduce risk. http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslsock.c (right): http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslsock.c:204: SSL_LIBRARY_VERSION_TLS_1_1 ekr: should we use SSL_LIBRARY_VERSION_DTLS_1_0 instead? Or do you want to emphasize that this is the base TLS protocol version?
http://codereview.chromium.org/9764001/diff/19001/AUTHORS File AUTHORS (right): http://codereview.chromium.org/9764001/diff/19001/AUTHORS#newcode174 AUTHORS:174: Eric Rescorla <ekr@rtfm.com> Verified the CLA. Looks good. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/der... File net/third_party/nss/ssl/derive.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/der... net/third_party/nss/ssl/derive.c:586: /* TODO: Add a SSL_CBP_TLS1_1 mask here */ Should this be XXX, per NSS style? I think the TODOs have been introduced by me or agl. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (left): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:7385: } On 2012/03/22 01:11:42, wtc wrote: > > Removing this check allows an SSL server to request a > self-signed client certificate. I'm not sure I agree with this explanation, but I'm fine with its removal. Just curious why it was done in the DTLS patch, since the DTLS code doesn't support server mode (yet?) http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:45: /* TODO(ekr): Implement HelloVerifyRequest on server side. OK for now. */ XXX instead (if upstreaming is intended) http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:1223: (suite_def->bulk_cipher_alg != cipher_rc4_56)); My understanding is that PORT_Assert is intended to catch implementation bugs / developer bugs, not possible (but unlikely) runtime conditions. It seems that this should either be a static check, causing a SECFailure return (making sure to release the lock as on 1215 / 1259). Since RC4 is MUST NOT, seems to me it should be an always-on check. The other thing to consider is whether or not the "Is bulk cipher allowed in DTLS" logic should be moved elsewhere. Yes, I realize RC4 is the only one with issues ( http://www.iana.org/assignments/tls-parameters/tls-parameters.xml ), so it may be YAGNI, but since there are security considerations, it seems like such logic should be centralized - either in a function or as a flag on the bulk cipher alg. Minor FWIW. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:1802: if (cwSpec->epoch == 65535) { nit: PR_UINT16_MAX ? http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:2329: type, pIn, 1, wrBuf); nit: tabs & spaces inconsistently mixed. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:3558: ss->ssl3.hs.sendMessageSeq++; I admit, I haven't fully analyzed this code, but when I see a naked increment like this, I wonder - where is the code that checks for over/underflow, if necessary? IIRC, this resets every CCS message, but it was just a thought. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:6495: ss->securityHandshake = NULL; style nit: use 0 rather than NULL, for consistency with other NSS code that clears these values LOCK BUG/SAFETY? I notice that, under the other times nextHandshake / securityHandshake are modified, ssl_Get1stHandshakeLock is held (and prior to the SSL3HandshakeLock, if that is also held). See SSL_ResetHandshake for the most reduced case, or see sslimpl.h which notes it as firstHandshakeLock guarded. Considering that the SSL3HandshakeLock is acquired much higher in the code (ssl3_HandleHandshakeMessge only requires SSL3HandshakeLock and is called by ssl3_HandleHandshake. ssl3_HandleHandshake only requires SSL3HandshakeLock be held, and is ultimately called by ssl3_HandleRecord), I don't see any place where you can reasonably guarantee/acquire firstHandshakeLock when this code executes. Have I missed some path? Is this lock important (I think it is, if only for general safety). I loved over line 9964, so I realize that ssl3_HandleHandshake is bypassed here, but I'm still concerned about the lock ordering. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:7900: } nit: Inconsistent tabbing within this (compared to 7805-7814). Then again, NSS tabbing is a mess to begin with. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:9306: (ss->ssl3.hs.msg_type != hello_verify_request)) { Should there also be a check for && (!IS_DTLS(ss) || ss->ssl3.hs.msg_type != client_hello)) Or is that something to omit until/if a server implementation is done (Just looking at 4347 4.2.6 and the list of messages excluded) http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:9326: rv = ssl3_UpdateHandshakeHashes(ss, (unsigned char*) dtlsData, nit: delete the space in "(unsigned char*) dtlsData" http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:9698: if (dtls_RecordGetRecvd(&crSpec->recvdRecords, dtls_seq_num)) { nit: While totally valid, I wonder if it makes more sense / self-documenting code to explicitly check != 0. This makes it clear that both -1 and 1 are handled cases. I had to look up what this methods return values were and why this check was OK, where I think the != 0 may have been clearer that the only 'acceptable' result IS zero (not received yet). Minor though, I defer to wtc here. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3gthr.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3gthr.c:186: * This loop returns when either (a) an error or EOF occurs, newline before (a) ? http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3gthr.c:209: if (gs->dtlsPacket.space < MAX_FRAGMENT_LENGTH + 2048 + 5) { What's the magic 2048 + 5? Should that be documented in the comment on line 208? http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3gthr.c:240: if ((gs->dtlsPacket.len - gs->dtlsPacketOffset) < 13) { nit: Use DTLS_RECORD_HEADER_LENGTH instead of 13 throughout here, for cleanliness. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3gthr.c:342: if (IS_DTLS(ss)) { Your normal ordering has been !IS_DTLS then handle DTLS in the else when there is an SSL-or-DTLS block, and IS_DTLS when there's just a DTLS block. Swap the order? http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3gthr.c:350: ssl_GetSSL3HandshakeLock(ss); XXX - Check the layering of locks (line 308, line 350) to ensure there is no deadlock opportunity. I thoguht Handshake came before Recv. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslgathr.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslgathr.c:437: gs->dtlsPacketOffset = 0; What about gs->dtlsPacket.len ? http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslsock.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslsock.c:294: if (os->protocolVariant != ssl_variant_stream) { !IS_DTLS(os) ? http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:6497: } On 2012/03/22 21:59:54, wtc wrote: > > ekr: I put this code inside IS_DTLS(ss) to reduce risk. See my note about the whole firstHandshakeLock needing to be held here. http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslsecur.c (right): http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslsecur.c:248: ss->handshaking = sslHandshakingAsClient; Does this not need to be updated for IS_DTLS(ss) ?
rsleevi: thank you for the review! Please review patch set 5. I made all the changes you suggested, except - TODO => XXX - the PORT_Assert about RC4 I will deal with them later. (I think using TODO is fine.) http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (left): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:7385: } On 2012/03/22 22:26:37, Ryan Sleevi wrote: > > Just curious why it was done in the DTLS patch, since the DTLS code doesn't > support server mode (yet?) DTLS server mode works. Only HelloVerifyRequest isn't implemented. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:3558: ss->ssl3.hs.sendMessageSeq++; On 2012/03/22 22:26:37, Ryan Sleevi wrote: > > IIRC, this resets every CCS message, but it was just a thought. This should be reset at the beginning of each handshake, and is incremented for each handshake message. A retransmitted handshake message uses the same message sequence number. So with the current DTLS handshake protocol ss->ssl3.hs.sendMessageSeq won't be larger than 20. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:6495: ss->securityHandshake = NULL; The complete lock rank is documented in ssl/notes.txt. I reproduce it here: recvLock ->\ firstHandshake -> recvbuf -> ssl3Handshake -> xmitbuf -> "spec" sendLock ->/ Since this function asserts recvbuf and ssl3Handshake locks are held, this function cannot acquire firstHandshake lock. I believe firstHandshake lock is already held when we get here. See SSL_ForceHandshake (called by Chrome) and ssl_SecureRecv and ssl_SecureSend in sslsecur.c. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:7900: } On 2012/03/22 22:26:37, Ryan Sleevi wrote: > nit: Inconsistent tabbing within this (compared to 7805-7814). The new code uses tabs. I think Rietveld doesn't show the tabs except to contrast with the original code. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:9306: (ss->ssl3.hs.msg_type != hello_verify_request)) { On 2012/03/22 22:26:37, Ryan Sleevi wrote: > Should there also be a check for > > && (!IS_DTLS(ss) || ss->ssl3.hs.msg_type != client_hello)) I guess you're asking about excluding the initial ClientHello. I had the same question, and EKR pointed out that it is excluded because each call to ssl3_SendClientHello restarts the handshake hashes, and on the server side that is done on lines 9295-9299 above. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:9326: rv = ssl3_UpdateHandshakeHashes(ss, (unsigned char*) dtlsData, On 2012/03/22 22:26:37, Ryan Sleevi wrote: > nit: delete the space in "(unsigned char*) dtlsData" I suspect that space was copied from line 9307 above. Actually the type cast is not necessary because PRUint8 is unsigned char. (dtlsData is an array of PRUint8s.) http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3gthr.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3gthr.c:209: if (gs->dtlsPacket.space < MAX_FRAGMENT_LENGTH + 2048 + 5) { On 2012/03/22 22:26:37, Ryan Sleevi wrote: > What's the magic 2048 + 5? Should that be documented in the comment on line 208? The magic constant is copied from ssl3_GatherData on lines 134-137. I will also copy the comment here and note that 5 should be changed to 13 (the size of the record header). http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3gthr.c:240: if ((gs->dtlsPacket.len - gs->dtlsPacketOffset) < 13) { On 2012/03/22 22:26:37, Ryan Sleevi wrote: > > Use DTLS_RECORD_HEADER_LENGTH instead of 13 throughout here, for cleanliness. This will make line 253 hard to read gs->remainder = (gs->hdr[11] << 8) | gs->hdr[12]; because 11 and 12 will need to become DTLS_RECORD_HEADER_LENGTH - 2 and DTLS_RECORD_HEADER_LENGTH - 1. Not sure if that's the right trade-off. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3gthr.c:342: if (IS_DTLS(ss)) { On 2012/03/22 22:26:37, Ryan Sleevi wrote: > > Swap the order? I actually prefer if (IS_DTLS(ss)) { .... } else { .... } What's the general recommendation when the two blocks are lopsided? Do the simple block first? http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3gthr.c:350: ssl_GetSSL3HandshakeLock(ss); On 2012/03/22 22:26:37, Ryan Sleevi wrote: > > I thoguht Handshake came before Recv. The lock order is recvbuf -> ssl3Handshake, so this is fine. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslgathr.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslgathr.c:437: gs->dtlsPacketOffset = 0; On 2012/03/22 22:26:37, Ryan Sleevi wrote: > What about gs->dtlsPacket.len ? I don't fully understand this code, so I am proving by analogy. dtls_GatherData is analogous to ssl3_GatherData. dtls_GatherData operates on gs->dtlsPacket. ssl3_GatherData operates on gs->inbuf. Since this function does not set gs->inbuf.len to 0, I concluded that it doesn't need to set gs->dtlsPacket.len to 0 either. See also lines 448-449 below, where ssl_DestroyGather frees gs->inbuf.buf and gs->dtlsPacket.buf at the same time. http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslsecur.c (right): http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslsecur.c:248: ss->handshaking = sslHandshakingAsClient; On 2012/03/22 22:26:37, Ryan Sleevi wrote: > Does this not need to be updated for IS_DTLS(ss) ? I don't know. This may be why EKR needs to add the IS_DTLS(ss) special handling code to ssl_GatherRecord1stHandshake and ssl3_HandleClientHello.
Thanks for the changes, wtc. I'm still not entirely convinced about the dtlsPacket.len, but it's quite possible I'm missing something. See my replies below. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3gthr.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3gthr.c:342: if (IS_DTLS(ss)) { On 2012/03/23 00:21:18, wtc wrote: > > On 2012/03/22 22:26:37, Ryan Sleevi wrote: > > > > Swap the order? > > I actually prefer > if (IS_DTLS(ss)) { > .... > } else { > .... > } > > What's the general recommendation when the two blocks > are lopsided? Do the simple block first? I had assumed so - since often the 'simple block' is the error handling. Here, my thought was more about 'common' block - !IS_DTLS() is how I think about the code when reading (eg: I want to see the TLS implementation first). But that's just my implementation bias, being more familiar with TLS rather than DTLS :) http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslgathr.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslgathr.c:437: gs->dtlsPacketOffset = 0; On 2012/03/23 00:21:18, wtc wrote: > > On 2012/03/22 22:26:37, Ryan Sleevi wrote: > > What about gs->dtlsPacket.len ? > > I don't fully understand this code, so I am proving by > analogy. > > dtls_GatherData is analogous to ssl3_GatherData. > > dtls_GatherData operates on gs->dtlsPacket. > ssl3_GatherData operates on gs->inbuf. > > Since this function does not set gs->inbuf.len to 0, > I concluded that it doesn't need to set gs->dtlsPacket.len > to 0 either. > > See also lines 448-449 below, where ssl_DestroyGather frees > gs->inbuf.buf and gs->dtlsPacket.buf at the same time. Yes, but neither PORT_Free resets gs->inbuf.len / gs->dtlsPacket.len. My concern was that I wasn't seeing where gs->dtlsPacketOffset.len was reset. If you look in dtls_GatherData (which is where the GS_INIT state leads to), it only sets ->dtlsPacket.len /after/ it checked that dtlsPacketOffset (reset here) equals gs->dtlsPacket.len. Note that for GS_INIT, ssl3gthr.c:82 (ssl3_GatherData) resets gs->inbuf.len to 0 for GS_INIT, but I see no such equivalent code in ssl3gthr.c:192 (dtls_GatherData). That's why I have trouble understanding where gs->dtlsPacket.len is properly zero initialized.
ekr: one more question for you. http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:10080: ss->ssl3.hs.sendMessageSeq = 0; ekr: RFC 6347 (DTLS 1.2) Section 4.2.2 Handshake Message Format says: The first message each side transmits in each handshake always has message_seq = 0. Is that done here? Do you know if NSS calls this function before starting a renegotiation? I am worried about the if (ss->ssl3.initialized) return SECSuccess; /* Function should be idempotent */ early return at the beginning of this function.
Thanks so much for the thorough review. It's really great to have people do such a great job. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/der... File net/third_party/nss/ssl/derive.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/der... net/third_party/nss/ssl/derive.c:586: /* TODO: Add a SSL_CBP_TLS1_1 mask here */ On 2012/03/22 22:26:37, Ryan Sleevi wrote: > Should this be XXX, per NSS style? > > I think the TODOs have been introduced by me or agl. I'm the one who added this TODO, but I'll leave this to the people who understand the style better. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (left): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:7385: } On 2012/03/23 00:21:18, wtc wrote: > > On 2012/03/22 22:26:37, Ryan Sleevi wrote: > > > > Just curious why it was done in the DTLS patch, since the DTLS code doesn't > > support server mode (yet?) > > DTLS server mode works. Only HelloVerifyRequest isn't > implemented. Correct, and this is optional. Let me give you a more concrete explanation for why I removed it. A primary reason I am doing this work is to enable DTLS-SRTP (RFC 5764) which uses self-signed certs and therefore has no sensible thing to configure here. I would be more than happy to have an extra option required to disable this check if it makes people nervous. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:1223: (suite_def->bulk_cipher_alg != cipher_rc4_56)); On 2012/03/22 22:26:37, Ryan Sleevi wrote: > My understanding is that PORT_Assert is intended to catch implementation bugs / > developer bugs, not possible (but unlikely) runtime conditions. > > It seems that this should either be a static check, causing a SECFailure return > (making sure to release the lock as on 1215 / 1259). Since RC4 is MUST NOT, > seems to me it should be an always-on check. > > The other thing to consider is whether or not the "Is bulk cipher allowed in > DTLS" logic should be moved elsewhere. Yes, I realize RC4 is the only one with > issues ( http://www.iana.org/assignments/tls-parameters/tls-parameters.xml ), so > it may be YAGNI, but since there are security considerations, it seems like such > logic should be centralized - either in a function or as a flag on the bulk > cipher alg. Minor FWIW. There is a separate place where this is allegedly enforced: ssl3_DisableNonDTLSSuites(sslSocket * ss). However, when we put this in I got nervous that a coding error could result in us missing an invalid cipher. E.g., we added one but forgot to update nonDTLSCipherSuites. The purpose of this check was to catch such errors. Does that seem bad? http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:9698: if (dtls_RecordGetRecvd(&crSpec->recvdRecords, dtls_seq_num)) { On 2012/03/22 22:26:37, Ryan Sleevi wrote: > nit: While totally valid, I wonder if it makes more sense / self-documenting > code to explicitly check != 0. This makes it clear that both -1 and 1 are > handled cases. I had to look up what this methods return values were and why > this check was OK, where I think the != 0 may have been clearer that the only > 'acceptable' result IS zero (not received yet). > > Minor though, I defer to wtc here. That would be fine with me as well. I'm not sure my old "memcmp/strcmp" idioms are serving well here. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3gthr.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3gthr.c:209: if (gs->dtlsPacket.space < MAX_FRAGMENT_LENGTH + 2048 + 5) { On 2012/03/23 00:21:18, wtc wrote: > > On 2012/03/22 22:26:37, Ryan Sleevi wrote: > > What's the magic 2048 + 5? Should that be documented in the comment on line > 208? > > The magic constant is copied from ssl3_GatherData on lines > 134-137. I will also copy the comment here and note that > 5 should be changed to 13 (the size of the record header). I just noticed that as well. We could change that in the code if you wanted (Note that in any sane network this is really way too big anyway), and the effect is simply to reduce the effective MTU if it somehow is magically as big as 16k. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslgathr.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslgathr.c:437: gs->dtlsPacketOffset = 0; On 2012/03/23 00:38:28, Ryan Sleevi wrote: > On 2012/03/23 00:21:18, wtc wrote: > > > > On 2012/03/22 22:26:37, Ryan Sleevi wrote: > > > What about gs->dtlsPacket.len ? > > > > I don't fully understand this code, so I am proving by > > analogy. > > > > dtls_GatherData is analogous to ssl3_GatherData. > > > > dtls_GatherData operates on gs->dtlsPacket. > > ssl3_GatherData operates on gs->inbuf. > > > > Since this function does not set gs->inbuf.len to 0, > > I concluded that it doesn't need to set gs->dtlsPacket.len > > to 0 either. > > > > See also lines 448-449 below, where ssl_DestroyGather frees > > gs->inbuf.buf and gs->dtlsPacket.buf at the same time. > > Yes, but neither PORT_Free resets gs->inbuf.len / gs->dtlsPacket.len. > > My concern was that I wasn't seeing where gs->dtlsPacketOffset.len was reset. If > you look in dtls_GatherData (which is where the GS_INIT state leads to), it only > sets ->dtlsPacket.len /after/ it checked that dtlsPacketOffset (reset here) > equals gs->dtlsPacket.len. > > Note that for GS_INIT, ssl3gthr.c:82 (ssl3_GatherData) resets gs->inbuf.len to 0 > for GS_INIT, but I see no such equivalent code in ssl3gthr.c:192 > (dtls_GatherData). That's why I have trouble understanding where > gs->dtlsPacket.len is properly zero initialized. So your concern here is the initial state of gs->dtlsPacket.len? The reason that it is initially 0 is that the sslSocket structure (containing the gather) is PR_ZAlloc()'d, so I *think* it should be zero at the beginning and then all future code paths should leave it correct. However, I agree it should be explicitly zerod. I suggest that what we need is to set it to zero in ssl_InitGather(). Does that sound right to you? http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslsock.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslsock.c:294: if (os->protocolVariant != ssl_variant_stream) { On 2012/03/22 22:26:37, Ryan Sleevi wrote: > !IS_DTLS(os) ? Yes. http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:1257: dtls_InitRecvdRecords(&pwSpec->recvdRecords); On 2012/03/22 21:59:54, wtc wrote: > > ekr: can we also move this > dtls_InitRecvdRecords(&pwSpec->recvdRecords); > call to ssl3_InitPendingCipherSpec, next to where it sets > pwSpec->epoch? I have have the nagging feeling that I tried this and it didn't work for some reason (which would be worrisome), though it could equally well be that I got lost in the maze of different cipher spec setup functions and just opted to add a comment rather than fix. Do you think we should analyze/try it? http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:10080: ss->ssl3.hs.sendMessageSeq = 0; On 2012/03/23 02:10:43, wtc wrote: > > ekr: RFC 6347 (DTLS 1.2) Section 4.2.2 Handshake Message Format says: > > The first message each side transmits in each handshake always has > message_seq = 0. > > Is that done here? Do you know if NSS calls this function > before starting a renegotiation? I am worried about the > if (ss->ssl3.initialized) > return SECSuccess; /* Function should be idempotent */ > early return at the beginning of this function. Oops. I think you're right here. I think we need to move/copy some of this to somewhere in the code where initial handshake or rehandshake happens. Any thoughts about where?
http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:1223: (suite_def->bulk_cipher_alg != cipher_rc4_56)); I think the use of PORT_Assert is appropriate here because we are not detecting a possible but unlikely runtime condition. We are detecting an NSS bug. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslgathr.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslgathr.c:437: gs->dtlsPacketOffset = 0; I analyzed this last night and have fixed this in my local tree by setting gs->dtlsPacket.len to 0. http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:1257: dtls_InitRecvdRecords(&pwSpec->recvdRecords); On 2012/03/23 12:46:41, ekr wrote: > Do you think we should analyze/try it? Yes, please. http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:10080: ss->ssl3.hs.sendMessageSeq = 0; On 2012/03/23 12:46:41, ekr wrote: > > Oops. I think you're right here. I think we need to move/copy some of this to > somewhere in the code where initial handshake or rehandshake happens. Any > thoughts about where? No idea off the top of my head. I guess it has to be done in a function that is also called by functions that initiate a renegotiation (SSL_ReHandshake?) or handle a HelloRequest.
rsleevi: please review patch set 6. I made all the changes you suggested except the ones already noted. Thanks.
Had no network today and too tired to do anything useful now. Will check out the two issues below tomorrow AM my time. -Ekr On Fri, Mar 23, 2012 at 2:48 PM, <wtc@chromium.org> wrote: > > http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... > File net/third_party/nss/ssl/ssl3con.c (right): > > http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... > net/third_party/nss/ssl/ssl3con.c:1223: (suite_def->bulk_cipher_alg != > cipher_rc4_56)); > I think the use of PORT_Assert is appropriate here because > we are not detecting a possible but unlikely runtime > condition. We are detecting an NSS bug. > > > http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... > File net/third_party/nss/ssl/sslgathr.c (right): > > http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... > net/third_party/nss/ssl/sslgathr.c:437: gs->dtlsPacketOffset = 0; > I analyzed this last night and have fixed this in my local > tree by setting gs->dtlsPacket.len to 0. > > > http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... > File net/third_party/nss/ssl/ssl3con.c (right): > > http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... > net/third_party/nss/ssl/ssl3con.c:1257: > dtls_InitRecvdRecords(&pwSpec->recvdRecords); > On 2012/03/23 12:46:41, ekr wrote: >> >> Do you think we should analyze/try it? > > > Yes, please. > > > http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl... > File net/third_party/nss/ssl/ssl3con.c (right): > > http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl... > net/third_party/nss/ssl/ssl3con.c:10080: ss->ssl3.hs.sendMessageSeq = 0; > > On 2012/03/23 12:46:41, ekr wrote: > >> Oops. I think you're right here. I think we need to move/copy some of > > this to >> >> somewhere in the code where initial handshake or rehandshake happens. > > Any >> >> thoughts about where? > > > No idea off the top of my head. I guess it has to be > done in a function that is also called by functions > that initiate a renegotiation (SSL_ReHandshake?) or > handle a HelloRequest. > > http://codereview.chromium.org/9764001/
On Fri, Mar 23, 2012 at 2:48 PM, <wtc@chromium.org> wrote: > http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... > File net/third_party/nss/ssl/ssl3con.c (right): > > http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl... > net/third_party/nss/ssl/ssl3con.c:1257: > dtls_InitRecvdRecords(&pwSpec->recvdRecords); > On 2012/03/23 12:46:41, ekr wrote: >> >> Do you think we should analyze/try it? > > > Yes, please. This seems to work. Patch below. --- a/mozilla/security/nss/lib/ssl/ssl3con.c +++ b/mozilla/security/nss/lib/ssl/ssl3con.c @@ -1255,10 +1255,6 @@ ssl3_SetupPendingCipherSpec(sslSocket *ss) pwSpec->compression_method = ss->ssl3.hs.compression; pwSpec->compressContext = NULL; pwSpec->decompressContext = NULL; - - /* Note: pwSpec == prSpec here so we're really doing the read side. - The epoch is set up in InitPendingCipherSpec */ - dtls_InitRecvdRecords(&pwSpec->recvdRecords); ssl_ReleaseSpecWriteLock(ss); /*******************************/ return SECSuccess; @@ -1822,6 +1818,8 @@ ssl3_InitPendingCipherSpec(sslSocket *ss, PK11SymKey *pms) pwSpec->read_seq_num.high = pwSpec->write_seq_num.high = pwSpec->epoch << 16; + + dtls_InitRecvdRecords(&pwSpec->recvdRecords); } pwSpec->read_seq_num.low = pwSpec->write_seq_num.low = 0; -Ekr
Changes LGTM. Per your request, I have not reviewed the DLTS implementation. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:1223: (suite_def->bulk_cipher_alg != cipher_rc4_56)); On 2012/03/23 13:48:49, wtc wrote: > I think the use of PORT_Assert is appropriate here because > we are not detecting a possible but unlikely runtime > condition. We are detecting an NSS bug. Good point. SGTM. http://codereview.chromium.org/9764001/diff/31008/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/31008/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:9686: * consistent with the guidance of RFC 6347 S 4.1 and S 4.2.4.1 nit: I notice elsewhere you updated S -> Section. Applicable here as well? http://codereview.chromium.org/9764001/diff/31008/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3gthr.c (right): http://codereview.chromium.org/9764001/diff/31008/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3gthr.c:187: * This loop returns when either (a) an error or EOF occurs, You updated line 63 but not this one
rsleevi: thanks for the review. Patch set 8 has all the changes you suggested. I will commit it after the Chrome 18 branch point. ekr: it would be nice if you can go over the changes I made to your original patch (patch set 1). Start from http://codereview.chromium.org/9764001/diff2/1:34004/AUTHORS?context=10&colum... and the use the 'j' and 'k' keys to move forward and backward between files.
Willdo. Right now I'm trying to track down some memory complaints Valgrind is giving me. I *think* it's a valgrind screwup, but I'm seeing some weirdness in my libjingle test harness so I'd like to make sure. -Ekr On Mon, Mar 26, 2012 at 9:52 PM, <wtc@chromium.org> wrote: > rsleevi: thanks for the review. Patch set 8 has all the > changes you suggested. I will commit it after the Chrome 18 > branch point. > > ekr: it would be nice if you can go over the changes I made > to your original patch (patch set 1). Start from > > http://codereview.chromium.org/9764001/diff2/1:34004/AUTHORS?context=10&colum... > > and the use the 'j' and 'k' keys to move forward and backward between files. > > http://codereview.chromium.org/9764001/
On 2012/03/26 19:54:20, ekr wrote: > Willdo. Right now I'm trying to track down some memory complaints Valgrind is > giving me. I *think* it's a valgrind screwup, but I'm seeing some weirdness in > my libjingle test harness so I'd like to make sure. > > -Ekr Before you spend too much time digging in, there are a number of suppressions for NSS that we use due to how Chromium uses it. You should run the tests using the test harnesses in tools/valgrind, which will incorporate all of the suppression files/filters (see tools/valgrind/suppressions.py). http://www.chromium.org/developers/how-tos/using-valgrind has the full list of steps specific to Chromium.
Thanks for the heads-up... Turns out that building valgrind from SVN head removes the error. Apparently it wasn't redirecting memmove properly. Of course, I still need to debug the error I was trying to use valgrind to debug :( -Ekr On Mon, Mar 26, 2012 at 10:02 PM, <rsleevi@chromium.org> wrote: > On 2012/03/26 19:54:20, ekr wrote: >> >> Willdo. Right now I'm trying to track down some memory complaints Valgrind >> is >> giving me. I *think* it's a valgrind screwup, but I'm seeing some >> weirdness in >> my libjingle test harness so I'd like to make sure. > > >> -Ekr > > > Before you spend too much time digging in, there are a number of > suppressions > for NSS that we use due to how Chromium uses it. > > You should run the tests using the test harnesses in tools/valgrind, which > will > incorporate all of the suppression files/filters (see > tools/valgrind/suppressions.py). > > http://www.chromium.org/developers/how-tos/using-valgrind has the full list > of > steps specific to Chromium. > > http://codereview.chromium.org/9764001/
These generally look right. I'm still diagnosing why I'm seeing sporadic errors in DTLS in my libjingle test harness. Do you think it makes sense to hold off on landing this until we know that they're in libjingle rather than NSS? http://codereview.chromium.org/9764001/diff/1/net/third_party/nss/ssl/ssl3con.c File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/1/net/third_party/nss/ssl/ssl3con... net/third_party/nss/ssl/ssl3con.c:230: #define RECORD_HEADER_LENGTH(ss_) ((!ss_->ssl3.isDtls) ? \ Looks to me like this is no longer used, which is why you removed it. You agree? http://codereview.chromium.org/9764001/diff/1/net/third_party/nss/ssl/ssl3con... net/third_party/nss/ssl/ssl3con.c:2219: cipherBytes += ivBytes; On 2012/03/21 01:36:40, ekr wrote: > On 2012/03/21 01:22:07, wtc wrote: > > > > This is a merging error. > > > > cipherBytes already includes the IV length. See line 2113 > > above. Your ivBytes variable is always 0. > > Oops. This merge got ugly and I thought I caught all the errors, but I guess > not. I double-checked this and I agree. http://codereview.chromium.org/9764001/diff/1/net/third_party/nss/ssl/ssl3con... net/third_party/nss/ssl/ssl3con.c:7948: if (rv != SECSuccess || (client_version != ss->clientHelloVersion)) { On 2012/03/21 01:36:40, ekr wrote: > On 2012/03/21 01:22:07, wtc wrote: > > > > I think this > > rv != SECSuccess || > > was added incorrectly. At this point, rv must be equal to > > SECSuccess because of the test on line 7938 above. > > You're right. This was an error introduced when I changed the semantics of > dtls_DTLSVersion...() so it could never fail and did not update this code. Done. http://codereview.chromium.org/9764001/diff/1/net/third_party/nss/ssl/ssl3con... net/third_party/nss/ssl/ssl3con.c:7948: if (rv != SECSuccess || (client_version != ss->clientHelloVersion)) { On 2012/03/21 01:22:07, wtc wrote: > > I think this > rv != SECSuccess || > was added incorrectly. At this point, rv must be equal to > SECSuccess because of the test on line 7938 above. Done. http://codereview.chromium.org/9764001/diff/1/net/third_party/nss/ssl/ssl3con... net/third_party/nss/ssl/ssl3con.c:9828: cipher_def = crSpec->cipher_def; This looks like it was just redundant? http://codereview.chromium.org/9764001/diff/1/net/third_party/nss/ssl/sslsock.c File net/third_party/nss/ssl/sslsock.c (right): http://codereview.chromium.org/9764001/diff/1/net/third_party/nss/ssl/sslsock... net/third_party/nss/ssl/sslsock.c:1736: break; /* Belt and suspenders */ I guess this is house style, but I've gotten very paranoid about having any case fall through even in situations where you are exiting the function. I find that when you refactor you sometimes miss these. Your call though. http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:10080: ss->ssl3.hs.sendMessageSeq = 0; Conveniently, I already solved this problem and have a DTLS function for it, so we can extend it. Patch below. As you can imagine, I basically haven't tested rehandshake for DTLS yet. --- a/mozilla/security/nss/lib/ssl/dtls1con.c +++ b/mozilla/security/nss/lib/ssl/dtls1con.c @@ -935,6 +935,7 @@ dtls_RehandshakeCleanup(sslSocket *ss) { dtls_CancelTimer(ss); ssl3_DestroyCipherSpec(ss->ssl3.pwSpec, PR_FALSE); + ss->ssl3.hs.sendMessageSeq = 0; } /* Set the MTU to the next step less than or equal to the http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/dtl... File net/third_party/nss/ssl/dtls1con.c (right): http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/dtl... net/third_party/nss/ssl/dtls1con.c:223: PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss)); A lot of the existing code has these extra spaces in it. See, for instance: ssl3con.c:1193... PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss)); I'm not arguing for or against this formatting, but that's where I got the leading space, at least, and I suspect I cut-and-pasted these lines entirely... http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/dtl... net/third_party/nss/ssl/dtls1con.c:467: Did you reverse these for a technical reason or is it just aesthetic? It looks equivalent to me, but I worry I'm missing something. http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:45: /* TODO(ekr): Implement HelloVerifyRequest on server side. OK for now. */ Do we want to make this XXX? http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:1760: pwSpec = ss->ssl3.pwSpec; Was there a reason to reverse these? http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:2564: PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss)); Should we clean up the spaces here for consistency with your other changes? http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3gthr.c (right): http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3gthr.c:216: if (gs->dtlsPacket.space < MAX_FRAGMENT_LENGTH + 2048 + 13) { Good catch here. http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3gthr.c:379: /* As above, we need to test datagram mode */ We could probably just remove this comment, since it was a rationale for why not using IS_DTLS() but now that that refers to a socket property, we are using it. http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslsock.c (right): http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslsock.c:296: return NULL; I'm not sure I understand the assert/versus PORT_SetError() convention. This is a clear error by the programmer. Do we only use asserts for internal errors? http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslsock.c:1771: PORT_SetError(SEC_ERROR_INVALID_ARGS); I wonder if this should be an assert? Since SSLProtocolVariant is an enum, how could you get a different value without someone deliberately casting from int to enum?
Ekr: thanks for your comments. I responded to them below. http://codereview.chromium.org/9764001/diff/1/net/third_party/nss/ssl/ssl3con.c File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/1/net/third_party/nss/ssl/ssl3con... net/third_party/nss/ssl/ssl3con.c:230: #define RECORD_HEADER_LENGTH(ss_) ((!ss_->ssl3.isDtls) ? \ On 2012/03/26 21:49:18, ekr wrote: > Looks to me like this is no longer used, which is why you removed it. You agree? Yes, I noticed (and verified) RECORD_HEADER_LENGTH is no longer used and removed it. http://codereview.chromium.org/9764001/diff/1/net/third_party/nss/ssl/ssl3con... net/third_party/nss/ssl/ssl3con.c:9828: cipher_def = crSpec->cipher_def; On 2012/03/26 21:49:18, ekr wrote: > This looks like it was just redundant? Yes, I bet this is also a merging error. Brian Smith moved it up for the new TLS 1.1 explicit IV code. http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:10080: ss->ssl3.hs.sendMessageSeq = 0; On 2012/03/26 21:49:18, ekr wrote: > Conveniently, I already solved this problem and have a DTLS function for it, so > we can extend it. Patch below. > > As you can imagine, I basically haven't tested rehandshake for DTLS yet. > > > --- a/mozilla/security/nss/lib/ssl/dtls1con.c > +++ b/mozilla/security/nss/lib/ssl/dtls1con.c > @@ -935,6 +935,7 @@ dtls_RehandshakeCleanup(sslSocket *ss) > { > dtls_CancelTimer(ss); > ssl3_DestroyCipherSpec(ss->ssl3.pwSpec, PR_FALSE); > + ss->ssl3.hs.sendMessageSeq = 0; > } > > /* Set the MTU to the next step less than or equal to the Should dtls_RehandshakeCleanup also set ss->ssl3.hs.recvMessageSeq to 0? http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/dtl... File net/third_party/nss/ssl/dtls1con.c (right): http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/dtl... net/third_party/nss/ssl/dtls1con.c:467: On 2012/03/26 21:49:18, ekr wrote: > Did you reverse these for a technical reason or is it just aesthetic? It is just aesthetic. http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:45: /* TODO(ekr): Implement HelloVerifyRequest on server side. OK for now. */ On 2012/03/26 21:49:18, ekr wrote: > Do we want to make this XXX? I deliberately kept this TODO(ekr). Your call :-) http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:1760: pwSpec = ss->ssl3.pwSpec; On 2012/03/26 21:49:18, ekr wrote: > Was there a reason to reverse these? This is also just aesthetic. http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:2564: PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss)); On 2012/03/26 21:49:18, ekr wrote: > Should we clean up the spaces here for consistency with your other changes? I only cleaned up the spaces in dtls1con.c because it is a brand new file. In ssl3con.c I refrained from fixing style issues to keep the diffs small. http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslsock.c (right): http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslsock.c:296: return NULL; On 2012/03/26 21:49:18, ekr wrote: > I'm not sure I understand the assert/versus PORT_SetError() convention. This is > a clear error by the programmer. Do we only use asserts for internal errors? We use asserts for NSS internal errors. Occasionally we also use asserts for extremely serious application errors. I just analyzed the ssl_DupSocket function. I found that DTLS_ImportFD with a non-null 'model' socket will land us here. So it is safer to use real error handling instead of an assert here. (The other caller of ssl_DupSocket is ssl_Accept, which only makes sense for stream sockets. So an assert would be appropriate for that caller.) http://codereview.chromium.org/9764001/diff/34004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslsock.c:1771: PORT_SetError(SEC_ERROR_INVALID_ARGS); On 2012/03/26 21:49:18, ekr wrote: > I wonder if this should be an assert? Since SSLProtocolVariant is an enum, how > could you get a different value without someone deliberately casting from int to > enum? You are right. The problem is that C is more relaxed with enums than C++ is, so you can convert an int to the SSLProtocolVariant enum without a cast. We can reinstate the assert, but it is important to keep the PORT_SetError for the release (non-debug) build.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wtc@chromium.org/9764001/49003
Try job failure for 9764001-49003 (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wtc@chromium.org/9764001/49003
Change committed as 129778
http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3con.c:10080: ss->ssl3.hs.sendMessageSeq = 0; On 2012/03/27 00:28:25, wtc wrote: > > On 2012/03/26 21:49:18, ekr wrote: > > Conveniently, I already solved this problem and have a DTLS function for it, > so > > we can extend it. Patch below. > > > > As you can imagine, I basically haven't tested rehandshake for DTLS yet. > > > > > > --- a/mozilla/security/nss/lib/ssl/dtls1con.c > > +++ b/mozilla/security/nss/lib/ssl/dtls1con.c > > @@ -935,6 +935,7 @@ dtls_RehandshakeCleanup(sslSocket *ss) > > { > > dtls_CancelTimer(ss); > > ssl3_DestroyCipherSpec(ss->ssl3.pwSpec, PR_FALSE); > > + ss->ssl3.hs.sendMessageSeq = 0; > > } > > > > /* Set the MTU to the next step less than or equal to the > > Should dtls_RehandshakeCleanup also set > ss->ssl3.hs.recvMessageSeq to 0? Yes, I think it should. |