[openssl.org #3150] Bug Report (with trivial fix): fips module segfault

2013-10-23 Thread Stephen Henson via RT 
On Wed Oct 23 21:06:00 2013, mco...@akamai.com wrote:
>
> For my curiosity, what's difficult about modifying FIPS? More involved
> change-vetting process?
>

Any change has to be approved as part of a change letter process with labs
which takes time and costs real money. We normally try to include any fixes
whenever someone wants a change letter for another purpose.

Changes to what is deemed "security sensitive code" can't be done at all
without a complete revalidation.

Steve.
--
Dr Stephen N. Henson. OpenSSL project core developer.
Commercial tech support now available see: http://www.openssl.org

__
OpenSSL Project http://www.openssl.org
Development Mailing List   openssl-dev@openssl.org
Automated List Manager   majord...@openssl.org

Re: Self-initialization of locking/threadid callbacks and auto-detection of features

2013-10-23 Thread Peter Waltenberg 
No, multiple independently developed libraries in the same process space
calling the same crypto. code was the problem.

Multiple thread models can't work if they call common code, agreed
there :).

The problem we hit early on was that as a library the only way we could
ensure the stack above us was stable was to use the OS default locking
scheme internally, nothing else would work once the complexity started
climbing. We did it as you suggest, hooked the shared library 'init' entry
points and created the locks when the library was loaded.



Peter




From:   Kurt Roeckx 
To: openssl-dev@openssl.org,
Date:   24/10/2013 06:44
Subject:Re: Self-initialization of locking/threadid callbacks and
auto-detection of features
Sent by:owner-openssl-...@openssl.org



