Block cert modification in callbacks based on new flag

ChangeLog.md

@@ -2,6 +2,30 @@
 
 ## Behavioral Changes
 
+* **Behavioral change (loading a certificate or key on a context from inside a
+  callback)**: the sni callback set with `wolfSSL_CTX_set_servername_callback()`
+  and the certificate setup callback set with `wolfSSL_CTX_set_cert_cb()` run in
+  the middle of a handshake, at which point every session made from that context
+  is pointing at the context's certificate, chain and key.  Replacing one of
+  those frees what those handshakes are reading.  Such a load is now refused
+  while the calling thread is inside one of those callbacks, and the reason,
+  `BAD_STATE_E`, is left where `wolfSSL_get_error()` and the OpenSSL error queue
+  can report it.  The calls affected are `wolfSSL_CTX_use_certificate()`, its
+  `_file` and `_buffer` forms, `wolfSSL_CTX_use_PrivateKey_file()`, `_buffer`,
+  `_Id` and `_Label`, the `wolfSSL_CTX_use_AltPrivateKey_*` pair, the
+  `wolfSSL_CTX_use_certificate_chain_*` family, and
+  `wolfSSL_CTX_add0_chain_cert()`, `wolfSSL_CTX_add1_chain_cert()` and
+  `wolfSSL_CTX_add_extra_chain_cert()`.  An application that set a certificate
+  this way should set it on the session instead, with
+  `wolfSSL_use_certificate_file()` and its relatives, or hand the session a
+  different context with `wolfSSL_set_SSL_CTX()`; both are untouched, as are
+  loads into the trust store such as `wolfSSL_CTX_load_verify_locations()` and
+  anything called outside a callback.  The note that a callback is running is
+  kept per thread, so one thread's callback does not refuse another thread's
+  loads; on a build without thread local storage it is shared, where overlapping
+  callbacks can refuse a load that would have been allowed, or allow one that
+  would have been refused.
+
 * **Behavioral change (`wc_PufReadSram` health tests the raw SRAM readout)**:
   the raw readout is now health tested before the context accepts it, and a
   readout that cannot be SRAM power-on noise is rejected with `PUF_READ_E`

doc/dox_comments/header_files/ssl.h

@@ -5959,6 +5959,13 @@ void wolfSSL_CTX_set_client_cert_cb(WOLFSSL_CTX *ctx, client_cert_cb cb);
     application can inspect, set or clear certificates - for example to react
     to a CA list sent by the peer.
 
+    Set the certificate on the WOLFSSL object, with wolfSSL_use_certificate_file
+    and friends, or hand it a different context with wolfSSL_set_SSL_CTX.
+    Loading one on the WOLFSSL_CTX the handshake is running against is refused
+    from inside the callback and returns failure with BAD_STATE_E: sessions
+    already made from that context point at its certificate, and replacing it
+    would free what they are reading.
+
     \param ctx The WOLFSSL_CTX object.
     \param cb  The callback function for certificate setup.
     \param arg User argument to pass to the callback.

src/internal.c

@@ -7704,8 +7704,70 @@ static int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
 
     return ret;
 }
+
 #endif /* NO_CERTS */
 
