Merge pull request #10279 from julek-wolfssl/zd/21661

zd/21661: harden X.509 chain validation, session ticket identity binding, and peer cert restore

Author: dgarske

examples/client/client.c

@@ -155,7 +155,7 @@ static int quieter = 0; /* Print fewer messages. This is helpful with overly
 #ifdef HAVE_SESSION_TICKET
 
 #ifndef SESSION_TICKET_LEN
-#define SESSION_TICKET_LEN 256
+#define SESSION_TICKET_LEN 2048
 #endif
     static int sessionTicketCB(WOLFSSL* ssl,
                         const unsigned char* ticket, int ticketSz,

src/internal.c

@@ -38542,6 +38542,11 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
                 if((ret=ALPN_Select(ssl)))
                     goto out;
     #endif
+    #if defined(HAVE_SESSION_TICKET) && \
+        (defined(HAVE_SNI) || defined(HAVE_ALPN))
+                if((ret=VerifyTicketBinding(ssl)))
+                    goto out;
+    #endif
 
                 i += totalExtSz;
 #else
@@ -39323,6 +39328,77 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
         return ret;
     }
 
+#ifdef HAVE_SNI
+    /* Hash server-selected SNI; zeros dst when none. */
+    static int TicketSniHash(WOLFSSL* ssl, byte* dst)
+    {
+        char* name = NULL;
+        word16 nameLen;
+
+        nameLen = TLSX_SNI_GetRequest(ssl->extensions,
+                                      WOLFSSL_SNI_HOST_NAME,
+                                      (void**)&name, 0);
+        if (name != NULL && nameLen > 0) {
+            return wc_Hash(TICKET_BINDING_HASH_TYPE, (const byte*)name,
+                           nameLen, dst, TICKET_BINDING_HASH_SZ);
+        }
+
+        XMEMSET(dst, 0, TICKET_BINDING_HASH_SZ);
+        return 0;
+    }
+#endif
+
+#ifdef HAVE_ALPN
+    /* Hash negotiated ALPN; zeros dst when none. */
+    static int TicketAlpnHash(WOLFSSL* ssl, byte* dst)
+    {
+        char* proto = NULL;
+        word16 protoLen = 0;
+
+        if (TLSX_ALPN_GetRequest(ssl->extensions, (void**)&proto,
+                                 &protoLen) == WOLFSSL_SUCCESS &&
+                proto != NULL && protoLen > 0) {
+            return wc_Hash(TICKET_BINDING_HASH_TYPE, (const byte*)proto,
+                           protoLen, dst, TICKET_BINDING_HASH_SZ);
+        }
+
+        XMEMSET(dst, 0, TICKET_BINDING_HASH_SZ);
+        return 0;
+    }
+#endif
+
+#if defined(HAVE_SNI) || defined(HAVE_ALPN)
+    /* Server-side: verify the SNI/ALPN bindings carried on a resumed
+     * session match what was negotiated for the current connection.
+     * Must be called after extension parsing and ALPN_Select.
+     * Returns 0 on match, WOLFSSL_FATAL_ERROR on mismatch. */
+    int VerifyTicketBinding(WOLFSSL* ssl)
+    {
+        byte curHash[TICKET_BINDING_HASH_SZ];
+
+        if (!ssl->options.resuming || !ssl->options.useTicket)
+            return 0;
+
+#ifdef HAVE_SNI
+        if (TicketSniHash(ssl, curHash) != 0 ||
+                XMEMCMP(curHash, ssl->session->sniHash,
+                        TICKET_BINDING_HASH_SZ) != 0) {
+            WOLFSSL_MSG("Ticket SNI mismatch");
+            return WOLFSSL_FATAL_ERROR;
+        }
+#endif
+#ifdef HAVE_ALPN
+        if (TicketAlpnHash(ssl, curHash) != 0 ||
+                XMEMCMP(curHash, ssl->session->alpnHash,
+                        TICKET_BINDING_HASH_SZ) != 0) {
+            WOLFSSL_MSG("Ticket ALPN mismatch");
+            return WOLFSSL_FATAL_ERROR;
+        }
+#endif
+        return 0;
+    }
+#endif
+
     /* create a new session ticket, 0 on success
      * Do any kind of setup in SetupTicket */
     int CreateTicket(WOLFSSL* ssl)
