From 77e08d39e088d4e49062fab366289c5ea8b27f00 Mon Sep 17 00:00:00 2001 From: tb Date: Thu, 28 Sep 2023 11:29:10 +0000 Subject: [PATCH] Fix EVP_CIPHER_CTX_iv_length() In today's episode of "curly nonsense from EVP land" we deal with a quite harmless oversight and a not too bad suboptimal fix, relatively speaking. At some point EVP_CIPHER_{CCM,GCM}_SET_IVLEN was added. It modified some object hanging off of EVP_CIPHER. However, EVP_CIPHER_CTX_iv_length() wasn't taught about this and kept returning the hardcoded default value on the EVP_CIPHER. Once it transpired that a doc fix isn't going to cut it, this was fixed. And of course it's easy to fix: you only have to dive through about three layers of EVP, test and set a flag and handle a control in a couple methods. The upstream fix was done poorly and we begrudgingly have to match the API: the caller is expected to pass a raw pointer next to a 0 length along with EVP_CIPHER_GET_IV_LENGTH and the control handler goes *(int *)ptr = length in full YOLO mode. That's never going to be an issue because of course the caller will always pass a properly aligned pointer backing a sufficient amount of memory. Yes, unlikely to be a real issue, but it could have been done with proper semantics and checks without complicating the code. But why do I even bother to complain? We're used to this. Of note here is that there was some pushback painting other corners of a bikeshed until the reviewer gave up with a resigned That kind of changes the semantics and is one extra complexity level, but [shrug] ok... Anyway, the reason this matters now after so many years is that rust-openssl has an assert, notably added in a +758 -84 commit with the awesome message "Docs" that gets triggered by recent tests added to py-cryptography. Thanks to Alex Gaynor for reporting this. Let me take the opportunity to point out that pyca contributed to improve rust-openssl, in particular its libressl support, quite a bit. That's much appreciated and very noticeable. Regress coverage to follow in subsequent commits. Based on OpenSSL PR #9499 and issue #8330. ok beck jsing PS: A few macros were kept internal for now to avoid impact on the release cycle that is about to finish. They will be exposed after release. --- lib/libcrypto/evp/e_aes.c | 15 ++++++++++++--- lib/libcrypto/evp/e_chacha20poly1305.c | 14 +++++++++++--- lib/libcrypto/evp/evp_lib.c | 17 +++++++++++++++-- lib/libcrypto/evp/evp_local.h | 8 +++++++- 4 files changed, 45 insertions(+), 9 deletions(-) diff --git a/lib/libcrypto/evp/e_aes.c b/lib/libcrypto/evp/e_aes.c index 3d3b1a9d6c2..3d357f01197 100644 --- a/lib/libcrypto/evp/e_aes.c +++ b/lib/libcrypto/evp/e_aes.c @@ -1,4 +1,4 @@ -/* $OpenBSD: e_aes.c,v 1.53 2023/07/07 19:37:53 beck Exp $ */ +/* $OpenBSD: e_aes.c,v 1.54 2023/09/28 11:29:10 tb Exp $ */ /* ==================================================================== * Copyright (c) 2001-2011 The OpenSSL Project. All rights reserved. * @@ -1305,7 +1305,11 @@ aes_gcm_ctrl(EVP_CIPHER_CTX *c, int type, int arg, void *ptr) gctx->tls_aad_len = -1; return 1; - case EVP_CTRL_GCM_SET_IVLEN: + case EVP_CTRL_AEAD_GET_IVLEN: + *(int *)ptr = gctx->ivlen; + return 1; + + case EVP_CTRL_AEAD_SET_IVLEN: if (arg <= 0) return 0; /* Allocate memory for IV if needed */ @@ -1631,6 +1635,7 @@ aes_gcm_cipher(EVP_CIPHER_CTX *ctx, unsigned char *out, #define CUSTOM_FLAGS \ ( EVP_CIPH_FLAG_DEFAULT_ASN1 | EVP_CIPH_CUSTOM_IV | \ + EVP_CIPH_FLAG_CUSTOM_IV_LENGTH | \ EVP_CIPH_FLAG_CUSTOM_CIPHER | EVP_CIPH_ALWAYS_CALL_INIT | \ EVP_CIPH_CTRL_INIT | EVP_CIPH_CUSTOM_COPY ) @@ -1968,7 +1973,11 @@ aes_ccm_ctrl(EVP_CIPHER_CTX *c, int type, int arg, void *ptr) cctx->len_set = 0; return 1; - case EVP_CTRL_CCM_SET_IVLEN: + case EVP_CTRL_AEAD_GET_IVLEN: + *(int *)ptr = 15 - cctx->L; + return 1; + + case EVP_CTRL_AEAD_SET_IVLEN: arg = 15 - arg; case EVP_CTRL_CCM_SET_L: diff --git a/lib/libcrypto/evp/e_chacha20poly1305.c b/lib/libcrypto/evp/e_chacha20poly1305.c index 33d09315e0d..4a393c2458d 100644 --- a/lib/libcrypto/evp/e_chacha20poly1305.c +++ b/lib/libcrypto/evp/e_chacha20poly1305.c @@ -1,4 +1,4 @@ -/* $OpenBSD: e_chacha20poly1305.c,v 1.31 2023/08/24 04:33:08 tb Exp $ */ +/* $OpenBSD: e_chacha20poly1305.c,v 1.32 2023/09/28 11:29:10 tb Exp $ */ /* * Copyright (c) 2022 Joel Sing @@ -18,6 +18,7 @@ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ +#include #include #include @@ -551,6 +552,12 @@ chacha20_poly1305_ctrl(EVP_CIPHER_CTX *ctx, int type, int arg, void *ptr) cpx->nonce_len = sizeof(cpx->nonce); return 1; + case EVP_CTRL_AEAD_GET_IVLEN: + if (cpx->nonce_len > INT_MAX) + return 0; + *(int *)ptr = (int)cpx->nonce_len; + return 1; + case EVP_CTRL_AEAD_SET_IVLEN: if (arg <= 0 || arg > sizeof(cpx->nonce)) return 0; @@ -592,8 +599,9 @@ static const EVP_CIPHER cipher_chacha20_poly1305 = { .key_len = 32, .iv_len = 12, .flags = EVP_CIPH_ALWAYS_CALL_INIT | EVP_CIPH_CTRL_INIT | - EVP_CIPH_CUSTOM_IV | EVP_CIPH_FLAG_AEAD_CIPHER | - EVP_CIPH_FLAG_CUSTOM_CIPHER | EVP_CIPH_FLAG_DEFAULT_ASN1, + EVP_CIPH_CUSTOM_IV | EVP_CIPH_FLAG_CUSTOM_IV_LENGTH | + EVP_CIPH_FLAG_AEAD_CIPHER | EVP_CIPH_FLAG_CUSTOM_CIPHER | + EVP_CIPH_FLAG_DEFAULT_ASN1, .init = chacha20_poly1305_init, .do_cipher = chacha20_poly1305_cipher, .cleanup = chacha20_poly1305_cleanup, diff --git a/lib/libcrypto/evp/evp_lib.c b/lib/libcrypto/evp/evp_lib.c index 24ce1963dfb..f4e46aea412 100644 --- a/lib/libcrypto/evp/evp_lib.c +++ b/lib/libcrypto/evp/evp_lib.c @@ -1,4 +1,4 @@ -/* $OpenBSD: evp_lib.c,v 1.27 2023/07/07 19:37:53 beck Exp $ */ +/* $OpenBSD: evp_lib.c,v 1.28 2023/09/28 11:29:10 tb Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -266,7 +266,20 @@ EVP_CIPHER_iv_length(const EVP_CIPHER *cipher) int EVP_CIPHER_CTX_iv_length(const EVP_CIPHER_CTX *ctx) { - return ctx->cipher->iv_len; + int iv_length = 0; + + if ((ctx->cipher->flags & EVP_CIPH_FLAG_CUSTOM_IV_LENGTH) == 0) + return ctx->cipher->iv_len; + + /* + * XXX - sanity would suggest to pass the size of the pointer along, + * but unfortunately we have to match the other crowd. + */ + if (EVP_CIPHER_CTX_ctrl((EVP_CIPHER_CTX *)ctx, EVP_CTRL_GET_IVLEN, 0, + &iv_length) != 1) + return -1; + + return iv_length; } unsigned char * diff --git a/lib/libcrypto/evp/evp_local.h b/lib/libcrypto/evp/evp_local.h index e0a8afd6b8b..015fbb50a94 100644 --- a/lib/libcrypto/evp/evp_local.h +++ b/lib/libcrypto/evp/evp_local.h @@ -1,4 +1,4 @@ -/* $OpenBSD: evp_local.h,v 1.4 2023/08/11 05:10:35 tb Exp $ */ +/* $OpenBSD: evp_local.h,v 1.5 2023/09/28 11:29:10 tb Exp $ */ /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL * project 2000. */ @@ -61,6 +61,12 @@ __BEGIN_HIDDEN_DECLS +/* XXX - move these to evp.h after unlock. */ +#define EVP_CTRL_GET_IVLEN 0x25 +#define EVP_CIPH_FLAG_CUSTOM_IV_LENGTH 0x400000 + +#define EVP_CTRL_AEAD_GET_IVLEN EVP_CTRL_GET_IVLEN + /* * Don't free md_ctx->pctx in EVP_MD_CTX_cleanup(). Needed for ownership * handling in EVP_MD_CTX_set_pkey_ctx(). -- 2.20.1