+/* Context this thread is currently inside a callback for, if any.
+ *
+ * Kept per thread rather than on the context: the context is shared between
+ * threads, so a flag on it would race with other handshakes and would be
+ * written to a context the callback may have swapped out from under us. Only
+ * ever compared, never followed, so a context freed during the callback does
+ * no harm. Where the build has no thread local storage this is one shared
+ * pointer, which costs the guard accuracy when threads overlap but still
+ * cannot corrupt anything.
+ */
+static THREAD_LS_T WOLFSSL_CTX* inCbCtx = NULL;
+
+/* Note that this thread is entering a callback on a context.
+ *
+ * @param [in] ctx  SSL context object the callback belongs to.
+ * @return  What was noted before, to hand back to CtxCallbackExit().
+ */
+WOLFSSL_CTX* CtxCallbackEnter(WOLFSSL_CTX* ctx)
+{
+    WOLFSSL_CTX* prev = inCbCtx;
+
+    inCbCtx = ctx;
+
+    return prev;
+}
+
+/* Note that this thread has left the callback.
+ *
+ * @param [in] prev  What CtxCallbackEnter() handed back.
+ */
+void CtxCallbackExit(WOLFSSL_CTX* prev)
+{
+    inCbCtx = prev;
+}
+
+#ifndef NO_CERTS
+/* Refuse to replace a certificate or key on a context from its own callback.
+ *
+ * Sessions made from a context point at its buffers, so replacing one frees
+ * what handshakes already under way are reading. Setting a certificate on the
+ * session alone, or handing it a different context, is what the callbacks are
+ * for.
+ *
+ * @param [in] ctx  SSL context object. May be NULL.
+ * @return  0 when the load may go ahead.
+ * @return  BAD_STATE_E while this thread is in a callback on the context.
+ */
+int CheckCtxCertLoad(WOLFSSL_CTX* ctx)
+{
+    int ret = 0;
+
+    if ((ctx != NULL) && (ctx == inCbCtx)) {
+        WOLFSSL_MSG("Certificate load refused: callback running on context");
+        ret = BAD_STATE_E;
+        WOLFSSL_ERROR_VERBOSE(ret);
+    }
+
+    return ret;
+}
+#endif /* !NO_CERTS */
+
 int SetSSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup)
 {
     int ret = WOLFSSL_SUCCESS; /* set default ret */
@@ -45375,8 +45437,14 @@ static int DefTicketEncCb(WOLFSSL* ssl, byte key_name[WOLFSSL_TICKET_NAME_SZ],
         /* Stunnel supports a custom sni callback to switch an SSL's ctx
         * when SNI is received. Call it now if exists */
         if(ssl && ssl->ctx && ssl->ctx->sniRecvCb) {
+            WOLFSSL_CTX* prevCbCtx;
+
             WOLFSSL_MSG("Calling custom sni callback");
+            prevCbCtx = CtxCallbackEnter(ssl->ctx);
             sniRet = ssl->ctx->sniRecvCb(ssl, &ad, ssl->ctx->sniRecvCbArg);
+            /* The callback may have switched this session to another context,
+             * so put back what was noted rather than reading ssl->ctx again. */
+            CtxCallbackExit(prevCbCtx);
             switch (sniRet) {
                 case warning_return:
                     WOLFSSL_MSG("Error in custom sni callback. Warning alert");

src/ssl_api_cert.c

@@ -2812,8 +2812,13 @@ int CertSetupCbWrapper(WOLFSSL* ssl)
     int ret = 0;
 
     if (ssl->ctx->certSetupCb != NULL) {
+        WOLFSSL_CTX* prevCbCtx;
+
         WOLFSSL_MSG("Calling user cert setup callback");
+        prevCbCtx = CtxCallbackEnter(ssl->ctx);
         ret = ssl->ctx->certSetupCb(ssl, ssl->ctx->certSetupCbArg);
+        /* The callback may have switched contexts; restore what was noted. */
+        CtxCallbackExit(prevCbCtx);
         if (ret == 1) {
             WOLFSSL_MSG("User cert callback returned success");
             ret = 0;

src/ssl_load.c

@@ -2677,6 +2677,11 @@ int ProcessBuffer(WOLFSSL_CTX* ctx, const unsigned char* buff, long sz,
     if ((ret == 0) && (sz < 0)) {
         ret = BAD_FUNC_ARG;
     }
+    /* Sessions made from this context hold these by pointer. */
+    if ((ret == 0) && (ssl == NULL) && ((type == CERT_TYPE) ||
+            (type == PRIVATEKEY_TYPE) || (type == ALT_PRIVATEKEY_TYPE))) {
+        ret = CheckCtxCertLoad(ctx);
+    }
 
 #ifdef WOLFSSL_SMALL_STACK
     if (ret == 0) {
@@ -4464,6 +4469,10 @@ int wolfSSL_CTX_use_PrivateKey_Id(WOLFSSL_CTX* ctx, const unsigned char* id,
         return 0;
     }
 
+    if (CheckCtxCertLoad(ctx) != 0) {
+        return 0;
+    }
+
     /* Dispose of old private key and allocate and copy in id. */
     FreeDer(&ctx->privateKey);
     if (AllocCopyDer(&ctx->privateKey, id, (word32)sz, PRIVATEKEY_TYPE,
@@ -4542,6 +4551,10 @@ int wolfSSL_CTX_use_PrivateKey_Label(WOLFSSL_CTX* ctx, const char* label,
 
     sz = (word32)XSTRLEN(label) + 1;
 
+    if (CheckCtxCertLoad(ctx) != 0) {
+        return 0;
+    }
+
     /* Dispose of old private key and allocate and copy in label. */
     FreeDer(&ctx->privateKey);
     if (AllocCopyDer(&ctx->privateKey, (const byte*)label, (word32)sz,
@@ -5206,6 +5219,11 @@ static int wolfssl_ctx_add_to_chain(WOLFSSL_CTX* ctx, const byte* der,
     int ret;
     DerBuffer* derBuffer = NULL;
 
+    /* Sessions made from this context hold the chain by pointer. */
+    if (CheckCtxCertLoad(ctx) != 0) {
+        return 0;
+    }
+
     /* Create a DER buffer from DER encoding. */
     ret = AllocCopyDer(&derBuffer, der, (word32)derSz, CERT_TYPE, ctx->heap);
     if (ret != 0) {
@@ -5313,6 +5331,10 @@ int wolfSSL_CTX_use_certificate(WOLFSSL_CTX *ctx, WOLFSSL_X509 *x)
         res = 0;
     }
 
+    if ((res == 1) && (CheckCtxCertLoad(ctx) != 0)) {
+        res = 0;
+    }
+
     if (res == 1) {
         /* Replace certificate buffer with one holding the new certificate. */
         FreeDer(&ctx->certificate);
@@ -5400,6 +5422,11 @@ int wolfSSL_CTX_add1_chain_cert(WOLFSSL_CTX* ctx, WOLFSSL_X509* x509)
         ret = 0;
     }
 
+    /* Sessions made from this context hold the chain by pointer. */
+    if ((ret == 1) && (CheckCtxCertLoad(ctx) != 0)) {
+        ret = 0;
+    }
+
     /* Check if we already have set a certificate. */
     if ((ret == 1) && (ctx->certificate == NULL)) {
         /* Use the certificate. */