1287279 - TLS 1.3: Configure RSA server certs with PKCS#1 v1.5 signatures to be available for RSA-PSS signatures

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1287279-TLS-1.3-Configure-RSA-server-certs-with-.patch

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 8771681 [details] [diff] [review]
0001-Bug-1287279-TLS-1.3-Configure-RSA-server-certs-with-.patch

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

::: lib/ssl/sslcert.c
@@ +427,5 @@
>                          return rv;
>                      }
>                  }
>  
> +                /* Configure RSA certs with PKCS#1 v1.5 signatures to be

I think that I would rather this override here.  That is, you have to set authType in order for this to work.

Comment 3

[Martin Thomson [:mt:]]

I think that this might work:

diff --git a/lib/ssl/sslcert.c b/lib/ssl/sslcert.c
--- a/lib/ssl/sslcert.c
+++ b/lib/ssl/sslcert.c
@@ -423,17 +423,19 @@ ssl_ConfigCertByUsage(sslSocket *ss, CER
                      * Configure both slots only if no explicit slot was set. */
                     arg.authType = ssl_auth_rsa_decrypt;
                     rv = ssl_ConfigCert(ss, cert, keyPair, &arg);
                     if (rv != SECSuccess) {
                         return rv;
                     }
                 }
 
-                arg.authType = ssl_auth_rsa_sign;
+                if (arg.authType != ssl_auth_rsa_pss) {
+                    arg.authType = ssl_auth_rsa_sign;
+                }
             } else if (cert->keyUsage & KU_KEY_ENCIPHERMENT) {
                 arg.authType = ssl_auth_rsa_decrypt;
             }
             break;
 
         case SEC_OID_PKCS1_RSA_PSS_SIGNATURE:
             if (cert->keyUsage & KU_DIGITAL_SIGNATURE) {
                 arg.authType = ssl_auth_rsa_pss;

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1287279-TLS-1.3-Configure-RSA-server-certs-with-.patch, v2

I believe that the right fix would be this, a combination of both our patches.

When ssl_ConfigCertByUsage() is called with ssl_auth_rsa_sign we'll fill the ssl_auth_rsa_sign slot.

When ssl_ConfigCertByUsage() is called with ssl_auth_rsa_pss we'll fill the ssl_auth_rsa_pss slot.

When ssl_ConfigCertByUsage() is called with ssl_auth_null we'll fill both the ssl_auth_rsa_sign and the ssl_auth_rsa_pss slot.

At least IIUIC that's the desired behavior.

Comment 5

[Martin Thomson [:mt:]]

Comment on attachment 8771973 [details] [diff] [review]
0001-Bug-1287279-TLS-1.3-Configure-RSA-server-certs-with-.patch, v2

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

Let's try to think about how you might want to test this change.

I also think that you have a much neater change that might be made here.

Comment 6

[Martin Thomson [:mt:]]

Attachment: Simpler fix

Does this work better?

Comment 7

[Martin Thomson [:mt:]]

Whoops, assume that the first _decrypt is _pss instead.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Martin Thomson [:mt:] from comment #5)
> Let's try to think about how you might want to test this change.

I think the easiest would be to test this together with the RSA-PSS signing patch. Then we can check that servers configured with either a _sign or _pss cert work with only PSS signatures enabled. We could land these together or I take this change here and move it to the other bug?

(In reply to Martin Thomson [:mt:] from comment #6)
> Does this work better?

This looks better, but without ...

> if (arg.authType != ssl_auth_rsa_pss) {
>     arg.authType = ssl_auth_rsa_sign;
> }

... we fail when we're called with sign_auth_rsa_pss and a cert with a PKCS#1 v1.5 signature. I thought that in this case we'd want to fill the _pss slot with the given cert and succeed?

Comment 9

[Martin Thomson [:mt:]]

(In reply to Tim Taubert [:ttaubert] from comment #8)
> (In reply to Martin Thomson [:mt:] from comment #5)
> > Let's try to think about how you might want to test this change.
> 
> I think the easiest would be to test this together with the RSA-PSS signing
> patch. Then we can check that servers configured with either a _sign or _pss
> cert work with only PSS signatures enabled. We could land these together or
> I take this change here and move it to the other bug?

Yes, let's make this part of the larger patch.  I prefer to land tests for all new code where possible.

> ... we fail when we're called with sign_auth_rsa_pss and a cert with a
> PKCS#1 v1.5 signature. I thought that in this case we'd want to fill the
> _pss slot with the given cert and succeed?

Oops, I was hacking on top of another patch and lost it.  You are right, of course.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1287279-TLS-1.3-Configure-RSA-server-certs-with-.patch, v3

(In reply to Martin Thomson [:mt:] from comment #9)
> Yes, let's make this part of the larger patch.  I prefer to land tests for
> all new code where possible.

Ok, so how about this? I'll merge it with the other bug if this looks good.

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 8772326 [details] [diff] [review]
0001-Bug-1287279-TLS-1.3-Configure-RSA-server-certs-with-.patch, v3

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

Yeah, this is good.  Next step is tests :)

::: lib/ssl/sslcert.c
@@ +438,3 @@
>                  }
>  
> +                /* Succeed when the slot is PSS but we only have an RSA cert. */

/* This falls through only if the slot is explicitly PSS.  Otherwise, pick the RSA signing slot. */

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Martin, here's the same patch with tests:

https://codereview.appspot.com/302410043

I decided against landing this with bug 1280439 to keep patches smaller and easier to review.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=4c8aefdcec7c4db73533dcacc9c690a05747d483

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/71987d6f797f