1237514 - Use SSLAuthType to select certificates rather than SSLKEAType

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Martin Thomson [:mt:]]

(This is a bit of an essay, but it's a non-trivial change set that apparently needs a lot of explanation.)

The code is currently filled with comments like this:

          /* XXX SSLKEAType isn't really a good choice for 
           * indexing certificates. It doesn't work for
           * (EC)DHE-* ciphers. Here we use a hack to ensure
           * that the server uses an RSA cert for (EC)DHE-RSA.
           */

And a bunch of kludgy code like:

   /* It has been suggested to test kea_def->signKeyType instead, and to use
    * ssl_auth_* instead. Investigate what to do. See bug 102794. */
   if (kea_def->kea == kea_dhe_rsa)
       certIndex = ssl_kea_rsa;
   else
       certIndex = ssl_kea_dh;
   key = ss->serverCerts[certIndex].SERVERKEY

This is because the code originally didn't distinguish properly between:

 1. key exchange
 2. signature schemes
 3. authentication basis

The patch I'm about to upload soon makes that separation.  It indexes server certificates on sockets by SSLAuthType rather than SSLKEAType and deals with all the consequences that follow from that.

libssl no longer uses the kt_* macros for anything as a result.  Everything properly identifies and uses SSLKEAType, SSLSignType or SSLAuthType.  Legacy exported functions are the only place where there is any remaining confusion.

New API points are added for configuring certificates and certificate chains without this confusion, including the supplementary information for certificates (OCSP and CT SignedCertTimestamps).  Old API points are retained, but just call into the new stuff.

I discovered that I accidentally broke ECDH_RSA and ECDH_ECDSA suites (note static ECDH, not ECDHE) during the process, but I've made some changes to the affected code that should make it less brittle:

 - Added a value for SSLAuthType (ssl_auth_ecdh) covers ECDH suites.  That's a change to the info we give out in sslinfo.c, which is the only functionality change.  More on that below.

 - The session cache is keyed based on SSLAUTHType rather than SSLKEAType.

 - Session tickets don't store the exchKeyType value any more, using the existing authAlgorithm field.

I discovered that we abuse ECDSA certificates in various ways.  The worst offense is that the key from an ECDSA certificate is used as an ECDH key in wrapping a session ticket key (we don't *know* that this is bad, but we do know that we don't know that it's good; in other words, Karthik says don't do that).

The new code based on SSLAuthType separates ECDH and ECDSA certificates.  Old code that configures a cert for kt_ecdh actually populates both ECDH and ECDSA slots so that it keeps working.

The change that I've made to SSL_GetCipherSuiteInfo is probably the only backwards compatibility hazard.  The current interpretation of SSLAuthType is that it is the type of signature that signs the end-entity certificate.  With a real mix of certificates today, and the signature_algorithms extension, my opinion is that doesn't make a lot of sense.

I've taken the liberty of changing all the ECDH cipher suites so that they report an SSLAuthType of ECDH.  SSLAuthType now uniformly means the type of key that the certificate contains.  This is consistent with our ECDHE_ECDSA suites, which could have an RSA signature in the certificate.  I think that it's the right change, but it will need some explanation.

I haven't changed the rules for disabling cipher suites though, which rely on this strange certificate characteristic.  An ECDH_RSA suite will only be offered if the certificate is signed with an RSA key.

There were a few bugs I discovered in the process:

 - SSL_ConfigSecureServer() seems to accept an SSLKEAType of ssl_kea_null, which is never really used, but that value causes a lot of checks to pass.  I think that we should reject that value.  We could also remove the slot that this takes up (which isn't ever used), but that would probably mean introducing a slot lookup function.  I'd like input on this.

 - We don't (and haven't ever) checked that we have a token that supports the key exchange type.  See also bug 1198298.

 - NSS accepts Diffie-Hellman keys in certificate and uses them for DSA.  That seems pretty badly wrong, but I didn't want to remove them.

 - The kea_alg_defs table in ssl3con.c checked that we had CKM_DH_PKCS_DERIVE for a DSA certificate.  I changed that to CKM_DSA.  What was CKM_ECDH1_DERIVE is now CKM_ECDSA and CKM_ECDH1_DERIVE to account for the new SSLAuthType value.

I added four new functions:

SSL_ConfigSecureServerCert - replaces SSL_ConfigSecureServer
SSL_ConfigSecureServerCertChain - replaces SSL_ConfigSecureServerWithCertChain
SSL_SetSignedCertTimestampsA - replaces SSL_SetSignedCertTimestamps
SSL_SetStapledOCSPResponsesA - replaces SSL_SetStapledOCSPResponses

All the old functions just call into to their newer version with a mapped type.  I'm interested in whether we might just replace SSL_SetSignedCertTimestamps given that it was introduced so recently.

I didn't add an equivalent to NSS_FindCertKEAType, which was a helper to determine where to slot a certificate.  We don't use it internally any more, but I've left it with a note.

Comment 1

[Martin Thomson [:mt:]]

Attachment: bug1237514.patch

I can't upload this to rietveld for indecipherable and frustrating reasons.

Comment 2

[Martin Thomson [:mt:]]

http://codereview.appspot.com/280660043

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8704982 [details] [diff] [review]
bug1237514.patch

Might be good to get Tim to look at this too.

Comment 4

[Martin Thomson [:mt:]]

Note to self: fix ssl3_KEAAllowsSessionTicket

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8704982 [details] [diff] [review]
bug1237514.patch

Comments on rietveld.

Comment 6

[Martin Thomson [:mt:]]

Attachment: bug1237514.patch

Hi Bob, I realize that this is a relatively large patch, but I'm only looking for a fairly narrow review.  The code in sslcert.c that implements the old certificate configuration methods needs checking to ensure that I've not regressed ABI compatibility.

The methods I'm concerned about are:
SSL_ConfigSecureServerWithCertChain, SSL_ConfigSecureServer, SSL_ConfigSecureServer, and SSL_SetSignedCertTimestamps.

You can read comment 0 to understand the approach that I've taken, which might help.  In short, new certificates are configured based on their type alone.  They are segregated into slots by type, ECC certs are further segregated by named curve.

These all take SSLKEAType arguments, which I'm mapping onto the new certificate slot type; sometimes they map onto multiple of the new certificate slots.  In that case, I've stored multiple copies of the information.

Note that the existing tests call the above methods in an unexpected order, and I had to maintain the ability to partially configure an SSLKEAType with OCSP responses or timestamps (even without a certificate).

If you get a chance, checking for changes in SSL_GetCipherSuiteInfo would be helpful.  I've added a field to that, and I know that makes the ABI compatibility checks complain.

Comment 7

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/72ae99c547be

Comment 8

[Martin Thomson [:mt:]]

Attachment: bug1237514-landed.patch

As landed.  I would still appreciate a sanity check.

Comment 9

[Franziskus Kiefer [:franziskus]]

Attachment: sslcert-coverity.patch

Coverity found some issues in this patch. This should fix them.

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 8737603 [details] [diff] [review]
sslcert-coverity.patch

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

Thanks for looking into this.  Conditional on fixing the other thing.

I note that the keyPair leak is also a problem with SSL_ConfigServerCert().  Add a call to ssl3_FreeKeyPair to the end of that function as well.

::: lib/ssl/sslcert.c
@@ +672,5 @@
>                                                 cert, certChainOpt, keyPair);
>  #endif
>  
>          default:
> +            ssl3_FreeKeyPair(keyPair);

