Re: [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  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.






___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [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 +, 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 
#elif defined(OPENSSL_SYS_NETWARE) && !defined(_WINSOCK2API_)
# include 
#elif defined(OPENSSL_SYS_VXWORKS)
# include 
#elif !defined(OPENSSL_SYS_WIN32)
# include 
#endif

While bss_dgram.c just has:
# if !(defined(_WIN32) || defined(OPENSSL_SYS_VMS))
#  include 
# endif
# if defined(OPENSSL_SYS_VMS)
#  include 
# 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


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Kurt Roeckx via RT]

On Tue, Jun 02, 2015 at 03:50:19PM +0200, Pascal Cuoq via RT wrote:
> The attached archive contains a collection of patches for undefined behaviors
> that happen while the tests in directory tests/ are executed, with a recent
> (as of June 2015) OpenSSL git version.
> 
> Each undefined behavior really happens for at least one
> execution, the execution of the test. In other terms, none of these is a
> "false positive". The issues broadly fall in the following categories:

So some of the patches got applied, but I have some comments about
the remaining:

- cast_lcl.h.patch: Your patch has the same effect as defining
  PEDANTIC.  I recommend you at least run your tool with PEDANTIC
  defined.
- 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.
- 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?
  - 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.  It's a little confusing
that it's in the same patch where you can't directly see the
malloc failing.


Kurt


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Salz, Rich via RT]

Generally, these look good.  I have concerns about three (that you raised); 
quoting from your README.  Any comments from others?

+ err.c.patch
The 'int_thread_del_item' function calls 'int_thread_release' that accesses 
(*hash), but this is invalid because  'int_thread_del_item' frees 
'int_thread_hash' that can be an alias of 'hash'. This patch fixes the problem, 
but WARNING: it changes the program behavior since 'int_thread_release' now 
returns earlier and then doesn't call CRYPTO_add. Don't know whether this is 
the correct fix for this problem.

+ mem_dbg.c.patch
The 'pop_info' function return 'ret' after OPENSSL_free(ret), and the returned 
value is then tested (ret = (pop_info() != NULL)) in CRYPTO_pop_info,
which is incorrect since the address is now a dangling pointer (indeterminate 
in the C standard). This patch fixes the problem, but don't know whether this 
is the correct fix regarding the behavior of the 'pop_info' callers. 
Regardless, returning an address that has just been passed to free() is never 
useful and a change is necessary here.

+ Patches about catching memory allocation errors are grouped in malloc.patch
Most of them consist on adding tests about fields being non-NULL before 
accessing to sub-fields, or tests on the returned value of functions that where 
memory allocation may have failed.

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Pascal Cuoq via RT]

The attached archive contains a collection of patches for undefined behaviors
that happen while the tests in directory tests/ are executed, with a recent
(as of June 2015) OpenSSL git version.

Each undefined behavior really happens for at least one
execution, the execution of the test. In other terms, none of these is a
“false positive”. The issues broadly fall in the following categories:

- accessing uninitialized data, sometimes as a result of not testing
the error code of a function (the patch fixes the caller to check for success 
of the
function that's supposed to allocate or initialize);

- dereferencing NULL (often for the same reason of failing to check for success
of called functions);

- using dangling pointers in comparisons as a result of the order in which they
are freed and compared.

A README file discusses the changes for which discussion seems necessary.

The undefined behaviors were found using a Valgrind-like, ASan-like tool to be
released as Open-Source soon: http://trust-in-soft.com/tis-interpreter/



openssl_git_patches.tgz
Description: Binary data
___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev