Bug 1414931, send correct alert on inconsistent signature scheme, r=mt
authorDaiki Ueno <dueno@redhat.com>
Wed, 15 Aug 2018 10:25:34 +0200
changeset 14443 6349fa699c3bc482bf34f34b47fb68ca93980565
parent 14442 d26e112957a9e82586e5d03b4018f319972a6fec
child 14444 81d0cfe0c88f8fecaadb05c6295cc1d482a8b682
push id3160
push userdueno@redhat.com
push dateWed, 15 Aug 2018 08:27:25 +0000
reviewersmt
bugs1414931
Bug 1414931, send correct alert on inconsistent signature scheme, r=mt Summary: This fixes the corner cases where incorrect alert is sent (or even no alert is sent): - when the client's CertificateVerify is signed with rsa_pss_pss_*, while the certificate is RSA - when the client's CertificateVerify is signed with rsa_pss_rsae_*, while the certificate is RSA-PSS - when ServerKeyExchange is signed with an inconsistent signature scheme with the server certificate Reviewers: mt Reviewed By: mt Bug #: 1414931 Differential Revision: https://phabricator.services.mozilla.com/D3321
gtests/ssl_gtest/ssl_auth_unittest.cc
gtests/ssl_gtest/ssl_dhe_unittest.cc
lib/ssl/ssl3con.c
--- a/gtests/ssl_gtest/ssl_auth_unittest.cc
+++ b/gtests/ssl_gtest/ssl_auth_unittest.cc
@@ -192,16 +192,91 @@ TEST_P(TlsConnectTls12, ClientAuthBigRsa
   client_->SetupClientAuth();
   server_->RequestClientAuth(true);
   Connect();
   CheckKeys();
   CheckSigScheme(capture_cert_verify, 0, server_, ssl_sig_rsa_pss_rsae_sha256,
                  2048);
 }
 