Actually, this isn't entirely right.  The keyPair needs to be freed unconditionally.  All the methods that accept a key pair also increment the refCount, leaving the reference we create here hanging.

Comment 11

[Franziskus Kiefer [:franziskus]]

Attachment: sslcert-coverity.patch

landed as //hg.mozilla.org/projects/nss/rev/8c45bd9b37a6

Comment 12

[Robert Relyea]

Comment on attachment 8737531 [details] [diff] [review]
bug1237514-landed.patch

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

OK, I've reviewed most of this patch, and have the following general comments:

First, this patch is long time needed and applaud the general design. In the face of some serious deficiencies, it should not be lost that the majority of this patch is needed and well put together. Now the issues.

1) There were at least 2 places where it appears the patch won't actually compile. I presume if the patch is truly applied, that those areas were already fixed.
2) The patch as reviewed removed a number of global functions. That will cause problems in real servers that are using those functions. New versions need to be written which integrates in the new way of doing things so that old servers will keep working.
3) I see no patches to selfserv. While updating and testing new functionality in gtests are fine, existing users look to selfserv to see how to use new functionality. I'm sort of at a loss to understand how selfserv is still functioning given that we've removed functions that it depends on.

The above 3 things lead me to believe that this isn't the latest patch as checked in as our build trees should have failed.

I have a few other global comments.
a. We need separate certs for ecdh_ecdsa and ecdh_rsa. We should split those auth types.
b. It appears auth type dh (really dh_dsa and dh_rsa) are missing.
c. I think we shouldn't restrict the negotiated EC curve based on the ECDSA certificate. My main worry is this is a change in behavior between what we have been doing and what this new code does. I may be able to be convinced otherwise (perhaps the old code just worked by accident).

Thanks for doing this patch martin. Is there a newer version available?

::: lib/ssl/ssl.h
@@ +814,5 @@
> +** already been configured.
> +*/
> +SSL_IMPORT SECStatus SSL_ConfigServerCert(
> +    PRFileDesc *fd, CERTCertificate *cert, SECKEYPrivateKey *key,
> +    const SSLExtraServerCertData *data, unsigned int data_len);

My only comment is while we're revamping SSL_ConfigServerCert, it might be worth adding a way to handle SNI support at this level. That would require adding an optional URL to the function.

The underlying code to support this may be a bit  more complex, though and adding a non-functioning prameter is probably wrong. So I think this is the best choice. It fixes a number of existing deficiencies with the current API.

