Bug 1563078 - Set authKeyBits for delegated credentials, r=jcj

The delegated credentials patch left the channel info unmodified, which meant
that it reported the key strength of the end entity certificate and not the
delegated credential.  For a using application, this is problematic because it
can't access information about delegated credentials.  In this case, the only
omission was the strength of the key.

Firefox checks key strength for the entire certificate chain according to its
policies, but it also wants to apply the same sort of policy to the delegated
credential.  In particular, it wants to ensure that an RSA credential (which
shouldn't be used, but whatever...) has a long enough modulus, because the NSS
policy is less strict than the Firefox one.

To address this use case, SSLChannelInfo.authKeyBits is set to the length of the
delegated credential key when delegated credentials are in use.  This is
consistent with the definition of the parameter, but implies a different
understanding of its meaning when delegated credentials are enabled.

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

--HG--
extra : rebase_source : f0da859a0c947fc816a98984607687f8fddf8e0d

cpputil/tls_parser.h

@@ -47,6 +47,7 @@ const uint8_t kTlsAlertIllegalParameter = 47;
 const uint8_t kTlsAlertDecodeError = 50;
 const uint8_t kTlsAlertDecryptError = 51;
 const uint8_t kTlsAlertProtocolVersion = 70;
+const uint8_t kTlsAlertInsufficientSecurity = 71;
 const uint8_t kTlsAlertInternalError = 80;
 const uint8_t kTlsAlertInappropriateFallback = 86;
 const uint8_t kTlsAlertMissingExtension = 109;

gtests/ssl_gtest/tls_agent.cc

@@ -161,10 +161,10 @@ void TlsAgent::DelegateCredential(const std::string& name,
                                   PRUint32 dc_valid_for, PRTime now,
                                   SECItem* dc) {
   ScopedCERTCertificate cert;
-  ScopedSECKEYPrivateKey certPriv;
-  EXPECT_TRUE(TlsAgent::LoadCertificate(name, &cert, &certPriv));
+  ScopedSECKEYPrivateKey cert_priv;
+  EXPECT_TRUE(TlsAgent::LoadCertificate(name, &cert, &cert_priv));
   EXPECT_EQ(SECSuccess,
-            SSL_DelegateCredential(cert.get(), certPriv.get(), dc_pub.get(),
+            SSL_DelegateCredential(cert.get(), cert_priv.get(), dc_pub.get(),
                                    dc_cert_verify_alg, dc_valid_for, now, dc));
 }
 
@@ -372,10 +372,6 @@ void TlsAgent::CheckCipherSuite(uint16_t suite) {
   EXPECT_EQ(csinfo_.cipherSuite, suite);
 }
 
-void TlsAgent::CheckPeerDelegCred(bool expected) {
-  EXPECT_EQ(expected, info_.peerDelegCred);
-}
-
 void TlsAgent::RequestClientAuth(bool requireAuth) {
   ASSERT_EQ(SERVER, role_);
 
@@ -787,26 +783,26 @@ void TlsAgent::WaitForErrorCode(int32_t expected, uint32_t delay) const {
 }
 
 void TlsAgent::CheckPreliminaryInfo() {
-  SSLPreliminaryChannelInfo info;
+  SSLPreliminaryChannelInfo preinfo;
   EXPECT_EQ(SECSuccess,
-            SSL_GetPreliminaryChannelInfo(ssl_fd(), &info, sizeof(info)));
-  EXPECT_EQ(sizeof(info), info.length);
-  EXPECT_TRUE(info.valuesSet & ssl_preinfo_version);
-  EXPECT_TRUE(info.valuesSet & ssl_preinfo_cipher_suite);
+            SSL_GetPreliminaryChannelInfo(ssl_fd(), &preinfo, sizeof(preinfo)));
+  EXPECT_EQ(sizeof(preinfo), preinfo.length);
+  EXPECT_TRUE(preinfo.valuesSet & ssl_preinfo_version);
+  EXPECT_TRUE(preinfo.valuesSet & ssl_preinfo_cipher_suite);
 
   // A version of 0 is invalid and indicates no expectation.  This value is
   // initialized to 0 so that tests that don't explicitly set an expected
   // version can negotiate a version.
   if (!expected_version_) {
-    expected_version_ = info.protocolVersion;
+    expected_version_ = preinfo.protocolVersion;
   }
-  EXPECT_EQ(expected_version_, info.protocolVersion);
+  EXPECT_EQ(expected_version_, preinfo.protocolVersion);
 
   // As with the version; 0 is the null cipher suite (and also invalid).
   if (!expected_cipher_suite_) {
-    expected_cipher_suite_ = info.cipherSuite;
+    expected_cipher_suite_ = preinfo.cipherSuite;
   }
-  EXPECT_EQ(expected_cipher_suite_, info.cipherSuite);
+  EXPECT_EQ(expected_cipher_suite_, preinfo.cipherSuite);
 }
 
 // Check that all the expected callbacks have been called.

gtests/ssl_gtest/tls_agent.h

@@ -184,7 +184,6 @@ class TlsAgent : public PollTarget {
   void DisableECDHEServerKeyReuse();
   bool GetPeerChainLength(size_t* count);
   void CheckCipherSuite(uint16_t cipher_suite);
-  void CheckPeerDelegCred(bool expected);
   void SetResumptionTokenCallback();
   bool MaybeSetResumptionToken();
   void SetResumptionToken(const std::vector<uint8_t>& resumption_token) {
@@ -224,16 +223,17 @@ class TlsAgent : public PollTarget {
   PRFileDesc* ssl_fd() const { return ssl_fd_.get(); }
   std::shared_ptr<DummyPrSocket>& adapter() { return adapter_; }
 
+  const SSLChannelInfo& info() const {
+    EXPECT_EQ(STATE_CONNECTED, state_);
+    return info_;
+  }
   bool is_compressed() const {
-    return info_.compressionMethod != ssl_compression_null;
+    return info().compressionMethod != ssl_compression_null;
   }
   uint16_t server_key_bits() const { return server_key_bits_; }
   uint16_t min_version() const { return vrange_.min; }
   uint16_t max_version() const { return vrange_.max; }
-  uint16_t version() const {
-    EXPECT_EQ(STATE_CONNECTED, state_);
-    return info_.protocolVersion;
-  }
+  uint16_t version() const { return info().protocolVersion; }
 
   bool cipher_suite(uint16_t* suite) const {
     if (state_ != STATE_CONNECTED) return false;

gtests/ssl_gtest/tls_subcerts_unittest.cc

@@ -20,6 +20,14 @@ const std::string kDCId = TlsAgent::kServerEcdsa256;
 const SSLSignatureScheme kDCScheme = ssl_sig_ecdsa_secp256r1_sha256;
 const PRUint32 kDCValidFor = 60 * 60 * 24 * 7 /* 1 week (seconds */;
 
+static void CheckPeerDelegCred(const std::shared_ptr<TlsAgent>& client,
+                               bool expected, PRUint32 key_bits = 0) {
+  EXPECT_EQ(expected, client->info().peerDelegCred);
+  if (expected) {
+    EXPECT_EQ(key_bits, client->info().authKeyBits);
+  }
+}
+
 // Attempt to configure a DC when either the DC or DC private key is missing.
 TEST_P(TlsConnectTls13, DCNotConfigured) {
   // Load and delegate the credential.
@@ -57,7 +65,7 @@ TEST_P(TlsConnectTls13, DCConnectEcdsaP256) {
   Connect();
 
   EXPECT_TRUE(cfilter->captured());
-  client_->CheckPeerDelegCred(true);
+  CheckPeerDelegCred(client_, true, 256);
 }
 
 // Connected with ECDSA-P521.
@@ -74,7 +82,7 @@ TEST_P(TlsConnectTls13, DCConnectEcdsaP521) {
   Connect();
 
   EXPECT_TRUE(cfilter->captured());
-  client_->CheckPeerDelegCred(true);
+  CheckPeerDelegCred(client_, true, 521);
 }
 
 // Connected with RSA-PSS.
@@ -89,7 +97,76 @@ TEST_P(TlsConnectTls13, DCConnectRsaPss) {
   Connect();
 
   EXPECT_TRUE(cfilter->captured());
-  client_->CheckPeerDelegCred(true);
+  CheckPeerDelegCred(client_, true, 1024);
+}
+
+// Generate a weak key.  We can't do this in the fixture because certutil
+// won't sign with such a tiny key.  That's OK, because this is fast(ish).
+static void GenerateWeakRsaKey(ScopedSECKEYPrivateKey& priv,
+                               ScopedSECKEYPublicKey& pub) {
+  ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
+  ASSERT_TRUE(slot);
+  PK11RSAGenParams rsaparams;
+  // The absolute minimum size of RSA key that we can use with SHA-256 is
+  // 256bit (hash) + 256bit (salt) + 8 (start byte) + 8 (end byte) = 528.
+  rsaparams.keySizeInBits = 528;
+  rsaparams.pe = 65537;
+
+  // Bug 1012786: PK11_GenerateKeyPair can fail if there is insufficient
+  // entropy to generate a random key. We can fake some.
+  for (int retry = 0; retry < 10; ++retry) {
+    SECKEYPublicKey* p_pub = nullptr;
+    priv.reset(PK11_GenerateKeyPair(slot.get(), CKM_RSA_PKCS_KEY_PAIR_GEN,
+                                    &rsaparams, &p_pub, false, false, nullptr));
+    pub.reset(p_pub);
+    if (priv) {
+      return;
+    }
+
+    ASSERT_FALSE(pub);
+    if (PORT_GetError() != SEC_ERROR_PKCS11_FUNCTION_FAILED) {
+      break;
+    }
+
+    // https://xkcd.com/221/
+    static const uint8_t FRESH_ENTROPY[16] = {4};
+    ASSERT_EQ(
+        SECSuccess,
+        PK11_RandomUpdate(
+            const_cast<void*>(reinterpret_cast<const void*>(&FRESH_ENTROPY)),
+            sizeof(FRESH_ENTROPY)));
+    break;
+  }
+  ADD_FAILURE() << "Unable to generate an RSA key: "
+                << PORT_ErrorToName(PORT_GetError());
+}
+
+// Fail to connect with a weak RSA key.
+TEST_P(TlsConnectTls13, DCWeakKey) {
+  Reset(kDelegatorId);
+  EnsureTlsSetup();
+
+  ScopedSECKEYPrivateKey dc_priv;
+  ScopedSECKEYPublicKey dc_pub;
+  GenerateWeakRsaKey(dc_priv, dc_pub);
+  ASSERT_TRUE(dc_priv);
+
+  // Construct a DC.
+  StackSECItem dc;
+  TlsAgent::DelegateCredential(kDelegatorId, dc_pub,
+                               ssl_sig_rsa_pss_rsae_sha256, kDCValidFor, now(),
+                               &dc);
+
+  // Configure the DC on the server.
+  SSLExtraServerCertData extra_data = {ssl_auth_null, nullptr, nullptr,
+                                       nullptr,       &dc,     dc_priv.get()};
+  EXPECT_TRUE(server_->ConfigServerCert(kDelegatorId, true, &extra_data));
+
+  client_->EnableDelegatedCredentials();
+
+  auto cfilter = MakeTlsFilter<TlsExtensionCapture>(
+      client_, ssl_delegated_credentials_xtn);
+  ConnectExpectAlert(client_, kTlsAlertInsufficientSecurity);
 }
 
 // Aborted because of incorrect DC signature algorithm indication.
@@ -156,7 +233,7 @@ TEST_P(TlsConnectTls13, DCConnectNoClientSupport) {
   Connect();
 
   EXPECT_FALSE(cfilter->captured());
-  client_->CheckPeerDelegCred(false);
+  CheckPeerDelegCred(client_, false);
 }
 
 // Connected without DC because of no server DC.
@@ -169,7 +246,7 @@ TEST_P(TlsConnectTls13, DCConnectNoServerSupport) {
   Connect();
 
   EXPECT_TRUE(cfilter->captured());
-  client_->CheckPeerDelegCred(false);
+  CheckPeerDelegCred(client_, false);
 }
 
 // Connected without DC because client doesn't support TLS 1.3.
@@ -189,7 +266,7 @@ TEST_P(TlsConnectTls13, DCConnectClientNoTls13) {
 
   // Should fallback to TLS 1.2 and not negotiate a DC.
   EXPECT_FALSE(cfilter->captured());
-  client_->CheckPeerDelegCred(false);
+  CheckPeerDelegCred(client_, false);
 }
 
 // Connected without DC because server doesn't support TLS 1.3.
@@ -210,7 +287,7 @@ TEST_P(TlsConnectTls13, DCConnectServerNoTls13) {
   // Should fallback to TLS 1.2 and not negotiate a DC. The client will still
   // send the indication because it supports 1.3.
   EXPECT_TRUE(cfilter->captured());
-  client_->CheckPeerDelegCred(false);
+  CheckPeerDelegCred(client_, false);
 }
 
 // Connected without DC because client doesn't support the signature scheme.
@@ -232,7 +309,7 @@ TEST_P(TlsConnectTls13, DCConnectExpectedCertVerifyAlgNotSupported) {
 
   // Client sends indication, but the server doesn't send a DC.
   EXPECT_TRUE(cfilter->captured());
-  client_->CheckPeerDelegCred(false);
+  CheckPeerDelegCred(client_, false);
 }
 
 }  // namespace nss_test