Closed Bug 102794 Opened 23 years ago Closed 9 years ago

Implement the server-side code of the DHE SSL ciphersuites.

Categories

(NSS :: Libraries, enhancement, P2)

Product:
NSS
Component:
Libraries
Version:
3.3.1
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: wtc, Assigned: KaiE)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 17 obsolete files)

dhe-api.patch
3.45 KB, patch
mt
: superreview+
Details | Diff | Splinter Review
v39.patch
122.46 KB, patch
KaiE
: review+
Details | Diff | Splinter Review 
Dr. Stephen Henson contributed the client-side implementation
of the DHE SSL ciphersuites.  We still need the server-side
code of these ciphersuites.
Target NSS 3.4, but not a P1.
Assignee: wtc → nelsonb
Priority: -- → P2
Target Milestone: --- → 3.4
Changed the QA contact to Bishakha.
QA Contact: sonja.mirtitsch → bishakhabanerjee
Set target milestone to NSS 3.5.
Target Milestone: 3.4 → 3.5
Target Milestone: 3.5 → 3.7
*** Bug 172240 has been marked as a duplicate of this bug. ***
Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.
Target Milestone: 3.7 → 3.8
Remove target milestone of 3.8, since these bugs didn't get into that release.
Target Milestone: 3.8 → ---
Did any of the ECC changes affect the missing DHE server-side support ?

(In reply to comment #7)
> Did any of the ECC changes affect the missing DHE server-side support ?

When we added the ECDHE cipher suites, we did not add server-side support
for DHE ciphers. However, the ECDHE and DHE cipher suites are similar enough
that, at least theoretically,  one should be able to add this functionality by
essentially mirroring the ECDHE code.

vipul
QA Contact: bishakhabanerjee → jason.m.reid
Keywords: helpwanted
QA Contact: jason.m.reid → libraries
Attached patch working patch (obsolete) — — Splinter Review 
No ready for review. There are some unaddressed problems left.
I am intentionally "sitting" on server side enhancement requests that were 
not initiated by developers of NSS-based server products.  

There are other aspects of NSS that need attention more urgently than 
this enhancement.   Let's focus on them.
Nelson,

This functionality has been requested a few times by customers at Sun, so I don't think we should just sit on it, even though I agree that it's not high priority right now.

The lack of the server-side support for these cipher suites is a very ugly gap that cannot be expressed in our API - there is no SSL API that says a cipher suite is supported for server or client specifically. It also means we have zero QA for these cipher suites that we support only as client today. So, it would be a good thing to close this gap.

RFC 4306 suggests the following 1024 bit prime and generator:

   The prime is 2^1024 - 2^960 - 1 + 2^64 * { [2^894 pi] + 129093 }.
   Its hexadecimal value is:

        FFFFFFFF FFFFFFFF C90FDAA2 2168C234 C4C6628B 80DC1CD1 29024E08
        8A67CC74 020BBEA6 3B139B22 514A0879 8E3404DD EF9519B3 CD3A431B
        302B0A6D F25F1437 4FE1356D 6D51C245 E485B576 625E7EC6 F44C42E9
        A637ED6B 0BFF5CB6 F406B7ED EE386BFB 5A899FA5 AE9F2411 7C4B1FE6
        49286651 ECE65381 FFFFFFFF FFFFFFFF

   The generator is 2.

Nelson, this answered a long-time question of mine.  This prime and
generator are what Microsoft IIS is using.  See bug 348359 comment 54.
Our AllPeers extension for Firefox 2.0 uses NSS for implementation of SSL server. Because DHE key exchange is currently a standard for TLS session establishment for its 'perfect forward secrecy' property we would like to be capable of it. We want to offer our users maximally secured P2P connections not only because of upcoming payed features.

I would like to ask what is status and target milestone of this bug. There is an option for us to apply valid and reviewed patches to NSS source code and link with NSS statically. Anyway, I would like to prevent this step.

For info: we are not using Necko's PSM "SSL" socket provider because it is not suitable for implementation of a secure server, even non-scalable.
In light of http://tools.ietf.org/html/draft-sheffer-tls-bcp-00, this may become a pretty serious gap for server usage of NSS. Any progress on this?
Chris,

NSS supports the server-side for ECDHE, just not for DHE/RSA.
(In reply to Alexei Volkov from comment #9)
> 
> No ready for review. There are some unaddressed problems left.

I wish he had said WHICH problems are unaddressed!
Attached patch patch v2, merged and adjusted (obsolete) — — Splinter Review 
This is an attempt to merge Alexei's old patch to current NSS.

I excluded the #ifdef wrappers, it seems unnecessary.

I skipped the minor changes to tests, they seem unrelated.

The old code attempted to reuse a different nickname command line parameter to search for a DSA certificate in the database. That was gone, so I decided to add a new, separate parameter.
Attachment #238120 - Attachment is obsolete: true
This code doesn't work yet, at least in some scenarios.

I attempted to start a "selfserv" process, using a RSA server certificate created by the NSS test suite (directory "server", nickname "localhost.localdomain"), I tested -c o (TLS_DHE_DSS_WITH_RC4_128_SHA) and -c x (TLS_DHE_RSA_WITH_AES_256_CBC_SHA), and used tstclnt to connect to the server.

In both scenarios, the server stuck into an endless loop with 100% cpu usage, I gave up after 20 minutes, the calculation never succeeded.
The reason for the "never found a prime" was apparently, the code requested a 8192-bit sized prime, which takes too long to find. The new code used a bit-sized parameter when it should have provided a byte-size, so we use factor 8 too much...

After fixing the above, a client connecting to the NSS server gets SSL_ERROR_RX_MALFORMED_SERVER_KEY_EXCH.

Looks like additional fixes to the patch are required.
Attached patch patch v3 (obsolete) — — Splinter Review 
(In reply to Kai Engert (:kaie) from comment #20)
> After fixing the above, a client connecting to the NSS server gets
> SSL_ERROR_RX_MALFORMED_SERVER_KEY_EXCH.
> 
> Looks like additional fixes to the patch are required.

That's because TLS 1.2 was used in my test, which requires additional signature and hash information in the server key exchange handshake message (which the old patch obviously didn't add yet).

If I force TLS 1.0 and use tstclnt -c u (TLS_DHE_RSA_WITH_AES_128_CBC_SHA) the connection succeeds.

The openssl equivalent of the cipher is DHE-RSA-AES128-SHA.
I can also get a successful connection using an openssl client:
  openssl s_client -tls1 -cipher DHE-RSA-AES128-SHA

The remaining work is (at least):
- add TLS 1.2 support
- decide if the prime sizes in ssl3_CreateDHParams can remain fixed values
  or not
- add tests
- potentially allow 
    SSL_ConfigSecureServerWithCertChain / ssl_ConfigSecureServer
  to read pre-generated DH params from a file, instead of re-generating
  them every time
Attachment #8562954 - Attachment is obsolete: true
Attached patch Patch v4 (obsolete) — — Splinter Review 
Patch v4 adds:

- TLS 1.2 support ("openssl s_client -tls1_2" succeeds).

- selfserv -G can be used to generate permanent DH parameters (save to file).
  File format is DER, same binary encoding as used by "openssl dhparam".
  (requires new SSL library API: SSL_GenerateDHParams)

- selfserv -F can be used to load pre-generated DH parameters from file.
  (requires new SSL library API: SSL_SetDHParams)

Because the existing code still allows "step down", the implementation will create/consume two sets of parameters (read from/write to two files), unless step down is disabled.
Assignee: nelson → kaie
Attachment #8563024 - Attachment is obsolete: true
Attachment #8564199 - Flags: review?(rrelyea)
Comment on attachment 8564199 [details] [diff] [review]
Patch v4

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

TL/DR I've made lots of comments, but only 1 needs action: ssl.def


You can change ssl.def. the SECStatus without further review, and new comments without further review.

The kt_dh -> dsa cert comments are just informative.

I was initially OK with ripping out the dh_export code, but then I realized we already support the client side of that code, so it's probably best to leave it in, even though I documented throughout the review where it can be ripped out. I think we will be ripping it out at some point, but since we already support the client side, it's probably best to leave it in this patch.

::: cmd/selfserv/selfserv.c
@@ +1839,5 @@
>          }
>      }
>  }
>  
> +int

It might be better for this to be a SECStatus and return SECSuccess or SECFailure..

