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

2015-06-02 Thread 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


[openssl-dev] [openssl.org #3950] Standard mem* functions called with length 0 and invalid pointer arguments

2015-07-22 Thread Pascal Cuoq via RT
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

2015-10-26 Thread Pascal Cuoq via RT
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

2015-10-19 Thread Pascal Cuoq via RT
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

2015-10-08 Thread 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


[openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init

2015-10-14 Thread Pascal Cuoq via RT
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

2015-11-22 Thread Pascal Cuoq via RT
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

2015-11-24 Thread Pascal Cuoq via RT
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