@@ -39421,6 +39497,18 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
         it->sessionCtxSz = ssl->sessionCtxSz;
         XMEMCPY(it->sessionCtx, ssl->sessionCtx, ID_LEN);
 #endif
+#ifdef HAVE_SNI
+        ret = TicketSniHash(ssl, it->sniHash);
+        if (ret != 0)
+            goto error;
+        XMEMCPY(ssl->session->sniHash, it->sniHash, TICKET_BINDING_HASH_SZ);
+#endif
+#ifdef HAVE_ALPN
+        ret = TicketAlpnHash(ssl, it->alpnHash);
+        if (ret != 0)
+            goto error;
+        XMEMCPY(ssl->session->alpnHash, it->alpnHash, TICKET_BINDING_HASH_SZ);
+#endif
 
 #if defined(OPENSSL_ALL) && defined(KEEP_PEER_CERT) && \
     !defined(NO_CERT_IN_TICKET)
@@ -39776,6 +39864,8 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
                         ssl->sessionCtxSz) != 0))
             return WOLFSSL_FATAL_ERROR;
 #endif
+        /* SNI/ALPN binding is verified after ALPN_Select via
+         * VerifyTicketBinding(). */
         return 0;
     }
 #endif /* WOLFSSL_SLT13 */
@@ -39787,36 +39877,54 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
         word16 peerCertLen = 0;
         ato16(it->peerCertLen, &peerCertLen);
 
