Bug 1564727 - RSS PSS SPKI for delegated credentials, r=kjacobs

Differential Revision: https://phabricator.services.mozilla.com/D37521

3 files changed, 273 insertions, 16 deletions

diff --git a/cpputil/nss_scoped_ptrs.h b/cpputil/nss_scoped_ptrs.h
index 9b1fbb47f..24116b63f 100644
--- a/cpputil/nss_scoped_ptrs.h
+++ b/cpputil/nss_scoped_ptrs.h
@@ -90,7 +90,8 @@ SCOPED(SECMODModule);
 
 struct StackSECItem : public SECItem {
   StackSECItem() : SECItem({siBuffer, nullptr, 0}) {}
-  ~StackSECItem() { SECITEM_FreeItem(this, PR_FALSE); }
+  ~StackSECItem() { Reset(); }
+  void Reset() { SECITEM_FreeItem(this, PR_FALSE); }
 };
 
 #endif  // nss_scoped_ptrs_h__
diff --git a/gtests/ssl_gtest/tls_subcerts_unittest.cc b/gtests/ssl_gtest/tls_subcerts_unittest.cc
index 8c0cb5bdb..75e01018b 100644
--- a/gtests/ssl_gtest/tls_subcerts_unittest.cc
+++ b/gtests/ssl_gtest/tls_subcerts_unittest.cc
@@ -6,6 +6,7 @@
 
 #include <ctime>
 
+#include "prtime.h"
 #include "secerr.h"
 #include "ssl.h"
 
