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

[Kurt Roeckx via RT]

On Thu, Oct 08, 2015 at 01:36:07PM +0000, Pascal Cuoq via RT wrote:
> > - 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.

I understand that it's a portability issue.  But since it's not
part of the C standard we can't unconditionally include it either
because that would create other protability issues.  d1_lib.c for
instance has:
#if defined(OPENSSL_SYS_VMS)
# include <sys/timeb.h>
#elif defined(OPENSSL_SYS_NETWARE) && !defined(_WINSOCK2API_)
# include <sys/timeval.h>
#elif defined(OPENSSL_SYS_VXWORKS)
# include <sys/times.h>
#elif !defined(OPENSSL_SYS_WIN32)
# include <sys/time.h>
#endif

While bss_dgram.c just has:
# if !(defined(_WIN32) || defined(OPENSSL_SYS_VMS))
#  include <sys/time.h>
# endif
# if defined(OPENSSL_SYS_VMS)
#  include <sys/timeb.h>
# endif

> > - 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.

I see a check for the malloc() return value there, and as far as I
can see it's always been there. 

But you only mention the free calls while the variables are used
at other places without checking that it's NULL or not.  So I
wonder if this is some false positive, which I understand you
actually try to avoid.  So I'm guessing we're missing something,
like calling free after new failed or something.

> - 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.

That would be great.


Kurt