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.
This commit is contained in:
Alex Gaynor 2023-03-10 23:14:27 -05:00 committed by GitHub
parent dd43b55882
commit 5e3061c05e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 27 additions and 11 deletions

View file

@ -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

View file

@ -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)

View file

@ -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
),
}

View file

@ -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