From patchwork Wed Apr 7 09:15:54 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Maximilian Fillinger X-Patchwork-Id: 1721 Return-Path: Delivered-To: patchwork@openvpn.net Delivered-To: patchwork@openvpn.net Received: from director11.mail.ord1d.rsapps.net ([172.27.255.55]) by backend30.mail.ord1d.rsapps.net with LMTP id iHrSKmYFbmAnKwAAIUCqbw (envelope-from ) for ; Wed, 07 Apr 2021 15:17:58 -0400 Received: from proxy7.mail.iad3a.rsapps.net ([172.27.255.55]) by director11.mail.ord1d.rsapps.net with LMTP id 6IuOKmYFbmCPQgAAvGGmqA (envelope-from ) for ; Wed, 07 Apr 2021 15:17:58 -0400 Received: from smtp53.gate.iad3a ([172.27.255.55]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) by proxy7.mail.iad3a.rsapps.net with LMTPS id KNjvI2YFbmCUVgAAnPvY+A (envelope-from ) for ; Wed, 07 Apr 2021 15:17:58 -0400 X-Spam-Threshold: 95 X-Spam-Score: 0 X-Spam-Flag: NO X-Virus-Scanned: OK X-Orig-To: openvpnslackdevel@openvpn.net X-Originating-Ip: [216.105.38.7] Authentication-Results: smtp53.gate.iad3a.rsapps.net; iprev=pass policy.iprev="216.105.38.7"; spf=pass smtp.mailfrom="openvpn-devel-bounces@lists.sourceforge.net" smtp.helo="lists.sourceforge.net"; dkim=fail (signature verification failed) header.d=sourceforge.net; dkim=fail (signature verification failed) header.d=sf.net; dkim=fail (key not found in DNS) header.d=foxcrypto.com; dmarc=fail (p=none; dis=none) header.from=foxcrypto.com X-Suspicious-Flag: YES X-Classification-ID: f5af4376-97d5-11eb-9d19-5254009c3572-1-1 Received: from [216.105.38.7] ([216.105.38.7:46826] helo=lists.sourceforge.net) by smtp53.gate.iad3a.rsapps.net (envelope-from ) (ecelerity 4.2.38.62370 r(:)) with ESMTPS (cipher=DHE-RSA-AES256-GCM-SHA384) id 24/7F-10486-5650E606; Wed, 07 Apr 2021 15:17:58 -0400 Received: from [127.0.0.1] (helo=sfs-ml-1.v29.lw.sourceforge.com) by sfs-ml-1.v29.lw.sourceforge.com with esmtp (Exim 4.90_1) (envelope-from ) id 1lUDfq-000423-Dl; Wed, 07 Apr 2021 19:17:10 +0000 Received: from [172.30.20.202] (helo=mx.sourceforge.net) by sfs-ml-1.v29.lw.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lUDfp-00041p-08 for openvpn-devel@lists.sourceforge.net; Wed, 07 Apr 2021 19:17:09 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Content-Type:MIME-Version:References:In-Reply-To: Date:Subject:CC:To:From:Sender:Reply-To:Message-ID:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=uwnc4RPNe0JFUiLsDnjJoJwWsRjCyboFjCsy9RDnSts=; b=JP2mz+jZ0wV73bVDE5zGXwlkZb +l++JWlCpRg+j+AJK03qc18I4GZ/oZOCV/hzTaSjFaiqZcwmLNU0rItQrnluV3pCJQcK58ksR9Kok OM853I8qsJ1keiaNXWFEKrMVXNll50ycCxzTskbN/bAfxtP+rvi8ctna9RE5rJq5T4iw=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Content-Type:MIME-Version:References:In-Reply-To:Date:Subject:CC:To:From: Sender:Reply-To:Message-ID:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=uwnc4RPNe0JFUiLsDnjJoJwWsRjCyboFjCsy9RDnSts=; b=KX8F6wwRGHs+hF8PeSdDgxatxx D5y6z1quyL3BmbKKw62PU8mdVQ3Q5blPkd2kZvP+WXeXDbo1no3yOtW8TaVL5vpRo5sOuSM7i354F fG6tqx7ekNsIqJvBMJAXl9+KY5SALH0BO3YRpSTg4F6rFPN8Zns7Fck9HeAVH2oAidpY=; Received: from nl-dft-mx-01.fox-it.com ([178.250.144.135]) by sfi-mx-1.v28.lw.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.92.2) id 1lUDfj-000MO3-Ru for openvpn-devel@lists.sourceforge.net; Wed, 07 Apr 2021 19:17:08 +0000 From: Max Fillinger To: Date: Wed, 7 Apr 2021 21:15:54 +0200 X-Mailer: git-send-email 2.11.0 In-Reply-To: <20210407191554.25811-1-maximilian.fillinger@foxcrypto.com> References: <20210407191554.25811-1-maximilian.fillinger@foxcrypto.com> MIME-Version: 1.0 X-ClientProxiedBy: FOXDFT1EX01.FOX.local (10.0.0.129) To FOXDFT1EX01.FOX.local (10.0.0.129) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; d=foxcrypto.com; s=NL-DFT-MX-01; c=relaxed/relaxed; h=from:to:cc:subject:date:references:mime-version:content-type; bh=uwnc4RPNe0JFUiLsDnjJoJwWsRjCyboFjCsy9RDnSts=; b=f+MOXHDavd2a7MEdfAoekHRDq/3SjYazFtkCeeTcIE3ngYrEUN27H75h6ipo5qVLRpZIvptOUg46 VWe8Rw4ESEhzBxMyzMrX0qySRfPx8nx1EWIOrkSFfphNCArJEHGx0mg2RpO42XXaEL8eHemXPPa2 7GW9KlKdYnmwP20Bw4uboZWHtpOHwxg8VP4QxUeTNwLv6Vb2ZJNFkiePRs8G8YCt6BAJ86AZJfvx 3IVfZtBGNml90u2s7PNe6d9sfy6DdZlGB6ZdFhCUqJ3QaRdGwloNd3xp0nbPmlslneYp4RWyljA8 8Cyshx+tEGK2wKIQtIyIRfemMPTU0+4scm6+uA== X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. -0.0 SPF_PASS SPF: sender matches SPF record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid 1.0 MISSING_MID Missing Message-Id: header 0.1 DKIM_INVALID DKIM or DK signature exists, but is not valid X-Headers-End: 1lUDfj-000MO3-Ru Subject: [Openvpn-devel] [PATCH 1/1] Rework mbedtls CRL handling X-BeenThere: openvpn-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: openvpn-devel-bounces@lists.sourceforge.net Message-Id: X-getmail-retrieved-from-mailbox: Inbox This commit fixes the following two issues: The config belonging to a mbedtls_ssl_ctx struct is not supposed to be changed after mbedtls_ssl_setup() has been called. Previously, we modified the CRL structure in place when a new CRL was loaded, but a pointer to this struct appears in configs that are currently in use. This commit uses reference counting to ensure that CRLs remain in memory while the mbedtls_ssl_ctx struct and config remain. The other issue fixed by this commit is that, if OpenVPN failed to read the CRL in init_ssl(), but succeeded in reading it later, it would not actually use that CRL. --- src/openvpn/ssl.c | 8 +++ src/openvpn/ssl_mbedtls.c | 103 ++++++++++++++++++++++++++++++++++----- src/openvpn/ssl_mbedtls.h | 25 +++++++++- src/openvpn/ssl_verify_mbedtls.c | 2 +- 4 files changed, 125 insertions(+), 13 deletions(-) diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index d8662d00..0f2cb62d 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -2737,6 +2737,14 @@ tls_process(struct tls_multi *multi, { tls_ctx_reload_crl(&session->opt->ssl_ctx, session->opt->crl_file, session->opt->crl_file_inline); + +#ifdef ENABLE_CRYPTO_MBEDTLS + /* With mbedtls, the new CRL doesn't end up in the key_state + * automatically, so we need to put it there and recreate the + * mbedtls_ssl_context. */ + update_key_state_crl(&ks->ks_ssl, &session->opt->ssl_ctx); +#endif + } /* New connection, remove any old X509 env variables */ diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c index 8917fb18..87a4b8df 100644 --- a/src/openvpn/ssl_mbedtls.c +++ b/src/openvpn/ssl_mbedtls.c @@ -105,6 +105,45 @@ tls_clear_error(void) { } +/* Functions to handle reference counting for CRLs. + * + * The config of an mbedtls_ssl_context is not supposed to be changed + * after mbedtls_ssl_setup() has been called. Therefore, we need to + * leave previous CRLs in memory. They will be freed when they are no + * longer needed. + */ +static struct refcounted_crl * +init_refcounted_crl(void) +{ + struct refcounted_crl *ref_crl; + ALLOC_OBJ(ref_crl, struct refcounted_crl); + mbedtls_x509_crl_init(&ref_crl->crl); + ref_crl->refcount = 1; + + return ref_crl; +} + +static struct refcounted_crl * +get_refcounted_crl(struct refcounted_crl *ref_crl) +{ + ASSERT(ref_crl->refcount < SIZE_MAX); + ref_crl->refcount += 1; + return ref_crl; +} + +static void +release_refcounted_crl(struct refcounted_crl *ref_crl) +{ + if (ref_crl != NULL) { + ASSERT(ref_crl->refcount > 0); + if (--ref_crl->refcount == 0) + { + mbedtls_x509_crl_free(&ref_crl->crl); + free(ref_crl); + } + } +} + void tls_ctx_server_new(struct tls_root_ctx *ctx) { @@ -149,8 +188,7 @@ tls_ctx_free(struct tls_root_ctx *ctx) mbedtls_dhm_free(ctx->dhm_ctx); free(ctx->dhm_ctx); - mbedtls_x509_crl_free(ctx->crl); - free(ctx->crl); + release_refcounted_crl(ctx->ref_crl); #if defined(ENABLE_PKCS11) pkcs11h_certificate_freeCertificate(ctx->pkcs11_cert); @@ -1013,15 +1051,12 @@ backend_tls_ctx_reload_crl(struct tls_root_ctx *ctx, const char *crl_file, { ASSERT(crl_file); - if (ctx->crl == NULL) - { - ALLOC_OBJ_CLEAR(ctx->crl, mbedtls_x509_crl); - } - mbedtls_x509_crl_free(ctx->crl); + release_refcounted_crl(ctx->ref_crl); + ctx->ref_crl = init_refcounted_crl(); if (crl_inline) { - if (!mbed_ok(mbedtls_x509_crl_parse(ctx->crl, + if (!mbed_ok(mbedtls_x509_crl_parse(&ctx->ref_crl->crl, (const unsigned char *)crl_file, strlen(crl_file) + 1))) { @@ -1031,7 +1066,7 @@ backend_tls_ctx_reload_crl(struct tls_root_ctx *ctx, const char *crl_file, } else { - if (!mbed_ok(mbedtls_x509_crl_parse_file(ctx->crl, crl_file))) + if (!mbed_ok(mbedtls_x509_crl_parse_file(&ctx->ref_crl->crl, crl_file))) { msg(M_WARN, "CRL: cannot read CRL from file %s", crl_file); goto err; @@ -1040,7 +1075,9 @@ backend_tls_ctx_reload_crl(struct tls_root_ctx *ctx, const char *crl_file, return; err: - mbedtls_x509_crl_free(ctx->crl); + /* Leaving the CRL in the freed state causes all connection attempts to + * be rejected. */ + mbedtls_x509_crl_free(&ctx->ref_crl->crl); } void @@ -1121,8 +1158,17 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl, } mbedtls_ssl_conf_verify(ks_ssl->ssl_config, verify_callback, session); + if (ssl_ctx->ref_crl) + { + ks_ssl->ref_crl = get_refcounted_crl(ssl_ctx->ref_crl); + } + else + { + ks_ssl->ref_crl = NULL; + } + /* TODO: mbed TLS does not currently support sending the CA chain to the client */ - mbedtls_ssl_conf_ca_chain(ks_ssl->ssl_config, ssl_ctx->ca_chain, ssl_ctx->crl); + mbedtls_ssl_conf_ca_chain(ks_ssl->ssl_config, ssl_ctx->ca_chain, &ks_ssl->ref_crl->crl); /* Initialize minimum TLS version */ { @@ -1196,10 +1242,45 @@ key_state_ssl_free(struct key_state_ssl *ks_ssl) buf_free_entries(&ks_ssl->bio_ctx->out); free(ks_ssl->bio_ctx); } + release_refcounted_crl(ks_ssl->ref_crl); CLEAR(*ks_ssl); } } +void +update_key_state_crl(struct key_state_ssl *ks_ssl, struct tls_root_ctx *ctx) +{ + ASSERT(ks_ssl != NULL); + ASSERT(ks_ssl->ctx != NULL); + ASSERT(ks_ssl->bio_ctx != NULL); + + /* Free the old mbedtls_ssl_context struct. The config should not be + * updated while it is in use. */ + mbedtls_ssl_free(ks_ssl->ctx); + + release_refcounted_crl(ks_ssl->ref_crl); + if (ctx->ref_crl != NULL) + { + ks_ssl->ref_crl = get_refcounted_crl(ctx->ref_crl); + } + else + { + ks_ssl->ref_crl = NULL; + } + + /* Set new CRL in ssl_config. */ + mbedtls_ssl_conf_ca_chain(ks_ssl->ssl_config, ctx->ca_chain, &ks_ssl->ref_crl->crl); + + /* Initialize new mbedtls_ssl_context. */ + mbedtls_ssl_init(ks_ssl->ctx); + mbedtls_ssl_setup(ks_ssl->ctx, ks_ssl->ssl_config); + + buf_free_entries(&ks_ssl->bio_ctx->in); + buf_free_entries(&ks_ssl->bio_ctx->out); + mbedtls_ssl_set_bio(ks_ssl->ctx, ks_ssl->bio_ctx, ssl_bio_write, + ssl_bio_read, NULL); +} + int key_state_write_plaintext(struct key_state_ssl *ks, struct buffer *buf) { diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h index ff64e17c..1be5c749 100644 --- a/src/openvpn/ssl_mbedtls.h +++ b/src/openvpn/ssl_mbedtls.h @@ -96,6 +96,17 @@ struct tls_key_cache { }; #endif /** + * This structure wraps a CRL and counts how many structures currently + * have a pointer to it. The config of a mbedtls_ssl_context is not + * supposed to be changed after changed after setup, so we have to leave + * old CRLs in memory after loading a new one. + */ +struct refcounted_crl { + mbedtls_x509_crl crl; + size_t refcount; +}; + +/** * Structure that wraps the TLS context. Contents differ depending on the * SSL library used. * @@ -110,7 +121,7 @@ struct tls_root_ctx { mbedtls_x509_crt *crt_chain; /**< Local Certificate chain */ mbedtls_x509_crt *ca_chain; /**< CA chain for remote verification */ mbedtls_pk_context *priv_key; /**< Local private key */ - mbedtls_x509_crl *crl; /**< Certificate Revocation List */ + struct refcounted_crl *ref_crl; /**< Certificate Revocation List */ time_t crl_last_mtime; /**< CRL last modification time */ off_t crl_last_size; /**< size of last loaded CRL */ #ifdef ENABLE_PKCS11 @@ -125,6 +136,7 @@ struct tls_root_ctx { struct key_state_ssl { mbedtls_ssl_config *ssl_config; /**< mbedTLS global ssl config */ mbedtls_ssl_context *ctx; /**< mbedTLS connection context */ + struct refcounted_crl *ref_crl; /**< CRL belonging to ssl_config and ctx */ bio_ctx *bio_ctx; struct tls_key_cache tls_key_cache; @@ -144,4 +156,15 @@ int tls_ctx_use_external_signing_func(struct tls_root_ctx *ctx, external_sign_func sign_func, void *sign_ctx); +/** + * Update the Certificate Revocation List in ks_ssl. + * + * This involves creating a new mbedtls_ssl_context in ks_ssl and should only + * be called if that context has not been used yet. + * + * @param ks_ssl The SSL channel's state info + * @param ctx The TLS context with the new CRL + */ +void update_key_state_crl(struct key_state_ssl *ks_ssl, struct tls_root_ctx *ctx); + #endif /* SSL_MBEDTLS_H_ */ diff --git a/src/openvpn/ssl_verify_mbedtls.c b/src/openvpn/ssl_verify_mbedtls.c index ef3847bb..03ae9355 100644 --- a/src/openvpn/ssl_verify_mbedtls.c +++ b/src/openvpn/ssl_verify_mbedtls.c @@ -560,7 +560,7 @@ bool tls_verify_crl_missing(const struct tls_options *opt) { if (opt->crl_file && !(opt->ssl_flags & SSLF_CRL_VERIFY_DIR) - && (opt->ssl_ctx.crl == NULL || opt->ssl_ctx.crl->version == 0)) + && (opt->ssl_ctx.ref_crl == NULL || opt->ssl_ctx.ref_crl->crl.version == 0)) { return true; }