Block cert modification in callbacks based on new flag

ChangeLog.md

@@ -2,6 +2,19 @@
 
 ## Behavioral Changes
 
+* **Behavioral change (loading a certificate or key on a context while one of
+  its callbacks runs)**: loading a certificate or key on a `WOLFSSL_CTX` while
+  its sni callback or certificate setup callback is running is now refused with
+  `BAD_STATE_E`.  The calls affected are `wolfSSL_CTX_use_certificate()` and
+  its `_file` and `_buffer` forms, `wolfSSL_CTX_use_PrivateKey_file()`,
+  `_buffer`, `_Id` and `_Label`, the `wolfSSL_CTX_use_AltPrivateKey_*` and
+  `wolfSSL_CTX_use_certificate_chain_*` families,
+  `wolfSSL_CTX_load_verify_chain_buffer_format()` with DER input,
+  `wolfSSL_CTX_add0_chain_cert()`, `wolfSSL_CTX_add1_chain_cert()`,
+  `wolfSSL_CTX_add_extra_chain_cert()`, `SSL_CTX_set0_chain()` and
+  `SSL_CTX_set1_chain()`.  Set the certificate on the `WOLFSSL` object instead,
+  or switch it to a context that already holds one with `wolfSSL_set_SSL_CTX()`.
+
 * **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

@@ -5980,6 +5980,14 @@ 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, including
+    one handed over with wolfSSL_set_SSL_CTX, is refused from inside the
+    callback and fails with BAD_STATE_E, logged and put on the error queue:
+    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

@@ -7730,6 +7730,7 @@ static int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
 
     return ret;
 }
+
 #endif /* NO_CERTS */
 
 #ifndef NO_DH
@@ -7785,6 +7786,118 @@ int CopySSL_CTX_DhParams(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
 }
 #endif /* !NO_DH */
 