@@ -58,7 +59,9 @@ TEST_P(TlsConnectTls13, DCNotConfigured) {
 TEST_P(TlsConnectTls13, DCConnectEcdsaP256) {
   Reset(kDelegatorId);
   client_->EnableDelegatedCredentials();
-  server_->AddDelegatedCredential(kDCId, kDCScheme, kDCValidFor, now());
+  server_->AddDelegatedCredential(TlsAgent::kServerEcdsa256,
+                                  ssl_sig_ecdsa_secp256r1_sha256, kDCValidFor,
+                                  now());
 
   auto cfilter = MakeTlsFilter<TlsExtensionCapture>(
       client_, ssl_delegated_credentials_xtn);
@@ -66,6 +69,7 @@ TEST_P(TlsConnectTls13, DCConnectEcdsaP256) {
 
   EXPECT_TRUE(cfilter->captured());
   CheckPeerDelegCred(client_, true, 256);
+  EXPECT_EQ(ssl_sig_ecdsa_secp256r1_sha256, client_->info().signatureScheme);
 }
 
 // Connected with ECDSA-P521.
@@ -83,10 +87,11 @@ TEST_P(TlsConnectTls13, DCConnectEcdsaP521) {
 
   EXPECT_TRUE(cfilter->captured());
   CheckPeerDelegCred(client_, true, 521);
+  EXPECT_EQ(ssl_sig_ecdsa_secp521r1_sha512, client_->info().signatureScheme);
 }
 
-// Connected with RSA-PSS.
-TEST_P(TlsConnectTls13, DCConnectRsaPss) {
+// Connected with RSA-PSS, using an RSAE SPKI.
+TEST_P(TlsConnectTls13, DCConnectRsaPssRsae) {
   Reset(kDelegatorId);
   client_->EnableDelegatedCredentials();
   server_->AddDelegatedCredential(
@@ -98,6 +103,30 @@ TEST_P(TlsConnectTls13, DCConnectRsaPss) {
 
   EXPECT_TRUE(cfilter->captured());
   CheckPeerDelegCred(client_, true, 1024);
+  EXPECT_EQ(ssl_sig_rsa_pss_rsae_sha256, client_->info().signatureScheme);
+}
+
+// Connected with RSA-PSS, using a PSS SPKI.
+TEST_P(TlsConnectTls13, DCConnectRsaPssPss) {
+  Reset(kDelegatorId);
+
+  // Need to enable PSS-PSS, which is not on by default.
+  static const SSLSignatureScheme kSchemes[] = {ssl_sig_ecdsa_secp256r1_sha256,
+                                                ssl_sig_rsa_pss_pss_sha256};
+  client_->SetSignatureSchemes(kSchemes, PR_ARRAY_SIZE(kSchemes));
+  server_->SetSignatureSchemes(kSchemes, PR_ARRAY_SIZE(kSchemes));
+
+  client_->EnableDelegatedCredentials();
+  server_->AddDelegatedCredential(
+      TlsAgent::kServerRsaPss, ssl_sig_rsa_pss_pss_sha256, kDCValidFor, now());
+
+  auto cfilter = MakeTlsFilter<TlsExtensionCapture>(
+      client_, ssl_delegated_credentials_xtn);
+  Connect();
+
+  EXPECT_TRUE(cfilter->captured());
+  CheckPeerDelegCred(client_, true, 1024);
+  EXPECT_EQ(ssl_sig_rsa_pss_pss_sha256, client_->info().signatureScheme);
 }
 
 // Generate a weak key.  We can't do this in the fixture because certutil
@@ -133,7 +162,7 @@ static void GenerateWeakRsaKey(ScopedSECKEYPrivateKey& priv,
     ASSERT_EQ(
         SECSuccess,
         PK11_RandomUpdate(
-            const_cast<void*>(reinterpret_cast<const void*>(&FRESH_ENTROPY)),
+            const_cast<void*>(reinterpret_cast<const void*>(FRESH_ENTROPY)),
             sizeof(FRESH_ENTROPY)));
     break;
   }
@@ -169,18 +198,37 @@ TEST_P(TlsConnectTls13, DCWeakKey) {
   ConnectExpectAlert(client_, kTlsAlertInsufficientSecurity);
 }
 
+class ReplaceDCSigScheme : public TlsHandshakeFilter {
+ public:
+  ReplaceDCSigScheme(const std::shared_ptr<TlsAgent>& a)
+      : TlsHandshakeFilter(a, {ssl_hs_certificate_verify}) {}
+
+ protected:
+  PacketFilter::Action FilterHandshake(const HandshakeHeader& header,
+                                       const DataBuffer& input,
+                                       DataBuffer* output) override {
+    *output = input;
+    output->Write(0, ssl_sig_ecdsa_secp384r1_sha384, 2);
+    return CHANGE;
+  }
+};
+
 // Aborted because of incorrect DC signature algorithm indication.
 TEST_P(TlsConnectTls13, DCAbortBadExpectedCertVerifyAlg) {
   Reset(kDelegatorId);
   client_->EnableDelegatedCredentials();
   server_->AddDelegatedCredential(TlsAgent::kServerEcdsa256,
-                                  ssl_sig_ecdsa_secp521r1_sha512, kDCValidFor,
+                                  ssl_sig_ecdsa_secp256r1_sha256, kDCValidFor,
                                   now());
+  auto filter = MakeTlsFilter<ReplaceDCSigScheme>(server_);
+  filter->EnableDecryption();
   ConnectExpectAlert(client_, kTlsAlertIllegalParameter);
+  client_->CheckErrorCode(SSL_ERROR_DC_CERT_VERIFY_ALG_MISMATCH);
+  server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT);
 }
 
 // Aborted because of invalid DC signature.
-TEST_P(TlsConnectTls13, DCABortBadSignature) {
+TEST_P(TlsConnectTls13, DCAbortBadSignature) {
   Reset(kDelegatorId);
   EnsureTlsSetup();
   client_->EnableDelegatedCredentials();
@@ -201,6 +249,8 @@ TEST_P(TlsConnectTls13, DCABortBadSignature) {
   EXPECT_TRUE(server_->ConfigServerCert(kDelegatorId, true, &extra_data));
 
   ConnectExpectAlert(client_, kTlsAlertIllegalParameter);
+  client_->CheckErrorCode(SSL_ERROR_DC_BAD_SIGNATURE);
+  server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT);
 }
 
 // Aborted because of expired DC.
@@ -212,6 +262,8 @@ TEST_P(TlsConnectTls13, DCAbortExpired) {
   // DC expired.
   AdvanceTime((static_cast<PRTime>(kDCValidFor) + 1) * PR_USEC_PER_SEC);
   ConnectExpectAlert(client_, kTlsAlertIllegalParameter);
+  client_->CheckErrorCode(SSL_ERROR_DC_EXPIRED);
+  server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT);
 }
 
 // Aborted because of invalid key usage.
@@ -312,4 +364,61 @@ TEST_P(TlsConnectTls13, DCConnectExpectedCertVerifyAlgNotSupported) {
   CheckPeerDelegCred(client_, false);
 }
 
+class DCDelegation : public ::testing::Test {};
+
+TEST_F(DCDelegation, DCDelegations) {
+  PRTime now = PR_Now();
+  ScopedCERTCertificate cert;
+  ScopedSECKEYPrivateKey priv;
+  ASSERT_TRUE(TlsAgent::LoadCertificate(kDelegatorId, &cert, &priv));
+
+  ScopedSECKEYPublicKey pub_rsa;
+  ScopedSECKEYPrivateKey priv_rsa;
+  ASSERT_TRUE(
+      TlsAgent::LoadKeyPairFromCert(TlsAgent::kServerRsa, &pub_rsa, &priv_rsa));
+
+  StackSECItem dc;
+  EXPECT_EQ(SECFailure,
+            SSL_DelegateCredential(cert.get(), priv.get(), pub_rsa.get(),
+                                   ssl_sig_ecdsa_secp256r1_sha256, kDCValidFor,
+                                   now, &dc));
+  EXPECT_EQ(SSL_ERROR_INCORRECT_SIGNATURE_ALGORITHM, PORT_GetError());
+
+  // Using different PSS hashes should be OK.
+  EXPECT_EQ(SECSuccess,
+            SSL_DelegateCredential(cert.get(), priv.get(), pub_rsa.get(),
+                                   ssl_sig_rsa_pss_rsae_sha256, kDCValidFor,
+                                   now, &dc));
+  // Make sure to reset |dc| after each success.
+  dc.Reset();
+  EXPECT_EQ(SECSuccess, SSL_DelegateCredential(
+                            cert.get(), priv.get(), pub_rsa.get(),
+                            ssl_sig_rsa_pss_pss_sha256, kDCValidFor, now, &dc));
+  dc.Reset();
+  EXPECT_EQ(SECSuccess, SSL_DelegateCredential(
+                            cert.get(), priv.get(), pub_rsa.get(),
+                            ssl_sig_rsa_pss_pss_sha384, kDCValidFor, now, &dc));
+  dc.Reset();
+
+  ScopedSECKEYPublicKey pub_ecdsa;
+  ScopedSECKEYPrivateKey priv_ecdsa;
+  ASSERT_TRUE(TlsAgent::LoadKeyPairFromCert(TlsAgent::kServerEcdsa256,
+                                            &pub_ecdsa, &priv_ecdsa));
+
+  EXPECT_EQ(SECFailure,
+            SSL_DelegateCredential(cert.get(), priv.get(), pub_ecdsa.get(),
+                                   ssl_sig_rsa_pss_rsae_sha256, kDCValidFor,
+                                   now, &dc));
+  EXPECT_EQ(SSL_ERROR_INCORRECT_SIGNATURE_ALGORITHM, PORT_GetError());
+  EXPECT_EQ(SECFailure, SSL_DelegateCredential(
+                            cert.get(), priv.get(), pub_ecdsa.get(),
+                            ssl_sig_rsa_pss_pss_sha256, kDCValidFor, now, &dc));
+  EXPECT_EQ(SSL_ERROR_INCORRECT_SIGNATURE_ALGORITHM, PORT_GetError());
+  EXPECT_EQ(SECFailure,
+            SSL_DelegateCredential(cert.get(), priv.get(), pub_ecdsa.get(),
+                                   ssl_sig_ecdsa_secp384r1_sha384, kDCValidFor,
+                                   now, &dc));
+  EXPECT_EQ(SSL_ERROR_INCORRECT_SIGNATURE_ALGORITHM, PORT_GetError());
+}
+
 }  // namespace nss_test
diff --git a/lib/ssl/tls13subcerts.c b/lib/ssl/tls13subcerts.c
index 206d93490..43d31b516 100644
--- a/lib/ssl/tls13subcerts.c
+++ b/lib/ssl/tls13subcerts.c
@@ -493,13 +493,155 @@ tls13_VerifyDelegatedCredential(sslSocket *ss,
     return rv;
 }
 
+static CERTSubjectPublicKeyInfo *
+tls13_MakePssSpki(const SECKEYPublicKey *pub, SECOidTag hashOid)
+{
+    SECStatus rv;
+    PLArenaPool *arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
+    if (!arena) {
+        goto loser; /* Code already set. */
+    }
+    CERTSubjectPublicKeyInfo *spki = PORT_ArenaZNew(arena, CERTSubjectPublicKeyInfo);
+    if (!spki) {
+        goto loser; /* Code already set. */
+    }
+    spki->arena = arena;
+
+    SECKEYRSAPSSParams params = { 0 };
+    params.hashAlg = PORT_ArenaZNew(arena, SECAlgorithmID);
+    rv = SECOID_SetAlgorithmID(arena, params.hashAlg, hashOid, NULL);
+    if (rv != SECSuccess) {
+        goto loser; /* Code already set. */
+    }
+
+    /* Set the mask hash algorithm too, which is an argument to
+     * a SEC_OID_PKCS1_MGF1 value. */
+    SECAlgorithmID maskHashAlg;
+    memset(&maskHashAlg, 0, sizeof(maskHashAlg));
+    rv = SECOID_SetAlgorithmID(arena, &maskHashAlg, hashOid, NULL);
+    if (rv != SECSuccess) {
+        goto loser; /* Code already set. */
+    }
+    SECItem *maskHashAlgItem =
+        SEC_ASN1EncodeItem(arena, NULL, &maskHashAlg,
+                           SEC_ASN1_GET(SECOID_AlgorithmIDTemplate));
+    if (!maskHashAlgItem) {
+        /* Probably OOM, but not certain. */
+        PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
+        goto loser;
+    }
+
+    params.maskAlg = PORT_ArenaZNew(arena, SECAlgorithmID);
+    rv = SECOID_SetAlgorithmID(arena, params.maskAlg, SEC_OID_PKCS1_MGF1,
+                               maskHashAlgItem);
+    if (rv != SECSuccess) {
+        goto loser; /* Code already set. */
+    }
+
+    SECItem *algorithmItem =
+        SEC_ASN1EncodeItem(arena, NULL, &params,
+                           SEC_ASN1_GET(SECKEY_RSAPSSParamsTemplate));
+    if (!algorithmItem) {
+        PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
+        goto loser; /* Code already set. */
+    }
+    rv = SECOID_SetAlgorithmID(arena, &spki->algorithm,
+                               SEC_OID_PKCS1_RSA_PSS_SIGNATURE, algorithmItem);
+    if (rv != SECSuccess) {
+        goto loser; /* Code already set. */
+    }
+
+    PORT_Assert(pub->u.rsa.modulus.type == siUnsignedInteger);
+    PORT_Assert(pub->u.rsa.publicExponent.type == siUnsignedInteger);
+    SECItem *pubItem = SEC_ASN1EncodeItem(arena, &spki->subjectPublicKey, pub,
+                                          SEC_ASN1_GET(SECKEY_RSAPublicKeyTemplate));
+    if (!pubItem) {
+        PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
+        goto loser;
+    }
+    spki->subjectPublicKey.len *= 8; /* Key length is in bits. */
+    return spki;
+
+loser:
+    PORT_FreeArena(arena, PR_FALSE);
+    return NULL;
+}
+
+static CERTSubjectPublicKeyInfo *
+tls13_MakeDcSpki(const SECKEYPublicKey *dcPub, SSLSignatureScheme dcCertVerifyAlg)
+{
+    switch (SECKEY_GetPublicKeyType(dcPub)) {
+        case rsaKey: {
+            SECOidTag hashOid;
+            switch (dcCertVerifyAlg) {
+                /* Though we might prefer to use a pure PSS SPKI here, we can't
+                 * because we have to choose based on client preferences. And
+                 * not all clients advertise the pss_pss schemes.  So use the
+                 * default SPKI construction for an RSAE SPKI. */
+                case ssl_sig_rsa_pss_rsae_sha256:
+                case ssl_sig_rsa_pss_rsae_sha384:
+                case ssl_sig_rsa_pss_rsae_sha512:
+                    return SECKEY_CreateSubjectPublicKeyInfo(dcPub);
+
+                case ssl_sig_rsa_pss_pss_sha256:
+                    hashOid = SEC_OID_SHA256;
+                    break;
+                case ssl_sig_rsa_pss_pss_sha384:
+                    hashOid = SEC_OID_SHA384;
+                    break;
+                case ssl_sig_rsa_pss_pss_sha512:
+                    hashOid = SEC_OID_SHA512;
+                    break;
+
+                default:
+                    PORT_SetError(SSL_ERROR_INCORRECT_SIGNATURE_ALGORITHM);
+                    return NULL;
+            }
+            return tls13_MakePssSpki(dcPub, hashOid);
+        }
+
+        case ecKey: {
+            const sslNamedGroupDef *group = ssl_ECPubKey2NamedGroup(dcPub);
+            if (!group) {
+                PORT_SetError(SSL_ERROR_INCORRECT_SIGNATURE_ALGORITHM);
+                return NULL;
+            }
+            SSLSignatureScheme keyScheme;
+            switch (group->name) {
+                case ssl_grp_ec_secp256r1:
+                    keyScheme = ssl_sig_ecdsa_secp256r1_sha256;
+                    break;
+                case ssl_grp_ec_secp384r1:
+                    keyScheme = ssl_sig_ecdsa_secp384r1_sha384;
+                    break;
+                case ssl_grp_ec_secp521r1:
+                    keyScheme = ssl_sig_ecdsa_secp521r1_sha512;
+                    break;
+                default:
+                    PORT_SetError(SEC_ERROR_INVALID_KEY);
+                    return NULL;
+            }
+            if (keyScheme != dcCertVerifyAlg) {
+                PORT_SetError(SSL_ERROR_INCORRECT_SIGNATURE_ALGORITHM);
+                return NULL;
+            }
+            return SECKEY_CreateSubjectPublicKeyInfo(dcPub);
+        }
+
+        default:
+            break;
+    }
+
+    PORT_SetError(SEC_ERROR_INVALID_KEY);
+    return NULL;
+}
+
 /* Returns a serialized DC with the given parameters.
  *
  * Note that this function is meant primarily for testing. In particular, it
  * DOES NOT verify any of the following:
  *  - |certPriv| is the private key corresponding to |cert|;
  *  - that |checkCertKeyUsage(cert) == SECSuccess|;
- *  - |dcCertVerifyAlg| is consistent with |dcPub|;
  *  - |dcValidFor| is less than 7 days (the maximum permitted by the spec); or
  *  - validTime doesn't overflow a PRUint32.
  *
@@ -518,7 +660,7 @@ SSLExp_DelegateCredential(const CERTCertificate *cert,
 {
     SECStatus rv;
     SSL3Hashes hash;
-    SECItem *tmp = NULL;
+    CERTSubjectPublicKeyInfo *spki = NULL;
     SECKEYPrivateKey *tmpPriv = NULL;
     sslDelegatedCredential *dc = NULL;
     sslBuffer dcBuf = SSL_BUFFER_EMPTY;
@@ -536,17 +678,20 @@ SSLExp_DelegateCredential(const CERTCertificate *cert,
         goto loser;
     }
     dc->validTime = ((now - start) / PR_USEC_PER_SEC) + dcValidFor;
+
+    /* Building the SPKI also validates |dcCertVerifyAlg|. */
+    spki = tls13_MakeDcSpki(dcPub, dcCertVerifyAlg);
+    if (!spki) {
+        goto loser;
+    }
     dc->expectedCertVerifyAlg = dcCertVerifyAlg;
 
-    tmp = SECKEY_EncodeDERSubjectPublicKeyInfo(dcPub);
-    if (!tmp) {
+    SECItem *spkiDer =
+        SEC_ASN1EncodeItem(NULL /*arena*/, &dc->derSpki, spki,
+                           SEC_ASN1_GET(CERT_SubjectPublicKeyInfoTemplate));
+    if (!spkiDer) {
         goto loser;
     }
-    /* Transfer |tmp| into |dc->derSpki|. */
-    dc->derSpki.type = tmp->type;
-    dc->derSpki.data = tmp->data;
-    dc->derSpki.len = tmp->len;
-    PORT_Free(tmp);
 
     rv = ssl_SignatureSchemeFromSpki(&cert->subjectPublicKeyInfo,
                                      PR_TRUE /* isTls13 */, &dc->alg);
@@ -590,12 +735,14 @@ SSLExp_DelegateCredential(const CERTCertificate *cert,
         goto loser;
     }
 
+    SECKEY_DestroySubjectPublicKeyInfo(spki);
     SECKEY_DestroyPrivateKey(tmpPriv);
     tls13_DestroyDelegatedCredential(dc);
     sslBuffer_Clear(&dcBuf);
     return SECSuccess;
 
 loser:
+    SECKEY_DestroySubjectPublicKeyInfo(spki);
     SECKEY_DestroyPrivateKey(tmpPriv);
     tls13_DestroyDelegatedCredential(dc);
     sslBuffer_Clear(&dcBuf);