| Message ID | 20221123154912.28394-1-maximilian.fillinger@foxcrypto.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Check if pkcs11_cert is NULL before freeing it | expand |
Am 23.11.22 um 16:49 schrieb Max Fillinger: > When running openvpn --show-tls with mbedtls, it showed a null pointer > error at the end because of this. > > Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com> > --- > src/openvpn/ssl_mbedtls.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c > index ea06cf70..aa55a1a0 100644 > --- a/src/openvpn/ssl_mbedtls.c > +++ b/src/openvpn/ssl_mbedtls.c > @@ -165,7 +165,10 @@ tls_ctx_free(struct tls_root_ctx *ctx) > free(ctx->crl); > > #if defined(ENABLE_PKCS11) > - pkcs11h_certificate_freeCertificate(ctx->pkcs11_cert); > + if (ctx->pkcs11_cert) > + { > + pkcs11h_certificate_freeCertificate(ctx->pkcs11_cert); > + } > #endif > > free(ctx->allowed_ciphers); Sigh, a function that violates the C paradigm that calling somethingfree on a null pointer is fine. Maybe we should add as a comment that this function is special in this way. Acked-By: Arne Schwabe <arne@rfc2549.org> Arne
Hi, On Wed, Nov 23, 2022 at 12:18 PM Arne Schwabe <arne@rfc2549.org> wrote: > Am 23.11.22 um 16:49 schrieb Max Fillinger: > > When running openvpn --show-tls with mbedtls, it showed a null pointer > > error at the end because of this. > > > > Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com> > > --- > > src/openvpn/ssl_mbedtls.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c > > index ea06cf70..aa55a1a0 100644 > > --- a/src/openvpn/ssl_mbedtls.c > > +++ b/src/openvpn/ssl_mbedtls.c > > @@ -165,7 +165,10 @@ tls_ctx_free(struct tls_root_ctx *ctx) > > free(ctx->crl); > > > > #if defined(ENABLE_PKCS11) > > - pkcs11h_certificate_freeCertificate(ctx->pkcs11_cert); > > + if (ctx->pkcs11_cert) > > + { > > + pkcs11h_certificate_freeCertificate(ctx->pkcs11_cert); > > + } > > #endif > > > > free(ctx->allowed_ciphers); > > Sigh, a function that violates the C paradigm that calling somethingfree > on a null pointer is fine. Maybe we should add as a comment that this > function is special in this way. > pkcs11h_certiciate_freeCertificate() does seem to handle NULL argument. With --show-tls, are we calling this before intializing the pkcs11 library? That could trigger an ASSERT. Selva
Hi, On Wed, Nov 23, 2022 at 01:37:47PM -0500, Selva Nair wrote: > pkcs11h_certiciate_freeCertificate() does seem to handle NULL argument. > With --show-tls, are we calling this before intializing the pkcs11 library? > That could trigger an ASSERT. If I build on Linux with mbedtls && --enable-pkcs11 (which I normally don't do), and run --show-tls, this is what happens... TLS-PSK-WITH-CAMELLIA-128-GCM-SHA256 TLS-PSK-WITH-CAMELLIA-128-CBC-SHA256 TLS-PSK-WITH-AES-128-CCM-8 openvpn: pkcs11h-certificate.c:1213: pkcs11h_certificate_freeCertificate: Assertion `_g_pkcs11h_data!=NULL' failed. Aborted (core dumped) ... so, the crash seems to agree with your analysis :-) gert
Confirmed that it crashes without the patch, and does not crash with it.
Added a comment, as suggested.
Your patch has been applied to the master and release/2.5 branch (bugfix).
commit 19c64f16baebbce966d55c62135d1ef066f7c8c2 (master)
commit 4f5e57d2c7ff9384b16c42eb9aa5af11d31f5dd1 (release/2.5)
Author: Max Fillinger
Date: Wed Nov 23 16:49:12 2022 +0100
Check if pkcs11_cert is NULL before freeing it
Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
Acked-by: Arne Schwabe <arne@rfc2549.org>
Message-Id: <20221123154912.28394-1-maximilian.fillinger@foxcrypto.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25530.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c index ea06cf70..aa55a1a0 100644 --- a/src/openvpn/ssl_mbedtls.c +++ b/src/openvpn/ssl_mbedtls.c @@ -165,7 +165,10 @@ tls_ctx_free(struct tls_root_ctx *ctx) free(ctx->crl); #if defined(ENABLE_PKCS11) - pkcs11h_certificate_freeCertificate(ctx->pkcs11_cert); + if (ctx->pkcs11_cert) + { + pkcs11h_certificate_freeCertificate(ctx->pkcs11_cert); + } #endif free(ctx->allowed_ciphers);
When running openvpn --show-tls with mbedtls, it showed a null pointer error at the end because of this. Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com> --- src/openvpn/ssl_mbedtls.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)