+// Replaces the signature scheme in a CertificateVerify message.
+class TlsReplaceSignatureSchemeFilter : public TlsHandshakeFilter {
+ public:
+  TlsReplaceSignatureSchemeFilter(const std::shared_ptr<TlsAgent>& a,
+                                  SSLSignatureScheme scheme)
+      : TlsHandshakeFilter(a, {kTlsHandshakeCertificateVerify}),
+        scheme_(scheme) {
+    EnableDecryption();
+  }
+
+ protected:
+  virtual PacketFilter::Action FilterHandshake(const HandshakeHeader& header,
+                                               const DataBuffer& input,
+                                               DataBuffer* output) {
+    *output = input;
+    output->Write(0, scheme_, 2);
+    return CHANGE;
+  }
+
+ private:
+  SSLSignatureScheme scheme_;
+};
+
+// Check if CertificateVerify signed with rsa_pss_rsae_* is properly
+// rejected when the certificate is RSA-PSS.
+//
+// This only works under TLS 1.2, because PSS doesn't work with TLS
+// 1.0 or TLS 1.1 and the TLS 1.3 1-RTT handshake is partially
+// successful at the client side.
+TEST_P(TlsConnectTls12, ClientAuthInconsistentRsaeSignatureScheme) {
+  static const SSLSignatureScheme kSignatureSchemePss[] = {
+      ssl_sig_rsa_pss_pss_sha256, ssl_sig_rsa_pss_rsae_sha256};
+
+  Reset(TlsAgent::kServerRsa, "rsa_pss");
+  client_->SetSignatureSchemes(kSignatureSchemePss,
+                               PR_ARRAY_SIZE(kSignatureSchemePss));
+  server_->SetSignatureSchemes(kSignatureSchemePss,
+                               PR_ARRAY_SIZE(kSignatureSchemePss));
+  client_->SetupClientAuth();
+  server_->RequestClientAuth(true);
+
+  EnsureTlsSetup();
+
+  MakeTlsFilter<TlsReplaceSignatureSchemeFilter>(client_,
+                                                 ssl_sig_rsa_pss_rsae_sha256);
+
+  ConnectExpectAlert(server_, kTlsAlertIllegalParameter);
+}
+
+// Check if CertificateVerify signed with rsa_pss_pss_* is properly
+// rejected when the certificate is RSA.
+//
+// This only works under TLS 1.2, because PSS doesn't work with TLS
+// 1.0 or TLS 1.1 and the TLS 1.3 1-RTT handshake is partially
+// successful at the client side.
+TEST_P(TlsConnectTls12, ClientAuthInconsistentPssSignatureScheme) {
+  static const SSLSignatureScheme kSignatureSchemePss[] = {
+      ssl_sig_rsa_pss_rsae_sha256, ssl_sig_rsa_pss_pss_sha256};
+
+  Reset(TlsAgent::kServerRsa, "rsa");
+  client_->SetSignatureSchemes(kSignatureSchemePss,
+                               PR_ARRAY_SIZE(kSignatureSchemePss));
+  server_->SetSignatureSchemes(kSignatureSchemePss,
+                               PR_ARRAY_SIZE(kSignatureSchemePss));
+  client_->SetupClientAuth();
+  server_->RequestClientAuth(true);
+
+  EnsureTlsSetup();
+
+  MakeTlsFilter<TlsReplaceSignatureSchemeFilter>(client_,
+                                                 ssl_sig_rsa_pss_pss_sha256);
+
+  ConnectExpectAlert(server_, kTlsAlertIllegalParameter);
+}
+
 class TlsZeroCertificateRequestSigAlgsFilter : public TlsHandshakeFilter {
  public:
   TlsZeroCertificateRequestSigAlgsFilter(const std::shared_ptr<TlsAgent>& a)
       : TlsHandshakeFilter(a, {kTlsHandshakeCertificateRequest}) {}
   virtual PacketFilter::Action FilterHandshake(
       const TlsHandshakeFilter::HandshakeHeader& header,
       const DataBuffer& input, DataBuffer* output) {
     TlsParser parser(input);
@@ -405,39 +480,16 @@ TEST_P(TlsConnectTls13, SignatureAlgorit
 // only fails when the Finished is checked.
 TEST_P(TlsConnectTls12, SignatureAlgorithmDrop) {
   MakeTlsFilter<TlsExtensionDropper>(client_, ssl_signature_algorithms_xtn);
   ConnectExpectAlert(server_, kTlsAlertDecryptError);
   client_->CheckErrorCode(SSL_ERROR_DECRYPT_ERROR_ALERT);
   server_->CheckErrorCode(SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE);
 }
 
-// Replaces the signature scheme in a TLS 1.3 CertificateVerify message.
-class TlsReplaceSignatureSchemeFilter : public TlsHandshakeFilter {
- public:
-  TlsReplaceSignatureSchemeFilter(const std::shared_ptr<TlsAgent>& a,
-                                  SSLSignatureScheme scheme)
-      : TlsHandshakeFilter(a, {kTlsHandshakeCertificateVerify}),
-        scheme_(scheme) {
-    EnableDecryption();
-  }
-
- protected:
-  virtual PacketFilter::Action FilterHandshake(const HandshakeHeader& header,
-                                               const DataBuffer& input,
-                                               DataBuffer* output) {
-    *output = input;
-    output->Write(0, scheme_, 2);
-    return CHANGE;
-  }
-
- private:
-  SSLSignatureScheme scheme_;
-};
-
 TEST_P(TlsConnectTls13, UnsupportedSignatureSchemeAlert) {
   EnsureTlsSetup();
   MakeTlsFilter<TlsReplaceSignatureSchemeFilter>(server_, ssl_sig_none);
 
   ConnectExpectAlert(client_, kTlsAlertIllegalParameter);
   server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT);
   client_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_CERT_VERIFY);
 }
--- a/gtests/ssl_gtest/ssl_dhe_unittest.cc
+++ b/gtests/ssl_gtest/ssl_dhe_unittest.cc
@@ -638,9 +638,48 @@ TEST_P(TlsConnectGenericPre13, InvalidDE
 
   MakeTlsFilter<TlsDheSkeChangeSignature>(server_, version_, kBogusDheSignature,
                                           sizeof(kBogusDheSignature));
 
   ConnectExpectAlert(client_, kTlsAlertDecryptError);
   client_->CheckErrorCode(SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE);
 }
 
+// Replace SignatureAndHashAlgorithm of a SKE.
+class DHEServerKEXSigAlgReplacer : public TlsHandshakeFilter {
+ public:
+  DHEServerKEXSigAlgReplacer(const std::shared_ptr<TlsAgent>& server,
+                             SSLSignatureScheme sig_scheme)
+      : TlsHandshakeFilter(server, {kTlsHandshakeServerKeyExchange}),
+        sig_scheme_(sig_scheme) {}
+
+ protected:
+  virtual PacketFilter::Action FilterHandshake(const HandshakeHeader& header,
+                                               const DataBuffer& input,
+                                               DataBuffer* output) {
+    *output = input;
+
+    uint32_t len;
+    uint32_t idx = 0;
+    EXPECT_TRUE(output->Read(idx, 2, &len));
+    idx += 2 + len;
+    EXPECT_TRUE(output->Read(idx, 2, &len));
+    idx += 2 + len;
+    EXPECT_TRUE(output->Read(idx, 2, &len));
+    idx += 2 + len;
+    output->Write(idx, sig_scheme_, 2);
+
+    return CHANGE;
+  }
+
+ private:
+  SSLSignatureScheme sig_scheme_;
+};
+
+TEST_P(TlsConnectTls12, ConnectInconsistentSigAlgDHE) {
+  EnableOnlyDheCiphers();
+
+  MakeTlsFilter<DHEServerKEXSigAlgReplacer>(server_,
+                                            ssl_sig_ecdsa_secp256r1_sha256);
+  ConnectExpectAlert(client_, kTlsAlertIllegalParameter);
+}
+
 }  // namespace nss_test
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -6956,22 +6956,22 @@ ssl_HandleDHServerKeyExchange(sslSocket 
     if (!ssl_IsValidDHEShare(&dh_p, &dh_Ys)) {
         errCode = SSL_ERROR_RX_MALFORMED_DHE_KEY_SHARE;
         goto alert_loser;
     }
 
     if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_2) {
         rv = ssl_ConsumeSignatureScheme(ss, &b, &length, &sigScheme);
         if (rv != SECSuccess) {
-            goto loser; /* malformed or unsupported. */
+            goto alert_loser; /* malformed or unsupported. */
         }
         rv = ssl_CheckSignatureSchemeConsistency(ss, sigScheme,
                                                  ss->sec.peerCert);
         if (rv != SECSuccess) {
-            goto loser;
+            goto alert_loser;
         }
         hashAlg = ssl_SignatureSchemeToHashType(sigScheme);
     } else {
         /* Use ssl_hash_none to represent the MD5+SHA1 combo. */
         hashAlg = ssl_hash_none;
         sigScheme = ssl_sig_none;
     }
     rv = ssl3_ConsumeHandshakeVariable(ss, &signature, 2, &b, &length);
@@ -9567,17 +9567,17 @@ ssl3_HandleCertificateVerify(sslSocket *
         rv = ssl_ConsumeSignatureScheme(ss, &b, &length, &sigScheme);
         if (rv != SECSuccess) {
             goto loser; /* malformed or unsupported. */
         }
         rv = ssl_CheckSignatureSchemeConsistency(ss, sigScheme,
                                                  ss->sec.peerCert);
         if (rv != SECSuccess) {
             errCode = PORT_GetError();
-            desc = decrypt_error;
+            desc = illegal_parameter;
             goto alert_loser;
         }
 
         rv = ssl3_ComputeHandshakeHash(ss->ssl3.hs.messages.buf,
                                        ss->ssl3.hs.messages.len,
                                        ssl_SignatureSchemeToHashType(sigScheme),
                                        &hashes);
     } else {