@@ +2029,5 @@
>      }
>  
>      for (kea = kt_rsa; kea < kt_kea_size; kea++) {
>  	if (cert[kea] != NULL) {
> +	    if (inputDHParamBasename && (kea == kt_dh || kea == kt_rsa)) {

A comment here that DHE ciphers uses signing certs. DSA is stored under kt_dh and rsa is stored under kt_rsa. See comment in ssl3 code as well.

@@ +2037,5 @@
> +
> +		paramsRegular = ReadDHParamsFromFile(inputDHParamBasename, ".der");
> +		if (!paramsRegular)
> +		    loadFailed = PR_TRUE;
> +		if (!disableStepDown) {

I don't think we really need to support dhe_export and stepdown.
I suspect we'll be turning export ciphers off. I would be OK if you ripped out all the stepDown and export cipher related code. I'll point it out in the rest of the review.

@@ +2505,5 @@
> +
> +	paramsRegular = SSL_GenerateDHParams(128);
> +	rv = WriteDHParamsToFile(paramsRegular, outputDHParamBasename, ".der");
> +	if (rv != 0) return rv;
> +	if (!disableStepDown) {

More stepdown code you could skip.

@@ +2510,5 @@
> +	    paramsStepDown = SSL_GenerateDHParams(64 /*EXPORT_RSA_KEY_LENGTH*/);
> +	    rv = WriteDHParamsToFile(paramsStepDown, outputDHParamBasename,
> +				     "-stepdown.der");
> +	}
> +	return rv;

If you change WriteDH to be a SECStatus, you should probably also change this return. In fact exit(x) would be better since this is main.

::: lib/ssl/ssl.def
@@ +170,5 @@
>  SSL_SetCanFalseStartCallback;
>  ;+    local:
>  ;+*;
>  ;+};
> +;+NSS_3.18 {    # NSS 3.18 release

Make sure you update this before you check in. I think the current is 3.19.:)

::: lib/ssl/ssl.h
@@ +826,5 @@
> +SSL_IMPORT DHParams *
> +SSL_GenerateDHParams(int primeLenInBytes);
> +
> +SSL_IMPORT SECStatus
> +SSL_SetDHParams(PRFileDesc *fd, DHParams *paramsRegular, DHParams *paramsStepDown);

more stepdown code...

::: lib/ssl/ssl3con.c
@@ +772,5 @@
>  	     * that the server uses an RSA cert for (EC)DHE-RSA.
>  	     */
>  	    switch (cipher_def->key_exchange_alg) {
>  	    case kea_ecdhe_rsa:
> +	    case kea_dhe_dss_export:

more step down code (well export). see comment in selfserv.c

@@ +7588,5 @@
>      }
> +    else if (kea_def->kea == kea_dhe_dss ||
> +             kea_def->kea == kea_dhe_rsa ||
> +             kea_def->kea == kea_dhe_dss_export ||
> +             kea_def->kea == kea_dhe_rsa_export) {

More export/step down.

@@ +8765,5 @@
> +          break;
> +      case kea_dhe_dss_export:
> +      case kea_dhe_rsa_export:
> +          dhParams = ss->dheStepDownParams->dhParams;
> +          break;

More stepdown/export code.

@@ +8818,5 @@
> +    
> +    if (kea_def->kea == kea_dhe_rsa)
> +        certIndex = kt_rsa;
> +    else 
> +        certIndex = kt_dh;

We're reusing key exchange for ephemeral. There is precedence, we do the same for ecdhe. There are 2 semantics this will generate:

1) if we use dhe and rsa ciphers, both will use the same RSA cert. That cert would need both encrypt and signing usages. We currently do this for ecdhe_rsa.

2) we reuse kt_dh for a DSA cert. This means we can't turn dh and dhe_dss on a the same time. This is also true of ecdh and ecdhe_ecdsa

This is not a problem, just making clear what the code is doing.

@@ +9595,5 @@
>  skip:
> +    if (kea_def->kea == kea_dhe_dss ||
> +        kea_def->kea == kea_dhe_dss_export ||
> +        kea_def->kea == kea_dhe_rsa ||
> +        kea_def->kea == kea_dhe_rsa_export) {

More export/stepdown code.

@@ +10895,5 @@
>      }
>  
> +    if (ss->ssl3.hs.kea_def->kea == kea_ecdhe_rsa ||
> +        ss->ssl3.hs.kea_def->kea == kea_dhe_rsa ||
> +        ss->ssl3.hs.kea_def->kea == kea_dhe_rsa_export) {

more export/stepdown

@@ +12222,5 @@
> +    }
> +    if (ss->dheStepDownParams) {
> +        ssl3_FreeDHParams(ss->dheStepDownParams);
> +        ss->dheStepDownParams = NULL;
> +    }

more export/stepdown.

@@ +12232,5 @@
> +    if (paramsStepDown) {
> +	ss->dheStepDownParams = ssl3_NewDHParams((SECKEYDHParams*)paramsStepDown);
> +	if (!ss->dheStepDownParams) {
> +	    return SECFailure;      /* error is set */
> +	}

more export/stepdown.

@@ +12257,5 @@
> +    }
> +
> +    if (noStepDown == PR_TRUE) {
> +        return SECSuccess;
> +    }

more export/stepdown here, and immediately below..

::: lib/ssl/sslimpl.h
@@ +1214,5 @@
>  
>      ssl3KeyPair *         stepDownKeyPair;	/* RSA step down keys */
>  
> +    ssl3DHParams *      dheParams;          /* DHE param */
> +    ssl3DHParams *      dheStepDownParams;  /* DHE step down param */

more export/setpdown

::: lib/ssl/sslsecur.c
@@ +790,5 @@
>              goto loser;
>          }
>      }
> +    if (kea == kt_dh || kea == kt_rsa) {
> +        if (ssl3_EnsureDHParams(ss, ss->opt.noStepDown) != SECSuccess) {

more export/stepdown.

::: lib/ssl/sslsock.c
@@ +270,5 @@
> +            ss->dheKeyPair = !os->dheKeyPair ? NULL :
> +                             ssl3_GetKeyPairRef(os->dheKeyPair);
> +            ss->dheParams = !os->dheParams ? NULL :
> +                            ssl3_GetDHParamsRef(os->dheParams);
> +            ss->dheStepDownParams = !os->dheStepDownParams ? NULL :

more export/stepdown

@@ +400,5 @@
> +    }
> +    if (ss->dheStepDownParams) {
> +        ssl3_FreeDHParams(ss->dheStepDownParams);
> +        ss->dheStepDownParams = NULL;
> +    }

more export/stepdown.
Attachment #8564199 - Flags: review?(rrelyea) → review+
Comment on attachment 8564199 [details] [diff] [review]
Patch v4

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

Problematic for distributions that don't allow pkcs11 bypass.

::: lib/ssl/ssl3con.c
@@ +12183,5 @@
> +{
> +    DHParams *params;
> +
> +    do {
> +	SECStatus rv = DH_GenParam(primeLenInBytes, &params);

That's a freebl function declared on blapi.h. When we compile with NSS_NO_PKCS11_BYPASS then it's not declared. Adding something like
extern SECStatus DH_GenParam(int primeLen, DHParams ** params);
locally won't do it since this is not exported.
Attachment #8564199 - Flags: feedback-
(In reply to Robert Relyea from comment #23)
> ::: cmd/selfserv/selfserv.c
> @@ +1839,5 @@
> >          }
> >      }
> >  }
> >  
> > +int
> 
> It might be better for this to be a SECStatus and return SECSuccess or
> SECFailure..

I'll skip this, because this function is a direct subfunction of "main", and its return value is directly used as the return value of the main function. Your change would require to introduce introducing and unnecessary layer of translating back to int, which doesn't see to be useful for a internal-only function in a helper utility.
(In reply to Robert Relyea from comment #23)
> @@ +2029,5 @@
> >      }
> >  
> >      for (kea = kt_rsa; kea < kt_kea_size; kea++) {
> >  	if (cert[kea] != NULL) {
> > +	    if (inputDHParamBasename && (kea == kt_dh || kea == kt_rsa)) {
> 
> A comment here that DHE ciphers uses signing certs. DSA is stored under
> kt_dh and rsa is stored under kt_rsa. See comment in ssl3 code as well.


I don't understand this suggestion.

The code added here loads DH parameters from a file, because that's slow, and reusing parameters stored in a file can speed up testing and using selfserv.

I don't understand why this new parameter loading code should get a comment that "DHE ciphers use signing certs", I don't see the connection, can you please elaborate?

Since this was a soft suggestion, I could check in a comment separately.
(In reply to Elio Maldonado from comment #24)
> That's a freebl function declared on blapi.h. When we compile with
> NSS_NO_PKCS11_BYPASS then it's not declared. Adding something like
> extern SECStatus DH_GenParam(int primeLen, DHParams ** params);
> locally won't do it since this is not exported.

Thanks for pointing out this issue.

Can you suggest how to solve it?
Flags: needinfo?(emaldona)
I have some very rough idea as I talked to Bob about it and he suggested creating a somewhat equivalent function at the nss level that would use the higher level stuct's.

At the freebl level we use from blapit.h
struct DHParamsStr {
    PLArenaPool * arena;
    SECItem prime; /* p */
    SECItem base; /* g */
};
typedef struct DHParamsStr DHParams;

at the nss level we have in keyhi.h
struct SECKEYDHParamsStr {
    PLArenaPool * arena;
    SECItem prime; /* p */
    SECItem base; /* g */
};
typedef struct SECKEYDHParamsStr SECKEYDHParams;

The sussgestion is to create a higher level counterpart to static DHParams, something like
SECStatus ssl3_GenDHParam(int primeLenInBytes, SECKEYDHParams *params)

and to look in seckey.c that for several SECKEY_CopyXXX functions worth studying to create one for this purpose.
Flags: needinfo?(emaldona)
Sort of. The code already just casts the lower level structure to the higher level structure.

No, my suggesting was to use PK11_PQG stuff to get a SECKEYDSAParams, and then copy the P and g from that.

bob
So ideally what we really want is to create a pk11wrap version that uses CKM_DH_PKCS_PARAM_GEN. We would have to update softoken to use take CKM_DH_PKCS_PARAM_GEN and call the freebl function.

I think we should do the PK11_PQG thing downstream (in RHEL) since we can't change softoken right now, but do the CKM_DH_PKCS_PARAM_GEN upstream (here).
Do you want this work done immediately or could it be delayed?

It seems that applications/servers typically generate DH parameters once, and then reuse them.

Right now, we have two places where we try to generate the parameters:

- on the fly, but ONLY if the application hasn't preloaded them yet using SSL_SetDHParams
- in selfserv, when the user requested to create parameters and dump them into a file.

Could we start with the following lazy approach?
In SSL_GenerateDHParams, pseudo code:

    #ifdef NO_PKCS11_BYPASS
      params = NULL;
    #else
      params = DH_GenParam();
    #endif

In other words, if we're compiled with NO_PKCS11_BYPASS, we simply fail.


This would mean these initial restrictions:

- if you want to use selfserv for parameter generation, you MUST NOT
  define NO_PKCS11_BYPASS.

- if you want to run NSS server code compiled with NO_PKCS11_BYPASS,
  then you cannot use on the fly parameter generation, but must use SSL_SetDHParams


Do you think this could be a useful first step in getting this work done,
and implement NO_PKCS11_BYPASS DH parameter generation in a follow up bug at a later time?
Flags: needinfo?(rrelyea)
Attached patch 102794-v5.patch (obsolete) — — Splinter Review 
This patch addresses the requests to change the .def file (using version 3.19.1),
it changes WriteDHParamsToFile to return SECStatus and selfserv to use exit(),
and it uses my lazy suggestion to simply fail in SSL_GenerateDHParams if NO_PKCS11_BYPASS is defined.
Attachment #8564199 - Attachment is obsolete: true
(In reply to Kai Engert (:kaie) from comment #31)
> Do you think this could be a useful first step in getting this work done,
> and implement NO_PKCS11_BYPASS DH parameter generation in a follow up bug at
> a later time?

I'm answering the question myself.

I think RH requires that compilation of all parts of NSS using NO_PKCS11_BYPASS works, without combining different modes of compilation.
Flags: needinfo?(rrelyea)
Comment on attachment 8602793 [details] [diff] [review]
102794-v5.patch

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

::: lib/ssl/ssl3con.c
@@ +774,5 @@
>  	    switch (cipher_def->key_exchange_alg) {
>  	    case kea_ecdhe_rsa:
> +	    case kea_dhe_dss_export:
> +	    case kea_dhe_dss:
> +		svrAuth = ss->serverCerts + kt_dh;

I think this part of the patch is mistaken, because it defines kea_ecdhe_rsa to use a certificate with kt_dh. With this change, I see an assertion failure ssl3_config_match_init because no matching key pair is found, if C013 (ECDHE_RSA_WITH_AES_128_CBC_SHA) is the only enabled cipher on the server side, and having both rsa and ec server certificates available.
Attached patch 102794-v6.patch (obsolete) — — Splinter Review 
This patch fixes the issue I mentioned in the previous comment and keeps key type rsa for that scecnario. With this patch, NOT defining NSS_NO_PKCS11_BYPASS, the test suites passes.
Attachment #8602793 - Attachment is obsolete: true
We need what the the default behavior of servers should be, wrt to generation of DH parameters.

With the current patch, we'll introduce long delays.

The automatic generation of DH parameters affects:

- any existing user of selfserv that doesn't use the new parameter -F 

- any existing NSS server software,
  which obviously doesn't reuse prepared DH parameters using SSL_SetDHParams yet,
  and which uses an RSA key pair,
  will be affected by the new parameter generation and long delay,
  whenever a SSL/TLS server socket is configured.

The question is whether this automatic generation is desired and the delay is acceptable,
or,
whether it's considered a regression if we automatically introduce such delays.

All existing NSS server software, which upgrades to the enhanced NSS, without being changed to call the new SSL_SetDHParams API, would be affected.


We must decide which default we prefer:

(a) automatic use of DHE server side ciphers for all existing server software,
    which introduces the new delay for each SSL/TLS server socket.

(b) Don't enable DHE server side ciphers by default.
    Require a new API call to enable them.
    With such a requirement, software developers can become aware of the need
    for DH parameters creation, and can decide if they wish to accept the delay,
    or if they prefer to implement one time generation and loading and reuse of
    parameters from files.
(In reply to Kai Engert (:kaie) from comment #36)
> (b) Don't enable DHE server side ciphers by default.
>     Require a new API call to enable them.

I think that this is a reasonable position.  Generating DH params is, as you say, non-trivial.
Comment on attachment 8603251 [details] [diff] [review]
102794-v6.patch

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

I skimmed through.  Lots of trailing whitespace here.

::: lib/ssl/ssl3con.c
@@ +8768,5 @@
> +          dhParams = ss->dheParams->dhParams;
> +          break;
> +      case kea_dhe_dss_export:
> +      case kea_dhe_rsa_export:
> +          dhParams = ss->dheStepDownParams->dhParams;

I'd rather this were not supported at all.  That would save a lot of code.

@@ +9031,5 @@
>  	         2 + signed_hash.len;
>  
> +	if (ss->ssl3.pwSpec->version >= SSL_LIBRARY_VERSION_TLS_1_2) {
> +	    length += 2;
> +	}

This can only be correcting a bug.  If that is the case, shouldn't we do something about that?

@@ +9607,5 @@
> +                ss->sec.keaKeyBits = 
> +                    SECKEY_PublicKeyStrengthInBits(serverKeyPair->pubKey);
> +            }
> +        }
> +    } else 

This is going to rot if bug 1138554 hits first.

@@ +12141,5 @@
> +{
> +    PRInt32 newCount =  PR_AtomicDecrement(&dhParams->refCount);
> +    if (!newCount) {
> +        if (dhParams->dhParams) 
> +            PORT_FreeArena(dhParams->dhParams->arena, PR_FALSE);

braces, or maybe && rather than this, I think.

@@ +12233,5 @@
> +    if (!ss->dheParams) {
> +        return SECFailure;      /* error is set */
> +    }
> +    if (paramsStepDown) {
> +	ss->dheStepDownParams = ssl3_NewDHParams((SECKEYDHParams*)paramsStepDown);

Do we really need step down parameters?  As I understand it, these are for export strength suites, which we have decided to rip out.
(In reply to Martin Thomson [:mt] from comment #38)
> >  	         2 + signed_hash.len;
> >  
> > +	if (ss->ssl3.pwSpec->version >= SSL_LIBRARY_VERSION_TLS_1_2) {
> > +	    length += 2;
> > +	}
> 
> This can only be correcting a bug.  If that is the case, shouldn't we do
> something about that?


Martin, please see bug 1132941
(In reply to Martin Thomson [:mt] from comment #38)
> Lots of trailing whitespace here.

I don't understand why people keep worry about a few single trailing whitespaces here and there.

And now I hate myself for having tried to fix it, I used my editor's function to strip all trailing whitespace on file save, and now I have trippled my patch file, which defeats the intention of producing minimal patches :(
Attached patch 102794-v11.patch (obsolete) — — Splinter Review 
Martin, thanks for your review, and I'd like to apologize for my rant, which was mostly inspired by hating myself for my mistake.

This patch addresses your comments:
- it should remove trailing whitespace in all modified lines
- I changed the nested condition to use "&&"
- I've checked in the separate TLS 1.2 length issue, which agl had
  already reviewed in the other bug, and I've removed it from this patch.

Regarding your suggestion to avoid stepdown, please see the 4th paragraph in Bob's comment 23 in this bug, where he argues that it's reasonable to keep it for now, until we can get to rip all of the stepdown completely. Even if this code may be short lived, it's not that much of code, it's rather trivial, and it allows me to actually successfully run the test suite while I work on this code.
Bob, I understand that we need a first version of this patch for consumption in RHEL, where we cannot yet upgrade softokn/freebl.

I would like to give you a summary.

In patch v11, I've made the following changes compared to v4, which you had previously reviewed:
- to solve the no-bypass issue, I've implemented your suggestion to use PK11_PQG.
  I understand you prefer not to have it in upstream,
  but since you want it in RH, I'd suggest that you please review it anyway,
  because it might be best to just review it all in one place.
- it fixes the .def file (using version 3.19.1),
- it changes WriteDHParamsToFile to return SECStatus and selfserv main to use exit()

In addition, other changes I haven't mentioned yet:
- I changed selfserv to immediately exit, if -G parameter was used
  (create parameters and save to file)
- I enhanced the test suite to create parameters once, and reuse them
  for all test executions of selfserv.
- I fixed the apparent mistake I mentioned in comment 34, and continue to use
  kt_rsa for kea_ecdhe_rsa.

Please see my comment 26 in this bug, because it isn't clear what you were referring to, if you'd still like me to add a comment.

I'm not requesting review yet. I want to figure out how to disable the DHE ciphers by default, and require an API call to enable them.

One thing that surprises me, it seem that using your approach, using the PK11_PQG API to create two primes: That approach is much, much faster than the old approach. While both variations produce two primes of size 1024 bit, the old code required around 1-2 minutes on average on my machine, the PQG code is always done in less than 5 seconds. Does this indicate any problem with the new approach, or does it mean the old approach had a bug?
Flags: needinfo?(rrelyea)
How about this approach:

- in ssl3_config_match_init, keep kea_dhe_dss and kea_dhe_rsa disabled by default
  (just as it was in the past)

- if SSL_SetDHParams has been called for the given socket, with specific parameters,
  then allow the use of kea_dhe_dss and kea_dhe_rsa

- in addition, allow the application to call SSL_SetDHParams(socket, 0, 0).
  This could set a flag in the socket, which marks it as allowed for automatic generation
  of DH parameters.

If selfserv is executed without the -F parameter, we'd call SSL_SetDHParams(socket, 0, 0).
Another thing that's worthy to discuss is the API of SSL_SetDHParams().

(1)

Right now it's:
  SECStatus SSL_SetDHParams(PRFileDesc *fd, 
                            DHParams *paramsRegular, DHParams *paramsStepDown)

If we intend to remove support for stepdown in the short term, then this might be a bad choice.

Do you have any idea how we could define a stable API that allows stepdown parameters, too?


(2)

Alternatively, should we use the following API:
  SECStatus SSL_SetDHParams(PRFileDesc *fd, DHParams *paramsRegular)
and 
declare that the smaller DH parameters for stepdown are always created dynamically, as long as we support them?


(3)

Or do we really need an API that allows the application pass an array of parameters,
which might be future proof, if we must ever support multiple variations?

  SECStatus SSL_SetDHParams(PRFileDesc *fd, int nParams, DHParams **params)


I'd appreciate your thoughts. Thanks.
Comment on attachment 8603394 [details] [diff] [review]
102794-v11.patch

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

::: lib/ssl/ssl3con.c
@@ +12175,5 @@
>      return rv;
>  }
>  
> +DHParams *
> +SSL_GenerateDHParams(int primeLenInBytes)

Kai, we should not generate new Diffie-Hellman domain parameters
on the fly. Please find good DH domain parameters of each common
size (1024 bits, 2048 bits, etc.) and use them hardcoded. Note
that this goes one step further than what you suggested at the
end of your comment 21.

You can start with the DH domain parameters in
https://tools.ietf.org/html/rfc5114 and
https://tools.ietf.org/html/draft-ietf-tls-negotiated-ff-dhe-08

Bob, Eric, and Martin can all suggest good DH domain parameters
to use.
If the DH parameters can remain constant and can be known to everyone, that will simplify the code a lot...
Kai: the DH domain parameters are public. The DH domain parameters are
the counterparts of elliptic curves. They can remain constant until
someone breaks them.

In your comment 44 you asked about stepdown support. I assume that means
export cipher suites support. Although we probably should not intentionally
break existing export cipher suite code, we should not write new code to
support new export cipher suites.
Yes, Wan-Teh is right.  We should limit the groups to those that are defined in either the IKE groups, or the ff-dhe draft.  I would probably not use both, but that's largely preference.  Since the ff-dhe draft is likely to be what we end up doing anyway, I'd use those, even if they might not be as stable.

As for the sizes to allow for, we could do all of them, but 8192 is quite large.
(In reply to Kai Engert (:kaie) from comment #44)
> Another thing that's worthy to discuss is the API of SSL_SetDHParams().
> 
> (1)
> 
> Right now it's:
>   SECStatus SSL_SetDHParams(PRFileDesc *fd, 
>                             DHParams *paramsRegular, DHParams
> *paramsStepDown)
> 
> If we intend to remove support for stepdown in the short term, then this
> might be a bad choice.
> 
> Do you have any idea how we could define a stable API that allows stepdown
> parameters, too?
> 
> 
> (2)
> 
> Alternatively, should we use the following API:
>   SECStatus SSL_SetDHParams(PRFileDesc *fd, DHParams *paramsRegular)
> and 
> declare that the smaller DH parameters for stepdown are always created
> dynamically, as long as we support them?
> 
> 
> (3)
> 
> Or do we really need an API that allows the application pass an array of
> parameters,
> which might be future proof, if we must ever support multiple variations?
> 
>   SECStatus SSL_SetDHParams(PRFileDesc *fd, int nParams, DHParams **params)
> 
> 
> I'd appreciate your thoughts. Thanks.

I don't see use for export grade DHE, it will be completely useless code.

I'd like to see support for automatic DH parameter selection based on size of key in certificate, so that if I'm using 2048 bit RSA certificate, I will get 2048 bit DHE parameters, without the need for user to provide file with parameters or tell the library which should be used.

That being said, the library should also allow the ability to override this default selection - Java 6 doesn't support DH parameters bigger than 1024 bit, Java 7 and later - 2048 bit.

With that in mind, it may be necessary to use option (3) - so that it is possible to select separate DH params for DSS and RSA certificate. Not to mention SNI virtual hosts, though I'm not sure how it binds together.

See https://bugzilla.redhat.com/show_bug.cgi?id=1057687 and https://bugzilla.redhat.com/show_bug.cgi?id=1073078 for similar bugs resolved in mod_ssl.
I disagree with Hubert regarding the automatic size selection.  Instead, I'd suggest one of two options:

 - we implement the ff-dhe draft
 - we set the group size in configuration, and limit the choice to the ff-dhe groups
 - both (with the configured size becoming a minimum if ff-dhe is supported, and the value used for legacy clients)

That makes the configuration option:
SECStatus SSL_SetDHGroups(PRFileDesc *fd, FFDHEGroup *groups, size_t groups);

Or, what I think works better:
SECStatus SSL_SetDHGroup(PRFileDesc *fd, FFDHEGroup group, PRBool enabled);
SECStatus SSL_GetDHGroup(PRFileDesc *fd, FFDHEGroup group, PRBool enabled);

Then the server can choose the smallest that is enabled and that the peer supports.  If the peer supports named groups, then it's a matching process.  Otherwise, pretend that they support anything.
Comment 49 suggests that we should support DH parameters with a prime size of 1024 bit.

In the ff-dhe document that Martin and Wan-Teh mentioned:
  https://tools.ietf.org/html/draft-ietf-tls-negotiated-ff-dhe-08#appendix-A
the smallest prime is listed in section ffdhe2048, where p consists of 2048 bits.

If we want to support smaller DH parameters, I guess we should use the numbers from the other document that Wan-Teh mentioned:
  https://tools.ietf.org/html/rfc5114
My (strong) recommendation would be to not implement anything smaller than 2048.  We know with reasonable confidence that breaking a 512-bit field costs very little ($70 is the number I've seen).  That suggests that the next common step at 768 is within reach of well-resourced attackers.  1024 isn't that much stronger than that.

We no longer consider 1024 RSA to be adequate.  I don't see any reason to implement something of that strength for DH.
Also of interest: http://tools.ietf.org/html/rfc3766

Some might find this to be supportive of the notion that 2014 is good.  

   Silverman's conclusion, based on the history of factoring efforts and
   Moore's Law, is that 1024-bit RSA moduli will not be factored until
   about 2037.

But the preceding text claims that 512 is good.  We know that to be false.  Considerable advances have been made in this space.

RFC 3766 produces a formula that identifies 2048 as being good until 2016 only (see http://www.keylength.com/en/7/).  That seems like a much more plausible claim.
(In reply to Martin Thomson [:mt] from comment #50)
> I disagree with Hubert regarding the automatic size selection.

Could you explain why?

This is behaviour implemented in mod_ssl and vsftpd (so applications want such a thing) and is recommended by NIST SP 800-56.

>  Instead, I'd
> suggest one of two options:
> 
>  - we implement the ff-dhe draft
>  - we set the group size in configuration, and limit the choice to the
> ff-dhe groups
>  - both (with the configured size becoming a minimum if ff-dhe is supported,
> and the value used for legacy clients)
> 
> That makes the configuration option:
> SECStatus SSL_SetDHGroups(PRFileDesc *fd, FFDHEGroup *groups, size_t groups);
> 
> Or, what I think works better:
> SECStatus SSL_SetDHGroup(PRFileDesc *fd, FFDHEGroup group, PRBool enabled);
> SECStatus SSL_GetDHGroup(PRFileDesc *fd, FFDHEGroup group, PRBool enabled);
> 
> Then the server can choose the smallest that is enabled and that the peer
> supports.  If the peer supports named groups, then it's a matching process. 
> Otherwise, pretend that they support anything.

Implementation of ff-dhe draft is a separate problem.

For now it's not standardised and we will need to support clients which don't advertise this extension anyway. For that we need some default (fallback if you will) DHE parameters, I say that this default should be no smaller than the RSA or DSA key size. It also should be possible to set it to some arbitrary parameters, e.g. when the server needs to interoperate with Java clients.
(In reply to Hubert Kario from comment #54)
> (In reply to Martin Thomson [:mt] from comment #50)
> > I disagree with Hubert regarding the automatic size selection.
> 
> Could you explain why?

Because I believed that configuration would be sufficient for this.  

But it isn't because we don't run the SSLSNISocketConfig hook until most of the handshake is actually sent.

My thought was that we would allow the hook to pick the right certificate AND to set the DH size. But that only works if we move the SSLSNISocketConfig hook earlier in ssl3_HandleClientHello.  It also makes your request to make the size conditional impossible.

I think that I would suggest that Kai not worry about this specific problem and address that in a follow-on.  There are other implications to a reordering of ssl3_HandleClientHello that would need to be considered and this is already significant work.


> Implementation of ff-dhe draft is a separate problem.

I understand how you might think that.  But as we discussed, this can simplify the group selection process and the follow-on to support the extension is then relatively simple.  It's also the right thing to do for clients, for all the reasons articulated in the draft.

> For now it's not standardised and we will need to support clients which
> don't advertise this extension anyway. For that we need some default
> (fallback if you will) DHE parameters, I say that this default should be no
> smaller than the RSA or DSA key size. It also should be possible to set it
> to some arbitrary parameters, e.g. when the server needs to interoperate
> with Java clients.

The problem with arbitrary groups is that it is hard to verify that the don't have a small subgroup.  So clients have to take it on faith that the server is acting faithfully.  And we know from the triple-HS attack that this faith can be abused.
(In reply to Martin Thomson [:mt] from comment #55)
> (In reply to Hubert Kario from comment #54)
> > (In reply to Martin Thomson [:mt] from comment #50)
> > > I disagree with Hubert regarding the automatic size selection.
> > 
> > Could you explain why?
> 
> Because I believed that configuration would be sufficient for this.  
> 
> But it isn't because we don't run the SSLSNISocketConfig hook until most of
> the handshake is actually sent.
> 
> My thought was that we would allow the hook to pick the right certificate
> AND to set the DH size. But that only works if we move the
> SSLSNISocketConfig hook earlier in ssl3_HandleClientHello.  It also makes
> your request to make the size conditional impossible.

sorry, but I don't follow...
you select the parameters at the same time as when you load the certificate
from file

it's not like the certificates are in quantum state and you need to observe them
for every connection to know how big DH parameters you'll need this time...

> > Implementation of ff-dhe draft is a separate problem.
> 
> I understand how you might think that.  But as we discussed, this can
> simplify the group selection process and the follow-on to support the
> extension is then relatively simple.  It's also the right thing to do for
> clients, for all the reasons articulated in the draft.

you still need to do something sensible if the client doesn't advertise
support for ff-dhe. That's the part I'm talking about.

I have nothing against use of DH parameters from the ff-dhe draft or supporting
just ff-dhe draft parameters by default.

But there are real world cases where the ff-dhe draft parameters are not
good. Do we really should tell the users then to forget about PFS?
(there is a difference between paying $30 to crack a 512 bit RSA key and
paying $30 to crack every connection that uses 512 bit DH...)

> > For now it's not standardised and we will need to support clients which
> > don't advertise this extension anyway. For that we need some default
> > (fallback if you will) DHE parameters, I say that this default should be no
> > smaller than the RSA or DSA key size. It also should be possible to set it
> > to some arbitrary parameters, e.g. when the server needs to interoperate
> > with Java clients.
> 
> The problem with arbitrary groups is that it is hard to verify that the
> don't have a small subgroup.  So clients have to take it on faith that the
> server is acting faithfully.  And we know from the triple-HS attack that
> this faith can be abused.

Yes, it's a problem for the client. But the fact that NSS gains the ability to
send arbitrary DHE parameters doesn't change anything. It won't be the only
DHE implementation out there.
(In reply to Hubert Kario from comment #56)
> sorry, but I don't follow...
> you select the parameters at the same time as when you load the certificate
> from file

Yep, that was my idea.  I would not have NSS do the calculation though, the thing using NSS would load the certificate and set the configuration.

> it's not like the certificates are in quantum state and you need to observe
> them
> for every connection to know how big DH parameters you'll need this time...

Well, sometimes you send a different certificate based on SNI.  That was what I was worried about.  The case where there are multiple certificates and those certificates have different key sizes.

> you still need to do something sensible if the client doesn't advertise
> support for ff-dhe. That's the part I'm talking about.

Yes, I know.  But I'm suggesting that we use the groups in ff-dhe and those groups only.

> I have nothing against use of DH parameters from the ff-dhe draft or
> supporting
> just ff-dhe draft parameters by default.
> 
> But there are real world cases where the ff-dhe draft parameters are not
> good. Do we really should tell the users then to forget about PFS?
> (there is a difference between paying $30 to crack a 512 bit RSA key and
> paying $30 to crack every connection that uses 512 bit DH...)

What reasons do you have for saying that the parameters are not good?  Because they are too big?  Isn't that what ECDHE is for?

> Yes, it's a problem for the client. But the fact that NSS gains the ability
> to
> send arbitrary DHE parameters doesn't change anything. It won't be the only
> DHE implementation out there.

Yes, we have to deal with that situation on the client already, but I don't see how making the problem worse by using our own arbitrary, unverifiable groups is helpful.
Flags: needinfo?(rrelyea)
Attached file dhffe-parameters.c (obsolete) — 
This is a C file that encodes the p parameters from 
  https://tools.ietf.org/html/draft-ietf-tls-negotiated-ff-dhe-08#appendix-A
as C arrays.

I believe we must encode P and G, and G is always 2 in that document.

In addition, in case we might have to support a scenario, where a server explicitly wishes to configure 1024-bit DHE parameter, I have included the 1024-bit parameters from https://tools.ietf.org/html/rfc5114
Attachment #8603251 - Attachment is obsolete: true
Attachment #8603394 - Attachment is obsolete: true
Martin, I'm not quite clear on why you don't want selection based on signing certificate. I prefer such a selection for the following reasons:

1) some bad clients are holding back some people from using stronger than 1024 bit. Those same clients will force the signing cert to have those same restrictions.

2) I really prefer being able to react with configuration changes for servers rather than software changes. Unlike clients, where users are usually forced to update their software every 6 months or so (sometime faster), servers try to run the same code for decades and tweak their configurations when necessary. Since this is a server side change..

3) We already do exactly this for ECDHE, including mapping RSA key strength to DH key strength, something that's even more complicated than we are proposing here.

I totally agree we should not be using ECDHE key sizes under 1024. I agree that 1024 is quite rightly going out of favour. That being said, I'm proposing using 1024 bits if the server signing key is 1024 bits. If the server signing key is 1024 bits, then that becomes the security bottle neck and your session is already whatever the strength of 1024 bit discrete log/factoring is. If the key size is 1025, we'd use 2048 PG parameters.

I'm totally OK with allowing loading custom PG parameters to override our defaults, and in that case just using the custom parameters always.

bob
> What reasons do you have for saying that the parameters are not good?  Because they are too big?
>  Isn't that what ECDHE is for?

There are 2 reasons to dial back on parameter size: 1) compatibility, 2) performance. 

re 1: We want to allow support for clients that are stuck in the 1k only world without allowing most users to move forward.
re 2: Each time you double the size of one of these keys, you multiply the cost to perform the basic operation 4x. We don't really want to use 16K keys, for instance. 1k, 2k, and 4k keys with an easy way to performance select seems like a reasonable compromise (and finer grain can be had by loading paramenters automatically.

> Well, sometimes you send a different certificate based on SNI.  That was what I was worried about.  
> The case where there are multiple certificates and those certificates have different key sizes.

Not a problem. You'll get the Parameters for the negotiated certificate just like you do with ECDHE today.

Actually looking at the EC code, we also factor in the symetric key strength along with the signing key strength and set the DHE curve to the weaker of the two (smallest curve that doesn't weaken the connection).

bob
> > >      }
> > >  
> > >      for (kea = kt_rsa; kea < kt_kea_size; kea++) {
> > >  	if (cert[kea] != NULL) {
> > > +	    if (inputDHParamBasename && (kea == kt_dh || kea == kt_rsa)) {
> > 
> > A comment here that DHE ciphers uses signing certs. DSA is stored under
> > kt_dh and rsa is stored under kt_rsa. See comment in ssl3 code as well.
> 
> 
> I don't understand this suggestion.
> 
> The code added here loads DH parameters from a file, because that's 
> slow, and reusing parameters stored in a file can speed up testing 
> and using selfserv.


My comment isn't about the loading, it's about the check. It's not clear initially what kt_dh and particularly kt_rsa has to do with the dhe key exchange. The answer is there is no kt_dhe, and kt_dh and kt_rsa are used to store the signing cert for dhe. That is what the comment should clear up...
(In reply to Robert Relyea from comment #59)
> Martin, I'm not quite clear on why you don't want selection based on signing
> certificate. I prefer such a selection for the following reasons:
>
> 1) some bad clients are holding back some people from using stronger than
> 1024 bit. Those same clients will force the signing cert to have those same
> restrictions.

I don't want to support 1024 DHE in 2016.  It's not strong enough.

If the concern is performance, then ECDHE is available.  And much faster.

If the concern is compatibility, I'm wondering why we don't have server support for DHE already.

But I think that I (finally) understand what you intend here.  You want a means to select a stronger key exchange than the minimum you will tolerate.  You want to use the certificate public key as this signal.

I think that's strictly worse than just having explicit configuration.  On that point, I'd rather do what we do for cipher suites and have a fixed preference order baked into NSS.  At a greater cost and complexity, an explicit means for configuring a preference order would be OK.  I'd expect to see the same for cipher suites though.

> 2) I really prefer being able to react with configuration changes for
> servers rather than software changes. Unlike clients, where users are
> usually forced to update their software every 6 months or so (sometime
> faster), servers try to run the same code for decades and tweak their
> configurations when necessary. Since this is a server side change..

That's what I'm proposing.  Configuration alone would determine what group is selected.

There are actually good reasons to have different strengths for authentication and key exchange.  Authentication key compromise is only of value during the lifetime of the credentials it is bound to.  Key exchange compromise allows for plaintext recovery at any time if ciphertext were captured.

> 3) We already do exactly this for ECDHE, including mapping RSA key strength
> to DH key strength, something that's even more complicated than we are
> proposing here.

See above.

> I'm totally OK with allowing loading custom PG parameters to override our
> defaults, and in that case just using the custom parameters always.

I'm not.  Clients then have to place considerable faith in the server's ability to choose safe groups: because they can't reasonably check them.  That's a problem.

> > Well, sometimes you send a different certificate based on SNI.  That was what I was worried about.  
> > The case where there are multiple certificates and those certificates have different key sizes.
> 
> Not a problem. You'll get the Parameters for the negotiated certificate just
> like you do with ECDHE today.

OK, I see that now: https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3ecc.c#438

I was initially concerned that we'd committed to something during the handling of the ClientHello.

> Actually looking at the EC code, we also factor in the symetric key strength
> along with the signing key strength and set the DHE curve to the weaker of
> the two (smallest curve that doesn't weaken the connection).

That might be reasonable.  I still prefer something explicit.
> I don't want to support 1024 DHE in 2016.  It's not strong enough.

Unfortunately that's a base requirement for us (the guys who actually have servers that will use this code).

> I'm not.  Clients then have to place considerable faith in the server's ability to choose safe groups:
>  because they can't reasonably check them.  That's a problem.

If I can't have the 1024 bit in the static then I absolutely need to load PG parameters. If I have autoselect based on the server key, then maybe we can dial back on the need to load PG parameters externally. Supporting a 1024 minimum is an absolute requirement, however (as well as having a way to drop away to something stronger).

Just to make sure: You are objecting to loading a user supplied PG parameter file (not generating one on the fly) at server start time (which, if done, would provide the semantic of locking a single PG parameter list stronger than the signing key strength that you are asking for). I just want to make sure that we aren't actually agreeing here.

> There are actually good reasons to have different strengths for authentication and key exchange.
>   Authentication key compromise is only of value during the lifetime of the credentials it is bound to.
>  Key exchange compromise allows for plaintext recovery at any time if ciphertext were captured.

And key exchange only affects the session, authentication affects the whole server.

bob
Re comment 42

After some discussion, I think wtc's suggestion of static parameters is fine for RHEL 7. and probably fine for the upstream patch. We should probably export DHParam generation at some point, but this bug would stop being the driver of it.

I think the rest of the questions in comment 42 were already except the PK11_PQG question.
There are two reasons the PQG generates primes are faster: 1) PQG starts by generating q where length of q is ECC curve length (160 bits when generating a 1024 bit p) and then finds p such that q divides p-1. DH produces primes p such that (p-1)/2 is itself a prime. That prime is longer than q (512 bits in the 1024 bit p case), thus takes longer to generate. 2) the PQG code uses shawe-Taylor deterministic prime generation rather than probablistic prime checks. I think Shawe-Taylor is inherently faster.
(In reply to Robert Relyea from comment #63)
> > I don't want to support 1024 DHE in 2016.  It's not strong enough.
> 
> Unfortunately that's a base requirement for us (the guys who actually have
> servers that will use this code).

I wanted to push (quite hard) on that.  Why would someone (in 2015) want to use crypto at this strength, against all the good advice out there?

What pressures are causing you to say that this is an absolute requirement?

We've fairly good evidence that discrete log over modular groups approaching 1024 bits in size is increasingly feasible.  Unless you are proposing that a new private value be generated for every connection.  That's still bad in the sense that it (likely) exposes individual sessions, but I'll agree that it doesn't create the same sort of exposure for the server.

To be clear, I care about the sessions as well as the server.

> Just to make sure: You are objecting to loading a user supplied PG parameter
> file (not generating one on the fly) at server start time (which, if done,
> would provide the semantic of locking a single PG parameter list stronger
> than the signing key strength that you are asking for). I just want to make
> sure that we aren't actually agreeing here.

User supplied PG parameters would be bad.  I guess that we could test them for accepting them, but that doesn't help the client, who can't afford the time to run that check.

If we have a 1024 group, we should pick a single static one that we know is "good".

> And key exchange only affects the session, authentication affects the whole
> server.

Except that (unless I'm missing something) we're reusing private keys over an extended period.  The alternative - generating a fresh key for each connection - could be more costly than moving to a larger modulus.  Well, for the server, that is.
(In reply to Martin Thomson [:mt] from comment #65)
> (In reply to Robert Relyea from comment #63)
> > > I don't want to support 1024 DHE in 2016.  It's not strong enough.
> > 
> > Unfortunately that's a base requirement for us (the guys who actually have
> > servers that will use this code).
> 
> I wanted to push (quite hard) on that.  Why would someone (in 2015) want to
> use crypto at this strength, against all the good advice out there?
> 
> What pressures are causing you to say that this is an absolute requirement?

I already said: compatibility with Java 6. It can't do anything stronger than
1024 bit DH. Java 7 is not much better, it is limited to 2048 bit DH.

Most server operators won't need to be compatible with clients like that, but for
those that do, that most likely will be B2B communication, so they /will/ have
to make the server compatible.

And I prefer those links to use PFS than not.

> We've fairly good evidence that discrete log over modular groups approaching
> 1024 bits in size is increasingly feasible.

> Unless you are proposing that a
> new private value be generated for every connection. 

Honestly, I don't want to see code that does this in any other way...
DHE without PFS is completely useless for RSA keys.

> That's still bad in
> the sense that it (likely) exposes individual sessions, but I'll agree that
> it doesn't create the same sort of exposure for the server.

Yes, I'd argue that attacking individual weak (1024bit DH) but numerous sessions
are harder and worse use of resources for the attacker than attacking one slightly
stronger key (2048 bit RSA) but essentially getting keys to the kingdom allowing
not only decryption of all past, current and future sessions but also ability
to do trivial MITM.

And more importantly, exfiltration of private key from server (the most common way
to attack those systems in real world) doesn't give the ability to decrypt
previous sessions.
 
> To be clear, I care about the sessions as well as the server.
> 
> > Just to make sure: You are objecting to loading a user supplied PG parameter
> > file (not generating one on the fly) at server start time (which, if done,
> > would provide the semantic of locking a single PG parameter list stronger
> > than the signing key strength that you are asking for). I just want to make
> > sure that we aren't actually agreeing here.
> 
> User supplied PG parameters would be bad.  I guess that we could test them
> for accepting them, but that doesn't help the client, who can't afford the
> time to run that check.

The client doesn't know which server it's /really/ connecting to. Attacker can use
one of the two dozen TLS servers that allow them to use broken parameters.

It's not like it will be easy for the attacker to force the server or the user
to use attacker provided parameters. Any google search will tell you to use
openssl dhparam to get your parameters if you want non standard, not to download
them from this "totally honest website".

> If we have a 1024 group, we should pick a single static one that we know is
> "good".

I'm against that only because I want the server operators to have a harder time
misconfiguring their server (using 1024 bit when they really don't have a reason to)
- making the 1024 bit configuration require "gen this parameters, you
will need few minutes for that" gives time to reflect on their actions, simple
switch won't.
 
> > And key exchange only affects the session, authentication affects the whole
> > server.
> 
> Except that (unless I'm missing something) we're reusing private keys over
> an extended period.  The alternative - generating a fresh key for each
> connection - could be more costly than moving to a larger modulus.  Well,
> for the server, that is.

there are two values to generate: parameters and private values.

Parameters are very costly to generate, but they are public and can be reused.
We should reuse them (take them either from the old IKE RFC or the new ff-dhe draft).

Private values are far less costly to generate (well not really generate, compute
from randomly selected value), but their regeneration for every connection 
is essential to provide PFS.

At 2048 bit RSA with a measly Atom @ 1.8GHz I get 167 connections/s, bump it
to 2048 bit DHE with 2048 bit RSA and I get 24 connections/s with mod_ssl.
So, yes, it is slower, but it's not prohibitive.
> User supplied PG parameters would be bad.  I guess that we could test them for accepting them, but that 
> doesn't help the client, who can't afford the time to run that check.
> 

This is the fundamental weakness of DHE from the beginning. The client has no idea that the server is really using perfect forward secrecy even (yet alone given good strong parameters). This is true no matter what NSS does here because, quite frankly, the chances that Firefox is talking to an NSS server these days are quite small. Mostly you are talking to an openssl server, which already allows admin loaded parameters, so there is no way to abrogate the clients responsibility of verifying the parameters if they aren't already trusted.

In short we aren't going to solve any of the clients issues with what we do here. Everything we are proposing is already what the servers the clients need to talk to already do. (If that weren't the case, we would have made firefox reject everything under 2k rather than 1k Ephemeral keys when we recently upped the NSS limit there [was something like 512 or 768 up to about 6 months ago).

bob
OK, I think that I have a handle on this.  Here's what I propose:

We support a fixed set of groups (i.e., parameters, or values for p and g) that include the ff-dhe groups AND a 1024-bit group (which might be taken from the IKE RFC).  The 1024-bit group is disabled by default.  We can discuss what is needed to enable DHE separately. 

A new private value is generated frequently (it need not be unique per connection, but that would be ideal).  It looks like the last patch reuses the same key, which isn't ideal.
(In reply to Robert Relyea from comment #67)
> This is the fundamental weakness of DHE from the beginning. The client has
> no idea that the server is really using perfect forward secrecy even (yet
> alone given good strong parameters). This is true no matter what NSS does
> here because, quite frankly, the chances that Firefox is talking to an NSS
> server these days are quite small. 

We do it all the time with WebRTC.  Though less so now that Google are further along with BoringSSL.
Thanks marting...

> A new private value is generated frequently (it need not be unique per connection, but that would be ideal).  
> It looks like the last patch reuses the same key, which isn't ideal.

That should be relatively easy, for some definition of frequently. (order of 100 or 1000. At those numbers the regen cost is negligible).

> We support a fixed set of groups (i.e., parameters, or values for p and g) that include the ff-dhe groups 
> AND a 1024-bit group (which might be taken from the IKE RFC).  The 1024-bit group is disabled by default.  
> We can discuss what is needed to enable DHE separately.

Actually that's the part we need to figure out ASAP. We need to roll something into RHEL 7 in the next week or so. If we add need to add APIs, we need to lock them down sooner than later. Unfortunately our normal trick with environment variables do not work on servers, who regularly trash their environment before they start.

Hubert is there some other reason you want loadable parameters?
(In reply to Robert Relyea from comment #70)
> Hubert is there some other reason you want loadable parameters?

no, the only problem I know of is Java, and that will be fixed by any 1024 bit DH params
Then we need to get an API in place.  Here's what I suggest:

Add a new enum to sslt.h that has values for each of the groups we agreed.

Add a new function for setting the group preferences:
  SECStatus SSL_DHEGroupPrefSet(PRFileDesc *fd, NewEnum *values, size_t count);
I think that this should default to the ff-dhe groups and not include 1024.

Add two new configuration options:

#define SSL_ENABLE_SERVER_DHE 29
I don't much care what this is set to.

#define SSL_REUSE_SERVER_DHE_KEY 30
I'd prefer if this defaults to false, unlike the existing ECDHE equivalent.

You can, I suppose, change this last one to have time bounds instead, but that requires more API points to support.
> #define SSL_ENABLE_SERVER_DHE 29
> I don't much care what this is set to.

Do we need this since the server can just turn the cipher suite on or off? 

>  SECStatus SSL_DHEGroupPrefSet(PRFileDesc *fd, NewEnum *values, size_t count);
> I think that this should default to the ff-dhe groups and not include 1024.

Sounds good. We just need to define the groups and include one 1024. I'm totally OK with 1024 being turned off by default.

> #define SSL_REUSE_SERVER_DHE_KEY 30
> I'd prefer if this defaults to false, unlike the existing ECDHE equivalent.
> You can, I suppose, change this last one to have time bounds instead, but that requires more API points to 
> support.

A count bounds would be preferable if we need to make it configurable.
(In reply to Robert Relyea from comment #73)
> > #define SSL_ENABLE_SERVER_DHE 29
> > I don't much care what this is set to.
> 
> Do we need this since the server can just turn the cipher suite on or off? 

No, of course we don't :)

> > #define SSL_REUSE_SERVER_DHE_KEY 30
> > I'd prefer if this defaults to false, unlike the existing ECDHE equivalent.
> > You can, I suppose, change this last one to have time bounds instead, but that requires more API points to 
> > support.
> 
> A count bounds would be preferable if we need to make it configurable.

Either way; though if you are under time pressure, I'd defer that level of configuration.
Oh yeah, I remember now why I had an SSL_ENABLE_SERVER_DHE option.  If we accept the default configuration, this will cause DHE to be enabled by default.  If you don't like that idea, you might want to have an option.
> Either way; though if you are under time pressure, I'd defer that level of configuration.

Agreed.

> h yeah, I remember now why I had an SSL_ENABLE_SERVER_DHE option.  If we accept the default configuration,
> this will cause DHE to be enabled by default.  If you don't like that idea, you might want to have an option.

Yup, agreed.

Kai I think we have enough to tweak the patch now.

bob
Thanks for your discussion and decisions. I'll work on it Friday.
Let me try to summarize what has been decided, for the bare minimum initial patch.

- no dynamic DH parameter generation

- support DH parameters built into NSS, only

- we use the parameters that I attached in comment 58

- new socket option
    #define SSL_ENABLE_SERVER_DHE 
  -> disabled by default

- keep using the private value all the time
  (if you cannot accept that for the initial patch, please suggest the initial default
   frequency for re-generation of the private value, which cannot be configured in
   the initial implementation)

- add new API that allows the application to configure the allowed DHE parameters,
  and as Martin suggest, use an array of IDs of named parameters

- by default, disable the 1024 bit group

- by default, enable the 2048 bit group


Later:
- a mechanism to configure the reuse of the private value, and/or frequency
  of automatic re-generation of private value (SSL_REUSE_SERVER_DHE_KEY)
I believe we have agreed, for the initial implementation, we don't do automatic selection of DH parameters.

We'll use the first group that is included in the default array, or the first group in the array that is set using SSL_DHEGroupPrefSet().

We might implement some sort of automatic selection of DH parameters at a later time, which is probably the idea behind using an array of group IDs in SSL_DHEGroupPrefSet().

Is my understanding correct?
The existing code in libssl doesn't use any locks to protect the modification of global defaults (options, enabled ciphers, version range).

The suggest API SSL_DHEGroupPrefSet would require updating two global variables.

In order to avoid having to introduce a lock, and to ensure updating our global preference will be atomic, I'd suggest that we'll use a wrapper structure around 
  {array of ids, number of ids in the array}
and use a pointer to that type in the API.
(In reply to Kai Engert (:kaie) from comment #80)
> The existing code in libssl doesn't use any locks to protect the
> modification of global defaults (options, enabled ciphers, version range).

Yeah, and I wouldn't advise touching them once you start actually using the software.  libssl only occasionally pretends to be thread safe.  It's probably not crashing purely by chance.

> The suggest API SSL_DHEGroupPrefSet would require updating two global
> variables.
> 
> In order to avoid having to introduce a lock, and to ensure updating our
> global preference will be atomic, I'd suggest that we'll use a wrapper
> structure around 
>   {array of ids, number of ids in the array}
> and use a pointer to that type in the API.

Why not have a socket local configuration (ss->ssl3.dheGroups) and rely on thread confinement.  Adding locks (or worse, relying on some sort of atomicity) is a little dangerous.  The memory overhead here should be minimal.
(In reply to Kai Engert (:kaie) from comment #78)
> - keep using the private value all the time
>   (if you cannot accept that for the initial patch, please suggest the
> initial default
>    frequency for re-generation of the private value, which cannot be
> configured in
>    the initial implementation)

I've a strong preference for a new key for every connection.  That means that the code is safe by default.  We can add configuration to make it less safe (and perhaps more performant as a result).  But later.
+1 to that
Martin,

Why do we need the SSL_ENABLE_SERVER_DHE option? Any new option adds
burden to the NSS users who care to review their SSL socket options,
and Bob's comment 73 showed he didn't think he needed it. Only people
who know NSS didn't support DHE on the server side and NSS had (or
still has?) the absolutely no surprise policy will understand the
reason for the SSL_ENABLE_SERVER_DHE option.

Under the absolutely no surprise policy, we would not be able to
turn off SSL 3.0 by default. So I'd like to request that you reconsider
your suggestion for adding SSL_ENABLE_SERVER_DHE.

As for SSL_REUSE_SERVER_DHE_KEY, I would suggest we repurpose the
existing SSL_REUSE_SERVER_ECDHE_KEY option to also cover DHE, or
just document that NSS servers never reuse DHE keys for multiple
connections. I think we should try to keep our configuration
settings at a minimum.

Wan-Teh
Wan-Teh,

I don't think that we do need to have DHE disabled by default.  I offered the option to to be complete, but I agree with you that having fewer options is better.

Unfortunately, SSL_REUSE_SERVER_ECDHE_KEY has a default that I don't like much.
(In reply to Martin Thomson [:mt] from comment #85)
>
> Unfortunately, SSL_REUSE_SERVER_ECDHE_KEY has a default that I don't like
> much.

Then I propose we start with always generating a new DHE key for each
connection, and only add an SSL_REUSE_SERVER_DHE_KEY option (default
to false) when someone requests it.

We should also update the comment for SSL_REUSE_SERVER_ECDHE_KEY to
confirm that it doesn't cover DHE.
Yes.  On both counts.  (The name might be enough of a clue on the second part that it's not a pressing concern.)
(In reply to Wan-Teh Chang from comment #84)
> Why do we need the SSL_ENABLE_SERVER_DHE option?

Consider existing server software, which iterates over SSL_ImplementedCiphers[] and enables all of them (maybe at most excluding blacklisted weak ciphers).

If a server gets upgraded to a newer version of NSS, that implements DHE server side ciphers, but keeping the server software unmodified, what will happen?

Could upgrading NSS automatically enable the selection of DHE on such servers?

If yes, then an upgrade of NSS might break connections to Java 6 clients.
The reason is that Java 6 clients cannot handle our default of 2048 bit DH parameters.

I'm under the impression the intended changes to ssl3_config_match_init would automatically enable DHE on such servers.

Requiring the use of a new SSL_ENABLE_SERVER_DHE socket configuration option would avoid the risk of potentially breaking such client/server pairs by upgrading NSS.
(In reply to Martin Thomson [:mt] from comment #81)
> Why not have a socket local configuration (ss->ssl3.dheGroups) and rely on
> thread confinement.  Adding locks (or worse, relying on some sort of
> atomicity) is a little dangerous.

You're right, you already suggested to use an API that's specific to a socket, and yes, I agree we should avoid new locks.

Nevertheless, if we pass an array with unknown size to a configuration API, I'm afraid we'll need a new wrapper struct and also use the usual mechanisms for copying and cleaning up...
(In reply to Kai Engert (:kaie) from comment #89)
> Nevertheless, if we pass an array with unknown size to a configuration API,
> I'm afraid we'll need a new wrapper struct and also use the usual mechanisms
> for copying and cleaning up...

Alright, we're only talking integers, not nested structs, so we can simply always copy the input array...
(In reply to Kai Engert (:kaie) from comment #88)
> If yes, then an upgrade of NSS might break connections to Java 6 clients.
> The reason is that Java 6 clients cannot handle our default of 2048 bit DH
> parameters.

On the other hand, I agree with Wan-Teh, requiring applications to set another socket option for enabling good ciphers like DHE is unfortunate.

I wonder if we could go another route.

For those environments, that use old servers, and that require compatibility with Java 6, and that somehow break after we enable DHE by default:

Maybe it's OK to require them to recompile NSS in a non-standard default configure. We could use an optional #define that enables the 1024 bit group by default, and we could require those users to recompile NSS with that weaker default.

Bob, Hubert, do you think that might be acceptable for the mentioned edge case?
(I believe we don't know yet if anyone deploys servers with the anticipated behavior.)
I work on an NSS server likely to have Java 6 clients impacted by this. Our server already has a configuration setting to adjust enabled cipher suites; that option can be used to disable DHE cipher suites if needed. I'd consider a release note to use the existing option sufficient to interoperate with an EOL Java version such as Java 6. If there is actively supported mail client software with the 1024 bit DHE limitation (I do not know if that is the case), then I'd prefer to have an NSS config setting to enable the 1024 bit group, but I'd still prefer 1024 to be disabled by default -- I'm not a fan of insecure-by-default software, even via a build option.

This is my personal opinion only, developers of other servers might have a different opinion.
Attached patch v14.patch (obsolete) — — Splinter Review 
Latest work-in-progress patch, builds and runs.
Comment on attachment 8606637 [details] [diff] [review]
v14.patch

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

This looks pretty good.  I'd encourage you to check that the gtests (in external_tests/ssl_gtest/) actually still run and pass.  I suspect that they won't, but it should be easy to fix them.

::: lib/ssl/dhe-param.c
@@ +451,5 @@
> +    0xD6, 0x8C, 0x8B, 0xB7, 0xC5, 0xC6, 0x42, 0x4C,
> +    0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
> +};
> +
> +static const unsigned char ff_dhe_8192_g[] = { 2 };

Do you need a separate _g value for each of the ff_dhe groups given that they are the same?

::: lib/ssl/ssl3con.c
@@ +12196,5 @@
> +    SSLDHEGroupsType selectedGroup = ssl_dhe_group_none;
> +
> +    if (!ss->ssl3.dheGroups) {
> +	size_t number_of_default_groups =
> +	    sizeof(ssl_default_dhe_groups) / sizeof(SSLDHEGroupsType);

PR_ARRAY_SIZE?

::: lib/ssl/sslsock.c
@@ +3027,5 @@
>          ss->stepDownKeyPair    = NULL;
> +
> +        ss->dheParams = NULL;
> +        ss->dheKeyPair = NULL;
> +        

ws

::: lib/ssl/sslt.h
@@ +205,5 @@
> +    ssl_ff_dhe_4096_group = 4,
> +    ssl_ff_dhe_6144_group = 5,
> +    ssl_ff_dhe_8192_group = 6,
> +    ssl_dhe_group_max
> +} SSLDHEGroupsType;

I think that it should be ...GroupType, singular.
(In reply to Kai Engert (:kaie) from comment #91)
> For those environments, that use old servers, and that require compatibility
> with Java 6, and that somehow break after we enable DHE by default:
> 
> Maybe it's OK to require them to recompile NSS in a non-standard default
> configure. We could use an optional #define that enables the 1024 bit group
> by default, and we could require those users to recompile NSS with that
> weaker default.
> 
> Bob, Hubert, do you think that might be acceptable for the mentioned edge
> case?
> (I believe we don't know yet if anyone deploys servers with the anticipated
> behavior.)

that's not acceptable for environments like RHEL

and yes, we know of such deployments

(In reply to Chris Newman from comment #92)
> I work on an NSS server likely to have Java 6 clients impacted by this. Our
> server already has a configuration setting to adjust enabled cipher suites;
> that option can be used to disable DHE cipher suites if needed. I'd consider
> a release note to use the existing option sufficient to interoperate with an
> EOL Java version such as Java 6. If there is actively supported mail client
> software with the 1024 bit DHE limitation (I do not know if that is the
> case), then I'd prefer to have an NSS config setting to enable the 1024 bit
> group, but I'd still prefer 1024 to be disabled by default -- I'm not a fan
> of insecure-by-default software, even via a build option.
> 
> This is my personal opinion only, developers of other servers might have a
> different opinion.

so you need to recompile the application either way to add the "yes I want the insecure 1024bit DH", if you want DHE too, you can just add two changes in one recompilation. Point is, that we don't want to break applications which recompilation is not possible for the user (system administrator).

Java 6 also includes OpenJDK 6, which is still fully supported.
(In reply to Hubert Kario from comment #95)
> Java 6 also includes OpenJDK 6, which is still fully supported.

If it's supported, does it have SkipTLS fixes?  Can the maintainer actually lift the 1024-bit limit and provide their users with a *secure* DH exchange?

I thought that this was a case that we were dealing with a significant base of unsupported software.  But this changes things.
(In reply to Martin Thomson [:mt] from comment #96)
> (In reply to Hubert Kario from comment #95)
> > Java 6 also includes OpenJDK 6, which is still fully supported.
> 
> If it's supported, does it have SkipTLS fixes?  Can the maintainer actually
> lift the 1024-bit limit and provide their users with a *secure* DH exchange?
> 
> I thought that this was a case that we were dealing with a significant base
> of unsupported software.  But this changes things.

those changes were deemed too significant to backport
Which?  SkipTLS or 1024?
(In reply to Martin Thomson [:mt] from comment #98)
> Which?  SkipTLS or 1024?

1024bit DH
(In reply to Martin Thomson [:mt] from comment #98)
> Which?  SkipTLS or 1024?

SkipTLS was fixed, both
in OpenJDK: https://rhn.redhat.com/errata/RHSA-2015-0085.html
and in Oracle Java 6: http://www.oracle.com/technetwork/topics/security/cpujan2015-1972971.html#AppendixJAVA

though I'm a bit confused why you're bringing up SkipTLS in a discussion about DHE...
If SkipTLS wasn't too big to add, I'm surprised that 1024-bit DH doesn't make the grade.  It should be a tiny change.
bring it to Java upstream, I'm just a messenger
Given comment 95 and comment 92, it seems we must indeed implement a socket
option to enable server side DHE.

Maybe we could patch NSS on RHEL to disable server side DHE by default,
but on the main development line of NSS, could we enable it by default?
From our perspective, I would prefer that it be enabled.  We'll still prefer ECDHE over DHE, so the impact is minimal.
(In reply to Kai Engert (:kaie) from comment #103)
> Given comment 95 and comment 92, it seems we must indeed implement a socket
> option to enable server side DHE.

IIUC, the correct place to implement the additional check for "DHE enabled" is function
  config_match
in ssl3con.c
This patch implements the new socket option,
adds testing for the new ciphers,
addresses the suggestions from comment 94,
and implements a new parameter for selfserv to override the library default.

I've tested various combinations.

With "disabled by default", without selfserv parameter, 
  the DHE tests fail (no cipher overlap), as expected.

With "disabled by default", but starting selfserv with -H 1, 
  the DHE tests work, as expected.

With "enabled by default", without selfserv parameter,
  the DHE tests work, as expected.


For the patch to check in, unless Bob or Hubert object, I'll set
- enabled by default
- start selfserv with -H1, to ensure a NSS with patched change of default will still 
                           pass the test suite
Attached patch v17.patch (obsolete) — — Splinter Review 

        Attachment #8605411 -
        Attachment is obsolete: true

        Attachment #8606637 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Martin Thomson [:mt] from comment #94)
> I'd encourage you to check that the gtests (in
> external_tests/ssl_gtest/) actually still run and pass.  I suspect that they
> won't, but it should be easy to fix them.

I ran into a crash:

#0  0x00007f41948f0aff in getWrappingKey (ss=0x1a0bcc0, masterSecretSlot=0x167edb0, exchKeyType=ssl_kea_dh, masterWrapMech=306, pwArg=0x0) at ssl3con.c:5654
#1  0x00007f41948fba60 in ssl3_CacheWrappedMasterSecret (ss=0x1a0bcc0, sid=0x7ffeda054380, spec=0x1a0d880, effectiveExchKeyType=ssl_kea_dh) at ssl3con.c:10777
#2  0x00007f419490b6d1 in ssl3_SendNewSessionTicket (ss=0x1a0bcc0) at ssl3ext.c:1134
#3  0x00007f41948fbed6 in ssl3_HandleFinished (ss=0x1a0bcc0,

    
    

    
  
That looks like a pretty problem.  Good thing you ran the tests then :)  It looks like the key agreement type is being used to index a structure, maybe incorrectly.

    
    

    
  
It looks like kt_dh (which should be ssl_kea_dh doesn't have an entry in the switch statement in getWrappingKey either.

    
    

    
  
Kai, I'm Ok with the upstream NSS code having DHE on for the server by default. We probably make a different choice for existing RHEL, but it's not that big a diff to maintain.

    
    

    
  
(In reply to Kai Engert (:kaie) from comment #106)
> For the patch to check in, unless Bob or Hubert object, I'll set
> - enabled by default
> - start selfserv with -H1, to ensure a NSS with patched change of default
> will still 
>                            pass the test suite

I'm OK with upstream having different default for that option, but the API and option must be in upstream version.

    
    

    
  
I believe comment 108 + 109 + 110 mean:

The original patch was incomplete, we need to check how many other places in NSS need to be enhanced to correctly support DHE on the server side.

I don't know yet what's necessary for kt_dh in function "getWrappingKey" (ssl3con.c).

    
    

    
  
(In reply to Martin Thomson [:mt] from comment #109)
> That looks like a pretty problem.  Good thing you ran the tests then :)  It
> looks like the key agreement type is being used to index a structure, maybe
> incorrectly.

The entry in the indexed structure exists, it's the array of server certs, however it doesn't contain the expected data.

(gdb) print ss->serverCerts[exchKeyType]
$4 = {serverCert = 0x0, serverCertChain = 0x0, serverKeyPair = 0x0, serverKeyBits = 0}

The code attempts to get serverKeyPair->privKey

Based on the assertion in the code that follows, this is considered an internal error.
We must find out where we're violating the internal assumptions, and don't update the serverCerts array (prior to getWrappingKey() being called).

    
    

    
  
I can reproduce the crash found using gtests by executing selfserv and tstclnt with the -u option that enables session tickets. It isn't necessary to specifically require the use of DHE ciphers, it crashes even if both server/client don't limit the set of ciphers (however, I enabled DHE on the server side using the new -H 1 parameter).

I find it worrying that the classic NSS test suite didn't run into this crash. I wonder if the automated testing for session tickets is insufficient, and I wonder, which other TLS features might have insufficient test coverage in the NSS test suite.

    
    

    
  
Kai,

I think the issue here is that in the case of DHE case it is expecting not kt_dh but rather
the key in the certificate (probably RSA). You probably need a block like:

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c#10653

    if (ss->ssl3.hs.kea_def->kea == kea_ecdhe_rsa) {
	effectiveExchKeyType = kt_rsa;
    } else {
	effectiveExchKeyType = ss->ssl3.hs.kea_def->exchKeyType;
    }

    
    

    
  
A correct check would be more comprehensive than this.  After dealing with this exact issue yesterday, but elsewhere...

If the intent is to capture the type of the public key that is embedded in the certificate, the right call here should have been SECKEY_GetPublicKeyType(SECKEY_GetPublicKey(cert)), using the result of that as an index.  Of course, that produces different indices, which might have to be mapped.

Also, kt_* are deprecated in favour of ssl_kea_*

    
    

    
  
Who is responsible to destroy the dhParams that are given to
SECKEY_CreateDHPrivateKey ?

I haven't researched this yet, so if you know the answer already, please let me know.

If it's the callers responsibility, then the patch can be simplified
to NOT copy the prime/base, and instead pass a pointer to our static data.

    
    

    
  
(In reply to Eric Rescorla (:ekr) from comment #116)
> I think the issue here is that in the case of DHE case it is expecting not
> kt_dh but rather
> the key in the certificate (probably RSA). You probably need a block like:

Thanks Eric, that was very helpful! I found the place in sslext.3 that needs this block.

It fixed the assertion I had reported earlier.

I ran into another issue on shutdown

#2  0x00007f22fd669625 in PR_Assert (s=0x7f22fe016bd0 "secmod_PrivateModuleCount == 0", file=0x7f22fe016bc4 "pk11util.c", ln=88) at ../../../../pr/src/io/prlog.c:559


I'll have to doublecheck the reference counting. Maybe it's even related to my question in comment 118 (currently the code assumes ownership is transferred).

    
    

    
  
(In reply to Martin Thomson [:mt] from comment #117)
> If the intent is to capture the type of the public key that is embedded in
> the certificate, the right call here should have been
> SECKEY_GetPublicKeyType(SECKEY_GetPublicKey(cert)), using the result of that
> as an index.  Of course, that produces different indices, which might have
> to be mapped.

It's not clear to me how this would work.

In the given scenario, socket and handshake states are the input, and the cert is the output, then the private key from that cert is taken.

Your function call seems to use a cert as input, and I don't understand which cert you'd like to use as input.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v19.patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8607251 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Kai Engert (:kaie) from comment #120)
> (In reply to Martin Thomson [:mt] from comment #117)
> > If the intent is to capture the type of the public key that is embedded in
> > the certificate, the right call here should have been
> > SECKEY_GetPublicKeyType(SECKEY_GetPublicKey(cert)), using the result of that
> > as an index.  Of course, that produces different indices, which might have
> > to be mapped.
> 
> It's not clear to me how this would work.
> 
> In the given scenario, socket and handshake states are the input, and the
> cert is the output, then the private key from that cert is taken.
> 
> Your function call seems to use a cert as input, and I don't understand
> which cert you'd like to use as input.

The reason I suggested that was that the code in question seems to encipher the material toward the corresponding server certificate.  The certificate you would use is the server certificate.

    
    

    
  
Sorry for reopening this discussion, but I think we should re-evaluate using hardcoded 1024 bit prime:
https://www.ietf.org/mail-archive/web/tls/current/msg16496.html

From Karthikeyan Bhargavan <karthik.bhargavan@gmail.com>:
> 3) Do not use fixed 1024-bit groups.
>    If ECDHE or 2048-bit groups are both infeasible and 1024-bit groups need
>    to be used for legacy reasons, we recommend that servers generate fresh
>    1024-bit groups regularly to increase the computational cost of an attack.
>    When a particular group size becomes computationally reachable, the clock
>    starts ticking on all groups of that size. After that point, the use of
>    fixed groups begins to bite, because the cost of breaking each group can
>    be amortised across its users.
>    We (and others) believe that 1024-bit groups may have become breakable,
>    but the cost of precomputation for each group is still quite high.
>    Let’s make things harder for the adversaries by changing groups so that
>    their precomputations are wasted.
>    As usual, we need to take care when generating new groups: e.g. use
>    randomly-generated safe primes.

    
    

    
  
Regarding comment 119, ssl_gtest crashes even without my patch
 (Assertion failure: secmod_PrivateModuleCount == 0, at pk11util.c:88)

I've filed bug 1168425.

Regarding comment 18, I saw that existing callers don't copy, so I removed my code to copy the DH parameter.

    
    

    
  
Today I've learned:

For *ECDHE*, by default, the NSS server code reuses the epheremal key.

In bug 1028582, a socket option SSL_REUSE_SERVER_ECDHE_KEY had been added, with the default of "yes reuse". An application must explicitly set the socket option to false to request refreshing for every handshake.

(I've used wireshark, default selfserv, and modified selfserv, to confirm the above.)


I think NSS should be consistent. Either both ECDHE and DHE sockets should reuse by default, or both ECDHE and DHE should always refresh by default.

If we cannot change the default for ECDHE, then I'd suggest that we reuse the epheremal key for DHE by default, too.


Again, for consistency, I think we should have an equivalent socket option to control the reuse of the epheremal key for DHE, too.

However, if you'd like to minimize the amount of socket options, I'd ask:

Could we define that the existing socket option SSL_REUSE_SERVER_ECDHE_KEY controls reuse/refresh for *both* DHE and ECDHE?

    
    

    
  
I didn't realize that you didn't know this.  Sorry, I thought that you had seen the discussion.  See comment 86 and preceding comments.

When ekr landed the change to add SSL_REUSE_SERVER_ECDHE_KEY, we did very much want to have this value set to false by default.  However, the agreement was that the default needed to ensure that we did not cause a performance regression (to the extent that using new ECDH keys is expensive).

As Wan-Teh notes, we can defer the addition of an option.

    
    

    
  
Ok, I had missed that detail in comment 86, thanks.

That means, you're already aware that we'll create an inconsistency of default behaviour between server side DHE and ECDHE and that you think it's acceptable.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v25.patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8608767 -
        Attachment is obsolete: true

    
    

    
  
I've tested the behaviour of patch v25 using wireshark.

Using a single selfserv process, and running tstclnt twice to connect to it, I see different "pubkey" values sent by the server. Apparently, this patch already implements the requested behaviour to generate fresh ephemeral keys for each socket.

I've sneaked in a small change to selfserv, which can be used to disable ephemeral key reuse for ECDHE.

I've changed the version number of the new API to 3.19.2.
We need to settle on this version number, because RHEL will start to use it, and the API version should be fixed.
Question is, does this new API justify a new minor release version bump, and should we rather use version 3.20 for this API?

I've added the clarification comment that Wan-Teh suggested in comment 86.

Added comments to explain the GroupPrefSet API and the ENABLE_SERVER_DHE socket option.

    
    

    
  
Comment on attachment 8610793 [details] [diff] [review]
v25.patch

This may be ready for review.

Looking forward to your comments.

        Attachment #8610793 -
        Flags: superreview?(martin.thomson)

        Attachment #8610793 -
        Flags: review?(rrelyea)

    
    

    
  
Will anyone double check that I copied the group parameters from the documents correctly?

I did use copy paste, and a small script to convert it to the C code format, so it "should" be correct.

    
    

    
  
(In reply to Hubert Kario from comment #123)
> Sorry for reopening this discussion, but I think we should re-evaluate using
> hardcoded 1024 bit prime:
> https://www.ietf.org/mail-archive/web/tls/current/msg16496.html

Given that we'll disable 1024 bit groups by default, I would like to suggest, let's consider the addition of dynamic group configuration after this initial work is done, in a separate follow up patch.

If necessary, we could potentially add another API at a later time, an alternative to the GroupPrefSet API, which doesn't use IDs of predefined groups, but the full parameter data.

    
    

    
  
Comment on attachment 8610793 [details] [diff] [review]
v25.patch

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

There are a few things in here that need attention.  I'm not sure what we should be doing about the use of the non-enum kt_* values.  I think that some of the uses here are hiding errors, in particular the use of kt_dh to signify the use of a DSA certificate.

::: cmd/selfserv/selfserv.c
@@ +2620,5 @@
>  	setupCertStatus(certStatusArena, ocspStaplingMode, cert[kt_rsa], kt_rsa,
>  			&pwdata);
>      }
> +    if (dsaNickName) {
> +	cert[kt_dh] = PK11_FindCertFromNickname(dsaNickName, NULL);

I'm not sure that this should be using kt_* as an index here.  Certificate choices should be based on the auth type, which is based on ssl_auth_*: https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/sslt.h#65

kt_* is marked as deprecated: https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/sslt.h#46

::: lib/ssl/dhe-param.c
@@ +36,5 @@
> +    0x85, 0x5E, 0x6E, 0xEB, 0x22, 0xB3, 0xB2, 0xE5,
> +};
> +
> +static const ssl3DHParams modp_dhe_1024 = {
> +    { 0, (unsigned char *)modp_dhe_1024_p, sizeof(modp_dhe_1024_p) },

Use siBuffer (or siUnsignedInteger?) for the first value in each of these.

It's a shame that the SECItem initializer is incapable of handling const values.

::: lib/ssl/ssl3con.c
@@ +840,5 @@
> +
> +    PORT_Assert(opt != NULL);
> +    PORT_Assert(sec != NULL);
> +    if (sec->isServer != 0 && !opt->dheServerSideAllowed &&
> +        kea_defs[cipher_def->key_exchange_alg].exchKeyType == kt_dh)

Why compare isServer with 0 rather than a bare test of (sec->isServer)?

s/kt_dh/ssl_kea_dh/

@@ +864,5 @@
>  	return 0;
>      }
>      for (i = 0; i < ssl_V3_SUITES_IMPLEMENTED; i++) {
> +	if (config_match(&ss->cipherSuites[i], policy, enabled, &ss->vrange,
> +			 &ss->opt, &ss->sec))

Can you just pass ss and save on the number of arguments?  It expands what config_match() needs to know about, but 6 args is at the point of being a little hard to follow.

@@ +8775,5 @@
> +	    break;
> +	default:
> +	    PORT_SetError(SSL_ERROR_SERVER_KEY_EXCHANGE_FAILURE);
> +	    return SECFailure;
> +    }

An if statement is probably easier here.  And I thought that you intended to support dh_anon.  Supporting dh_anon entails only dropping the signature.

@@ +8795,5 @@
> +        rv = SECFailure;
> +        goto loser;
> +    }
> +
> +    if (!(keyPair = ssl3_NewKeyPair(privKey, pubKey))) {

My preference is for assign and test on separate lines.

@@ +8824,5 @@
> +
> +    if (kea_def->kea == kea_dhe_rsa)
> +        certIndex = kt_rsa;
> +    else
> +        certIndex = kt_dh;

This goes back to the other comment I made regarding kt_dh.  Using kt_dh as the index for DSS certificate is likely to cause problems, if for no other reason that it is surprising.

I would have expected this to use kea_def->authKeyType.

@@ +9068,5 @@
>  	}
>  	PORT_Free(signed_hash.data);
>  	return SECSuccess;
>  
> +    case kt_dh: {

Let's use ssl_kea_* here.

@@ +9509,5 @@
> +        srvrPubKey = ss->dheKeyPair->pubKey;
> +    }
> +    if (srvrPubKey == NULL) {
> +        /* XXX Is this the right error code? */
> +        PORT_SetError(SSL_ERROR_EXTRACT_PUBLIC_KEY_FAILURE);

This seems OK to me, unless you want to create a new error code for the purpose.

The structure of the code is a little odd though.  I'd have tested for presence first and let the compiler deal with the double-reference:

if (!ss->dheKeyPair || !ss->dheKeyPair->pubKey) {
    PORT_SetError(...);
    return SECFailure;
}
srvrPubKey = ss->dheKeyPair->pubKey;

I do think that you should send an alert here.  I've started to add more alerts to failure modes, which helps considerably with the unit testing.

@@ +9523,5 @@
> +    rv = ssl3_ConsumeHandshakeVariable(ss, &clntPubKey.u.dh.publicValue,
> +	                               2, &b, &length);
> +    if (rv != SECSuccess) {
> +	SEND_ALERT
> +         goto loser;

correct indent here.

That bare macro for SEND_ALERT is a hazard.  I'd rather have this call SSL3_SendAlert() directly than use the macro.  It's not that much extra typing to be completely obvious.

@@ +9526,5 @@
> +	SEND_ALERT
> +         goto loser;
> +    }
> +
> +    isTLS = (PRBool)(ss->ssl3.prSpec->version > SSL_LIBRARY_VERSION_3_0);

(not a review comment, just an observation) I see this so invocation often that I wonder why we don't have a function for it...

@@ +9604,5 @@
> +        if (ss->dheKeyPair) {
> +            serverKeyPair = ss->dheKeyPair;
> +            if (serverKeyPair->pubKey) {
> +                ss->sec.keaKeyBits =
> +                    SECKEY_PublicKeyStrengthInBits(serverKeyPair->pubKey);

Check the fix for bug 1138554 here.  You will need to rebase.  You might not need to write any new code at all here.

@@ +12141,5 @@
> +static const SSLDHEGroupType ssl_default_dhe_groups[] = {
> +    ssl_ff_dhe_2048_group
> +};
> +
> +static const ssl3DHParams *all_ssl3DHParams[] = {

A comment here noting that this array needs to match up with SSLDHEGroupType seems pretty important.

@@ +12143,5 @@
> +};
> +
> +static const ssl3DHParams *all_ssl3DHParams[] = {
> +    NULL, /* none */
> +    &modp_dhe_1024,

As Hubert notes, I think that this might be inadvisable.

@@ +12169,5 @@
> +ssl3_SelectDHParams(sslSocket *ss)
> +{
> +    SSLDHEGroupType selectedGroup = ssl_dhe_group_none;
> +
> +    if (!ss->ssl3.dheGroups) {

I'd invert this check.

@@ +12172,5 @@
> +
> +    if (!ss->ssl3.dheGroups) {
> +	size_t number_of_default_groups = PR_ARRAY_SIZE(ssl_default_dhe_groups);
> +	selectedGroup = selectDHEGroup(ss, ssl_default_dhe_groups,
> +					   number_of_default_groups);

Check indent, here and below.

::: lib/ssl/sslimpl.h
@@ +328,5 @@
>      unsigned int enableNPN              : 1;  /* 26 */
>      unsigned int enableALPN             : 1;  /* 27 */
>      unsigned int reuseServerECDHEKey    : 1;  /* 28 */
>      unsigned int enableFallbackSCSV     : 1;  /* 29 */
> +    unsigned int dheServerSideAllowed   : 1;  /* 30 */

enableServerDhe might be a better name.

::: tests/ssl/sslcov.txt
@@ +94,5 @@
> +  noECC  TLS12 :0033  TLS12_DHE_RSA_WITH_AES_128_CBC_SHA
> +  noECC  TLS12 :0067  TLS12_DHE_RSA_WITH_AES_128_CBC_SHA256
> +  noECC  TLS12 :0039  TLS12_DHE_RSA_WITH_AES_256_CBC_SHA
> +  noECC  TLS12 :006B  TLS12_DHE_RSA_WITH_AES_256_CBC_SHA256
> +  noECC  TLS12 :0016  TLS12_DHE_RSA_WITH_3DES_EDE_CBC_SHA

I don't know this framework operates, but you have a whole lot of DSA code that isn't being tested.  Should this include the DHE_DSS suites?

        Attachment #8610793 -
        Flags: superreview?(martin.thomson)

    
    

    
  
Kai,

Re: SSL_REUSE_SERVER_ECDHE_KEY vs SSL_REUSE_SERVER_DHE_KEY, I also think it's acceptable to have different defaults.

We don't want to change SSL_REUSE_SERVER_ECDHE_KEY for binary compatibility reasons. Default was always on - until recently there was no way to regenerate the ephemeral keys during the lifetime of the server. Changing it to false would be a problem for existing servers that uptake new NSS.

Whereas here, you are adding previously unsupported DHE cipher suites, so there is no risk of causing a performance regression when uptaking new NSS. Existing servers just couldn't have been enabling these cipher suites in the past.

    
    

    
  
One other point regarding my review: I'm actually surprised that this passes even some of the basic tests in ssl_gtest.  I'd like to see at least one new test in there.  Success path at a minimum.  Maybe a resumption test.  A test that validated that the authBits value changes as a result of changing groups with SSL_DHEGroupPrefSet() might be useful too.

    
    

    
  
(In reply to Kai Engert (:kaie) from comment #125)
> Today I've learned:
> 
> For *ECDHE*, by default, the NSS server code reuses the epheremal key.
> 
> In bug 1028582, a socket option SSL_REUSE_SERVER_ECDHE_KEY had been added,
> with the default of "yes reuse". An application must explicitly set the
> socket option to false to request refreshing for every handshake.\

Yes, we also have a discussion to change it back to more sensible default, see bug 1166338.

But there is a difference when using FF-DHE - if the prime used is not a Sphie Germain prime, use of ephemeral values is required. If we enable loading of parameters later, there may be a need to override the default anyway.

    
    

    
  
(In reply to Kai Engert (:kaie) from comment #130)
> Comment on attachment 8610793 [details] [diff] [review]
> v25.patch
> 
> This may be ready for review.
> 
> Looking forward to your comments.

speaking of Sophie Germain primes, the modp_dhe_1024_p is *not* one.

It also doesn't match the Group 2 from RFC 2409... Where did you get it from?

    
    

    
  
(In reply to Hubert Kario from comment #137)
> speaking of Sophie Germain primes, the modp_dhe_1024_p is *not* one.
> It also doesn't match the Group 2 from RFC 2409... Where did you get it from?

As suggested by Wan-Teh in comment 45, I took the 1024 bit group from rfc 5114.
https://tools.ietf.org/html/rfc5114#page-15

    
    

    
  
(In reply to Kai Engert (:kaie) from comment #138)
> https://tools.ietf.org/html/rfc5114#page-15

Correct link is:
https://tools.ietf.org/html/rfc5114#page-4

    
    

    
  
(In reply to Kai Engert (:kaie) from comment #138)
> (In reply to Hubert Kario from comment #137)
> > speaking of Sophie Germain primes, the modp_dhe_1024_p is *not* one.
> > It also doesn't match the Group 2 from RFC 2409... Where did you get it from?
> 
> As suggested by Wan-Teh in comment 45, I took the 1024 bit group from rfc
> 5114.
> https://tools.ietf.org/html/rfc5114#page-15

So they were given in http://csrc.nist.gov/groups/ST/toolkit/documents/Examples/KS_FFC_All.pdf
as a companion to NIST SP 800-56A[1] (as claimed by RFC, not the NIST SP).

Problem is I don't see the description of an algorithm used to generate them. Wan-Teh, do you have reference for that?

 1 - http://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-56Ar2.pdf
Flags: needinfo?(wtc)

    
    

    
  
(In reply to Martin Thomson [:mt] from comment #133)
> kt_* is marked as deprecated:
> https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/sslt.h#46

If you don't want me to use the kt_* values, then using the ssl_kea_* replacements will be even more confusing, because it will create a mix of both types of identifiers.

I suggest that I do a full rename of all symbols in the files I'm touching, first.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        rename-kt-ssl-kea.patch (obsolete)
        — 
          — Splinter Review
      

    


  
This patch replaces all occurrences of (full-word) kt_* to ssl_kea_* in the .c files that I'm touching.

I'd like to check this in as soon as possible, to avoid bitrot and to make it easy to have follow up patches after this renaming.

        Attachment #8611234 -
        Flags: review?(martin.thomson)

    
    

    
  
> There are a few things in here that need attention.  I'm not sure what we should be doing about the use of the
> non-enum kt_* values.  I think that some of the uses here are hiding errors, in particular the use of kt_dh to
> signify the use of a DSA certificate.

yeah, that is known (I identified it in my review and asked for a comment). This is exactly how we currently work for ecdhe_dsa as well. It means you can't turn on ecdhe_dsa and ecdh at the same time. Similarly you can't turn on both dh and dhe_dsa at the same time. Changing this would require quite a bit of surgery for only a little gain.

bob

    
    

    
  
Comment on attachment 8611234 [details] [diff] [review]
rename-kt-ssl-kea.patch

I take that back.

Bob is worried that doing a rename in the middle of a functional patch is a bad idea.

And I'm worried that mixing symbols creates confusion, if my patch uses the new symbols, but surrounding code (e.g. in diff context) still uses old symbols.

But I guess it's the simplest, least confusing approach, that won't cause "merge hell" for those that need to take this patch into older snapshots.

So I'll use the new symbols in the lines I'm changing, only.

        Attachment #8611234 -
        Attachment is obsolete: true

        Attachment #8611234 -
        Flags: review?(martin.thomson)

    
    

    
  
(In reply to Kai Engert (:kaie) from comment #144)
> So I'll use the new symbols in the lines I'm changing, only.

That's all I was asking for.  And maybe in the case statements you find yourself hitting, so we avoid confusion over things like case kt_rsa: case ssl_kea_dh:

    
    

    
  
(In reply to Hubert Kario from comment #140)
>
> So they were given in
> http://csrc.nist.gov/groups/ST/toolkit/documents/Examples/KS_FFC_All.pdf
> as a companion to NIST SP 800-56A[1] (as claimed by RFC, not the NIST SP).
> 
> Problem is I don't see the description of an algorithm used to generate
> them. Wan-Teh, do you have reference for that?

Hi Hubert,

No, I don't have a reference for how the primes were generated.

Note: I found RFC 5114 as follows. I did a web search for "Diffie Hellman
groups RFC". The first search result was RFC 3526, but it didn't have a
1024-bit group. So I looked at the second search result, which was
RFC 5114. I recognized one of the authors, Stephen Kent, a well-known
figure in the IETF security area, so I recommended RFC 5114 to Kai.
That's all I know about RFC 5114.
Flags: needinfo?(wtc)

    
    

    
  
(In reply to Wan-Teh Chang from comment #146)
> (In reply to Hubert Kario from comment #140)
> >
> > So they were given in
> > http://csrc.nist.gov/groups/ST/toolkit/documents/Examples/KS_FFC_All.pdf
> > as a companion to NIST SP 800-56A[1] (as claimed by RFC, not the NIST SP).
> > 
> > Problem is I don't see the description of an algorithm used to generate
> > them. Wan-Teh, do you have reference for that?
> 
> Hi Hubert,
> 
> No, I don't have a reference for how the primes were generated.
> 
> Note: I found RFC 5114 as follows. I did a web search for "Diffie Hellman
> groups RFC". The first search result was RFC 3526, but it didn't have a
> 1024-bit group. So I looked at the second search result, which was
> RFC 5114. I recognized one of the authors, Stephen Kent, a well-known
> figure in the IETF security area, so I recommended RFC 5114 to Kai.
> That's all I know about RFC 5114.

Then I would strongly recommend not to use it. The prime is not a safe prime (Sophie Germain prime) and has dubious origin.

Given that Oakley Group 2 prime from RFC 2409 is a "well known" group and in range of precomputation for nation state level attackers (see weakdh.org) I'd suggest not distributing it either.

The best approach probably would be to generate a "DSA prime" (so that prime generation is fast) and lock the key exchange in "must use ephemeral keys" setting if the application requests 1024 bit DH - this way we give maximum protection for the users that are forced to use it. It would also have a negative performance impact just on them.

    
    

    
  
(In reply to Hubert Kario from comment #147)
> The best approach probably would be to generate a "DSA prime" (so that prime
> generation is fast) and lock the key exchange in "must use ephemeral keys"
> setting if the application requests 1024 bit DH - this way we give maximum
> protection for the users that are forced to use it. It would also have a
> negative performance impact just on them.

Allow me to weigh in. 

Please don't use DSA primes. DH parameters using DSA primes, and ephemeral server keys computed using them, are impossible to validate on the client-side because the subgroup order (q) is not included in the ServerKeyExchange message. 

Use safe primes (i.e. where the subgroup order (p-1)/2 is a Sophie Germain prime), even if they are more expensive to generate and that means regenerating them less often (e.g. weekly). 

It goes without saying that you shouldn't use hard-coded 1024-bit parameters.

I second Hubert's advice of locking the key exchange in "must use ephemeral keys" for 1024-bit primes. That makes downgrade attacks more difficult and offline decryption affect only one session.

Statement: I'm one of the authors of the 'Logjam' paper and a miTLS developer.

    
    

    
  
Hi Santiago,

Thank you for your advice. I agree that we shouldn't use hard-coded
1024-bit DH parameters.

Your comment seems to imply that we should validate p is a safe prime
on the client side. Is that correct?

I believe the NSS function DH_GenParam generates a safe prime because
it calls mpp_make_prime with strong=PR_TRUE:

http://mxr.mozilla.org/nss/ident?i=DH_GenParam
http://mxr.mozilla.org/nss/ident?i=mpp_make_prime

To the NSS team: I propose that the server automatically disables DHE
cipher suites if the server certificate is 1024-bit RSA. This will
allow us to just use fixed DH parameters for 2048 bits and higher.

    
    

    
  
In today's meeting, we came up with the following plan for the initial implementation:

- remove the fixed 1024 bit prime

- don't encode any fixed 1024 bit prime

- don't support loading DH parameters from file

- introduce an additional API named like SSL_EnableWeakDHEPrimeGroup(sslsocket*)

- the first time the above API is called, NSS will generate a new 1024 bit DH parameter,
  and will BLOCK until generation has completed.
  In the initial implementation, this DH parameter will be reused until the process
  terminates.

- because we are limited in the availablity of APIs that are currently exposed by
  softoken, and because this code must land on production branches were softoken cannot
  be upgraded, the initial implementation of this API will call existing code 
  used for generation of DSA primes

    
    

    
  
(In reply to Wan-Teh Chang from comment #149)
> To the NSS team: I propose that the server automatically disables DHE
> cipher suites if the server certificate is 1024-bit RSA. This will
> allow us to just use fixed DH parameters for 2048 bits and higher.

This would be problematic for us with WebRTC.  We currently use 1024-bit RSA keys for authentication there, on the understanding that we use them for a single session and that an attack on authentication in that time-frame is highly unlikely.  We rely - heavily - on the ephemeral key exchange to protect the long term confidentiality of the session.

We already prefer ECDHE and we're moving to ECDSA shortly, but a change like what you propose could cause us to lose around half of our cipher suites.  I don't know of any case where that causes problems, but we don't have great telemetry there.

And yes, DH_GenParam/mpp_make_prime look fine for the purposes of the API Kai describes.

    
    

    
  
Martin: why can't WebRTC use ECDHE_RSA cipher suites? I only proposed
disabling DHE_RSA cipher suites if the server certificate is 1024-bit
RSA.

    
    

    
  
As we discussed offline, WebRTC isn't able to use DHE on the server side now, and we aren't seeing any compatibility issues (well, some, but those are as a result of not having any PFS suites enabled, which I have little sympathy for).  This would simply make this change unavailable to us until we got better keys, which is soon.

I'm now OK with the idea.  It allows us to never implement 1024-bit DHE, which I think is better than the alternative.

    
    

    
  
(In reply to Wan-Teh Chang from comment #149)
> Your comment seems to imply that we should validate p is a safe prime
> on the client side. Is that correct?

It's more complicated. Testing only makes sense if the client
behaves different when p is not a safe prime. What would you do
if p is not a safe prime?

Some clients may be fine with trusting server choices and doing
just cursory checks (e.g. that p is large enough).

Clients that want to validate server parameters need to know the
subgroup order q. Clients can determine q on their own when p is
a safe prime. But absent a way for a server to communicate q
during the handshake, using DSA primes denies clients the
possibility of doing any meaningful validation.

I don't think that it's prohibitively expensive to test if p is a
safe prime. This was discussed recently on the IETF TLS mailing
list:

 https://www.ietf.org/mail-archive/web/tls/current/msg16493.html
 
> I believe the NSS function DH_GenParam generates a safe prime because
> it calls mpp_make_prime with strong=PR_TRUE:

That's my reading too.

    
  
Keywords: helpwanted
Target Milestone: --- → 3.20

    
    

    
  
Can this bug be limited to _DHE_ ?

(Supporting _DH_ ciphers appears to require additional work.)

    
    

    
  
yes, we're interested only in *_DHE_* ciphers, but ones that use RSA and DSS authentication.

Sidenote: *_DH_* were unsupported by openssl till not long ago, so basically nobody is using them.

    
  

        Attachment #8610793 -
        Attachment is obsolete: true

        Attachment #8610793 -
        Flags: review?(rrelyea)

    
    

    
  
(In reply to Martin Thomson [:mt:] from comment #133)
> @@ +8775,5 @@
> > +	    break;
> > +	default:
> > +	    PORT_SetError(SSL_ERROR_SERVER_KEY_EXCHANGE_FAILURE);
> > +	    return SECFailure;
> > +    }
> 
> An if statement is probably easier here.  And I thought that you intended to
> support dh_anon.  Supporting dh_anon entails only dropping the signature.

If I understand correctly, there is no DHE_anon.

If we're focusing on _DHE_, then I'd like to ignore DH_anon for now.

    
    

    
  
(In reply to Kai Engert (:kaie) from comment #157)
> (In reply to Martin Thomson [:mt:] from comment #133)
> > @@ +8775,5 @@
> > > +	    break;
> > > +	default:
> > > +	    PORT_SetError(SSL_ERROR_SERVER_KEY_EXCHANGE_FAILURE);
> > > +	    return SECFailure;
> > > +    }
> > 
> > An if statement is probably easier here.  And I thought that you intended to
> > support dh_anon.  Supporting dh_anon entails only dropping the signature.
> 
> If I understand correctly, there is no DHE_anon.

yes, there there are DH_anon ciphers instead, but they do use ephemeral DH key exchange so calling them DHE_anon is only partially wrong

    
    

    
  
As for implementing DH_anon. I'd say they are a perfect match (when used together with channel binding) for WebRTC use case...

Martin, what's your opinion on that?

    
    

    
  
Do you agree to do the DH_anon discussion, implementation, and potential needs for testing in a separate bug?

    
  
Blocks: 1170510

    
  
Depends on: 1170322

    
    

    
  
(In reply to Hubert Kario from comment #159)
> As for implementing DH_anon. I'd say they are a perfect match (when used
> together with channel binding) for WebRTC use case...
> 
> Martin, what's your opinion on that?

DH_anon isn't of much use to us in the WebRTC context, because we require certificate fingerprints to match sessions to signaling.

As for implementing it, I think that all it needs is the if statement I pointed out (check kea_def->signKeyType == ssl_sign_null and then don't include the signature).  A separate bug would be fine though.

    
    

    
  
(In reply to Martin Thomson [:mt:] from comment #133)
> > +    if (dsaNickName) {
> > +	cert[kt_dh] = PK11_FindCertFromNickname(dsaNickName, NULL);
> 
> I'm not sure that this should be using kt_* as an index here.  Certificate
> choices should be based on the auth type, which is based on ssl_auth_*:
> https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/sslt.h#65
> 
> kt_* is marked as deprecated:
> https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/sslt.h#46


(In reply to Robert Relyea from comment #143)
> This is exactly how we currently work for ecdhe_dsa as well. It means you
> can't turn on ecdhe_dsa and ecdh at the same time. Similarly you can't turn
> on both dh and dhe_dsa at the same time. Changing this would require quite a
> bit of surgery for only a little gain.


It took me a while to understand the above comments.
This is my attempt to summarize and draw a conclusion:

Bob seems mostly right, with one exception.

It seems possible for a server to support both ECDH_ECDSA and ECDHE_ECDSA at the same time, if the server uses an EC certificate that satifies all requirements. I can see we do so in the NSS test suite, a single certificate is used to test both.

However, I understand that Bob's correct that if you want ECDH_RSA, you'll require a different certificate which isn't compatible with ECDH_ECDSA/ECDHE_ECDSA.

It's more strict for DSA/DH keys, because each of DHE_DSS (DSA key with DSS sig), DH_RSA (DH key with RSA sig), DH_DSS (DH key with DSS sig) require a different combination of certificate key and certificate signature. Depending on what kind of cert you have, you can only support exactly one out of these three.


The NSS/TLS implementation uses an array that is indexed by key exchange alg.

sslServerCerts        serverCerts[kt_kea_size];

#define kt_kea_size ssl_kea_size

typedef enum {
    ssl_kea_null     = 0,
    ssl_kea_rsa      = 1,
    ssl_kea_dh       = 2,
    ssl_kea_fortezza = 3,
    ssl_kea_ecdh     = 4,
    ssl_kea_size
} SSLKEAType;

Because the array is defined to be sized according to the SSLKEAType, it seems to correct to me to use kt_* or ssl_kea_* when referring to entries in the array.

If you want to use ssl_auth_* values for accessing entries in the array, I think you should pursue a global change in a separate bug.

Because of this current limitation in the implementation, where a server can own only one certificate for each given type of key, which limits the set of ciphersuites the server can support, and because of the way the size of the array is defined, it seems consistent to use the KEA as the index.

    
    

    
  
(In reply to Martin Thomson [:mt:] from comment #133)
> > +	if (config_match(&ss->cipherSuites[i], policy, enabled, &ss->vrange,
> > +			 &ss->opt, &ss->sec))
> 
> Can you just pass ss and save on the number of arguments?  It expands what
> config_match() needs to know about, but 6 args is at the point of being a
> little hard to follow.

I'll do that, but it doesn't really save much. E.g. some callers don't take vrange from ss, but construct their own. Only the parameters I'm adding are consistently taken all the time from the ss. We'll only reduce the amount of parameters from 6 to 5.

    
    

    
  
(In reply to Martin Thomson [:mt:] from comment #133)
> That bare macro for SEND_ALERT is a hazard.  I'd rather have this call
> SSL3_SendAlert() directly than use the macro.  It's not that much extra
> typing to be completely obvious.

Have you noticed that the macro is actually a no-op?
It's defined as a comment: /* reminder */

I don't know why the patch had used this define/comment, and didn't actually send an alert.

Maybe it was more difficult to send an alert in 2006 when the original patch was written, more difficult than a simple function call?

Is it necessary to delay sending the alert until a later time in the protocol?

    
    

    
  
(In reply to Martin Thomson [:mt:] from comment #133)
> @@ +9604,5 @@
> > +        if (ss->dheKeyPair) {
> > +            serverKeyPair = ss->dheKeyPair;
> > +            if (serverKeyPair->pubKey) {
> > +                ss->sec.keaKeyBits =
> > +                    SECKEY_PublicKeyStrengthInBits(serverKeyPair->pubKey);
> 
> Check the fix for bug 1138554 here.  You will need to rebase.  You might not
> need to write any new code at all here.

I see you changed SECKEY_PublicKeyStrengthInBits(), but I don't understand what you're suggesting that I should do here.

    
    

    
  
Bob, Hubert:

Now that we've agreed on a separate API to enable the weaker 1024 bit group
  SSL_EnableWeakDHEPrimeGroup() :

I wonder if we really need the other API to select amongst the possible larger groups
  SSL_DHEGroupPrefSet()
in the initial version of the patch that we backport.

If you agreed we don't need SSL_DHEGroupPrefSet() now, then we could exclude it from the initial version.

Excluding SSL_DHEGroupPrefSet() at this time might be a good idea, if we're uncertain that it's a great API.
I'm not yet completely convinced that an array of multiple group identifiers, without further clarifying why it's an array, or how NSS choses from the multiple entries in the array, is a helpful API.

    
    

    
  
(In reply to Kai Engert (:kaie) from comment #166)
> I wonder if we really need the other API to select amongst the possible
> larger groups
>   SSL_DHEGroupPrefSet()

Hubert has reminded me why it's a good idea:

- first entry in the array can be default one,
  secondary entries can be "enabled" and automatically selected by a ff-dhe TLS extension

- it's good to have all DHE group control APIs available in the same version of NSS

    
    

    
  

      
      
      
      

        Attached patch
          
          
        dhe-api.patch
        — 
          — Splinter Review
      

    


  
I suggest that we get a formal sign off of the API, even if the rest of the patch gets delayed.

(I hope to have a full patch, that addresses all review comments to which I haven't responded, for review very soon.)

        Attachment #8614670 -
        Flags: superreview?(martin.thomson)

        Attachment #8614670 -
        Flags: review?(rrelyea)

        Attachment #8614670 -
        Flags: feedback?(wtc)

    
    

    
  
+** The weak group will be used as the default group, overriding the first group
+** potentially set with a call to SSL_DHEGroupPrefSet.

I meant to say:

Overriding the "preference for the first group" set with a call to SSL_DHEGroupPrefSet.
The first group set with SSL_DHEGroupPrefSet will still be enabled.

    
    

    
  
Comment on attachment 8614670 [details] [diff] [review]
dhe-api.patch

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

I can live with that.  I'll note that the API won't need to change if we add support for FF-DHE negotiation, which is good.

::: lib/ssl/ssl.h
@@ +307,5 @@
> +** For example, a TLS extension sent by the client might indicate a preference.
> +*/
> +SSL_IMPORT SECStatus SSL_DHEGroupPrefSet(PRFileDesc *fd,
> +                                         SSLDHEGroupType *groups,
> +                                         PRUint16 num_groups);

Wan-Teh noted in my last patch that the convention is to use 'unsigned int' for lengths.

@@ +319,5 @@
> +** At the time this API was initially implemented, the API will enable the
> +** use of 1024 bit DHE parameters. This value might get increased in future
> +** versions of NSS.
> +**
> +** It is allowed to call this API will a NULL value for parameter fd,

s/will/with/

        Attachment #8614670 -
        Flags: superreview?(martin.thomson) → superreview+

    
    

    
  
(In reply to Kai Engert (:kaie) from comment #164)
> (In reply to Martin Thomson [:mt:] from comment #133)
> > That bare macro for SEND_ALERT is a hazard.  I'd rather have this call
> > SSL3_SendAlert() directly than use the macro.  It's not that much extra
> > typing to be completely obvious.
> 
> Have you noticed that the macro is actually a no-op?
> It's defined as a comment: /* reminder */

I didn't even check.  I should know better than to expect that SEND_ALERT

> Is it necessary to delay sending the alert until a later time in the
> protocol?

In some cases, yes.  Sending the alert requires certain locks to not be in place.  In this case, I think you are safe, because they shouldn't be in place.

But I could be wrong.  The best way to work out what to do is to write a test case that hits these error conditions.  In my experience, that requires an alert, otherwise one peer fails the connection and the other one doesn't realize what has happened.  It all works out.

(In reply to Kai Engert (:kaie) from comment #165)
> > Check the fix for bug 1138554 here.  You will need to rebase.  You might not
> > need to write any new code at all here.
> 
> I see you changed SECKEY_PublicKeyStrengthInBits(), but I don't understand
> what you're suggesting that I should do here.

My point was that the section of code that you are adding to has completely changed.  I think that your code is largely correct, it's just been superceded.  It might pay to do the rebase and see.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v34.patch (obsolete)
        — 
          — Splinter Review
      

    


  
This might be close to be done (hopefully).

It enables additional ciphersuites and adds tests.

Support for 1024 bit DHE groups is untested. (I executed selfserv -W 1, it passed, but I haven't checked yet if the 1024 bit groups are used.)

I'm currently preparing an experimental build for Hubert with this patch, if that build succeeds, maybe he'll be able to give feedback.

At one place I use an ugly hack to cast away const.
+    prime = (SECItem *)&gWeakDHParams->prime;
+    base = (SECItem*)&gWeakDHParams->base;

I'll be traveling until Sunday, and I've finished this patch under time pressure, and I didn't understand why this was required, see the comment in the patch next to this code. I'm sure this will be easy to cleanup once understood.

I'll try to check your comments during the next 4 days while I'm travelling, in order to unblock this bug if necessary.

        Attachment #8614753 -
        Flags: review?(martin.thomson)

    
    

    
  
Note that the addition to the tests that create DSA keys were created by copying/pasting from the existing code that creates the other key types, and was simply modified to use DSA.

I think those code blocks in the test script don't require review.

    
    

    
  
Comment on attachment 8614753 [details] [diff] [review]
v34.patch

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

I haven't been completely thorough, because I have to take tomorrow and Friday off and I know that a) you want to make progress, b) I trust that you will fix this up, and c) this is in pretty good shape overall.

I would like to encourage you to try adding a test case (or several) to the gtests that we've started to build.  That should help with the alert problem you have (you can test some of those more specific error conditions), and it might be a quicker way to expose errors.

::: cmd/selfserv/selfserv.c
@@ +2629,5 @@
>  	setupCertStatus(certStatusArena, ocspStaplingMode, cert[kt_rsa], kt_rsa,
>  			&pwdata);
>      }
> +    if (dsaNickName) {
> +	cert[ssl_kea_dh] = PK11_FindCertFromNickname(dsaNickName, &pwdata);

I was looking into this, and I think that I understand this more now.

Did you know that (int)ssl_kea_dh == (int)ssl_auth_dsa ?

Would the latter work for you?

@@ +2639,5 @@
> +	if (privKey[ssl_kea_dh] == NULL) {
> +	    fprintf(stderr, "selfserv: Can't find Private Key for cert %s\n", 
> +	            dsaNickName);
> +	    exit(11);
> +	}	    

Trailing space here.

::: lib/ssl/ssl.h
@@ +192,5 @@
>  #define SSL_ENABLE_FALLBACK_SCSV       28 /* Send fallback SCSV in
>                                             * handshakes. */
>  
> +/* SSL_ENABLE_SERVER_DHE controls whether DHE is enabled for the server socket.
> + * SSL_ENABLE_SERVER_DHE is currently disabled by default.

The value is initialized to PR_TRUE in sslsock.c:87

@@ +322,5 @@
> +**
> +** It is allowed to call this API will a NULL value for parameter fd,
> +** which will prepare the global parameters that NSS will reuse for the remainder
> +** of the process lifetime. This can be used early after startup of a process,
> +** to avoid a delay when handling incoming client connections.

Please note that this will not enable the weak group on sockets and that it still needs to called for every socket individually.

::: lib/ssl/ssl3con.c
@@ +8825,5 @@
> +
> +    if (kea_def->kea == kea_dhe_rsa)
> +        certIndex = ssl_kea_rsa;
> +    else
> +        certIndex = ssl_kea_dh;

I still think that the test needs to be on kea_def->signKeyType.  And based on the above, certIndex might be changed to ssl_auth_*.

@@ +9493,5 @@
> +static SECStatus
> +ssl3_HandleDHClientKeyExchange(sslSocket *ss,
> +                               SSL3Opaque *b,
> +                               PRUint32 length,
> +                               SECKEYPrivateKey *serverKey)

I notice that the public key is taken from ss->dheKeyPair->pubKey, why pass in the private key specially?

::: lib/ssl/ssl3ext.c
@@ +1125,5 @@
>          sslSessionID sid;
>          PORT_Memset(&sid, 0, sizeof(sslSessionID));
>  
> +        if (ss->ssl3.hs.kea_def->kea == kea_ecdhe_rsa ||
> +            ss->ssl3.hs.kea_def->kea == kea_dhe_rsa) {

Does session resumption work with DSS?

        Attachment #8614753 -
        Flags: review?(martin.thomson) → review+

    
    

    
  
(In reply to Martin Thomson [:mt:] from comment #174)
> @@ +9493,5 @@
> > +static SECStatus
> > +ssl3_HandleDHClientKeyExchange(sslSocket *ss,
> > +                               SSL3Opaque *b,
> > +                               PRUint32 length,
> > +                               SECKEYPrivateKey *serverKey)
> 
> I notice that the public key is taken from ss->dheKeyPair->pubKey, why pass
> in the private key specially?

Because that's how existing code in caller function ssl3_HandleClientKeyExchange works, it's also given to the RSA and ECDH client key exchange functions.

It selects the serverKey, based on other criteria, including potential stepdown handling, and also error handling.

I see that for ECDH, the caller function already selects the public key, too, and passes it on. For consistency, I'll change the caller function to pass the public key to ssl3_HandleDHClientKeyExchange, too.

    
    

    
  
(In reply to Martin Thomson [:mt:] from comment #174)
> ::: cmd/selfserv/selfserv.c
> @@ +2629,5 @@
> >  	setupCertStatus(certStatusArena, ocspStaplingMode, cert[kt_rsa], kt_rsa,
> >  			&pwdata);
> >      }
> > +    if (dsaNickName) {
> > +	cert[ssl_kea_dh] = PK11_FindCertFromNickname(dsaNickName, &pwdata);
> 
> I was looking into this, and I think that I understand this more now.
> 
> Did you know that (int)ssl_kea_dh == (int)ssl_auth_dsa ?
> 
> Would the latter work for you?
> 
> ::: lib/ssl/ssl3con.c
> @@ +8825,5 @@
> > +
> > +    if (kea_def->kea == kea_dhe_rsa)
> > +        certIndex = ssl_kea_rsa;
> > +    else
> > +        certIndex = ssl_kea_dh;
> 
> I still think that the test needs to be on kea_def->signKeyType.  And based
> on the above, certIndex might be changed to ssl_auth_*.


I don't understand why you are still asking for this change after my comment 162.
Can you please say which part of comment 162 didn't convince you?

    
    

    
  
(In reply to Martin Thomson [:mt:] from comment #171)
> (In reply to Kai Engert (:kaie) from comment #165)
> > > Check the fix for bug 1138554 here.  You will need to rebase.  You might not
> > > need to write any new code at all here.
> > 
> > I see you changed SECKEY_PublicKeyStrengthInBits(), but I don't understand
> > what you're suggesting that I should do here.
> 
> My point was that the section of code that you are adding to has completely
> changed.  I think that your code is largely correct, it's just been
> superceded.  It might pay to do the rebase and see.


I still don't get it. I've been rebasing my patch to the latest NSS all the time. I'm looking at the original patch from 2006, attachment 238120 [details] [diff] [review], and the surrounding  context around the code block, 10 lines before and 10 lines after, has been completely unchanged.

If you want me to continue to look into this, then I'd like to ask that you please elaborate in more detail, like pasting the code blocks, old and new, that you are referring to, that you say have changed, need rebase, etc.

I believe you are talking about this code block:
    if (kea_def->kea == kea_dhe_dss ||
        kea_def->kea == kea_dhe_rsa) {
        if (ss->dheKeyPair) {
            serverKeyPair = ss->dheKeyPair;
            if (serverKeyPair->pubKey) {
                ss->sec.keaKeyBits =
                    SECKEY_PublicKeyStrengthInBits(serverKeyPair->pubKey);
            }
        }
and I'm sorry, but I don't understand what you're referring to.

    
    

    
  
(In reply to Kai Engert (:kaie) from comment #176)
> I don't understand why you are still asking for this change after my comment
> 162.
> Can you please say which part of comment 162 didn't convince you?

Oh, I read this:

> Because the array is defined to be sized according to the SSLKEAType, it seems to correct to me to use kt_* or ssl_kea_* when referring to entries in the array.

Which I think is bad logic.  And somehow missed this:

> If you want to use ssl_auth_* values for accessing entries in the array, I think you should pursue a global change in a separate bug.

Which I think is excellent logic.

    
    

    
  
(In reply to Kai Engert (:kaie) from comment #177)
> (In reply to Martin Thomson [:mt:] from comment #171)
> > (In reply to Kai Engert (:kaie) from comment #165)
> > > > Check the fix for bug 1138554 here.  You will need to rebase.  You might not
> > > > need to write any new code at all here.
> > > 
> > > I see you changed SECKEY_PublicKeyStrengthInBits(), but I don't understand
> > > what you're suggesting that I should do here.
> > 
> > My point was that the section of code that you are adding to has completely
> > changed.  I think that your code is largely correct, it's just been
> > superceded.  It might pay to do the rebase and see.
> 
> 
> I still don't get it.

And that's also my fault.  Sorry about that.  The code in question is basically identical to what I changed here: https://hg.mozilla.org/projects/nss/diff/ae72d76f8d24/lib/ssl/ssl3con.c#l1.122  And I missed the clues.

But that's for the ServerKeyExchange.  I wasn't prepared for this much code duplication.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v37.patch (obsolete)
        — 
          — Splinter Review
      

    


  
This patch adds more testing and should address all of the requests,
except:

(In reply to Martin Thomson [:mt:] from comment #174)
> Does session resumption work with DSS?

No. With the attached patch, the test suite reports failures.

(1) strsclnt reports failures if using DHE_DSS and the server is allowed to
    use the server session cache.

(2) strsclnt reports SSL_ERROR_RX_UNEXPECTED_NEW_SESSION_TICKET
    if both server and client are enabled to use session tickets.

    
    

    
  
Hmm.  I don't think that anyone would be sad if resumption didn't work for _DSS_ suites, but we should probably just let the full handshake complete, not fail hard.

    
    

    
  


  
(In reply to Martin Thomson [:mt:] from comment #181)
> Hmm.  I don't think that anyone would be sad if resumption didn't work for
> _DSS_ suites, but we should probably just let the full handshake complete,
> not fail hard.

I agree that we shouldn't fail.

Your suggestion to let the full handshake complete seems to require quite a bit of changes to the state machine.

I would like to avoid having to touch the state machine right now.

My alternative suggestion is, if we're using a DSS ciphersuite, the server code shouldn't send a session ticket at all.

This attachment (on top of the other changes) is my attempt of a minimal invasive patch that implements this suggestion. I no longer get a protocol failure with strsclnt (just cache misses).

    
  

        Attachment #8620925 -
        Flags: feedback?(martin.thomson)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v38.patch (obsolete)
        — 
          — Splinter Review
      

    


  
This might be ready for check in.

Bob, would you like to have a final look?

        Attachment #8614753 -
        Attachment is obsolete: true

        Attachment #8617489 -
        Attachment is obsolete: true

        Attachment #8620929 -
        Flags: review?(rrelyea)

    
    

    
  
Comment on attachment 8620925 [details] [diff] [review]
no-dss-session-ticket.patch (incremental)

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

That looks reasonable to me.  We don't see a lot of DSA use currently, so dropping support for resumption seems harmless.  It would be good to understand why, but I can appreciate wanting to concentrate on getting what you have done first.

::: lib/ssl/ssl3con.c
@@ +7635,5 @@
> +	case kea_dhe_dss_export:
> +	case kea_dh_dss_export:
> +	case kea_dh_dss:
> +	    /* TODO: Fix session tickets for DSS. As of bug 102794, the server
> +	     * code rejects the session ticket received from the client. */

It might be good to have a bug number for this TODO.

        Attachment #8620925 -
        Flags: feedback?(martin.thomson) → feedback+

    
    

    
  
Comment on attachment 8620929 [details] [diff] [review]
v38.patch

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

r+ with some notes:

There are a couple of places it looks like new ciphers which were added were not added everywhere, but one place seems like it would generate compile errors, so I think just answering my question will be sufficient. If you really did miss the new ciphers in sslinfo.c (which wouldn't generate any compile time, or even test errors), then please add them.

The rest are just general comments or quests to add comments  and license headers.

::: lib/ssl/dhe-param.c
@@ +1,1 @@
> +static const unsigned char ff_dhe_g2[] = { 2 };

Please include the standard mozilla license header here.

::: lib/ssl/ssl.def
@@ +173,5 @@
>  ;+};
> +;+NSS_3.20 {    # NSS 3.20 release
> +;+    global:
> +SSL_DHEGroupPrefSet;
> +SSL_EnableWeakDHEPrimeGroup;

We probably should add a comment that if new symbols are added before NSS 3.20 is released, they should be called NSS_3.20a

::: lib/ssl/sslimpl.h
@@ +296,1 @@
>  #endif /* NSS_DISABLE_ECC */

Make sure you do a debug build and run it before checking in. This looks right, but a debug build will verify it.

::: lib/ssl/sslinfo.c
@@ +144,5 @@
>  {0,CS(TLS_DHE_DSS_WITH_RC4_128_SHA),          S_DSA, K_DHE, C_RC4, B_128, M_SHA, 0, 0, 0, },
>  {0,CS(TLS_DHE_RSA_WITH_AES_128_CBC_SHA256),   S_RSA, K_DHE, C_AES, B_128, M_SHA256, 1, 0, 0, },
>  {0,CS(TLS_DHE_RSA_WITH_AES_128_GCM_SHA256),   S_RSA, K_DHE, C_AESGCM, B_128, M_AEAD_128, 1, 0, 0, },
>  {0,CS(TLS_DHE_RSA_WITH_AES_128_CBC_SHA),      S_RSA, K_DHE, C_AES, B_128, M_SHA, 1, 0, 0, },
> +{0,CS(TLS_DHE_DSS_WITH_AES_128_GCM_SHA256),   S_DSA, K_DHE, C_AESGCM, B_128, M_AEAD_128, 1, 0, 0, },

Hmm you added 3 new cipher suites in sslimpl.h, where are the other two?

::: lib/ssl/sslproto.h
@@ +196,1 @@
>  #define TLS_DHE_RSA_WITH_AES_256_CBC_SHA256     0x006B

where's TLS_DHE_DSS_WITH_AES_128_GCM_SHA256 which you added to sslenum.c and sslinfo.c?

::: lib/ssl/sslsock.c
@@ +1430,5 @@
> +
> +    PORT_Assert(!gWeakDHParams && !gWeakParamsPQG);
> +
> +    rv = PK11_PQG_ParamGenV2(1024, 160, 64 /*middle sized seed*/,
> +                             &gWeakParamsPQG, &vfy);

Good, this code will generate provable primes.
NOTE: for 1024 seedBytes of 20 would be sufficient. 64 is the max size of seed bytes that will work (above 64 bytes/512 bits we the verify will fail).

It's confusing because seedBytes is in bytes, but N is in bits (that's why 20 is sufficient 20*8=160).

bob

        Attachment #8620929 -
        Flags: review?(rrelyea) → review+

    
    

    
  
re: comment 184. It's almost certainly because the caching code on the server uses the server's key to wrap the master secret. In the RSA, DH, and ECDH/ECDSA cases you can use the server's cert to either wrap the key directly or to wrap generate an exchange key which you can wrap the key. In the DSA case, you can't (DSA keys won't work in PKCS #11 for derive).

Servers store the master secrets wrapped in a shared memory segment potentially shared by multiple processes, so we can't share a PKCS #11 handle which won't be accessible by the various servers, so we store the wrapped master secret. We know all the servers share a keystore, so we uses the server's private key to do the wrapping. We'll have to figure something else out for the DSA case.

    
  
Blocks: 1174677

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v39.patch
        — 
          — Splinter Review
      

    


  
(In reply to Martin Thomson [:mt:] from comment #184)
> > +	    /* TODO: Fix session tickets for DSS. As of bug 102794, the server
> > +	     * code rejects the session ticket received from the client. */
> 
> It might be good to have a bug number for this TODO.

Bug 1174677 filed and mentioned in the comment.


(In reply to Robert Relyea from comment #185)
> ::: lib/ssl/dhe-param.c
> Please include the standard mozilla license header here.

done


> ::: lib/ssl/ssl.def
> > +SSL_DHEGroupPrefSet;
> > +SSL_EnableWeakDHEPrimeGroup;
> 
> We probably should add a comment that if new symbols are added before NSS
> 3.20 is released, they should be called NSS_3.20a

done


> ::: lib/ssl/sslimpl.h
> @@ +296,1 @@
> >  #endif /* NSS_DISABLE_ECC */
> 
> Make sure you do a debug build and run it before checking in. This looks
> right, but a debug build will verify it.

My local builds and local test suite runs are always in debug mode, and they work.


> ::: lib/ssl/sslinfo.c
> Hmm you added 3 new cipher suites in sslimpl.h, where are the other two?

Thanks a lot, good catch. I've added the 2 missing ciphers to the information table.

They are CBC, and I've copied from the almost identical SHA1 versions, and changed to M_SHA to M_SHA256, so I assume the new lines are correct.


> ::: lib/ssl/sslproto.h
> where's TLS_DHE_DSS_WITH_AES_128_GCM_SHA256 which you added to sslenum.c and
> sslinfo.c?

Despite previously unused, the define was already there!


> ::: lib/ssl/sslsock.c
> > +    rv = PK11_PQG_ParamGenV2(1024, 160, 64 /*middle sized seed*/,
> > +                             &gWeakParamsPQG, &vfy);
> 
> NOTE: for 1024 seedBytes of 20 would be sufficient. 64 is the max size of
> seed bytes that will work (above 64 bytes/512 bits we the verify will fail).

I've clarified the comment to mention it's the maximum possible seed size.
More seed is better, right?


I'm carrying forward r+, as most changes are comments, and the duplicated/adjusted addition to sslinfo.c seems obviously correct:

 {0,CS(TLS_DHE_DSS_WITH_AES_256_CBC_SHA),      S_DSA, K_DHE, C_AES, B_256, M_SHA, 1, 0, 0, },
+{0,CS(TLS_DHE_DSS_WITH_AES_256_CBC_SHA256),   S_DSA, K_DHE, C_AES, B_256, M_SHA256, 1, 0, 0, },

 {0,CS(TLS_DHE_DSS_WITH_AES_128_CBC_SHA),      S_DSA, K_DHE, C_AES, B_128, M_SHA, 1, 0, 0, },
+{0,CS(TLS_DHE_DSS_WITH_AES_128_CBC_SHA256),   S_DSA, K_DHE, C_AES, B_128, M_SHA256, 1, 0, 0, },

        Attachment #8620925 -
        Attachment is obsolete: true

        Attachment #8620929 -
        Attachment is obsolete: true

        Attachment #8622408 -
        Flags: review+

    
  

        Attachment #8614670 -
        Flags: review?(rrelyea)

        Attachment #8614670 -
        Flags: feedback?(wtc)

    
    

    
  
https://hg.mozilla.org/projects/nss/rev/c4f90cedccdf
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED

    
    

    
  
Kai,

This seems to break the gtests:

[  FAILED  ] 5 tests, listed below:
[  FAILED  ] SkipTls10/TlsSkipTest.SkipCertificate/0, where GetParam() = ("TLS", 769)
[  FAILED  ] SkipVariants/TlsSkipTest.SkipCertificate/0, where GetParam() = ("TLS", 770)
[  FAILED  ] SkipVariants/TlsSkipTest.SkipCertificate/1, where GetParam() = ("TLS", 771)
[  FAILED  ] SkipVariants/TlsSkipTest.SkipCertificate/2, where GetParam() = ("DTLS", 770)
[  FAILED  ] SkipVariants/TlsSkipTest.SkipCertificate/3, where GetParam() = ("DTLS", 771)
Flags: needinfo?(kaie)

    
    

    
  
(In reply to Eric Rescorla (:ekr) from comment #189)
> This seems to break the gtests:

Thanks Eric for filing bug 1176732 and adjusting the tests for different error conditions when DHE is enabled by default.

I had not noticed the failed gtests earlier, because when run through all.sh, the final test output didn't mention those tests in its counter (only one failure with core caused by the reference counting imbalance, which we had discussed further above).
Flags: needinfo?(kaie)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: