From 084da16ebc272276557ac31f78b363b33d5e5335 Mon Sep 17 00:00:00 2001 From: Paul Kehrer Date: Sun, 19 Jul 2020 11:33:18 -0500 Subject: [PATCH] disable the osrandom engine on 1.1.1d+ (#5317) * disable the osrandom engine on 1.1.1d+ * skip (and run) some tests on 1.1.1d+ * simplify our conditionals * Update src/_cffi_src/openssl/src/osrandom_engine.c Co-authored-by: Alex Gaynor * words * more words * language * get coverage more cleverly * a word * Update .github/workflows/ci.yml Co-authored-by: Alex Gaynor Co-authored-by: Alex Gaynor --- .github/workflows/ci.yml | 24 ++++++++++--------- CHANGELOG.rst | 3 +++ docs/hazmat/backends/openssl.rst | 6 +++++ src/_cffi_src/openssl/cryptography.py | 9 +++++++ src/_cffi_src/openssl/src/osrandom_engine.c | 7 +++--- .../hazmat/backends/openssl/backend.py | 4 ++-- .../hazmat/bindings/openssl/binding.py | 2 +- tests/hazmat/backends/test_openssl.py | 10 ++++---- 8 files changed, 43 insertions(+), 22 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1e91b725b558..2a91b52ce837 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -14,11 +14,11 @@ jobs: strategy: matrix: PYTHON: - - {VERSION: "2.7", TOXENV: "py27"} - - {VERSION: "3.5", TOXENV: "py35"} - - {VERSION: "3.6", TOXENV: "py36"} - - {VERSION: "3.7", TOXENV: "py37"} - - {VERSION: "3.8", TOXENV: "py38"} + - {VERSION: "2.7", TOXENV: "py27", EXTRA_CFLAGS: ""} + - {VERSION: "3.5", TOXENV: "py35", EXTRA_CFLAGS: ""} + - {VERSION: "3.6", TOXENV: "py36", EXTRA_CFLAGS: ""} + - {VERSION: "3.7", TOXENV: "py37", EXTRA_CFLAGS: ""} + - {VERSION: "3.8", TOXENV: "py38", EXTRA_CFLAGS: "-DUSE_OSRANDOM_RNG_FOR_TESTING"} name: "Python ${{ matrix.PYTHON.VERSION }} on macOS" steps: - uses: actions/checkout@master @@ -40,10 +40,11 @@ jobs: run: | CRYPTOGRAPHY_SUPPRESS_LINK_FLAGS=1 \ LDFLAGS="${HOME}/openssl-macos/lib/libcrypto.a ${HOME}/openssl-macos/lib/libssl.a" \ - CFLAGS="-I${HOME}/openssl-macos/include -Werror -Wno-error=deprecated-declarations -Wno-error=incompatible-pointer-types-discards-qualifiers -Wno-error=unused-function -Wno-error=unused-command-line-argument -mmacosx-version-min=10.10 -march=core2" \ + CFLAGS="-I${HOME}/openssl-macos/include -Werror -Wno-error=deprecated-declarations -Wno-error=incompatible-pointer-types-discards-qualifiers -Wno-error=unused-function -Wno-error=unused-command-line-argument -mmacosx-version-min=10.10 -march=core2 $EXTRA_CFLAGS" \ tox -r -- --color=yes --wycheproof-root=wycheproof env: TOXENV: ${{ matrix.PYTHON.TOXENV }} + EXTRA_CFLAGS: ${{ matrix.PYTHON.EXTRA_CFLAGS }} - name: Upload coverage run: | @@ -58,11 +59,11 @@ jobs: - {ARCH: 'x86', WINDOWS: 'win32'} - {ARCH: 'x64', WINDOWS: 'win64'} PYTHON: - - {VERSION: "2.7", TOXENV: "py27", MSVC_VERSION: "2010"} - - {VERSION: "3.5", TOXENV: "py35", MSVC_VERSION: "2019"} - - {VERSION: "3.6", TOXENV: "py36", MSVC_VERSION: "2019"} - - {VERSION: "3.7", TOXENV: "py37", MSVC_VERSION: "2019"} - - {VERSION: "3.8", TOXENV: "py38", MSVC_VERSION: "2019"} + - {VERSION: "2.7", TOXENV: "py27", MSVC_VERSION: "2010", CL_FLAGS: ""} + - {VERSION: "3.5", TOXENV: "py35", MSVC_VERSION: "2019", CL_FLAGS: ""} + - {VERSION: "3.6", TOXENV: "py36", MSVC_VERSION: "2019", CL_FLAGS: ""} + - {VERSION: "3.7", TOXENV: "py37", MSVC_VERSION: "2019", CL_FLAGS: ""} + - {VERSION: "3.8", TOXENV: "py38", MSVC_VERSION: "2019", CL_FLAGS: "/D USE_OSRANDOM_RNG_FOR_TESTING"} name: "Python ${{ matrix.PYTHON.VERSION }} on ${{ matrix.WINDOWS.WINDOWS }}" steps: - uses: actions/checkout@master @@ -85,6 +86,7 @@ jobs: python .github/workflows/download_openssl.py windows openssl-${{ matrix.WINDOWS.WINDOWS }}-${{ matrix.PYTHON.MSVC_VERSION }} echo "::set-env name=INCLUDE::C:/openssl-${{ matrix.WINDOWS.WINDOWS }}-${{ matrix.PYTHON.MSVC_VERSION }}/include;%INCLUDE%" echo "::set-env name=LIB::C:/openssl-${{ matrix.WINDOWS.WINDOWS }}-${{ matrix.PYTHON.MSVC_VERSION }}/lib;%LIB%" + echo "::set-env name=CL::${{ matrix.PYTHON.CL_FLAGS }}" env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: git clone https://github.com/google/wycheproof diff --git a/CHANGELOG.rst b/CHANGELOG.rst index fe5e6b1652d6..456155a287ab 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -46,6 +46,9 @@ Changelog :meth:`~cryptography.x509.CertificateSigningRequest.get_attribute_for_oid`. * Added support for encoding attributes in certificate signing requests via :meth:`~cryptography.x509.CertificateSigningRequestBuilder.add_attribute`. +* On OpenSSL 1.1.1d and higher ``cryptography`` now uses OpenSSL's + built-in CSPRNG instead of its own OS random engine because these versions of + OpenSSL properly reseed on fork. .. _v2-9-2: diff --git a/docs/hazmat/backends/openssl.rst b/docs/hazmat/backends/openssl.rst index 56121cb55d96..0e695279dbe4 100644 --- a/docs/hazmat/backends/openssl.rst +++ b/docs/hazmat/backends/openssl.rst @@ -68,6 +68,12 @@ greater. OS random engine ---------------- +.. note:: + + As of OpenSSL 1.1.1d its CSPRNG is fork-safe by default. + ``cryptography`` does not compile or load the custom engine on + these versions. + By default OpenSSL uses a user-space CSPRNG that is seeded from system random ( ``/dev/urandom`` or ``CryptGenRandom``). This CSPRNG is not reseeded automatically when a process calls ``fork()``. This can result in situations diff --git a/src/_cffi_src/openssl/cryptography.py b/src/_cffi_src/openssl/cryptography.py index cd583313b431..369c23c74a5c 100644 --- a/src/_cffi_src/openssl/cryptography.py +++ b/src/_cffi_src/openssl/cryptography.py @@ -50,6 +50,14 @@ (OPENSSL_VERSION_NUMBER < 0x10101000 || CRYPTOGRAPHY_IS_LIBRESSL) #define CRYPTOGRAPHY_OPENSSL_LESS_THAN_111B \ (OPENSSL_VERSION_NUMBER < 0x10101020 || CRYPTOGRAPHY_IS_LIBRESSL) +#define CRYPTOGRAPHY_OPENSSL_LESS_THAN_111D \ + (OPENSSL_VERSION_NUMBER < 0x10101040 || CRYPTOGRAPHY_IS_LIBRESSL) +#if (CRYPTOGRAPHY_OPENSSL_LESS_THAN_111D && !defined(OPENSSL_NO_ENGINE)) || \ + defined(USE_OSRANDOM_RNG_FOR_TESTING) +#define CRYPTOGRAPHY_NEEDS_OSRANDOM_ENGINE 1 +#else +#define CRYPTOGRAPHY_NEEDS_OSRANDOM_ENGINE 0 +#endif """ TYPES = """ @@ -60,6 +68,7 @@ static const int CRYPTOGRAPHY_OPENSSL_LESS_THAN_102I; static const int CRYPTOGRAPHY_OPENSSL_LESS_THAN_111; static const int CRYPTOGRAPHY_OPENSSL_LESS_THAN_111B; +static const int CRYPTOGRAPHY_NEEDS_OSRANDOM_ENGINE; static const int CRYPTOGRAPHY_IS_LIBRESSL; """ diff --git a/src/_cffi_src/openssl/src/osrandom_engine.c b/src/_cffi_src/openssl/src/osrandom_engine.c index a0b6685217e2..a84857b86df4 100644 --- a/src/_cffi_src/openssl/src/osrandom_engine.c +++ b/src/_cffi_src/openssl/src/osrandom_engine.c @@ -17,8 +17,9 @@ #include #endif -#ifndef OPENSSL_NO_ENGINE -/* OpenSSL has ENGINE support so build the engine. */ +#if CRYPTOGRAPHY_NEEDS_OSRANDOM_ENGINE +/* OpenSSL has ENGINE support and is older than 1.1.1d (the first version that + * properly implements fork safety in its RNG) so build the engine. */ static const char *Cryptography_osrandom_engine_id = "osrandom"; /**************************************************************************** @@ -650,7 +651,7 @@ int Cryptography_add_osrandom_engine(void) { * to compile the osrandom engine, but we do need some * placeholders */ static const char *Cryptography_osrandom_engine_id = "no-engine-support"; -static const char *Cryptography_osrandom_engine_name = "osrandom_engine disabled due to no engine support"; +static const char *Cryptography_osrandom_engine_name = "osrandom_engine disabled"; int Cryptography_add_osrandom_engine(void) { return 0; diff --git a/src/cryptography/hazmat/backends/openssl/backend.py b/src/cryptography/hazmat/backends/openssl/backend.py index f8f59bbbde75..7e21b3366f85 100644 --- a/src/cryptography/hazmat/backends/openssl/backend.py +++ b/src/cryptography/hazmat/backends/openssl/backend.py @@ -135,7 +135,7 @@ def openssl_assert(self, ok): return binding._openssl_assert(self._lib, ok) def activate_builtin_random(self): - if self._lib.Cryptography_HAS_ENGINE: + if self._lib.CRYPTOGRAPHY_NEEDS_OSRANDOM_ENGINE: # Obtain a new structural reference. e = self._lib.ENGINE_get_default_RAND() if e != self._ffi.NULL: @@ -168,7 +168,7 @@ def _get_osurandom_engine(self): self.openssl_assert(res == 1) def activate_osrandom_engine(self): - if self._lib.Cryptography_HAS_ENGINE: + if self._lib.CRYPTOGRAPHY_NEEDS_OSRANDOM_ENGINE: # Unregister and free the current engine. self.activate_builtin_random() with self._get_osurandom_engine() as e: diff --git a/src/cryptography/hazmat/bindings/openssl/binding.py b/src/cryptography/hazmat/bindings/openssl/binding.py index 4e23cd53f7d0..6c025433d179 100644 --- a/src/cryptography/hazmat/bindings/openssl/binding.py +++ b/src/cryptography/hazmat/bindings/openssl/binding.py @@ -114,7 +114,7 @@ def _register_osrandom_engine(cls): # reliably clear the error queue. Once we clear it here we will # error on any subsequent unexpected item in the stack. cls.lib.ERR_clear_error() - if cls.lib.Cryptography_HAS_ENGINE: + if cls.lib.CRYPTOGRAPHY_NEEDS_OSRANDOM_ENGINE: result = cls.lib.Cryptography_add_osrandom_engine() _openssl_assert(cls.lib, result in (1, 2)) diff --git a/tests/hazmat/backends/test_openssl.py b/tests/hazmat/backends/test_openssl.py index 44fd3db4210d..14b4fb9e6e96 100644 --- a/tests/hazmat/backends/test_openssl.py +++ b/tests/hazmat/backends/test_openssl.py @@ -169,8 +169,8 @@ def test_bn_to_int(self): @pytest.mark.skipif( - backend._lib.Cryptography_HAS_ENGINE == 0, - reason="Requires OpenSSL with ENGINE support") + not backend._lib.CRYPTOGRAPHY_NEEDS_OSRANDOM_ENGINE, + reason="Requires OpenSSL with ENGINE support and OpenSSL < 1.1.1d") class TestOpenSSLRandomEngine(object): def setup(self): # The default RAND engine is global and shared between @@ -292,8 +292,8 @@ def test_activate_osrandom_already_default(self): @pytest.mark.skipif( - backend._lib.Cryptography_HAS_ENGINE == 1, - reason="Requires OpenSSL without ENGINE support") + backend._lib.CRYPTOGRAPHY_NEEDS_OSRANDOM_ENGINE, + reason="Requires OpenSSL without ENGINE support or OpenSSL >=1.1.1d") class TestOpenSSLNoEngine(object): def test_no_engine_support(self): assert backend._ffi.string( @@ -301,7 +301,7 @@ def test_no_engine_support(self): ) == b"no-engine-support" assert backend._ffi.string( backend._lib.Cryptography_osrandom_engine_name - ) == b"osrandom_engine disabled due to no engine support" + ) == b"osrandom_engine disabled" def test_activate_builtin_random_does_nothing(self): backend.activate_builtin_random()