Bug 1335069 - Fall back to SHA-1 signatures if CertReq.supported_sig_…

…algs is empty r=mt

Differential Revision: https://nss-review.dev.mozaws.net/D184

--HG--
extra : amend_source : ae84f7e3014d18248cda04045041b219a5b718f9

gtests/ssl_gtest/ssl_auth_unittest.cc

@@ -136,6 +136,75 @@ TEST_P(TlsConnectTls12, ClientAuthBigRsaCheckSigAlg) {
   CheckSigScheme(capture_cert_verify, 0, server_, ssl_sig_rsa_pss_sha256, 2048);
 }
 
+class TlsZeroCertificateRequestSigAlgsFilter : public TlsHandshakeFilter {
+ public:
+  virtual PacketFilter::Action FilterHandshake(
+      const TlsHandshakeFilter::HandshakeHeader& header,
+      const DataBuffer& input, DataBuffer* output) {
+    if (header.handshake_type() != kTlsHandshakeCertificateRequest) {
+      return KEEP;
+    }
+
+    TlsParser parser(input);
+    std::cerr << "Zeroing CertReq.supported_signature_algorithms" << std::endl;
+
+    DataBuffer cert_types;
+    if (!parser.ReadVariable(&cert_types, 1)) {
+      ADD_FAILURE();
+      return KEEP;
+    }
+
+    if (!parser.SkipVariable(2)) {
+      ADD_FAILURE();
+      return KEEP;
+    }
+
+    DataBuffer cas;
+    if (!parser.ReadVariable(&cas, 2)) {
+      ADD_FAILURE();
+      return KEEP;
+    }
+
+    size_t idx = 0;
+
+    // Write certificate types.
+    idx = output->Write(idx, cert_types.len(), 1);
+    idx = output->Write(idx, cert_types);
+
+    // Write zero signature algorithms.
+    idx = output->Write(idx, 0U, 2);
+
+    // Write certificate authorities.
+    idx = output->Write(idx, cas.len(), 2);
+    idx = output->Write(idx, cas);
+
+    return CHANGE;
+  }
+};
+
+// Check that we fall back to SHA-1 when the server doesn't provide any
+// supported_signature_algorithms in the CertificateRequest message.
+TEST_P(TlsConnectTls12, ClientAuthNoSigAlgsFallback) {
+  EnsureTlsSetup();
+  auto filter = new TlsZeroCertificateRequestSigAlgsFilter();
+  server_->SetPacketFilter(filter);
+  auto capture_cert_verify =
+      new TlsInspectorRecordHandshakeMessage(kTlsHandshakeCertificateVerify);
+  client_->SetPacketFilter(capture_cert_verify);
+  client_->SetupClientAuth();
+  server_->RequestClientAuth(true);
+
+  ConnectExpectFail();
+
+  // We're expecting a bad signature here because we tampered with a handshake
+  // message (CertReq). Previously, without the SHA-1 fallback, we would've
+  // seen a malformed record alert.
+  server_->CheckErrorCode(SEC_ERROR_BAD_SIGNATURE);
+  client_->CheckErrorCode(SSL_ERROR_DECRYPT_ERROR_ALERT);
+
+  CheckSigScheme(capture_cert_verify, 0, server_, ssl_sig_rsa_pkcs1_sha1, 1024);
+}
+
 static const SSLSignatureScheme SignatureSchemeEcdsaSha384[] = {
     ssl_sig_ecdsa_secp384r1_sha384};
 static const SSLSignatureScheme SignatureSchemeEcdsaSha256[] = {

gtests/ssl_gtest/tls_parser.h

@@ -30,6 +30,7 @@ const uint8_t kTlsHandshakeHelloRetryRequest = 6;
 const uint8_t kTlsHandshakeEncryptedExtensions = 8;
 const uint8_t kTlsHandshakeCertificate = 11;
 const uint8_t kTlsHandshakeServerKeyExchange = 12;
+const uint8_t kTlsHandshakeCertificateRequest = 13;
 const uint8_t kTlsHandshakeCertificateVerify = 15;
 const uint8_t kTlsHandshakeClientKeyExchange = 16;
 const uint8_t kTlsHandshakeFinished = 20;

lib/ssl/ssl3con.c

@@ -6438,6 +6438,33 @@ ssl_PickSignatureScheme(sslSocket *ss,
     return SECFailure;
 }
 
+static SECStatus
+ssl_PickFallbackSignatureScheme(sslSocket *ss, SECKEYPublicKey *pubKey)
+{
+    PRBool isTLS12 = ss->version >= SSL_LIBRARY_VERSION_TLS_1_2;
+
+    switch (SECKEY_GetPublicKeyType(pubKey)) {
+        case rsaKey:
+            if (isTLS12) {
+                ss->ssl3.hs.signatureScheme = ssl_sig_rsa_pkcs1_sha1;
+            } else {
+                ss->ssl3.hs.signatureScheme = ssl_sig_rsa_pkcs1_sha1md5;
+            }
+            break;
+        case ecKey:
+            ss->ssl3.hs.signatureScheme = ssl_sig_ecdsa_sha1;
+            break;
+        case dsaKey:
+            ss->ssl3.hs.signatureScheme = ssl_sig_dsa_sha1;
+            break;
+        default:
+            PORT_Assert(0);
+            PORT_SetError(SEC_ERROR_INVALID_KEY);
+            return SECFailure;
+    }
+    return SECSuccess;
+}
+
 /* ssl3_PickServerSignatureScheme selects a signature scheme for signing the
  * handshake.  Most of this is determined by the key pair we are using.
  * Prior to TLS 1.2, the MD5/SHA1 combination is always used. With TLS 1.2, a
@@ -6451,26 +6478,7 @@ ssl3_PickServerSignatureScheme(sslSocket *ss)
     if (!isTLS12 || !ssl3_ExtensionNegotiated(ss, ssl_signature_algorithms_xtn)) {
         /* If the client didn't provide any signature_algorithms extension then
          * we can assume that they support SHA-1: RFC5246, Section 7.4.1.4.1. */
-        switch (SECKEY_GetPublicKeyType(keyPair->pubKey)) {
-            case rsaKey:
-                if (isTLS12) {
-                    ss->ssl3.hs.signatureScheme = ssl_sig_rsa_pkcs1_sha1;
-                } else {
-                    ss->ssl3.hs.signatureScheme = ssl_sig_rsa_pkcs1_sha1md5;
-                }
-                break;
-            case ecKey:
-                ss->ssl3.hs.signatureScheme = ssl_sig_ecdsa_sha1;
-                break;
-            case dsaKey:
-                ss->ssl3.hs.signatureScheme = ssl_sig_dsa_sha1;
-                break;
-            default:
-                PORT_Assert(0);
-                PORT_SetError(SEC_ERROR_INVALID_KEY);
-                return SECFailure;
-        }
-        return SECSuccess;
+        return ssl_PickFallbackSignatureScheme(ss, keyPair->pubKey);
     }
 
     /* Sets error code, if needed. */