On Wed, Oct 23, 2013 at 12:59:53AM -0500, Nico Williams wrote:
> On Wed, Oct 23, 2013 at 08:32:35AM +1000, Peter Waltenberg wrote:
> > There is no 'safe' way to do this other than hardwired. Admitted, we
have a
> > fairly ugly stack on which to find that out, multiple independently
> > developed lumps of code jammed into the same process, quite a few using
> > dlopen()/dlclose() on other libraries - multiples of them calling the
> > crypto. code.
>
> Oh, good point.
>
> I think what I'll do is add targets that denote "always use the OS
> thread library; disallow setting these callbacks", and a corresponding
> command-line option to ./config.  This should be the best option in
> general because of the possibility of the text for callbacks being
> unmapped when the provider gets dlclose()ed.
>
> Then maybe there's no need to bother with the pthread_once()/
> InitOnceExecuteOnce() business.  I had assumed, going in, that I needed
> to preserve existing semantics as much as possible, but because that
> might still be the case (even if you're right as to what the ideal
> should be) I will do *both*.  (Who knows, maybe there's a program out
> there that insists on using the gnu pth library and not the OS' native
> threading library.  Or maybe there's no need to support such oddities.)

You're conserned that you might have 2 libraries in your address
space implementing pthreads?  That might of course happen, but
unless they're using symbol versioning it's going to fail.  So I
suggest you forget about it and let whoever wants to do that fix
things.


Kurt

__
OpenSSL Project http://www.openssl.org
Development Mailing List   openssl-dev@openssl.org
Automated List Manager   majord...@openssl.org



__
OpenSSL Project http://www.openssl.org
Development Mailing List   openssl-dev@openssl.org
Automated List Manager   majord...@openssl.org

Re: Self-initialization of locking/threadid callbacks and auto-detection of features

2013-10-23 Thread Kurt Roeckx 
On Wed, Oct 23, 2013 at 12:59:53AM -0500, Nico Williams wrote:
> On Wed, Oct 23, 2013 at 08:32:35AM +1000, Peter Waltenberg wrote:
> > There is no 'safe' way to do this other than hardwired. Admitted, we have a
> > fairly ugly stack on which to find that out, multiple independently
> > developed lumps of code jammed into the same process, quite a few using
> > dlopen()/dlclose() on other libraries - multiples of them calling the
> > crypto. code.
> 
> Oh, good point.
> 
> I think what I'll do is add targets that denote "always use the OS
> thread library; disallow setting these callbacks", and a corresponding
> command-line option to ./config.  This should be the best option in
> general because of the possibility of the text for callbacks being
> unmapped when the provider gets dlclose()ed.
> 
> Then maybe there's no need to bother with the pthread_once()/
> InitOnceExecuteOnce() business.  I had assumed, going in, that I needed
> to preserve existing semantics as much as possible, but because that
> might still be the case (even if you're right as to what the ideal
> should be) I will do *both*.  (Who knows, maybe there's a program out
> there that insists on using the gnu pth library and not the OS' native
> threading library.  Or maybe there's no need to support such oddities.)

You're conserned that you might have 2 libraries in your address
space implementing pthreads?  That might of course happen, but
unless they're using symbol versioning it's going to fail.  So I
suggest you forget about it and let whoever wants to do that fix
things.


Kurt

__
OpenSSL Project http://www.openssl.org
Development Mailing List   openssl-dev@openssl.org
Automated List Manager   majord...@openssl.org

Re: [openssl.org #3150] Bug Report (with trivial fix): fips module segfault

2013-10-23 Thread Micah Cowan via RT 
On 10/23/2013 06:16 AM, Stephen Henson via RT wrote:
> What version of OpenSSL are you using? This was worked around in 1.0.1e due to
> the difficulty of changing the FIPS module.

Ah, okay; I see the drbg_free_entropy functions are checking for NULL
there now, which works (even though it's probably still FIPS's bad).

We're using (modified) Ubuntu Precise's openssl1.0.0 (really 1.0.1)
debian package, which looks to have cherry-picked security fixes from
1.0.1e (and prior), but probably didn't grab the FIPS stuff under
consideration of the fact that they don't _build_ with FIPS stuff.

For my curiosity, what's difficult about modifying FIPS? More involved
change-vetting process?

-mjc


__
OpenSSL Project http://www.openssl.org
Development Mailing List   openssl-dev@openssl.org
Automated List Manager   majord...@openssl.org

Re: ssleay PRNG entropy

2013-10-23 Thread Richard Könning 

Am 23.10.2013 18:49, schrieb Fedor Indutny:

Hello Richard,

Yes, I see what this comment means. But what's the difference between
RAND_bytes() and RAND_pseudo_bytes() then? They seems to be using
exactly the same amount of entropy and can't ever fail or return `0`
(meaning that data is insecure).


When i don't overlook something the difference is only in the 
initialisation phase when the entropy pool hasn't reached a specific 
entropy limit at least once. Calling ssleay_rand_bytes with pseudo = 0 
in this phase will result in an error entry, calling it with pseudo = 1 
will give only the zero return code.



In my opinion, current implementation could be a RAND_pseudo_bytes()
backend, and RAND_bytes() should be something more secure (considering
that it is how its described in man documentation).


Well, my impression is that the creators of the respective code consider 
the bytes delivered secure enough after the entropy pool has been 
sufficiently seeded once.

Someone who doesn't share this opinion is free to do additional seedings.
Ciao,
Richard
__
OpenSSL Project http://www.openssl.org
Development Mailing List   openssl-dev@openssl.org
Automated List Manager   majord...@openssl.org

Re: ssleay PRNG entropy

2013-10-23 Thread Fedor Indutny 
Hello Richard,

Yes, I see what this comment means. But what's the difference between
RAND_bytes() and RAND_pseudo_bytes() then? They seems to be using exactly
the same amount of entropy and can't ever fail or return `0` (meaning that
data is insecure).

In my opinion, current implementation could be a RAND_pseudo_bytes()
backend, and RAND_bytes() should be something more secure (considering that
it is how its described in man documentation).

Cheers,
Fedor.


On Wed, Oct 23, 2013 at 6:53 PM, Richard Könning <
richard.koenn...@ts.fujitsu.com> wrote:

> Am 21.10.2013 13:09, schrieb Fedor Indutny:
>
>  Hello devs!
>>
>> I just found that its impossible to get error from `RAND_bytes()` if
>> running on default `RAND_SSLeay()` method.
>>
>> There're a couple of reasons and observations, that are confirming it
>> (sorry for using github, its just more convenient to me):
>>
>> 1. `RAND_poll()` is called only once in initialization of method:
>> https://github.com/openssl/**openssl/blob/master/crypto/**
>> rand/md_rand.c#L436-L440
>> and
>> https://github.com/openssl/**openssl/blob/master/crypto/**
>> rand/md_rand.c#L648-L652
>> 2. Static variable `entropy`, which is used to determine if the PRNG
>> output is secure is never decreased, and actually stays exactly at
>> `ENTROPY_NEEDED` value all the time. This happens because `entropy -=
>> ...` happens only in following condition:
>> https://github.com/openssl/**openssl/blob/master/crypto/**
>> rand/md_rand.c#L446-L463
>> ,
>> which is always true.
>>
>> I think I can contribute a patch to make it work properly, if this isn't
>> an intended behavior.
>>
>
> Well, the comment in the code states it imho clearly that this *is*
> intended behavior:
>
>  * Once we've had enough initial seeding we don't bother to
>> * adjust the entropy count, though, because we're not ambitious
>> * to provide *information-theoretic* randomness.
>>
>
> Ciao
> Richard
> __**__**__
> OpenSSL Project http://www.openssl.org
> Development Mailing List   openssl-dev@openssl.org
> Automated List Manager   majord...@openssl.org
>

Re: ssleay PRNG entropy

2013-10-23 Thread Richard Könning 

Am 21.10.2013 13:09, schrieb Fedor Indutny:

Hello devs!

I just found that its impossible to get error from `RAND_bytes()` if
running on default `RAND_SSLeay()` method.

There're a couple of reasons and observations, that are confirming it
(sorry for using github, its just more convenient to me):

1. `RAND_poll()` is called only once in initialization of method:
https://github.com/openssl/openssl/blob/master/crypto/rand/md_rand.c#L436-L440
and
https://github.com/openssl/openssl/blob/master/crypto/rand/md_rand.c#L648-L652
2. Static variable `entropy`, which is used to determine if the PRNG
output is secure is never decreased, and actually stays exactly at
`ENTROPY_NEEDED` value all the time. This happens because `entropy -=
...` happens only in following condition:
https://github.com/openssl/openssl/blob/master/crypto/rand/md_rand.c#L446-L463,
which is always true.

I think I can contribute a patch to make it work properly, if this isn't
an intended behavior.


Well, the comment in the code states it imho clearly that this *is* 
intended behavior:



* Once we've had enough initial seeding we don't bother to
* adjust the entropy count, though, because we're not ambitious
* to provide *information-theoretic* randomness.


Ciao
Richard
__
OpenSSL Project http://www.openssl.org
Development Mailing List   openssl-dev@openssl.org
Automated List Manager   majord...@openssl.org

Re: ssleay PRNG entropy

2013-10-23 Thread Fedor Indutny 
Hello again,

Is there any way to speed up discussion on this topic?

Cheers,
Fedor.


On Mon, Oct 21, 2013 at 3:09 PM, Fedor Indutny  wrote:

> Hello devs!
>
> I just found that its impossible to get error from `RAND_bytes()` if
> running on default `RAND_SSLeay()` method.
>
> There're a couple of reasons and observations, that are confirming it
> (sorry for using github, its just more convenient to me):
>
> 1. `RAND_poll()` is called only once in initialization of method:
> https://github.com/openssl/openssl/blob/master/crypto/rand/md_rand.c#L436-L440and
> https://github.com/openssl/openssl/blob/master/crypto/rand/md_rand.c#L648-L652
> 2. Static variable `entropy`, which is used to determine if the PRNG
> output is secure is never decreased, and actually stays exactly at
> `ENTROPY_NEEDED` value all the time. This happens because `entropy -= ...`
> happens only in following condition:
> https://github.com/openssl/openssl/blob/master/crypto/rand/md_rand.c#L446-L463,
> which is always true.
>
> I think I can contribute a patch to make it work properly, if this isn't
> an intended behavior.
>
> Basically, to my mind, if condition in pt.2 should be removed and
> `RAND_poll()` should be called when there're not enough entropy. But
> that'll lead to enormous amounts of `RAND_poll()` calls, which will make
> performance worse that it really is.
>
> Any thoughts, opinions?
>
> Thank you,
> Fedor.
>

[openssl.org #3150] Bug Report (with trivial fix): fips module segfault

2013-10-23 Thread Stephen Henson via RT 
On Wed Oct 23 08:59:59 2013, mco...@akamai.com wrote:
> *
> Issue:*
> The fips module has a bug that can result in segfaults when
> fips_get_entropy() fails during initialization of openssl-linked-with-fips.
>

What version of OpenSSL are you using? This was worked around in 1.0.1e due to
the difficulty of changing the FIPS module.

See PR#2786 for details.

Steve.
--
Dr Stephen N. Henson. OpenSSL project core developer.
Commercial tech support now available see: http://www.openssl.org

__
OpenSSL Project http://www.openssl.org
Development Mailing List   openssl-dev@openssl.org
Automated List Manager   majord...@openssl.org

[openssl.org #3150] Bug Report (with trivial fix): fips module segfault

2013-10-23 Thread Micah Cowan via RT 
*Version:
*This bug was found in openssl-fips 2.0.2; I looked in 2.0.5, and the
problem appears to be present there still.
*
Issue:*
The fips module has a bug that can result in segfaults when
fips_get_entropy() fails during initialization of openssl-linked-with-fips.

*Fix:
*Because the fix is extremely trivial, I'll identify it first, and give
the rationale afterwards (which may in fact be better explained by a
patch than by my detailed analysis that follows). The patch to cure this
problem is simply:

--- fips/rand/fips_drbg_lib.c.orig  2013-10-22 11:11:28.0 -0700
+++ fips/rand/fips_drbg_lib.c   2013-10-22 11:20:29.0 -0700
@@ -160,7 +160,8 @@ static size_t fips_get_entropy(DRBG_CTX
return dctx->get_entropy(dctx, pout, entropy, min_len,
max_len);
rv = dctx->get_entropy(dctx, &tout, entropy + bl,
min_len + bl, max_len + bl);
-   *pout = tout + bl;
+   if (tout != NULL)
+   *pout = tout + bl;
if (rv < (min_len + bl) || (rv % bl))
return 0;
/* Compare consecutive blocks for continuous PRNG test */


/White space is NOT correct in the above (I'm afraid tabs have been
replaced with spaces); making the described change manually will almost
certainly work better than copy/pasting the above snip into patch's stdin./

*Reproducibility:*
I was not able to find a convenient way to reproduce; HOWEVER, it is
easy to see that there is a problem in the code that I will point out to
you. In our case, we could only get the bug to exercise on one machine
(not another), and it would disappear when we ran it through a tool like
valgrind. Morover, it seemed to appear during initialization in one
program (curl), but didn't appear when an attempt was made to exactly
duplicate the openssl initialization in a small test program (that did
nothing more than initialize openssl. In other words, reproduction was
difficult.

The bug was later determined to only occur when the call to
dctx->get_entropy failed, called within fips_get_entropy, so if you can
reliably cause that to fail, you should be able to reliably reproduce
the issue. Perhaps by reading from /dev/random until it blocks, just
before attempting openssl (with fips) initialization?

*Bug Profile:*
All of this takes place within FIPS_drbg_instantiate, and the functions
called there, all located in the source file fips/rand/fips_drbg_lib.c.
All line numbers are as of openssl-fips 2.0.5, and refer to lines within
fips_drbg_lib.c.

FIPS_drbg_instantiate (beginning at 194) calls fips_get_entropy (at line
237):

entlen = fips_get_entropy(dctx, &entropy, dctx->strength,
dctx->min_entropy, dctx->max_entropy);

fips_get_entropy has this signature (line 152):

static size_t fips_get_entropy(DRBG_CTX *dctx, unsigned char **pout,
int entropy, size_t min_len, size_t max_len)

(Notice that the second argument in the call is &entropy, whereas the
second parameter in the function prototype is pout.)

and contains the following lines (beginning at line 161):

...
rv = dctx->get_entropy(dctx, &tout, entropy + bl,
min_len + bl, max_len + bl);
*pout = tout + bl;
if (rv < (min_len + bl) || (rv % bl))
return 0;


Note that the final if-statement is a "return early if we didn't get
enough entropy" check, but even before we check such a thing, or whether
tout == NULL, we add the value of bl to it. So, if bl == 20 and
dctx->get_entropy returned 0 and set tout to NULL, the variable entropy
in FIPS_drbg_instantiate() will now have the pointer value 20.

In such a case (as seen in the if-statement above), fips_get_entropy()
will have returned 0. FIPS_drbg_instantiate() detects this situation and
jumps to some cleanup code, which includes the following (line 277):

if (entropy && dctx->cleanup_entropy)
fips_cleanup_entropy(dctx, entropy, entlen);

Now, there's a good check for entropy == NULL before attempting to clean
it up; the trouble is, it is IMPOSSIBLE for entropy to ever be NULL at
this stage. In our failure case, it has the pointer value 20 (because bl
was 20). What happens in fips_cleanup_entropy() is that it subtracts the
value of bl (resulting in the NULL pointer value once again, but too
late to fail the check), and then passes that to dctx->cleanup_entropy,
which, upon dereferencing the NULL pointer, pukes.

Note that the function drbg_reseed() in the same module has the same
logic as was just described for FIPS_drbg_instantiate, probably has the
same bug, and the patch given should fix it as well. Only those two
functions appear to call fips_get_entropy() or fips_cleanup_entropy().

-mjc

__
OpenSSL Project http://www.openssl.org
Development Mailing List   ope