|
|
DescriptionBug 1266237 - Enable FFDHE and DHE for TLS 1.3
Patch Set 1 #
Total comments: 1
Patch Set 2 : Added resumption #
Total comments: 100
Patch Set 3 : First review round, less gratuity #
Total comments: 155
Patch Set 4 : Updated with comments, new tests #
Total comments: 46
Patch Set 5 : Latest #
Total comments: 2
Patch Set 6 : Correcting omissions #
Total comments: 27
Patch Set 7 : Moar tests #
Total comments: 20
Patch Set 8 : More tests, fix for overrun #
Total comments: 16
Patch Set 9 : Fixes, test restructure #
Total comments: 5
MessagesTotal messages: 30
I got partway through this, but there seem to be a very large number of unnecessary changes (moved, renamed, functions, etc.) I'm not sure I agree with these, but regardless it makes it hard to review. Can we please focus this PR on just the FFDHE changes. If you feel it's important to make these other changes, can you please write up why and we can consider them in a separate PR. https://codereview.appspot.com/297180043/diff/1/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/297180043/diff/1/lib/ssl/tls13con.c#newcode259 lib/ssl/tls13con.c:259: unsigned int i; This doesn't seem like the right answer. Generally we want to do p256 so why are we wasting time https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (left): https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#oldcode73 lib/ssl/sslsock.c:73: /* v2CompatibleHello */ /* now defaults to off in NSS 3.13 */ Please break this change into another PR. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode145 lib/ssl/sslsock.c:145: { 0, ec_secp192r1, 192, group_type_ec, ECOID(SECP192R1), PR_FALSE }, This is an odd choice to be first https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode260 lib/ssl/sslsock.c:260: } Unnecessary change. Please break this into a separate PR so I can review just the DH changes. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode403 lib/ssl/sslsock.c:403: ssl_FreeKeyPair(ss->stepDownKeyPair); I don't think it's a good idea to change ssl3_* to ssl_* https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode802 lib/ssl/sslsock.c:802: /* This feature is not supported if ECC isn't. */ Why? https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1419: PORT_CheckSuccess(ssl3_CipherPrefSet(ss, *suite, PR_FALSE)); Are you sure this executes if asserts are off? https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1422: } This seems pretty awful. I would far prefer a length. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1467: PRUint32 mask; This isn't a mask. It's a bitvector. Suggest renaming to "supportedGroups" or something. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1491: mask = ss->namedGroups; PORT_Assert that this doesn't shift past the end. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1515: break; Why do you need two separate values here? This seems like a recipe for problems. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1568: gWeakDHParams->name = ffdhe_weak; It's kind of unfortunate that ffdhe_weak is a DSA-style prime whereas the other primes are safe primes. Not your fault, though, I guess. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1652: } Can we get a table or something? https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1658: * group (if this is successful). */ RFC cite, please. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1671: This seems unnecessary. Since you need to check dh_p for the real group anywyy. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1682: PORT_Assert(params); Why not refactor this into a getter for the params. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1710: return SECSuccess; PORT_Assert that gWeakDHParams is nonzero. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1716: *params = ssl_GetDHEParams(&ssl_named_groups[i]); Refactor as above. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1721: /* Caller calls PORT_SetError() */ This is not a good idiom https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:2043: return NULL; What happened here? This doesn't seem like it's relevant. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:2051: ss->stepDownKeyPair = ssl_GetKeyPairRef(sm->stepDownKeyPair); Unnecessary change. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3450: } Why aren't you returning false if SECFailure? https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3451: return (ss->namedGroups & (1U << groupDef->index)) != 0; I feel like yu need a macro for these masks https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3458: PRUint32 mask = 0; Not a mask. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3474: } Why are you just enabling 2048 by default? https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3498: return pair; /* success */ This seems to just be the same as ssl3_NewKeyPair. Unnecessary cahnge. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3630: if (!ss) { Unnecessary change. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:259: unsigned int i; I don't think this is the right answer in the long term because we won't want to absorb the FFDHE cost by default. Maybe you can hack in a conditional based on one of the options temporarily? https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:266: for (i = 0; i < PR_ARRAY_SIZE(groups_to_try); ++i) { This is not going to be compatible with HRR. I would suggest at this point having groups_to_try being a member with a length. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:280: rv = ssl_CreateDHEKeyPair(groupDef, params, &keyPair); Why can't this take a namedGroupDEf https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:316: rv = tls13_HkdfExtractSharedKey(ss, shared, EphemeralSharedSecret); This seems like an unnecessary change. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:339: } Please check that length == 0 https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:348: peerKey.u.dh.base.data = keyPair->pubKey->u.dh.base.data; Any reason not to use memcpy for these assignments? https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:355: CKA_DERIVE, 0, CKD_NULL, NULL, NULL); This seems like it would be better with a refactor as I did with ECC so we don't have duplicate code with ssl3 https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:711: expectedGroup = ssl_GetECGroupForServerSocket(ss); This seems like an unnecessary rename https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:772: ssl_AddEphemeralKeyPair(ss, keyPair); ssl3_Add... https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:781: return SECFailure; /* Error code by the caller. */ By the caller? That seems wrong. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:795: return rv; /* Error code set by the caller. */ See above. You should always be doing PORT_SetError() before returningrv. Do you mean callee? https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:1185: PORT_Assert(PR_NEXT_LINK(&entry->link) == &ss->ssl3.hs.remoteKeyShares); This seems like an unnecessary change. I prefer the temporary I had here. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:1191: } Why don't you just assert this? You shouldn't be offering a key for a group that's not enabled and the check below checks that this is for a key we offered https://codereview.appspot.com/297180043/diff/20001/lib/util/secoid.c File lib/util/secoid.c (right): https://codereview.appspot.com/297180043/diff/20001/lib/util/secoid.c#newcode... lib/util/secoid.c:1717: ODE( SEC_OID_TLS_DHE_PSK, Man, this is crying out for another macro https://codereview.appspot.com/297180043/diff/20001/lib/util/secoid.c#newcode... lib/util/secoid.c:1718: "TLS HDE-PSK key exchange", CKM_INVALID_MECHANISM, INVALID_CERT_EXTENSION ), typo
Sign in to reply to this message.
More comments. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslenum.c File lib/ssl/sslenum.c (right): https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslenum.c#newcode55 lib/ssl/sslenum.c:55: TLS_DHE_PSK_WITH_AES_128_GCM_SHA256, Why does this depend on ECC? https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (left): https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#oldcode935 lib/ssl/sslimpl.h:935: #endif /* NSS_DISABLE_ECC */ Was this unused? https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (right): https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode184 lib/ssl/sslimpl.h:184: ffdhe_2048 = 256, I-D or RFC # please. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode188 lib/ssl/sslimpl.h:188: ffdhe_8192 = 260, These are named ffdhe# without underscore in the draft. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode205 lib/ssl/sslimpl.h:205: unsigned int bits; This seems like a place where things could go badly wrong. Where is it needed? https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode382 lib/ssl/sslimpl.h:382: unsigned int enableSignedCertTimestamps : 1; Unnecessary https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode... lib/ssl/sslimpl.h:1741: extern PRBool ssl_SuiteBOnly(sslSocket *ss); Unnecessary change. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode... lib/ssl/sslimpl.h:1784: const SECKEYPublicKey *pubKey); Unnecessary change? https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode... lib/ssl/sslimpl.h:1830: extern SECStatus ssl_HandleSupportedGroupsXtn(sslSocket *ss, No. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode... lib/ssl/sslimpl.h:1853: extern PRInt32 ssl_SendSupportedGroupsXtn(sslSocket *ss, No, https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode... lib/ssl/sslimpl.h:1990: sslEphemeralKeyPair **keyPair); No.
Sign in to reply to this message.
I have a patch ready, but I haven't split out the removal of dead options. I'll do that before posting an update. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslenum.c File lib/ssl/sslenum.c (right): https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslenum.c#newcode55 lib/ssl/sslenum.c:55: TLS_DHE_PSK_WITH_AES_128_GCM_SHA256, On 2016/05/10 09:42:46, ekr-webrtc wrote: > Why does this depend on ECC? Well, I was about to say that that was a mistake, but it's not. It is a TLS 1.3 DHE cipher suite and as such depends on having the supported_groups extension. And I haven't moved the supported_groups extension out from under the ECC conditional compilation. I can do so, but it requires a lot of moving code around. I should be more consistent and fix the other cases as well. Or, I could send the email that I just sent. It's the nuclear option, but I think that it's the right choice. p.s., I would rather remove the NSS_DISABLE_ECC conditional entirely, it's a source of headaches. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (left): https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#oldcode935 lib/ssl/sslimpl.h:935: #endif /* NSS_DISABLE_ECC */ On 2016/05/10 09:42:47, ekr-webrtc wrote: > Was this unused? No, but I replaced it with something better. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (right): https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode188 lib/ssl/sslimpl.h:188: ffdhe_8192 = 260, On 2016/05/10 09:42:47, ekr-webrtc wrote: > These are named ffdhe# without underscore in the draft. I know. I was trying to be a little more consistent with the ec_* ones. Note: this is private, so we can rename as we please. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode205 lib/ssl/sslimpl.h:205: unsigned int bits; On 2016/05/10 09:42:47, ekr-webrtc wrote: > This seems like a place where things could go badly wrong. Where is it needed? There is the insane EC code we use that tries to do the wrong thing with curve selection based on some combination of the symmetric key size and some other nonsense. The function called ssl3_GetCurveNameForServerSocket() that I named ssl_GetECGroupForServerSocket(). I really want to replace that logic but it's not quite broken enough to bubble to the top just yet. The ffdhe_ code doesn't use this value. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode382 lib/ssl/sslimpl.h:382: unsigned int enableSignedCertTimestamps : 1; On 2016/05/10 09:42:47, ekr-webrtc wrote: > Unnecessary Well, removing some bits that weren't being used seemed like a good idea once I cross the 32-bit threshold. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode... lib/ssl/sslimpl.h:1741: extern PRBool ssl_SuiteBOnly(sslSocket *ss); On 2016/05/10 09:42:47, ekr-webrtc wrote: > Unnecessary change. Being which, the name changes? The signatures all changed, so I figured that a name change to make them more accurate would be useful. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode... lib/ssl/sslimpl.h:1784: const SECKEYPublicKey *pubKey); On 2016/05/10 09:42:47, ekr-webrtc wrote: > Unnecessary change? I made the handling of the EC key shares consistent with the DHE handling, which uses public keys. There's also a principle that I could rely on here: http://c2.com/cgi/wiki?LawOfDemeter https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode... lib/ssl/sslimpl.h:1830: extern SECStatus ssl_HandleSupportedGroupsXtn(sslSocket *ss, On 2016/05/10 09:42:47, ekr-webrtc wrote: > No. Please use more words to aid my understanding. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (left): https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#oldcode73 lib/ssl/sslsock.c:73: /* v2CompatibleHello */ /* now defaults to off in NSS 3.13 */ On 2016/05/10 09:33:23, ekr-webrtc wrote: > Please break this change into another PR. OK. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode145 lib/ssl/sslsock.c:145: { 0, ec_secp192r1, 192, group_type_ec, ECOID(SECP192R1), PR_FALSE }, On 2016/05/10 09:33:23, ekr-webrtc wrote: > This is an odd choice to be first I have not changed the order Maybe this was implemented in assembler and was much faster than some of those that followed it. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode260 lib/ssl/sslsock.c:260: } On 2016/05/10 09:33:24, ekr-webrtc wrote: > Unnecessary change. Please break this into a separate PR so I can review just > the DH changes. Ack. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode403 lib/ssl/sslsock.c:403: ssl_FreeKeyPair(ss->stepDownKeyPair); On 2016/05/10 09:33:23, ekr-webrtc wrote: > I don't think it's a good idea to change ssl3_* to ssl_* For what reason? This isn't ssl3-specific code any more. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode802 lib/ssl/sslsock.c:802: /* This feature is not supported if ECC isn't. */ On 2016/05/10 09:33:23, ekr-webrtc wrote: > Why? It pulls in a bunch of the NSS_DISABLE_ECC-guarded code to do its work. See my mail. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1419: PORT_CheckSuccess(ssl3_CipherPrefSet(ss, *suite, PR_FALSE)); On 2016/05/10 09:33:23, ekr-webrtc wrote: > Are you sure this executes if asserts are off? Yes. I wrote the macro :) https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1422: } On 2016/05/10 09:33:23, ekr-webrtc wrote: > This seems pretty awful. I would far prefer a length. I might be able to do that... I'm not sure. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1467: PRUint32 mask; On 2016/05/10 09:33:23, ekr-webrtc wrote: > This isn't a mask. It's a bitvector. Suggest renaming to "supportedGroups" or > something. Done. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1491: mask = ss->namedGroups; On 2016/05/10 09:33:23, ekr-webrtc wrote: > PORT_Assert that this doesn't shift past the end. There is a static assert on the definition of ssl_named_group_count. Would that do? https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1515: break; On 2016/05/10 09:33:24, ekr-webrtc wrote: > Why do you need two separate values here? This seems like a recipe for problems. The public API that Kai already defined uses 0, 1, 2, ... The ffdhe draft defines 256, 257, ... The damage is contained to this function. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1568: gWeakDHParams->name = ffdhe_weak; On 2016/05/10 09:33:23, ekr-webrtc wrote: > It's kind of unfortunate that ffdhe_weak is a DSA-style prime whereas the other > primes are safe primes. Not your fault, though, I guess. Acknowledged. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1652: } On 2016/05/10 09:33:23, ekr-webrtc wrote: > Can we get a table or something? How does a table help? It's more code to write, and no clearer. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1658: * group (if this is successful). */ On 2016/05/10 09:33:23, ekr-webrtc wrote: > RFC cite, please. Done. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1671: On 2016/05/10 09:33:23, ekr-webrtc wrote: > This seems unnecessary. Since you need to check dh_p for the real group anywyy. Not following. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1682: PORT_Assert(params); On 2016/05/10 09:33:23, ekr-webrtc wrote: > Why not refactor this into a getter for the params. Shame I can't get hide ssl_GetDHEParams() as a result, but it's a nice idea anyway. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1710: return SECSuccess; On 2016/05/10 09:33:24, ekr-webrtc wrote: > PORT_Assert that gWeakDHParams is nonzero. Done. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1716: *params = ssl_GetDHEParams(&ssl_named_groups[i]); On 2016/05/10 09:33:23, ekr-webrtc wrote: > Refactor as above. Not following you. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1721: /* Caller calls PORT_SetError() */ On 2016/05/10 09:33:23, ekr-webrtc wrote: > This is not a good idiom I learned it from the best. Will fix. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:2043: return NULL; On 2016/05/10 09:33:23, ekr-webrtc wrote: > What happened here? This doesn't seem like it's relevant. Removed. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:2051: ss->stepDownKeyPair = ssl_GetKeyPairRef(sm->stepDownKeyPair); On 2016/05/10 09:33:23, ekr-webrtc wrote: > Unnecessary change. Acknowledged. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3450: } On 2016/05/10 09:33:23, ekr-webrtc wrote: > Why aren't you returning false if SECFailure? This is what the code does everywhere. I don't know why, but I haven't looked into it. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3458: PRUint32 mask = 0; On 2016/05/10 09:33:23, ekr-webrtc wrote: > Not a mask. Done. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3474: } On 2016/05/10 09:33:23, ekr-webrtc wrote: > Why are you just enabling 2048 by default? Consistency with the existing DHE code. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3498: return pair; /* success */ On 2016/05/10 09:33:23, ekr-webrtc wrote: > This seems to just be the same as ssl3_NewKeyPair. Unnecessary cahnge. It is the same code. I moved it here so that it could be with the ephemeral key stuff. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3630: if (!ss) { On 2016/05/10 09:33:23, ekr-webrtc wrote: > Unnecessary change. Done. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:259: unsigned int i; On 2016/05/10 09:33:24, ekr-webrtc wrote: > I don't think this is the right answer in the long term because we won't want to > absorb the FFDHE cost by default. Maybe you can hack in a conditional based on > one of the options temporarily? Yeah, I wanted to have this on by default until we did HRR. Would a comment here to make this smarter once we land HRR be acceptable? https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:266: for (i = 0; i < PR_ARRAY_SIZE(groups_to_try); ++i) { On 2016/05/10 09:33:24, ekr-webrtc wrote: > This is not going to be compatible with HRR. I would suggest at this point > having groups_to_try being a member with a length. I'm sure that that change can be made once we add HRR. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:280: rv = ssl_CreateDHEKeyPair(groupDef, params, &keyPair); On 2016/05/10 09:33:24, ekr-webrtc wrote: > Why can't this take a namedGroupDEf Fucking gWeakDHparams. The <= 1.2 code calls a different path to get the group parameters. Let me see if I can do something to fix it, but it takes a bit of wrangling. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:316: rv = tls13_HkdfExtractSharedKey(ss, shared, EphemeralSharedSecret); On 2016/05/10 09:33:24, ekr-webrtc wrote: > This seems like an unnecessary change. It saved me plumbing a constant through a new function. That's not unnecessary in my mind. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:339: } On 2016/05/10 09:33:24, ekr-webrtc wrote: > Please check that length == 0 Done. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:355: CKA_DERIVE, 0, CKD_NULL, NULL, NULL); On 2016/05/10 09:33:24, ekr-webrtc wrote: > This seems like it would be better with a refactor as I did with ECC so we don't > have duplicate code with ssl3 What overlaps exactly? I thought that your changes for ECC were to hide the ECC code in ssl3ecc.c. Here's the 1.2 code: if (isTLS) target = CKM_TLS_MASTER_KEY_DERIVE_DH; else target = CKM_SSL3_MASTER_KEY_DERIVE_DH; /* Determine the PMS */ pms = PK11_PubDerive(serverKeys->privKey, &clntPubKey, PR_FALSE, NULL, NULL, CKM_DH_PKCS_DERIVE, target, CKA_DERIVE, 0, NULL); https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:711: expectedGroup = ssl_GetECGroupForServerSocket(ss); On 2016/05/10 09:33:24, ekr-webrtc wrote: > This seems like an unnecessary rename I changed the signature, and changed the name to match. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:772: ssl_AddEphemeralKeyPair(ss, keyPair); On 2016/05/10 09:33:24, ekr-webrtc wrote: > ssl3_Add... Oh, we're going to have a great time with this one. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:795: return rv; /* Error code set by the caller. */ On 2016/05/10 09:33:24, ekr-webrtc wrote: > See above. You should always be doing PORT_SetError() before returningrv. Do you > mean callee? I have no idea what "below" meant in the original code. I assumed that it meant the caller of this function. Was I wrong? https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:1185: PORT_Assert(PR_NEXT_LINK(&entry->link) == &ss->ssl3.hs.remoteKeyShares); On 2016/05/10 09:33:24, ekr-webrtc wrote: > This seems like an unnecessary change. I prefer the temporary I had here. Your code didn't work. cur_p was always non-null, since an empty list points to itself. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:1191: } On 2016/05/10 09:33:24, ekr-webrtc wrote: > Why don't you just assert this? You shouldn't be offering a key for a group > that's not enabled and the check below checks that this is for a key we offered Done. https://codereview.appspot.com/297180043/diff/20001/lib/util/secoid.c File lib/util/secoid.c (right): https://codereview.appspot.com/297180043/diff/20001/lib/util/secoid.c#newcode... lib/util/secoid.c:1717: ODE( SEC_OID_TLS_DHE_PSK, On 2016/05/10 09:33:24, ekr-webrtc wrote: > Man, this is crying out for another macro Acknowledged. https://codereview.appspot.com/297180043/diff/20001/lib/util/secoid.c#newcode... lib/util/secoid.c:1718: "TLS HDE-PSK key exchange", CKM_INVALID_MECHANISM, INVALID_CERT_EXTENSION ), On 2016/05/10 09:33:24, ekr-webrtc wrote: > typo Done.
Sign in to reply to this message.
I got partway through sslsock.c. Going to need to pick up the rest tomorrow. https://codereview.appspot.com/297180043/diff/40001/cmd/selfserv/selfserv.c File cmd/selfserv/selfserv.c (right): https://codereview.appspot.com/297180043/diff/40001/cmd/selfserv/selfserv.c#n... cmd/selfserv/selfserv.c:1940: } PORT_Assert(configureDHE <= 2) https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:40: // This is the FFDHE 2048 group identifier. This comment isn't quite right because you included the length (nit). https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:46: // Requiring FFDHE on the server alone means that clients won't be able to It's not that you're requiring FFDHE but requiring the extsnsion https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:83: }; This appears to be unused which makes me wonder about test coverage :) https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:97: client_->CheckErrorCode(SSL_ERROR_WEAK_SERVER_EPHEMERAL_DH_KEY); server's error code? https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:103: // TLS 1.3 handshake to fail because the client add 3072 to its ClientHello. I assume you mean *only*? Because it's not a problem if it enables both, is it? I think partly it's just kinda lame that we don't pick the favorite enabled group, but I guess I understand it. https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/tls_agent.cc (right): https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/tls_agent.cc:239: void TlsAgent::EnableCiphersByAuthType(SSLAuthType authType) { Note: you have changed the behavior of this function. Is that your intent? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (left): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#oldcode... lib/ssl/ssl3con.c:5568: #endif /* NSS_DISABLE_ECC */ Hmm.. Are you sure? How do you disable ecc with SSLv3? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode851 lib/ssl/ssl3con.c:851: case ssl_kea_null: This is a little distressing. Do we really need it? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode867 lib/ssl/ssl3con.c:867: * already sent a ffdhe group. peerSupportsFfdhe is set to true in supportsffdhe seems like a bad name. It's sent an ffdhe group. Advertising dhe cipher suites means yous upport ffdhe. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode893 lib/ssl/ssl3con.c:893: if (authType == ssl_auth_null || authType == ssl_auth_psk) { I think I would prefer having a PSK check in the other location. Also why is this needed now when it wasn't needed before? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:1027: !tls13_AllowPskCipher(ss, cipher_def)) { Was there a bug here where we turned on PSK for non-TLS 1.3? It seems like the check already happens in tls13_AllowPskCipher https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:4200: * ssl_SendDHClientKeyExchange (for Full handshake) I think it's a mistake to rename these with ssl_. The convention in this file is ssl3_, so I would either leave or do that. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:6550: /* We already did this, but we need to know the group this time. */ This comment is confusing. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:6552: &svrPubKey->u.dh.base, Indent. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:6556: } else { IMPORTANT: This seems odd. You seem to be claiming that ffdhe_weak was used even if the peer coincidentally provided strong params. Specifically, this would report weak even if the other side provided ffdhe_4096. I think you want ffdhe_custom. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:7308: ssl_HandleRSAServerKeyExchange(sslSocket *ss, SSL3Opaque *b, PRUint32 length) Do we still need this? It seems like it's only required for export cipher suites, right? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:7435: SECItem dh_p = { siBuffer, NULL, 0 }; Why the whitespace? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:7471: if (ss->opt.requireDHENamedGroups || ss->ssl3.hs.peerSupportsFfdhe) { IMPORTANT: Can ss->ssl3.hs.peerSupportsFfDhe be set on the client? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:7487: } Compare Y to p-1 https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:7552: } I have no idea what this comment means https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:7629: return rv; Are you assuming that PORT_SetError() was called in the callee? A comment perhaps https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:8675: !ssl_NamedGroupEnabled(ss, cert->certType.u.namedCurve)) { This looks like it ignores the client's supported_groups extension. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:9069: errCode = PORT_GetError(); Does this ever change from malformed? Are you sure we want this? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:9880: if (!*keyPair) { Kinda ugh? Maybe a temporary. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:9925: ssl_AddEphemeralKeyPair(ss, keyPair); Not sure a function is merited here as opposed to just the PRC_LIST_APPEND https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:10712: sslKeyPair *serverKeys) I would call this keypair not keys, since you can have multiple keys https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:10743: pms = PK11_PubDerive(serverKeys->privKey, &clntPubKey, PR_FALSE, NULL, NULL, Shy are you not checking for bogus values here? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:10767: SECStatus rv; See above comment about keys https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:10785: PORT_Assert(kea_def->is_limited /* XXX OR cert is signing only */ Do we still need this stepdown stuff? Can we just get Bob to agree to dropping export instead? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:10824: serverKeys->privKey); Suggest changing this to take serverKeys for consistency. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:10848: return rv; Are you assuming PORT_SetError() has already been set? Maybe a comment. Don't any of these guys sent an alert? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (left): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#oldcode398 lib/ssl/ssl3ecc.c:398: publicValue = &pair->pubKey->u.ec.publicValue; Not sure removing the temporary is an improvement here. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode126 lib/ssl/ssl3ecc.c:126: return &ssl_named_groups[i]; What is offset here? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode255 lib/ssl/ssl3ecc.c:255: pubKey->u.ec.publicValue.len, 1); Do you understand why we previously destroyed these earlier? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode286 lib/ssl/ssl3ecc.c:286: return 1 + /* Length */ pubKey->u.ec.publicValue.len; I'm not sure this comment is clearer. Shouldn't the comment go before the + https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode308 lib/ssl/ssl3ecc.c:308: sslKeyPair *serverKeys) s/serverKeys/serverKeyPair/ https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode475 lib/ssl/ssl3ecc.c:475: return &ssl_named_groups[i]; to confirm: the named groups are in bits2curve order https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode554 lib/ssl/ssl3ecc.c:554: * the socket. Otherwise, the caller takes ownership. */ I don't see a socket argument so I wonder if this comment is wrong. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode570 lib/ssl/ssl3ecc.c:570: !(*keyPair = ssl_NewEphemeralKeyPair(ecGroup, privKey, pubKey))) { Suggest a temporary here. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode643 lib/ssl/ssl3ecc.c:643: ssl_AddEphemeralKeyPair(ss, ssl_CopyEphemeralKeyPair(keyPair)); PORT_Assert that the existing list is empty. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode806 lib/ssl/ssl3ecc.c:806: PORT_SetError(ssl_MapLowLevelError(errCode)); This seems not right. What error codes do you believe are being translated? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode825 lib/ssl/ssl3ecc.c:825: SECKEYPublicKey *pubKey; Can we assert that the list is empty? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode861 lib/ssl/ssl3ecc.c:861: ec_params.data[1] = keyPair->group->name >> 8; Isn't this always 0? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1136: unsigned char enabledCurves[64]; This seems like it would be better named enabledgroups and also would be better if it were tied to the total group count Seems like 64 may be too small? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1151: ss->ssl3.hs.peerSupportsFfdhe || When would peerSupportsFfdhe be set at this point? In renegotiation? The server isn't a sender here. Also, why isn't the client *always* sending this extension? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1256: /* Poor client doesn't support uncompressed points. */ It's actually illegal not to. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1378: ssl_DisableECSuitesBasedOnSupportedGroups(ss); You won't want to do this as a client. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:1221: } Unnecessary change. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:3095: return 2 + 2 + 2 + pubKey->u.dh.publicValue.len; Wouldn't it be nicer to have tls13_SizeofHDEKeyShareKEX() https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:3130: if (pubKey->keyType == ecKey) { What about a switch instead. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:3133: rv = ssl3_AppendHandshakeVariable(ss, pubKey->u.dh.publicValue.data, See above about wrapping this https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslimpl.h#newcode189 lib/ssl/sslimpl.h:189: ffdhe_weak = 511 /* special value */ As before, I think "custom" would be a better name, https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslimpl.h#newcode... lib/ssl/sslimpl.h:1697: SECKEYPrivateKey *privKey, SECKEYPublicKey *pubKey); This comment isn't correct because ssl_NewEphemeralKeyPair() doesn't add it to the list. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode150 lib/ssl/sslsock.c:150: const namedGroupDef ssl_named_groups[] = { NOTE TO SELF: NEed to check this list. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1511: count = PR_ARRAY_SIZE(default_dhe_groups); This null argument seems like a bad idea but I guess it was Kaiss
Sign in to reply to this message.
I got partway through sslsock.c. Going to need to pick up the rest tomorrow.
Sign in to reply to this message.
NOTE: This patch does not apply to master. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1538: break; Maybe a table? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1676: return NULL; A table seems like it would be fun here too. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1697: } Why don't you instead put this inside the loop at the point where you have successfully compared p. That way if g is ever not 2 it will still work https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1741: } If I am reading this code correctly you offer this group with TLS 1.2 even if the other side only offered 3072. That seems wrong. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3524: if (ssl_named_groups[i].type == group_type_ec && This is a funny comment since isn't suiteBOnly on by default https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3531: } So we only have ffdhe_2048 on by default? Why is that? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3600: PR_INIT_CLIST(&pair->link); Are you sure this is needed? I don't do it elsewere. Also below https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3657: } How can this happen? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:283: rv = ssl_CreateDHEKeyPair(groupDef, params, &keyPair); I think I said this before, but can we make this interface the same as ECDHE https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:351: peerKey.u.dh.base.data = keyPair->pubKey->u.dh.base.data; Can we have tls13_ImportDHEKeyShare? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:365: PK11_FreeSymKey(shared); This code seems like maybe it could be shared between ECDHE and DHE? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:784: return SECFailure; /* Error code by the caller. */ Callee. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:798: return rv; /* Error code set by the caller. */ Set by the callee.
Sign in to reply to this message.
I continue to believe that we should use ssl3_ uniformly. if you don't lets discuss on nss-dev pre-landing https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (right): https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslimpl.h#newcode... lib/ssl/sslimpl.h:1830: extern SECStatus ssl_HandleSupportedGroupsXtn(sslSocket *ss, On 2016/05/11 12:31:52, mt wrote: > On 2016/05/10 09:42:47, ekr-webrtc wrote: > > No. > > Please use more words to aid my understanding. This is just my whining about ssl_ versus ssl3_ https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1652: } On 2016/05/11 12:31:53, mt wrote: > On 2016/05/10 09:33:23, ekr-webrtc wrote: > > Can we get a table or something? > > How does a table help? It's more code to write, and no clearer. Then we can put all the values lined up in one place
Sign in to reply to this message.
I've updated this. I might add some tests later, but it's in a reasonable shape now, I think. https://codereview.appspot.com/297180043/diff/40001/cmd/selfserv/selfserv.c File cmd/selfserv/selfserv.c (right): https://codereview.appspot.com/297180043/diff/40001/cmd/selfserv/selfserv.c#n... cmd/selfserv/selfserv.c:1940: } On 2016/05/18 00:44:14, ekr-webrtc wrote: > PORT_Assert(configureDHE <= 2) Done. https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:40: // This is the FFDHE 2048 group identifier. On 2016/05/18 00:44:14, ekr-webrtc wrote: > This comment isn't quite right because you included the length (nit). Done. https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:46: // Requiring FFDHE on the server alone means that clients won't be able to On 2016/05/18 00:44:14, ekr-webrtc wrote: > It's not that you're requiring FFDHE but requiring the extsnsion Done. https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:83: }; On 2016/05/18 00:44:15, ekr-webrtc wrote: > This appears to be unused which makes me wonder about test coverage :) Ahh, I decided to enable the weak DH group instead of using this. But on consideration, both is probably a good idea, particularly since I want to remove the weak DH group. Thanks for picking that up. (As an aside, I need to spend some time on code coverage tools. Is that on Tim's list?) https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:97: client_->CheckErrorCode(SSL_ERROR_WEAK_SERVER_EPHEMERAL_DH_KEY); On 2016/05/18 00:44:15, ekr-webrtc wrote: > server's error code? Done. https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:103: // TLS 1.3 handshake to fail because the client add 3072 to its ClientHello. On 2016/05/18 00:44:15, ekr-webrtc wrote: > I assume you mean *only*? Because it's not a problem if it enables both, is it? > I think partly it's just kinda lame that we don't pick the favorite enabled > group, but I guess I understand it. Done. https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/tls_agent.cc (right): https://codereview.appspot.com/297180043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/tls_agent.cc:239: void TlsAgent::EnableCiphersByAuthType(SSLAuthType authType) { On 2016/05/18 00:44:15, ekr-webrtc wrote: > Note: you have changed the behavior of this function. Is that your intent? Now that we have multiple closely-related auth types that are enabled in tests, there was a subtle bug. client_->EnableCiphersByAuthType(ssl_auth_ecdh_ecdsa); client_->EnableCiphersByAuthType(ssl_auth_ecdh_rsa); Didn't work as expected. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (left): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#oldcode... lib/ssl/ssl3con.c:5568: #endif /* NSS_DISABLE_ECC */ On 2016/05/18 00:44:15, ekr-webrtc wrote: > Hmm.. Are you sure? How do you disable ecc with SSLv3? ssl3_CipherSuiteAllowedForVersionRange() covers this. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode851 lib/ssl/ssl3con.c:851: case ssl_kea_null: On 2016/05/18 00:44:15, ekr-webrtc wrote: > This is a little distressing. Do we really need it? Actually, no. I originally had this function called from ssl3_config_match_init, which was a bad idea, but there you have to permit ssl_kea_null (for consistency reasons, I don't actually think we need it). I will remove it. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode867 lib/ssl/ssl3con.c:867: * already sent a ffdhe group. peerSupportsFfdhe is set to true in On 2016/05/18 00:44:15, ekr-webrtc wrote: > supportsffdhe seems like a bad name. It's sent an ffdhe group. Advertising dhe > cipher suites means yous upport ffdhe. Yes, I'd been using "ffdhe" as synonymous with the extension, rather than "dhe". I'll add "groups" to the name. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode893 lib/ssl/ssl3con.c:893: if (authType == ssl_auth_null || authType == ssl_auth_psk) { On 2016/05/18 00:44:15, ekr-webrtc wrote: > I think I would prefer having a PSK check in the other location. Also why is > this needed now when it wasn't needed before? Because the PSK suites were marked as disabled always before, and we weren't checking that they were disabled when picking them. This is a fix ported from my other bug on being able to enable/disable PSK suites. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:1027: !tls13_AllowPskCipher(ss, cipher_def)) { On 2016/05/18 00:44:16, ekr-webrtc wrote: > Was there a bug here where we turned on PSK for non-TLS 1.3? It seems like the > check already happens in tls13_AllowPskCipher That's a good point. tls13_AllowPskCipher shouldn't duplicate that check. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:4200: * ssl_SendDHClientKeyExchange (for Full handshake) On 2016/05/18 00:44:16, ekr-webrtc wrote: > I think it's a mistake to rename these with ssl_. The convention in this file is > ssl3_, so I would either leave or do that. Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:6550: /* We already did this, but we need to know the group this time. */ On 2016/05/18 00:44:16, ekr-webrtc wrote: > This comment is confusing. Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:6552: &svrPubKey->u.dh.base, On 2016/05/18 00:44:15, ekr-webrtc wrote: > Indent. Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:6556: } else { On 2016/05/18 00:44:15, ekr-webrtc wrote: > IMPORTANT: This seems odd. You seem to be claiming that ffdhe_weak was used even > if the peer coincidentally provided strong params. Specifically, this would > report weak even if the other side provided ffdhe_4096. I think you want > ffdhe_custom. I was operating on the principle that custom == weak, but will add a custom value. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:7308: ssl_HandleRSAServerKeyExchange(sslSocket *ss, SSL3Opaque *b, PRUint32 length) On 2016/05/18 00:44:15, ekr-webrtc wrote: > Do we still need this? It seems like it's only required for export cipher > suites, right? It's used for all static RSA. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:7435: SECItem dh_p = { siBuffer, NULL, 0 }; On 2016/05/18 00:44:15, ekr-webrtc wrote: > Why the whitespace? I ran clang-format, I guess that's what happens when you do that. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:7471: if (ss->opt.requireDHENamedGroups || ss->ssl3.hs.peerSupportsFfdhe) { On 2016/05/18 00:44:16, ekr-webrtc wrote: > IMPORTANT: Can ss->ssl3.hs.peerSupportsFfDhe be set on the client? Yes, because I implemented the change where the server can send the extension in TLS 1.3. But that's a facetious response, because that doesn't change this code. I'll fix this. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:7487: } On 2016/05/18 00:44:15, ekr-webrtc wrote: > Compare Y to p-1 Done. I was also able to check that Ys < p for free. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:7552: } On 2016/05/18 00:44:15, ekr-webrtc wrote: > I have no idea what this comment means Nor do I. I only moved this code. (In this respect, reviewboard is superior to rietveld.) https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:7629: return rv; On 2016/05/18 00:44:15, ekr-webrtc wrote: > Are you assuming that PORT_SetError() was called in the callee? A comment > perhaps Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:8675: !ssl_NamedGroupEnabled(ss, cert->certType.u.namedCurve)) { On 2016/05/18 00:44:16, ekr-webrtc wrote: > This looks like it ignores the client's supported_groups extension. It doesn't. ss->namedGroups is updated when handling the extension. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:9069: errCode = PORT_GetError(); On 2016/05/18 00:44:15, ekr-webrtc wrote: > Does this ever change from malformed? Are you sure we want this? Take a look at each of the part2 functions. They each set a bunch of different error codes, but this function causes them to all reset. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:9880: if (!*keyPair) { On 2016/05/18 00:44:16, ekr-webrtc wrote: > Kinda ugh? Maybe a temporary. I still don't have a good model for what you find objectionable here. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:9925: ssl_AddEphemeralKeyPair(ss, keyPair); On 2016/05/18 00:44:16, ekr-webrtc wrote: > Not sure a function is merited here as opposed to just the PRC_LIST_APPEND I was thinking encapsulation. But you are right in observing that this is reducible. I think that I'll do that. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:10712: sslKeyPair *serverKeys) On 2016/05/18 00:44:15, ekr-webrtc wrote: > I would call this keypair not keys, since you can have multiple keys Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:10743: pms = PK11_PubDerive(serverKeys->privKey, &clntPubKey, PR_FALSE, NULL, NULL, On 2016/05/18 00:44:15, ekr-webrtc wrote: > Shy are you not checking for bogus values here? Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:10767: SECStatus rv; On 2016/05/18 00:44:15, ekr-webrtc wrote: > See above comment about keys Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:10785: PORT_Assert(kea_def->is_limited /* XXX OR cert is signing only */ On 2016/05/18 00:44:15, ekr-webrtc wrote: > Do we still need this stepdown stuff? > > Can we just get Bob to agree to dropping export instead? I am still waiting for Tim to land his patch. It will go, but I don't think that I need to worry about it here. It's only a minimal change here. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:10824: serverKeys->privKey); On 2016/05/18 00:44:16, ekr-webrtc wrote: > Suggest changing this to take serverKeys for consistency. Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:10848: return rv; On 2016/05/18 00:44:15, ekr-webrtc wrote: > Are you assuming PORT_SetError() has already been set? Maybe a comment. Done. > Don't any of these guys sent an alert? They do, but we have safeguards that prevent two fatal alerts from being sent and factoring this out seemed the easiest thing to do. I'll comment on that too. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (left): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#oldcode398 lib/ssl/ssl3ecc.c:398: publicValue = &pair->pubKey->u.ec.publicValue; On 2016/05/18 00:44:16, ekr-webrtc wrote: > Not sure removing the temporary is an improvement here. See earlier comment regarding models. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode126 lib/ssl/ssl3ecc.c:126: return &ssl_named_groups[i]; On 2016/05/18 00:44:16, ekr-webrtc wrote: > What is offset here? Magic. Mystery meat. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode255 lib/ssl/ssl3ecc.c:255: pubKey->u.ec.publicValue.len, 1); On 2016/05/18 00:44:16, ekr-webrtc wrote: > Do you understand why we previously destroyed these earlier? Paranoia is my best guess. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode286 lib/ssl/ssl3ecc.c:286: return 1 + /* Length */ pubKey->u.ec.publicValue.len; On 2016/05/18 00:44:16, ekr-webrtc wrote: > I'm not sure this comment is clearer. Shouldn't the comment go before the + Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode308 lib/ssl/ssl3ecc.c:308: sslKeyPair *serverKeys) On 2016/05/18 00:44:17, ekr-webrtc wrote: > s/serverKeys/serverKeyPair/ Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode475 lib/ssl/ssl3ecc.c:475: return &ssl_named_groups[i]; On 2016/05/18 00:44:16, ekr-webrtc wrote: > to confirm: the named groups are in bits2curve order Yes. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode554 lib/ssl/ssl3ecc.c:554: * the socket. Otherwise, the caller takes ownership. */ On 2016/05/18 00:44:16, ekr-webrtc wrote: > I don't see a socket argument so I wonder if this comment is wrong. Yep, that's very wrong now. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode570 lib/ssl/ssl3ecc.c:570: !(*keyPair = ssl_NewEphemeralKeyPair(ecGroup, privKey, pubKey))) { On 2016/05/18 00:44:16, ekr-webrtc wrote: > Suggest a temporary here. Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode643 lib/ssl/ssl3ecc.c:643: ssl_AddEphemeralKeyPair(ss, ssl_CopyEphemeralKeyPair(keyPair)); On 2016/05/18 00:44:16, ekr-webrtc wrote: > PORT_Assert that the existing list is empty. Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode806 lib/ssl/ssl3ecc.c:806: PORT_SetError(ssl_MapLowLevelError(errCode)); On 2016/05/18 00:44:16, ekr-webrtc wrote: > This seems not right. What error codes do you believe are being translated? Yeah, mistake on my part. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode825 lib/ssl/ssl3ecc.c:825: SECKEYPublicKey *pubKey; On 2016/05/18 00:44:16, ekr-webrtc wrote: > Can we assert that the list is empty? Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode861 lib/ssl/ssl3ecc.c:861: ec_params.data[1] = keyPair->group->name >> 8; On 2016/05/18 00:44:16, ekr-webrtc wrote: > Isn't this always 0? I don't like that sort of thing because it means that adding a group with a value of 550 would mean finding and changing this. This does no harm. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1136: unsigned char enabledCurves[64]; On 2016/05/18 00:44:16, ekr-webrtc wrote: > This seems like it would be better named enabledgroups and also would be better > if it were tied to the total group count Seems like 64 may be too small? 64 is plenty. We can't have more than 32 curves supported without overflowing the PRUint32 bit vector. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1151: ss->ssl3.hs.peerSupportsFfdhe || On 2016/05/18 00:44:16, ekr-webrtc wrote: > When would peerSupportsFfdhe be set at this point? In renegotiation? The server > isn't a sender here. Yes, the flag check isn't needed. > Also, why isn't the client *always* sending this extension? Well, we don't want to send it if neither ffdhe nor ec is enabled. I guess that we could say that we send it every time, but that doesn't help if we are only doing static RSA (for example). https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1256: /* Poor client doesn't support uncompressed points. */ On 2016/05/18 00:44:17, ekr-webrtc wrote: > It's actually illegal not to. Indeed. I'm changing this to return SECFailure here. We will fail the handshake if we get the extension and it doesn't include uncompressed. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1378: ssl_DisableECSuitesBasedOnSupportedGroups(ss); On 2016/05/18 00:44:16, ekr-webrtc wrote: > You won't want to do this as a client. I'm actually fairly sure that I don't want to do this at all now that I check supported groups in ssl_KEAEnabled(). https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:1221: } On 2016/05/18 00:44:17, ekr-webrtc wrote: > Unnecessary change. I'm sorry, I saw some ugly code and fixed it. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:3095: return 2 + 2 + 2 + pubKey->u.dh.publicValue.len; On 2016/05/18 00:44:17, ekr-webrtc wrote: > Wouldn't it be nicer to have tls13_SizeofHDEKeyShareKEX() I considered it, but I would prefer to inline tls13_SizeOfECDHEKeyShareKEX() if you think that consistency is valuable. That way we don't need to explain what all the values are in multiple places. This is about as clear as anything like this can be. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:3130: if (pubKey->keyType == ecKey) { On 2016/05/18 00:44:17, ekr-webrtc wrote: > What about a switch instead. Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:3133: rv = ssl3_AppendHandshakeVariable(ss, pubKey->u.dh.publicValue.data, On 2016/05/18 00:44:17, ekr-webrtc wrote: > See above about wrapping this Same response. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslimpl.h#newcode189 lib/ssl/sslimpl.h:189: ffdhe_weak = 511 /* special value */ On 2016/05/18 00:44:17, ekr-webrtc wrote: > As before, I think "custom" would be a better name, Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslimpl.h#newcode... lib/ssl/sslimpl.h:1697: SECKEYPrivateKey *privKey, SECKEYPublicKey *pubKey); On 2016/05/18 00:44:17, ekr-webrtc wrote: > This comment isn't correct because ssl_NewEphemeralKeyPair() doesn't add it to > the list. Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1511: count = PR_ARRAY_SIZE(default_dhe_groups); On 2016/05/18 00:44:17, ekr-webrtc wrote: > This null argument seems like a bad idea but I guess it was Kaiss Yes, and now we are kinda stuck with it. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1538: break; On 2016/05/18 22:57:27, ekr-webrtc wrote: > Maybe a table? A table doesn't really improve things, because then you need to search it. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1676: return NULL; On 2016/05/18 22:57:26, ekr-webrtc wrote: > A table seems like it would be fun here too. You have an odd idea about what fun is. See previous comment regarding searching. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1697: } On 2016/05/18 22:57:26, ekr-webrtc wrote: > Why don't you instead put this inside the loop at the point where you have > successfully compared p. That way if g is ever not 2 it will still work Good idea. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1741: } On 2016/05/18 22:57:26, ekr-webrtc wrote: > If I am reading this code correctly you offer this group with TLS 1.2 even if > the other side only offered 3072. That seems wrong. "only offered 3072" is confusing here. You mean that if the client offers a DHE group that we support in supported_groups, we should use that, even if weak groups are enabled? I'll change this to avoid the weak group if the client has indicates that it supports the ffdhe groups. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3524: if (ssl_named_groups[i].type == group_type_ec && On 2016/05/18 22:57:26, ekr-webrtc wrote: > > This is a funny comment since isn't suiteBOnly on by default Removed it. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3531: } On 2016/05/18 22:57:26, ekr-webrtc wrote: > So we only have ffdhe_2048 on by default? Why is that? Backward compatibility. Kai decided that we would have only 2048 enabled by default. I was reluctant to change that. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3600: PR_INIT_CLIST(&pair->link); On 2016/05/18 22:57:27, ekr-webrtc wrote: > Are you sure this is needed? I don't do it elsewere. Also below I was being paranoid is all. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3657: } On 2016/05/18 22:57:26, ekr-webrtc wrote: > How can this happen? Yeah, it can't. As you suggested elsewhere, I've moved to a direct APPEND_LINK in place of this function. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:283: rv = ssl_CreateDHEKeyPair(groupDef, params, &keyPair); On 2016/05/18 22:57:27, ekr-webrtc wrote: > I think I said this before, but can we make this interface the same as ECDHE You did, and I explained that we can't accept a custom group from the server without permitting the extra parameter. Once we get rid of custom groups, this will be fine. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:351: peerKey.u.dh.base.data = keyPair->pubKey->u.dh.base.data; On 2016/05/18 22:57:27, ekr-webrtc wrote: > Can we have tls13_ImportDHEKeyShare? Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:365: PK11_FreeSymKey(shared); On 2016/05/18 22:57:27, ekr-webrtc wrote: > This code seems like maybe it could be shared between ECDHE and DHE? Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:784: return SECFailure; /* Error code by the caller. */ On 2016/05/18 22:57:27, ekr-webrtc wrote: > Callee. Done. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:798: return rv; /* Error code set by the caller. */ On 2016/05/18 22:57:27, ekr-webrtc wrote: > Set by the callee. Done.
Sign in to reply to this message.
Shouldn't have rebased, but here the new stuff is here: https://codereview.appspot.com/295300043/
Sign in to reply to this message.
Dude, what is going on? There's all sorts of totally irrelevant stuff here. Is this a rebase or..? https://codereview.appspot.com/297180043/diff/40001/cmd/tstclnt/tstclnt.c File cmd/tstclnt/tstclnt.c (right): https://codereview.appspot.com/297180043/diff/40001/cmd/tstclnt/tstclnt.c#new... cmd/tstclnt/tstclnt.c:242: fprintf(stderr, "%-20s Enable the extended master secret extension (session hash).\n", "-G"); What happened here?
Sign in to reply to this message.
It is a rebase. See my last comment. On 29 May 2016 4:26 AM, <ekr-webrtc@rtfm.com> wrote: > Dude, what is going on? There's all sorts of totally irrelevant stuff > here. Is this a rebase or..? > > > https://codereview.appspot.com/297180043/diff/40001/cmd/tstclnt/tstclnt.c > File cmd/tstclnt/tstclnt.c (right): > > > https://codereview.appspot.com/297180043/diff/40001/cmd/tstclnt/tstclnt.c#new... > cmd/tstclnt/tstclnt.c:242: fprintf(stderr, "%-20s Enable the extended > master secret extension (session hash).\n", "-G"); > What happened here? > > https://codereview.appspot.com/297180043/ >
Sign in to reply to this message.
OK, I guess I misread the comment. This is going to be a real pain to review.
Sign in to reply to this message.
It looks like something went wrong with the rebase. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:7308: ssl_HandleRSAServerKeyExchange(sslSocket *ss, SSL3Opaque *b, PRUint32 length) On 2016/05/21 00:38:48, mt wrote: > On 2016/05/18 00:44:15, ekr-webrtc wrote: > > Do we still need this? It seems like it's only required for export cipher > > suites, right? > > It's used for all static RSA. That seems very unlikely. I assume you are thinking of ssl3_HandleRSAClientKeyExchange. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:9047: #endif IMPORTANT: Where is this checked now? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:9608: ssl3_FilterECCipherSuitesByServerCerts(ss); See above comment https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:9880: if (!*keyPair) { On 2016/05/21 00:38:48, mt wrote: > On 2016/05/18 00:44:16, ekr-webrtc wrote: > > Kinda ugh? Maybe a temporary. > > I still don't have a good model for what you find objectionable here. Since this is basically a hack around having multiple return values, I prefer to assign to it only on success. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:10785: PORT_Assert(kea_def->is_limited /* XXX OR cert is signing only */ On 2016/05/21 00:38:48, mt wrote: > On 2016/05/18 00:44:15, ekr-webrtc wrote: > > Do we still need this stepdown stuff? > > > > Can we just get Bob to agree to dropping export instead? > > I am still waiting for Tim to land his patch. It will go, but I don't think > that I need to worry about it here. It's only a minimal change here. OK, please get Tim to review this section of code. I don't understand it well and didn't review it. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1136: unsigned char enabledCurves[64]; On 2016/05/21 00:38:49, mt wrote: > On 2016/05/18 00:44:16, ekr-webrtc wrote: > > This seems like it would be better named enabledgroups and also would be > better > > if it were tied to the total group count Seems like 64 may be too small? > > 64 is plenty. We can't have more than 32 curves supported without overflowing > the PRUint32 bit vector. Is there an assert somewhere to catch this? https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1364: } Can you walk me through the logic that replaces this deleted code and the code above. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ext.c#newcode... lib/ssl/ssl3ext.c:3095: return 2 + 2 + 2 + pubKey->u.dh.publicValue.len; On 2016/05/21 00:38:50, mt wrote: > On 2016/05/18 00:44:17, ekr-webrtc wrote: > > Wouldn't it be nicer to have tls13_SizeofHDEKeyShareKEX() > > I considered it, but I would prefer to inline tls13_SizeOfECDHEKeyShareKEX() if > you think that consistency is valuable. That way we don't need to explain what > all the values are in multiple places. This is about as clear as anything like > this can be. Let's leave it because we're going to be rewriting this with -13 anyway https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:283: rv = ssl_CreateDHEKeyPair(groupDef, params, &keyPair); On 2016/05/21 00:38:50, mt wrote: > On 2016/05/18 22:57:27, ekr-webrtc wrote: > > I think I said this before, but can we make this interface the same as ECDHE > > You did, and I explained that we can't accept a custom group from the server > without permitting the extra parameter. Once we get rid of custom groups, this > will be fine. OK https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:503: return PR_FALSE; IMPORTANT: Why is this safe to remove https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:94: client_->CheckErrorCode(SSL_ERROR_WEAK_SERVER_EPHEMERAL_DH_KEY); Why does XORing with 73 cause a weak key You're not changing hte high bit. https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:125: offset += 2 + dh_len; This is reading g, right? https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:130: << "Length of dh_Ys must equal length of dh_p"; This isn't a guarantee. Do we pad Y out in TLS 1.2 and below? https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:149: --output->data()[offset + p.len() - 1]; Why not just set this to 0xfe? https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:150: break; Note: this only works with specially constructed p https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:216: Connect(); Why doesn't this fail? I thought that the weak group on the server was 1024. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/dtlscon.c File lib/ssl/dtlscon.c (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/dtlscon.c#newcode349 lib/ssl/dtlscon.c:349: rv = ssl3_HandleHandshakeMessage(ss, buf.buf, ss->ssl3.hs.msg_len); IMPORTANT: This is a rebase failure. You lost changes from one of my commits. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:1070: return PR_FALSE; Do you think it would make sense to have a IsFooEnabled function with a function pointer that does the check? https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:4218: * ssls_SendDHClientKeyExchange (for Full handshake) NIT: ssl3 https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:6555: pad -= chunkLen; Given that it will be very rare to pad >2 bytes, I would suggest just padding one byte at a time, this will let you skip this PR_MIN logic. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3ecc.c#newcode382 lib/ssl/ssl3ecc.c:382: { Why did you go to an explicit arena here? https://codereview.appspot.com/297180043/diff/60001/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/sslimpl.h#newcode342 lib/ssl/sslimpl.h:342: #define ssl_V3_SUITES_IMPLEMENTED 39 What two did we lose here? https://codereview.appspot.com/297180043/diff/60001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1679: } IMPORTANT: 1. This comment does not match the code, which validates <, not == 2. You need to check bit length, not length, because otherwise if you have a non-standard prime you could still have p<Y https://codereview.appspot.com/297180043/diff/60001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1706: } This seems complicated. I would just compute p-1 and compare. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:340: return peerKey; Is there a way to merge this with the EC code? Maybe with draft-13 https://codereview.appspot.com/297180043/diff/60001/lib/ssl/tls13hkdf.c File lib/ssl/tls13hkdf.c (left): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/tls13hkdf.c#oldco... lib/ssl/tls13hkdf.c:103: PRUint8 info[110]; IMPORTANT: This looks like a rebase failure https://codereview.appspot.com/297180043/diff/60001/tests/ssl/ssl.sh File tests/ssl/ssl.sh (right): https://codereview.appspot.com/297180043/diff/60001/tests/ssl/ssl.sh#newcode93 tests/ssl/ssl.sh:93: CIPHER_SUITES="-c :0016:0032:0033:0038:0039:003B:003C:003D:0040:0041:0067:006A:006B:0084:009C:009E:00A2:CCAAcdefgijklmnvyz" MT: did you change this? does it need review? https://codereview.appspot.com/297180043/diff/60001/tests/ssl/sslcov.txt File tests/ssl/sslcov.txt (left): https://codereview.appspot.com/297180043/diff/60001/tests/ssl/sslcov.txt#oldc... tests/ssl/sslcov.txt:167: ECC TLS12 :CCA8 TLS12_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 MT: Did you change this file?
Sign in to reply to this message.
https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:9047: #endif On 2016/05/29 22:08:25, ekr-webrtc wrote: > IMPORTANT: Where is this checked now? It gets checked in config_match() https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:9608: ssl3_FilterECCipherSuitesByServerCerts(ss); On 2016/05/29 22:08:25, ekr-webrtc wrote: > See above comment Ack. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:9880: if (!*keyPair) { On 2016/05/29 22:08:25, ekr-webrtc wrote: > On 2016/05/21 00:38:48, mt wrote: > > On 2016/05/18 00:44:16, ekr-webrtc wrote: > > > Kinda ugh? Maybe a temporary. > > > > I still don't have a good model for what you find objectionable here. > > Since this is basically a hack around having multiple return values, I prefer to > assign to it only on success. Of course. That's obvious. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:10785: PORT_Assert(kea_def->is_limited /* XXX OR cert is signing only */ On 2016/05/29 22:08:25, ekr-webrtc wrote: > On 2016/05/21 00:38:48, mt wrote: > > On 2016/05/18 00:44:15, ekr-webrtc wrote: > > > Do we still need this stepdown stuff? > > > > > > Can we just get Bob to agree to dropping export instead? > > > > I am still waiting for Tim to land his patch. It will go, but I don't think > > that I need to worry about it here. It's only a minimal change here. > > OK, please get Tim to review this section of code. I don't understand it well > and didn't review it. I'm hoping to have Tim review all of this stuff. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1136: unsigned char enabledCurves[64]; On 2016/05/29 22:08:25, ekr-webrtc wrote: > On 2016/05/21 00:38:49, mt wrote: > > On 2016/05/18 00:44:16, ekr-webrtc wrote: > > > This seems like it would be better named enabledgroups and also would be > > better > > > if it were tied to the total group count Seems like 64 may be too small? > > > > 64 is plenty. We can't have more than 32 curves supported without overflowing > > the PRUint32 bit vector. > > Is there an assert somewhere to catch this? Yes. See line 186 of sslsock.c (below the new table). https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1364: } On 2016/05/29 22:08:25, ekr-webrtc wrote: > Can you walk me through the logic that replaces this deleted code and the code > above. The old code used to process extensions, then remove cipher suites that don't fit. If we don't have a common curve, eliminate all the ECC suites, that sort of thing. This function. The new code simply processes the named curves and puts them in supportedGroups. When it comes to choosing a cipher suite, any suite that depends on a named group will be skipped when config_match() returns false. I've added a call to the new ssl_KEAEnabled() function there that will check if there is a named group of the right type enabled. I just realized that we should also have a more precise filtering of suites based on the certificates we have. We only do coarse filtering based on whether we have the right type of certificate, but the new certificate stuff I added allows us to filter based on the specific curves. I will add that. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:503: return PR_FALSE; On 2016/05/29 22:08:25, ekr-webrtc wrote: > IMPORTANT: Why is this safe to remove Now that this check isn't terminal (you used to return the value of this function), this line duplicates the identical check in ssl3_CipherSuiteAllowedForVersionRange() in ssl3con.c.
Sign in to reply to this message.
https://bugzilla.mozilla.org/show_bug.cgi?id=1276546 https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:1070: return PR_FALSE; On 2016/05/29 22:08:26, ekr-webrtc wrote: > Do you think it would make sense to have a IsFooEnabled function with a function > pointer that does the check? For this, DHE, and ECC? Maybe. I'll open a bug on it. That will let us do better than O(N^2) for this, which is pretty gross.
Sign in to reply to this message.
This does not apply against origin/master.
Sign in to reply to this message.
Thanks for rebasing, but this seems to be missing the rest of my review comments. Can you address those please?
Sign in to reply to this message.
On 2016/05/31 12:22:29, ekr-webrtc wrote: > Thanks for rebasing, but this seems to be missing the rest of my review > comments. Can you address those please? Did you save some comments and didn't send them? I checked through all of the comments and couldn't find any that I hadn't addressed. I did find a tiny omission that is probably harmless that I'll upload. It's frustrating, but apparently clang-format doesn't do anything on my machine, so there's some of that going on too.
Sign in to reply to this message.
https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:94: client_->CheckErrorCode(SSL_ERROR_WEAK_SERVER_EPHEMERAL_DH_KEY); On 2016/05/29 22:08:25, ekr-webrtc wrote: > Why does XORing with 73 cause a weak key You're not changing hte high bit. Here's something you missed. does that help?
Sign in to reply to this message.
Dammit, I had a long explanation of what changed and lost it... https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:94: client_->CheckErrorCode(SSL_ERROR_WEAK_SERVER_EPHEMERAL_DH_KEY); On 2016/05/29 22:08:25, ekr-webrtc wrote: > Why does XORing with 73 cause a weak key You're not changing hte high bit. Anything that alters the prime causes us to believe that it's weak. The prime has to match. I'll comment to that effect. https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:125: offset += 2 + dh_len; On 2016/05/29 22:08:25, ekr-webrtc wrote: > This is reading g, right? Skipping, yes. https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:130: << "Length of dh_Ys must equal length of dh_p"; On 2016/05/29 22:08:26, ekr-webrtc wrote: > This isn't a guarantee. Do we pad Y out in TLS 1.2 and below? We do now. https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:149: --output->data()[offset + p.len() - 1]; On 2016/05/29 22:08:25, ekr-webrtc wrote: > Why not just set this to 0xfe? Pursuant your comment below, I have asserted that the last octet of P is odd and then kept the --. That keeps this code generic. https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:216: Connect(); On 2016/05/29 22:08:25, ekr-webrtc wrote: > Why doesn't this fail? > > I thought that the weak group on the server was 1024. As the comment above the test says, the weak DH group is enabled, but the server won't use it when it detects that the client supports named groups for FFDHE. It only uses the weak DH group if the client doesn't include an FFDHE group in supported_groups. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/dtlscon.c File lib/ssl/dtlscon.c (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/dtlscon.c#newcode349 lib/ssl/dtlscon.c:349: rv = ssl3_HandleHandshakeMessage(ss, buf.buf, ss->ssl3.hs.msg_len); On 2016/05/29 22:08:26, ekr-webrtc wrote: > IMPORTANT: This is a rebase failure. You lost changes from one of my commits. Odd, that's not what the change shows on my copy of the file. I guess that rietveld is confused. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:4218: * ssls_SendDHClientKeyExchange (for Full handshake) On 2016/05/29 22:08:26, ekr-webrtc wrote: > NIT: ssl3 Done. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:6555: pad -= chunkLen; On 2016/05/29 22:08:26, ekr-webrtc wrote: > Given that it will be very rare to pad >2 bytes, I would suggest just padding > one byte at a time, this will let you skip this PR_MIN logic. Sure. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3ecc.c#newcode382 lib/ssl/ssl3ecc.c:382: { On 2016/05/29 22:08:26, ekr-webrtc wrote: > Why did you go to an explicit arena here? Consistency with the DH stuff. It lets me share the arena management and error handling code, which simplifies these functions a fair bit. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/sslimpl.h#newcode342 lib/ssl/sslimpl.h:342: #define ssl_V3_SUITES_IMPLEMENTED 39 On 2016/05/29 22:08:26, ekr-webrtc wrote: > What two did we lose here? I think that's rietveld getting confused again. Those changes are the reverse of the changes that Elio made for doing AES-GCM-256. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1679: } On 2016/05/29 22:08:26, ekr-webrtc wrote: > IMPORTANT: > 1. This comment does not match the code, which validates <, not == > 2. You need to check bit length, not length, because otherwise if you have a > non-standard prime you could still have p<Y Yeah, I originally required that they were the same size, but we can't rely on that. Given that these can both be independently padded, I think that any check like that is not useful. I will try to work out a better check. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1706: } On 2016/05/29 22:08:26, ekr-webrtc wrote: > This seems complicated. I would just compute p-1 and compare. We don't include mpi.h from lib/ssl, so I didn't want to start doing that. This isn't that complicated. I have some changes that should make it easier to follow. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:340: return peerKey; On 2016/05/29 22:08:26, ekr-webrtc wrote: > Is there a way to merge this with the EC code? Maybe with draft-13 Maybe. There is already quite a bit of overlap. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/tls13hkdf.c File lib/ssl/tls13hkdf.c (left): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/tls13hkdf.c#oldco... lib/ssl/tls13hkdf.c:103: PRUint8 info[110]; On 2016/05/29 22:08:26, ekr-webrtc wrote: > IMPORTANT: This looks like a rebase failure rietveld failure. again https://codereview.appspot.com/297180043/diff/60001/tests/ssl/ssl.sh File tests/ssl/ssl.sh (right): https://codereview.appspot.com/297180043/diff/60001/tests/ssl/ssl.sh#newcode93 tests/ssl/ssl.sh:93: CIPHER_SUITES="-c :0016:0032:0033:0038:0039:003B:003C:003D:0040:0041:0067:006A:006B:0084:009C:009E:00A2:CCAAcdefgijklmnvyz" On 2016/05/29 22:08:26, ekr-webrtc wrote: > MT: did you change this? does it need review? This is rietveld again. https://codereview.appspot.com/297180043/diff/60001/tests/ssl/sslcov.txt File tests/ssl/sslcov.txt (left): https://codereview.appspot.com/297180043/diff/60001/tests/ssl/sslcov.txt#oldc... tests/ssl/sslcov.txt:167: ECC TLS12 :CCA8 TLS12_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 On 2016/05/29 22:08:26, ekr-webrtc wrote: > MT: Did you change this file? Nope. Rietveld!
Sign in to reply to this message.
Do we have agreement on removing the ECC ifdef? This seems pretty close, but I'd like to see another version just because some of the code is tricky https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:94: client_->CheckErrorCode(SSL_ERROR_WEAK_SERVER_EPHEMERAL_DH_KEY); On 2016/06/01 13:02:56, mt wrote: > On 2016/05/29 22:08:25, ekr-webrtc wrote: > > Why does XORing with 73 cause a weak key You're not changing hte high bit. > > Anything that alters the prime causes us to believe that it's weak. The prime > has to match. I'll comment to that effect. As we discussed in IM, you should have separate tests for key validity even when custom groups are in use. https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:149: --output->data()[offset + p.len() - 1]; On 2016/06/01 13:02:56, mt wrote: > On 2016/05/29 22:08:25, ekr-webrtc wrote: > > Why not just set this to 0xfe? > > Pursuant your comment below, I have asserted that the last octet of P is odd and > then kept the --. That keeps this code generic. OK, a comment here perhaps. https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:216: Connect(); On 2016/06/01 13:02:56, mt wrote: > On 2016/05/29 22:08:25, ekr-webrtc wrote: > > Why doesn't this fail? > > > > I thought that the weak group on the server was 1024. > > As the comment above the test says, the weak DH group is enabled, but the server > won't use it when it detects that the client supports named groups for FFDHE. > It only uses the weak DH group if the client doesn't include an FFDHE group in > supported_groups. Acknowledged. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c#newcode483 lib/ssl/ssl3con.c:483: #endif /* NSS_DISABLE_ECC */ I am assuming that the above isn't changed except for the addition of PSK. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:1070: return PR_FALSE; On 2016/05/30 02:29:37, mt wrote: > On 2016/05/29 22:08:26, ekr-webrtc wrote: > > Do you think it would make sense to have a IsFooEnabled function with a > function > > pointer that does the check? > > For this, DHE, and ECC? Maybe. I'll open a bug on it. That will let us do > better than O(N^2) for this, which is pretty gross. SGTM. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:6555: pad -= chunkLen; On 2016/06/01 13:02:56, mt wrote: > On 2016/05/29 22:08:26, ekr-webrtc wrote: > > Given that it will be very rare to pad >2 bytes, I would suggest just padding > > one byte at a time, this will let you skip this PR_MIN logic. > > Sure. Are you planning to do this? https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3ecc.c#newcode382 lib/ssl/ssl3ecc.c:382: { On 2016/06/01 13:02:56, mt wrote: > On 2016/05/29 22:08:26, ekr-webrtc wrote: > > Why did you go to an explicit arena here? > > Consistency with the DH stuff. It lets me share the arena management and error > handling code, which simplifies these functions a fair bit. Acknowledged. https://codereview.appspot.com/297180043/diff/80001/lib/ssl/derive.c File lib/ssl/derive.c (right): https://codereview.appspot.com/297180043/diff/80001/lib/ssl/derive.c#newcode797 lib/ssl/derive.c:797: ssl_GetECGroupWithStrength(0xffffffffUL, NIT: I wonder if a constant would be better than this explicit hex value. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:1003: * the given version range. */ This comment needs to change to remove the bit about enabled state. Also, can you explain to me why that's no longer applicable? It's kind of odd code in the first place, since if I am reading it correctly, enabled == FALSE will cause the code to always return PR_FALSE. My inspection suggests that enabled was never set to PR_FALSE? https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:7633: if (!ssl_ValidDHEShare(&dh_p, &dh_Ys)) { NIT: IsValidKeyShare https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:10855: ssl_MapLowLevelError(SSL_ERROR_RX_MALFORMED_DHE_KEY_SHARE); Do you still need to map low level error here rather than PORT_SetError. That's not how you have it above. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:389: PORT_SetError(SSL_ERROR_RX_MALFORMED_ECDHE_KEY_SHARE); Why did you remove the tls13_FatalError() here? https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:390: return rv; return SECFailure here for consistency. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:409: return rv; Return SECFailure here and below. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1099: * no point if we aren't going to do TLS 1.3 only (client) or we have s/aren't/are/? https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1103: (!ss->sec.isServer && ss->vrange.min >= SSL_LIBRARY_VERSION_TLS_1_3)) I don't think you need the !ss->sec.isServer on this line because if you are a server and min > 1.3, then the previous line will fire (or the handshake will fail) https://codereview.appspot.com/297180043/diff/100001/lib/ssl/sslcert.h File lib/ssl/sslcert.h (right): https://codereview.appspot.com/297180043/diff/100001/lib/ssl/sslcert.h#newcode29 lib/ssl/sslcert.h:29: const namedGroupDef *namedCurve; Should we name this namedGroup? https://codereview.appspot.com/297180043/diff/100001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1650: c = dh_p->len - dh_Ys->len; len is unsigned int and this value will not be representable as a signed int. Per C99 6.3.1.3 this is an implementation defined converstion and hence not portable. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1675: } Don't you need to check for *y > *c? Why not just use PORT_Memcmp() here? https://codereview.appspot.com/297180043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1686: return PR_TRUE; I think there may be a more straightforward. 1. Check that p is odd. 2. Check that Y > 1 2. If bits(p) > bits(Y) then return because Y < p-1 (because an odd number #1 has the same number of bits) 3. If bits(Y) > bits(p) then Y>p and it's invalid. 4. Take the lower ((bits)/8) + !!(bits%8) bytes of Y and p [this value must be the same] and PORT_memcmp(). If Y >= p, then it's invalid. 5. At this point, p > Y, so now just check p[last]-1 >= Y[last] https://codereview.appspot.com/297180043/diff/100001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/297180043/diff/100001/lib/ssl/tls13con.c#newco... lib/ssl/tls13con.c:366: return rv; return SECFailure here and above https://codereview.appspot.com/297180043/diff/100001/lib/ssl/tls13con.c#newco... lib/ssl/tls13con.c:383: PORT_InitCheapArena(&arena, DER_DEFAULT_CHUNKSIZE); This is an odd value to use.
Sign in to reply to this message.
Partial response, I have spent most of my time writing up new tests. https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_dhe_unittest.cc:130: << "Length of dh_Ys must equal length of dh_p"; On 2016/06/01 13:02:56, mt wrote: > On 2016/05/29 22:08:26, ekr-webrtc wrote: > > This isn't a guarantee. Do we pad Y out in TLS 1.2 and below? > > We do now. Actually, in my testing, I had a test fail because I missed a share that should have been padded. Good thing that I have lots of tests. That 1/256 actually happened. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c#newcode483 lib/ssl/ssl3con.c:483: #endif /* NSS_DISABLE_ECC */ On 2016/06/01 18:08:51, ekr-webrtc wrote: > I am assuming that the above isn't changed except for the addition of PSK. Yes, the other changes are Elio's. https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:6555: pad -= chunkLen; On 2016/06/01 18:08:50, ekr-webrtc wrote: > On 2016/06/01 13:02:56, mt wrote: > > On 2016/05/29 22:08:26, ekr-webrtc wrote: > > > Given that it will be very rare to pad >2 bytes, I would suggest just > padding > > > one byte at a time, this will let you skip this PR_MIN logic. > > > > Sure. > > Are you planning to do this? I did. It is in the latest patch set. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:1003: * the given version range. */ On 2016/06/01 18:08:51, ekr-webrtc wrote: > This comment needs to change to remove the bit about enabled state. > > Also, can you explain to me why that's no longer applicable? It's kind of odd > code in the first place, since if I am reading it correctly, enabled == FALSE > will cause the code to always return PR_FALSE. My inspection suggests that > enabled was never set to PR_FALSE? Yes, enabled was never set.
Sign in to reply to this message.
I added some more tests. I also discovered that I didn't properly pad all the dh_Ys values that we send, thanks to running the extra tests a few times. That manifested as an intermittent failure, which was fun. New revision up. https://codereview.appspot.com/297180043/diff/80001/lib/ssl/derive.c File lib/ssl/derive.c (right): https://codereview.appspot.com/297180043/diff/80001/lib/ssl/derive.c#newcode797 lib/ssl/derive.c:797: ssl_GetECGroupWithStrength(0xffffffffUL, On 2016/06/01 18:08:51, ekr-webrtc wrote: > NIT: I wonder if a constant would be better than this explicit hex value. Done: PR_UINT32_MAX. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:7633: if (!ssl_ValidDHEShare(&dh_p, &dh_Ys)) { On 2016/06/01 18:08:51, ekr-webrtc wrote: > NIT: IsValidKeyShare Done. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:10855: ssl_MapLowLevelError(SSL_ERROR_RX_MALFORMED_DHE_KEY_SHARE); On 2016/06/01 18:08:51, ekr-webrtc wrote: > Do you still need to map low level error here rather than PORT_SetError. That's > not how you have it above. Done. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:389: PORT_SetError(SSL_ERROR_RX_MALFORMED_ECDHE_KEY_SHARE); On 2016/06/01 18:08:51, ekr-webrtc wrote: > Why did you remove the tls13_FatalError() here? It is invoked by tls13_HandleKeyShare(), which already did it, just with a different error code, which ended up overriding this one. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:390: return rv; On 2016/06/01 18:08:51, ekr-webrtc wrote: > return SECFailure here for consistency. Done. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:409: return rv; On 2016/06/01 18:08:51, ekr-webrtc wrote: > Return SECFailure here and below. Done. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1099: * no point if we aren't going to do TLS 1.3 only (client) or we have On 2016/06/01 18:08:51, ekr-webrtc wrote: > s/aren't/are/? Done. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1103: (!ss->sec.isServer && ss->vrange.min >= SSL_LIBRARY_VERSION_TLS_1_3)) On 2016/06/01 18:08:51, ekr-webrtc wrote: > I don't think you need the !ss->sec.isServer on this line because if you are a > server and min > 1.3, then the previous line will fire (or the handshake will > fail) Done. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/sslcert.h File lib/ssl/sslcert.h (right): https://codereview.appspot.com/297180043/diff/100001/lib/ssl/sslcert.h#newcode29 lib/ssl/sslcert.h:29: const namedGroupDef *namedCurve; On 2016/06/01 18:08:51, ekr-webrtc wrote: > Should we name this namedGroup? It only applies to ECC, so namedCurve is OK I think. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1675: } On 2016/06/01 18:08:51, ekr-webrtc wrote: > Don't you need to check for *y > *c? Yes. > Why not just use PORT_Memcmp() here? Yes. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1686: return PR_TRUE; On 2016/06/01 18:08:51, ekr-webrtc wrote: > I think there may be a more straightforward. > > 1. Check that p is odd. > 2. Check that Y > 1 > 2. If bits(p) > bits(Y) then return because Y < p-1 (because an odd number #1 > has the same number of bits) > 3. If bits(Y) > bits(p) then Y>p and it's invalid. > 4. Take the lower ((bits)/8) + !!(bits%8) bytes of Y and p [this value must be > the same] and PORT_memcmp(). If Y >= p, then it's invalid. If Y < p over these bits, then it's valid; if Y > p over these bytes then it's invalid. If Y == p over these bytes, then only the final byte will tell. > 5. At this point, p > Y, so now just check p[last]-1 >= Y[last] Done. It's not that much simpler, but it's not bad. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/297180043/diff/100001/lib/ssl/tls13con.c#newco... lib/ssl/tls13con.c:366: return rv; On 2016/06/01 18:08:51, ekr-webrtc wrote: > return SECFailure here and above Done. https://codereview.appspot.com/297180043/diff/100001/lib/ssl/tls13con.c#newco... lib/ssl/tls13con.c:383: PORT_InitCheapArena(&arena, DER_DEFAULT_CHUNKSIZE); On 2016/06/01 18:08:51, ekr-webrtc wrote: > This is an odd value to use. That's what everyone else does. Cargo cult.
Sign in to reply to this message.
LGTM with comments below. https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:161: // At that point, Y is greater than P without being easily detectable as I had to read this comment several times, because the point is that you are going to make this value Y. Maybe rephrase. https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:174: break; I have a simpler suggestion: set the first 32 octets to 0xff. This is guaranteed to be bigger than all the fixed primes and has a 2^{-256} chance of not being bigger than the random ones. https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:198: typedef std::tuple<std::string, uint16_t, TlsDheSkeChangeY::ChangeYTo, bool> I feel like this hasenough things int he tuple that it might need a comment. https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:203: Extra whitespace https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:209: static ::testing::internal::ParamGenerator<bool> kTrueFalse; Do you think these would be better outside the class. Question of taste, I guess https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:218: TlsDheSkeChangeY::kYZeroPad This feels like it might be better nearer the enum, or even perhaps part of TlsDheSkeChangeY. https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:234: server_->SetPacketFilter(new TlsDheSkeChangeY(change)); Don't we also need tests for CKE? https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:242: } else { Why does this report a DeviceError? That seems odd. https://codereview.appspot.com/297180043/diff/120001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/120001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:1477: bufLen = 2 * SSL3_RANDOM_LENGTH + 2 + dh_p.len + 2 + dh_g.len + 2 + dh_p.len; Good catch! I guess this proves that we are verifyingt he signature :) https://codereview.appspot.com/297180043/diff/120001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/120001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1674: if (dh_Ys->data[dh_Ys->len - 1] >= dh_p->data[dh_p->len - 1] - 1) { Can you put the RHS in parentheses here? Also, maybe update this comment to note that at this point Y and p differ in only the final octet.
Sign in to reply to this message.
Unfortunately, this crashes when I run it. Stack trace below. IMPORTANT 1: I see that you are padding Y in the verification stage. I think that's a mistake. You should verify precisely the bits you were sent. I apologize for missing it sooner, but you need to fix this. IMPORTANT 2: The assert is firing because you are writing off the end of hashBuf because len(Y_s) > len(p). If you follow my advice #1 above, then this problem should go away, however otherwise you need to check this earlier. You might in any case validate that len(dh_Y) <= len(dh_S) because it is silly for people to pad past that point. [ RUN ] DamageYStream/TlsDamageDHYTest.DamageY/10 .... 22039: SSL3[12405728]: handle server_hello handshake 22039: SSL3[12405728]: Set XXX Pending Cipher Suite to 0x0033 22039: SSL3[12405728]: handle certificate handshake 22039: SSL3[12405728]: handle server_key_exchange handshake Assertion failure: (unsigned int)(pBuf - hashBuf) == bufLen, at ssl3con.c:1510 Process 22039 stopped * thread #1: tid = 0x74ae9a, 0x00007fff8a8cff06 libsystem_kernel.dylib`__pthread_kill + 10, queue = 'com.apple.main-thread', stop reason = signal SIGABRT frame #0: 0x00007fff8a8cff06 libsystem_kernel.dylib`__pthread_kill + 10 libsystem_kernel.dylib`__pthread_kill: -> 0x7fff8a8cff06 <+10>: jae 0x7fff8a8cff10 ; <+20> 0x7fff8a8cff08 <+12>: movq %rax, %rdi 0x7fff8a8cff0b <+15>: jmp 0x7fff8a8ca7cd ; cerror_nocancel 0x7fff8a8cff10 <+20>: retq (lldb) up frame #1: 0x00007fff9f1564ec libsystem_pthread.dylib`pthread_kill + 90 libsystem_pthread.dylib`pthread_kill: 0x7fff9f1564ec <+90>: cmpl $-0x1, %eax 0x7fff9f1564ef <+93>: jne 0x7fff9f1564f8 ; <+102> 0x7fff9f1564f1 <+95>: callq 0x7fff9f1575fe ; symbol stub for: __error 0x7fff9f1564f6 <+100>: movl (%rax), %eax
Sign in to reply to this message.
In doing this, I discovered that we had bugs in a few of the key exchange functions. If the PMS was not correctly calculated, we would return SECSuccess because we had code like this: rv = doSomethingThatSucceeds(); pms = failAndReturnNull(); if (!pms) { goto loser; } ... rv = SECSuccess; loser: cleanup(); return rv; https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:161: // At that point, Y is greater than P without being easily detectable as On 2016/06/02 18:50:04, ekr-webrtc wrote: > I had to read this comment several times, because the point is that you are > going to make this value Y. Maybe rephrase. Done. https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:174: break; On 2016/06/02 18:50:04, ekr-webrtc wrote: > I have a simpler suggestion: set the first 32 octets to 0xff. This is guaranteed > to be bigger than all the fixed primes and has a 2^{-256} chance of not being > bigger than the random ones. > Done. https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:198: typedef std::tuple<std::string, uint16_t, TlsDheSkeChangeY::ChangeYTo, bool> On 2016/06/02 18:50:04, ekr-webrtc wrote: > I feel like this hasenough things int he tuple that it might need a comment. Done. https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:203: On 2016/06/02 18:50:04, ekr-webrtc wrote: > Extra whitespace Done. https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:209: static ::testing::internal::ParamGenerator<bool> kTrueFalse; On 2016/06/02 18:50:04, ekr-webrtc wrote: > Do you think these would be better outside the class. Question of taste, I guess Done. https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:218: TlsDheSkeChangeY::kYZeroPad On 2016/06/02 18:50:04, ekr-webrtc wrote: > This feels like it might be better nearer the enum, or even perhaps part of > TlsDheSkeChangeY. It still needs to be instantiated outside the class, so I think that I'll take the spirit of your last comment and leave it outside. I will move it closer to the INSTANTIATE_TEST_CASE_P calls though. https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:234: server_->SetPacketFilter(new TlsDheSkeChangeY(change)); On 2016/06/02 18:50:04, ekr-webrtc wrote: > Don't we also need tests for CKE? Ugh, you are right, that's another dimension to the matrix and a more complicated filter. https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:242: } else { On 2016/06/02 18:50:04, ekr-webrtc wrote: > Why does this report a DeviceError? That seems odd. Yes, super odd. https://codereview.appspot.com/297180043/diff/120001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/120001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:1477: bufLen = 2 * SSL3_RANDOM_LENGTH + 2 + dh_p.len + 2 + dh_g.len + 2 + dh_p.len; On 2016/06/02 18:50:04, ekr-webrtc wrote: > Good catch! I guess this proves that we are verifyingt he signature :) Acknowledged. https://codereview.appspot.com/297180043/diff/120001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/120001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1674: if (dh_Ys->data[dh_Ys->len - 1] >= dh_p->data[dh_p->len - 1] - 1) { On 2016/06/02 18:50:04, ekr-webrtc wrote: > Can you put the RHS in parentheses here? > > Also, maybe update this comment to note that at this point Y and p differ in > only the final octet. Done.
Sign in to reply to this message.
I think this needs another spin https://codereview.appspot.com/297180043/diff/140001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/140001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:123: return KEEP; You should have the type be provided by the subclass. As written, this will change both CKE and SKE even if it's of the other type. Oh, I see what you did here. I might have done things differently. :) https://codereview.appspot.com/297180043/diff/140001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:136: break; You need to ASSERT_EQ(len(Y), len(prime)) or you are going to be a sad panda. https://codereview.appspot.com/297180043/diff/140001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:153: // improbably large, but we use smaller, fixed prime currently. I'm not sure why you say "smaller". There are plenty of small mersenne primes. Rather, we have two cases: 1. Fixed primes which we know aren't Mersenne 2. Random primes which are chosen at bit lengths that can't be Mersenne, and then just have an improbably low chance of having all the high bits be 1s Why are you copying p[0] and not just using 0xff? I think we can agree 0xff >= p[0] https://codereview.appspot.com/297180043/diff/140001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/140001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:6768: return rv; Why isn't this returning SECFailure, because the only way to get here is through goto loser? ISTM you still haven't fixed the issue on line 6733 without that fix. And with that in mind, why did you change the gotos above. Maybe they don't need to run the codicil, but isn't it simpler to have the same logic. https://codereview.appspot.com/297180043/diff/140001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:10862: if (pms == NULL) { It seems like you are now leaking the ephemeral key pairs on pms failure. https://codereview.appspot.com/297180043/diff/140001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/297180043/diff/140001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:274: return SECFailure; See above comments about always running codicil. If you're only going to run it when you know you need cleanup, then it seems like you could (for instance) not null-check |pms| https://codereview.appspot.com/297180043/diff/140001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/140001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1676: return PR_FALSE; I think this code is fine, but I just thought of a slightly simpler algorithm: 1. PORT_memcmp() the entire string, including the last byte and return PR_FALSE on equality. 2. Then equality compare Y[len-1]+1 and p[len-1]. This this works if 0 <= Y]len-1] < 254. However, if Y[len-1] == 255, then either p[len-1] == 255 (in which case, we would have rejected above for equality) or p[len-1] < 255 (in which case we would have rejected above for Y > p). So we don't need to worry about this case. 3. Note that you can now omit the oddness check too, because you only needed that to ensure that p[len-1] didn't wrap. Please double-check my logic here, but I think I am right.
Sign in to reply to this message.
I restructured the tests so that they are more straightforward. Still not perfect, but much better I think. I fixed the stupid snafu in ssl3con.c. The missing cleanup in the ECDH function was benign, but I added some anyway. https://codereview.appspot.com/297180043/diff/140001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/140001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:123: return KEEP; On 2016/06/03 15:44:32, ekr-webrtc wrote: > You should have the type be provided by the subclass. As written, this will > change both CKE and SKE even if it's of the other type. > > Oh, I see what you did here. I might have done things differently. :) Yeah, I don't like this a whole lot, but it's hard to come up with a structure that factors the common parts cleanly. https://codereview.appspot.com/297180043/diff/140001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:136: break; On 2016/06/03 15:44:32, ekr-webrtc wrote: > You need to ASSERT_EQ(len(Y), len(prime)) or you are going to be a sad panda. Yeah, I thought that the check on line 207 was enough, but not for CKE. (Well, it is OK for us because we pad now, but no point in relying too much on that.) https://codereview.appspot.com/297180043/diff/140001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:153: // improbably large, but we use smaller, fixed prime currently. On 2016/06/03 15:44:32, ekr-webrtc wrote: > I'm not sure why you say "smaller". There are plenty of small mersenne primes. Smaller, but of the same bit length. I'll update the comment. > Rather, we have two cases: > 1. Fixed primes which we know aren't Mersenne > 2. Random primes which are chosen at bit lengths that can't be Mersenne, and > then just have an improbably low chance of having all the high bits be 1s > > Why are you copying p[0] and not just using 0xff? I think we can agree 0xff >= > p[0] Yeah, but if p[0] = 1, then Y will be longer, not just greater than. https://codereview.appspot.com/297180043/diff/140001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/140001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:6768: return rv; On 2016/06/03 15:44:32, ekr-webrtc wrote: > Why isn't this returning SECFailure, because the only way to get here is through > goto loser? ISTM you still haven't fixed the issue on line 6733 without that > fix. > > And with that in mind, why did you change the gotos above. Maybe they don't need > to run the codicil, but isn't it simpler to have the same logic. Oops, I means to change that. Done. https://codereview.appspot.com/297180043/diff/140001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:10862: if (pms == NULL) { On 2016/06/03 15:44:32, ekr-webrtc wrote: > It seems like you are now leaking the ephemeral key pairs on pms failure. I decided against it on the basis that the ephemeral keys get cleaned up when the socket is destroyed. Since these are properly ephemeral and never get used, having them around for a little longer didn't seem harmful. But I think that you are right. https://codereview.appspot.com/297180043/diff/140001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/297180043/diff/140001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:274: return SECFailure; On 2016/06/03 15:44:32, ekr-webrtc wrote: > See above comments about always running codicil. If you're only going to run it > when you know you need cleanup, then it seems like you could (for instance) not > null-check |pms| Yes, I think that goto loser everywhere is fine. https://codereview.appspot.com/297180043/diff/140001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/140001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1676: return PR_FALSE; On 2016/06/03 15:44:32, ekr-webrtc wrote: > I think this code is fine, but I just thought of a slightly simpler algorithm: > > 1. PORT_memcmp() the entire string, including the last byte and return PR_FALSE > on equality. > > 2. Then equality compare Y[len-1]+1 and p[len-1]. This this works if 0 <= > Y]len-1] < 254. However, if Y[len-1] == 255, then either p[len-1] == 255 (in > which case, we would have rejected above for equality) or p[len-1] < 255 (in > which case we would have rejected above for Y > p). So we don't need to worry > about this case. > > 3. Note that you can now omit the oddness check too, because you only needed > that to ensure that p[len-1] didn't wrap. > > Please double-check my logic here, but I think I am right. That fails with a false negative with 2^-8 probability. The problem with that code is that if Y compares smaller, but Y[len-1] == (p[len-1]-1), you will reject the share. But Y might have reported as less-than because any other octet was smaller.
Sign in to reply to this message.
Partial comments. Will review more tomorrow https://codereview.appspot.com/297180043/diff/140001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/140001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:153: // improbably large, but we use smaller, fixed prime currently. On 2016/06/04 00:19:47, mt wrote: > On 2016/06/03 15:44:32, ekr-webrtc wrote: > > I'm not sure why you say "smaller". There are plenty of small mersenne primes. > > Smaller, but of the same bit length. I'll update the comment. > > > Rather, we have two cases: > > 1. Fixed primes which we know aren't Mersenne > > 2. Random primes which are chosen at bit lengths that can't be Mersenne, and > > then just have an improbably low chance of having all the high bits be 1s > > > > Why are you copying p[0] and not just using 0xff? I think we can agree 0xff >= > > p[0] > > Yeah, but if p[0] = 1, then Y will be longer, not just greater than. Don't all our primes have 0x80 set? Anyway, seems fine as-is https://codereview.appspot.com/297180043/diff/140001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/140001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1676: return PR_FALSE; On 2016/06/04 00:19:47, mt wrote: > On 2016/06/03 15:44:32, ekr-webrtc wrote: > > I think this code is fine, but I just thought of a slightly simpler algorithm: > > > > 1. PORT_memcmp() the entire string, including the last byte and return > PR_FALSE > > on equality. > > > > 2. Then equality compare Y[len-1]+1 and p[len-1]. This this works if 0 <= > > Y]len-1] < 254. However, if Y[len-1] == 255, then either p[len-1] == 255 (in > > which case, we would have rejected above for equality) or p[len-1] < 255 (in > > which case we would have rejected above for Y > p). So we don't need to worry > > about this case. > > > > 3. Note that you can now omit the oddness check too, because you only needed > > that to ensure that p[len-1] didn't wrap. > > > > Please double-check my logic here, but I think I am right. > > That fails with a false negative with 2^-8 probability. > > The problem with that code is that if Y compares smaller, but Y[len-1] == > (p[len-1]-1), you will reject the share. But Y might have reported as less-than > because any other octet was smaller. > Yes, I see what you mean. I agree you need to compare the string minus the last byte. You can still save this construction by then comparing the last byte and returning false if Y>=p and then and only then doing the p-1 comparison, but it's not clear that that's better than the odd check at the beginning, https://codereview.appspot.com/297180043/diff/160001/external_tests/ssl_gtest... File external_tests/ssl_gtest/manifest.mn (right): https://codereview.appspot.com/297180043/diff/160001/external_tests/ssl_gtest... external_tests/ssl_gtest/manifest.mn:38: $(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX) Why was this needed? https://codereview.appspot.com/297180043/diff/160001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/160001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:122: << "Length of dh_Ys must equal length of dh_p"; Shouldn't this be ASSERT_EQ?
Sign in to reply to this message.
MT, I have only reviewed the diffs that correspond to my comments, under the assumption that everything else is due to the rebase from Kai. Please inform me if there are any other sections that need review. https://codereview.appspot.com/297180043/diff/160001/external_tests/ssl_gtest... File external_tests/ssl_gtest/manifest.mn (right): https://codereview.appspot.com/297180043/diff/160001/external_tests/ssl_gtest... external_tests/ssl_gtest/manifest.mn:38: $(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX) On 2016/06/04 01:06:57, ekr-webrtc wrote: > Why was this needed? Assuming it's Kai? https://codereview.appspot.com/297180043/diff/160001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/160001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:6962: } Perfect. https://codereview.appspot.com/297180043/diff/160001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/297180043/diff/160001/lib/ssl/tls13con.c#newco... lib/ssl/tls13con.c:529: CKA_DERIVE, tls13_GetHashSize(ss), Good catch.
Sign in to reply to this message.
|