[Openvpn-devel,3/9] Remove cipher_ctx_get_cipher_kt and replace with direct context calls
Commit Message
We currently have a number of calls that fetch the cipher_kt from a
cipher_ctx to then do a query on the cipher_kt. Directly fetching the
desired property from the context is cleaner and helps for using the
proper APIs with OpenSSL 3.0 and mbed TLS 3.0
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
src/openvpn/crypto.c | 36 ++++++++---------------
src/openvpn/crypto_backend.h | 29 ++++++++++++++----
src/openvpn/crypto_mbedtls.c | 21 +++++++++++--
src/openvpn/crypto_openssl.c | 57 ++++++++++++++++++++++++++++++++++--
src/openvpn/openssl_compat.h | 1 +
src/openvpn/ssl.c | 8 ++---
src/openvpn/ssl_ncp.c | 18 +++++-------
7 files changed, 119 insertions(+), 51 deletions(-)
Comments
Hi,
On Wed, Dec 01, 2021 at 07:07:21PM +0100, Arne Schwabe wrote:
> We currently have a number of calls that fetch the cipher_kt from a
> cipher_ctx to then do a query on the cipher_kt. Directly fetching the
> desired property from the context is cleaner and helps for using the
> proper APIs with OpenSSL 3.0 and mbed TLS 3.0
Generally speaking this seems reasonable, I just wonder about
two things:
> @@ -68,12 +68,10 @@ openvpn_encrypt_aead(struct buffer *buf, struct buffer work,
> int outlen = 0;
> const struct key_ctx *ctx = &opt->key_ctx_bi.encrypt;
> uint8_t *mac_out = NULL;
> - const cipher_kt_t *cipher_kt = cipher_ctx_get_cipher_kt(ctx->cipher);
> const int mac_len = OPENVPN_AEAD_TAG_LENGTH;
>
> /* IV, packet-ID and implicit IV required for this mode. */
> ASSERT(ctx->cipher);
> - ASSERT(cipher_kt_mode_aead(cipher_kt));
> ASSERT(packet_id_initialized(&opt->packet_id));
This ASSERT goes away now. It's there to ensure that "yeah, we're
really using an AEAD ciphere here" - which should, of course, never
trigger, but I assume Steffan had some reason to put it here.
> @@ -371,7 +364,6 @@ openvpn_decrypt_aead(struct buffer *buf, struct buffer work,
> ASSERT(frame);
> ASSERT(buf->len > 0);
> ASSERT(ctx->cipher);
> - ASSERT(cipher_kt_mode_aead(cipher_kt));
Same thing here.
So - can we be "crypto level" secure that this is never needed?
gert
Am 05.12.21 um 18:33 schrieb Gert Doering:
> Hi,
>
> On Wed, Dec 01, 2021 at 07:07:21PM +0100, Arne Schwabe wrote:
>> We currently have a number of calls that fetch the cipher_kt from a
>> cipher_ctx to then do a query on the cipher_kt. Directly fetching the
>> desired property from the context is cleaner and helps for using the
>> proper APIs with OpenSSL 3.0 and mbed TLS 3.0
>
> Generally speaking this seems reasonable, I just wonder about
> two things:
>
>> @@ -68,12 +68,10 @@ openvpn_encrypt_aead(struct buffer *buf, struct buffer work,
>> int outlen = 0;
>> const struct key_ctx *ctx = &opt->key_ctx_bi.encrypt;
>> uint8_t *mac_out = NULL;
>> - const cipher_kt_t *cipher_kt = cipher_ctx_get_cipher_kt(ctx->cipher);
>> const int mac_len = OPENVPN_AEAD_TAG_LENGTH;
>>
>> /* IV, packet-ID and implicit IV required for this mode. */
>> ASSERT(ctx->cipher);
>> - ASSERT(cipher_kt_mode_aead(cipher_kt));
>> ASSERT(packet_id_initialized(&opt->packet_id));
>
> This ASSERT goes away now. It's there to ensure that "yeah, we're
> really using an AEAD ciphere here" - which should, of course, never
> trigger, but I assume Steffan had some reason to put it here.
Since we no longer use cipher_kt in that function at all, we also no
longer need to check it if it is AEAD. On the ctx we already check if
it is AEAD before calling this function:
if (cipher_ctx_mode_aead(opt->key_ctx_bi.encrypt.cipher))
{
openvpn_encrypt_aead(buf, work, opt);
}
It is basically a overly cautions "Is the cipher type really AEAD from a
AEAD context?"
>
>> @@ -371,7 +364,6 @@ openvpn_decrypt_aead(struct buffer *buf, struct buffer work,
>> ASSERT(frame);
>> ASSERT(buf->len > 0);
>> ASSERT(ctx->cipher);
>> - ASSERT(cipher_kt_mode_aead(cipher_kt));
>
> Same thing here.
>
> So - can we be "crypto level" secure that this is never needed?
Same here.
Acked-by: Gert Doering <gert@greenie.muc.de>
Change makes sense, resulting code passes t_client tests (with and
without AES ciphers), on OpenSSL 1.1 and mbedTLS builds.
Thanks for the explanation wrt the ASSERT() calls - indeed, if the
caller already checks the cipher itself, no need to double-check it
in the called function.
Reviewing the change to crypto_backend.h I noticed that the comments
seem to be "german invalid grammar" according to what I remember from
school...
+ * @param ctx Cipher's context. May not be NULL.
shouldn't that be "Must not be NULL" for "NULL is not allowed" here?
(There are many more of these, so it's not *this* patch introducing
the error - so, not changing these on the fly)
Another observation: cipher_ctx_mode_cbc() and cipher_ctx_mode_ofb_cfb()
seem to want the same thing ("no AEAD, no EVP_CIPH_FLAG_CTS") but
do it in different ways - this looks a bit weird.
And then, cipher_ctx_mode_aead() does not use "early return" on
if (!ctx)... which is inconsistent to the previous two.
And *then*, _cbc() use EVP_CIPHER_CTX_get_mode(), while _ofb_cfb()
uses EVP_CIPHER_CTX_mode(), which seems to be the 3.0 <-> 1.1 variants,
and we do have a compat call...
Could these be unified in a followup patch, please?
Your patch has been applied to the master branch.
commit 0b1c721e4f19c84ea35a2e266c3913eb893355ec
Author: Arne Schwabe
Date: Wed Dec 1 19:07:21 2021 +0100
Remove cipher_ctx_get_cipher_kt and replace with direct context calls
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20211201180727.2496903-3-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23278.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
@@ -68,12 +68,10 @@ openvpn_encrypt_aead(struct buffer *buf, struct buffer work,
int outlen = 0;
const struct key_ctx *ctx = &opt->key_ctx_bi.encrypt;
uint8_t *mac_out = NULL;
- const cipher_kt_t *cipher_kt = cipher_ctx_get_cipher_kt(ctx->cipher);
const int mac_len = OPENVPN_AEAD_TAG_LENGTH;
/* IV, packet-ID and implicit IV required for this mode. */
ASSERT(ctx->cipher);
- ASSERT(cipher_kt_mode_aead(cipher_kt));
ASSERT(packet_id_initialized(&opt->packet_id));
gc_init(&gc);
@@ -171,7 +169,6 @@ openvpn_encrypt_v1(struct buffer *buf, struct buffer work,
{
uint8_t iv_buf[OPENVPN_MAX_IV_LENGTH] = {0};
const int iv_size = cipher_ctx_iv_length(ctx->cipher);
- const cipher_kt_t *cipher_kt = cipher_ctx_get_cipher_kt(ctx->cipher);
int outlen;
/* Reserve space for HMAC */
@@ -182,7 +179,7 @@ openvpn_encrypt_v1(struct buffer *buf, struct buffer work,
hmac_start = BEND(&work);
}
- if (cipher_kt_mode_cbc(cipher_kt))
+ if (cipher_ctx_mode_cbc(ctx->cipher))
{
/* generate pseudo-random IV */
prng_bytes(iv_buf, iv_size);
@@ -197,7 +194,7 @@ openvpn_encrypt_v1(struct buffer *buf, struct buffer work,
goto err;
}
}
- else if (cipher_kt_mode_ofb_cfb(cipher_kt))
+ else if (cipher_ctx_mode_ofb_cfb(ctx->cipher))
{
struct buffer b;
@@ -245,7 +242,7 @@ openvpn_encrypt_v1(struct buffer *buf, struct buffer work,
ASSERT(buf_inc_len(&work, outlen));
/* For all CBC mode ciphers, check the last block is complete */
- ASSERT(cipher_kt_mode(cipher_kt) != OPENVPN_MODE_CBC
+ ASSERT(cipher_ctx_mode(ctx->cipher) != OPENVPN_MODE_CBC
|| outlen == iv_size);
}
else /* No Encryption */
@@ -301,10 +298,7 @@ openvpn_encrypt(struct buffer *buf, struct buffer work,
{
if (buf->len > 0 && opt)
{
- const cipher_kt_t *cipher_kt =
- cipher_ctx_get_cipher_kt(opt->key_ctx_bi.encrypt.cipher);
-
- if (cipher_kt_mode_aead(cipher_kt))
+ if (cipher_ctx_mode_aead(opt->key_ctx_bi.encrypt.cipher))
{
openvpn_encrypt_aead(buf, work, opt);
}
@@ -360,7 +354,6 @@ openvpn_decrypt_aead(struct buffer *buf, struct buffer work,
static const char error_prefix[] = "AEAD Decrypt error";
struct packet_id_net pin = { 0 };
const struct key_ctx *ctx = &opt->key_ctx_bi.decrypt;
- const cipher_kt_t *cipher_kt = cipher_ctx_get_cipher_kt(ctx->cipher);
uint8_t *tag_ptr = NULL;
int outlen;
struct gc_arena gc;
@@ -371,7 +364,6 @@ openvpn_decrypt_aead(struct buffer *buf, struct buffer work,
ASSERT(frame);
ASSERT(buf->len > 0);
ASSERT(ctx->cipher);
- ASSERT(cipher_kt_mode_aead(cipher_kt));
dmsg(D_PACKET_CONTENT, "DECRYPT FROM: %s",
format_hex(BPTR(buf), BLEN(buf), 80, &gc));
@@ -537,7 +529,6 @@ openvpn_decrypt_v1(struct buffer *buf, struct buffer work,
if (ctx->cipher)
{
const int iv_size = cipher_ctx_iv_length(ctx->cipher);
- const cipher_kt_t *cipher_kt = cipher_ctx_get_cipher_kt(ctx->cipher);
uint8_t iv_buf[OPENVPN_MAX_IV_LENGTH] = { 0 };
int outlen;
@@ -589,7 +580,7 @@ openvpn_decrypt_v1(struct buffer *buf, struct buffer work,
/* Get packet ID from plaintext buffer or IV, depending on cipher mode */
{
- if (cipher_kt_mode_cbc(cipher_kt))
+ if (cipher_ctx_mode_cbc(ctx->cipher))
{
if (packet_id_initialized(&opt->packet_id))
{
@@ -600,7 +591,7 @@ openvpn_decrypt_v1(struct buffer *buf, struct buffer work,
have_pin = true;
}
}
- else if (cipher_kt_mode_ofb_cfb(cipher_kt))
+ else if (cipher_ctx_mode_ofb_cfb(ctx->cipher))
{
struct buffer b;
@@ -660,8 +651,7 @@ openvpn_decrypt(struct buffer *buf, struct buffer work,
if (buf->len > 0 && opt)
{
- const struct key_ctx *ctx = &opt->key_ctx_bi.decrypt;
- if (cipher_kt_mode_aead(cipher_ctx_get_cipher_kt(ctx->cipher)))
+ if (cipher_ctx_mode_aead(opt->key_ctx_bi.decrypt.cipher))
{
ret = openvpn_decrypt_aead(buf, work, opt, frame, ad_start);
}
@@ -1036,14 +1026,12 @@ test_crypto(struct crypto_options *co, struct frame *frame)
/* init implicit IV */
{
- const cipher_kt_t *cipher =
- cipher_ctx_get_cipher_kt(co->key_ctx_bi.encrypt.cipher);
-
- if (cipher_kt_mode_aead(cipher))
+ cipher_ctx_t *cipher = co->key_ctx_bi.encrypt.cipher;
+ if (cipher_ctx_mode_aead(cipher))
{
- size_t impl_iv_len = cipher_kt_iv_size(cipher) - sizeof(packet_id_type);
- ASSERT(cipher_kt_iv_size(cipher) <= OPENVPN_MAX_IV_LENGTH);
- ASSERT(cipher_kt_iv_size(cipher) >= OPENVPN_AEAD_MIN_IV_LEN);
+ size_t impl_iv_len = cipher_ctx_iv_length(cipher) - sizeof(packet_id_type);
+ ASSERT(cipher_ctx_iv_length(cipher) <= OPENVPN_MAX_IV_LENGTH);
+ ASSERT(cipher_ctx_iv_length(cipher) >= OPENVPN_AEAD_MIN_IV_LEN);
/* Generate dummy implicit IV */
ASSERT(rand_bytes(co->key_ctx_bi.encrypt.implicit_iv,
@@ -338,7 +338,7 @@ void cipher_ctx_init(cipher_ctx_t *ctx, const uint8_t *key, int key_len,
* @param ctx The cipher's context
*
* @return Size of the IV, in bytes, or \c 0 if the cipher does not
- * use an IV or ctx was NULL.
+ * use an IV.
*/
int cipher_ctx_iv_length(const cipher_ctx_t *ctx);
@@ -371,14 +371,31 @@ int cipher_ctx_block_size(const cipher_ctx_t *ctx);
int cipher_ctx_mode(const cipher_ctx_t *ctx);
/**
- * Returns the static cipher parameters for this context.
+ * Check if the supplied cipher is a supported CBC mode cipher.
+ *
+ * @param ctx Cipher's context. May not be NULL.
+ *
+ * @return true iff the cipher is a CBC mode cipher.
+ */
+bool cipher_ctx_mode_cbc(const cipher_ctx_t *ctx);
+
+/**
+ * Check if the supplied cipher is a supported OFB or CFB mode cipher.
+ *
+ * @param ctx Cipher's context. May not be NULL.
+ *
+ * @return true iff the cipher is a OFB or CFB mode cipher.
+ */
+bool cipher_ctx_mode_ofb_cfb(const cipher_ctx_t *ctx);
+
+/**
+ * Check if the supplied cipher is a supported AEAD mode cipher.
*
- * @param ctx Cipher's context.
+ * @param ctx Cipher's context. May not be NULL.
*
- * @return Static cipher parameters for the supplied context, or
- * NULL if unable to determine cipher parameters.
+ * @return true iff the cipher is a AEAD mode cipher.
*/
-const cipher_kt_t *cipher_ctx_get_cipher_kt(const cipher_ctx_t *ctx);
+bool cipher_ctx_mode_aead(const cipher_ctx_t *ctx);
/**
* Resets the given cipher context, setting the IV to the specified value.
@@ -591,10 +591,25 @@ cipher_ctx_mode(const mbedtls_cipher_context_t *ctx)
return cipher_kt_mode(ctx->cipher_info);
}
-const cipher_kt_t *
-cipher_ctx_get_cipher_kt(const cipher_ctx_t *ctx)
+bool cipher_ctx_mode_cbc(const cipher_ctx_t *ctx)
{
- return ctx ? ctx->cipher_info : NULL;
+ return ctx && cipher_ctx_mode(ctx) == OPENVPN_MODE_CBC;
+}
+
+
+bool cipher_ctx_mode_ofb_cfb(const cipher_ctx_t *ctx)
+{
+ return ctx && (cipher_ctx_mode(ctx) == OPENVPN_MODE_OFB
+ || cipher_ctx_mode(ctx) == OPENVPN_MODE_CFB);
+}
+
+bool cipher_ctx_mode_aead(const cipher_ctx_t *ctx)
+{
+ return ctx && (cipher_ctx_mode(ctx) == OPENVPN_MODE_GCM
+#ifdef MBEDTLS_CHACHAPOLY_C
+ || cipher_ctx_mode(ctx) == MBEDTLS_MODE_CHACHAPOLY
+#endif
+ );
}
int
@@ -797,10 +797,61 @@ cipher_ctx_mode(const EVP_CIPHER_CTX *ctx)
return EVP_CIPHER_CTX_mode(ctx);
}
-const cipher_kt_t *
-cipher_ctx_get_cipher_kt(const cipher_ctx_t *ctx)
+
+bool
+cipher_ctx_mode_cbc(const cipher_ctx_t *ctx)
+{
+ if (!ctx)
+ {
+ return false;
+ }
+
+ int flags = EVP_CIPHER_CTX_flags(ctx);
+ int mode = EVP_CIPHER_CTX_mode(ctx);
+
+ return mode == EVP_CIPH_CBC_MODE
+ /* Exclude AEAD cipher modes, they require a different API */
+#ifdef EVP_CIPH_FLAG_CTS
+ && !(flags & EVP_CIPH_FLAG_CTS)
+#endif
+ && !(flags & EVP_CIPH_FLAG_AEAD_CIPHER);
+}
+
+bool
+cipher_ctx_mode_ofb_cfb(const cipher_ctx_t *ctx)
+{
+ if (!ctx)
+ {
+ return false;
+ }
+
+ int mode = EVP_CIPHER_CTX_get_mode(ctx);
+
+ return (mode == EVP_CIPH_OFB_MODE || mode == EVP_CIPH_CFB_MODE)
+ /* Exclude AEAD cipher modes, they require a different API */
+ && !(EVP_CIPHER_CTX_flags(ctx) & EVP_CIPH_FLAG_AEAD_CIPHER);
+}
+
+bool
+cipher_ctx_mode_aead(const cipher_ctx_t *ctx)
{
- return ctx ? EVP_CIPHER_CTX_cipher(ctx) : NULL;
+ if (ctx)
+ {
+ int flags = EVP_CIPHER_CTX_flags(ctx);
+ if (flags & EVP_CIPH_FLAG_AEAD_CIPHER)
+ {
+ return true;
+ }
+
+#if defined(NID_chacha20_poly1305) && OPENSSL_VERSION_NUMBER < 0x30000000L
+ if (EVP_CIPHER_CTX_nid(ctx) == NID_chacha20_poly1305)
+ {
+ return true;
+ }
+#endif
+ }
+
+ return false;
}
@@ -757,6 +757,7 @@ int EVP_PKEY_get_group_name(EVP_PKEY *pkey, char *gname, size_t gname_sz,
#if OPENSSL_VERSION_NUMBER < 0x30000000L
#define EVP_MD_get0_name EVP_MD_name
+#define EVP_CIPHER_CTX_get_mode EVP_CIPHER_CTX_mode
/* Mimics the functions but only when the default context without
* options is chosen */
@@ -1803,14 +1803,12 @@ exit:
static void
key_ctx_update_implicit_iv(struct key_ctx *ctx, uint8_t *key, size_t key_len)
{
- const cipher_kt_t *cipher_kt = cipher_ctx_get_cipher_kt(ctx->cipher);
-
/* Only use implicit IV in AEAD cipher mode, where HMAC key is not used */
- if (cipher_kt_mode_aead(cipher_kt))
+ if (cipher_ctx_mode_aead(ctx->cipher))
{
size_t impl_iv_len = 0;
- ASSERT(cipher_kt_iv_size(cipher_kt) >= OPENVPN_AEAD_MIN_IV_LEN);
- impl_iv_len = cipher_kt_iv_size(cipher_kt) - sizeof(packet_id_type);
+ ASSERT(cipher_ctx_iv_length(ctx->cipher) >= OPENVPN_AEAD_MIN_IV_LEN);
+ impl_iv_len = cipher_ctx_iv_length(ctx->cipher) - sizeof(packet_id_type);
ASSERT(impl_iv_len <= OPENVPN_MAX_IV_LENGTH);
ASSERT(impl_iv_len <= key_len);
memcpy(ctx->implicit_iv, key, impl_iv_len);
@@ -466,19 +466,17 @@ p2p_mode_ncp(struct tls_multi *multi, struct tls_session *session)
if (!common_cipher)
{
struct buffer out = alloc_buf_gc(128, &gc);
- struct key_state *ks = get_key_scan(multi, KS_PRIMARY);
+ const cipher_kt_t *cipher = session->opt->key_type.cipher;
- const cipher_ctx_t *ctx = ks->crypto_options.key_ctx_bi.encrypt.cipher;
- const cipher_kt_t *cipher = cipher_ctx_get_cipher_kt(ctx);
- const char *fallback_name = cipher_kt_name(cipher);
+ /* at this point we do not really know if our fallback is
+ * not enabled or if we use 'none' cipher as fallback, so
+ * keep this ambiguity here and print fallback-cipher: none
+ */
- if (!cipher)
+ const char *fallback_name = "none";
+ if (cipher)
{
- /* at this point we do not really know if our fallback is
- * not enabled or if we use 'none' cipher as fallback, so
- * keep this ambiguity here and print fallback-cipher: none
- */
- fallback_name = "none";
+ fallback_name = cipher_kt_name(cipher);
}
buf_printf(&out, "(not negotiated, fallback-cipher: %s)", fallback_name);