Issue 307060043: Named group preferences
DescriptionNamed group preferences
Patch Set 1 #Patch Set 2 : SSL_NamedGroupConfig #
Total comments: 37
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 43
Patch Set 6 : #
Total comments: 48
Patch Set 7 : #Patch Set 8 : #
Total comments: 47
Patch Set 9 : #
Total comments: 15
Patch Set 10 : #
Total comments: 10
MessagesTotal messages: 15
https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:81: TEST_P(TlsConnectGeneric, P384Priority) { We should test that the server's preference wins as well. You might have to wait for my HRR patch to land (real soon now). https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:84: client_->EnableCiphersByKeyExchange(ssl_kea_ecdh); You can skip these two lines: ecdh is our default. https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:106: }; For the supported_groups extension, rather than use this method of checking, I would prefer if you simply encode the groups and compare binary-wise. That will let you check that the order of groups is correct. This is fine for checking the shares. Also add a server capture verify that the server *didn't* send a HelloRetryRequest. https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/tls_agent.h (right): https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... external_tests/ssl_gtest/tls_agent.h:144: void ConfigNamedGroup(SSLNamedGroup group, bool en); The named group configuration was added in 3.27. We should remove it and just use your API. https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/tls_connect.cc (right): https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... external_tests/ssl_gtest/tls_connect.cc:103: class CheckDuplicateGroup { This class can be private on TlsConnectTestBase. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl.h File lib/ssl/ssl.h (right): https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl.h#newcode402 lib/ssl/ssl.h:402: unsigned int p); I think that we can safely remove this function, as long as we get this patch in during 3.27. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl.h#newcode423 lib/ssl/ssl.h:423: PRUint16 num_groups); unnecessary indent change. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl3ecc.c#newcode434 lib/ssl/ssl3ecc.c:434: if (ss->numConfiguredGroups && ss->namedGroupsConfig) { BUG: This will crash when invoked from the code in derive.c https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl3ecc.c#newcode438 lib/ssl/ssl3ecc.c:438: } Initialize a pointer to the list as well at the same time. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl3ecc.c#newcode451 lib/ssl/ssl3ecc.c:451: if ((curvemsk & (1U << group->index))) { This mask is crappy, we should be using ssl_NamedGroupEnabled() once we have a socket (and assume true if we don't). https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (right): https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslimpl.h#newcode... lib/ssl/sslimpl.h:1318: const namedGroupDef **namedGroupsConfig; namedGroupPreferences; namedGroupPreferenceCount; https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c#newcode1 lib/ssl/sslsock.c:1: /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ This file is missing changes that copy the configured preferences to other sockets. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1534: SSL_NamedGroupConfig(PRFileDesc *fd, SSLNamedGroup *groups, unsigned int num_groups) You need to clear ss->namedGroups and set the bit on each group that is listed. You also need to reject when there are zero *supported* groups. Currently you are putting a lot of nulls in the list. It's OK if you over-allocate, but the list should only include supported groups. See the signature algorithm preference code. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1551: ss->namedGroupsConfig = (const namedGroupDef **)PORT_Alloc(num_groups*sizeof(namedGroupDef*)); PORT_ZArrayNew https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1553: PORT_SetError(SEC_ERROR_NO_MEMORY); You don't need to set this again. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3811: ss->namedGroupsConfig = NULL; And the count? https://codereview.appspot.com/307060043/diff/20001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/307060043/diff/20001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:338: } same comment as in ssl3ecc.c
Sign in to reply to this message.
https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:81: TEST_P(TlsConnectGeneric, P384Priority) { On 2016/08/24 00:19:32, mt wrote: > We should test that the server's preference wins as well. You might have to > wait for my HRR patch to land (real soon now). I added a test for this for 1.2. We can add a 1.3 test when your HRR patch landed. https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:84: client_->EnableCiphersByKeyExchange(ssl_kea_ecdh); On 2016/08/24 00:19:32, mt wrote: > You can skip these two lines: ecdh is our default. well, we currently need this. See bug 1297734. https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:106: }; On 2016/08/24 00:19:32, mt wrote: > For the supported_groups extension, rather than use this method of checking, I > would prefer if you simply encode the groups and compare binary-wise. That will > let you check that the order of groups is correct. > > This is fine for checking the shares. > > Also add a server capture verify that the server *didn't* send a > HelloRetryRequest. added a HRR check and check that the order of supported groups matches as well. https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/tls_agent.h (right): https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... external_tests/ssl_gtest/tls_agent.h:144: void ConfigNamedGroup(SSLNamedGroup group, bool en); On 2016/08/24 00:19:32, mt wrote: > The named group configuration was added in 3.27. We should remove it and just > use your API. Done. https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/tls_connect.cc (right): https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... external_tests/ssl_gtest/tls_connect.cc:103: class CheckDuplicateGroup { On 2016/08/24 00:19:32, mt wrote: > This class can be private on TlsConnectTestBase. Done. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl.h File lib/ssl/ssl.h (right): https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl.h#newcode402 lib/ssl/ssl.h:402: unsigned int p); On 2016/08/24 00:19:32, mt wrote: > I think that we can safely remove this function, as long as we get this patch in > during 3.27. Done. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl.h#newcode423 lib/ssl/ssl.h:423: PRUint16 num_groups); On 2016/08/24 00:19:32, mt wrote: > unnecessary indent change. Done. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl3ecc.c#newcode434 lib/ssl/ssl3ecc.c:434: if (ss->numConfiguredGroups && ss->namedGroupsConfig) { On 2016/08/24 00:19:32, mt wrote: > BUG: This will crash when invoked from the code in derive.c Done. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl3ecc.c#newcode438 lib/ssl/ssl3ecc.c:438: } On 2016/08/24 00:19:32, mt wrote: > Initialize a pointer to the list as well at the same time. well, if it would be that easy... ssl_named_groups is an array of namedGroupDefs while ss->nnamedGroupsConfig is an array of pointers to namedGroupDefs. We can't copy the values because we rely on actually always having a pointer to a group in ssl_named_groups. So I don't see a way right to make this work without doing the if again in the loop. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl3ecc.c#newcode451 lib/ssl/ssl3ecc.c:451: if ((curvemsk & (1U << group->index))) { On 2016/08/24 00:19:32, mt wrote: > This mask is crappy, we should be using ssl_NamedGroupEnabled() once we have a > socket (and assume true if we don't). Done. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (right): https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslimpl.h#newcode... lib/ssl/sslimpl.h:1318: const namedGroupDef **namedGroupsConfig; On 2016/08/24 00:19:32, mt wrote: > namedGroupPreferences; > namedGroupPreferenceCount; Done. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1551: ss->namedGroupsConfig = (const namedGroupDef **)PORT_Alloc(num_groups*sizeof(namedGroupDef*)); On 2016/08/24 00:19:32, mt wrote: > PORT_ZArrayNew Do we have something like this? I didn't find a function with this name one that would do something like this. I can do PORT_ZAlloc. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1553: PORT_SetError(SEC_ERROR_NO_MEMORY); On 2016/08/24 00:19:32, mt wrote: > You don't need to set this again. Done. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:3811: ss->namedGroupsConfig = NULL; On 2016/08/24 00:19:32, mt wrote: > And the count? Done.
Sign in to reply to this message.
https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c#newcode1 lib/ssl/sslsock.c:1: /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ On 2016/08/24 00:19:32, mt wrote: > This file is missing changes that copy the configured preferences to other > sockets. Done. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1534: SSL_NamedGroupConfig(PRFileDesc *fd, SSLNamedGroup *groups, unsigned int num_groups) On 2016/08/24 00:19:32, mt wrote: > You need to clear ss->namedGroups and set the bit on each group that is listed. > > You also need to reject when there are zero *supported* groups. Done > Currently you > are putting a lot of nulls in the list. It's OK if you over-allocate, but the > list should only include supported groups. See the signature algorithm > preference code. Yeah, if we want to avoid putting nulls in the list we'd have to loop twice to see which groups are enabled before allocating. I added checks that we add only enabled curves.
Sign in to reply to this message.
I think it's a mistake to keep the other (mask-type) supported groups representation as well as a master list which you check only if the custom groups aren't configured. You should just initialize the sslSocket with the default groups and leave it at that and then only check that list. NOTE: I have not checked that you left the FFDHE-Weak behavior the same. you will need to check that with Kai https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:63: SSLNamedGroup groups[] = {ssl_grp_ec_secp384r1, ssl_grp_ffdhe_2048}; You shouldn't need ff2048 https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:91: groups[(i / 2) - 1] = static_cast<SSLNamedGroup>(tmp); I would instead index in group space, not byte space. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:103: std::vector<PacketFilter *> captures; clientCaptures https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:127: EXPECT_TRUE(!memcmp(groups, incomingGroups, PR_ARRAY_SIZE(incomingGroups))); BUG: You want sizeof(), not PR_ARRAY_SIZE() because sizeof(NamedGroup) is 2 https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:149: server_->SetPacketFilter(new ChainedPacketFilter(serverCaptures)); I'm feeling like you should abstract out this code https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:171: }; This is pretty clunky. I wonder if we want direct access. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:195: When we add the new negotiation we will want a test that shows we can prioritize FFDHE over ECDHE. Just a placeholder. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/tls_agent.cc (right): https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/tls_agent.cc:127: ConfigNamedGroups(groups[i], PR_ARRAY_SIZE(groups)); BUG: Why does this even compile. ConfigNamedGroups takes a SSLNamedGroup* and you are passing an SSLNamedGroup. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/tls_connect.cc (right): https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/tls_connect.cc:135: check_group(group, 0); count in groups not bytes. Why the 0 argument? It's not like you have a typedef. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/tls_connect.cc:149: ASSERT_TRUE(shares.Read(i, 2, &tmp)); This is pretty hard to read. Can you rewrite with internal increments and a do/while https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/tls_connect.h (right): https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/tls_connect.h:131: class CheckDuplicateGroup { DuplicaeGroupChecker https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl.h File lib/ssl/ssl.h (right): https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl.h#newcode400 lib/ssl/ssl.h:400: SSL_IMPORT SECStatus SSL_NamedGroupConfig(PRFileDesc *fd, SSLNamedGroup *groups, const SSLNAmedGroup* https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl3ecc.c#newcode434 lib/ssl/ssl3ecc.c:434: if (ss && ss->namedGroupPreferenceCount && ss->namedGroupPreferences) { Can count be nonzero but preferences be zero? If not, assert the latter. How do I enable no groups? I bet I give it a zero-length list? https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl3ecc.c#newcode447 lib/ssl/ssl3ecc.c:447: if (group->type != group_type_ec || BUG? You seem to assume here that this list has no NULLS https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1074: // groups = ssl_named_groups; Commented out, plus C++ -style comments. https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1098: enabledGroups[enabledGroupsLen++] = group->name & 0xff; ssl3_EncodeUintX https://codereview.appspot.com/307060043/diff/80001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/80001/lib/ssl/sslsock.c#newcode346 lib/ssl/sslsock.c:346: } BUG: PORT_memcpy() needed here. https://codereview.appspot.com/307060043/diff/80001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1513: SSL_NamedGroupConfig(PRFileDesc *fd, SSLNamedGroup *groups, unsigned int num_groups) Convention here is numGroups, etc. https://codereview.appspot.com/307060043/diff/80001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1552: break; You shouldn't gate groups being on on which cipher suites are enabled. It creates an unpleasant API ordering issue. Also, this will leave NULLs in the array for groups which correspond to disabled ciphers or otherwise don't match the API. But you assume elsewhere that that's not true. https://codereview.appspot.com/307060043/diff/80001/lib/ssl/sslt.h File lib/ssl/sslt.h (right): https://codereview.appspot.com/307060043/diff/80001/lib/ssl/sslt.h#newcode282 lib/ssl/sslt.h:282: ssl_hello_retry_request_xtn = 6, What is this? There is no HRR xtn. https://codereview.appspot.com/307060043/diff/80001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/307060043/diff/80001/lib/ssl/tls13con.c#newcod... lib/ssl/tls13con.c:343: const namedGroupDef *groupDef = NULL; You don't need to initialize this.
Sign in to reply to this message.
> I think it's a mistake to keep the other (mask-type) supported groups > representation as well as a master list which you check only if the custom > groups aren't configured. You should just initialize the sslSocket with the > default groups and leave it at that and then only check that list. Did that and replaced all uses of ssl_named_gropus with ss->namedGroupPreferences where it matter whether the groups are enabled or not. However, this adds a loop over the groups to the socket initialisation as well as heavily over-allocates the group array when SSL_NamedGroupConfig isn't used. But considering that it's only an array of pointers that might be ok. > NOTE: I have not checked that you left the FFDHE-Weak behavior the same. you > will need to check that with Kai I don't think these change should influence that behaviour. But I'll ni? Kai on the patch. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:63: SSLNamedGroup groups[] = {ssl_grp_ec_secp384r1, ssl_grp_ffdhe_2048}; On 2016/08/30 21:02:20, ekr-rietveld wrote: > You shouldn't need ff2048 But I do. See bug 1297734. I'd fix it but I'm not sure what the behaviour is we want here. We can either enforce in SSL_NamedGroupConfig that at least one FF and EC group are enabled, or we have to disable DHE/ECDH suites when sending the client hello and no FF/EC groups are enabled. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:91: groups[(i / 2) - 1] = static_cast<SSLNamedGroup>(tmp); On 2016/08/30 21:02:20, ekr-rietveld wrote: > I would instead index in group space, not byte space. Done. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:103: std::vector<PacketFilter *> captures; On 2016/08/30 21:02:20, ekr-rietveld wrote: > clientCaptures Done. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:127: EXPECT_TRUE(!memcmp(groups, incomingGroups, PR_ARRAY_SIZE(incomingGroups))); On 2016/08/30 21:02:20, ekr-rietveld wrote: > BUG: You want sizeof(), not PR_ARRAY_SIZE() because sizeof(NamedGroup) is 2 Done. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:149: server_->SetPacketFilter(new ChainedPacketFilter(serverCaptures)); On 2016/08/30 21:02:20, ekr-rietveld wrote: > I'm feeling like you should abstract out this code Done. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:171: }; On 2016/08/30 21:02:19, ekr-rietveld wrote: > This is pretty clunky. I wonder if we want direct access. I refactored the tests here. I'll re-use this functionality once 25519 landed. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/tls_agent.cc (right): https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/tls_agent.cc:127: ConfigNamedGroups(groups[i], PR_ARRAY_SIZE(groups)); On 2016/08/30 21:02:20, ekr-rietveld wrote: > BUG: Why does this even compile. ConfigNamedGroups takes a SSLNamedGroup* and > you are passing an SSLNamedGroup. ugh, I didn't update this after the last changes. This flag really has to die. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/tls_connect.cc (right): https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/tls_connect.cc:135: check_group(group, 0); On 2016/08/30 21:02:20, ekr-rietveld wrote: > count in groups not bytes. Why the 0 argument? It's not like you have a typedef. removed. it's not used anymore after refactoring the tests. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/tls_connect.cc:149: ASSERT_TRUE(shares.Read(i, 2, &tmp)); On 2016/08/30 21:02:20, ekr-rietveld wrote: > This is pretty hard to read. Can you rewrite with internal increments and a > do/while I added a custom function to check shares and reverted the changes here. (note that the functions here are only moved from ssl_dhe_unittest) https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/tls_connect.h (right): https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/tls_connect.h:131: class CheckDuplicateGroup { On 2016/08/30 21:02:20, ekr-rietveld wrote: > DuplicaeGroupChecker Done. https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl.h File lib/ssl/ssl.h (right): https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl.h#newcode400 lib/ssl/ssl.h:400: SSL_IMPORT SECStatus SSL_NamedGroupConfig(PRFileDesc *fd, SSLNamedGroup *groups, On 2016/08/30 21:02:21, ekr-rietveld wrote: > const SSLNAmedGroup* Done. https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl3ecc.c#newcode434 lib/ssl/ssl3ecc.c:434: if (ss && ss->namedGroupPreferenceCount && ss->namedGroupPreferences) { On 2016/08/30 21:02:21, ekr-rietveld wrote: > Can count be nonzero but preferences be zero? If not, assert the latter. > > How do I enable no groups? I bet I give it a zero-length list? Do we want to allow disabling all groups? (I don't think we should.) SSL_NamedGroupConfig currently rejects setting a zero-length list. https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl3ecc.c#newcode447 lib/ssl/ssl3ecc.c:447: if (group->type != group_type_ec || On 2016/08/30 21:02:21, ekr-rietveld wrote: > BUG? You seem to assume here that this list has no NULLS I do. Would that be something we want to allow? (I wouldn't know why.) But yeah we should check that group != NULL. (And I changed SSL_NamedGroupConfig to not add NULL groups.) https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1074: // groups = ssl_named_groups; On 2016/08/30 21:02:21, ekr-rietveld wrote: > Commented out, plus C++ -style comments. Acknowledged. https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl3ecc.c#newcode... lib/ssl/ssl3ecc.c:1098: enabledGroups[enabledGroupsLen++] = group->name & 0xff; On 2016/08/30 21:02:21, ekr-rietveld wrote: > ssl3_EncodeUintX Done. https://codereview.appspot.com/307060043/diff/80001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/80001/lib/ssl/sslsock.c#newcode346 lib/ssl/sslsock.c:346: } On 2016/08/30 21:02:22, ekr-rietveld wrote: > BUG: PORT_memcpy() needed here. not memcpy (we have only pointers), but I forgot to assign the values. I added a test for this. https://codereview.appspot.com/307060043/diff/80001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1513: SSL_NamedGroupConfig(PRFileDesc *fd, SSLNamedGroup *groups, unsigned int num_groups) On 2016/08/30 21:02:21, ekr-rietveld wrote: > Convention here is numGroups, etc. Done. https://codereview.appspot.com/307060043/diff/80001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1552: break; On 2016/08/30 21:02:22, ekr-rietveld wrote: > You shouldn't gate groups being on on which cipher suites are enabled. It > creates an unpleasant API ordering issue. > > Also, this will leave NULLs in the array for groups which correspond to disabled > ciphers or otherwise don't match the API. But you assume elsewhere that that's > not true. I agree, I'd be adding all requested groups. I added this on suggestion from mt, but I took it out again.
Sign in to reply to this message.
Per my comments, I think you should consolidate the mask and the list. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:63: SSLNamedGroup groups[] = {ssl_grp_ec_secp384r1, ssl_grp_ffdhe_2048}; On 2016/08/31 14:16:11, franziskus wrote: > On 2016/08/30 21:02:20, ekr-rietveld wrote: > > You shouldn't need ff2048 > > But I do. See bug 1297734. I'd fix it but I'm not sure what the behaviour is we > want here. We can either enforce in SSL_NamedGroupConfig that at least one FF > and EC group are enabled, or we have to disable DHE/ECDH suites when sending the > client hello and no FF/EC groups are enabled. Ugh, I see what you mean. this will be fixed with my renegotiation patch. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/tls_connect.cc (right): https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/... external_tests/ssl_gtest/tls_connect.cc:149: ASSERT_TRUE(shares.Read(i, 2, &tmp)); On 2016/08/31 14:16:11, franziskus wrote: > On 2016/08/30 21:02:20, ekr-rietveld wrote: > > This is pretty hard to read. Can you rewrite with internal increments and a > > do/while > > I added a custom function to check shares and reverted the changes here. (note > that the functions here are only moved from ssl_dhe_unittest) Done. https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl3ecc.c#newcode434 lib/ssl/ssl3ecc.c:434: if (ss && ss->namedGroupPreferenceCount && ss->namedGroupPreferences) { On 2016/08/31 14:16:11, franziskus wrote: > On 2016/08/30 21:02:21, ekr-rietveld wrote: > > Can count be nonzero but preferences be zero? If not, assert the latter. > > > > How do I enable no groups? I bet I give it a zero-length list? > > Do we want to allow disabling all groups? (I don't think we should.) > SSL_NamedGroupConfig currently rejects setting a zero-length list. Well, what if you want to disable PFS? I could live with forbidding that, I guess. https://codereview.appspot.com/307060043/diff/80001/lib/ssl/ssl3ecc.c#newcode447 lib/ssl/ssl3ecc.c:447: if (group->type != group_type_ec || On 2016/08/31 14:16:11, franziskus wrote: > On 2016/08/30 21:02:21, ekr-rietveld wrote: > > BUG? You seem to assume here that this list has no NULLS > > I do. Would that be something we want to allow? (I wouldn't know why.) But yeah > we should check that group != NULL. (And I changed SSL_NamedGroupConfig to not > add NULL groups.) It looked to me like your algorithm could produce that. That's why I asked. https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:97: } Not happy about the multiple inheritance here. would rather see the filter created explicitly. https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:122: groups.push_back(static_cast<SSLNamedGroup>(tmp)); This seems to still index in byte space here and below. https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:127: unsigned int num) { Coding style is that references need to be const T&. Pass a T* or better yet, just return the vector https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:147: if (version_ == SSL_LIBRARY_VERSION_TLS_1_3) { >= https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:151: !memcmp(shares, &incomingShares[0], sizeof(uint16_t) * numShares)); you don't seem to be checking that incomingshares isn't bigger. Above as well. https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:171: CheckKEXDetails(PR_ARRAY_SIZE(groups), groups, PR_ARRAY_SIZE(shares), shares); ptr first, length second. https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:183: if (version_ == SSL_LIBRARY_VERSION_TLS_1_3) { >= https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:192: PR_ARRAY_SIZE(shares), shares); Is shares even checked in this second case? https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:434: if (!group || group->type != group_type_ec || Oh, so much better. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:434: if (!group || group->type != group_type_ec || I actually think group should not be allowed to be null. I would forbid that and do an assert.... https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1222: ss->namedGroups &= peerGroups; So, my suggestion was to remove this mask entirely. Are you keeping it to make renegotiation work? https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:341: goto loser; Where is this allocated?? I am suspecting that an assert is needed here instead or you have a bug. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:344: if (os->namedGroupPreferences && ss->namedGroupPreferenceCount) { How would os->namedGroupPreferences be 0? https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:348: } Why can't you memcpy this? https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1553: } I have a simpler proposal: Set disable to all 1s Then iterate through namedGroupPreferenceCount and for all the groups that exist, add them and 0 the corresponding bit. Then disable whatever disable is. Just error on any group which doesn't exist. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1595: } This is deprecated? So we have two structures: the stuff in this list and then a mask? That seems suboptimal. Can we instead mark things on the list on or off.
Sign in to reply to this message.
I think that ekr covered most of this. https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:81: TEST_P(TlsConnectGeneric, P384Priority) { On 2016/08/25 13:03:20, franziskus wrote: > On 2016/08/24 00:19:32, mt wrote: > > We should test that the server's preference wins as well. You might have to > > wait for my HRR patch to land (real soon now). > > I added a test for this for 1.2. We can add a 1.3 test when your HRR patch > landed. Acknowledged. https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:84: client_->EnableCiphersByKeyExchange(ssl_kea_ecdh); On 2016/08/25 13:03:20, franziskus wrote: > On 2016/08/24 00:19:32, mt wrote: > > You can skip these two lines: ecdh is our default. > > well, we currently need this. See bug 1297734. How so? You aren't causing ECDH groups to be disabled. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/20001/lib/ssl/ssl3ecc.c#newcode438 lib/ssl/ssl3ecc.c:438: } On 2016/08/25 13:03:20, franziskus wrote: > On 2016/08/24 00:19:32, mt wrote: > > Initialize a pointer to the list as well at the same time. > > well, if it would be that easy... ssl_named_groups is an array of namedGroupDefs > while ss->nnamedGroupsConfig is an array of pointers to namedGroupDefs. We can't > copy the values because we rely on actually always having a pointer to a group > in ssl_named_groups. So I don't see a way right to make this work without doing > the if again in the loop. Acknowledged. https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c#newcode... lib/ssl/sslsock.c:1551: ss->namedGroupsConfig = (const namedGroupDef **)PORT_Alloc(num_groups*sizeof(namedGroupDef*)); On 2016/08/25 13:03:21, franziskus wrote: > On 2016/08/24 00:19:32, mt wrote: > > PORT_ZArrayNew > > Do we have something like this? I didn't find a function with this name one that > would do something like this. I can do PORT_ZAlloc. Sorry, it was PORT_ZNewArray: http://searchfox.org/nss/rev/1d98a506a629bbab9d0f63f71711424e1936a9c6/lib/uti... https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:97: } On 2016/09/01 00:27:14, ekr-rietveld wrote: > Not happy about the multiple inheritance here. would rather see the filter > created explicitly. Like ekr, the filter can just use TlsInspectorRecordHandshakeMessage https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:119: ASSERT_EQ((ext.len() - 2) / 2, num); You can assert the expected number outside. Better yet, you can use the == operator on std::vector. Get the groups (as std::vector). Build a vector of what you expect it to be and EXPECT_EQ(expected, GetGroupDetails(extension)) https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... File external_tests/ssl_gtest/tls_connect.cc (right): https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/tls_connect.cc:431: server_model_->ConfigNamedGroups(groups, PR_ARRAY_SIZE(groups)); I'd rather this method only did the construction and init. The extra steps can be run in the individual tests that depend on the model. Move lines 424-5 to the ALPN test and these new lines to your new test. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl.h File lib/ssl/ssl.h (right): https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl.h#newcode437 lib/ssl/ssl.h:437: ** used, unless it is certain that the client supports larger group parameters. This isn't quite correct. The point is that for a server that chooses TLS <= 1.2 we will use the weak group unless the client has expressly indicated (using RFC7919) that it supports a stronger ffdhe group. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:432: for (i = 0; i < ss->namedGroupPreferenceCount; i++) { BUG: This is going to crash when called from derive.c still. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1069: ssl_EncodeUintX(group->name, 2, &enabledGroups[enabledGroupsLen]); Maybe case to (void) https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1222: ss->namedGroups &= peerGroups; On 2016/09/01 00:27:14, ekr-rietveld wrote: > So, my suggestion was to remove this mask entirely. Are you keeping it to make > renegotiation work? The problem I see here is that we remove groups from the list when the peer doesn't support them. But I think that we can manage that. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1536: ss->namedGroupPreferenceCount = numGroups; You should free the old value. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1553: } On 2016/09/01 00:27:14, ekr-rietveld wrote: > I have a simpler proposal: > Set disable to all 1s > Then iterate through namedGroupPreferenceCount and for all the groups that > exist, add them and 0 the corresponding bit. Then disable whatever disable is. Or don't worry about the bitmask as suggested elsewhere. Which means that you can remove the ->index field from the struct. > Just error on any group which doesn't exist. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:3799: ss->namedGroupPreferences = Why not just set ss->namedGroupPreferences = &ssl_named_groups; ss->namedGroupPreferenceCount = ssl_named_group_count; And check before freeing that this isn't the case. Saves a lot of work in the most common case (where the groups aren't configured). It just means that you need to malloc when someone sets the pref. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/307060043/diff/100001/lib/ssl/tls13con.c#newco... lib/ssl/tls13con.c:338: if (!groupDef || !ssl_NamedGroupEnabled(ss, groupDef)) { Don't allow nulls.
Sign in to reply to this message.
The next patch should be only using the socket's namedGroupPreferences for everything, including enabling/disabling groups. This isn't great because it require a lot of looping over that list (compared to the simple bit mask before) but unifies handling of group preferences. https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:97: } On 2016/09/01 05:55:45, mt wrote: > On 2016/09/01 00:27:14, ekr-rietveld wrote: > > Not happy about the multiple inheritance here. would rather see the filter > > created explicitly. > > Like ekr, the filter can just use TlsInspectorRecordHandshakeMessage Done. https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:119: ASSERT_EQ((ext.len() - 2) / 2, num); On 2016/09/01 05:55:45, mt wrote: > You can assert the expected number outside. Better yet, you can use the == > operator on std::vector. > > Get the groups (as std::vector). Build a vector of what you expect it to be and > EXPECT_EQ(expected, GetGroupDetails(extension)) Done. https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:122: groups.push_back(static_cast<SSLNamedGroup>(tmp)); On 2016/09/01 00:27:13, ekr-rietveld wrote: > This seems to still index in byte space here and below. changed it here, and made it a while loop below. https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:127: unsigned int num) { On 2016/09/01 00:27:13, ekr-rietveld wrote: > Coding style is that references need to be const T&. Pass a T* or better yet, > just return the vector Done. https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:151: !memcmp(shares, &incomingShares[0], sizeof(uint16_t) * numShares)); On 2016/09/01 00:27:14, ekr-rietveld wrote: > you don't seem to be checking that incomingshares isn't bigger. > > Above as well. comparing vectors takes care of this now. https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:171: CheckKEXDetails(PR_ARRAY_SIZE(groups), groups, PR_ARRAY_SIZE(shares), shares); On 2016/09/01 00:27:13, ekr-rietveld wrote: > ptr first, length second. non pointers anymore https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:183: if (version_ == SSL_LIBRARY_VERSION_TLS_1_3) { On 2016/09/01 00:27:13, ekr-rietveld wrote: > >= Done. https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:192: PR_ARRAY_SIZE(shares), shares); On 2016/09/01 00:27:13, ekr-rietveld wrote: > Is shares even checked in this second case? no it's not. but we have to pass in something. using an empty vector now. https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... File external_tests/ssl_gtest/tls_connect.cc (right): https://codereview.appspot.com/307060043/diff/100001/external_tests/ssl_gtest... external_tests/ssl_gtest/tls_connect.cc:431: server_model_->ConfigNamedGroups(groups, PR_ARRAY_SIZE(groups)); On 2016/09/01 05:55:45, mt wrote: > I'd rather this method only did the construction and init. The extra steps can > be run in the individual tests that depend on the model. Move lines 424-5 to > the ALPN test and these new lines to your new test. Done. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl.h File lib/ssl/ssl.h (right): https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl.h#newcode437 lib/ssl/ssl.h:437: ** used, unless it is certain that the client supports larger group parameters. On 2016/09/01 05:55:45, mt wrote: > This isn't quite correct. The point is that for a server that chooses TLS <= > 1.2 we will use the weak group unless the client has expressly indicated (using > RFC7919) that it supports a stronger ffdhe group. ah, that makes sense. I reverted the changes and added the hint to <= TLS 1.2. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:432: for (i = 0; i < ss->namedGroupPreferenceCount; i++) { On 2016/09/01 05:55:45, mt wrote: > BUG: This is going to crash when called from derive.c still. ah, this was fixed and I tried to do a last change... https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1069: ssl_EncodeUintX(group->name, 2, &enabledGroups[enabledGroupsLen]); On 2016/09/01 05:55:45, mt wrote: > Maybe case to (void) Done. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1222: ss->namedGroups &= peerGroups; On 2016/09/01 05:55:45, mt wrote: > On 2016/09/01 00:27:14, ekr-rietveld wrote: > > So, my suggestion was to remove this mask entirely. Are you keeping it to make > > renegotiation work? > > The problem I see here is that we remove groups from the list when the peer > doesn't support them. But I think that we can manage that. Ah, I think I misunderstood the first time. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:341: goto loser; On 2016/09/01 00:27:14, ekr-rietveld wrote: > Where is this allocated?? > > I am suspecting that an assert is needed here instead or you have a bug. I put it on the stack now. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:344: if (os->namedGroupPreferences && ss->namedGroupPreferenceCount) { On 2016/09/01 00:27:14, ekr-rietveld wrote: > How would os->namedGroupPreferences be 0? Done. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:348: } On 2016/09/01 00:27:14, ekr-rietveld wrote: > Why can't you memcpy this? I could, but it's a simple struct without pointers so it's not necessary. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1553: } On 2016/09/01 05:55:45, mt wrote: > On 2016/09/01 00:27:14, ekr-rietveld wrote: > > I have a simpler proposal: > > Set disable to all 1s > > Then iterate through namedGroupPreferenceCount and for all the groups that > > exist, add them and 0 the corresponding bit. Then disable whatever disable is. > > Or don't worry about the bitmask as suggested elsewhere. Which means that you > can remove the ->index field from the struct. > > > Just error on any group which doesn't exist. > the index is used in other places as well. We can't get rid of that. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:3799: ss->namedGroupPreferences = On 2016/09/01 05:55:45, mt wrote: > Why not just set > > ss->namedGroupPreferences = &ssl_named_groups; > ss->namedGroupPreferenceCount = ssl_named_group_count; > > And check before freeing that this isn't the case. Saves a lot of work in the > most common case (where the groups aren't configured). It just means that you > need to malloc when someone sets the pref. Acknowledged.
Sign in to reply to this message.
This needs another round https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:7028: IMPORTANT: Why is this safe to remove. https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:348: } On 2016/09/02 13:23:30, franziskus wrote: > On 2016/09/01 00:27:14, ekr-rietveld wrote: > > Why can't you memcpy this? > > I could, but it's a simple struct without pointers so it's not necessary. Well, my point is that you can just memcpy the entire array without looping. https://codereview.appspot.com/307060043/diff/140001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/140001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:100: new TlsInspectorRecordHandshakeMessage(kTlsHandshakeHelloRetryRequest); Why not create this in the ctor? https://codereview.appspot.com/307060043/diff/140001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:114: for (size_t i = 1; i < ext.len() / 2; i += 1) { assert that ext.len() is even This is really not future-proofed if (for instance) we added an odd-length thing to this extension. You should work in the space of group list. https://codereview.appspot.com/307060043/diff/140001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:170: groups, groups + sizeof(groups) / sizeof(SSLNamedGroup)); PR_ARRAY_SIZE https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:840: ssl_NamedGroupEnabled(ss, &ss->namedGroupPreferences[i])) { Do you really need to check policy? Can't you just check enabled? https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:7044: PR_FALSE); IMPORTANT: You are no longer sending an alert. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:12562: } PORT_Memcpy(sid->namedGroupPreferences, ss->namedGroupPreferences, sizeof())... https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:442: for (i = 0; i < groupCount; i++) { Nit: we seem to be converging on ++i https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1031: // PORT_Assert(ss->namedGroupPreferences[i].index == i); Commented out code. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1184: SSLNamedGroup toEnable[30]; /* this has to be ssl_named_group_count */ Then you should assert it https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1198: /* build bit vector of peer's supported groups */ This comment is now wrong. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1209: toEnable[k++] = group->name; BUG: You do not appear to be checking that k is in bounds, merely that the group is valid If data contains multiple copies of this group, you may overrun toEnable. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1244: } Instead of creating a toEnable temporary why not just set the entire list to disabled and then iterate through it for each incoming code point. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (right): https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslimpl.h#newcod... lib/ssl/sslimpl.h:1316: namedGroupDef namedGroupPreferences[30]; Please no numeric constants here. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslimpl.h#newcod... lib/ssl/sslimpl.h:1317: PRUint32 namedGroupPreferenceCount; IMPORTANT: How does this interact with renegotiation? Say I do a handshake, which sets various curves to disabled. Won't I then not offer them on renegotioation. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1531: sizeof(namedGroupDef)); Now, here I would assign rather than memcpying https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1600: for (j = 0; j < ss->namedGroupPreferenceCount; ++j) { Didn't these start true? What makes them false https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1856: ssl_NamedGroupEnabled(ss, &ss->namedGroupPreferences[i])) { I'm not sure why you aren't filtering through policy before you make this list, in which case you could just look at group->enabled https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:3674: pair->group = PORT_Memcpy(pair->group, group, sizeof(namedGroupDef)); BUG: You can't assign the result of PORT_Memcpy() like this. Does it even compile? Here and below. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:3793: } PORT_Memcpy() all this. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/307060043/diff/140001/lib/ssl/tls13con.c#newco... lib/ssl/tls13con.c:338: if (!groupDef || !ssl_NamedGroupEnabled(ss, groupDef)) { How can groupDef be null?
Sign in to reply to this message.
https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:348: } On 2016/09/07 23:30:05, ekr-rietveld wrote: > On 2016/09/02 13:23:30, franziskus wrote: > > On 2016/09/01 00:27:14, ekr-rietveld wrote: > > > Why can't you memcpy this? > > > > I could, but it's a simple struct without pointers so it's not necessary. > > Well, my point is that you can just memcpy the entire array without looping. ah, sure. can do that. https://codereview.appspot.com/307060043/diff/140001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/140001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:100: new TlsInspectorRecordHandshakeMessage(kTlsHandshakeHelloRetryRequest); On 2016/09/07 23:30:05, ekr-rietveld wrote: > Why not create this in the ctor? That would require to manually call the setup as the agent wouldn't be initialised otherwise. https://codereview.appspot.com/307060043/diff/140001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:114: for (size_t i = 1; i < ext.len() / 2; i += 1) { On 2016/09/07 23:30:05, ekr-rietveld wrote: > assert that ext.len() is even. We can't assert in functions with return value in gtests (that's at least my conclusion from trying it). I added an expect. > This is really not future-proofed if (for instance) we added an odd-length thing > to this extension. You should work in the space of group list Not sure if I understand. The supported_groups/elliptic_curves extension is a list that can only contain elements of two octets length. https://codereview.appspot.com/307060043/diff/140001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:170: groups, groups + sizeof(groups) / sizeof(SSLNamedGroup)); On 2016/09/07 23:30:05, ekr-rietveld wrote: > PR_ARRAY_SIZE Done. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:840: ssl_NamedGroupEnabled(ss, &ss->namedGroupPreferences[i])) { On 2016/09/07 23:30:06, ekr-rietveld wrote: > Do you really need to check policy? Can't you just check enabled? That's the semantic we had so far. We don't check the policy when enabling groups so we have to do it here. I also argue that that's the right way to do it. Otherwise we'd have to require that the policy is set before enabling groups in order to check that we only enable groups that work with the set policies. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:7044: PR_FALSE); On 2016/09/07 23:30:06, ekr-rietveld wrote: > IMPORTANT: You are no longer sending an alert. where? I didn't change anything here. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:12562: } On 2016/09/07 23:30:06, ekr-rietveld wrote: > PORT_Memcpy(sid->namedGroupPreferences, ss->namedGroupPreferences, sizeof())... > Done. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:442: for (i = 0; i < groupCount; i++) { On 2016/09/07 23:30:06, ekr-rietveld wrote: > Nit: we seem to be converging on ++i Done. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1031: // PORT_Assert(ss->namedGroupPreferences[i].index == i); On 2016/09/07 23:30:06, ekr-rietveld wrote: > Commented out code. Done. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1184: SSLNamedGroup toEnable[30]; /* this has to be ssl_named_group_count */ On 2016/09/07 23:30:06, ekr-rietveld wrote: > Then you should assert it Done. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1198: /* build bit vector of peer's supported groups */ On 2016/09/07 23:30:06, ekr-rietveld wrote: > This comment is now wrong. Done. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1209: toEnable[k++] = group->name; On 2016/09/07 23:30:06, ekr-rietveld wrote: > BUG: You do not appear to be checking that k is in bounds, merely that the group > is valid If data contains multiple copies of this group, you may overrun > toEnable. indeed! see comment below. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1244: } On 2016/09/07 23:30:06, ekr-rietveld wrote: > Instead of creating a toEnable temporary why not just set the entire list to > disabled and then iterate through it for each incoming code point. Then we'd be enabling groups that are actually disabled. But we can get out the enabled state first, that's a bit nicer and solves the problem with duplicate groups as well. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (right): https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslimpl.h#newcod... lib/ssl/sslimpl.h:1316: namedGroupDef namedGroupPreferences[30]; On 2016/09/07 23:30:06, ekr-rietveld wrote: > Please no numeric constants here. Then we have to move this to the heap. I'd prefer an array here. I moved it to the constant SSL_NAMED_GROUP_COUNT though. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslimpl.h#newcod... lib/ssl/sslimpl.h:1317: PRUint32 namedGroupPreferenceCount; On 2016/09/07 23:30:06, ekr-rietveld wrote: > IMPORTANT: How does this interact with renegotiation? Say I do a handshake, > which sets various curves to disabled. Won't I then not offer them on > renegotioation. No, the socket is re-used and the group preferences stay. Only the sid cache gets flushed. I added a test for this. http://searchfox.org/nss/source/lib/ssl/ssl3con.c#14114 https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1531: sizeof(namedGroupDef)); On 2016/09/07 23:30:06, ekr-rietveld wrote: > Now, here I would assign rather than memcpying wfm, is the same here anyway. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1600: for (j = 0; j < ss->namedGroupPreferenceCount; ++j) { On 2016/09/07 23:30:06, ekr-rietveld wrote: > Didn't these start true? What makes them false Ugh, they should all get disabled before enabling them again here. I added a test to check that it's actually working. I wonder if we should name the new API similar to this one (SSL_GroupPrefSet). https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1856: ssl_NamedGroupEnabled(ss, &ss->namedGroupPreferences[i])) { On 2016/09/07 23:30:06, ekr-rietveld wrote: > I'm not sure why you aren't filtering through policy before you make this list, > in which case you could just look at group->enabled not sure if I understand. We have to check all FF groups here in the order they appear in namedGroupPreferences and ssl_NamedGroupEnabled checks for policy before checking that the group is enabled. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:3674: pair->group = PORT_Memcpy(pair->group, group, sizeof(namedGroupDef)); On 2016/09/07 23:30:06, ekr-rietveld wrote: > BUG: You can't assign the result of PORT_Memcpy() like this. Does it even > compile? Here and below. ugh, not sure what happened here. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:3793: } On 2016/09/07 23:30:06, ekr-rietveld wrote: > PORT_Memcpy() all this. Done. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/307060043/diff/140001/lib/ssl/tls13con.c#newco... lib/ssl/tls13con.c:338: if (!groupDef || !ssl_NamedGroupEnabled(ss, groupDef)) { On 2016/09/07 23:30:06, ekr-rietveld wrote: > How can groupDef be null? not anymore...
Sign in to reply to this message.
https://codereview.appspot.com/307060043/diff/140001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/140001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:100: new TlsInspectorRecordHandshakeMessage(kTlsHandshakeHelloRetryRequest); On 2016/09/08 17:26:57, franziskus wrote: > On 2016/09/07 23:30:05, ekr-rietveld wrote: > > Why not create this in the ctor? > > That would require to manually call the setup as the agent wouldn't be > initialised otherwise. I meant just make capture_hrr_ in the ctor, but in retrospect that would create memory management issues. https://codereview.appspot.com/307060043/diff/140001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:114: for (size_t i = 1; i < ext.len() / 2; i += 1) { On 2016/09/08 17:26:57, franziskus wrote: > On 2016/09/07 23:30:05, ekr-rietveld wrote: > > assert that ext.len() is even. > > We can't assert in functions with return value in gtests (that's at least my > conclusion from trying it). I added an expect. > > > This is really not future-proofed if (for instance) we added an odd-length > thing > > to this extension. You should work in the space of group list > > Not sure if I understand. The supported_groups/elliptic_curves extension is a > list that can only contain elements of two octets length. yes, I'm just sad that if that were to change this code would be brittle, but OK https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:840: ssl_NamedGroupEnabled(ss, &ss->namedGroupPreferences[i])) { On 2016/09/08 17:26:57, franziskus wrote: > On 2016/09/07 23:30:06, ekr-rietveld wrote: > > Do you really need to check policy? Can't you just check enabled? > > That's the semantic we had so far. We don't check the policy when enabling > groups so we have to do it here. I also argue that that's the right way to do > it. Otherwise we'd have to require that the policy is set before enabling groups > in order to check that we only enable groups that work with the set policies. All right https://codereview.appspot.com/307060043/diff/140001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:7044: PR_FALSE); On 2016/09/08 17:26:57, franziskus wrote: > On 2016/09/07 23:30:06, ekr-rietveld wrote: > > IMPORTANT: You are no longer sending an alert. > > where? I didn't change anything here. Probably a rebase artifact. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (right): https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslimpl.h#newcod... lib/ssl/sslimpl.h:1317: PRUint32 namedGroupPreferenceCount; On 2016/09/08 17:26:58, franziskus wrote: > On 2016/09/07 23:30:06, ekr-rietveld wrote: > > IMPORTANT: How does this interact with renegotiation? Say I do a handshake, > > which sets various curves to disabled. Won't I then not offer them on > > renegotioation. > > No, the socket is re-used and the group preferences stay. Only the sid cache > gets flushed. I added a test for this. > > http://searchfox.org/nss/source/lib/ssl/ssl3con.c#14114 But this is my point: Say that I support P-348 and P-256 and the client supports only P-384. If the client re-handshakes, I believe I start out with P-256 off even if the client now offers P-256. No? https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:1856: ssl_NamedGroupEnabled(ss, &ss->namedGroupPreferences[i])) { On 2016/09/08 17:26:58, franziskus wrote: > On 2016/09/07 23:30:06, ekr-rietveld wrote: > > I'm not sure why you aren't filtering through policy before you make this > list, > > in which case you could just look at group->enabled > > not sure if I understand. We have to check all FF groups here in the order they > appear in namedGroupPreferences and ssl_NamedGroupEnabled checks for policy > before checking that the group is enabled. All right. https://codereview.appspot.com/307060043/diff/140001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:3674: pair->group = PORT_Memcpy(pair->group, group, sizeof(namedGroupDef)); On 2016/09/08 17:26:58, franziskus wrote: > On 2016/09/07 23:30:06, ekr-rietveld wrote: > > BUG: You can't assign the result of PORT_Memcpy() like this. Does it even > > compile? Here and below. > > ugh, not sure what happened here. Why didn't this fail on compilation https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:481: TEST_P(TlsConnectGenericPre13, DisabledFfdhe) { Name this MismatchDHE perhaps? https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:494: server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT); IMPORTANT: This is not the error you should be getting. If the client offers the FFDHE extension and there is a mismatch, then the server MUST NOT do DHE. https://tools.ietf.org/html/rfc7919#section-4 https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:224: TEST_P(TlsConnectStreamPre13, P384PriorityRenegotiate) { This isn't priority, its "the only one" https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:240: CheckKeys(ssl_kea_ecdh, ssl_auth_rsa_sign, 384); Where are you renegotiating? https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:13111: } I am assuming that the rest of this file is rebase artifacts and am ignoring. https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1183: PRBool enabled[30]; /* this has to be ssl_named_group_count */ Don't use 30 here. Do a #define or an enum or something https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1199: for (i = 0; i < ss->namedGroupPreferenceCount; ++i) { Note that you are making a copy.
Sign in to reply to this message.
This got much bigger than I thought it would. If we can land this tomorrow and directly land a new beta in FF we should be fine to uplift and of the week. Otherwise we should probably think of something else. Main changes in the latest patch are that ssl_LookupNamedGroup uses the configured groups now, except when called from ConvertToSID (for which we have no tests). I also fixed a bug in the TLS 1.3 key share extension handling, i.e. we failed to read the key share when we didn't know the group and thus kept on interpreting the key share as groups. Not sure why this worked before. > But this is my point: > Say that I support P-348 and P-256 and the client supports only P-384. If the > client re-handshakes, I believe I start out with P-256 off even if the client > now offers P-256. No? (I assume you meant P-384 at the end.) But I don't see how this is influencing renegotiation. If the client advertises only P384, that's what the server always picks. (Yes we'll see thatP256 is enabled on the server, but it won't be used.) The other way round, client P384 and P256 with server only P384, should look the same. Or am I missing something here? > > > BUG: You can't assign the result of PORT_Memcpy() like this. Does it even > > > compile? Here and below. > > > > ugh, not sure what happened here. > > Why didn't this fail on compilation Actually, this is correct. memcpy returns the destination pointer. (Not that I think we should do that here, but it is ok to do.) https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:481: TEST_P(TlsConnectGenericPre13, DisabledFfdhe) { On 2016/09/10 19:09:50, ekr-rietveld wrote: > Name this MismatchDHE perhaps? Done. https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:494: server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT); On 2016/09/10 19:09:50, ekr-rietveld wrote: > IMPORTANT: This is not the error you should be getting. If the client offers the > FFDHE extension and there is a mismatch, then the server MUST NOT do DHE. > > https://tools.ietf.org/html/rfc7919#section-4 right, I had to change ssl_LookupNamedGroup to use the configured curves as well. We should get the right errors now. https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:224: TEST_P(TlsConnectStreamPre13, P384PriorityRenegotiate) { On 2016/09/10 19:09:50, ekr-rietveld wrote: > This isn't priority, its "the only one" Done. https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_ecdh_unittest.cc:240: CheckKeys(ssl_kea_ecdh, ssl_auth_rsa_sign, 384); On 2016/09/10 19:09:50, ekr-rietveld wrote: > Where are you renegotiating? looks like I optimised this out... doing it again. https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3con.c#newcod... lib/ssl/ssl3con.c:13111: } On 2016/09/10 19:09:50, ekr-rietveld wrote: > I am assuming that the rest of this file is rebase artifacts and am ignoring. Acknowledged. https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1183: PRBool enabled[30]; /* this has to be ssl_named_group_count */ On 2016/09/10 19:09:50, ekr-rietveld wrote: > Don't use 30 here. Do a #define or an enum or something Done, only forgot to use it here... https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1199: for (i = 0; i < ss->namedGroupPreferenceCount; ++i) { On 2016/09/10 19:09:51, ekr-rietveld wrote: > Note that you are making a copy. I make a copy of the enabled state before resetting things, yep. (If that's what you mean.)
Sign in to reply to this message.
On 2016/09/12 03:55:16, franziskus wrote: > This got much bigger than I thought it would. If we can land this tomorrow and > directly land a new beta in FF we should be fine to uplift and of the week. > Otherwise we should probably think of something else. > > Main changes in the latest patch are that ssl_LookupNamedGroup uses the > configured groups now, except when called from ConvertToSID (for which we have > no tests). I'll review this tomorrow. > I also fixed a bug in the TLS 1.3 key share extension handling, i.e. we failed > to read the key share when we didn't know the group and thus kept on > interpreting the key share as groups. Not sure why this worked before. Nice catch. What exercises this case, thought? > > But this is my point: > > Say that I support P-348 and P-256 and the client supports only P-384. If the > > client re-handshakes, I believe I start out with P-256 off even if the client > > now offers P-256. No? > > (I assume you meant P-384 at the end.) But I don't see how this is influencing > renegotiation. If the client advertises only P384, that's what the server always > picks. (Yes we'll see thatP256 is enabled on the server, but it won't be used.) > The other way round, client P384 and P256 with server only P384, should look the > same. Or am I missing something here? What I'm saying is that if in round 1, the client doesn't support P-384, it seems like the server forgets that it also supports P-384 even if the client offers it in round 2. Am I wrong? > > > > BUG: You can't assign the result of PORT_Memcpy() like this. Does it even > > > > compile? Here and below. > > > > > > ugh, not sure what happened here. > > > > Why didn't this fail on compilation > > Actually, this is correct. memcpy returns the destination pointer. (Not that I > think we should do that here, but it is ok to do.) > > https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... > File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): > > https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... > external_tests/ssl_gtest/ssl_dhe_unittest.cc:481: TEST_P(TlsConnectGenericPre13, > DisabledFfdhe) { > On 2016/09/10 19:09:50, ekr-rietveld wrote: > > Name this MismatchDHE perhaps? > > Done. > > https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... > external_tests/ssl_gtest/ssl_dhe_unittest.cc:494: > server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT); > On 2016/09/10 19:09:50, ekr-rietveld wrote: > > IMPORTANT: This is not the error you should be getting. If the client offers > the > > FFDHE extension and there is a mismatch, then the server MUST NOT do DHE. > > > > https://tools.ietf.org/html/rfc7919#section-4 > > right, I had to change ssl_LookupNamedGroup to use the configured curves as > well. We should get the right errors now. > > https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... > File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): > > https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... > external_tests/ssl_gtest/ssl_ecdh_unittest.cc:224: TEST_P(TlsConnectStreamPre13, > P384PriorityRenegotiate) { > On 2016/09/10 19:09:50, ekr-rietveld wrote: > > This isn't priority, its "the only one" > > Done. > > https://codereview.appspot.com/307060043/diff/160001/external_tests/ssl_gtest... > external_tests/ssl_gtest/ssl_ecdh_unittest.cc:240: CheckKeys(ssl_kea_ecdh, > ssl_auth_rsa_sign, 384); > On 2016/09/10 19:09:50, ekr-rietveld wrote: > > Where are you renegotiating? > > looks like I optimised this out... doing it again. > > https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3con.c > File lib/ssl/ssl3con.c (right): > > https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3con.c#newcod... > lib/ssl/ssl3con.c:13111: } > On 2016/09/10 19:09:50, ekr-rietveld wrote: > > I am assuming that the rest of this file is rebase artifacts and am ignoring. > > Acknowledged. > > https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3ecc.c > File lib/ssl/ssl3ecc.c (right): > > https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3ecc.c#newcod... > lib/ssl/ssl3ecc.c:1183: PRBool enabled[30]; /* this has to be > ssl_named_group_count */ > On 2016/09/10 19:09:50, ekr-rietveld wrote: > > Don't use 30 here. Do a #define or an enum or something > > Done, only forgot to use it here... > > https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3ecc.c#newcod... > lib/ssl/ssl3ecc.c:1199: for (i = 0; i < ss->namedGroupPreferenceCount; ++i) { > On 2016/09/10 19:09:51, ekr-rietveld wrote: > > Note that you are making a copy. > > I make a copy of the enabled state before resetting things, yep. (If that's what > you mean.)
Sign in to reply to this message.
This seems pretty close modulo the issues below https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1199: for (i = 0; i < ss->namedGroupPreferenceCount; ++i) { On 2016/09/12 03:55:15, franziskus wrote: > On 2016/09/10 19:09:51, ekr-rietveld wrote: > > Note that you are making a copy. > > I make a copy of the enabled state before resetting things, yep. (If that's what > you mean.) Sorry, I meant add it to the comment. https://codereview.appspot.com/307060043/diff/180001/external_tests/ssl_gtest... File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/307060043/diff/180001/external_tests/ssl_gtest... external_tests/ssl_gtest/ssl_dhe_unittest.cc:495: client_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP); Why do these produce different codes? https://codereview.appspot.com/307060043/diff/180001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/180001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:1184: PR_ASSERT(ssl_named_group_count == PR_ARRAY_SIZE(enabled)); PORT_Assert https://codereview.appspot.com/307060043/diff/180001/lib/ssl/ssl3ext.c File lib/ssl/ssl3ext.c (right): https://codereview.appspot.com/307060043/diff/180001/lib/ssl/ssl3ext.c#newcod... lib/ssl/ssl3ext.c:3308: } Nice catch. Why don't you just put: if (!groupDef) { return SECSuccess; } after line 3314. https://codereview.appspot.com/307060043/diff/180001/lib/ssl/sslcert.c File lib/ssl/sslcert.c (right): https://codereview.appspot.com/307060043/diff/180001/lib/ssl/sslcert.c#newcod... lib/ssl/sslcert.c:152: cert->certType.namedCurve->name != certType->namedCurve->name) { Was this a bug before? https://codereview.appspot.com/307060043/diff/180001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/180001/lib/ssl/sslsock.c#newcod... lib/ssl/sslsock.c:3598: } I think it would be better to have two separate functions here, because the semantics are very different.
Sign in to reply to this message.
https://codereview.appspot.com/307060043/diff/180001/external_tests/ssl_gtest... File external_tests/ssl_gtest/tls_agent.cc (right): https://codereview.appspot.com/307060043/diff/180001/external_tests/ssl_gtest... external_tests/ssl_gtest/tls_agent.cc:126: ConfigNamedGroups(groups, PR_ARRAY_SIZE(groups)); This should probably include the 2048-bit ffdhe group as well. https://codereview.appspot.com/307060043/diff/180001/external_tests/ssl_gtest... File external_tests/ssl_gtest/tls_agent.h (right): https://codereview.appspot.com/307060043/diff/180001/external_tests/ssl_gtest... external_tests/ssl_gtest/tls_agent.h:144: void ConfigNamedGroups(const SSLNamedGroup* groups, uint8_t num); size_t instead of uint8_t https://codereview.appspot.com/307060043/diff/180001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/180001/lib/ssl/ssl3ecc.c#newcod... lib/ssl/ssl3ecc.c:436: groups = ss->namedGroupPreferences; namedGroupPreferences can be (const namedGroupDef *) https://codereview.appspot.com/307060043/diff/180001/lib/ssl/sslimpl.h File lib/ssl/sslimpl.h (right): https://codereview.appspot.com/307060043/diff/180001/lib/ssl/sslimpl.h#newcod... lib/ssl/sslimpl.h:615: namedGroupDef namedGroupPreferences[SSL_NAMED_GROUP_COUNT]; I'd much prefer if this were const namedGroupDef *[]. That complicates the code that uses this of course. That probably means initializing a list, probably using PR_CallOnce. https://codereview.appspot.com/307060043/diff/180001/lib/ssl/sslimpl.h#newcod... lib/ssl/sslimpl.h:1089: namedGroupDef *group; const
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
