Issue 119690044: Negotiate TLS 1.3 draft and use extension to make sure we have matching draft versions (Closed)
DescriptionNegotiate TLS 1.3 draft and use extension to make sure we have matching draft versions
Patch Set 1 #
Total comments: 28
Patch Set 2 : Revised per WTC #Patch Set 3 : WTC's comments with Makefile #
Total comments: 8
Patch Set 4 : Revised per MT's comments #
Total comments: 16
Patch Set 5 : Revised per Wan-Teh's comments #Patch Set 6 : Fix configuration flag (missed comment) #
Total comments: 3
Patch Set 7 : Revert broken extension behavior #
Total comments: 1
MessagesTotal messages: 20
Wan-Teh, I have attached a draft patch to negotiate TLS 1.3. It doesn't make any TLS protocol changes other than that and adding an extension so that we can determine which TLS 1.3 draft is being negotiated so that we can avoid negotiating incompatible drafts while the protocol is under development. I will email the list shortly to propose that extension.
Sign in to reply to this message.
Patch set 1 LGTM. Please upload a new patch set. Would be nice to also have Martin take a look. Thanks! https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode86 lib/ssl/ssl3ext.c:86: PRUint32 maxBytes); Nit: indentation is off by one. https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode89 lib/ssl/ssl3ext.c:89: /* Please remove the spaces you added by accident. https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2435 lib/ssl/ssl3ext.c:2435: ssl3_ServerHandleDraftVersionXtn(sslSocket * ss, PRUint16 ex_type, Nit: please swap these two functions so that we define ssl3_ClientSend before ssl3_ServerHandle. This matches the declaration order, and also the order in which these two events occur. https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2440 lib/ssl/ssl3ext.c:2440: /* Ignore this extension if we aren't doing TLS 1.2 or greater. */ TLS 1.2 or greater => TLS 1.3 https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2441 lib/ssl/ssl3ext.c:2441: if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3) { It seems that this should be ss->version != SSL_LIBRARY_VERSION_TLS_1_3, right? Similarly for the test on line 2484. https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2446 lib/ssl/ssl3ext.c:2446: ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type; Hmm... it seems that we should only increment numNegotiated when returning SECSuccess. But I see all the other extension handlers do this, so this is probably OK. https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2468 lib/ssl/ssl3ext.c:2468: TLS 1.3 implementation. Fall back to TLS 1.2 */ Nit: add a '*' at the beginning of this line, as follows: /* Something went wrong parsing this, which means it is an incompatible * TLS 1.3 implementation. Fall back to TLS 1.2 */ https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2471 lib/ssl/ssl3ext.c:2471: ss->version = SSL_LIBRARY_VERSION_TLS_1_2; IMPORTANT: it seems bad to change ss->version like this because we may have executed code that makes decision based on ss->version being TLS 1.3. It seems that we should fail the handshake and rely on the application retrying the handshake with TLS 1.2. Martin wrote an NSS patch to make a SECFailure return value from an extension handler fail the handshake, but that patch hasn't been checked in. Until then, I think we can send a handshake_failure alert message at line 2462 to abort the handshake. https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2473 lib/ssl/ssl3ext.c:2473: return SECSuccess; Let's return SECFailure here, if you agree with my suggestion above. https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2497 lib/ssl/ssl3ext.c:2497: Nit: I suggest deleting this blank line and line 2503. https://codereview.appspot.com/119690044/diff/1/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (right): https://codereview.appspot.com/119690044/diff/1/lib/ssl/sslimpl.h#newcode1538 lib/ssl/sslimpl.h:1538: #define SSL_LIBRARY_VERSION_MAX_SUPPORTED SSL_LIBRARY_VERSION_TLS_1_3 This should be controlled by a build variable so that by default SSL_LIBRARY_VERSION_MAX_SUPPORTED is defined as TLS 1.2. https://codereview.appspot.com/119690044/diff/1/lib/ssl/sslproto.h File lib/ssl/sslproto.h (right): https://codereview.appspot.com/119690044/diff/1/lib/ssl/sslproto.h#newcode22 lib/ssl/sslproto.h:22: between incompatible pre-standard TLS 1.3 drafts */ Nit: Please add a '*' at the beginning of this line, like this: /* The TLS 1.3 draft version. Used to avoid negotiating * between incompatible pre-standard TLS 1.3 drafts. */ https://codereview.appspot.com/119690044/diff/1/lib/ssl/sslproto.h#newcode23 lib/ssl/sslproto.h:23: #define SSL_LIBRARY_VERSION_TLS_1_3_DRAFT_VERSION 3 Let's shorten this to "TLS_1_3_DRAFT_VERSION", IMPORTANT: This header is a public header. If this macro is only used inside NSS, please move this macro definition to the private header ssl3prot.h. https://codereview.appspot.com/119690044/diff/1/lib/ssl/sslt.h File lib/ssl/sslt.h (right): https://codereview.appspot.com/119690044/diff/1/lib/ssl/sslt.h#newcode194 lib/ssl/sslt.h:194: ssl_renegotiation_info_xtn = 0xff01, /* experimental number */ Should we remove the "experimental number" comment for the renegotiation_info extension?
Sign in to reply to this message.
Wan-Teh, I wanted to discuss the use of the draft version extension. The intent here is to make it safe for implementations to unconditionally offer TLS 1.3 (modulo extension/version intolerance issues). It is true that in HTTPS contexts the browsers will fall back, but because there is (hopefully) not really a version intolerance issue for WebRTC, the browsers do not do so and so we would like to keep it that way. I did investigate the question you raised about decisions being made based on the version number that would then no longer be accurate. Here is where the version is negotiated: http://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c?... Subsequent to that, we have: 1. The handshake hash initialization, which is currently the same between TLS 1.2 and 1.3: http://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c?... 2. Reading a bunch of variables, which is safe (through lines 7729) 3. Processing the extensions: http://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c?... The reversion obviously happens during the extension processing. So, I believe that the major place that you have version dependency would be during the extension processing. A cursory review of the places that ss->version is used in ssl3ext.c suggests that this is safe. However, perhaps as a double-check we could require that: (a) the client always sends this extension first. This is in violation of RFC 5246 section 7.4.1.4 but since this is temporary, that's probably OK. (b) if the server detects that this extension is not first, it simply generates an error as you suggest above. It appears to me that this could easily be implemented by reordering the values in clientHelloSendersTLS and then having some special case code in ssl3_HandleHelloExtension. We could revert both of these changes as soon as TLS 1.3 went to RFC. Does this seem worth doing? I recognize this is a bit clunky, but I do think it's important to make it safe to offer the wrong version of TLS 1.3 without penalty. I would certainly be open to any other suggestions you had for how to achieve this. I can make the rest of the changes you suggest.
Sign in to reply to this message.
Here is a revised patch set. Please ignore patch set 2 which inadvertantly did not contain the Makefile. Martin, can you PTAL? https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode86 lib/ssl/ssl3ext.c:86: PRUint32 maxBytes); On 2014/08/13 21:41:21, wtc wrote: > > Nit: indentation is off by one. Done. https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode89 lib/ssl/ssl3ext.c:89: /* On 2014/08/13 21:41:21, wtc wrote: > > Please remove the spaces you added by accident. Done. https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2435 lib/ssl/ssl3ext.c:2435: ssl3_ServerHandleDraftVersionXtn(sslSocket * ss, PRUint16 ex_type, On 2014/08/13 21:41:21, wtc wrote: > > Nit: please swap these two functions so that we define ssl3_ClientSend before > ssl3_ServerHandle. This matches the declaration order, and also the order in > which these two events occur. Done. https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2440 lib/ssl/ssl3ext.c:2440: /* Ignore this extension if we aren't doing TLS 1.2 or greater. */ On 2014/08/13 21:41:21, wtc wrote: > > TLS 1.2 or greater => TLS 1.3 Done. https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2441 lib/ssl/ssl3ext.c:2441: if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3) { On 2014/08/13 21:41:21, wtc wrote: > > It seems that this should be ss->version != SSL_LIBRARY_VERSION_TLS_1_3, right? > > Similarly for the test on line 2484. Done. https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2446 lib/ssl/ssl3ext.c:2446: ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type; On 2014/08/13 21:41:21, wtc wrote: > > Hmm... it seems that we should only increment numNegotiated when returning > SECSuccess. But I see all the other extension handlers do this, so this is > probably OK. Acknowledged. https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2468 lib/ssl/ssl3ext.c:2468: TLS 1.3 implementation. Fall back to TLS 1.2 */ On 2014/08/13 21:41:21, wtc wrote: > > Nit: add a '*' at the beginning of this line, as follows: > > /* Something went wrong parsing this, which means it is an incompatible > * TLS 1.3 implementation. Fall back to TLS 1.2 */ Done. https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2471 lib/ssl/ssl3ext.c:2471: ss->version = SSL_LIBRARY_VERSION_TLS_1_2; On 2014/08/13 21:41:21, wtc wrote: > > IMPORTANT: it seems bad to change ss->version like this because we may have > executed code that makes decision based on ss->version being TLS 1.3. It seems > that we should fail the handshake and rely on the application retrying the > handshake with TLS 1.2. > > Martin wrote an NSS patch to make a SECFailure return value from an extension > handler fail the handshake, but that patch hasn't been checked in. Until then, I > think we can send a handshake_failure alert message at line 2462 to abort the > handshake. Added a TODO while we think about this, as discussed in e-mail https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2473 lib/ssl/ssl3ext.c:2473: return SECSuccess; On 2014/08/13 21:41:21, wtc wrote: > > Let's return SECFailure here, if you agree with my suggestion above. Left as-is per above, https://codereview.appspot.com/119690044/diff/1/lib/ssl/ssl3ext.c#newcode2497 lib/ssl/ssl3ext.c:2497: On 2014/08/13 21:41:21, wtc wrote: > > Nit: I suggest deleting this blank line and line 2503. Done. https://codereview.appspot.com/119690044/diff/1/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (right): https://codereview.appspot.com/119690044/diff/1/lib/ssl/sslimpl.h#newcode1538 lib/ssl/sslimpl.h:1538: #define SSL_LIBRARY_VERSION_MAX_SUPPORTED SSL_LIBRARY_VERSION_TLS_1_3 On 2014/08/13 21:41:22, wtc wrote: > > This should be controlled by a build variable so that by default > SSL_LIBRARY_VERSION_MAX_SUPPORTED is defined as TLS 1.2. Done. https://codereview.appspot.com/119690044/diff/1/lib/ssl/sslproto.h File lib/ssl/sslproto.h (right): https://codereview.appspot.com/119690044/diff/1/lib/ssl/sslproto.h#newcode22 lib/ssl/sslproto.h:22: between incompatible pre-standard TLS 1.3 drafts */ On 2014/08/13 21:41:22, wtc wrote: > > Nit: Please add a '*' at the beginning of this line, like this: > > /* The TLS 1.3 draft version. Used to avoid negotiating > * between incompatible pre-standard TLS 1.3 drafts. */ Done. https://codereview.appspot.com/119690044/diff/1/lib/ssl/sslproto.h#newcode23 lib/ssl/sslproto.h:23: #define SSL_LIBRARY_VERSION_TLS_1_3_DRAFT_VERSION 3 On 2014/08/13 21:41:22, wtc wrote: > > Let's shorten this to "TLS_1_3_DRAFT_VERSION", > > IMPORTANT: This header is a public header. If this macro is only used inside > NSS, please move this macro definition to the private header ssl3prot.h. Done. https://codereview.appspot.com/119690044/diff/1/lib/ssl/sslt.h File lib/ssl/sslt.h (right): https://codereview.appspot.com/119690044/diff/1/lib/ssl/sslt.h#newcode194 lib/ssl/sslt.h:194: ssl_renegotiation_info_xtn = 0xff01, /* experimental number */ On 2014/08/13 21:41:22, wtc wrote: > > Should we remove the "experimental number" comment for the renegotiation_info > extension? Yes. Done.
Sign in to reply to this message.
I'm a little concerned about the extension handling. Until https://bugzilla.mozilla.org/show_bug.cgi?id=753136 is landed, this isn't going to work quite right. https://codereview.appspot.com/119690044/diff/40001/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/119690044/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2444: extension_length = 6; /* Type + length + number */ Constants seem more appropriate here, don't they? But then, NSS seems a little constant-averse. https://codereview.appspot.com/119690044/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2486: ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type; Don't we normally include this only if the extension is valid? https://codereview.appspot.com/119690044/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2502: goto loser; So this is a case for bug 753136. This is not going to work properly until that lands. You want to return SECFailure on this branch and then get that in. Otherwise, your handshake won't fail. https://codereview.appspot.com/119690044/diff/40001/lib/ssl/sslt.h File lib/ssl/sslt.h (right): https://codereview.appspot.com/119690044/diff/40001/lib/ssl/sslt.h#newcode195 lib/ssl/sslt.h:195: ssl_tls13_draft_version_xtn = 0xff02 /* experimental number */ comment on line above, plural
Sign in to reply to this message.
https://codereview.appspot.com/119690044/diff/40001/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/119690044/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2444: extension_length = 6; /* Type + length + number */ On 2014/08/19 17:03:30, mt wrote: > Constants seem more appropriate here, don't they? But then, NSS seems a little > constant-averse. I am following NSS house style as I understand it. https://codereview.appspot.com/119690044/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2486: ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type; On 2014/08/19 17:03:30, mt wrote: > Don't we normally include this only if the extension is valid? Based on WT's previous review, it seems like no. https://codereview.appspot.com/119690044/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2502: goto loser; On 2014/08/19 17:03:30, mt wrote: > So this is a case for bug 753136. This is not going to work properly until that > lands. You want to return SECFailure on this branch and then get that in. > > Otherwise, your handshake won't fail. Sorry for the confusion. The idea is that it is supposed to just fall back to TLS 1.2 if there is anything wrong with the extension. So there are three cases, which MT and I propose to handle as follows. 1. Valid extension with matching # -> SECSuccess 2. Valid extension with non-matching # -> SECSuccess and fall back to 1.2 3. Invalid extension -> SECFailure MT, WTC: should I still mark the extension as negotiated in case #3?
Sign in to reply to this message.
https://codereview.appspot.com/119690044/diff/40001/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/119690044/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2502: goto loser; On 2014/08/19 18:00:19, ekr-webrtc wrote: > MT, WTC: should I still mark the extension as negotiated in case #3? I think that you only want to mark the extension negotiated in case 1, and maybe 2. If you aren't using the extension (i.e., you plan to ignore it or it's broken), then don't claim that it's there. The only reason you might include it in case #2 is if you want to know whether 1.3 was offered at some point while you are doing 1.2. Since you can't see why (the extension value isn't saved), I don't much see the point.
Sign in to reply to this message.
This is updated to match MT's review. WTC: I believe this is now ready to land if you are OK with the rollback strategy. PTAL.
Sign in to reply to this message.
Patch set 4 LGTM. Thanks! https://codereview.appspot.com/119690044/diff/60001/lib/ssl/Makefile File lib/ssl/Makefile (right): https://codereview.appspot.com/119690044/diff/60001/lib/ssl/Makefile#newcode67 lib/ssl/Makefile:67: endif Please move this to lib/ssl/config.mk. https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2478: PRInt32 draft_number; Nit: draft_number => draft_version ? https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2488: /* Get the draft number out of the handshake */ Nit: number => version ? https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2499: * here. Need to double-check. */ I haven't had time to check it is safe to roll back here. It is fine to add this TODO comment and check this in first. https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2501: SSL_GETPID(), ss->fd, draft_number)); Nit: This message should also print our draft version: TLS_1_3_DRAFT_VERSION. https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2503: return SECSuccess; IMPORTANT: Should we set ss->xtnData.negotiated in this case? https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2507: ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type; I agree it is better to set ss->xtnData.negotiated only when we return SECSuccess. We should change all the other extension handlers to do this.
Sign in to reply to this message.
Eric: I didn't read the discussion between you and Martin. So I may have asked about something that you and Martin already settled.
Sign in to reply to this message.
Wan-Teh, Let me know what your preference is on the comment below and I will prepare a new final patch taking care of the other issues. Would you like me to post the patch here or in Bugzilla? https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2503: return SECSuccess; On 2014/08/21 18:54:30, wtc wrote: > > IMPORTANT: Should we set ss->xtnData.negotiated in this case? Martin argued that we should not on the basis that we were not *accepting* TLS 1.3.It looks to me like the only time that this is used aside from checking for specific extensions is to check whether an extension appears twice. If we don't set it here, the result will be that this extension can appear twice in a ClientHello though we will ignore the second one. If you would prefer a failure I can set negotiated here.
Sign in to reply to this message.
Eric: Please upload the final patch set here. In Bugzilla it is fine to just post a link to this code review URL. https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2503: return SECSuccess; On 2014/08/21 19:37:33, ekr-webrtc wrote: > On 2014/08/21 18:54:30, wtc wrote: > > > > IMPORTANT: Should we set ss->xtnData.negotiated in this case? > > Martin argued that we should not on the basis that we were not *accepting* TLS > 1.3.It looks to me like the only time that this is used aside from checking for > specific extensions is to check whether an extension appears twice. If we don't > set it here, the result will be that this extension can appear twice in a > ClientHello though we will ignore the second one. > > If you would prefer a failure I can set negotiated here. It seems that we should not allow this extension to appear twice. Even though we don't accept TLS 1.3 in this case, we have processed this extension and made a change (fallback to TLS 1.2) based on that. So I think we should set ss->xtnData.negotiated here. Does this make sense?
Sign in to reply to this message.
On 2014/08/21 20:46:37, wtc wrote: > Eric: > > Please upload the final patch set here. In Bugzilla it is fine to just post a > link to this code review URL. > > https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c > File lib/ssl/ssl3ext.c (right): > > https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... > lib/ssl/ssl3ext.c:2503: return SECSuccess; > On 2014/08/21 19:37:33, ekr-webrtc wrote: > > On 2014/08/21 18:54:30, wtc wrote: > > > > > > IMPORTANT: Should we set ss->xtnData.negotiated in this case? > > > > Martin argued that we should not on the basis that we were not *accepting* TLS > > 1.3.It looks to me like the only time that this is used aside from checking > for > > specific extensions is to check whether an extension appears twice. If we > don't > > set it here, the result will be that this extension can appear twice in a > > ClientHello though we will ignore the second one. > > > > If you would prefer a failure I can set negotiated here. > > It seems that we should not allow this extension to appear twice. Even though we > don't accept TLS 1.3 in this case, we have processed this extension and made a > change (fallback to TLS 1.2) based on that. So I think we should set > ss->xtnData.negotiated here. > > Does this make sense? Yes, that seems reasonable. I will upload a new patch.
Sign in to reply to this message.
https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2455: 2); Nit: this probably all fits on one line.
Sign in to reply to this message.
Hopefully final patch. I will also file a bugzilla bug pointing to this. https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2455: 2); On 2014/08/21 20:59:20, wtc wrote: > > Nit: this probably all fits on one line. Done. https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2478: PRInt32 draft_number; On 2014/08/21 18:54:30, wtc wrote: > > Nit: draft_number => draft_version ? Done. https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2488: /* Get the draft number out of the handshake */ On 2014/08/21 18:54:30, wtc wrote: > > Nit: number => version ? Done. https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2501: SSL_GETPID(), ss->fd, draft_number)); On 2014/08/21 18:54:30, wtc wrote: > > Nit: This message should also print our draft version: TLS_1_3_DRAFT_VERSION. Done. https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2503: return SECSuccess; On 2014/08/21 20:46:37, wtc wrote: > On 2014/08/21 19:37:33, ekr-webrtc wrote: > > On 2014/08/21 18:54:30, wtc wrote: > > > > > > IMPORTANT: Should we set ss->xtnData.negotiated in this case? > > > > Martin argued that we should not on the basis that we were not *accepting* TLS > > 1.3.It looks to me like the only time that this is used aside from checking > for > > specific extensions is to check whether an extension appears twice. If we > don't > > set it here, the result will be that this extension can appear twice in a > > ClientHello though we will ignore the second one. > > > > If you would prefer a failure I can set negotiated here. > > It seems that we should not allow this extension to appear twice. Even though we > don't accept TLS 1.3 in this case, we have processed this extension and made a > change (fallback to TLS 1.2) based on that. So I think we should set > ss->xtnData.negotiated here. > > Does this make sense? Yes. I removed the return and moved the setting up, as well as changing the comment. https://codereview.appspot.com/119690044/diff/60001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:2507: ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type; On 2014/08/21 18:54:30, wtc wrote: > > I agree it is better to set ss->xtnData.negotiated only when we return > SECSuccess. We should change all the other extension handlers to do this. Acknowledged.
Sign in to reply to this message.
Patch set 6 LGTM. Please make two small changes. https://codereview.appspot.com/119690044/diff/100001/lib/ssl/config.mk File lib/ssl/config.mk (right): https://codereview.appspot.com/119690044/diff/100001/lib/ssl/config.mk#newcode9 lib/ssl/config.mk:9: IMPORTANT: please add the new code here. You added the new code inside a big ifdef block. Here it is clearer that it is outside any ifdef block. https://codereview.appspot.com/119690044/diff/100001/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/119690044/diff/100001/lib/ssl/ssl3ext.c#newcod... lib/ssl/ssl3ext.c:2494: ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type; This is just a comment: I now suspect ss->xtnData.negotiated was originally only intended to detect duplicate extensions, rather than indicating a well-formed extension has been successfully processed. At some point, we need to look into this and figure out whether we need a separate array ss->xtnData.received for detecting duplicate extensions, and use ss->xtnData.negotiated only to indicate a well-formed, successfully handled extension. https://codereview.appspot.com/119690044/diff/100001/lib/ssl/ssl3ext.c#newcod... lib/ssl/ssl3ext.c:2501: SSL_TRC(30, ("%d: SSL3[%d]: Incompatible version of TLS 1.3 (%d), expected", IMPORTANT: we're missing a "%d" after "expected".
Sign in to reply to this message.
Wan-Teh, I can make these changes but I wanted to get your opinion on the comment that I made in Bugzilla before I did so. Currently if someone sends you a malformed extension we return SECFailure but this has no effect prior to Martin's patch to handle SECFailure, thus you attempt to continue to do TLS 1.3, which will presumably eventually fail.. I propose to instead fall back to TLS 1.2 as with well-formed extensions with the wrong version number. This seems safer. If you agree, I will make this change (which actually reverts a previous patchset). Otherwise, I will just make these two changes. LMK what you think
Sign in to reply to this message.
On 2014/08/22 18:23:18, ekr-webrtc wrote: > > Currently if someone sends you a malformed extension we return SECFailure but > this has no effect prior to Martin's patch to handle SECFailure, thus you > attempt to continue to do TLS 1.3, which will presumably eventually fail.. I > propose to instead fall back to TLS 1.2 as with well-formed extensions with the > wrong version number. This seems safer. Yes, I agree. A malformed extension should be handled like a draft version mismatch. Note: I still intend to review and check in Martin's patch to handle SECFailure.
Sign in to reply to this message.
WTC, OK I think this is ready with the new (by which I mean old) behavior
Sign in to reply to this message.
Patch set 7 LGTM. Checked in: https://hg.mozilla.org/projects/nss/rev/970075886b70 https://codereview.appspot.com/119690044/diff/120001/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/119690044/diff/120001/lib/ssl/ssl3ext.c#newcod... lib/ssl/ssl3ext.c:2498: SSL_TRC(30, ("%d: SSL3[%d]: Incompatible version of TLS 1.3 (%d), expected %d", This line is longer than 80 characters. So I broke the string literal into two before I checked this in.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
