Prevent cert modification in callbacks

src/internal.c

@@ -8268,6 +8268,49 @@ static int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
 
     return ret;
 }
+
+#if defined(HAVE_SNI) || defined(WOLFSSL_CERT_SETUP_CB)
+static int IsStaleCtxDer(const DerBuffer* der, byte weOwn,
+    const DerBuffer* ctxDer)
+{
+    return (!weOwn) && (der != NULL) && (der != ctxDer);
+}
+
+/* Check the CTX still has the buffers the SSL points to but does not own.
+ * Returns BAD_STATE_E when the CTX replaced one. */
+int CheckSSL_CTX_CertsAndKeys(const WOLFSSL* ssl)
+{
+    const WOLFSSL_CTX* ctx = ssl->ctx;
+    const DerBuffer* ctxKey = ctx->privateKey;
+    int stale = 0;
+
+#ifdef WOLFSSL_DUAL_ALG_CERTS
+    /* Signing with the alternative key swaps it in as the key. */
+    if (ssl->buffers.key == ctx->altPrivateKey) {
+        ctxKey = ctx->altPrivateKey;
+    }
+    /* After the swap, the buffer is owned by the key. */
+    if (ssl->buffers.altKey != ssl->buffers.key) {
+        stale = IsStaleCtxDer(ssl->buffers.altKey, ssl->buffers.weOwnAltKey,
+            ctx->altPrivateKey);
+    }
+#endif
+    if (stale ||
+            IsStaleCtxDer(ssl->buffers.certificate, ssl->buffers.weOwnCert,
+                ctx->certificate) ||
+            IsStaleCtxDer(ssl->buffers.certChain, ssl->buffers.weOwnCertChain,
+                ctx->certChain) ||
+            /* A chain replaced at the same address can differ in count. */
+            (!ssl->buffers.weOwnCertChain && (ssl->buffers.certChain != NULL) &&
+                (ssl->buffers.certChainCnt != ctx->certChainCnt)) ||
+            IsStaleCtxDer(ssl->buffers.key, ssl->buffers.weOwnKey, ctxKey)) {
+        WOLFSSL_MSG("CTX certificate or key replaced during handshake");
+        WOLFSSL_ERROR_VERBOSE(BAD_STATE_E);
+        return BAD_STATE_E;
+    }
+    return 0;
+}
+#endif /* HAVE_SNI || WOLFSSL_CERT_SETUP_CB */
 #endif /* NO_CERTS */
 
 #ifndef NO_DH
