From 5e3061c05ed2e2dca55f1c395df4d6f7c1e101a6 Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Fri, 10 Mar 2023 23:14:27 -0500 Subject: [PATCH] Stop validating keys in ECDH exchange (#8490) The theory here is that we're already doing sufficient validation key loading, and this is purely duplicative. Note that there's at least _some_ validationg that was previously occurring only ECDH, the LowOrderPublic check that can be seen in wycheproof. --- src/_cffi_src/openssl/evp.py | 9 +++++++++ src/cryptography/hazmat/backends/openssl/utils.py | 14 ++++++++++---- .../hazmat/bindings/openssl/_conditional.py | 7 +++++++ tests/wycheproof/test_ecdh.py | 8 +------- 4 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/_cffi_src/openssl/evp.py b/src/_cffi_src/openssl/evp.py index 19bdcf38b..b8a38995c 100644 --- a/src/_cffi_src/openssl/evp.py +++ b/src/_cffi_src/openssl/evp.py @@ -38,6 +38,7 @@ static const long Cryptography_HAS_EVP_DIGESTFINAL_XOF; static const long Cryptography_HAS_300_FIPS; static const long Cryptography_HAS_300_EVP_CIPHER; static const long Cryptography_HAS_EVP_PKEY_DH; +static const long Cryptography_HAS_EVP_PKEY_SET_PEER_EX; """ FUNCTIONS = """ @@ -119,6 +120,7 @@ int EVP_PKEY_keygen_init(EVP_PKEY_CTX *); int EVP_PKEY_keygen(EVP_PKEY_CTX *, EVP_PKEY **); int EVP_PKEY_derive_init(EVP_PKEY_CTX *); int EVP_PKEY_derive_set_peer(EVP_PKEY_CTX *, EVP_PKEY *); +int EVP_PKEY_derive_set_peer_ex(EVP_PKEY_CTX *, EVP_PKEY *, int); int EVP_PKEY_derive(EVP_PKEY_CTX *, unsigned char *, size_t *); int EVP_PKEY_id(const EVP_PKEY *); @@ -198,6 +200,13 @@ static const long Cryptography_HAS_RAW_KEY = 1; static const long Cryptography_HAS_EVP_DIGESTFINAL_XOF = 1; #endif +#if CRYPTOGRAPHY_OPENSSL_300_OR_GREATER +static const long Cryptography_HAS_EVP_PKEY_SET_PEER_EX = 1; +#else +static const long Cryptography_HAS_EVP_PKEY_SET_PEER_EX = 0; +int (*EVP_PKEY_derive_set_peer_ex)(EVP_PKEY_CTX *, EVP_PKEY *, int) = NULL; +#endif + /* This is tied to X25519 support so we reuse the Cryptography_HAS_X25519 conditional to remove it. OpenSSL 1.1.0 didn't have this define, but 1.1.1 will when it is released. We can remove this in the distant diff --git a/src/cryptography/hazmat/backends/openssl/utils.py b/src/cryptography/hazmat/backends/openssl/utils.py index 0a4c29595..019f412c7 100644 --- a/src/cryptography/hazmat/backends/openssl/utils.py +++ b/src/cryptography/hazmat/backends/openssl/utils.py @@ -17,10 +17,16 @@ def _evp_pkey_derive(backend: "Backend", evp_pkey, peer_public_key) -> bytes: ctx = backend._ffi.gc(ctx, backend._lib.EVP_PKEY_CTX_free) res = backend._lib.EVP_PKEY_derive_init(ctx) backend.openssl_assert(res == 1) - res = backend._lib.EVP_PKEY_derive_set_peer(ctx, peer_public_key._evp_pkey) - if res != 1: - errors_with_text = backend._consume_errors_with_text() - raise ValueError("Error computing shared key.", errors_with_text) + + if backend._lib.Cryptography_HAS_EVP_PKEY_SET_PEER_EX: + res = backend._lib.EVP_PKEY_derive_set_peer_ex( + ctx, peer_public_key._evp_pkey, 0 + ) + else: + res = backend._lib.EVP_PKEY_derive_set_peer( + ctx, peer_public_key._evp_pkey + ) + backend.openssl_assert(res == 1) keylen = backend._ffi.new("size_t *") res = backend._lib.EVP_PKEY_derive(ctx, backend._ffi.NULL, keylen) diff --git a/src/cryptography/hazmat/bindings/openssl/_conditional.py b/src/cryptography/hazmat/bindings/openssl/_conditional.py index 0f9977bc1..c34fc3ae6 100644 --- a/src/cryptography/hazmat/bindings/openssl/_conditional.py +++ b/src/cryptography/hazmat/bindings/openssl/_conditional.py @@ -271,6 +271,10 @@ def cryptography_has_get_extms_support() -> typing.List[str]: return ["SSL_get_extms_support"] +def cryptography_has_evp_pkey_set_peer_ex() -> typing.List[str]: + return ["EVP_PKEY_derive_set_peer_ex"] + + # This is a mapping of # {condition: function-returning-names-dependent-on-that-condition} so we can # loop over them and delete unsupported names at runtime. It will be removed @@ -322,4 +326,7 @@ CONDITIONAL_NAMES = { cryptography_has_ssl_op_ignore_unexpected_eof ), "Cryptography_HAS_GET_EXTMS_SUPPORT": cryptography_has_get_extms_support, + "Cryptography_HAS_EVP_PKEY_SET_PEER_EX": ( + cryptography_has_evp_pkey_set_peer_ex + ), } diff --git a/tests/wycheproof/test_ecdh.py b/tests/wycheproof/test_ecdh.py index a797c224f..e2624a45a 100644 --- a/tests/wycheproof/test_ecdh.py +++ b/tests/wycheproof/test_ecdh.py @@ -86,13 +86,7 @@ def test_ecdh(backend, wycheproof): except UnsupportedAlgorithm: return - if wycheproof.valid or ( - wycheproof.acceptable - and not ( - wycheproof.has_flag("LowOrderPublic") - and backend._lib.CRYPTOGRAPHY_OPENSSL_300_OR_GREATER - ) - ): + if wycheproof.valid or wycheproof.acceptable: computed_shared = private_key.exchange(ec.ECDH(), public_key) expected_shared = binascii.unhexlify(wycheproof.testcase["shared"]) assert computed_shared == expected_shared