::: lib/ssl/ssl3ecc.c
@@ -47,5 @@
> -#define SSL_IS_CURVE_NEGOTIATED(curvemsk, curveName) \
> -    ((curveName > ec_noName) &&                      \
> -     (curveName < ec_pastLastName) &&                \
> -     ((1UL << curveName) & curvemsk) != 0)
> -

So you are deleteing SSL_IS_CURVE_NEGOTIATED() but you still use it below.

@@ +587,5 @@
> +        return ec_noName;
> +    }
> +
> +    if (ss->sec.serverCert->certType.authType == ssl_auth_ecdsa ||
> +        ss->sec.serverCert->certType.authType == ssl_auth_ecdh) {

So there is a difference between ecdh_ecdsa and ecdh_rsa. They will point to two different certs. If we are making this change, we should fix the current restriction that we can't support both.

@@ +594,5 @@
> +         * negotiated (or supported_curves was absent), double check that. */
> +        PORT_Assert(SSL_IS_CURVE_NEGOTIATED(ss->ssl3.hs.negotiatedECCurves,
> +                                            ec_curve));
> +        if (!SSL_IS_CURVE_NEGOTIATED(ss->ssl3.hs.negotiatedECCurves,
> +                                     ec_curve)) {

I'm not sure this is right. We do need to theck the ecdh case, but in the ecdsa case, the cure doesn't necessarily have to patch the ECDSA key (at least it didn't). If it does then that pretty much restricts our ability to negotiate curves on the server side since our ECDSA key will determine the one curve we can support. Dropping the authType == ssl_auth_ecdsa should fix this.

::: lib/ssl/sslcon.c
@@ +1,2 @@
>  /*
> + * Basic SSL handshake functions.

I think is this all SSL2 code at this point.

::: lib/ssl/sslsecur.c
@@ -626,5 @@
> -/*
> -** Return SSLKEAType derived from cert's Public Key algorithm info.
> -*/
> -SSLKEAType
> -NSS_FindCertKEAType(CERTCertificate *cert)

This is a global function exported by ssl.def, so we can't just remove it. It needs to return something rational.

@@ -769,5 @@
> -
> -/* XXX need to protect the data that gets changed here.!! */
> -
> -SECStatus
> -SSL_ConfigSecureServer(PRFileDesc *fd, CERTCertificate *cert,

These may be depricated, but we still need to support them or existing servers. They need to map kea to ssl_auth_type (perhaps with info from the cert).

@@ -779,5 @@
> -
> -SECStatus
> -SSL_ConfigSecureServerWithCertChain(PRFileDesc *fd, CERTCertificate *cert,
> -                                    const CERTCertificateList *certChainOpt,
> -                                    SECKEYPrivateKey *key, SSL3KEAType kea)

Same.

::: lib/ssl/sslsnce.c
@@ +453,5 @@
> +    to->u.ssl3.certTypeArgs = 0U;
> +    switch(from->authType) {
> +#ifndef NSS_DISABLE_ECC
> +        case ssl_auth_ecdsa:
> +        case ssl_auth_ecdh:

I think we only want to do this for ecdh not ecdsa.

@@ +540,5 @@
> +    switch (from->authType) {
> +#ifndef NSS_DISABLE_ECC
> +        case ssl_auth_ecdsa:
> +        case ssl_auth_ecdh:
> +            to->certType.u.namedCurve = (ECName)from->u.ssl3.certTypeArgs;

Same comment as above.

::: lib/ssl/sslsock.c
@@ -2685,5 @@
>  }
>  
>  SECStatus
> -SSL_SetStapledOCSPResponses(PRFileDesc *fd, const SECItemArray *responses,
> -                            SSLKEAType kea)

This function is global. We need to emulate it because this is how existing servers work.
ssl.def:SSL_SetStapledOCSPResponses;
ssl.h:/* SSL_SetStapledOCSPResponses stores an array of one or multiple OCSP responses
ssl.h:SSL_SetStapledOCSPResponses(PRFileDesc *fd, const SECItemArray *responses,
sslsock.c:SSL_SetStapledOCSPResponses(PRFileDesc *fd, const SECItemArray *responses,

::: lib/ssl/sslt.h
@@ +100,5 @@
> +    ssl_auth_ecdh = 5,
> +    ssl_auth_rsa_sign = 6, /* RSA PKCS#1.5 signing */
> +    ssl_auth_rsa_pss = 7,
> +    ssl_auth_psk = 8,
> +    ssl_auth_size /* number of authentication types */

Question: where is auth_dh?

Also we need to distinguish (at leas in the code) between auth_ecdh_ecdsa and auth_ecdh_rsa. They are two different certs. If we are updating this code we should remove the restriction that we can only support either ecdh_ecdsa and ecdh_rsa. Same for auth_dh.

::: lib/ssl/tls13con.c
@@ +473,5 @@
> +     * it again, provided that the current ssl socket has had its server certs
> +     * configured the same as the previous one. */
> +    if (!ssl_FindServerCert(ss, &sid->certType)) {
> +        return PR_FALSE;
> +    }

Does this handle the SNI case where the server may have selected the cert?

@@ +600,5 @@
> +        FATAL_ERROR(ss, PORT_GetError(), internal_error);
> +        goto loser;
> +    }
> +
> +>>>>>>> source

This looks like a messed up merge conflict.

Comment 13

[Kai Engert (:KaiE:)]

This appears to be causing test failures in Firefox, as Franziskus has reported.

I think this needs to be reopened, as it's not completely fixed.

Given that this needs a follow-up fix, and also since Bob has made some r- comments after the patch landed, it seems reasonable to back it out.

Comment 14

[Kai Engert (:KaiE:)]

backed out:
https://hg.mozilla.org/projects/nss/rev/611ef59ebea6
https://hg.mozilla.org/projects/nss/rev/dac2c3e36c78
https://hg.mozilla.org/projects/nss/rev/f3deda60e3b5

Comment 15

[Martin Thomson [:mt:]]

n-i myself, so that I don't miss it.  Bob must have an incomplete patch, the functions that appear to be removed were moved (which isn't a problem for reviewing, since they were re-implemented).

Comment 16

[Robert Relyea]

Thanks martin, I'm pretty sure the patch as attached wasn't what was checked in because we wouldn't have gotten very far;).

Comment 17

[Martin Thomson [:mt:]]

(In reply to Robert Relyea from comment #12)
> 1) There were at least 2 places where it appears the patch won't actually
> compile. I presume if the patch is truly applied, that those areas were
> already fixed.

Yes, my upload was busted.

> 2) The patch as reviewed removed a number of global functions. That will
> cause problems in real servers that are using those functions. New versions
> need to be written which integrates in the new way of doing things so that
> old servers will keep working.

Those should be included, maybe a consequence of the above.

> 3) I see no patches to selfserv. While updating and testing new
> functionality in gtests are fine, existing users look to selfserv to see how
> to use new functionality. I'm sort of at a loss to understand how selfserv
> is still functioning given that we've removed functions that it depends on.

I tweaked selfserv.

> The above 3 things lead me to believe that this isn't the latest patch as
> checked in as our build trees should have failed.
> 
> I have a few other global comments.
> a. We need separate certs for ecdh_ecdsa and ecdh_rsa. We should split those
> auth types.
> b. It appears auth type dh (really dh_dsa and dh_rsa) are missing.
> c. I think we shouldn't restrict the negotiated EC curve based on the ECDSA
> certificate. My main worry is this is a change in behavior between what we
> have been doing and what this new code does. I may be able to be convinced
> otherwise (perhaps the old code just worked by accident).

I don't think that the existing code worked, it just failed when the ECDSA cert had a namedCurve that didn't match (or, more correctly, it just generated a certificate using the wrong curve).

Thus, in terms of backward compatibility, this is a small change that perhaps requires a release note.  If someone signals that they don't support P-256 and we have a P-256 cert, will currently sign anyway (I think).  This causes the handshake to fail.  We could blast the P-256 cert into multiple slots for the old APIs, but I think that would be a bad idea.

> Thanks for doing this patch martin. Is there a newer version available?
> 
> ::: lib/ssl/ssl.h
> @@ +814,5 @@
> > +** already been configured.
> > +*/
> > +SSL_IMPORT SECStatus SSL_ConfigServerCert(
> > +    PRFileDesc *fd, CERTCertificate *cert, SECKEYPrivateKey *key,
> > +    const SSLExtraServerCertData *data, unsigned int data_len);
> 
> My only comment is while we're revamping SSL_ConfigServerCert, it might be
> worth adding a way to handle SNI support at this level. That would require
> adding an optional URL to the function.
> 
> The underlying code to support this may be a bit  more complex, though and
> adding a non-functioning prameter is probably wrong. So I think this is the
> best choice. It fixes a number of existing deficiencies with the current API.

Adding a name is something that I think is a good idea, but something that we can do later.
 
> ::: lib/ssl/ssl3ecc.c
> @@ -47,5 @@
> > -#define SSL_IS_CURVE_NEGOTIATED(curvemsk, curveName) \
> 
> So you are deleteing SSL_IS_CURVE_NEGOTIATED() but you still use it below.

I just moved it to sslimpl.h so that I could use it in ssl3con.c

> @@ +587,5 @@
> > +        return ec_noName;
> > +    }
> > +
> > +    if (ss->sec.serverCert->certType.authType == ssl_auth_ecdsa ||
> > +        ss->sec.serverCert->certType.authType == ssl_auth_ecdh) {
> 
> So there is a difference between ecdh_ecdsa and ecdh_rsa. They will point to
> two different certs. If we are making this change, we should fix the current
> restriction that we can't support both.

Yes, as I note below, I've not addressed that limitation in the current code.  There is a potential for that to be added later.
 
> @@ +594,5 @@
> > +         * negotiated (or supported_curves was absent), double check that. */
> > +        PORT_Assert(SSL_IS_CURVE_NEGOTIATED(ss->ssl3.hs.negotiatedECCurves,
> > +                                            ec_curve));
> > +        if (!SSL_IS_CURVE_NEGOTIATED(ss->ssl3.hs.negotiatedECCurves,
> > +                                     ec_curve)) {
> 
> I'm not sure this is right. We do need to theck the ecdh case, but in the
> ecdsa case, the cure doesn't necessarily have to patch the ECDSA key (at
> least it didn't). If it does then that pretty much restricts our ability to
> negotiate curves on the server side since our ECDSA key will determine the
> one curve we can support. Dropping the authType == ssl_auth_ecdsa should fix
> this.

See above.  I don't think that we were doing the right thing previously.

> ::: lib/ssl/sslcon.c
> @@ +1,2 @@
> >  /*
> > + * Basic SSL handshake functions.
> 
> I think is this all SSL2 code at this point.

Not any more, since SSL2 is gone.
 
> ::: lib/ssl/sslsecur.c
> @@ -626,5 @@
> > -/*
> > -** Return SSLKEAType derived from cert's Public Key algorithm info.
> > -*/
> > -SSLKEAType
> > -NSS_FindCertKEAType(CERTCertificate *cert)

Note that all the removed functions can be found, re-implemented, in sslcert.c

> ::: lib/ssl/sslsnce.c
> @@ +453,5 @@
> > +    to->u.ssl3.certTypeArgs = 0U;
> > +    switch(from->authType) {
> > +#ifndef NSS_DISABLE_ECC
> > +        case ssl_auth_ecdsa:
> > +        case ssl_auth_ecdh:
> 
> I think we only want to do this for ecdh not ecdsa.

See note above.

> ::: lib/ssl/sslsock.c
> >  SECStatus
> > -SSL_SetStapledOCSPResponses(PRFileDesc *fd, const SECItemArray *responses,
> > -                            SSLKEAType kea)
> 
> This function is global. We need to emulate it because this is how existing
> servers work.

You will find this has been emulated in sslcert.c

> ::: lib/ssl/sslt.h
> @@ +100,5 @@
> > +    ssl_auth_ecdh = 5,
> > +    ssl_auth_rsa_sign = 6, /* RSA PKCS#1.5 signing */
> > +    ssl_auth_rsa_pss = 7,
> > +    ssl_auth_psk = 8,
> > +    ssl_auth_size /* number of authentication types */
> 
> Question: where is auth_dh?

We don't support static DH certificates.  If we were to add support, we could add another type at that point.

> Also we need to distinguish (at leas in the code) between auth_ecdh_ecdsa
> and auth_ecdh_rsa. They are two different certs. If we are updating this
> code we should remove the restriction that we can only support either
> ecdh_ecdsa and ecdh_rsa. Same for auth_dh.

Maybe later.  In this respect, I have maintained the current behaviour.  I don't believe that it is worthwhile improving the situation with respect to ECDH cipher suites here.  The structure that I've defined allows for that possibility in the future, should we choose to do that.

> ::: lib/ssl/tls13con.c
> @@ +473,5 @@
> > +     * it again, provided that the current ssl socket has had its server certs
> > +     * configured the same as the previous one. */
> > +    if (!ssl_FindServerCert(ss, &sid->certType)) {
> > +        return PR_FALSE;
> > +    }
> 
> Does this handle the SNI case where the server may have selected the cert?

Yes.  At least as much as the existing code already does.  The selection of a certificate in the SNI hook doesn't really allow for the cipher suite to change.  ekr and I discussed moving the SNI hook higher up in the processing path, even first, but that is a big change.  The SNI handling is large, ugly and brittle and we wanted to make that change separately so that any breakage is easier to fix.

I added a test for this and will include that in the next upload.

Comment 18

[Martin Thomson [:mt:]]

Attachment: bug1237514-4 selfserv.patch

Comment 19

[Martin Thomson [:mt:]]

Attachment: bug1237514-3.patch

Comment 20

[Martin Thomson [:mt:]]

Attachment: bug1237514-2.patch

Comment 21

[Martin Thomson [:mt:]]

Attachment: bug1237514-1.patch

Comment 22

[Eric Rescorla (:ekr)]

MT, can you comment on what these patches do?

Comment 23

[Martin Thomson [:mt:]]

Comment on attachment 8740374 [details] [diff] [review]
bug1237514-4 selfserv.patch

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

This modifies selfserv to use the new API.  It also retains one example of the old API, because changing that piece of code is tricky and having both seemed appropriate.  I've commented the old section.

Comment 24

[Martin Thomson [:mt:]]

Comment on attachment 8740375 [details] [diff] [review]
bug1237514-3.patch

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

This adds a test that verifies that switching out certs in the SNI hook still works.  selfserv already tested that, but this makes it more accessible by putting it in the gtests.

I also cleaned up the certificate handling in the tests to use the scoped pointers Tim built for us.

Comment 25

[Martin Thomson [:mt:]]

Comment on attachment 8740377 [details] [diff] [review]
bug1237514-2.patch

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

This modifies the existing code to allow the |data| argument to the new certificate config function to be properly optional.

Comment 26

[Martin Thomson [:mt:]]

Comment on attachment 8740378 [details] [diff] [review]
bug1237514-1.patch

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

This is the exact code that was in the tree before being backed out.  I have not modified it.

Comment 27

[Martin Thomson [:mt:]]

http://codereview.appspot.com/280660043 contains the squashed patch.  I will squash all these patches before landing, but figured that there wasn't much point in re-reviewing the other stuff (except for Bob, who needs to see the stuff in sslcert.c).

Comment 28

[Robert Relyea]

> I don't think that the existing code worked, it just failed when the ECDSA cert had a namedCurve
> that didn't match (or, more correctly, it just generated a certificate using the wrong curve).

Er do you mean just generated a *KEY* using the wrong curve? My question is what do you mean by the wrong curve, the negotiated curve or the curve used in the certificate.(I thought the code did the latter, if it did the former, then I'm fine with the change. If it just failed, I'm also fine with the change. Even in my case I may still be convinced the change is fine;).

>> The underlying code to support this may be a bit  more complex, though and
>> adding a non-functioning prameter is probably wrong. So I think this is the
>> best choice. It fixes a number of existing deficiencies with the current API.
>> Adding a name is something that I think is a good idea, but something that we can do later.

Agreed.


>> So there is a difference between ecdh_ecdsa and ecdh_rsa. They will point to
>> two different certs. If we are making this change, we should fix the current
>> restriction that we can't support both.
>Yes, as I note below, I've not addressed that limitation in the current code.  There is a potential for that to
> be added later.

Except 1) it should be relatively easy just to make the change now, and 2) after the change ssl_auth_ecdh goes away (replaced by ssl_auth_ecdh_ecdsa, ssl_auth_ecdh_rsa).

> We don't support static DH certificates.  If we were to add support, we could add another type at that point.

We did on the client side.

>> SNI discussion....
> I added a test for this and will include that in the next upload.

I think that should be sufficient.

bob

Summary. I think I only have the following outstanding (though I haven't rereviewed the patch yet).

1) I really think we need to add the ecdh_ecdsa/ecdh_rsa split in this patch.
2) I'm not yet (but almost) convinced the ecdsa semantic change is right.