@@ -47416,6 +47459,13 @@ static int DefTicketEncCb(WOLFSSL* ssl, byte key_name[WOLFSSL_TICKET_NAME_SZ],
         if(ssl && ssl->ctx && ssl->ctx->sniRecvCb) {
             WOLFSSL_MSG("Calling custom sni callback");
             sniRet = ssl->ctx->sniRecvCb(ssl, &ad, ssl->ctx->sniRecvCbArg);
+        #ifndef NO_CERTS
+            ret = CheckSSL_CTX_CertsAndKeys(ssl);
+            if (ret != 0) {
+                SendAlert(ssl, alert_fatal, internal_error);
+                return ret;
+            }
+        #endif
             switch (sniRet) {
                 case warning_return:
                     /* TLS 1.3 error alerts are always fatal (RFC 8446 6.2) */

src/ssl_api_cert.c

@@ -2949,6 +2949,8 @@ void wolfSSL_CTX_set_cert_cb(WOLFSSL_CTX* ctx,
  *          unrecognized value. A fatal alert is sent when it failed.
  * @return  WOLFSSL_ERROR_WANT_X509_LOOKUP when the callback returned a
  *          negative value to ask to be called again.
+ * @return  BAD_STATE_E when the callback replaced a certificate or key on the
+ *          CTX that the SSL uses. A fatal alert is sent.
  */
 int CertSetupCbWrapper(WOLFSSL* ssl)
 {
@@ -2957,6 +2959,12 @@ int CertSetupCbWrapper(WOLFSSL* ssl)
     if (ssl->ctx->certSetupCb != NULL) {
         WOLFSSL_MSG("Calling user cert setup callback");
         ret = ssl->ctx->certSetupCb(ssl, ssl->ctx->certSetupCbArg);
+    #ifndef NO_CERTS
+        if (CheckSSL_CTX_CertsAndKeys(ssl) != 0) {
+            SendAlert(ssl, alert_fatal, internal_error);
+            return BAD_STATE_E;
+        }
+    #endif
         if (ret == 1) {
             WOLFSSL_MSG("User cert callback returned success");
             ret = 0;

tests/api/test_tls.c

@@ -4168,6 +4168,270 @@ int test_tls13_session_resumption_sni_mismatch(void)
     return EXPECT_RESULT();
 }
 
+#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \
+    (defined(HAVE_SNI) || defined(WOLFSSL_CERT_SETUP_CB)) && \
+    !defined(NO_RSA) && !defined(NO_FILESYSTEM)
+    #define TEST_CB_CTX_CERT_LOAD
+#endif
+
+#ifdef TEST_CB_CTX_CERT_LOAD
+enum {
+    CB_SNI,
+    CB_CERT_SETUP
+};
+
+enum {
+    CB_LOAD_CTX_CERT,
+    CB_LOAD_CTX_KEY,
+    CB_LOAD_SSL_CERT,
+    CB_SWITCH_CTX
+};
+
+typedef struct CbLoadCase {
+    WOLFSSL_CTX* ctx2;
+    int action;
+    int retry;
+    int loadRet;
+} CbLoadCase;
+
+static void cb_load_run(WOLFSSL* ssl, CbLoadCase* c)
+{
+    WOLFSSL_CTX* ctx = wolfSSL_get_SSL_CTX(ssl);
+
+    switch (c->action) {
+        case CB_LOAD_CTX_CERT:
+            c->loadRet = wolfSSL_CTX_use_certificate_file(ctx, svrCertFile,
+                CERT_FILETYPE);
+            break;
+        case CB_LOAD_CTX_KEY:
+            c->loadRet = wolfSSL_CTX_use_PrivateKey_file(ctx, svrKeyFile,
+                CERT_FILETYPE);
+            break;
+    #ifdef OPENSSL_EXTRA
+        case CB_SWITCH_CTX:
+            c->loadRet = (wolfSSL_set_SSL_CTX(ssl, c->ctx2) == c->ctx2) ?
+                WOLFSSL_SUCCESS : WOLFSSL_FAILURE;
+            break;
+    #endif
+        default:
+            c->loadRet = wolfSSL_use_certificate_file(ssl, svrCertFile,
+                CERT_FILETYPE);
+            break;
+    }
+}
+
+#ifdef HAVE_SNI
+static int sni_load_cb(WOLFSSL* ssl, int* ad, void* arg)
+{
+    CbLoadCase* c = (CbLoadCase*)arg;
+
+    (void)ad;
+    cb_load_run(ssl, c);
+    return c->retry ? warning_return : 0;
+}
+#endif
+
+#ifdef WOLFSSL_CERT_SETUP_CB
+static int cert_setup_load_cb(WOLFSSL* ssl, void* arg)
+{
+    CbLoadCase* c = (CbLoadCase*)arg;
+
+    cb_load_run(ssl, c);
+    return c->retry ? -1 : 1;
+}
+#endif
+
+static int test_cb_ctx_cert_load_one(int cb, method_provider client,
+    method_provider server, int action, int retry, int err)
+{
+    EXPECT_DECLS;
+    WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL;
+    WOLFSSL *ssl_c = NULL, *ssl_s = NULL;
+    struct test_memio_ctx test_ctx;
+    CbLoadCase c;
+
+    XMEMSET(&test_ctx, 0, sizeof(test_ctx));
+    XMEMSET(&c, 0, sizeof(c));
+    c.action = action;
+    c.retry = retry;
+    ExpectNotNull(c.ctx2 = wolfSSL_CTX_new(server()));
+    ExpectIntEQ(wolfSSL_CTX_use_certificate_file(c.ctx2, svrCertFile,
+        CERT_FILETYPE), WOLFSSL_SUCCESS);
+    ExpectIntEQ(wolfSSL_CTX_use_PrivateKey_file(c.ctx2, svrKeyFile,
+        CERT_FILETYPE), WOLFSSL_SUCCESS);
+#ifndef NO_DH
+    /* Switching contexts takes the DH state from the new one. */
+    SetDHCtx(c.ctx2);
+#endif
+    ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
+        client, server), 0);
+#ifdef HAVE_SNI
+    if (cb == CB_SNI) {
+        const char* host = "example.com";
+
+        wolfSSL_CTX_set_servername_callback(ctx_s, sni_load_cb);
+        ExpectIntEQ(wolfSSL_CTX_set_servername_arg(ctx_s, &c),
+            WOLFSSL_SUCCESS);
+        ExpectIntEQ(wolfSSL_UseSNI(ssl_c, WOLFSSL_SNI_HOST_NAME, host,
+            (word16)XSTRLEN(host)), WOLFSSL_SUCCESS);
+    }
+#endif
+#ifdef WOLFSSL_CERT_SETUP_CB
+    if (cb == CB_CERT_SETUP) {
+        wolfSSL_CTX_set_cert_cb(ctx_s, cert_setup_load_cb, &c);
+    }
+#endif
+    if (err == 0) {
+        ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
+    }
+    else {
+        ExpectIntNE(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
+        ExpectIntEQ(wolfSSL_get_error(ssl_s, WOLFSSL_FATAL_ERROR), err);
+    }
+    ExpectIntEQ(c.loadRet, WOLFSSL_SUCCESS);
+
+    wolfSSL_free(ssl_c);
+    wolfSSL_free(ssl_s);
+    wolfSSL_CTX_free(ctx_c);
+    wolfSSL_CTX_free(ctx_s);
+    wolfSSL_CTX_free(c.ctx2);
+    return EXPECT_RESULT();
+}
+#endif /* TEST_CB_CTX_CERT_LOAD */
+
+/* Replacing the certificate or key on the context from the SNI or certificate
+ * setup callback fails the handshake. Loading one on the session is allowed. */
+int test_tls_cb_ctx_cert_load(void)
+{
+    EXPECT_DECLS;
+#ifdef TEST_CB_CTX_CERT_LOAD
+    static const int cbs[] = {
+    #ifdef HAVE_SNI
+        CB_SNI,
+    #endif
+    #ifdef WOLFSSL_CERT_SETUP_CB
+        CB_CERT_SETUP,
+    #endif
+    };
+    static const struct {
+        method_provider client;
+        method_provider server;
+    } methods[] = {
+    #ifndef WOLFSSL_NO_TLS12
+        { wolfTLSv1_2_client_method, wolfTLSv1_2_server_method },
+    #endif
+    #ifdef WOLFSSL_TLS13
+        { wolfTLSv1_3_client_method, wolfTLSv1_3_server_method },
+    #endif
+    };
+    static const struct {
+        int action;
+        int retry; /* callback returns its warning or retry value */
+        int err;
+    } cases[] = {
+    #ifdef WOLFSSL_COPY_CERT
+        { CB_LOAD_CTX_CERT, 0, 0 },
+    #else
+        { CB_LOAD_CTX_CERT, 0, WC_NO_ERR_TRACE(BAD_STATE_E) },
+        { CB_LOAD_CTX_CERT, 1, WC_NO_ERR_TRACE(BAD_STATE_E) },
+    #endif
+    #if defined(WOLFSSL_COPY_KEY) || defined(WOLFSSL_BLIND_PRIVATE_KEY)
+        { CB_LOAD_CTX_KEY, 0, 0 },
+    #else
+        { CB_LOAD_CTX_KEY, 0, WC_NO_ERR_TRACE(BAD_STATE_E) },
+    #endif
+        { CB_LOAD_SSL_CERT, 0, 0 },
+    #ifdef OPENSSL_EXTRA
+        { CB_SWITCH_CTX, 0, 0 },
+    #endif
+    };
+    size_t i;
+    size_t j;
+    size_t k;
+
+    for (k = 0; k < XELEM_CNT(cbs); k++) {
+        for (i = 0; i < XELEM_CNT(methods); i++) {
+            for (j = 0; j < XELEM_CNT(cases); j++) {
+                ExpectIntEQ(test_cb_ctx_cert_load_one(cbs[k],
+                    methods[i].client, methods[i].server, cases[j].action,
+                    cases[j].retry, cases[j].err), TEST_SUCCESS);
+            }
+        }
+    }
+#endif
+    return EXPECT_RESULT();
+}
+
+#if defined(WOLFSSL_DUAL_ALG_CERTS) && defined(HAVE_ECC) && \
+    !defined(WOLFSSL_COPY_KEY) && !defined(WOLFSSL_BLIND_PRIVATE_KEY)
+    #define TEST_SSL_CTX_ALT_KEY
+#endif
+
+/* The buffers the SSL shares with its context are checked by pointer and by
+ * chain count. The alternative key swapped in to sign is still the context's.
+ */
+int test_tls_ssl_ctx_certs_check(void)
+{
+    EXPECT_DECLS;
+#if (defined(HAVE_SNI) || defined(WOLFSSL_CERT_SETUP_CB)) && \
+    !defined(NO_CERTS) && !defined(NO_TLS) && !defined(NO_WOLFSSL_SERVER) && \
+    !defined(NO_FILESYSTEM) && defined(WOLFSSL_PEM_TO_DER) && \
+    !defined(NO_RSA) && !defined(WOLFSSL_COPY_CERT)
+    WOLFSSL_CTX* ctx = NULL;
+    WOLFSSL* ssl = NULL;
+#ifdef TEST_SSL_CTX_ALT_KEY
+    byte* altKey = NULL;
+    size_t altKeySz = 0;
+#endif
+
+    ExpectNotNull(ctx = wolfSSL_CTX_new(wolfSSLv23_server_method()));
+    ExpectIntEQ(wolfSSL_CTX_use_certificate_chain_file(ctx, svrCertFile),
+        WOLFSSL_SUCCESS);
+    ExpectIntEQ(wolfSSL_CTX_use_PrivateKey_file(ctx, svrKeyFile,
+        CERT_FILETYPE), WOLFSSL_SUCCESS);
+#ifdef TEST_SSL_CTX_ALT_KEY
+    ExpectIntEQ(wolfSSL_CTX_use_AltPrivateKey_file(ctx, eccKeyFile,
+        CERT_FILETYPE), WOLFSSL_SUCCESS);
+#endif
+    ExpectNotNull(ssl = wolfSSL_new(ctx));
+    ExpectIntEQ(CheckSSL_CTX_CertsAndKeys(ssl), 0);
+
+    /* Chain replaced at the same address with a different count. */
+    if (ctx != NULL) {
+        ctx->certChainCnt++;
+    }
+    ExpectIntEQ(CheckSSL_CTX_CertsAndKeys(ssl), WC_NO_ERR_TRACE(BAD_STATE_E));
+    if (ctx != NULL) {
+        ctx->certChainCnt--;
+    }
+
+#ifdef TEST_SSL_CTX_ALT_KEY
+    /* Same swap as SendTls13CertificateVerify with the alternative key. */
+    if (ssl != NULL) {
+        ssl->buffers.key = ssl->buffers.altKey;
+        ssl->buffers.weOwnKey = ssl->buffers.weOwnAltKey;
+        ssl->buffers.weOwnAltKey = 0;
+    }
+    ExpectIntEQ(CheckSSL_CTX_CertsAndKeys(ssl), 0);
+
+    /* A new alternative key on the SSL leaves the swapped key with the CTX. */
+    ExpectIntEQ(load_file("./certs/ecc-key.der", &altKey, &altKeySz), 0);
+    ExpectIntEQ(wolfSSL_use_AltPrivateKey_buffer(ssl, altKey, (long)altKeySz,
+        WOLFSSL_FILETYPE_ASN1), WOLFSSL_SUCCESS);
+    ExpectIntEQ(CheckSSL_CTX_CertsAndKeys(ssl), 0);
+
+    ExpectIntEQ(wolfSSL_CTX_use_AltPrivateKey_file(ctx, eccKeyFile,
+        CERT_FILETYPE), WOLFSSL_SUCCESS);
+    ExpectIntEQ(CheckSSL_CTX_CertsAndKeys(ssl), WC_NO_ERR_TRACE(BAD_STATE_E));
+    XFREE(altKey, NULL, DYNAMIC_TYPE_TMP_BUFFER);
+#endif
+
+    wolfSSL_free(ssl);
+    wolfSSL_CTX_free(ctx);
+#endif
+    return EXPECT_RESULT();
+}
+
 /* Regression test for the post-ALPN_Select PSK-head check.
  * When ALPN_Select runs before CheckPreSharedKeys (so the per-PSK
  * binding check has the negotiated ALPN available), TLSX_SetALPN

tests/api/test_tls.h

@@ -71,6 +71,8 @@ int test_tls12_client_hello_no_tls13_suites(void);
 int test_tls_set_session_min_downgrade(void);
 int test_tls12_session_id_resumption_sni_mismatch(void);
 int test_tls13_session_resumption_sni_mismatch(void);
+int test_tls_cb_ctx_cert_load(void);
+int test_tls_ssl_ctx_certs_check(void);
 int test_tls13_resumption_with_alpn(void);
 int test_tls12_session_id_resumption_alpn_mismatch(void);
 int test_tls13_session_resumption_alpn_mismatch(void);
@@ -152,6 +154,8 @@ int test_tls_param_flags_crl_check(void);
         TEST_DECL_GROUP("tls", test_tls_set_session_min_downgrade),            \
         TEST_DECL_GROUP("tls", test_tls12_session_id_resumption_sni_mismatch), \
         TEST_DECL_GROUP("tls", test_tls13_session_resumption_sni_mismatch),    \
+        TEST_DECL_GROUP("tls", test_tls_cb_ctx_cert_load),                     \
+        TEST_DECL_GROUP("tls", test_tls_ssl_ctx_certs_check),                  \
         TEST_DECL_GROUP("tls", test_tls13_resumption_with_alpn),              \
         TEST_DECL_GROUP("tls", test_tls12_session_id_resumption_alpn_mismatch),\
         TEST_DECL_GROUP("tls", test_tls13_session_resumption_alpn_mismatch),   \

wolfssl/internal.h

@@ -5684,6 +5684,13 @@ typedef struct Buffers {
  * not reference counted, so a session must not point at the context's. */
 WOLFSSL_LOCAL int CopySSL_CTX_DhParams(WOLFSSL* ssl, WOLFSSL_CTX* ctx);
 #endif
+#if !defined(NO_CERTS) && \
+    (defined(HAVE_SNI) || defined(WOLFSSL_CERT_SETUP_CB))
+#ifdef WOLFSSL_API_PREFIX_MAP
+    #define CheckSSL_CTX_CertsAndKeys wolfSSL_CheckSSL_CTX_CertsAndKeys
+#endif
+WOLFSSL_TEST_VIS int CheckSSL_CTX_CertsAndKeys(const WOLFSSL* ssl);
+#endif
 
 /* sub-states for send/do key share (key exchange) */
 enum asyncState {