1286141 - Implement a control for enabling/disabling groups

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Martin Thomson [:mt:]]

This should make it easier to test bug 1286140.  And it seems separable.  Also, it gives us a chance to improve the API introduced in bug 102794.

Comment 1

[Martin Thomson [:mt:]]

Kai, Tim, I've added some code that enables configuration for named groups.

http://codereview.appspot.com/305750043

I just now realize that I added a regression in Bug 1266237.  The SSL_DHEGroupPrefSet() documentation says that the first item in that list is used as the default by the server.  This is no longer the case.  We now just pick the first from the list that we maintain here: http://searchfox.org/nss/source/lib/ssl/sslsock.c#144-174

Kai, if this is a problem that you feel we have to fix, then let me know and I'll open a bug to fix it.  It's annoying, but relatively trivial to patch.  Even if we don't fix it, we should fix the documentation for SSL_DHEGroupPrefSet().

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

LGTM + some comments on Rietveld

Comment 3

[Kai Engert (:KaiE:)]

(In reply to Martin Thomson [:mt:] from comment #1)
> I just now realize that I added a regression in Bug 1266237.

A regression to TLS 1.3?

We shouldn't ship NSS 3.25 with a known regression, if possible.

Comment 4

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #3)
> (In reply to Martin Thomson [:mt:] from comment #1)
> > I just now realize that I added a regression in Bug 1266237.
> 
> A regression to TLS 1.3?

This was a typo. Of course I wanted to ask "a regression to TLS 1.2 ?".

Comment 5

[Kai Engert (:KaiE:)]

(In reply to Martin Thomson [:mt:] from comment #1)
> I just now realize that I added a regression in Bug 1266237.  The
> SSL_DHEGroupPrefSet() documentation says that the first item in that list is
> used as the default by the server.  This is no longer the case.  We now just
> pick the first from the list that we maintain here:


Can we think of a good reason why a server programmer might want to configure a preferred group, which is used in absence of other influence factors, then we should keep that promised behavior. (RHEL has already shipped this API, but I don't know if anyone has started to rely on it.)

Comment 6

[Hubert Kario]

I haven't looked at new code so don't know what it exactly does, but there are two situations in which you will want to use a specific group if the client have not advertised support for FFDHE groups in supported_groups extensions:

 1. relatively old Java supports at most 1024 bit DHE parameters, code we ship in RHEL now (3.21) supports that via special option that generates parameters on the fly. How does that work together with FFHDE negotiation?
 2. current java still has a maximum of 2048 bit DHE (they didn't remove the limit, they just increased a little bit), so defaulting to 2048 for some servers may be a necessity

at the same time, we have NIST that is telling people to use the highest sized parameters they can

So a situation in which you may want to negotiate 4096 bit or 8192 bit with clients that support it, while still defaulting to 2048 or (even) 1024 in absence of FFDHE groups in extension, is not too far fetched, IMHO.

Comment 7

[Martin Thomson [:mt:]]

The regression in question is that - in TLS 1.2 - a server that is configured to pick a larger group over a smaller one will get the smallest group that is configured.

SSL_DHEPrefSet() with [2048, 4096] results in 2048.
SSL_DHEPrefSet() with [4096, 2048] results in 2048. -- regression

To answer Hubert, if the weak group is enabled that is always picked, as before.

Note that I am not aware of anyone providing the negotiated groups.  We might be the only ones to do that right now, and that is only if TLS 1.3 or the new FFDHE option is enabled.  As above, if the client signals support for ffdhe groups we will pick the smallest group that we have in common (absent this signal, we assume that all groups are supported).

Comment 8

[Hubert Kario]

> Note that I am not aware of anyone providing the negotiated groups.  We might be the only ones to do that right now

it's still just a draft, not an RFC...

Comment 9

[Martin Thomson [:mt:]]

True.  But I'm also acutely aware that many major implementations have declared that they don't intend to implement ffdhe.

(n.b., RFC 7919: https://www.rfc-editor.org/auth48/C255)

Comment 10

[Hubert Kario]

Are you trying to tell me that OpenSSL has done that? can you point me to it?

(Yes, I know that google's not interested, but google is google, most companies are not google)

Comment 11

[Martin Thomson [:mt:]]

Attachment: bug1286141-1.patch

Kai, I have this up on rietveld here: https://codereview.appspot.com/305750043

Note that rietveld tends to pick up changes when you rebase and this might show some artifacts of the other change that I just landed.

Comment 12

[Kai Engert (:KaiE:)]

+;+NSS_3.26 {    # NSS 3.26 release

Now we're at 3.27


Array ssl_named_groups has 29 entries.
Enum SSLNamedGroup defines 30 entries (31 minus custom).

Intentional, or did you miss something?

Comment 13

[Kai Engert (:KaiE:)]

When you rebased, did you accidentally or intentionally remove dhePreferredGroup (which you just added a few days ago) ?


+            PRUint32 bit = 1U << ssl_named_groups[i].index;

Do you think the number of named groups might ever grow larger then 32?
If yes, do you want a (debug) assertion for (index < sizeof(bit)*8) ?

Comment 14

[Kai Engert (:KaiE:)]

(In reply to Martin Thomson [:mt:] from comment #11)
> 
> Note that rietveld tends to pick up changes when you rebase and this might
> show some artifacts of the other change that I just landed.

Yes, the patch on rietveld shows you're removing the new code.

Comment 15

[Martin Thomson [:mt:]]

(In reply to Kai Engert (:kaie) from comment #12)
> +;+NSS_3.26 {    # NSS 3.26 release
> 
> Now we're at 3.27

Thanks.

> Array ssl_named_groups has 29 entries.
> Enum SSLNamedGroup defines 30 entries (31 minus custom).
> 
> Intentional, or did you miss something?

That's a long-standing bug (or feature?).  Apparently we never supported sect571k1, though we had a codepoint defined for it.  I could add it to the list, but given that we don't 

(In reply to Kai Engert (:kaie) from comment #13)
> Do you think the number of named groups might ever grow larger then 32?
> If yes, do you want a (debug) assertion for (index < sizeof(bit)*8) ?

Given that we don't actually have implementations for most of the curves, the hope is to remove those shortly.  See http://searchfox.org/nss/rev/cd38b98fc1c63809fc65d3dc1e512d69591e6a80/lib/ssl/sslsock.c#179 for the assert.

(In reply to Kai Engert (:kaie) from comment #14)
> Yes, the patch on rietveld shows you're removing the new code.

The changes on rietveld incorrectly show that this removes new code.  It does not.  You can see that in the splinter patch.  I've made you a new one:

http://codereview.appspot.com/304280043

Comment 16

[Kai Engert (:KaiE:)]

(In reply to Martin Thomson [:mt:] from comment #15)
> 
> http://codereview.appspot.com/304280043

r=kaie

Comment 17

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/3ed709ba186a

Comment 18

[Kai Engert (:KaiE:)]

This appears to have broken ssl_gtest on Fedora.

e.g. on this builder: https://bot.nss-crypto.org:8011/builders/fedora-x32-DBG
First failed build: https://bot.nss-crypto.org:8011/builders/fedora-x32-DBG/builds/829

[  FAILED  ] 10 tests, listed below:
[  FAILED  ] GenericStream/TlsConnectGeneric.ConnectEcdheP384/0, where GetParam() = ("TLS", 771)
[  FAILED  ] GenericStream/TlsConnectGeneric.ConnectEcdheP384/1, where GetParam() = ("TLS", 770)
[  FAILED  ] GenericStream/TlsConnectGeneric.ConnectEcdheP384/2, where GetParam() = ("TLS", 769)
[  FAILED  ] GenericStream/TlsConnectGeneric.ConnectEcdheGroupMismatch/0, where GetParam() = ("TLS", 771)
[  FAILED  ] GenericStream/TlsConnectGeneric.ConnectEcdheGroupMismatch/1, where GetParam() = ("TLS", 770)
[  FAILED  ] GenericStream/TlsConnectGeneric.ConnectEcdheGroupMismatch/2, where GetParam() = ("TLS", 769)
[  FAILED  ] GenericDatagram/TlsConnectGeneric.ConnectEcdheP384/0, where GetParam() = ("DTLS", 771)
[  FAILED  ] GenericDatagram/TlsConnectGeneric.ConnectEcdheP384/1, where GetParam() = ("DTLS", 770)
[  FAILED  ] GenericDatagram/TlsConnectGeneric.ConnectEcdheGroupMismatch/0, where GetParam() = ("DTLS", 771)
[  FAILED  ] GenericDatagram/TlsConnectGeneric.ConnectEcdheGroupMismatch/1, where GetParam() = ("DTLS", 770)

Comment 19

[Martin Thomson [:mt:]]

Kai, I realize that I never got around to responding here.  I see that the builders are mostly happy recently.  Can we close this now?

Comment 20

[Kai Engert (:KaiE:)]

yes, those failures seem to be gone, thanks