bob

Comment 29

[Robert Relyea]

OK, actually scratch number 2 above. If we worked by accident, it required a client to support the certificate's curve anyway. If it couldn't it wouldn't work, if it could, then we should be able to negotiate that curve. The only case that would have worked before that fails now is if the client supported the certificate's curve, but didn't advertise that curve in it's curve list, which is pathological anyway.

Comment 30

[Robert Relyea]

Comment on attachment 8740378 [details] [diff] [review]
bug1237514-1.patch

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

I've basically just reviewed sslcert.c

I think I've found an issue that prevents explicit setting of the certificate from the application.

I'd also like to see the ssl_auth_ecdh type split since it'll be a pain to do later and it's relatively easy to do now (particularly now that you have a pretty flexible structure in the code).

::: lib/ssl/sslcert.c
@@ +383,5 @@
> +
> +#ifndef NSS_DISABLE_ECC
> +        case SEC_OID_ANSIX962_EC_PUBLIC_KEY:
> +            /* Yes, this means that you can't do both ECDSA and ECDH, but that is a
> +             * bad practice, and ECDH isn't used that much (if at all). */

I originally thought that we should probably do the same thing for RSA as we did here. (The old code forced using both in the same cert if you wanted to support both ECDH and ECDHE_ECDSA). I thought that certificate server test cases would fail if we didn't allow both here. However I see that the old API still allows for both to be set, so instead I think if both usage bits are present, and the user has given a preference, we should allow the preference (rather than forcing it to ssl_auth_ecdsa). I don't think we need to do what we are doing for RSA in this case, but still allow the application to set a cert it knows can do both in both slots manually.

