From ef500c2e6222209b2559c8e6938a00badb2d52c9 Mon Sep 17 00:00:00 2001 From: Colton Willey Date: Tue, 13 Aug 2024 13:32:25 -0700 Subject: [PATCH 1/7] Add new option to always copy cert buffer for each SSL object --- src/internal.c | 30 ++++++++++++++++++++++++++++++ src/ssl.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/ssl_load.c | 11 +++++++++++ tests/api.c | 18 ++++++++++++++++++ 4 files changed, 99 insertions(+) diff --git a/src/internal.c b/src/internal.c index b08e6f771..5ae5382d3 100644 --- a/src/internal.c +++ b/src/internal.c @@ -6803,9 +6803,39 @@ int SetSSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup) #endif /* HAVE_RPK */ #ifndef NO_CERTS +#ifdef WOLFSSL_COPY_CERT + /* If WOLFSSL_COPY_CERT is defined, always copy the cert */ + if (ctx->certificate != NULL) { + if (ssl->buffers.certificate != NULL) { + FreeDer(&ssl->buffers.certificate); + } + ret = AllocCopyDer(&ssl->buffers.certificate, ctx->certificate->buffer, + ctx->certificate->length, ctx->certificate->type, + ctx->certificate->heap); + if (ret != 0) { + return ret; + } + + ret = WOLFSSL_SUCCESS; + } + if (ctx->certChain != NULL) { + if (ssl->buffers.certChain != NULL) { + FreeDer(&ssl->buffers.certChain); + } + ret = AllocCopyDer(&ssl->buffers.certChain, ctx->certChain->buffer, + ctx->certChain->length, ctx->certChain->type, + ctx->certChain->heap); + if (ret != 0) { + return ret; + } + + ret = WOLFSSL_SUCCESS; + } +#else /* ctx still owns certificate, certChain, key, dh, and cm */ ssl->buffers.certificate = ctx->certificate; ssl->buffers.certChain = ctx->certChain; +#endif #ifdef WOLFSSL_TLS13 ssl->buffers.certChainCnt = ctx->certChainCnt; #endif diff --git a/src/ssl.c b/src/ssl.c index afd505027..a0f377bb9 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -10806,6 +10806,11 @@ int wolfSSL_set_compression(WOLFSSL* ssl) return BAD_FUNC_ARG; } + #ifdef WOLFSSL_COPY_CERT + /* If WOLFSSL_COPY_CERT defined, always free cert buffers in SSL obj */ + FreeDer(&ssl->buffers.certificate); + FreeDer(&ssl->buffers.certChain); + #endif if (ssl->buffers.weOwnCert && !ssl->keepCert) { WOLFSSL_MSG("Unloading cert"); FreeDer(&ssl->buffers.certificate); @@ -19549,6 +19554,11 @@ void wolfSSL_certs_clear(WOLFSSL* ssl) /* ctx still owns certificate, certChain, key, dh, and cm */ if (ssl->buffers.weOwnCert) FreeDer(&ssl->buffers.certificate); +#ifdef WOLFSSL_COPY_CERT + /* If WOLFSSL_COPY_CERT defined, always free cert buffers in SSL obj */ + FreeDer(&ssl->buffers.certificate); + FreeDer(&ssl->buffers.certChain); +#endif ssl->buffers.certificate = NULL; if (ssl->buffers.weOwnCertChain) FreeDer(&ssl->buffers.certChain); @@ -20151,9 +20161,39 @@ WOLFSSL_CTX* wolfSSL_set_SSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx) ssl->ctx = ctx; #ifndef NO_CERTS +#ifdef WOLFSSL_COPY_CERT + /* If WOLFSSL_COPY_CERT defined, always make new copy of cert */ + if (ctx->certificate != NULL) { + if (ssl->buffers.certificate != NULL) { + FreeDer(&ssl->buffers.certificate); + } + ret = AllocCopyDer(&ssl->buffers.certificate, ctx->certificate->buffer, + ctx->certificate->length, ctx->certificate->type, + ctx->certificate->heap); + if (ret != 0) { + return NULL; + } + + ret = WOLFSSL_SUCCESS; + } + if (ctx->certChain != NULL) { + if (ssl->buffers.certChain != NULL) { + FreeDer(&ssl->buffers.certChain); + } + ret = AllocCopyDer(&ssl->buffers.certChain, ctx->certChain->buffer, + ctx->certChain->length, ctx->certChain->type, + ctx->certChain->heap); + if (ret != 0) { + return NULL; + } + + ret = WOLFSSL_SUCCESS; + } +#else /* ctx owns certificate, certChain and key */ ssl->buffers.certificate = ctx->certificate; ssl->buffers.certChain = ctx->certChain; +#endif #ifdef WOLFSSL_TLS13 ssl->buffers.certChainCnt = ctx->certChainCnt; #endif diff --git a/src/ssl_load.c b/src/ssl_load.c index da4279e39..ee11273bf 100644 --- a/src/ssl_load.c +++ b/src/ssl_load.c @@ -236,6 +236,9 @@ 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. */ + #ifdef WOLFSSL_COPY_CERT + FreeDer(&ssl->buffers.certChain); + #endif if (ssl->buffers.weOwnCertChain) { FreeDer(&ssl->buffers.certChain); } @@ -2079,6 +2082,10 @@ static int ProcessBufferCertHandleDer(WOLFSSL_CTX* ctx, WOLFSSL* ssl, /* Leaf certificate - our certificate. */ else if (type == CERT_TYPE) { if (ssl != NULL) { +#ifdef WOLFSSL_COPY_CERT + /* Always Free previously set if WOLFSSL_COPY_CERT defined */ + FreeDer(&ssl->buffers.certificate); +#endif /* Free previous certificate if we own it. */ if (ssl->buffers.weOwnCert) { FreeDer(&ssl->buffers.certificate); @@ -4560,6 +4567,10 @@ static int wolfssl_add_to_chain(DerBuffer** chain, int weOwn, const byte* cert, c32to24(certSz, newChain->buffer + len); XMEMCPY(newChain->buffer + len + CERT_HEADER_SZ, cert, certSz); +#ifdef WOLFSSL_COPY_CERT + FreeDer(chain); +#endif + /* Dispose of old chain if we own it. */ if (weOwn) { FreeDer(chain); diff --git a/tests/api.c b/tests/api.c index 39b9933c0..c7fb37b9b 100644 --- a/tests/api.c +++ b/tests/api.c @@ -77291,9 +77291,18 @@ static int test_wolfSSL_set_SSL_CTX(void) #ifdef WOLFSSL_SESSION_ID_CTX ExpectIntEQ(XMEMCMP(ssl->sessionCtx, session_id2, 4), 0); #endif +#ifdef WOLFSSL_COPY_CERT + if (ctx2->certificate != NULL) { + ExpectFalse(ssl->buffers.certificate == ctx2->certificate); + } + if (ctx2->certChain != NULL) { + ExpectFalse(ssl->buffers.certChain == ctx2->certChain); + } +#else ExpectTrue(ssl->buffers.certificate == ctx2->certificate); ExpectTrue(ssl->buffers.certChain == ctx2->certChain); #endif +#endif #ifdef HAVE_SESSION_TICKET ExpectIntNE((wolfSSL_get_options(ssl) & SSL_OP_NO_TICKET), 0); @@ -77310,8 +77319,17 @@ static int test_wolfSSL_set_SSL_CTX(void) #endif /* MUST change */ #ifdef WOLFSSL_INT_H +#ifdef WOLFSSL_COPY_CERT + if (ctx1->certificate != NULL) { + ExpectFalse(ssl->buffers.certificate == ctx1->certificate); + } + if (ctx1->certChain != NULL) { + ExpectFalse(ssl->buffers.certChain == ctx1->certChain); + } +#else ExpectTrue(ssl->buffers.certificate == ctx1->certificate); ExpectTrue(ssl->buffers.certChain == ctx1->certChain); +#endif #ifdef WOLFSSL_SESSION_ID_CTX ExpectIntEQ(XMEMCMP(ssl->sessionCtx, session_id1, 4), 0); #endif From f4decf84dac1775810e32ac0e305f618ef32696f Mon Sep 17 00:00:00 2001 From: Colton Willey Date: Wed, 14 Aug 2024 12:16:14 -0700 Subject: [PATCH 2/7] Enable cert copy by default for openssl extra --- wolfssl/wolfcrypt/settings.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/wolfssl/wolfcrypt/settings.h b/wolfssl/wolfcrypt/settings.h index 25b961479..9d86b1497 100644 --- a/wolfssl/wolfcrypt/settings.h +++ b/wolfssl/wolfcrypt/settings.h @@ -3261,6 +3261,11 @@ extern void uITRON4_free(void *p) ; #define KEEP_PEER_CERT #endif +#if defined(OPENSSL_ALL) && !defined(WOLFSSL_NO_COPY_CERT) + #undef WOLFSSL_COPY_CERT + #define WOLFSSL_COPY_CERT +#endif + /* * Keeps the "Finished" messages after a TLS handshake for use as the so-called * "tls-unique" channel binding. See comment in internal.h around clientFinished From 337cddfd905681f8fc67b9e3fbecc033080ad30f Mon Sep 17 00:00:00 2001 From: Colton Willey Date: Wed, 14 Aug 2024 13:13:25 -0700 Subject: [PATCH 3/7] Rework implementation to use existing weOwnCert logic --- src/internal.c | 2 ++ src/ssl.c | 12 ++---------- src/ssl_load.c | 11 ----------- 3 files changed, 4 insertions(+), 21 deletions(-) diff --git a/src/internal.c b/src/internal.c index 5ae5382d3..58f8ddf44 100644 --- a/src/internal.c +++ b/src/internal.c @@ -6816,6 +6816,7 @@ int SetSSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup) return ret; } + ssl->buffers.weOwnCert = TRUE; ret = WOLFSSL_SUCCESS; } if (ctx->certChain != NULL) { @@ -6829,6 +6830,7 @@ int SetSSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup) return ret; } + ssl->buffers.weOwnCertChain = TRUE; ret = WOLFSSL_SUCCESS; } #else diff --git a/src/ssl.c b/src/ssl.c index a0f377bb9..2137035b6 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -10806,11 +10806,6 @@ int wolfSSL_set_compression(WOLFSSL* ssl) return BAD_FUNC_ARG; } - #ifdef WOLFSSL_COPY_CERT - /* If WOLFSSL_COPY_CERT defined, always free cert buffers in SSL obj */ - FreeDer(&ssl->buffers.certificate); - FreeDer(&ssl->buffers.certChain); - #endif if (ssl->buffers.weOwnCert && !ssl->keepCert) { WOLFSSL_MSG("Unloading cert"); FreeDer(&ssl->buffers.certificate); @@ -19554,11 +19549,6 @@ void wolfSSL_certs_clear(WOLFSSL* ssl) /* ctx still owns certificate, certChain, key, dh, and cm */ if (ssl->buffers.weOwnCert) FreeDer(&ssl->buffers.certificate); -#ifdef WOLFSSL_COPY_CERT - /* If WOLFSSL_COPY_CERT defined, always free cert buffers in SSL obj */ - FreeDer(&ssl->buffers.certificate); - FreeDer(&ssl->buffers.certChain); -#endif ssl->buffers.certificate = NULL; if (ssl->buffers.weOwnCertChain) FreeDer(&ssl->buffers.certChain); @@ -20174,6 +20164,7 @@ WOLFSSL_CTX* wolfSSL_set_SSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx) return NULL; } + ssl->buffers.weOwnCert = TRUE; ret = WOLFSSL_SUCCESS; } if (ctx->certChain != NULL) { @@ -20187,6 +20178,7 @@ WOLFSSL_CTX* wolfSSL_set_SSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx) return NULL; } + ssl->buffers.weOwnCertChain = TRUE; ret = WOLFSSL_SUCCESS; } #else diff --git a/src/ssl_load.c b/src/ssl_load.c index ee11273bf..da4279e39 100644 --- a/src/ssl_load.c +++ b/src/ssl_load.c @@ -236,9 +236,6 @@ 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. */ - #ifdef WOLFSSL_COPY_CERT - FreeDer(&ssl->buffers.certChain); - #endif if (ssl->buffers.weOwnCertChain) { FreeDer(&ssl->buffers.certChain); } @@ -2082,10 +2079,6 @@ static int ProcessBufferCertHandleDer(WOLFSSL_CTX* ctx, WOLFSSL* ssl, /* Leaf certificate - our certificate. */ else if (type == CERT_TYPE) { if (ssl != NULL) { -#ifdef WOLFSSL_COPY_CERT - /* Always Free previously set if WOLFSSL_COPY_CERT defined */ - FreeDer(&ssl->buffers.certificate); -#endif /* Free previous certificate if we own it. */ if (ssl->buffers.weOwnCert) { FreeDer(&ssl->buffers.certificate); @@ -4567,10 +4560,6 @@ static int wolfssl_add_to_chain(DerBuffer** chain, int weOwn, const byte* cert, c32to24(certSz, newChain->buffer + len); XMEMCPY(newChain->buffer + len + CERT_HEADER_SZ, cert, certSz); -#ifdef WOLFSSL_COPY_CERT - FreeDer(chain); -#endif - /* Dispose of old chain if we own it. */ if (weOwn) { FreeDer(chain); From 15abea7f201acd07eacb64a26d9512c27dcf7da0 Mon Sep 17 00:00:00 2001 From: Colton Willey Date: Wed, 14 Aug 2024 13:19:43 -0700 Subject: [PATCH 4/7] Use 1 instead of TRUE --- src/internal.c | 4 ++-- src/ssl.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/internal.c b/src/internal.c index 58f8ddf44..d80080dba 100644 --- a/src/internal.c +++ b/src/internal.c @@ -6816,7 +6816,7 @@ int SetSSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup) return ret; } - ssl->buffers.weOwnCert = TRUE; + ssl->buffers.weOwnCert = 1; ret = WOLFSSL_SUCCESS; } if (ctx->certChain != NULL) { @@ -6830,7 +6830,7 @@ int SetSSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup) return ret; } - ssl->buffers.weOwnCertChain = TRUE; + ssl->buffers.weOwnCertChain = 1; ret = WOLFSSL_SUCCESS; } #else diff --git a/src/ssl.c b/src/ssl.c index 2137035b6..1b8e60310 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -20164,7 +20164,7 @@ WOLFSSL_CTX* wolfSSL_set_SSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx) return NULL; } - ssl->buffers.weOwnCert = TRUE; + ssl->buffers.weOwnCert = 1; ret = WOLFSSL_SUCCESS; } if (ctx->certChain != NULL) { @@ -20178,7 +20178,7 @@ WOLFSSL_CTX* wolfSSL_set_SSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx) return NULL; } - ssl->buffers.weOwnCertChain = TRUE; + ssl->buffers.weOwnCertChain = 1; ret = WOLFSSL_SUCCESS; } #else From dcf3af538250ce972838fb087a9619522a78b672 Mon Sep 17 00:00:00 2001 From: Colton Willey Date: Wed, 14 Aug 2024 14:33:38 -0700 Subject: [PATCH 5/7] Modify tests to make analyzers happy --- tests/api.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/api.c b/tests/api.c index c7fb37b9b..016b98021 100644 --- a/tests/api.c +++ b/tests/api.c @@ -77292,10 +77292,10 @@ static int test_wolfSSL_set_SSL_CTX(void) ExpectIntEQ(XMEMCMP(ssl->sessionCtx, session_id2, 4), 0); #endif #ifdef WOLFSSL_COPY_CERT - if (ctx2->certificate != NULL) { + if (ctx2 != NULL && ctx2->certificate != NULL) { ExpectFalse(ssl->buffers.certificate == ctx2->certificate); } - if (ctx2->certChain != NULL) { + if (ctx2 != NULL && ctx2->certChain != NULL) { ExpectFalse(ssl->buffers.certChain == ctx2->certChain); } #else @@ -77320,10 +77320,10 @@ static int test_wolfSSL_set_SSL_CTX(void) /* MUST change */ #ifdef WOLFSSL_INT_H #ifdef WOLFSSL_COPY_CERT - if (ctx1->certificate != NULL) { + if (ctx1 != NULL && ctx1->certificate != NULL) { ExpectFalse(ssl->buffers.certificate == ctx1->certificate); } - if (ctx1->certChain != NULL) { + if (ctx1 != NULL && ctx1->certChain != NULL) { ExpectFalse(ssl->buffers.certChain == ctx1->certChain); } #else From 65d7c6a533f50a2d13e1234fb91b6f5d146fd5a4 Mon Sep 17 00:00:00 2001 From: Colton Willey Date: Wed, 14 Aug 2024 17:07:20 -0700 Subject: [PATCH 6/7] Do not overwrite cert in wolfSSL_set_SSL_CTX if one is already set, remove unreachable frees. --- src/internal.c | 6 ------ src/ssl.c | 15 ++++++--------- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/src/internal.c b/src/internal.c index d80080dba..4ba6013fe 100644 --- a/src/internal.c +++ b/src/internal.c @@ -6806,9 +6806,6 @@ int SetSSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup) #ifdef WOLFSSL_COPY_CERT /* If WOLFSSL_COPY_CERT is defined, always copy the cert */ if (ctx->certificate != NULL) { - if (ssl->buffers.certificate != NULL) { - FreeDer(&ssl->buffers.certificate); - } ret = AllocCopyDer(&ssl->buffers.certificate, ctx->certificate->buffer, ctx->certificate->length, ctx->certificate->type, ctx->certificate->heap); @@ -6820,9 +6817,6 @@ int SetSSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup) ret = WOLFSSL_SUCCESS; } if (ctx->certChain != NULL) { - if (ssl->buffers.certChain != NULL) { - FreeDer(&ssl->buffers.certChain); - } ret = AllocCopyDer(&ssl->buffers.certChain, ctx->certChain->buffer, ctx->certChain->length, ctx->certChain->type, ctx->certChain->heap); diff --git a/src/ssl.c b/src/ssl.c index 1b8e60310..1b18d8da1 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -20152,11 +20152,10 @@ WOLFSSL_CTX* wolfSSL_set_SSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx) #ifndef NO_CERTS #ifdef WOLFSSL_COPY_CERT - /* If WOLFSSL_COPY_CERT defined, always make new copy of cert */ - if (ctx->certificate != NULL) { - if (ssl->buffers.certificate != NULL) { - FreeDer(&ssl->buffers.certificate); - } + /* If WOLFSSL_COPY_CERT defined, make new copy of cert from ctx + * unless SSL object already has a cert */ + if ((ctx->certificate != NULL) && + (ssl->buffers.certificate == NULL)) { ret = AllocCopyDer(&ssl->buffers.certificate, ctx->certificate->buffer, ctx->certificate->length, ctx->certificate->type, ctx->certificate->heap); @@ -20167,10 +20166,8 @@ WOLFSSL_CTX* wolfSSL_set_SSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx) ssl->buffers.weOwnCert = 1; ret = WOLFSSL_SUCCESS; } - if (ctx->certChain != NULL) { - if (ssl->buffers.certChain != NULL) { - FreeDer(&ssl->buffers.certChain); - } + if ((ctx->certChain != NULL) && + (ssl->buffers.certChain == NULL)) { ret = AllocCopyDer(&ssl->buffers.certChain, ctx->certChain->buffer, ctx->certChain->length, ctx->certChain->type, ctx->certChain->heap); From d056b6374286a0043541c7dfb2728cc2fb3ca790 Mon Sep 17 00:00:00 2001 From: Colton Willey Date: Thu, 15 Aug 2024 09:24:44 -0700 Subject: [PATCH 7/7] Always free existing SSL cert to be compatible with openssl behavior --- src/ssl.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/ssl.c b/src/ssl.c index 1b18d8da1..594cc0cee 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -20152,10 +20152,11 @@ WOLFSSL_CTX* wolfSSL_set_SSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx) #ifndef NO_CERTS #ifdef WOLFSSL_COPY_CERT - /* If WOLFSSL_COPY_CERT defined, make new copy of cert from ctx - * unless SSL object already has a cert */ - if ((ctx->certificate != NULL) && - (ssl->buffers.certificate == NULL)) { + /* If WOLFSSL_COPY_CERT defined, always make new copy of cert from ctx */ + if (ctx->certificate != NULL) { + if (ssl->buffers.certificate != NULL) { + FreeDer(&ssl->buffers.certificate); + } ret = AllocCopyDer(&ssl->buffers.certificate, ctx->certificate->buffer, ctx->certificate->length, ctx->certificate->type, ctx->certificate->heap); @@ -20166,8 +20167,10 @@ WOLFSSL_CTX* wolfSSL_set_SSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx) ssl->buffers.weOwnCert = 1; ret = WOLFSSL_SUCCESS; } - if ((ctx->certChain != NULL) && - (ssl->buffers.certChain == NULL)) { + if (ctx->certChain != NULL) { + if (ssl->buffers.certChain != NULL) { + FreeDer(&ssl->buffers.certChain); + } ret = AllocCopyDer(&ssl->buffers.certChain, ctx->certChain->buffer, ctx->certChain->length, ctx->certChain->type, ctx->certChain->heap);