1296239 - Allow changing ssl_named_groups priorities

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

Attachment: groupPriority.patch

We should allow setting group priorities, not only enable/disable.
I suggest changing SSL_NamedGroupPrefSet to take a priority instead of true/false (0 disables). This in particular also allows the a client to send a key share that is not of the first group in the list, which is needed for testing.

The attached WIP patch does this. Not sure if we want this for <= 1.2 as well.

Comment 1

[Martin Thomson [:mt:]]

Comment on attachment 8782347 [details] [diff] [review]
groupPriority.patch

Review of attachment 8782347 [details] [diff] [review]:
-----------------------------------------------------------------

From what I'm seeing, this doesn't help with the problem that we have with 25519 and key_share, it still only picks one group to generate a key for.

I would prefer to see a simple ordering of groups over something like this.  This is hard to use, and certainly hard to get right.

::: external_tests/ssl_gtest/ssl_ecdh_unittest.cc
@@ +97,5 @@
> +
> +  CheckKeys(ssl_kea_ecdh, ssl_auth_rsa_sign, 384);
> +  auto enabled_groups = [](uint16_t group) {
> +    // we can't actually check which group was used, only which ones are enabled
> +    EXPECT_TRUE(0x017U == group || 0x018U == group || 0x019U == group)

There are values you can use for this (ssl_grp_ec_...)

Comment 2

[Franziskus Kiefer [:franziskus]]

> From what I'm seeing, this doesn't help with the problem that we have with 25519 and key_share, it still only picks one group to generate a key for.

No, it doesn't solve that problem (and it's not supposed to), but it allows to trigger the problem. It's not possible at the moment to run into the problem because we always send the key_share of the first enabled group. So we either end up with no cipher overlap or with matching key shares.

> I would prefer to see a simple ordering of groups over something like this.  This is hard to use, and certainly hard to get right.

Sure, will do if we agree that setting priorities in SSL_NamedGroupPrefSet is what we want.

Comment 3

[Martin Thomson [:mt:]]

To be clear, I'm not sure that we want the complication of having prioritization of groups.  When it comes to generating multiple key shares, the only outcome I see is a greatly increased complexity.  My point was that this makes it more difficult to generate shares for P-256 and 25519 because we have to build a prioritization scheme that allows users to set that as well (unless we had some sort of separate idea of equivalence that we wouldn't allow users to change, which would suck for those users, I think).  If we are going to prioritize, we need to find some way to order things properly.

One API that might work is:

SSL_NamedGroupPrefSet(ss, pref_order, PR_ARRAY_SIZE(pref_order));
  --> pref_list == NULL to accept the defaults
SSL_GenerateTls13KeyShares(ss, group);
  --> which would actually do keygen, by default you get keygen on the first named group only

I was going to point out that NSS has generally been opinionated about what preferences it has, but I think that's softened lately.

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 8782347 [details] [diff] [review]
groupPriority.patch

Review of attachment 8782347 [details] [diff] [review]:
-----------------------------------------------------------------

This is kinda complicated. Can you put it on Rietveld?

Comment 5

[Franziskus Kiefer [:franziskus]]

(In reply to Eric Rescorla (:ekr) from comment #4)
> Comment on attachment 8782347 [details] [diff] [review]
> groupPriority.patch
> 
> Review of attachment 8782347 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is kinda complicated. Can you put it on Rietveld?

sure https://codereview.appspot.com/307060043

(In reply to Martin Thomson [:mt:] from comment #3)
> To be clear, I'm not sure that we want the complication of having
> prioritization of groups.

If you have a way to test the problematic case of P256/25519 equal priority, having priorities might not be necessary. But I don't see how that should work right now.

> One API that might work is:
> 
> SSL_NamedGroupPrefSet(ss, pref_order, PR_ARRAY_SIZE(pref_order));
>   --> pref_list == NULL to accept the defaults
> SSL_GenerateTls13KeyShares(ss, group);
>   --> which would actually do keygen, by default you get keygen on the first
> named group only

That would work as well. But it would require a complete group list even if you want to enable/disable only a single group. Maybe a second API (e.g. SSL_NamedGroupOrderSet) would be better.

Comment 6

[Franziskus Kiefer [:franziskus]]

I updated the patch at https://codereview.appspot.com/307060043 that adds a new function SSL_NamedGroupConfig now to set a custom group list.

Comment 7

[Martin Thomson [:mt:]]

I'd like to see this replace the existing configuration option.  It doesn't make sense to offer two ways to do the same thing in the same release.  Let's try to get this finished before we finalize 3.27.

Comment 8

[Kai Engert [:KaiE:]]

This bug blocks the release of NSS 3.27, as it seems you want to change the newly added API (which must be stable prior to the release).

Could you please make this bug a high priority? Is there an ETA for getting it fixed?
It would be good to be able to finalize the NSS 3.27 release soon.
Thanks

Comment 9

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/07593068fee9

Comment 10

[Franziskus Kiefer [:franziskus]]

follow up for more than suite b

https://hg.mozilla.org/projects/nss/rev/64dce9501f81