@@ +387,5 @@
> +             * bad practice, and ECDH isn't used that much (if at all). */
> +            if (cert->keyUsage & KU_DIGITAL_SIGNATURE) {
> +                arg.authType = ssl_auth_ecdsa;
> +            } else if (cert->keyUsage & KU_KEY_AGREEMENT) {
> +                arg.authType = ssl_auth_ecdh;

The only places that would have to change to support ssl_auth_ecdh_xxxx would be here, where the authtype would look at the cert's signature to determine if it was signed by RSA or DSA, any of the switch statements that have ssl_auth_ecdh (it would have both ecdh_ecdsa and ecdh_rsa), and in the ssl3con cipher tables where you specify ssl_auth_xxxx. It should be a relatively simple change now. If you want and release with ssl_auth_ecdh, then we have the ambiguity of what ssl_auth_ecdh means now that it's really split.

@@ +396,5 @@
> +        default:
> +            return SECFailure;
> +    }
> +
> +    /* Setting data->authType explicitly limits the usage. */

Wait I'm not understanding this comment, or the purpose of the check.

If the application specified data->authType Explicitly, this code guarrentees itthe ssl_ConfigCert() will not happen. I don't think that's what is intended, but I'm unclear what the intent is.

I can see that if authType isn't specified, and arg.authType is NULL, We don't want to call ssl_ConfigCert().
I can see that if authType is specified, but doesn't pass the filter that you don't want to call ssl_ConfigCert().

