diff --git a/src/internal.c b/src/internal.c index 3765426a327..9497cd418aa 100644 --- a/src/internal.c +++ b/src/internal.c @@ -7174,6 +7174,15 @@ static int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx) /* ctx still owns certificate, certChain, key, dh, and cm */ ssl->buffers.certificate = ctx->certificate; ssl->buffers.certChain = ctx->certChain; + /* Inc refcount on the shared DER buffers */ + if (!RefDer(ssl->buffers.certificate)) { + ssl->buffers.certificate = NULL; + return BAD_MUTEX_E; + } + if (!RefDer(ssl->buffers.certChain)) { + ssl->buffers.certChain = NULL; + return BAD_MUTEX_E; + } #endif ssl->buffers.certChainCnt = ctx->certChainCnt; #ifndef WOLFSSL_BLIND_PRIVATE_KEY @@ -7191,10 +7200,15 @@ static int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx) ssl->buffers.weOwnKey = 1; } else { - ssl->buffers.key = ctx->privateKey; + ssl->buffers.key = ctx->privateKey; } #else - ssl->buffers.key = ctx->privateKey; + ssl->buffers.key = ctx->privateKey; + /* Inc refcount on the shared DER buffer */ + if (!RefDer(ssl->buffers.key)) { + ssl->buffers.key = NULL; + return BAD_MUTEX_E; + } #endif #else if (ctx->privateKey != NULL) { @@ -7225,6 +7239,10 @@ static int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx) #ifdef WOLFSSL_DUAL_ALG_CERTS #ifndef WOLFSSL_BLIND_PRIVATE_KEY ssl->buffers.altKey = ctx->altPrivateKey; + if (!RefDer(ssl->buffers.altKey)) { + ssl->buffers.altKey = NULL; + return BAD_MUTEX_E; + } #else if (ctx->altPrivateKey != NULL) { ret = AllocCopyDer(&ssl->buffers.altKey, ctx->altPrivateKey->buffer, @@ -9218,6 +9236,12 @@ void wolfSSL_ResourceFree(WOLFSSL* ssl) #ifndef NO_CERTS ssl->keepCert = 0; /* make sure certificate is free'd */ wolfSSL_UnloadCertsKeys(ssl); + FreeSslDer(&ssl->buffers.certificate, ssl->buffers.weOwnCert); + FreeSslDer(&ssl->buffers.certChain, ssl->buffers.weOwnCertChain); + FreeSslDer(&ssl->buffers.key, ssl->buffers.weOwnKey); +#ifdef WOLFSSL_DUAL_ALG_CERTS + FreeSslDer(&ssl->buffers.altKey, ssl->buffers.weOwnAltKey); +#endif #endif #ifndef NO_RSA FreeKey(ssl, DYNAMIC_TYPE_RSA, (void**)&ssl->peerRsaKey); @@ -29047,6 +29071,9 @@ const char* wolfSSL_ERR_reason_error_string(unsigned long e) case RPK_UNTRUSTED_E: return "RFC 7250 Raw Public Key not trusted"; + + case CTX_BUSY_E: + return "Context in use by active sessions, operation not allowed"; } return "unknown error number"; @@ -43830,8 +43857,13 @@ 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* cbCtx = ssl->ctx; + byte inSniPrev = cbCtx->inSniCallback; + WOLFSSL_MSG("Calling custom sni callback"); - sniRet = ssl->ctx->sniRecvCb(ssl, &ad, ssl->ctx->sniRecvCbArg); + cbCtx->inSniCallback = 1; + sniRet = cbCtx->sniRecvCb(ssl, &ad, cbCtx->sniRecvCbArg); + cbCtx->inSniCallback = inSniPrev; switch (sniRet) { case warning_return: WOLFSSL_MSG("Error in custom sni callback. Warning alert"); diff --git a/src/ssl.c b/src/ssl.c index f5a309552c1..c4efa7e1212 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -12727,30 +12727,14 @@ void wolfSSL_certs_clear(WOLFSSL* ssl) if (ssl == NULL) return; - /* ctx still owns certificate, certChain, key, dh, and cm */ - if (ssl->buffers.weOwnCert) { - FreeDer(&ssl->buffers.certificate); - ssl->buffers.weOwnCert = 0; - } - ssl->buffers.certificate = NULL; - if (ssl->buffers.weOwnCertChain) { - FreeDer(&ssl->buffers.certChain); - ssl->buffers.weOwnCertChain = 0; - } - ssl->buffers.certChain = NULL; + FreeSslDer(&ssl->buffers.certificate, ssl->buffers.weOwnCert); + FreeSslDer(&ssl->buffers.certChain, ssl->buffers.weOwnCertChain); #ifdef WOLFSSL_TLS13 ssl->buffers.certChainCnt = 0; #endif - if (ssl->buffers.weOwnKey) { - FreeDer(&ssl->buffers.key); - #ifdef WOLFSSL_BLIND_PRIVATE_KEY - FreeDer(&ssl->buffers.keyMask); - #endif - ssl->buffers.weOwnKey = 0; - } - ssl->buffers.key = NULL; + FreeSslDer(&ssl->buffers.key, ssl->buffers.weOwnKey); #ifdef WOLFSSL_BLIND_PRIVATE_KEY - ssl->buffers.keyMask = NULL; + FreeDer(&ssl->buffers.keyMask); #endif ssl->buffers.keyType = 0; ssl->buffers.keyId = 0; @@ -12758,16 +12742,9 @@ void wolfSSL_certs_clear(WOLFSSL* ssl) ssl->buffers.keySz = 0; ssl->buffers.keyDevId = 0; #ifdef WOLFSSL_DUAL_ALG_CERTS - if (ssl->buffers.weOwnAltKey) { - FreeDer(&ssl->buffers.altKey); - #ifdef WOLFSSL_BLIND_PRIVATE_KEY - FreeDer(&ssl->buffers.altKeyMask); - #endif - ssl->buffers.weOwnAltKey = 0; - } - ssl->buffers.altKey = NULL; + FreeSslDer(&ssl->buffers.altKey, ssl->buffers.weOwnAltKey); #ifdef WOLFSSL_BLIND_PRIVATE_KEY - ssl->buffers.altKeyMask = NULL; + FreeDer(&ssl->buffers.altKeyMask); #endif #endif /* WOLFSSL_DUAL_ALG_CERTS */ } @@ -12830,6 +12807,10 @@ long wolfSSL_CTX_ctrl(WOLFSSL_CTX* ctx, int cmd, long opt, void* pt) ret = WOLFSSL_FAILURE; break; } + if (wolfssl_ctx_certs_locked(ctx)) { + ret = CTX_BUSY_E; + break; + } /* Clear certificate chain */ FreeDer(&ctx->certChain); if (sk) { diff --git a/src/ssl_load.c b/src/ssl_load.c index b63a3b84300..357094a618d 100644 --- a/src/ssl_load.c +++ b/src/ssl_load.c @@ -242,10 +242,8 @@ static int ProcessUserChainRetain(WOLFSSL_CTX* ctx, WOLFSSL* ssl, /* Store in SSL object if available. */ if (ssl != NULL) { - /* Dispose of old chain if not reference to context's. */ - if (ssl->buffers.weOwnCertChain) { - FreeDer(&ssl->buffers.certChain); - } + /* Let go of the old chain before taking on a new one. */ + FreeSslDer(&ssl->buffers.certChain, ssl->buffers.weOwnCertChain); /* Allocate and copy the buffer into SSL object. */ ret = AllocCopyDer(&ssl->buffers.certChain, chainBuffer, len, type, heap); @@ -1320,13 +1318,12 @@ static int ProcessBufferPrivKeyHandleDer(WOLFSSL_CTX* ctx, WOLFSSL* ssl, else #endif /* WOLFSSL_DUAL_ALG_CERTS */ if (ssl != NULL) { - /* Dispose of previous key if not context's. */ - if (ssl->buffers.weOwnKey) { - FreeDer(&ssl->buffers.key); - #ifdef WOLFSSL_BLIND_PRIVATE_KEY - FreeDer(&ssl->buffers.keyMask); - #endif - } + /* Let go of the old key before taking on a new one. */ + FreeSslDer(&ssl->buffers.key, ssl->buffers.weOwnKey); + #ifdef WOLFSSL_BLIND_PRIVATE_KEY + /* The mask is always this object's own. */ + FreeDer(&ssl->buffers.keyMask); + #endif ssl->buffers.keyId = 0; ssl->buffers.keyLabel = 0; ssl->buffers.keyDevId = INVALID_DEVID; @@ -2224,15 +2221,15 @@ static int ProcessBufferCertHandleDer(WOLFSSL_CTX* ctx, WOLFSSL* ssl, /* Leaf certificate - our certificate. */ else if (type == CERT_TYPE) { if (ssl != NULL) { - /* Free previous certificate if we own it. */ + #ifdef KEEP_OUR_CERT if (ssl->buffers.weOwnCert) { - FreeDer(&ssl->buffers.certificate); - #ifdef KEEP_OUR_CERT /* Dispose of X509 version of certificate. */ wolfSSL_X509_free(ssl->ourCert); ssl->ourCert = NULL; - #endif } + #endif + /* Let go of the old certificate before taking on a new one. */ + FreeSslDer(&ssl->buffers.certificate, ssl->buffers.weOwnCert); /* Store certificate as ours. */ ssl->buffers.certificate = der; #ifdef KEEP_OUR_CERT @@ -2377,6 +2374,72 @@ static int ProcessBufferResetSuites(WOLFSSL_CTX* ctx, WOLFSSL* ssl, int type) ((type) == ALT_PRIVATEKEY_TYPE)) #endif +/* Whether sessions created from this context are still around, and thus + * buffers may still be shared. + * + * @param [in] ctx SSL context object. + * @return 1 when sessions exist, 0 otherwise. + */ +static int wolfssl_ctx_has_sessions(WOLFSSL_CTX* ctx) +{ + int inUse = 0; + + if (ctx != NULL) { + if (wolfSSL_RefWithMutexLock(&ctx->ref) != 0) { + /* Lock failed, assume it is still used. */ + inUse = 1; + } + else { + inUse = (ctx->ref.count > 1); + (void)wolfSSL_RefWithMutexUnlock(&ctx->ref); + } + } + + return inUse; +} + +/* Whether the context's certificate and key may be changed right now. + * + * @param [in] ctx SSL context object. + * @return 1 when the certificate and key must not be changed, 0 otherwise. + */ +static int wolfssl_ctx_certs_locked(WOLFSSL_CTX* ctx) +{ + int locked = 0; + + if (ctx != NULL) { +#ifdef HAVE_SNI + /* Changing the context cert/key from the SNI callback would not affect + * the in-flight session. */ + locked = ctx->inSniCallback; +#endif + /* Fall back to the ref.count check */ +#if !defined(WOLFSSL_DER_REFCOUNT) && \ + (!defined(WOLFSSL_COPY_CERT) || !defined(WOLFSSL_COPY_KEY)) + if (!locked) { + locked = wolfssl_ctx_has_sessions(ctx); + } +#endif + } + + return locked; +} + +#ifndef NO_DH +/* Whether the context's DH parameters may be changed right now. + * + * They are not reference counted, so they must not be replaced while sessions + * created from this context still point at them. + * + * @param [in] ctx SSL context object. + * @return 1 when the DH parameters must not be changed, 0 otherwise. + */ +static int wolfssl_ctx_dh_locked(WOLFSSL_CTX* ctx) +{ + return wolfssl_ctx_has_sessions(ctx); +} +#endif + /* Process a buffer of data. * * Data type is a private key or a certificate. @@ -2435,6 +2498,11 @@ int ProcessBuffer(WOLFSSL_CTX* ctx, const unsigned char* buff, long sz, if ((ret == 0) && (sz < 0)) { ret = BAD_FUNC_ARG; } + if ((ret == 0) && (ssl == NULL) && + ((type == CERT_TYPE) || IS_PRIVKEY_TYPE(type)) && + wolfssl_ctx_certs_locked(ctx)) { + ret = CTX_BUSY_E; + } #ifdef WOLFSSL_SMALL_STACK if (ret == 0) { @@ -3206,6 +3274,10 @@ int wolfSSL_CTX_use_certificate_file(WOLFSSL_CTX* ctx, const char* file, WOLFSSL_ENTER("wolfSSL_CTX_use_certificate_file"); + if (wolfssl_ctx_certs_locked(ctx)) { + return CTX_BUSY_E; + } + ret = ProcessFile(ctx, file, format, CERT_TYPE, NULL, 0, NULL, GET_VERIFY_SETTING_CTX(ctx)); @@ -3231,6 +3303,10 @@ int wolfSSL_CTX_use_PrivateKey_file(WOLFSSL_CTX* ctx, const char* file, WOLFSSL_ENTER("wolfSSL_CTX_use_PrivateKey_file"); + if (wolfssl_ctx_certs_locked(ctx)) { + return CTX_BUSY_E; + } + ret = ProcessFile(ctx, file, format, PRIVATEKEY_TYPE, NULL, 0, NULL, GET_VERIFY_SETTING_CTX(ctx)); @@ -3255,6 +3331,10 @@ int wolfSSL_CTX_use_AltPrivateKey_file(WOLFSSL_CTX* ctx, const char* file, WOLFSSL_ENTER("wolfSSL_CTX_use_AltPrivateKey_file"); + if (wolfssl_ctx_certs_locked(ctx)) { + return CTX_BUSY_E; + } + ret = ProcessFile(ctx, file, format, ALT_PRIVATEKEY_TYPE, NULL, 0, NULL, GET_VERIFY_SETTING_CTX(ctx)); @@ -3279,6 +3359,10 @@ int wolfSSL_CTX_use_certificate_chain_file(WOLFSSL_CTX* ctx, const char* file) /* process up to MAX_CHAIN_DEPTH plus subject cert */ WOLFSSL_ENTER("wolfSSL_CTX_use_certificate_chain_file"); + if (wolfssl_ctx_certs_locked(ctx)) { + return CTX_BUSY_E; + } + #ifdef WOLFSSL_PEM_TO_DER ret = ProcessFile(ctx, file, WOLFSSL_FILETYPE_PEM, CERT_TYPE, NULL, 1, NULL, GET_VERIFY_SETTING_CTX(ctx)); @@ -3309,6 +3393,10 @@ int wolfSSL_CTX_use_certificate_chain_file_format(WOLFSSL_CTX* ctx, WOLFSSL_ENTER("wolfSSL_CTX_use_certificate_chain_file_format"); + if (wolfssl_ctx_certs_locked(ctx)) { + return CTX_BUSY_E; + } + ret = ProcessFile(ctx, file, format, CERT_TYPE, NULL, 1, NULL, GET_VERIFY_SETTING_CTX(ctx)); @@ -4211,6 +4299,9 @@ int wolfSSL_CTX_use_PrivateKey_Id(WOLFSSL_CTX* ctx, const unsigned char* id, if (ctx == NULL || id == NULL || sz < 0) { return 0; } + if (wolfssl_ctx_certs_locked(ctx)) { + return CTX_BUSY_E; + } /* Dispose of old private key and allocate and copy in id. */ FreeDer(&ctx->privateKey); @@ -4287,6 +4378,9 @@ int wolfSSL_CTX_use_PrivateKey_Label(WOLFSSL_CTX* ctx, const char* label, if (ctx == NULL || label == NULL) { return 0; } + if (wolfssl_ctx_certs_locked(ctx)) { + return CTX_BUSY_E; + } sz = (word32)XSTRLEN(label) + 1; @@ -4330,6 +4424,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) && wolfssl_ctx_certs_locked(ctx)) { + ret = CTX_BUSY_E; + } if (ret == 1) { FreeDer(&ctx->altPrivateKey); @@ -4380,6 +4477,9 @@ int wolfSSL_CTX_use_AltPrivateKey_Label(WOLFSSL_CTX* ctx, const char* label, if ((ctx == NULL) || (label == NULL)) { ret = 0; } + if ((ret == 1) && wolfssl_ctx_certs_locked(ctx)) { + ret = CTX_BUSY_E; + } if (ret == 1) { sz = (word32)XSTRLEN(label) + 1; @@ -4624,13 +4724,11 @@ int wolfSSL_use_PrivateKey_Id(WOLFSSL* ssl, const unsigned char* id, return 0; } - /* Dispose of old private key if owned and allocate and copy in id. */ - if (ssl->buffers.weOwnKey) { - FreeDer(&ssl->buffers.key); - #ifdef WOLFSSL_BLIND_PRIVATE_KEY - FreeDer(&ssl->buffers.keyMask); - #endif - } + /* Let go of the old key, then allocate and copy in the id. */ + FreeSslDer(&ssl->buffers.key, ssl->buffers.weOwnKey); +#ifdef WOLFSSL_BLIND_PRIVATE_KEY + FreeDer(&ssl->buffers.keyMask); +#endif if (AllocCopyDer(&ssl->buffers.key, id, (word32)sz, PRIVATEKEY_TYPE, ssl->heap) != 0) { ret = 0; @@ -4700,13 +4798,11 @@ int wolfSSL_use_PrivateKey_Label(WOLFSSL* ssl, const char* label, int devId) sz = (word32)XSTRLEN(label) + 1; - /* Dispose of old private key if owned and allocate and copy in label. */ - if (ssl->buffers.weOwnKey) { - FreeDer(&ssl->buffers.key); - #ifdef WOLFSSL_BLIND_PRIVATE_KEY - FreeDer(&ssl->buffers.keyMask); - #endif - } + /* Let go of the old key, then allocate and copy in the label. */ + FreeSslDer(&ssl->buffers.key, ssl->buffers.weOwnKey); +#ifdef WOLFSSL_BLIND_PRIVATE_KEY + FreeDer(&ssl->buffers.keyMask); +#endif if (AllocCopyDer(&ssl->buffers.key, (const byte*)label, (word32)sz, PRIVATEKEY_TYPE, ssl->heap) != 0) { ret = 0; @@ -4954,6 +5050,10 @@ static int wolfssl_ctx_add_to_chain(WOLFSSL_CTX* ctx, const byte* der, int ret; DerBuffer* derBuffer = NULL; + if (wolfssl_ctx_certs_locked(ctx)) { + return CTX_BUSY_E; + } + /* Create a DER buffer from DER encoding. */ ret = AllocCopyDer(&derBuffer, der, (word32)derSz, CERT_TYPE, ctx->heap); if (ret != 0) { @@ -5063,6 +5163,9 @@ int wolfSSL_CTX_use_certificate(WOLFSSL_CTX *ctx, WOLFSSL_X509 *x) WOLFSSL_MSG("Bad parameter"); res = 0; } + if ((res == 1) && wolfssl_ctx_certs_locked(ctx)) { + res = CTX_BUSY_E; + } if (res == 1) { /* Replace certificate buffer with one holding the new certificate. */ @@ -5752,6 +5855,10 @@ static int wolfssl_ctx_set_tmp_dh(WOLFSSL_CTX* ctx, unsigned char* p, int pSz, if ((ctx == NULL) || (p == NULL) || (g == NULL)) ret = BAD_FUNC_ARG; + if ((ret == 1) && wolfssl_ctx_dh_locked(ctx)) { + ret = CTX_BUSY_E; + } + /* Check the size of the prime meets the requirements of the SSL context. */ if (ret == 1) { if (((word16)pSz < ctx->minDhKeySz) || ((word16)pSz > ctx->maxDhKeySz)) { diff --git a/src/tls13.c b/src/tls13.c index d0e3b5b3419..14586a24544 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -10146,20 +10146,28 @@ static int SendTls13CertificateVerify(WOLFSSL* ssl) } ssl->buffers.keyType = ssl->buffers.altKeyType; ssl->buffers.keySz = ssl->buffers.altKeySz; - /* If we own it, free key before overriding it. */ - if (ssl->buffers.weOwnKey) { - FreeDer(&ssl->buffers.key); - #ifdef WOLFSSL_BLIND_PRIVATE_KEY - FreeDer(&ssl->buffers.keyMask); - #endif - } + /* Let go of the current key before overriding it. */ + FreeSslDer(&ssl->buffers.key, ssl->buffers.weOwnKey); + #ifdef WOLFSSL_BLIND_PRIVATE_KEY + FreeDer(&ssl->buffers.keyMask); + #endif - /* Swap keys */ + /* Swap keys. Both now name the same buffer, so take a + * hold for the new alias. */ ssl->buffers.key = ssl->buffers.altKey; ssl->buffers.weOwnKey = ssl->buffers.weOwnAltKey; + if (!RefDer(ssl->buffers.key)) { + ssl->buffers.key = NULL; + ssl->buffers.weOwnKey = 0; + ERROR_OUT(BAD_MUTEX_E, exit_scv); + } #ifdef WOLFSSL_BLIND_PRIVATE_KEY ssl->buffers.keyMask = ssl->buffers.altKeyMask; + if (!RefDer(ssl->buffers.keyMask)) { + ssl->buffers.keyMask = NULL; + ERROR_OUT(BAD_MUTEX_E, exit_scv); + } /* Unblind the alternative key before decoding */ wolfssl_priv_der_blind_toggle(ssl->buffers.key, ssl->buffers.keyMask); #endif diff --git a/tests/api.c b/tests/api.c index 238ea673976..73b8675e145 100644 --- a/tests/api.c +++ b/tests/api.c @@ -4007,6 +4007,7 @@ static int test_wolfSSL_CTX_use_certificate_chain_buffer_format(void) !defined(NO_WOLFSSL_CLIENT) && !defined(NO_RSA) && \ (!defined(NO_FILESYSTEM) || defined(USE_CERT_BUFFERS_2048)) WOLFSSL_CTX* ctx = NULL; + WOLFSSL_CTX* ctx2 = NULL; WOLFSSL* ssl = NULL; #ifndef NO_FILESYSTEM const char* cert = svrCertFile; @@ -4017,7 +4018,10 @@ static int test_wolfSSL_CTX_use_certificate_chain_buffer_format(void) #endif ExpectNotNull(ctx = wolfSSL_CTX_new(wolfSSLv23_client_method())); - ExpectNotNull(ssl = wolfSSL_new(ctx)); + /* The session goes on its own context so the one under test stays free of + * sessions and its loaders behave normally. */ + ExpectNotNull(ctx2 = wolfSSL_CTX_new(wolfSSLv23_client_method())); + ExpectNotNull(ssl = wolfSSL_new(ctx2)); /* Invalid parameters. */ #ifndef NO_FILESYSTEM @@ -4071,6 +4075,7 @@ static int test_wolfSSL_CTX_use_certificate_chain_buffer_format(void) wolfSSL_free(ssl); wolfSSL_CTX_free(ctx); + wolfSSL_CTX_free(ctx2); #ifndef NO_FILESYSTEM if (buf != NULL) { XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER); @@ -5093,14 +5098,23 @@ static int test_wolfSSL_SetTmpDH_file(void) CERT_FILETYPE)); #if defined(WOLFSSL_WPAS) && !defined(NO_DSA) if (EXPECT_SUCCESS()) { - if (ctx->minDhKeySz <= 128 /* bytes */) { - ExpectIntEQ(WOLFSSL_SUCCESS, wolfSSL_CTX_SetTmpDH_file(ctx, dsaParamFile, - CERT_FILETYPE)); - } - else { - ExpectIntEQ(DH_KEY_SIZE_E, wolfSSL_CTX_SetTmpDH_file(ctx, dsaParamFile, - CERT_FILETYPE)); + /* DH parameters are aliased by the active session, so they are set on + * a context with no session; the one above is refused. */ + WOLFSSL_CTX* ctxDh = wolfSSL_CTX_new(wolfSSLv23_server_method()); + ExpectNotNull(ctxDh); + ExpectIntEQ(CTX_BUSY_E, wolfSSL_CTX_SetTmpDH_file(ctx, dsaParamFile, + CERT_FILETYPE)); + if (ctxDh != NULL) { + if (ctxDh->minDhKeySz <= 128 /* bytes */) { + ExpectIntEQ(WOLFSSL_SUCCESS, wolfSSL_CTX_SetTmpDH_file(ctxDh, + dsaParamFile, CERT_FILETYPE)); + } + else { + ExpectIntEQ(DH_KEY_SIZE_E, wolfSSL_CTX_SetTmpDH_file(ctxDh, + dsaParamFile, CERT_FILETYPE)); + } } + wolfSSL_CTX_free(ctxDh); } #endif @@ -13049,6 +13063,7 @@ static int test_wolfSSL_tmp_dh(void) BIO* bio = NULL; SSL* ssl = NULL; SSL_CTX* ctx = NULL; + SSL_CTX* ctxDh = NULL; #ifndef NO_WOLFSSL_CLIENT SSL* ssl_c = NULL; SSL_CTX* ctx_c = NULL; @@ -13107,13 +13122,20 @@ static int test_wolfSSL_tmp_dh(void) WC_NO_ERR_TRACE(BAD_FUNC_ARG)); ExpectIntEQ((int)wolfSSL_CTX_SetTmpDH(NULL, p , 0, g , 0), WC_NO_ERR_TRACE(BAD_FUNC_ARG)); - ExpectIntEQ((int)wolfSSL_CTX_SetTmpDH(ctx , p , 1, g , 1), + /* The parameters are only looked at on a context with no sessions; one + * that has them refuses the change outright (checked further down). */ +#ifndef NO_WOLFSSL_SERVER + ExpectNotNull(ctxDh = SSL_CTX_new(wolfSSLv23_server_method())); +#else + ExpectNotNull(ctxDh = SSL_CTX_new(wolfSSLv23_client_method())); +#endif + ExpectIntEQ((int)wolfSSL_CTX_SetTmpDH(ctxDh, p , 1, g , 1), WC_NO_ERR_TRACE(DH_KEY_SIZE_E)); - ExpectIntEQ((int)wolfSSL_CTX_SetTmpDH(ctx , buff, 6000, g , 1), + ExpectIntEQ((int)wolfSSL_CTX_SetTmpDH(ctxDh, buff, 6000, g , 1), WC_NO_ERR_TRACE(DH_KEY_SIZE_E)); #if !defined(WOLFSSL_OLD_PRIME_CHECK) && !defined(HAVE_FIPS) && \ !defined(HAVE_SELFTEST) - ExpectIntEQ((int)wolfSSL_CTX_SetTmpDH(ctx, bad_p, pSz, g, gSz), + ExpectIntEQ((int)wolfSSL_CTX_SetTmpDH(ctxDh, bad_p, pSz, g, gSz), WC_NO_ERR_TRACE(DH_CHECK_PUB_E)); #endif ExpectIntEQ((int)wolfSSL_SetTmpDH(NULL, NULL, 0, NULL, 0), @@ -13159,9 +13181,13 @@ static int test_wolfSSL_tmp_dh(void) DH_free(dh2); dh2 = NULL; + /* DH parameters are shared with the sessions and are not reference + * counted, so a context with sessions refuses to change them. */ ExpectIntEQ((int)wolfSSL_CTX_SetTmpDH(ctx, p, pSz, g, gSz), + WC_NO_ERR_TRACE(CTX_BUSY_E)); + ExpectIntEQ((int)wolfSSL_CTX_SetTmpDH(ctxDh, p, pSz, g, gSz), WOLFSSL_SUCCESS); - ExpectIntEQ((int)SSL_CTX_set_tmp_dh(ctx, dh), WOLFSSL_SUCCESS); + ExpectIntEQ((int)SSL_CTX_set_tmp_dh(ctxDh, dh), WOLFSSL_SUCCESS); #ifndef NO_WOLFSSL_SERVER ExpectIntEQ((int)SSL_set_tmp_dh(ssl, dh), WOLFSSL_SUCCESS); #else @@ -13183,6 +13209,7 @@ static int test_wolfSSL_tmp_dh(void) } #endif SSL_CTX_free(ctx); + SSL_CTX_free(ctxDh); #endif return EXPECT_RESULT(); } diff --git a/tests/api/test_tls13.c b/tests/api/test_tls13.c index ed5ce88edcb..fcc3b2cec1c 100644 --- a/tests/api/test_tls13.c +++ b/tests/api/test_tls13.c @@ -3691,6 +3691,181 @@ int test_tls13_pha(void) return EXPECT_RESULT(); } + +/* A server whose certificate has been renewed on disk reloads it on the + * context while it is still serving. Sessions already running keep the + * certificate they started with; sessions made afterwards get the new one. */ +int test_tls13_ctx_cert_rotation(void) +{ + EXPECT_DECLS; +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(WOLFSSL_TLS13) && \ + !defined(NO_RSA) && defined(WOLFSSL_DER_REFCOUNT) && \ + !defined(WOLFSSL_COPY_CERT) + WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL; + WOLFSSL *ssl_c = NULL, *ssl_s = NULL; + WOLFSSL *ssl_s2 = NULL; + struct test_memio_ctx test_ctx; + DerBuffer* startCert = NULL; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0); + + if (ssl_s != NULL) { + startCert = ssl_s->buffers.certificate; + } + ExpectNotNull(startCert); + + /* This is only interesting when the session shares the context's buffer. + * Builds that copy it (WOLFSSL_COPY_CERT) are safe with no reference + * count, so there is nothing to check. */ + if (ssl_s != NULL && ssl_s->buffers.certificate == ctx_s->certificate) { + /* Reload it on the context. This is what a server does once the + * renewed certificate lands on disk, and it has to work while + * sessions run. */ + ExpectIntEQ(wolfSSL_CTX_use_certificate_file(ctx_s, svrCertFile, + CERT_FILETYPE), WOLFSSL_SUCCESS); + + /* The context holds a new buffer now. The running session still points + * at the one it started with, which its reference keeps alive. */ + ExpectPtrNE(ctx_s->certificate, startCert); + ExpectPtrEq(ssl_s->buffers.certificate, startCert); + ExpectNotNull(ssl_s->buffers.certificate->buffer); + + /* It finishes its handshake with that certificate. */ + ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); + + /* A session made after the reload picks up the new certificate. */ + ExpectNotNull(ssl_s2 = wolfSSL_new(ctx_s)); + ExpectPtrEq(ssl_s2->buffers.certificate, ctx_s->certificate); + ExpectPtrNE(ssl_s2->buffers.certificate, startCert); + } + + wolfSSL_free(ssl_s2); + wolfSSL_free(ssl_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); +#endif + return EXPECT_RESULT(); +} + + +/* Without reference counting the old buffer would be freed while running + * sessions still point at it, so the reload is refused instead. */ +int test_tls13_ctx_cert_rotation_refused(void) +{ + EXPECT_DECLS; +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(WOLFSSL_TLS13) && \ + !defined(NO_RSA) && !defined(WOLFSSL_DER_REFCOUNT) && \ + !defined(WOLFSSL_COPY_CERT) + WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL; + WOLFSSL *ssl_c = NULL, *ssl_s = NULL; + struct test_memio_ctx test_ctx; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0); + + ExpectIntEQ(wolfSSL_CTX_use_certificate_file(ctx_s, svrCertFile, + CERT_FILETYPE), CTX_BUSY_E); + ExpectIntEQ(wolfSSL_CTX_use_PrivateKey_file(ctx_s, svrKeyFile, + CERT_FILETYPE), CTX_BUSY_E); + + /* Allowed again once the session is gone. */ + wolfSSL_free(ssl_s); + ssl_s = NULL; + ExpectIntEQ(wolfSSL_CTX_use_certificate_file(ctx_s, svrCertFile, + CERT_FILETYPE), WOLFSSL_SUCCESS); + + wolfSSL_free(ssl_c); + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); +#endif + return EXPECT_RESULT(); +} + + +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(WOLFSSL_TLS13) && \ + !defined(NO_RSA) && defined(HAVE_SNI) +/* Change the context certificate from the SNI callback. The session that is + * handshaking took its certificate pointer when it was created, so this can + * never reach it and is refused. */ +static int test_sni_cert_change_cb(WOLFSSL* ssl, int* ad, void* arg) +{ + WOLFSSL_CTX* ctx = wolfSSL_get_SSL_CTX(ssl); + int* cbRet = (int*)arg; + (void)ad; + /* Report the result back to the parent function below. */ + *cbRet = wolfSSL_CTX_use_certificate_file(ctx, svrCertFile, CERT_FILETYPE); + /* 0 means ack the servername and continue the handshake. */ + return 0; +} +#endif + +int test_tls13_sni_cb_cert_change_refused(void) +{ + EXPECT_DECLS; +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(WOLFSSL_TLS13) && \ + !defined(NO_RSA) && defined(HAVE_SNI) + WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL; + WOLFSSL *ssl_c = NULL, *ssl_s = NULL; + struct test_memio_ctx test_ctx; + const char* host = "example.com"; + int cbRet = WOLFSSL_FATAL_ERROR; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0); + + wolfSSL_CTX_set_servername_callback(ctx_s, test_sni_cert_change_cb); + ExpectIntEQ(wolfSSL_CTX_set_servername_arg(ctx_s, &cbRet), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_UseSNI(ssl_c, WOLFSSL_SNI_HOST_NAME, host, + (word16)XSTRLEN(host)), WOLFSSL_SUCCESS); + + /* Refused, so nothing changes and the handshake runs to completion. */ + ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); + ExpectIntEQ(cbRet, CTX_BUSY_E); + + wolfSSL_free(ssl_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); +#endif + return EXPECT_RESULT(); +} + + +/* DH parameters are not reference counted, so they may not be replaced while + * sessions created from the context still point at them. */ +int test_tls13_ctx_dh_change_refused(void) +{ + EXPECT_DECLS; +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(WOLFSSL_TLS13) && \ + !defined(NO_RSA) && !defined(NO_DH) + WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL; + WOLFSSL *ssl_c = NULL, *ssl_s = NULL; + struct test_memio_ctx test_ctx; + /* Refused before the parameters are looked at, so any non-NULL will do. */ + static const unsigned char p[] = { 0x01 }; + static const unsigned char g[] = { 0x02 }; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0); + + ExpectIntEQ(wolfSSL_CTX_SetTmpDH(ctx_s, p, (int)sizeof(p), g, + (int)sizeof(g)), CTX_BUSY_E); + + wolfSSL_free(ssl_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); +#endif + return EXPECT_RESULT(); +} + + #if defined(HAVE_RPK) && defined(WOLFSSL_TLS13) && \ !defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER) && \ !defined(NO_SHA256) diff --git a/tests/api/test_tls13.h b/tests/api/test_tls13.h index fdb733d1ee2..2cfd961c3c4 100644 --- a/tests/api/test_tls13.h +++ b/tests/api/test_tls13.h @@ -30,6 +30,10 @@ int test_tls13_bad_psk_binder(void); int test_tls13_rpk_handshake(void); int test_tls13_rpk_handshake_no_negotiation(void); int test_tls13_pha(void); +int test_tls13_ctx_cert_rotation(void); +int test_tls13_ctx_cert_rotation_refused(void); +int test_tls13_sni_cb_cert_change_refused(void); +int test_tls13_ctx_dh_change_refused(void); int test_tls13_rpk_untrusted(void); int test_tls13_rpk_trust(void); int test_tls13_pq_groups(void); @@ -110,6 +114,10 @@ int test_tls13_pqc_hybrid_async_server(void); TEST_DECL_GROUP("tls13", test_tls13_rpk_handshake), \ TEST_DECL_GROUP("tls13", test_tls13_rpk_handshake_no_negotiation), \ TEST_DECL_GROUP("tls13", test_tls13_pha), \ + TEST_DECL_GROUP("tls13", test_tls13_ctx_cert_rotation), \ + TEST_DECL_GROUP("tls13", test_tls13_ctx_cert_rotation_refused), \ + TEST_DECL_GROUP("tls13", test_tls13_sni_cb_cert_change_refused), \ + TEST_DECL_GROUP("tls13", test_tls13_ctx_dh_change_refused), \ TEST_DECL_GROUP("tls13", test_tls13_rpk_untrusted), \ TEST_DECL_GROUP("tls13", test_tls13_rpk_trust), \ TEST_DECL_GROUP("tls13", test_tls13_pq_groups), \ diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 8b870e3ce0b..0191e64ddc6 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -24879,6 +24879,17 @@ int AllocDer(DerBuffer** pDer, word32 length, int type, void* heap) der->heap = heap; der->buffer = (byte*)der + sizeof(DerBuffer); der->length = length; +#ifdef WOLFSSL_DER_REFCOUNT + /* Buffer starts with a single holder. */ + wolfSSL_RefInit(&der->ref, &ret); + #ifdef WOLFSSL_REFCNT_ERROR_RETURN + if (ret != 0) { + XFREE(der, heap, dynType); + *pDer = NULL; + return ret; + } + #endif +#endif ret = 0; /* Success */ } else { ret = BAD_FUNC_ARG; @@ -24886,6 +24897,27 @@ int AllocDer(DerBuffer** pDer, word32 length, int type, void* heap) return ret; } +/* Inc refcount on a DER buffer. + * + * Without WOLFSSL_DER_REFCOUNT this is a no-op. + * + * @param [in, out] der DER buffer. May be NULL. + * @return 1 on success. + * @return 0 when the reference could not be taken. + */ +int RefDer(DerBuffer* der) +{ + int err = 0; +#ifdef WOLFSSL_DER_REFCOUNT + if (der != NULL) { + wolfSSL_RefInc(&der->ref, &err); + } +#else + (void)der; +#endif + return !err; +} + int AllocCopyDer(DerBuffer** pDer, const unsigned char* buff, word32 length, int type, void* heap) { @@ -24901,15 +24933,29 @@ void FreeDer(DerBuffer** pDer) { if (pDer && *pDer) { DerBuffer* der = (DerBuffer*)*pDer; + int isZero = 1; +#ifdef WOLFSSL_DER_REFCOUNT + int ref_err = 0; + wolfSSL_RefDec(&der->ref, &isZero, &ref_err); + /* If error, don't free. Leak is better than UAF. */ + if (ref_err != 0) { + isZero = 0; + } +#endif - /* ForceZero private keys */ - if (((der->type == PRIVATEKEY_TYPE) || - (der->type == ALT_PRIVATEKEY_TYPE)) && der->buffer != NULL) { - ForceZero(der->buffer, der->length); + if (isZero) { + /* ForceZero private keys */ + if (((der->type == PRIVATEKEY_TYPE) || + (der->type == ALT_PRIVATEKEY_TYPE)) && der->buffer != NULL) { + ForceZero(der->buffer, der->length); + } + der->buffer = NULL; + der->length = 0; +#ifdef WOLFSSL_DER_REFCOUNT + wolfSSL_RefFree(&der->ref); +#endif + XFREE(der, der->heap, der->dynType); } - der->buffer = NULL; - der->length = 0; - XFREE(der, der->heap, der->dynType); *pDer = NULL; } diff --git a/wolfssl/error-ssl.h b/wolfssl/error-ssl.h index 2783f00813b..6df29aa813a 100644 --- a/wolfssl/error-ssl.h +++ b/wolfssl/error-ssl.h @@ -250,7 +250,10 @@ enum wolfSSL_ErrorCodes { RPK_UNTRUSTED_E = -521, /* RFC 7250 Raw Public Key not trusted * out of band */ - WOLFSSL_LAST_E = -521 + CTX_BUSY_E = -522, /* Context in use by active sessions; + * cannot change certs, keys or DH */ + + WOLFSSL_LAST_E = -522 /* codes -1000 to -1999 are reserved for wolfCrypt. */ }; diff --git a/wolfssl/internal.h b/wolfssl/internal.h index d54496e0f9c..9f7e016fcc1 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -4240,6 +4240,7 @@ struct WOLFSSL_CTX { #ifdef HAVE_SNI CallbackSniRecv sniRecvCb; void* sniRecvCbArg; + byte inSniCallback; #endif #if defined(WOLFSSL_MULTICAST) && defined(WOLFSSL_DTLS) CallbackMcastHighwater mcastHwCb; /* Sequence number highwater callback */ @@ -5106,6 +5107,30 @@ typedef struct Buffers { #endif /* HAVE_PK_CALLBACKS */ } Buffers; +/* Dec refcount and maybe free the DER buffer. + * + * pDer is the buffer to release. + * + * weOwn is set when this object allocated its own copy of the buffer, + * and 0 when the buffer is an alias of the buffer owned by the context. +*/ +#ifdef WOLFSSL_DER_REFCOUNT + #define FreeSslDer(pDer, weOwn) \ + do { \ + FreeDer(pDer); \ + (weOwn) = 0; \ + } while (0) +#else + #define FreeSslDer(pDer, weOwn) \ + do { \ + if (weOwn) { \ + FreeDer(pDer); \ + (weOwn) = 0; \ + } \ + *(pDer) = NULL; \ + } while (0) +#endif + /* sub-states for send/do key share (key exchange) */ enum asyncState { TLS_ASYNC_BEGIN = 0, diff --git a/wolfssl/wolfcrypt/asn.h b/wolfssl/wolfcrypt/asn.h index 009c258742e..9f2bb0646d2 100644 --- a/wolfssl/wolfcrypt/asn.h +++ b/wolfssl/wolfcrypt/asn.h @@ -2834,6 +2834,7 @@ WOLFSSL_API int AllocDer(DerBuffer** der, word32 length, int type, WOLFSSL_LOCAL int AllocCopyDer(DerBuffer** der, const unsigned char* buff, word32 length, int type, void* heap); WOLFSSL_API void FreeDer(DerBuffer** der); +WOLFSSL_LOCAL int RefDer(DerBuffer* der); #ifdef WOLFSSL_ASN_PARSE_KEYUSAGE WOLFSSL_LOCAL int ParseKeyUsageStr(const char* value, word16* keyUsage, diff --git a/wolfssl/wolfcrypt/asn_public.h b/wolfssl/wolfcrypt/asn_public.h index d557ac94cd0..be78e603c69 100644 --- a/wolfssl/wolfcrypt/asn_public.h +++ b/wolfssl/wolfcrypt/asn_public.h @@ -243,6 +243,9 @@ typedef struct DerBuffer { word32 length; int type; /* enum CertType */ int dynType; /* DYNAMIC_TYPE_* */ +#ifdef WOLFSSL_DER_REFCOUNT + wolfSSL_Ref ref; +#endif } DerBuffer; typedef struct WOLFSSL_ASN1_TIME { diff --git a/wolfssl/wolfcrypt/settings.h b/wolfssl/wolfcrypt/settings.h index a814d912b0c..ac2dfe75c17 100644 --- a/wolfssl/wolfcrypt/settings.h +++ b/wolfssl/wolfcrypt/settings.h @@ -4951,6 +4951,24 @@ blinding by defining WC_BLINDING_NO_RNG_ACKNOWLEDGE_WEAKNESS." #define WOLFSSL_BASE64_DECODE #endif +/* Reference count the DER buffers a context shares with its sessions, so the + * certificate or key can be replaced while older sessions still use the one + * they started with. Servers that reload certificates at runtime need this. + * + * It costs a counter in every DerBuffer, so it is left out of minimal builds. + * It is turned on for the builds whose applications reload certificates on a + * live context: the OpenSSL compatibility layer and memcached. An application + * that does the same in another build should define WOLFSSL_DER_REFCOUNT + * itself. It is not needed when each session gets its own copy of the + * certificate and key (WOLFSSL_COPY_CERT/WOLFSSL_COPY_KEY above), which is + * already safe. Define WOLFSSL_NO_DER_REFCOUNT to force it off. */ +#if !defined(WOLFSSL_DER_REFCOUNT) && !defined(WOLFSSL_NO_DER_REFCOUNT) + #if (defined(OPENSSL_EXTRA) || defined(HAVE_MEMCACHED)) && \ + (!defined(WOLFSSL_COPY_CERT) || !defined(WOLFSSL_COPY_KEY)) + #define WOLFSSL_DER_REFCOUNT + #endif +#endif + #if defined(OPENSSL_EXTRA) || defined(OPENSSL_EXTRA_X509_SMALL) || \ defined(HAVE_WEBSERVER) || defined(HAVE_FIPS) || \ defined(HAVE_ECC_CDH) || defined(HAVE_SELFTEST) || \