Re: Thread sanitiser problems
> On Jul 30, 2019, at 10:02 PM, Dr Paul Dale wrote: > > The #9454 description includes thread sanitisizer logs showing different lock > orderings — this has the potential to dead lock. Agreed with Rich that > giving up the lock would make sense, but I don’t see a way for this to be > easily done. My take is that we should never hold any lock long enough to even consider acquiring another lock. No more than one lock should be held at any one time, and only long enough to bump a reference count to ensure that the object of interest is no deallocated before we (or our caller in a "get1" type interface) is done with it. I don't know what "long-term" locks we're holding, but it would be great if it were possible to never (or never recursively) hold any such locks. -- Viktor.
Re: Thread sanitiser problems
Yes, I’m mostly talking about #9454 here. #9455 is a bug (clearing the flush flag after flushing not before). The fix in #9477 addresses this and also removes the dependence on RAND_bytes. The #9454 description includes thread sanitisizer logs showing different lock orderings — this has the potential to dead lock. Agreed with Rich that giving up the lock would make sense, but I don’t see a way for this to be easily done. That this also involves the RAND subsystem could be coincidental. The other stack trace is getting an MD via a digest operation (i.e. both traces are for algorithms that use other algorithms). The locks in question are the provider store lock and the context lock. crypto/context.c: OPENSSL_CTX *ctx; /* function argument */ CRYPTO_THREAD_read_lock(ctx->oncelock); crypto/provider_core.c: OPENSSL_CTX *ctx; /* function argument */ struct provider_store_st *store = get_provider_store(ctx); CRYPTO_THREAD_read_lock(store->lock); Pauli -- Dr Paul Dale | Cryptographer | Network Security & Encryption Phone +61 7 3031 7217 Oracle Australia > On 30 Jul 2019, at 8:52 pm, Matthias St. Pierre > wrote: > > Sorry, my reply was misleading, since Pauli is talking mainly about #9454. > Please take a look at the issue description > > https://github.com/openssl/openssl/issues/9454 > > instead. > > Matthias > > > > On 30.07.19 12:47, Matthias St. Pierre wrote: >> >> >> On 30.07.19 12:43, Kurt Roeckx wrote: >>> >>> I currently fail to see how that's a problem, unless that >>> EVP_CIPHER_CTX tries to use a DRBG. >> >> This is what I mean when I say that things have gotten more complicated >> under the hood >> due to the replumbing. To understand the problem, please take at a look of >> the sanitizer >> callstack in >> >> https://github.com/openssl/openssl/issues/9455 >> >> >> Matthias >> >> >> >
OpenSSL Security Advisory
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 OpenSSL Security Advisory [30 July 2019] Windows builds with insecure path defaults (CVE-2019-1552) == Severity: Low OpenSSL has internal defaults for a directory tree where it can find a configuration file as well as certificates used for verification in TLS. This directory is most commonly referred to as OPENSSLDIR, and is configurable with the --prefix / --openssldir configuration options. For OpenSSL versions 1.1.0 and 1.1.1, the mingw configuration targets assume that resulting programs and libraries are installed in a Unix-like environment and the default prefix for program installation as well as for OPENSSLDIR should be '/usr/local'. However, mingw programs are Windows programs, and as such, find themselves looking at sub-directories of 'C:/usr/local', which may be world writable, which enables untrusted users to modify OpenSSL's default configuration, insert CA certificates, modify (or even replace) existing engine modules, etc. For OpenSSL 1.0.2, '/usr/local/ssl' is used as default for OPENSSLDIR on all Unix and Windows targets, including Visual C builds. However, some build instructions for the diverse Windows targets on 1.0.2 encourage you to specify your own --prefix. OpenSSL versions 1.1.1, 1.1.0 and 1.0.2 are affected by this issue. Due to the limited scope of affected deployments this has been assessed as low severity and therefore we are not creating new releases at this time. The mitigations are found in these commits: - - For 1.1.1, commit 54aa9d51b09d67e90db443f682cface795f5af9e - - For 1.1.0, commit e32bc855a81a2d48d215c506bdeb4f598045f7e9 and b15a19c148384e73338aa7c5b12652138e35ed28 - - For 1.0.2, commit d333ebaf9c77332754a9d5e111e2f53e1de54fdd The 1.1.1 and 1.1.0 mitigation set more appropriate defaults for mingw, while the 1.0.2 mitigation documents the issue and provides enhanced examples. This issue was reported by Rich Mirth. The fix was developed by Richard Levitte from the OpenSSL development team. It was reported to OpenSSL on 9th Jun 2019. Note = OpenSSL 1.0.2 and 1.1.0 are currently only receiving security updates. Support for 1.0.2 will end on 31st December 2019. Support for 1.1.0 will end on 11th September 2019. Users of these versions should upgrade to OpenSSL 1.1.1. Referenses == URL for this Security Advisory: https://www.openssl.org/news/secadv/20190730.txt Note: the online version of the advisory may be updated with additional details over time. For details of OpenSSL severity classifications please see: https://www.openssl.org/policies/secpolicy.html -BEGIN PGP SIGNATURE- iQIzBAEBCgAdFiEEeVOsH7w9yLOykjk+1enkP3357owFAl1AU3sACgkQ1enkP335 7oxnEw//ebb9FK16oXpvW6nifNgSHUBYRaq+3ApvSfGG8Er1M0Zn80iD/WY8wzM7 ZabUUNlOdnOs0iQivMYzy+8QzP9NRaqX2WZk/Q1koNT5WAt9+VDCw6hhbp6FN8B9 9aeRvdawNME9JPysl3KOR6DnYJQnpJgV0yQ2pJM2yMKNuDFkvy6E9ieMoWAGx5Ya 8JZ4KGFubA1vDPj5xowkRDxZo+SLdAaEMQw0YG8DWSK5BViZV+3d4OMAAL1RjnZy s4OSghqi7wUbgo8XO38/roN4y4BEgmEXU0IpSRNf1xrwCoFM82hEgOO3xWxPtbZk EtDcMUTtMYa1g5IMdGIkVvS4wnNr2j2BAi8WECkPf5QCzCoaX/Xc9jutslTw20M/ UoZnyGgVoOQCsO6ECwLUnSEp772mhS1056c4OKb62kfhlIcGkWi5vk5wjWVZFxEx rXJC7xabp29e051mnrJtLr85UWUv5B/ywREPyvbdjWg6lJBxB0dOYXMQLpJi6B5i /bDX7czP/1EeOg+FDSGOR174JGIyMYmPqpyzGpdds72GfOQqtGHC2z41FlvHMglB 9VobSZnF97MIan4/9H4ge+gUUq0PeIZ+invvgCHzuW4oYBOngwwVD5QXfSQUjA9a etYHkJx+3t4hPrPKAT/J0jHA7AbWtYK7dL6qTxSwli2Gl/D4ipk= =gxli -END PGP SIGNATURE-
Re: Thread sanitiser problems
Do you need to hold the lock across dependant items? For example, why can't the DRBG code unlock before fetching the AES-CTR code?
Re: Thread sanitiser problems
Sorry, my reply was misleading, since Pauli is talking mainly about #9454. Please take a look at the issue description https://github.com/openssl/openssl/issues/9454 instead. Matthias On 30.07.19 12:47, Matthias St. Pierre wrote: On 30.07.19 12:43, Kurt Roeckx wrote: I currently fail to see how that's a problem, unless that EVP_CIPHER_CTX tries to use a DRBG. This is what I mean when I say that things have gotten more complicated under the hood due to the replumbing. To understand the problem, please take at a look of the sanitizer callstack in https://github.com/openssl/openssl/issues/9455 Matthias
Re: Thread sanitiser problems
On 30.07.19 12:43, Kurt Roeckx wrote: I currently fail to see how that's a problem, unless that EVP_CIPHER_CTX tries to use a DRBG. This is what I mean when I say that things have gotten more complicated under the hood due to the replumbing. To understand the problem, please take at a look of the sanitizer callstack in https://github.com/openssl/openssl/issues/9455 Matthias
Re: Thread sanitiser problems
On Tue, Jul 30, 2019 at 12:41:16PM +0200, Matthias St. Pierre wrote: > On 30.07.19 11:59, Kurt Roeckx wrote: > > On Tue, Jul 30, 2019 at 12:42:33PM +1000, Dr Paul Dale wrote: > > > Overly simplified, the problem boils down to the CTR DRBG needing an AES > > > CTR cipher context to work. When creating the former, a recursive call > > > is made to get the latter. > > I'm not sure what you mean with "CTR" both times. > > > > Are you saying that an AES requires a DRBG now? > > > > No. Pauli simply meant that the CTR DRBG utilizes an EVP_CIPHER_CTX for its > internal implementation. I currently fail to see how that's a problem, unless that EVP_CIPHER_CTX tries to use a DRBG. Kurt
Re: Thread sanitiser problems
On Tue, Jul 30, 2019 at 12:42:33PM +1000, Dr Paul Dale wrote: > Overly simplified, the problem boils down to the CTR DRBG needing an AES CTR > cipher context to work. When creating the former, a recursive call is made > to get the latter. I'm not sure what you mean with "CTR" both times. Are you saying that an AES requires a DRBG now? Kurt
Re: Thread sanitiser problems
On 30.07.19 04:42, Dr Paul Dale wrote: > Bringing the discussions over to the project list. That's a very good idea Pauli to bring this subject to a wider audience for discussion. I would like to take the opportunity to re-post a general remark which I made in https://github.com/openssl/openssl/issues/9455#issuecomment-515340391 > I am convinced that issues #9454 and #9455 might be only the tip of an iceberg > and we shouldn't just narrow down our focus and fix them as isolated issues. > Instead, the @openssl/omc should take them as an indication that it might be > necessary to pause and rethink the rules for how and when the low level core > routines are allowed to utilize higer level crypto routines (like RAND_bytes()). > Also, locking rules might be necessary to prevent lock-order inversion (#9454 (comment)). > Or it might be necessary to simplify the design, e.g. by replacing the context lock > and the store lock by a single lock. > > There has been a lot of replumbing going on recently and we need to take care that > the overall structure of OpenSSL remains stable and manageable. The double and > recursive lock issues are an indicator that things have become more complicated > "under the hood" (or should I say more appropriately "under the washing stand"?) > The original OpenSSL 3.0.0 Design document is only a snapshot from the very beginning. > It has not changed recently, and it might be a good time now to explitly write down > all the changes and innovations which have taken place since then. Matthias
