[openssl-dev] [openssl.org #3891] [PATCH] Fix undefined behavior executed through OpenSSL tests

[Pascal Cuoq via RT]

Hello Kurt, thanks for looking into these !

On 07 Oct 2015, at 22:17, Kurt Roeckx via RT <rt at openssl.org> wrote:
> 
> So some of the patches got applied, but I have some comments about
> the remaining:

> - ssl_locl.h.patch: I don't see a struct timeval
>  crypto/x509v3/v3_scts.c.  Does this comment still apply?  Maybe
>  we fixed the issue in some other way.

Sorry, this comment was unnecessarily confusing.

What we meant to say was that the OpenSSL header ssl/ssl_locl.h uses “struct timeval” (at line 1445: https://github.com/openssl/openssl/blob/b3e2272c59a5720467045e2ae62940fdb708ce76/ssl/ssl_locl.h#L1445 ) but the POSIX header that is guaranteed to provide this type, sys/time.h, is not included. This is just a portability matter. It works in practice of most platforms because that header is dragged in by other headers.

> - malloc.patch: I started looking at it, and I have some
>  comments/questions:
>  - I currently don't see a way that s->d1 can be NULL except
>    after an dtls1_free() call.  The same seem to go for
>    DTLS_RECORD_LAYER_free(), ssl3_free(), pkey_hmac_cleanup(),
>    aes_gcm_cleanup() and aes_ocb_cleanup().
>    Are you saying there are cases we could end up calling those
>    twice?

All the patches in the file malloc.patch are for problems, typically null pointer dereferences, that arise when the function malloc() is modeled as either returning a null pointer or a valid pointer to a fresh block. We know this because we also ran all the tests in a mode in which malloc was assumed to always succeed, and in that mode, none of the fixes recommended in malloc.patch were necessary.

So for instance, in the development version of OpenSSL that was current at the time of the report, that “s->d1” could be NULL because a malloc call had returned NULL.

>  - It seems to contain changes to the test suite to check return
>    values.  It seems non-obvious that this is about memory
>    allocation that might have failed, but it's probably the only
>    reasons those failures can happen.

Yes, exactly. The failures of OpenSSL functions that malloc.patch adds tests for happen because of a specific sequence of allocation successes and failures.

>  It's a little confusing
>    that it's in the same patch where you can't directly see the
>    malloc failing.

Yes, it is confusing! While we are confident that each of the null pointer dereferences we observed can happen for a specific sequence of malloc successes and failures (by design of the analysis process), the fixes that we are suggesting may be the wrong ones for the problems we have seen. What we are going to do now that you have already applied many of the patches is:

- replay the same verification process on the most recent version of OpenSSL, and update this bug report with the issues that are still present only;

- and then give you access to the analyzer results in a way that lets you observe the sequence of events leading to the null pointer dereferences, and choose for yourselves the best remedy.