Re: [openssl-dev] [openssl.org #3891] [PATCH] Fix undefined behavior executed through OpenSSL tests
Hello Kurt, thanks for looking into these ! On 07 Oct 2015, at 22:17, Kurt Roeckx via RTwrote: > > 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
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
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
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
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