-        if (peerCertLen > 0 && peerCertLen <= MAX_TICKET_PEER_CERT_SZ) {
+        /* Clear any peer cert state that may have been copied from the session
+         * cache by wolfSSL_DupSession before we got here. */
+        FreeX509(&ssl->peerCert);
+        InitX509(&ssl->peerCert, 0, ssl->heap);
 #ifdef SESSION_CERTS
-            /* Clear existing chain and add the peer certificate */
-            ssl->session->chain.count = 0;
-            AddSessionCertToChain(&ssl->session->chain,
-                                  it->peerCert, peerCertLen);
+        ssl->session->chain.count = 0;
 #endif
-            /* Also decode into ssl->peerCert for direct access */
-            {
-                int ret;
-                DecodedCert* dCert;
-
-                dCert = (DecodedCert*)XMALLOC(sizeof(DecodedCert), ssl->heap,
-                                               DYNAMIC_TYPE_DCERT);
-                if (dCert != NULL) {
-                    InitDecodedCert(dCert, it->peerCert, peerCertLen, ssl->heap);
-                    ret = ParseCertRelative(dCert, CERT_TYPE, 0, NULL, NULL);
-                    if (ret == 0) {
+
+        if (peerCertLen > 0 && peerCertLen <= MAX_TICKET_PEER_CERT_SZ) {
+            int ret;
+            DecodedCert* dCert;
+
+            dCert = (DecodedCert*)XMALLOC(sizeof(DecodedCert), ssl->heap,
+                                           DYNAMIC_TYPE_DCERT);
+            if (dCert != NULL) {
+                int verify = ssl->options.verifyPeer ? VERIFY : NO_VERIFY;
+                InitDecodedCert(dCert, it->peerCert, peerCertLen, ssl->heap);
+                /* Re-verify against the current trust store so that CA
+                 * removal since ticket issue is enforced. */
+                ret = ParseCertRelative(dCert, CERT_TYPE, verify,
+                                        SSL_CM(ssl), NULL);
+            #ifdef HAVE_OCSP
+                /* ParseCertRelative does not check revocation status.
+                 * Run OCSP if the CertManager has it enabled. */
+                if (ret == 0 && SSL_CM(ssl)->ocspEnabled) {
+                    ret = CheckCertOCSP_ex(SSL_CM(ssl)->ocsp, dCert, ssl);
+                }
+            #endif
+            #ifdef HAVE_CRL
+                if (ret == 0 && SSL_CM(ssl)->crlEnabled) {
+                    ret = CheckCertCRL(SSL_CM(ssl)->crl, dCert);
+                }
+            #endif
+                if (ret == 0) {
+            #ifdef SESSION_CERTS
+                    AddSessionCertToChain(&ssl->session->chain,
+                                          it->peerCert, peerCertLen);
+            #endif
+                    FreeX509(&ssl->peerCert);
+                    InitX509(&ssl->peerCert, 0, ssl->heap);
+                    ret = CopyDecodedToX509(&ssl->peerCert, dCert);
+                    if (ret != 0) {
                         FreeX509(&ssl->peerCert);
                         InitX509(&ssl->peerCert, 0, ssl->heap);
-                        ret = CopyDecodedToX509(&ssl->peerCert, dCert);
-                        if (ret != 0) {
-                            /* Failed to copy - clear peerCert */
-                            FreeX509(&ssl->peerCert);
-                            InitX509(&ssl->peerCert, 0, ssl->heap);
-                        }
                     }
-                    FreeDecodedCert(dCert);
-                    XFREE(dCert, ssl->heap, DYNAMIC_TYPE_DCERT);
                 }
+                FreeDecodedCert(dCert);
+                XFREE(dCert, ssl->heap, DYNAMIC_TYPE_DCERT);
             }
         }
     }
@@ -39853,6 +39961,14 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
             }
         }
 #endif
+        /* Carry the ticket bindings on the session for the deferred
+         * VerifyTicketBinding() check. */
+#ifdef HAVE_SNI
+        XMEMCPY(ssl->session->sniHash, it->sniHash, TICKET_BINDING_HASH_SZ);
+#endif
+#ifdef HAVE_ALPN
+        XMEMCPY(ssl->session->alpnHash, it->alpnHash, TICKET_BINDING_HASH_SZ);
+#endif
 
         if (!IsAtLeastTLSv1_3(ssl->version)) {
             if (ssl->arrays == NULL)
@@ -39962,6 +40078,12 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
         it->sessionCtxSz = sess->sessionCtxSz;
         XMEMCPY(it->sessionCtx, sess->sessionCtx, sess->sessionCtxSz);
 #endif
+#ifdef HAVE_SNI
+        XMEMCPY(it->sniHash, sess->sniHash, TICKET_BINDING_HASH_SZ);
+#endif
+#ifdef HAVE_ALPN
+        XMEMCPY(it->alpnHash, sess->alpnHash, TICKET_BINDING_HASH_SZ);
+#endif
 #if defined(OPENSSL_ALL) && defined(KEEP_PEER_CERT) && \
     defined(SESSION_CERTS) && !defined(NO_CERT_IN_TICKET)
         /* Store peer certificate from session chain */
@@ -40206,6 +40328,8 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
             goto cleanup;
         }
 
+        /* SNI/ALPN binding is verified after ALPN_Select via
+         * VerifyTicketBinding(). */
         DoClientTicketFinalize(ssl, it, NULL);
 
 cleanup:

src/ssl_sess.c

@@ -3069,6 +3069,7 @@ WOLFSSL_SESSION* wolfSSL_d2i_SSL_SESSION(WOLFSSL_SESSION** sess,
     (void)idx;
 
     if (sess != NULL) {
+        wolfSSL_FreeSession(NULL, *sess);
         *sess = s;
     }
 

src/tls13.c

@@ -7632,6 +7632,10 @@ int DoTls13ClientHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
      * select the ALPN protocol, if so requested */
     if ((ret = ALPN_Select(ssl)) != 0)
         goto exit_dch;
+#endif
+#if defined(HAVE_SESSION_TICKET) && (defined(HAVE_SNI) || defined(HAVE_ALPN))
+    if ((ret = VerifyTicketBinding(ssl)) != 0)
+        goto exit_dch;
 #endif
     } /* case TLS_ASYNC_BEGIN */
     FALL_THROUGH;

src/x509_str.c

@@ -706,23 +706,13 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx)
             /* We found our issuer in the non-trusted cert list, add it
              * to the CM and verify the current cert against it */
         #ifndef WOLFSSL_X509_STORE_ALLOW_NON_CA_INTERMEDIATE
