From: Matt Caswell Date: Thu, 14 Nov 2019 16:05:19 +0000 (+0000) Subject: EVP_CIPHER_CTX_set_keylen should not succeed if a bad keylen is passed X-Git-Tag: openssl-3.0.0-alpha1~933 X-Git-Url: https://git.librecmc.org/?a=commitdiff_plain;h=d23adad1133c5445cd834cab0e598d7817effc6b;p=oweals%2Fopenssl.git EVP_CIPHER_CTX_set_keylen should not succeed if a bad keylen is passed EVP_CIPHER_CTX_set_keylen() was succeeding even though a bad key length is passed to it. This is because the set_ctx_params() were all accepting this parameter and blindly changing the keylen even though the cipher did not accept a variable key length. Even removing this didn't entirely resolve the issue because set_ctx_params() functions succeed even if passed a parameter they do not recognise. This should fix various issues found by OSSfuzz/Cryptofuzz. Reviewed-by: Shane Lontis (Merged from https://github.com/openssl/openssl/pull/10449) --- diff --git a/crypto/evp/evp_enc.c b/crypto/evp/evp_enc.c index 3346e57dae..45e1dc67c2 100644 --- a/crypto/evp/evp_enc.c +++ b/crypto/evp/evp_enc.c @@ -1026,17 +1026,31 @@ int EVP_DecryptFinal_ex(EVP_CIPHER_CTX *ctx, unsigned char *out, int *outl) int EVP_CIPHER_CTX_set_key_length(EVP_CIPHER_CTX *c, int keylen) { - int ok; - OSSL_PARAM params[2] = { OSSL_PARAM_END, OSSL_PARAM_END }; - size_t len = keylen; + if (c->cipher->prov != NULL) { + int ok; + OSSL_PARAM params[2] = { OSSL_PARAM_END, OSSL_PARAM_END }; + size_t len = keylen; + + if (EVP_CIPHER_CTX_key_length(c) == keylen) + return 1; + + /* Check the cipher actually understands this parameter */ + if (OSSL_PARAM_locate_const(EVP_CIPHER_settable_ctx_params(c->cipher), + OSSL_CIPHER_PARAM_KEYLEN) == NULL) + return 0; - params[0] = OSSL_PARAM_construct_size_t(OSSL_CIPHER_PARAM_KEYLEN, &len); - ok = evp_do_ciph_ctx_setparams(c->cipher, c->provctx, params); + params[0] = OSSL_PARAM_construct_size_t(OSSL_CIPHER_PARAM_KEYLEN, &len); + ok = evp_do_ciph_ctx_setparams(c->cipher, c->provctx, params); - if (ok != EVP_CTRL_RET_UNSUPPORTED) - return ok; + return ok > 0 ? 1 : 0; + } /* TODO(3.0) legacy code follows */ + + /* + * Note there have never been any built-in ciphers that define this flag + * since it was first introduced. + */ if (c->cipher->flags & EVP_CIPH_CUSTOM_KEY_LENGTH) return EVP_CIPHER_CTX_ctrl(c, EVP_CTRL_SET_KEY_LENGTH, keylen, NULL); if (EVP_CIPHER_CTX_key_length(c) == keylen) diff --git a/providers/common/ciphers/cipher_common.c b/providers/common/ciphers/cipher_common.c index d06e7b8004..fe4560192d 100644 --- a/providers/common/ciphers/cipher_common.c +++ b/providers/common/ciphers/cipher_common.c @@ -71,6 +71,34 @@ CIPHER_DEFAULT_GETTABLE_CTX_PARAMS_END(cipher_generic) CIPHER_DEFAULT_SETTABLE_CTX_PARAMS_START(cipher_generic) CIPHER_DEFAULT_SETTABLE_CTX_PARAMS_END(cipher_generic) +/* + * Variable key length cipher functions for OSSL_PARAM settables + */ + +int cipher_var_keylen_set_ctx_params(void *vctx, const OSSL_PARAM params[]) +{ + PROV_CIPHER_CTX *ctx = (PROV_CIPHER_CTX *)vctx; + const OSSL_PARAM *p; + + if (!cipher_generic_set_ctx_params(vctx, params)) + return 0; + p = OSSL_PARAM_locate_const(params, OSSL_CIPHER_PARAM_KEYLEN); + if (p != NULL) { + size_t keylen; + + if (!OSSL_PARAM_get_size_t(p, &keylen)) { + ERR_raise(ERR_LIB_PROV, PROV_R_FAILED_TO_GET_PARAMETER); + return 0; + } + ctx->keylen = keylen; + } + return 1; +} + +CIPHER_DEFAULT_SETTABLE_CTX_PARAMS_START(cipher_var_keylen) +OSSL_PARAM_size_t(OSSL_CIPHER_PARAM_KEYLEN, NULL), +CIPHER_DEFAULT_SETTABLE_CTX_PARAMS_END(cipher_var_keylen) + /*- * AEAD cipher functions for OSSL_PARAM gettables and settables */ @@ -89,7 +117,6 @@ const OSSL_PARAM *cipher_aead_gettable_ctx_params(void) } static const OSSL_PARAM cipher_aead_known_settable_ctx_params[] = { - OSSL_PARAM_size_t(OSSL_CIPHER_PARAM_KEYLEN, NULL), OSSL_PARAM_size_t(OSSL_CIPHER_PARAM_AEAD_IVLEN, NULL), OSSL_PARAM_octet_string(OSSL_CIPHER_PARAM_AEAD_TAG, NULL, 0), OSSL_PARAM_octet_string(OSSL_CIPHER_PARAM_AEAD_TLS1_AAD, NULL, 0), @@ -362,16 +389,6 @@ int cipher_generic_set_ctx_params(void *vctx, const OSSL_PARAM params[]) } ctx->num = num; } - p = OSSL_PARAM_locate_const(params, OSSL_CIPHER_PARAM_KEYLEN); - if (p != NULL) { - size_t keylen; - - if (!OSSL_PARAM_get_size_t(p, &keylen)) { - ERR_raise(ERR_LIB_PROV, PROV_R_FAILED_TO_GET_PARAMETER); - return 0; - } - ctx->keylen = keylen; - } return 1; } diff --git a/providers/common/ciphers/cipher_gcm.c b/providers/common/ciphers/cipher_gcm.c index d7c67e8b6b..f479bc8fda 100644 --- a/providers/common/ciphers/cipher_gcm.c +++ b/providers/common/ciphers/cipher_gcm.c @@ -202,25 +202,6 @@ int gcm_set_ctx_params(void *vctx, const OSSL_PARAM params[]) } } - /* - * TODO(3.0) Temporary solution to address fuzz test crash, which will be - * reworked once the discussion in PR #9510 is resolved. i.e- We need a - * general solution for handling missing parameters inside set_params and - * get_params methods. - */ - p = OSSL_PARAM_locate_const(params, OSSL_CIPHER_PARAM_KEYLEN); - if (p != NULL) { - size_t keylen; - - if (!OSSL_PARAM_get_size_t(p, &keylen)) { - ERR_raise(ERR_LIB_PROV, PROV_R_FAILED_TO_GET_PARAMETER); - return 0; - } - /* The key length can not be modified for gcm mode */ - if (keylen != ctx->keylen) - return 0; - } - return 1; } diff --git a/providers/common/include/prov/ciphercommon.h b/providers/common/include/prov/ciphercommon.h index c9b0034017..1072b577fe 100644 --- a/providers/common/include/prov/ciphercommon.h +++ b/providers/common/include/prov/ciphercommon.h @@ -83,6 +83,8 @@ OSSL_OP_cipher_set_ctx_params_fn cipher_generic_set_ctx_params; OSSL_OP_cipher_gettable_params_fn cipher_generic_gettable_params; OSSL_OP_cipher_gettable_ctx_params_fn cipher_generic_gettable_ctx_params; OSSL_OP_cipher_settable_ctx_params_fn cipher_generic_settable_ctx_params; +OSSL_OP_cipher_set_ctx_params_fn cipher_var_keylen_set_ctx_params; +OSSL_OP_cipher_settable_ctx_params_fn cipher_var_keylen_settable_ctx_params; OSSL_OP_cipher_gettable_ctx_params_fn cipher_aead_gettable_ctx_params; OSSL_OP_cipher_settable_ctx_params_fn cipher_aead_settable_ctx_params; int cipher_generic_get_params(OSSL_PARAM params[], unsigned int md, @@ -119,8 +121,36 @@ const OSSL_DISPATCH alg##kbits##lcmode##_functions[] = { \ { 0, NULL } \ }; -#define IMPLEMENT_generic_cipher(alg, UCALG, lcmode, UCMODE, flags, kbits, \ - blkbits, ivbits, typ) \ +#define IMPLEMENT_var_keylen_cipher_func(alg, UCALG, lcmode, UCMODE, flags, \ + kbits, blkbits, ivbits, typ) \ +const OSSL_DISPATCH alg##kbits##lcmode##_functions[] = { \ + { OSSL_FUNC_CIPHER_NEWCTX, \ + (void (*)(void)) alg##_##kbits##_##lcmode##_newctx }, \ + { OSSL_FUNC_CIPHER_FREECTX, (void (*)(void)) alg##_freectx }, \ + { OSSL_FUNC_CIPHER_DUPCTX, (void (*)(void)) alg##_dupctx }, \ + { OSSL_FUNC_CIPHER_ENCRYPT_INIT, (void (*)(void))cipher_generic_einit }, \ + { OSSL_FUNC_CIPHER_DECRYPT_INIT, (void (*)(void))cipher_generic_dinit }, \ + { OSSL_FUNC_CIPHER_UPDATE, (void (*)(void))cipher_generic_##typ##_update },\ + { OSSL_FUNC_CIPHER_FINAL, (void (*)(void))cipher_generic_##typ##_final }, \ + { OSSL_FUNC_CIPHER_CIPHER, (void (*)(void))cipher_generic_cipher }, \ + { OSSL_FUNC_CIPHER_GET_PARAMS, \ + (void (*)(void)) alg##_##kbits##_##lcmode##_get_params }, \ + { OSSL_FUNC_CIPHER_GET_CTX_PARAMS, \ + (void (*)(void))cipher_generic_get_ctx_params }, \ + { OSSL_FUNC_CIPHER_SET_CTX_PARAMS, \ + (void (*)(void))cipher_var_keylen_set_ctx_params }, \ + { OSSL_FUNC_CIPHER_GETTABLE_PARAMS, \ + (void (*)(void))cipher_generic_gettable_params }, \ + { OSSL_FUNC_CIPHER_GETTABLE_CTX_PARAMS, \ + (void (*)(void))cipher_generic_gettable_ctx_params }, \ + { OSSL_FUNC_CIPHER_SETTABLE_CTX_PARAMS, \ + (void (*)(void))cipher_var_keylen_settable_ctx_params }, \ + { 0, NULL } \ +}; + + +#define IMPLEMENT_generic_cipher_genfn(alg, UCALG, lcmode, UCMODE, flags, \ + kbits, blkbits, ivbits, typ) \ static OSSL_OP_cipher_get_params_fn alg##_##kbits##_##lcmode##_get_params; \ static int alg##_##kbits##_##lcmode##_get_params(OSSL_PARAM params[]) \ { \ @@ -138,9 +168,21 @@ static void * alg##_##kbits##_##lcmode##_newctx(void *provctx) \ } \ return ctx; \ } \ + +#define IMPLEMENT_generic_cipher(alg, UCALG, lcmode, UCMODE, flags, kbits, \ + blkbits, ivbits, typ) \ +IMPLEMENT_generic_cipher_genfn(alg, UCALG, lcmode, UCMODE, flags, kbits, \ + blkbits, ivbits, typ) \ IMPLEMENT_generic_cipher_func(alg, UCALG, lcmode, UCMODE, flags, kbits, \ blkbits, ivbits, typ) +#define IMPLEMENT_var_keylen_cipher(alg, UCALG, lcmode, UCMODE, flags, kbits, \ + blkbits, ivbits, typ) \ +IMPLEMENT_generic_cipher_genfn(alg, UCALG, lcmode, UCMODE, flags, kbits, \ + blkbits, ivbits, typ) \ +IMPLEMENT_var_keylen_cipher_func(alg, UCALG, lcmode, UCMODE, flags, kbits, \ + blkbits, ivbits, typ) + PROV_CIPHER_HW_FN cipher_hw_generic_cbc; PROV_CIPHER_HW_FN cipher_hw_generic_ecb; PROV_CIPHER_HW_FN cipher_hw_generic_ofb128; @@ -262,7 +304,6 @@ const OSSL_PARAM * name##_gettable_ctx_params(void) \ #define CIPHER_DEFAULT_SETTABLE_CTX_PARAMS_START(name) \ static const OSSL_PARAM name##_known_settable_ctx_params[] = { \ - OSSL_PARAM_size_t(OSSL_CIPHER_PARAM_KEYLEN, NULL), \ OSSL_PARAM_uint(OSSL_CIPHER_PARAM_PADDING, NULL), \ OSSL_PARAM_uint(OSSL_CIPHER_PARAM_NUM, NULL), #define CIPHER_DEFAULT_SETTABLE_CTX_PARAMS_END(name) \ diff --git a/providers/implementations/ciphers/cipher_blowfish.c b/providers/implementations/ciphers/cipher_blowfish.c index 786cc15189..1ffb9452a8 100644 --- a/providers/implementations/ciphers/cipher_blowfish.c +++ b/providers/implementations/ciphers/cipher_blowfish.c @@ -39,10 +39,10 @@ static void *blowfish_dupctx(void *ctx) } /* bf_ecb_functions */ -IMPLEMENT_generic_cipher(blowfish, BLOWFISH, ecb, ECB, BF_FLAGS, 128, 64, 0, block) +IMPLEMENT_var_keylen_cipher(blowfish, BLOWFISH, ecb, ECB, BF_FLAGS, 128, 64, 0, block) /* bf_cbc_functions */ -IMPLEMENT_generic_cipher(blowfish, BLOWFISH, cbc, CBC, BF_FLAGS, 128, 64, 64, block) +IMPLEMENT_var_keylen_cipher(blowfish, BLOWFISH, cbc, CBC, BF_FLAGS, 128, 64, 64, block) /* bf_ofb_functions */ -IMPLEMENT_generic_cipher(blowfish, BLOWFISH, ofb64, OFB, BF_FLAGS, 64, 8, 64, stream) +IMPLEMENT_var_keylen_cipher(blowfish, BLOWFISH, ofb64, OFB, BF_FLAGS, 64, 8, 64, stream) /* bf_cfb_functions */ -IMPLEMENT_generic_cipher(blowfish, BLOWFISH, cfb64, CFB, BF_FLAGS, 64, 8, 64, stream) +IMPLEMENT_var_keylen_cipher(blowfish, BLOWFISH, cfb64, CFB, BF_FLAGS, 64, 8, 64, stream) diff --git a/providers/implementations/ciphers/cipher_cast5.c b/providers/implementations/ciphers/cipher_cast5.c index d049d836d0..473a7f02b9 100644 --- a/providers/implementations/ciphers/cipher_cast5.c +++ b/providers/implementations/ciphers/cipher_cast5.c @@ -11,6 +11,7 @@ #include "cipher_cast.h" #include "prov/implementations.h" +#include "prov/providercommonerr.h" #define CAST5_FLAGS (EVP_CIPH_VARIABLE_LENGTH) @@ -39,10 +40,10 @@ static void *cast5_dupctx(void *ctx) } /* cast5128ecb_functions */ -IMPLEMENT_generic_cipher(cast5, CAST, ecb, ECB, CAST5_FLAGS, 128, 64, 0, block) +IMPLEMENT_var_keylen_cipher(cast5, CAST, ecb, ECB, CAST5_FLAGS, 128, 64, 0, block) /* cast5128cbc_functions */ -IMPLEMENT_generic_cipher(cast5, CAST, cbc, CBC, CAST5_FLAGS, 128, 64, 64, block) +IMPLEMENT_var_keylen_cipher(cast5, CAST, cbc, CBC, CAST5_FLAGS, 128, 64, 64, block) /* cast564ofb64_functions */ -IMPLEMENT_generic_cipher(cast5, CAST, ofb64, OFB, CAST5_FLAGS, 64, 8, 64, stream) +IMPLEMENT_var_keylen_cipher(cast5, CAST, ofb64, OFB, CAST5_FLAGS, 64, 8, 64, stream) /* cast564cfb64_functions */ -IMPLEMENT_generic_cipher(cast5, CAST, cfb64, CFB, CAST5_FLAGS, 64, 8, 64, stream) +IMPLEMENT_var_keylen_cipher(cast5, CAST, cfb64, CFB, CAST5_FLAGS, 64, 8, 64, stream) diff --git a/providers/implementations/ciphers/cipher_rc2.c b/providers/implementations/ciphers/cipher_rc2.c index 135c6171ec..604c7ed637 100644 --- a/providers/implementations/ciphers/cipher_rc2.c +++ b/providers/implementations/ciphers/cipher_rc2.c @@ -130,7 +130,7 @@ static int rc2_set_ctx_params(void *vctx, OSSL_PARAM params[]) PROV_RC2_CTX *ctx = (PROV_RC2_CTX *)vctx; const OSSL_PARAM *p; - if (!cipher_generic_set_ctx_params(vctx, params)) + if (!cipher_var_keylen_set_ctx_params(vctx, params)) return 0; p = OSSL_PARAM_locate_const(params, OSSL_CIPHER_PARAM_RC2_KEYBITS); if (p != NULL) { @@ -176,6 +176,7 @@ OSSL_PARAM_size_t(OSSL_CIPHER_PARAM_RC2_KEYBITS, NULL), CIPHER_DEFAULT_GETTABLE_CTX_PARAMS_END(rc2) CIPHER_DEFAULT_SETTABLE_CTX_PARAMS_START(rc2) +OSSL_PARAM_size_t(OSSL_CIPHER_PARAM_KEYLEN, NULL), OSSL_PARAM_size_t(OSSL_CIPHER_PARAM_RC2_KEYBITS, NULL), CIPHER_DEFAULT_SETTABLE_CTX_PARAMS_END(rc2) diff --git a/providers/implementations/ciphers/cipher_rc4.c b/providers/implementations/ciphers/cipher_rc4.c index c3da8b4397..baf34f7b93 100644 --- a/providers/implementations/ciphers/cipher_rc4.c +++ b/providers/implementations/ciphers/cipher_rc4.c @@ -71,13 +71,13 @@ const OSSL_DISPATCH alg##kbits##_functions[] = { \ { OSSL_FUNC_CIPHER_GET_CTX_PARAMS, \ (void (*)(void))cipher_generic_get_ctx_params }, \ { OSSL_FUNC_CIPHER_SET_CTX_PARAMS, \ - (void (*)(void))cipher_generic_set_ctx_params }, \ + (void (*)(void))cipher_var_keylen_set_ctx_params }, \ { OSSL_FUNC_CIPHER_GETTABLE_PARAMS, \ (void (*)(void))cipher_generic_gettable_params }, \ { OSSL_FUNC_CIPHER_GETTABLE_CTX_PARAMS, \ (void (*)(void))cipher_generic_gettable_ctx_params }, \ { OSSL_FUNC_CIPHER_SETTABLE_CTX_PARAMS, \ - (void (*)(void))cipher_generic_settable_ctx_params }, \ + (void (*)(void))cipher_var_keylen_settable_ctx_params }, \ { 0, NULL } \ }; diff --git a/providers/implementations/ciphers/cipher_rc5.c b/providers/implementations/ciphers/cipher_rc5.c index 7f5780f062..e2e1cb6a31 100644 --- a/providers/implementations/ciphers/cipher_rc5.c +++ b/providers/implementations/ciphers/cipher_rc5.c @@ -44,7 +44,7 @@ static int rc5_set_ctx_params(void *vctx, const OSSL_PARAM params[]) PROV_RC5_CTX *ctx = (PROV_RC5_CTX *)vctx; const OSSL_PARAM *p; - if (!cipher_generic_set_ctx_params(vctx, params)) + if (!cipher_var_keylen_set_ctx_params(vctx, params)) return 0; p = OSSL_PARAM_locate_const(params, OSSL_CIPHER_PARAM_ROUNDS); @@ -71,6 +71,7 @@ CIPHER_DEFAULT_GETTABLE_CTX_PARAMS_START(rc5) CIPHER_DEFAULT_GETTABLE_CTX_PARAMS_END(rc5) CIPHER_DEFAULT_SETTABLE_CTX_PARAMS_START(rc5) + OSSL_PARAM_size_t(OSSL_CIPHER_PARAM_KEYLEN, NULL), OSSL_PARAM_uint(OSSL_CIPHER_PARAM_ROUNDS, NULL), CIPHER_DEFAULT_SETTABLE_CTX_PARAMS_END(rc5)