[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
[openssl-dev] [openssl.org #3950] Standard mem* functions called with length 0 and invalid pointer arguments
Recently, GCC began to assume for optimization purposes that p and q are non-null pointers when memcpy(p, q, n); is invoked. This means that the if is eliminated completely when compiling the following sequence of instructions: memcpy(p, q, n); if (!p) printf(good\n); And this causes a problem for any programmer that would expect “good” to be printed by the following program: #include string.h void f(void *p, void *q, size_t n) { memcpy(p, q, n); if (!p) printf(good\n); } int main(void) { f(0, 0, 0); } The clauses in the standard that allow GCC to “optimize” the program in this way are, in C11, 7.24.1:2 and 7.1.4. Clause 7.24.1:2 says: “Where an argument declared as size_t n specifies the length of the array for a function, n can have the value zero on a call to that function. Unless explicitly stated otherwise in the description of a particular function in this subclause, pointer arguments on such a call shall still have valid values, as described in 7.1.4” Clause 7.1.4 also allows compilers to assume that p and q are not pointers “one past” the end of an object: http://stackoverflow.com/questions/25390577/is-memcpya-1-b-1-0-defined-in-c11 Since you can expect GCC developers to take as much responsibility for the vulnerabilities introduced in previously working code when they add the optimization of assuming that p and q are not pointers “one past” than they did when they added the optimization of assuming that p and q are not null (i.e. none at all), it would be prudent never to call any standard function with pointers “one past”, even when these are functions that also take a length and the length is always 0 in these cases. OpenSSL's bignum implementation contains two invocations of standard functions that fail this property: https://github.com/openssl/openssl/blob/b39fc560612984e65ec30d7f37487303bf514fb3/crypto/bn/bn_add.c#L225 https://github.com/openssl/openssl/blob/b39fc560612984e65ec30d7f37487303bf514fb3/crypto/bn/bn_mont.c#L199 These two lines are actually reached with pointers “one past” and sizes of 0 during real executions. The prudent thing to do would be to guard these lines so that the standard function is not called with a pointer “one past”, which can be done as simply as: if (max - r-top) memset(rp[r-top], 0, sizeof(*rp) * (max - r-top)); if (dif) memcpy(rp, ap, sizeof(*rp) * dif); ___ 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
[openssl-dev] [openssl.org #4107] [PATCH] null pointer dereference: bn_wexpand return code not checked in bn_g2fm.c
The function bn_wexpand() can fail. Most of the invocations in bn_g2fm.c are guarded, but three of them aren't, causing a null pointer dereference when bn_wexpand() fails: https://github.com/openssl/openssl/blob/3f6c7691870d1cd2ad0e0c83638cef3f35a0b548/crypto/bn/bn_gf2m.c#L700 If the calls to bn_wexpand() are guarded as in the attached patch, the null pointer dereferences no longer occur. bn_wexpand.patch 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
[openssl-dev] [openssl.org #4100] Overlapping memcpy arguments in bn_add.c
Hello, this is a follow-up to #3891 (https://mta.openssl.org/pipermail/openssl-dev/2015-June/001667.html ). Kurt Roeckx has committed many fixes to the bugs aggregated in that report. Since, we have been replaying the tests in a recent OpenSSL development version, posterior to these commits, to see what issues remained and re-submit them individually with more explanation. This means that #3891 can now be closed (grouping too many fixes in a same entry may not have been such a good idea after all). First, an old problem for which detection was only implemented recently : the memcpy call in bn_add.c can be passed identical pointers, which are thus pointing to overlapping zones. The code has been so for a long time and someone would likely have noticed if this had practical consequences, but in principle, invoking memcpy to copy between overlapping memory zones is undefined behavior even if the overlap is exact. This can be fixed locally as in the attached patch. bn_memcpy_overlap.patch Description: Binary data One actual sequence for which the pointers ap and rp end up being identical is as follows: 1/ probable_prime_dh_safe calls BN_sub(q, q, t1) 2/ in BN_sub, r and a are then aliases 3/ BN_sub calls BN_usub(r, a, b) so r and a are still aliases in BN_usub 4/ in BN_usub, ap = a->d; and rp = r->d; then the 2 pointers can be incremented, but an identical number of times 5/ then memcpy is called with rp and ap that are still aliases, which is undefined behavior___ 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
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
[openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init
As of 2015-10-14, the function PACKET_buf_init in ssl/packet_locl.h reads: static inline int PACKET_buf_init(PACKET *pkt, unsigned char *buf, size_t len) { /* Sanity check for negative values. */ if (buf + len < buf) return 0; pkt->curr = buf; pkt->remaining = len; return 1; } Link: https://github.com/openssl/openssl/blob/310115448188415e270bb0bef958c7c130939838/ssl/packet_locl.h#L113 The rules of pointer arithmetics are such that the condition (buf + len < buf) is always false when it is defined. A modern compiler, or even an old compiler, could suppress the entire conditional from the generated code without a second thought and without a warning. Please read https://lwn.net/Articles/278137/ . Note that in that very similar instance, the GCC developers did not acknowledge any bug in their compiler, did not change the compiler's behavior, and simply regretted that "their project has been singled out in an unfair way". That LWN story is not a about a compiler bug, or in any case, it is about a compiler bug that is here to stay. And according to GCC developers, to be common to several C compilers. As far as I can tell, no current compiler recognizes that the very same reasoning as in that old LWN story justifies the suppression of the conditional. I tested the compilers currently available on https://gcc.godbolt.org . However, any compiler willing to miscompile the examples in the LWN article would gladly miscompile the function PACKET_buf_init given the chance. If the function is intended to return 0 for large values of len, then the test should look like: if (len > (size_t)(SIZE_MAX / 2)) ... Here I have chosen a constant that happens to give a behavior close to the one obtained by luck with current compilers. If another constant makes sense, then that other constant can be used. The current implementation is an invitation for the compiler to generate code that, even when len is above the limit, sets pkt->curr to buf, sets pkt->remaining to len, and returns 1, which is not what is intended, and could have serious security-related consequences latter on. If, as the comment implies, the function is not intended to be called with a len so large that it causes (uintptr_t)buf + len to be lower than (uintptr_t)buf, then please, please, please simplify the function by removing this nonsensical "sanity check", and make this function, which cannot fail, return void-as the history of the rest of OpenSSL shows, remembering of testing all error codes returned by called functions is difficult enough, even when only functions that have reason to fail return such codes. PACKET_buf_init is called with (size_t)-1 for len in test/packettest.c: https://github.com/openssl/openssl/blob/310115448188415e270bb0bef958c7c130939838/test/packettest.c#L341 Since PACKET_buf_init could no longer fail, that test could be simplified, which is good. If a sanity check is indispensable in PACKET_buf_init, but is really a check for something not supposed to happen, then in order to keep the type returned by the function as void, the check could be expressed as: if (len > (size_t)(SIZE_MAX / 2)) abort(); or assert (len <= (size_t)(SIZE_MAX / 2)); ___ 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
[openssl-dev] [openssl.org #4151] [PATCH] Function pop_info in crypto/mem_dbg.c returns a dangling pointer
In crypto/mem_dbg.c, the type of static function pop_info() is to return an APP_INFO*. This pointer can be a dangling pointer, as can be seen in the following link as long as one assumes that line 382 is reachable (it is). https://github.com/openssl/openssl/blob/90945fa31a42dcf3beb90540c618e4d627c595ea/crypto/mem_dbg.c#L382-L386 Returning a dangling pointer from a function is not terribly good style. Using dangling pointers for anything is technically undefined behavior, so there is nothing in theory that a caller can do with the function's result that will not be illegal when a dangling pointer is returned. (It is well-known that dereferencing dangling pointers is undefined behavior, but examples at http://blog.regehr.org/archives/767#comment-4717 and at http://trust-in-soft.com/dangling-pointer-indeterminate/ show that the behavior of a program the *only* fault of which is to compare a dangling pointer to something else can be surprising too). The function pop_info() being static, no-one may use it outside mem_dbg.c and the callers inside mem_dbg.c do not dereference the returned pointer. They do use it in a comparison though, which should ideally be avoided because of the examples above. The attached patch does three small things: - change the type of the function to return an int: 1 if the pointer ret was non-null before being possibly freed, 0 otherwise, and adjust the two call sites correspondingly; - add a one-line comment describing the behavior of the function; - rename the local variable ret, the name of which no longer makes sense, into "current". None of these three things should change the behavior in practice of OpenSSL. As compiled with current C compilers, the tests "pop_info() != NULL" evaluated to 1 when a dangling pointer was returned. These tests were replaced with "pop_info()" which now evaluates to 1 under the circumstances in which a dangling pointer was returned. pop_info.patch 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
[openssl-dev] [openssl.org #4155] In function int_thread_del_item, when hash == int_thread_hash, one is passed to free and the other is used in a comparison
This issue is similar in nature to 4151 (http://www.mail-archive.com/openssl-dev@openssl.org/msg40950.html ): it is about a dangling pointer being used, but not used for dereferencing, so it's not a memory error. The dangling pointer is used in a comparison. The function int_thread_del_item can reach the point were it calls “lh_ERR_STATE_free(int_thread_hash);” with hash == int_thread_hash. That attached patch prints a message like “hash == int_thread_hash == 0xb2a6d0” when this happens. Just after the call to lh_ERR_STATE_free, both hash and int_thread_hash contain dangling pointers. The variable int_thread_hash is immediately set to NULL. The problem that I am reporting is that just afterwards, is passed to the function int_thread_release: https://github.com/openssl/openssl/blob/079a1a9014b89661f0a612a5a9724ad9c77f21a3/crypto/err/err.c#L412 In that function, the argument “hash” points to the local variable “hash” of int_thread_del_item (which contains a dangling pointer). Thus the comparison “*hash == NULL” involves a dangling pointer: https://github.com/openssl/openssl/blob/079a1a9014b89661f0a612a5a9724ad9c77f21a3/crypto/err/err.c#L343 With the attached patch, executing test/enginetest reproduces the problem for me: $ ./enginetest enginetest beginning … Tests completed happily hash == int_thread_hash == 0x1a3f6d0 Now using *hash (0x1a3f6d0) in a comparison show_pointers.patch 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