What is happening here is if data->authType is explicitly set, then arg.authType is set to that value by the memcpy above, so arg.authType will always be set to something other than ssl_auth_null.

So some questions:  Are we just trying to preclude auth types that don't match the usage?

In that case you want the test to be:

if (data && data->authType != ssl_auth_null && arg.authType != data->authType) {
    arg.authType = ssl_auth_null;
}
if (arg.authType != ssl_auth_null) {
   rv = ssl_ConfigureCert()
}


In the ecdsa and ecdh and rsa_XXX cases you can use data->authType to select which authType to use.

As coded now you can't use explicit setting of auth type at all. I can see where explicit setting could be useful (to disambiguate between things like rsa_sign and rsa_decrypt or ecdsa and ecdh), but  neither of these cases work now, nor does explicitly setting the authType to the correct value.

Comment 31

[Martin Thomson [:mt:]]

Attachment: bug1237514-5.patch

Bob, this makes the changes you requested with respect to ECDSA and ECDH.  Both can be configured, ECDH certs are further separated by the signature on the EE cert.  BTW, I find this separation baffling, but I will agree that it's necessary based on the definition of the cipher suites.

I'm looking at your other comments now.

Comment 32

[Martin Thomson [:mt:]]

Comment on attachment 8741611 [details] [diff] [review]
bug1237514-5.patch

