Bug 1335069 - Fall back to SHA-1 signatures if CertReq.supported_sig_algs is empty r=mt
authorTim Taubert <ttaubert@mozilla.com>
Tue, 31 Jan 2017 09:02:04 +0100
changeset 13084 c6132464852505f0336a938bc9af8efaf2bd54cc
parent 13082 c5196cda0d0ccb0fe34f262aada01a26fcf934d9
child 13085 650e5f6cb6172a6a251960496cda7afc53f453ad
push id1980
push userttaubert@mozilla.com
push dateTue, 31 Jan 2017 08:02:43 +0000
reviewersmt
bugs1335069
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
gtests/ssl_gtest/ssl_auth_unittest.cc
gtests/ssl_gtest/tls_parser.h
lib/ssl/ssl3con.c
lib/ssl/ssl3exthandle.c
lib/ssl/tls13con.c
--- a/gtests/ssl_gtest/ssl_auth_unittest.cc
+++ b/gtests/ssl_gtest/ssl_auth_unittest.cc
@@ -131,16 +131,85 @@ TEST_P(TlsConnectTls12, ClientAuthBigRsa
   client_->SetPacketFilter(capture_cert_verify);
   client_->SetupClientAuth();
   server_->RequestClientAuth(true);
   Connect();
   CheckKeys();
   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[] = {
     ssl_sig_ecdsa_secp256r1_sha256};
 static const SSLSignatureScheme SignatureSchemeRsaSha384[] = {
     ssl_sig_rsa_pkcs1_sha384};
 static const SSLSignatureScheme SignatureSchemeRsaSha256[] = {
     ssl_sig_rsa_pkcs1_sha256};
--- a/gtests/ssl_gtest/tls_parser.h
+++ b/gtests/ssl_gtest/tls_parser.h
@@ -25,16 +25,17 @@ const uint8_t kTlsHandshakeType = 22;
 const uint8_t kTlsApplicationDataType = 23;
 
 const uint8_t kTlsHandshakeClientHello = 1;
 const uint8_t kTlsHandshakeServerHello = 2;
 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;
 
 const uint8_t kTlsAlertWarning = 1;
 const uint8_t kTlsAlertFatal = 2;
 
 const uint8_t kTlsAlertUnexpectedMessage = 10;
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -6433,49 +6433,57 @@ ssl_PickSignatureScheme(sslSocket *ss,
             }
         }
     }
 
     PORT_SetError(SSL_ERROR_UNSUPPORTED_SIGNATURE_ALGORITHM);
     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
  * client may advertise its support for signature and hash combinations. */
 static SECStatus
 ssl3_PickServerSignatureScheme(sslSocket *ss)
 {
     sslKeyPair *keyPair = ss->sec.serverCert->serverKeyPair;
     PRBool isTLS12 = ss->version >= SSL_LIBRARY_VERSION_TLS_1_2;
 
     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. */
     return ssl_PickSignatureScheme(ss, keyPair->pubKey, keyPair->privKey,
                                    ss->xtnData.clientSigSchemes,
                                    ss->xtnData.numClientSigScheme,
                                    PR_FALSE /* requireSha1 */);
 }
@@ -6483,19 +6491,29 @@ ssl3_PickServerSignatureScheme(sslSocket
 static SECStatus
 ssl_PickClientSignatureScheme(sslSocket *ss, const SSLSignatureScheme *schemes,
                               unsigned int numSchemes)
 {
     SECKEYPrivateKey *privKey = ss->ssl3.clientPrivateKey;
     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) {
         /* If the key is a 1024-bit RSA or DSA key, assume conservatively that
          * it may be unable to sign SHA-256 hashes. This is the case for older
          * Estonian ID cards that have 1024-bit RSA keys. In FIPS 186-2 and
          * older, DSA key size is at most 1024 bits and the hash function must
          * be SHA-1.
@@ -7399,30 +7417,35 @@ alert_loser:
 SECStatus
 ssl_ParseSignatureSchemes(const sslSocket *ss, PLArenaPool *arena,
                           SSLSignatureScheme **schemesOut,
                           unsigned int *numSchemesOut,
                           unsigned char **b, unsigned int *len)
 {
     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);
 
     if (arena) {
         schemes = PORT_ArenaZNewArray(arena, SSLSignatureScheme, max);
     } else {
         schemes = PORT_ZNewArray(SSLSignatureScheme, max);
     }
@@ -7446,16 +7469,17 @@ ssl_ParseSignatureSchemes(const sslSocke
 
     if (!numSchemes) {
         if (!arena) {
             PORT_Free(schemes);
         }
         schemes = NULL;
     }
 
+done:
     *schemesOut = schemes;
     *numSchemesOut = numSchemes;
     return SECSuccess;
 }
 
 /* Called from ssl3_HandlePostHelloHandshakeMessage() when it has deciphered
  * a complete ssl3 Certificate Request message.
  * Caller must hold Handshake and RecvBuf locks.
--- a/lib/ssl/ssl3exthandle.c
+++ b/lib/ssl/ssl3exthandle.c
@@ -2040,17 +2040,18 @@ ssl3_ServerHandleSigAlgsXtn(const sslSoc
     if (xtnData->clientSigSchemes) {
         PORT_Free(xtnData->clientSigSchemes);
         xtnData->clientSigSchemes = NULL;
     }
     rv = ssl_ParseSignatureSchemes(ss, NULL,
                                    &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;
     }
     /* Check for trailing data. */
     if (data->len != 0) {
         ssl3_ExtSendAlert(ss, alert_fatal, decode_error);
         PORT_SetError(SSL_ERROR_RX_MALFORMED_CLIENT_HELLO);
         return SECFailure;
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -1826,17 +1826,17 @@ tls13_HandleCertificateRequest(sslSocket
         goto loser;
     certRequest->arena = arena;
     certRequest->ca_list.arena = arena;
 
     rv = ssl_ParseSignatureSchemes(ss, arena,
                                    &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;
     }
 
     rv = ssl3_ParseCertificateRequestCAs(ss, &b, &length, arena,
                                          &certRequest->ca_list);
     if (rv != SECSuccess)