From 93c93d800e6205bcce2bfcad1aa17247d97739bb Mon Sep 17 00:00:00 2001 From: Paul Kehrer Date: Sat, 1 Oct 2022 14:00:41 -0500 Subject: [PATCH] add unsafe_skip_rsa_key_validation This allows users to skip RSA key validation when calling load_pem_private_key, load_der_private_key, and RSAPrivateNumbers.private_key. This is a significant performance improvement but is **only safe if you know the key is valid**. If you use this when the key is invalid OpenSSL makes no guarantees about what might happen. Infinite loops, crashes, and all manner of terrible things become possible if that occurs. Beware, beware, beware. --- CHANGELOG.rst | 8 +++ docs/glossary.rst | 5 ++ docs/hazmat/primitives/asymmetric/rsa.rst | 20 +++++-- .../primitives/asymmetric/serialization.rst | 32 +++++++++-- .../hazmat/backends/openssl/backend.py | 53 ++++++++++++++----- .../hazmat/backends/openssl/rsa.py | 9 +++- .../hazmat/primitives/asymmetric/rsa.py | 11 +++- .../hazmat/primitives/serialization/base.py | 12 ++++- tests/conftest.py | 9 ---- tests/hazmat/backends/test_openssl.py | 4 +- tests/hazmat/primitives/test_rsa.py | 16 +++--- tests/wycheproof/test_rsa.py | 3 ++ tests/wycheproof/utils.py | 4 +- 13 files changed, 138 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 8a0196bff2d84..3156ee365f8e0 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -27,6 +27,14 @@ Changelog other X.509 builders has been removed. * Added support for :ref:`disabling the legacy provider in OpenSSL 3.0.x`. +* Added support for disabling RSA key validation checks when loading RSA + keys via + :func:`~cryptography.hazmat.primitives.serialization.load_pem_private_key`, + :func:`~cryptography.hazmat.primitives.serialization.load_der_private_key`, + and + :meth:`~cryptography.hazmat.primitives.asymmetric.rsa.RSAPrivateNumbers.private_key`. + This speeds up key loading but is :term:`unsafe` if you are loading potentially + attacker supplied keys. .. _v38-0-1: diff --git a/docs/glossary.rst b/docs/glossary.rst index b85a61091e388..0fa40245d1b87 100644 --- a/docs/glossary.rst +++ b/docs/glossary.rst @@ -100,6 +100,11 @@ Glossary name. U-labels use unicode characters outside the ASCII range and are encoded as A-labels when stored in certificates. + unsafe + This is a term used to describe an operation where the user must + ensure that the input is correct. Failure to do so can result in + crashes, hangs, and other security issues. + .. _`hardware security module`: https://en.wikipedia.org/wiki/Hardware_security_module .. _`idna`: https://pypi.org/project/idna/ .. _`buffer protocol`: https://docs.python.org/3/c-api/buffer.html diff --git a/docs/hazmat/primitives/asymmetric/rsa.rst b/docs/hazmat/primitives/asymmetric/rsa.rst index d21cb801275fb..2743c6179b9c8 100644 --- a/docs/hazmat/primitives/asymmetric/rsa.rst +++ b/docs/hazmat/primitives/asymmetric/rsa.rst @@ -473,10 +473,24 @@ is unavailable. A `Chinese remainder theorem`_ coefficient used to speed up RSA operations. Calculated as: q\ :sup:`-1` mod p - .. method:: private_key() + .. method:: private_key(*, unsafe_skip_rsa_key_validation=False) - :returns: An instance of - :class:`~cryptography.hazmat.primitives.asymmetric.rsa.RSAPrivateKey`. + :param unsafe_skip_rsa_key_validation: + + .. versionadded:: 39.0.0 + + A ``kwarg`` only argument that defaults to ``False``. If ``True`` + RSA private keys will not be validated. This significantly speeds up + loading the keys, but is is :term:`unsafe` unless you are certain + the key is valid. User supplied keys should never be loaded with + this parameter set to ``True``. If you do load an invalid key this + way and attempt to use it OpenSSL may hang, crash, or otherwise + misbehave. + + :type unsafe_skip_rsa_key_validation: bool + + :returns: An instance of + :class:`~cryptography.hazmat.primitives.asymmetric.rsa.RSAPrivateKey`. Handling partial RSA private keys ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/docs/hazmat/primitives/asymmetric/serialization.rst b/docs/hazmat/primitives/asymmetric/serialization.rst index db3271b90d3cd..b1669c893bbc0 100644 --- a/docs/hazmat/primitives/asymmetric/serialization.rst +++ b/docs/hazmat/primitives/asymmetric/serialization.rst @@ -125,7 +125,7 @@ all begin with ``-----BEGIN {format}-----`` and end with ``-----END extract the public key with :meth:`Certificate.public_key `. -.. function:: load_pem_private_key(data, password) +.. function:: load_pem_private_key(data, password, *, unsafe_skip_rsa_key_validation=False) .. versionadded:: 0.6 @@ -141,7 +141,20 @@ all begin with ``-----BEGIN {format}-----`` and end with ``-----END :param password: The password to use to decrypt the data. Should be ``None`` if the private key is not encrypted. - :type data: :term:`bytes-like` + :type password: :term:`bytes-like` + + :param unsafe_skip_rsa_key_validation: + + .. versionadded:: 39.0.0 + + A ``kwarg`` only argument that defaults to ``False``. If ``True`` + RSA private keys will not be validated. This significantly speeds up + loading the keys, but is is :term:`unsafe` unless you are certain the + key is valid. User supplied keys should never be loaded with this + parameter set to ``True``. If you do load an invalid key this way and + attempt to use it OpenSSL may hang, crash, or otherwise misbehave. + + :type unsafe_skip_rsa_key_validation: bool :returns: One of :class:`~cryptography.hazmat.primitives.asymmetric.ed25519.Ed25519PrivateKey`, @@ -234,7 +247,7 @@ data is binary. DER keys may be in a variety of formats, but as long as you know whether it is a public or private key the loading functions will handle the rest. -.. function:: load_der_private_key(data, password) +.. function:: load_der_private_key(data, password, *, unsafe_skip_rsa_key_validation=False) .. versionadded:: 0.8 @@ -248,6 +261,19 @@ the rest. be ``None`` if the private key is not encrypted. :type password: :term:`bytes-like` + :param unsafe_skip_rsa_key_validation: + + .. versionadded:: 39.0.0 + + A ``kwarg`` only argument that defaults to ``False``. If ``True`` + RSA private keys will not be validated. This significantly speeds up + loading the keys, but is is :term:`unsafe` unless you are certain the + key is valid. User supplied keys should never be loaded with this + parameter set to ``True``. If you do load an invalid key this way and + attempt to use it OpenSSL may hang, crash, or otherwise misbehave. + + :type unsafe_skip_rsa_key_validation: bool + :returns: One of :class:`~cryptography.hazmat.primitives.asymmetric.ed25519.Ed25519PrivateKey`, :class:`~cryptography.hazmat.primitives.asymmetric.x25519.X25519PrivateKey`, diff --git a/src/cryptography/hazmat/backends/openssl/backend.py b/src/cryptography/hazmat/backends/openssl/backend.py index 890e2f9521a98..4cf713a1c0a4c 100644 --- a/src/cryptography/hazmat/backends/openssl/backend.py +++ b/src/cryptography/hazmat/backends/openssl/backend.py @@ -542,8 +542,9 @@ def generate_rsa_private_key( self.openssl_assert(res == 1) evp_pkey = self._rsa_cdata_to_evp_pkey(rsa_cdata) + # We can skip RSA key validation here since we just generated the key return _RSAPrivateKey( - self, rsa_cdata, evp_pkey, self._rsa_skip_check_key + self, rsa_cdata, evp_pkey, unsafe_skip_rsa_key_validation=True ) def generate_rsa_parameters_supported( @@ -556,7 +557,9 @@ def generate_rsa_parameters_supported( ) def load_rsa_private_numbers( - self, numbers: rsa.RSAPrivateNumbers + self, + numbers: rsa.RSAPrivateNumbers, + unsafe_skip_rsa_key_validation: bool, ) -> rsa.RSAPrivateKey: rsa._check_private_key_components( numbers.p, @@ -588,7 +591,10 @@ def load_rsa_private_numbers( evp_pkey = self._rsa_cdata_to_evp_pkey(rsa_cdata) return _RSAPrivateKey( - self, rsa_cdata, evp_pkey, self._rsa_skip_check_key + self, + rsa_cdata, + evp_pkey, + unsafe_skip_rsa_key_validation=unsafe_skip_rsa_key_validation, ) def load_rsa_public_numbers( @@ -653,7 +659,9 @@ def _read_mem_bio(self, bio) -> bytes: bio_data = self._ffi.buffer(buf[0], buf_len)[:] return bio_data - def _evp_pkey_to_private_key(self, evp_pkey) -> PRIVATE_KEY_TYPES: + def _evp_pkey_to_private_key( + self, evp_pkey, unsafe_skip_rsa_key_validation + ) -> PRIVATE_KEY_TYPES: """ Return the appropriate type of PrivateKey given an evp_pkey cdata pointer. @@ -666,7 +674,10 @@ def _evp_pkey_to_private_key(self, evp_pkey) -> PRIVATE_KEY_TYPES: self.openssl_assert(rsa_cdata != self._ffi.NULL) rsa_cdata = self._ffi.gc(rsa_cdata, self._lib.RSA_free) return _RSAPrivateKey( - self, rsa_cdata, evp_pkey, self._rsa_skip_check_key + self, + rsa_cdata, + evp_pkey, + unsafe_skip_rsa_key_validation=unsafe_skip_rsa_key_validation, ) elif ( key_type == self._lib.EVP_PKEY_RSA_PSS @@ -932,13 +943,16 @@ def create_cmac_ctx(self, algorithm: BlockCipherAlgorithm) -> _CMACContext: return _CMACContext(self, algorithm) def load_pem_private_key( - self, data: bytes, password: typing.Optional[bytes] + self, + data: bytes, + password: typing.Optional[bytes], + unsafe_skip_rsa_key_validation: typing.Optional[bool] = None, ) -> PRIVATE_KEY_TYPES: return self._load_key( self._lib.PEM_read_bio_PrivateKey, - self._evp_pkey_to_private_key, data, password, + unsafe_skip_rsa_key_validation, ) def load_pem_public_key(self, data: bytes) -> PUBLIC_KEY_TYPES: @@ -996,7 +1010,10 @@ def load_pem_parameters(self, data: bytes) -> dh.DHParameters: self._handle_key_loading_error() def load_der_private_key( - self, data: bytes, password: typing.Optional[bytes] + self, + data: bytes, + password: typing.Optional[bytes], + unsafe_skip_rsa_key_validation: typing.Optional[bool] = None, ) -> PRIVATE_KEY_TYPES: # OpenSSL has a function called d2i_AutoPrivateKey that in theory # handles this automatically, however it doesn't handle encrypted @@ -1005,15 +1022,17 @@ def load_der_private_key( bio_data = self._bytes_to_bio(data) key = self._evp_pkey_from_der_traditional_key(bio_data, password) if key: - return self._evp_pkey_to_private_key(key) + return self._evp_pkey_to_private_key( + key, unsafe_skip_rsa_key_validation + ) else: # Finally we try to load it with the method that handles encrypted # PKCS8 properly. return self._load_key( self._lib.d2i_PKCS8PrivateKey_bio, - self._evp_pkey_to_private_key, data, password, + unsafe_skip_rsa_key_validation, ) def _evp_pkey_from_der_traditional_key(self, bio_data, password): @@ -1146,7 +1165,9 @@ def _check_keys_correspond(self, key1, key2): if self._lib.EVP_PKEY_cmp(key1._evp_pkey, key2._evp_pkey) != 1: raise ValueError("Keys do not correspond") - def _load_key(self, openssl_read_func, convert_func, data, password): + def _load_key( + self, openssl_read_func, data, password, unsafe_skip_rsa_key_validation + ): mem_bio = self._bytes_to_bio(data) userdata = self._ffi.new("CRYPTOGRAPHY_PASSWORD_DATA *") @@ -1192,7 +1213,9 @@ def _load_key(self, openssl_read_func, convert_func, data, password): password is not None and userdata.called == 1 ) or password is None - return convert_func(evp_pkey) + return self._evp_pkey_to_private_key( + evp_pkey, unsafe_skip_rsa_key_validation + ) def _handle_key_loading_error(self) -> typing.NoReturn: errors = self._consume_errors() @@ -2191,7 +2214,11 @@ def load_pkcs12( if evp_pkey_ptr[0] != self._ffi.NULL: evp_pkey = self._ffi.gc(evp_pkey_ptr[0], self._lib.EVP_PKEY_free) - key = self._evp_pkey_to_private_key(evp_pkey) + # We don't support turning off RSA key validation when loading + # PKCS12 keys + key = self._evp_pkey_to_private_key( + evp_pkey, unsafe_skip_rsa_key_validation=False + ) if x509_ptr[0] != self._ffi.NULL: x509 = self._ffi.gc(x509_ptr[0], self._lib.X509_free) diff --git a/src/cryptography/hazmat/backends/openssl/rsa.py b/src/cryptography/hazmat/backends/openssl/rsa.py index 31cff16204610..694829d2c5f1c 100644 --- a/src/cryptography/hazmat/backends/openssl/rsa.py +++ b/src/cryptography/hazmat/backends/openssl/rsa.py @@ -367,7 +367,12 @@ class _RSAPrivateKey(RSAPrivateKey): _key_size: int def __init__( - self, backend: "Backend", rsa_cdata, evp_pkey, _skip_check_key: bool + self, + backend: "Backend", + rsa_cdata, + evp_pkey, + *, + unsafe_skip_rsa_key_validation: bool, ): res: int # RSA_check_key is slower in OpenSSL 3.0.0 due to improved @@ -375,7 +380,7 @@ def __init__( # since users don't load new keys constantly, but for TESTING we've # added an init arg that allows skipping the checks. You should not # use this in production code unless you understand the consequences. - if not _skip_check_key: + if not unsafe_skip_rsa_key_validation: res = backend._lib.RSA_check_key(rsa_cdata) if res != 1: errors = backend._consume_errors_with_text() diff --git a/src/cryptography/hazmat/primitives/asymmetric/rsa.py b/src/cryptography/hazmat/primitives/asymmetric/rsa.py index 5ffe767cde538..36d360f223df4 100644 --- a/src/cryptography/hazmat/primitives/asymmetric/rsa.py +++ b/src/cryptography/hazmat/primitives/asymmetric/rsa.py @@ -354,12 +354,19 @@ def iqmp(self) -> int: def public_numbers(self) -> "RSAPublicNumbers": return self._public_numbers - def private_key(self, backend: typing.Any = None) -> RSAPrivateKey: + def private_key( + self, + backend: typing.Any = None, + *, + unsafe_skip_rsa_key_validation: bool = False, + ) -> RSAPrivateKey: from cryptography.hazmat.backends.openssl.backend import ( backend as ossl, ) - return ossl.load_rsa_private_numbers(self) + return ossl.load_rsa_private_numbers( + self, unsafe_skip_rsa_key_validation + ) def __eq__(self, other: object) -> bool: if not isinstance(other, RSAPrivateNumbers): diff --git a/src/cryptography/hazmat/primitives/serialization/base.py b/src/cryptography/hazmat/primitives/serialization/base.py index 059b6e40f46d0..8a841766404f6 100644 --- a/src/cryptography/hazmat/primitives/serialization/base.py +++ b/src/cryptography/hazmat/primitives/serialization/base.py @@ -16,10 +16,14 @@ def load_pem_private_key( data: bytes, password: typing.Optional[bytes], backend: typing.Any = None, + *, + unsafe_skip_rsa_key_validation: bool = False, ) -> PRIVATE_KEY_TYPES: from cryptography.hazmat.backends.openssl.backend import backend as ossl - return ossl.load_pem_private_key(data, password) + return ossl.load_pem_private_key( + data, password, unsafe_skip_rsa_key_validation + ) def load_pem_public_key( @@ -42,10 +46,14 @@ def load_der_private_key( data: bytes, password: typing.Optional[bytes], backend: typing.Any = None, + *, + unsafe_skip_rsa_key_validation: bool = False, ) -> PRIVATE_KEY_TYPES: from cryptography.hazmat.backends.openssl.backend import backend as ossl - return ossl.load_der_private_key(data, password) + return ossl.load_der_private_key( + data, password, unsafe_skip_rsa_key_validation + ) def load_der_public_key( diff --git a/tests/conftest.py b/tests/conftest.py index 9049922ba51f8..a85b41ff9a0fc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -46,12 +46,3 @@ def backend(request): # Ensure the error stack is clear after the test errors = openssl_backend._consume_errors_with_text() assert not errors - - -@pytest.fixture -def disable_rsa_checks(backend): - # Use this fixture to skip RSA key checks in tests that need the - # performance. - backend._rsa_skip_check_key = True - yield - backend._rsa_skip_check_key = False diff --git a/tests/hazmat/backends/test_openssl.py b/tests/hazmat/backends/test_openssl.py index 7830019cac6a5..3d294dcbfcad2 100644 --- a/tests/hazmat/backends/test_openssl.py +++ b/tests/hazmat/backends/test_openssl.py @@ -477,7 +477,9 @@ def test_pem_password_cb_no_password(self): def test_unsupported_evp_pkey_type(self): key = backend._create_evp_pkey_gc() with raises_unsupported_algorithm(None): - backend._evp_pkey_to_private_key(key) + backend._evp_pkey_to_private_key( + key, unsafe_skip_rsa_key_validation=False + ) with raises_unsupported_algorithm(None): backend._evp_pkey_to_public_key(key) diff --git a/tests/hazmat/primitives/test_rsa.py b/tests/hazmat/primitives/test_rsa.py index 6f083cbcb5416..5a9fa19f37b47 100644 --- a/tests/hazmat/primitives/test_rsa.py +++ b/tests/hazmat/primitives/test_rsa.py @@ -487,7 +487,7 @@ class TestRSASignature: ), skip_message="Does not support SHA1 signature.", ) - def test_pkcs1v15_signing(self, backend, disable_rsa_checks, subtests): + def test_pkcs1v15_signing(self, backend, subtests): vectors = _flatten_pkcs1_examples( load_vectors_from_file( os.path.join("asymmetric", "RSA", "pkcs1v15sign-vectors.txt"), @@ -506,7 +506,7 @@ def test_pkcs1v15_signing(self, backend, disable_rsa_checks, subtests): public_numbers=rsa.RSAPublicNumbers( e=private["public_exponent"], n=private["modulus"] ), - ).private_key(backend) + ).private_key(backend, unsafe_skip_rsa_key_validation=True) signature = private_key.sign( binascii.unhexlify(example["message"]), padding.PKCS1v15(), @@ -1682,9 +1682,7 @@ class TestRSADecryption: ), skip_message="Does not support PKCS1v1.5.", ) - def test_decrypt_pkcs1v15_vectors( - self, backend, disable_rsa_checks, subtests - ): + def test_decrypt_pkcs1v15_vectors(self, backend, subtests): vectors = _flatten_pkcs1_examples( load_vectors_from_file( os.path.join("asymmetric", "RSA", "pkcs1v15crypt-vectors.txt"), @@ -1703,7 +1701,7 @@ def test_decrypt_pkcs1v15_vectors( public_numbers=rsa.RSAPublicNumbers( e=private["public_exponent"], n=private["modulus"] ), - ).private_key(backend) + ).private_key(backend, unsafe_skip_rsa_key_validation=True) ciphertext = binascii.unhexlify(example["encryption"]) assert len(ciphertext) == (skey.key_size + 7) // 8 message = skey.decrypt(ciphertext, padding.PKCS1v15()) @@ -1804,9 +1802,7 @@ def test_decrypt_oaep_vectors(self, subtests, backend): "Does not support OAEP using SHA224 MGF1 and SHA224 hash." ), ) - def test_decrypt_oaep_sha2_vectors( - self, backend, disable_rsa_checks, subtests - ): + def test_decrypt_oaep_sha2_vectors(self, backend, subtests): vectors = _build_oaep_sha2_vectors() for private, public, example, mgf1_alg, hash_alg in vectors: with subtests.test(): @@ -1820,7 +1816,7 @@ def test_decrypt_oaep_sha2_vectors( public_numbers=rsa.RSAPublicNumbers( e=private["public_exponent"], n=private["modulus"] ), - ).private_key(backend) + ).private_key(backend, unsafe_skip_rsa_key_validation=True) message = skey.decrypt( binascii.unhexlify(example["encryption"]), padding.OAEP( diff --git a/tests/wycheproof/test_rsa.py b/tests/wycheproof/test_rsa.py index 7925a5bf91b87..0670e1c47c005 100644 --- a/tests/wycheproof/test_rsa.py +++ b/tests/wycheproof/test_rsa.py @@ -98,6 +98,7 @@ def test_rsa_pkcs1v15_signature_generation(backend, wycheproof): wycheproof.testgroup["privateKeyPem"].encode(), password=None, backend=backend, + unsafe_skip_rsa_key_validation=True, ) assert isinstance(key, rsa.RSAPrivateKey) digest = _DIGESTS[wycheproof.testgroup["sha"]] @@ -193,6 +194,7 @@ def test_rsa_oaep_encryption(backend, wycheproof): wycheproof.testgroup["privateKeyPem"].encode("ascii"), password=None, backend=backend, + unsafe_skip_rsa_key_validation=True, ) assert isinstance(key, rsa.RSAPrivateKey) digest = _DIGESTS[wycheproof.testgroup["sha"]] @@ -228,6 +230,7 @@ def test_rsa_pkcs1_encryption(backend, wycheproof): wycheproof.testgroup["privateKeyPem"].encode("ascii"), password=None, backend=backend, + unsafe_skip_rsa_key_validation=True, ) assert isinstance(key, rsa.RSAPrivateKey) diff --git a/tests/wycheproof/utils.py b/tests/wycheproof/utils.py index 3c18e62afa435..eebbe7ce3bf6e 100644 --- a/tests/wycheproof/utils.py +++ b/tests/wycheproof/utils.py @@ -3,9 +3,7 @@ def wycheproof_tests(*paths): def wrapper(func): - def run_wycheproof( - backend, disable_rsa_checks, subtests, pytestconfig - ): + def run_wycheproof(backend, subtests, pytestconfig): wycheproof_root = pytestconfig.getoption( "--wycheproof-root", skip=True )