And I just processed your comments again and realized that I over-rotated on the design.  Let me take another pass.

Comment 33

[Martin Thomson [:mt:]]

I should have read to the end...  This was a good catch.

(In reply to Robert Relyea from comment #30)
> > +    /* Setting data->authType explicitly limits the usage. */
> 
> Wait I'm not understanding this comment, or the purpose of the check.

The point is to allow applications to pick an authType, in the case where they have a certificate that has multiple usages (because CAs tend to do that), but they only want to use one of them.

I've fixed this and it now reads:

    /* Check that we successfully picked an authType */
    if (arg.authType == ssl_auth_null) {
        return SECFailure;
    }
    /* |data->authType| has to either agree or be ssl_auth_null. */
    if (data->authType != ssl_auth_null && data->authType != arg.authType) {
        return SECFailure;
    }
    return ssl_ConfigCert(ss, cert, keyPair, &arg);

Which I think is clearer.  The second check captures my intent: ssl_auth_null lets NSS choose where to put the certificate, which it does based on key usage.  A specific value restricts the configuration to just that type.

BTW, I've done as you asked, if data->authType is ssl_auth_null, we now configure an EC cert as both ECDSA and ECDH.  But I remember that my original intent was to force the application to set data->authType.  I realize now that this wasn't what the code did though.  What do you think?  Configure in both slots automatically, or have the application explicitly set both slots itself?  I think that having EC be consistent with RSA is better on balance, even if it encourages people to do unwise things with their EC certs.

Comment 34

[Martin Thomson [:mt:]]

Attachment: bug1237514.patch

This is the entire patch set.  I'll attach an interdiff from the last state too.

Comment 35

[Martin Thomson [:mt:]]

Attachment: interdiff.patch

Difference between parts 1-4 and the complete diff in attachment 8741634 [details] [diff] [review].

Comment 36

[Martin Thomson [:mt:]]

Comment on attachment 8741635 [details] [diff] [review]
interdiff.patch

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

::: lib/ssl/ssl3ecc.c
@@ +576,5 @@
>  ssl3_GetCurveNameForServerSocket(sslSocket *ss)
>  {
>      ECName ec_curve = ec_noName;
> +    const sslServerCert *cert = ss->sec.serverCert;
> +    int certKeySize = 0;

I will remove this initialization.

@@ +585,5 @@
>          PORT_SetError(SSL_ERROR_NO_CYPHER_OVERLAP);
>          return ec_noName;
>      }
>  
> +    if (cert->certType.authType == ssl_auth_rsa_sign) {

Note that the old code was a little too permissive: it allowed ssl_auth_rsa_decrypt by accident.  This is a little leaner now.

::: lib/ssl/sslcert.h
@@ +18,5 @@
>  ** parameters further narrow the slot.
>  **
> +** An EC key (ssl_auth_ecdh or ssl_auth_ecdsa) is assigned to a slot based on
> +** the named curve of the key.  ECDH suites are further restricted based on the
> +** type of signature on the certificate.

I'll fix this comment.

Comment 37

[Martin Thomson [:mt:]]

On rietveld, with the above changes applied: https://codereview.appspot.com/284710043/

Comment 38

[Robert Relyea]

Comment on attachment 8741634 [details] [diff] [review]
bug1237514.patch

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

r+ rrelyea, comments from https://codereview.appspot.com/284710043/ included below

Looks good now r+ rrelyea. 

I have a few very minor comments. none serious enough to prevent the patch from
being checked in as is.

https://codereview.appspot.com/284710043/diff/150001/lib/pk11wrap/pk11auth.c
File lib/pk11wrap/pk11auth.c (right):

https://codereview.appspot.com/284710043/diff/150001/lib/pk11wrap/pk11auth.c#...
lib/pk11wrap/pk11auth.c:316: }
I'm not sure why this was necessary, But it is a fine change.

bob

https://codereview.appspot.com/284710043/diff/150001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/284710043/diff/150001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:484: 0x80000000L, /* ssl_auth_null */
probably CKM_INVALID_MECHANISM would be better

https://codereview.appspot.com/284710043/diff/150001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:487: 0x80000000L, /* ssl_auth_kea (unused) */
See above

https://codereview.appspot.com/284710043/diff/150001/lib/ssl/sslcert.c
File lib/ssl/sslcert.c (right):

https://codereview.appspot.com/284710043/diff/150001/lib/ssl/sslcert.c#newcod...
lib/ssl/sslcert.c:456: }
Yes, this is much clearer! (and correct now):).

