Re: Thread sanitiser problems

2019-07-30 Thread Viktor Dukhovni
> 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

2019-07-30 Thread Dr Paul Dale
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

2019-07-30 Thread OpenSSL
-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

2019-07-30 Thread Salz, Rich
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

2019-07-30 Thread Matthias St. Pierre

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

2019-07-30 Thread Matthias St. Pierre




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

2019-07-30 Thread Kurt Roeckx
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

2019-07-30 Thread Matthias St. Pierre

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.
(The original FIPS 2.0 implementation was based on low level crypto calls, but 
that was changed by you
to EVP in commit

https://github.com/openssl/openssl/commit/dbdcc04f27db70ac71748eb595ce23c9733afbe7

for performance reasons.)

Matthias


Re: Thread sanitiser problems

2019-07-30 Thread Kurt Roeckx
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

2019-07-30 Thread Matthias St. Pierre

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