+/* Add to the count of callbacks running on a context, under its lock.
+ *
+ * @param [in, out] ctx  SSL context object.
+ * @param [in]      by   Amount to add, which may be negative.
+ * @return  0 on success.
+ * @return  BAD_MUTEX_E when the context's lock cannot be taken.
+ */
+static int CtxCallbackCount(WOLFSSL_CTX* ctx, int by)
+{
+    int ret = wolfSSL_RefWithMutexLock(&ctx->ref);
+
+    if (ret == 0) {
+        ctx->callbackCnt += by;
+        wolfSSL_RefWithMutexUnlock(&ctx->ref);
+    }
+
+    return ret;
+}
+
+/* Note that an application callback is about to run on a session's context.
+ *
+ * The count is kept on the context, under its lock, so a callback on any
+ * thread is seen. The session is marked as well, so that handing it another
+ * context during the callback carries the count over with CtxCallbackMove().
+ *
+ * @param [in, out] ssl  SSL object the callback is for.
+ * @return  0 on success.
+ * @return  BAD_MUTEX_E when the context's lock cannot be taken.
+ */
+int CtxCallbackEnter(WOLFSSL* ssl)
+{
+    int ret = CtxCallbackCount(ssl->ctx, 1);
+
+    if (ret == 0) {
+        ssl->options.inCtxCb = 1;
+    }
+
+    return ret;
+}
+
+/* Note that the callback has returned.
+ *
+ * @param [in, out] ssl  SSL object the callback was for.
+ */
+void CtxCallbackExit(WOLFSSL* ssl)
+{
+    if (ssl->options.inCtxCb) {
+        ssl->options.inCtxCb = 0;
+        (void)CtxCallbackCount(ssl->ctx, -1);
+    }
+}
+
+/* Carry a running callback over to the context the session is switching to,
+ * so a load on that one is refused from here on just as on the first.
+ *
+ * @param [in, out] ssl  SSL object inside a callback, on its old context.
+ * @param [in, out] ctx  SSL context object the session is switching to.
+ * @return  0 on success.
+ * @return  BAD_MUTEX_E when a context's lock cannot be taken.
+ */
+int CtxCallbackMove(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
+{
+    int ret = CtxCallbackCount(ssl->ctx, -1);
+
+    if (ret == 0) {
+        ret = CtxCallbackCount(ctx, 1);
+    }
+    if (ret != 0) {
+        /* Nothing is counted any more, so there is nothing to take off. */
+        ssl->options.inCtxCb = 0;
+    }
+
+    return ret;
+}
+
+#ifndef NO_CERTS
+/* Refuse to replace a certificate or key on a context while a callback is
+ * running on it.
+ *
+ * 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 a callback is running on the context.
+ * @return  BAD_MUTEX_E when the context's lock cannot be taken.
+ */
+int CheckCtxCertLoad(WOLFSSL_CTX* ctx)
+{
+    int ret = 0;
+
+    if (ctx != NULL) {
+        int running = 0;
+
+        ret = wolfSSL_RefWithMutexLock(&ctx->ref);
+        if (ret == 0) {
+            running = ctx->callbackCnt;
+            wolfSSL_RefWithMutexUnlock(&ctx->ref);
+        }
+        if ((ret == 0) && (running > 0)) {
+            WOLFSSL_MSG("Cert load refused: callback running on context");
+            ret = BAD_STATE_E;
+            WOLFSSL_ERROR(ret);
+        }
+    }
+
+    return ret;
+}
+#endif /* !NO_CERTS */
+
 int SetSSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup)
 {
     int ret = WOLFSSL_SUCCESS; /* set default ret */
@@ -45639,7 +45752,12 @@ static int DefTicketEncCb(WOLFSSL* ssl, byte key_name[WOLFSSL_TICKET_NAME_SZ],
         * when SNI is received. Call it now if exists */
         if(ssl && ssl->ctx && ssl->ctx->sniRecvCb) {
             WOLFSSL_MSG("Calling custom sni callback");
+            ret = CtxCallbackEnter(ssl);
+            if (ret != 0) {
+                return ret;
+            }
             sniRet = ssl->ctx->sniRecvCb(ssl, &ad, ssl->ctx->sniRecvCbArg);
+            CtxCallbackExit(ssl);
             switch (sniRet) {
                 case warning_return:
                     WOLFSSL_MSG("Error in custom sni callback. Warning alert");

src/ssl.c

@@ -8527,6 +8527,11 @@ long wolfSSL_CTX_ctrl(WOLFSSL_CTX* ctx, int cmd, long opt, void* pt)
             ret = WOLFSSL_FAILURE;
             break;
         }
+        /* Sessions made from this context hold the chain by pointer. */
+        if (CheckCtxCertLoad(ctx) != 0) {
+            ret = WOLFSSL_FAILURE;
+            break;
+        }
         /* Clear certificate chain */
         FreeDer(&ctx->certChain);
         if (sk) {
@@ -8841,6 +8846,13 @@ WOLFSSL_CTX* wolfSSL_set_SSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
     (void)ret;
 #endif
 
+    /* A callback handing the session over keeps its guard, on the context the
+     * session is about to point at. */
+    if (ssl->options.inCtxCb && (CtxCallbackMove(ssl, ctx) != 0)) {
+        wolfSSL_CTX_free(ctx);
+        return NULL;
+    }
+
     if (ssl->ctx != NULL)
         wolfSSL_CTX_free(ssl->ctx);
     ssl->ctx = ctx;

src/ssl_api_cert.c

@@ -2821,14 +2821,20 @@ 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_MUTEX_E when the context's lock cannot be taken.
  */
 int CertSetupCbWrapper(WOLFSSL* ssl)
 {
     int ret = 0;
 
     if (ssl->ctx->certSetupCb != NULL) {
         WOLFSSL_MSG("Calling user cert setup callback");
+        ret = CtxCallbackEnter(ssl);
+        if (ret != 0) {
+            return ret;
+        }
         ret = ssl->ctx->certSetupCb(ssl, ssl->ctx->certSetupCbArg);
+        CtxCallbackExit(ssl);
         if (ret == 1) {
             WOLFSSL_MSG("User cert callback returned success");
             ret = 0;

src/ssl_load.c

@@ -2677,6 +2677,12 @@ 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 and the chain by pointer. */
+    if ((ret == 0) && (ssl == NULL) && ((type == CERT_TYPE) ||
+            (type == PRIVATEKEY_TYPE) || (type == ALT_PRIVATEKEY_TYPE) ||
+            userChain)) {
+        ret = CheckCtxCertLoad(ctx);
+    }
 
 #ifdef WOLFSSL_SMALL_STACK
     if (ret == 0) {
@@ -4464,6 +4470,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 +4552,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,
@@ -4582,6 +4596,9 @@ int wolfSSL_CTX_use_AltPrivateKey_Id(WOLFSSL_CTX* ctx, const unsigned char* id,
     if ((ctx == NULL) || (id == NULL) || (sz < 0)) {
         ret = 0;
     }
+    if ((ret == 1) && (CheckCtxCertLoad(ctx) != 0)) {
+        ret = 0;
+    }
 
     if (ret == 1) {
         FreeDer(&ctx->altPrivateKey);
@@ -4632,6 +4649,9 @@ int wolfSSL_CTX_use_AltPrivateKey_Label(WOLFSSL_CTX* ctx, const char* label,
     if ((ctx == NULL) || (label == NULL)) {
         ret = 0;
     }
+    if ((ret == 1) && (CheckCtxCertLoad(ctx) != 0)) {
+        ret = 0;
+    }
 
     if (ret == 1) {
         sz = (word32)XSTRLEN(label) + 1;
@@ -5207,6 +5227,11 @@ static int wolfssl_ctx_add_to_chain(WOLFSSL_CTX* ctx, const byte* der,
 {
     int res;
 
+    /* Sessions made from this context hold the chain by pointer. */
+    if (CheckCtxCertLoad(ctx) != 0) {
+        return 0;
+    }
+
     /* Add chain to DER buffer. */
     res = wolfssl_add_to_chain(&ctx->certChain, 1, der, (word32)derSz,
         ctx->heap);
@@ -5299,6 +5324,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);
@@ -5386,6 +5415,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. */