Comment 39

[Robert Relyea]

Comment on attachment 8740374 [details] [diff] [review]
bug1237514-4 selfserv.patch

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

r+ relyea

Comment 40

[Martin Thomson [:mt:]]

Nits from comment 38 fixed.

https://hg.mozilla.org/projects/nss/rev/7385cd821735

Comment 41

[Martin Thomson [:mt:]]

Attachment: bug1237514.patch

For posterity.

Comment 42

[Martin Thomson [:mt:]]

Busted it in a way GCC should really complain about: https://hg.mozilla.org/projects/nss/rev/7bc494e15205

Comment 43

[Martin Thomson [:mt:]]

Attachment: fixtures.patch

I need to fix the test fixtures or this will rot.

Comment 44

[Martin Thomson [:mt:]]

Attachment: fixtures.patch

I realized that the fixtures on my machine and the standard test script have diverged.  This highlighted a problem (see ssl3con.c changes).

Also, I took the opportunity to fix the fixtures.

Comment 45

[Martin Thomson [:mt:]]

Comment on attachment 8744178 [details] [diff] [review]
fixtures.patch

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

I discovered an issue, see sslcert.c, but the main thing that this fixes is the way we manage certificates in the ssl_gtests.

::: lib/ssl/ssl3con.c
@@ -6383,5 @@
>          ssl_MapLowLevelError(SSL_ERROR_CLIENT_KEY_EXCHANGE_FAILURE);
>          goto loser;
>      }
>  
> -#ifndef DISABLE_SSLKEYLOGFILE

These are accidental.  I don't know how these got here given that my local repo doesn't show them.

Comment 46

[Martin Thomson [:mt:]]

Attachment: fixtures.patch

Try again..

Comment 47

[Martin Thomson [:mt:]]

Comment on attachment 8744181 [details] [diff] [review]
fixtures.patch

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

::: lib/ssl/sslcert.c
@@ +139,5 @@
>  SECStatus
>  ssl_OneTimeCertSetup(sslSocket *ss, const sslServerCert *sc)
>  {
>      /* Generate a step-down RSA key. */
> +    if (sc->certType.authType == ssl_auth_rsa_decrypt &&

This was only being called if the cert was ssl_auth_rsa_sign, but ssl3_CreateRSAStepDownKeys() looked for ssl_auth_rsa_decrypt.

This wasn't a problem with the old fixtures, but I added a test that only has the sign keyUsage with the newer test fixtures and it caused the test to highlight this bug.

Comment 48

[Martin Thomson [:mt:]]

Attachment: psk.patch

OK, got the last 1.3 bug

Comment 49

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8744203 [details] [diff] [review]
psk.patch

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

lgtm

Comment 50

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8744181 [details] [diff] [review]
fixtures.patch

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

lgtm

Comment 51

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/452db0f19387
https://hg.mozilla.org/projects/nss/rev/415fe05309e3

Comment 52

[Kai Engert (:KaiE:)]

(In reply to Martin Thomson [:mt:] from comment #40)
> https://hg.mozilla.org/projects/nss/rev/7385cd821735

Reopening, as a mistake was made in this commit, which I found while starting the release notes.

You exported function SSL_ConfigServerCert as version 3.23, but rather a new section for 3.24 must be created.

This needs to block the 3.24 and must be fixed on both NSS trunk and NSS_3_24_BRANCH.

Comment 53

[Kai Engert (:KaiE:)]

Attachment: 1237514-fix-export.patch

Comment 54

[Martin Thomson [:mt:]]

Comment on attachment 8747247 [details] [diff] [review]
1237514-fix-export.patch

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

https://hg.mozilla.org/projects/nss/rev/4c1ab2e7961f - trunk
https://hg.mozilla.org/projects/nss/rev/15a630b3c01a - 3.24 branch