-            /* RFC 5280 6.1.3(k): a non-self-issued intermediate must have
-             * basicConstraints CA:TRUE to be used as a signing authority.
-             * Reject CA:FALSE intermediates here; the verify_cb (if any)
-             * may override.  Define WOLFSSL_X509_STORE_ALLOW_NON_CA_INTERMEDIATE
-             * to restore the legacy permissive behavior.
-             */
+            /* RFC 5280 4.2.1.9: reject non-CA issuer. verify_cb may
+             * suppress the INVALID_CA error to keep building the chain,
+             * but the leaf signature must still be verified against the
+             * issuer below - never skip X509StoreVerifyCert. */
             if (!issuer->isCa) {
-                /* error depth is current depth + 1.  The compat alias
-                 * X509_V_ERR_INVALID_CA (= 79) lives in wolfssl/openssl/x509.h
-                 * which is not always pulled into this translation unit
-                 * (e.g. some linuxkm build chains).  Define a local fallback
-                 * so callers reading X509_STORE_CTX_get_error() see the
-                 * OpenSSL-compatible value. */
-            #ifndef X509_V_ERR_INVALID_CA
-                #define X509_V_ERR_INVALID_CA 79
-            #endif
-                SetupStoreCtxError_ex(ctx, X509_V_ERR_INVALID_CA,
+                /* error depth is current depth + 1 */
+                SetupStoreCtxError_ex(ctx, WOLFSSL_X509_V_ERR_INVALID_CA,
                                 (ctx->chain) ? (int)(ctx->chain->num + 1) : 1);
             #if defined(OPENSSL_ALL) || defined(WOLFSSL_QT)
                 if (ctx->store->verify_cb) {
@@ -738,28 +728,25 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx)
                     ret = WOLFSSL_FAILURE;
                     goto exit;
                 }
-            } else
+            }
         #endif
-            {
-                ret = X509StoreAddCa(ctx->store, issuer,
-                                                WOLFSSL_TEMP_CA);
-                if (ret != WOLFSSL_SUCCESS) {
-                    X509VerifyCertSetupRetry(ctx, certs, failedCerts,
-                        &depth, origDepth);
-                    continue;
-                }
-                added = 1;
-                ret = X509StoreVerifyCert(ctx);
-                if (ret != WOLFSSL_SUCCESS) {
-                    if ((origDepth - depth) <= 1)
-                        added = 0;
-                    X509VerifyCertSetupRetry(ctx, certs, failedCerts,
-                        &depth, origDepth);
-                    continue;
-                }
-                /* Add it to the current chain and look at the issuer cert next */
-                wolfSSL_sk_X509_push(ctx->chain, ctx->current_cert);
+            ret = X509StoreAddCa(ctx->store, issuer, WOLFSSL_TEMP_CA);
+            if (ret != WOLFSSL_SUCCESS) {
+                X509VerifyCertSetupRetry(ctx, certs, failedCerts,
+                    &depth, origDepth);
+                continue;
             }
+            added = 1;
+            ret = X509StoreVerifyCert(ctx);
+            if (ret != WOLFSSL_SUCCESS) {
+                if ((origDepth - depth) <= 1)
+                    added = 0;
+                X509VerifyCertSetupRetry(ctx, certs, failedCerts,
+                    &depth, origDepth);
+                continue;
+            }
+            /* Add it to the current chain and look at the issuer cert next */
+            wolfSSL_sk_X509_push(ctx->chain, ctx->current_cert);
             ctx->current_cert = issuer;
         }
         else if (ret == WC_NO_ERR_TRACE(WOLFSSL_FAILURE)) {

tests/api.c

@@ -30232,9 +30232,11 @@ static int error_test(void)
         {11, 11},
         {17, 15},
         {19, 19},
+        {24, 24},
         {27, 26 },
         {61, 30},
         {63, 63},
+        {78, 65},
 #endif
         { -9, WC_SPAN1_FIRST_E + 1 },
         { -300, -300 },

tests/api/test_asn.c

@@ -969,7 +969,7 @@ int test_DecodeAltNames_length_underflow(void)
         0x06, 0x03, 0x55, 0x1d, 0x13, 0x01, 0x01, 0xff, 0x04, 0x02, 0x30, 0x00,
         /* SAN extension: correct SEQUENCE length 0x06 */
         0x30, 0x0f, 0x06, 0x03, 0x55, 0x1d, 0x11, 0x04, 0x08, 0x30, 0x06, 0x82,
-        0x04, 0x61, 0x2a, 0x00, 0x2a, 0x30, 0x1d, 0x06, 0x03, 0x55, 0x1d, 0x0e,
+        0x04, 0x61, 0x2a, 0x62, 0x2a, 0x30, 0x1d, 0x06, 0x03, 0x55, 0x1d, 0x0e,
         0x04, 0x16, 0x04, 0x14, 0x92, 0x6a, 0x1e, 0x52, 0x3a, 0x1a, 0x57, 0x9f,
         0xc9, 0x82, 0x9a, 0xce, 0xc8, 0xc0, 0xa9, 0x51, 0x9d, 0x2f, 0xc7, 0x72,
         0x30, 0x1f, 0x06, 0x03, 0x55, 0x1d, 0x23, 0x04, 0x18, 0x30, 0x16, 0x80,
@@ -1025,6 +1025,16 @@ int test_DecodeAltNames_length_underflow(void)
         WC_NO_ERR_TRACE(ASN_PARSE_E));
     wc_FreeDecodedCert(&cert);
 