@@ -6488,9 +6496,19 @@ ssl_PickClientSignatureScheme(sslSocket *ss, const SSLSignatureScheme *schemes,
     SECKEYPublicKey *pubKey;
     SECStatus rv;
 
+    PRBool isTLS13 = (PRBool)ss->version >= SSL_LIBRARY_VERSION_TLS_1_3;
     pubKey = CERT_ExtractPublicKey(ss->ssl3.clientCertificate);
     PORT_Assert(pubKey);
-    if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3 &&
+
+    if (!isTLS13 && numSchemes == 0) {
+        /* If the server didn't provide any signature algorithms
+         * then let's assume they support SHA-1. */
+        return ssl_PickFallbackSignatureScheme(ss, pubKey);
+    }
+
+    PORT_Assert(schemes && numSchemes > 0);
+
+    if (!isTLS13 &&
         (SECKEY_GetPublicKeyType(pubKey) == rsaKey ||
          SECKEY_GetPublicKeyType(pubKey) == dsaKey) &&
         SECKEY_PublicKeyStrengthInBits(pubKey) <= 1024) {
@@ -7404,20 +7422,25 @@ ssl_ParseSignatureSchemes(const sslSocket *ss, PLArenaPool *arena,
 {
     SECStatus rv;
     SECItem buf;
-    SSLSignatureScheme *schemes;
+    SSLSignatureScheme *schemes = NULL;
     unsigned int numSchemes = 0;
     unsigned int max;
 
     rv = ssl3_ExtConsumeHandshakeVariable(ss, &buf, 2, b, len);
     if (rv != SECSuccess) {
         return SECFailure;
     }
-    /* An empty or odd-length value is invalid. */
-    if (buf.len == 0 || (buf.len & 1) != 0) {
+    /* An odd-length value is invalid. */
+    if ((buf.len & 1) != 0) {
         ssl3_ExtSendAlert(ss, alert_fatal, decode_error);
         return SECFailure;
     }
 
+    /* Let the caller decide whether to alert here. */
+    if (buf.len == 0) {
+        goto done;
+    }
+
     /* Limit the number of schemes we read. */
     max = PR_MIN(buf.len / 2, MAX_SIGNATURE_SCHEMES);
 
@@ -7451,6 +7474,7 @@ ssl_ParseSignatureSchemes(const sslSocket *ss, PLArenaPool *arena,
         schemes = NULL;
     }
 
+done:
     *schemesOut = schemes;
     *numSchemesOut = numSchemes;
     return SECSuccess;

lib/ssl/ssl3exthandle.c

@@ -2045,7 +2045,8 @@ ssl3_ServerHandleSigAlgsXtn(const sslSocket *ss, TLSExtensionData *xtnData, PRUi
                                    &xtnData->clientSigSchemes,
                                    &xtnData->numClientSigScheme,
                                    &data->data, &data->len);
-    if (rv != SECSuccess) {
+    if (rv != SECSuccess || xtnData->numClientSigScheme == 0) {
+        ssl3_ExtSendAlert(ss, alert_fatal, decode_error);
         PORT_SetError(SSL_ERROR_RX_MALFORMED_CLIENT_HELLO);
         return SECFailure;
     }

lib/ssl/tls13con.c

@@ -1831,7 +1831,7 @@ tls13_HandleCertificateRequest(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
                                    &certRequest->signatureSchemes,
                                    &certRequest->signatureSchemeCount,
                                    &b, &length);
-    if (rv != SECSuccess) {
+    if (rv != SECSuccess || certRequest->signatureSchemeCount == 0) {
         FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_CERT_REQUEST,
                     decode_error);
         goto loser;