+    /* NUL in dNSName SAN must be rejected per RFC 5280 4.2.1.6. */
+    XMEMCPY(bad_san_cert, good_san_cert, sizeof(good_san_cert));
+    bad_san_cert[SAN_SEQ_LEN_OFFSET + 5] = 0x00;
+
+    wc_InitDecodedCert(&cert, bad_san_cert, (word32)sizeof(bad_san_cert),
+        NULL);
+    ExpectIntEQ(wc_ParseCert(&cert, CERT_TYPE, NO_VERIFY, NULL),
+        WC_NO_ERR_TRACE(ASN_PARSE_E));
+    wc_FreeDecodedCert(&cert);
+
 #endif /* !NO_CERTS && !NO_RSA && !NO_ASN */
     return EXPECT_RESULT();
 }

tests/api/test_ossl_x509.c

@@ -1144,7 +1144,7 @@ int test_wolfSSL_X509_bad_altname(void)
         0xf5, 0xe5, 0x09, 0x02, 0x01, 0x03, 0xa3, 0x61, 0x30, 0x5f, 0x30, 0x0c,
         0x06, 0x03, 0x55, 0x1d, 0x13, 0x01, 0x01, 0xff, 0x04, 0x02, 0x30, 0x00,
         0x30, 0x0f, 0x06, 0x03, 0x55, 0x1d, 0x11, 0x04, 0x08, 0x30, 0x06, 0x82,
-        0x04, 0x61, 0x2a, 0x00, 0x2a, 0x30, 0x1d, 0x06, 0x03, 0x55, 0x1d, 0x0e,
+        0x04, 0x61, 0x2a, 0x62, 0x2a, 0x30, 0x1d, 0x06, 0x03, 0x55, 0x1d, 0x0e,
         0x04, 0x16, 0x04, 0x14, 0x92, 0x6a, 0x1e, 0x52, 0x3a, 0x1a, 0x57, 0x9f,
         0xc9, 0x82, 0x9a, 0xce, 0xc8, 0xc0, 0xa9, 0x51, 0x9d, 0x2f, 0xc7, 0x72,
         0x30, 0x1f, 0x06, 0x03, 0x55, 0x1d, 0x23, 0x04, 0x18, 0x30, 0x16, 0x80,
@@ -1183,8 +1183,7 @@ int test_wolfSSL_X509_bad_altname(void)
     ExpectNotNull(x509 = wolfSSL_X509_load_certificate_buffer(
         malformed_alt_name_cert, certSize, SSL_FILETYPE_ASN1));
 
-    /* malformed_alt_name_cert has a malformed alternative
-     * name of "a*\0*". Ensure that it does not match "aaaaa" */
+    /* SAN "a*b*" must not match "aaaaa" under any wildcard flag. */
     ExpectIntNE(wolfSSL_X509_check_host(x509, name, nameLen,
         WOLFSSL_ALWAYS_CHECK_SUBJECT, NULL), 1);