Thanks to Debian OpenSSL developers

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Thanks to Debian OpenSSL developers

Guido Hennecke
Hi list,

I want to say: Thank you!

Why? The Debian developers seems the only developers, who investigated
the openssl code and not just used it. They found a strange thing in
the OpenSSL Code, asked the OpenSSL people and after all, they removed
the potentielly dangerous code peace.

OK, this time, it was a mistake, but who but them have not just used he
code for theire packages?

In Germany we say: "Wer nichts macht, macht auch nichts verkehrt".

The bug is hard, in particular hard to fix the problems in big
enviroments. But the Debian developers have done the job, unfortunately
this time, it was a mistake.

Anyway, thanks to the Debian openssl developers, to think about things
and not just copy them!

Heads up!

Regards, Guido


--
To UNSUBSCRIBE, email to [hidden email]
with a subject of "unsubscribe". Trouble? Contact [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Thanks to Debian OpenSSL developers

Steffen Schulz-2
On 080515 at 22:20, Guido Hennecke wrote:
> I want to say: Thank you!

Me too, but mostly for how quick+open the situation was and is handled.

I especially like that sshd doesn't accept weak keys anymore. I think
similar efforts should be made to check for weak keys in other
locations and at least notify root, better yet move them to .broken

> Why? The Debian developers seems the only developers, who investigated
> the openssl code and not just used it. They found a strange thing in
> the OpenSSL Code, asked the OpenSSL people and after all, they removed
> the potentielly dangerous code peace.


For what its worth...I see 3.5 problems that accumulated into this mess:

- OpenSSL is complex and critical but the code is little documented.
  Code pieces like the ones in question should have warning-labels
  printed all over them and a distinguished place and interface.
- There are published algorithms for good PRNGs, no need to help
  yourself with adding unintialized memory and praying the OS does a
  good job already.
- I don't know how much of an effort was made, to get the fix into
  upstream, but it seems it wasn't enough. Not enough to get a
  sufficient level of peer review. Instead it was decided to manage yet
  another debian-specific patch.
- (In theory, for critical software, a testcase should have been
  written to verify the problem and its solution. Difficult to test
  randomness if you don't know what to search for, though. But maybe it
  would have been noticed this way that the function in question
  actually has a different interface.)


Anyways, thanks for the good incident response..


/Steffen


--
To UNSUBSCRIBE, email to [hidden email]
with a subject of "unsubscribe". Trouble? Contact [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Thanks to Debian OpenSSL developers

Yves-Alexis Perez-2
On jeu, 2008-05-15 at 23:38 +0200, Steffen Schulz wrote:
> or what its worth...I see 3.5 problems that accumulated into this
> mess:
>
> - OpenSSL is complex and critical but the code is little documented.
>   Code pieces like the ones in question should have warning-labels
>   printed all over them and a distinguished place and interface.

There was a #ifndef PURIFY just before the instruction commented by #if
0.

> - There are published algorithms for good PRNGs, no need to help
>   yourself with adding unintialized memory and praying the OS does a
>   good job already.

You talk about the first instruction which doesn't add entropy in every
case, but doesn't hurt (except the lintian warning).
The problem was with the second one, which added the entropy pool the
content of a now initialized buffer.

> - I don't know how much of an effort was made, to get the fix into
>   upstream, but it seems it wasn't enough. Not enough to get a
>   sufficient level of peer review. Instead it was decided to manage
> yet
>   another debian-specific patch.

There wasn't any fix, that's why.  The “#ifdef PURIFY” was there for a
reason.

This issue is complex, not much people understand it 100% correctly (I'm
not in those people), but please don't repeat wrong stuff seen here and
there.

Cheers,
--
Yves-Alexis

signature.asc (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Thanks to Debian OpenSSL developers

Cat-4
On Fri, May 16, 2008 at 07:47:31AM +0200, Yves-Alexis Perez wrote:

> On jeu, 2008-05-15 at 23:38 +0200, Steffen Schulz wrote:
> > or what its worth...I see 3.5 problems that accumulated into this
> > mess:
> >
> > - OpenSSL is complex and critical but the code is little documented.
> >   Code pieces like the ones in question should have warning-labels
> >   printed all over them and a distinguished place and interface.
>
> There was a #ifndef PURIFY just before the instruction commented by #if
> 0.

/* Uninitialised memory used intentionally to add entropy */

That, I believe, speaks volumes.

#ifndef PURIFY says very little.

Most, if not all languages provide the ability to enter in comments for
a reason.

--
  "Police noticed some rustling sounds from Linn's bottom area
  and on closer inspection a roll of cash was found protruding
  from Linn's anus, the full amount of cash taken in the robbery."
    - http://www.smh.com.au/news/world/robber-hides-loot-up-his-booty/2008/05/09/1210131248617.html


--
To UNSUBSCRIBE, email to [hidden email]
with a subject of "unsubscribe". Trouble? Contact [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Thanks to Debian OpenSSL developers

Izak Burger
In reply to this post by Guido Hennecke
On Thu, May 15, 2008 at 9:58 PM, Guido Hennecke
<[hidden email]> wrote:
>  In Germany we say: "Wer nichts macht, macht auch nichts verkehrt".

Which means: he who does nothing makes no mistakes. (For those who
don't understand German)


--
To UNSUBSCRIBE, email to [hidden email]
with a subject of "unsubscribe". Trouble? Contact [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Thanks to Debian OpenSSL developers

Kevin B. McCarty-3
In reply to this post by Cat-4
CaT wrote:

> On Fri, May 16, 2008 at 07:47:31AM +0200, Yves-Alexis Perez wrote:
>> On jeu, 2008-05-15 at 23:38 +0200, Steffen Schulz wrote:
>> > or what its worth...I see 3.5 problems that accumulated into this
>> > mess:
>> >
>> > - OpenSSL is complex and critical but the code is little documented.
>> >   Code pieces like the ones in question should have warning-labels
>> >   printed all over them and a distinguished place and interface.
>>
>> There was a #ifndef PURIFY just before the instruction commented by #if
>> 0.
>
> /* Uninitialised memory used intentionally to add entropy */
>
> That, I believe, speaks volumes.
>
> #ifndef PURIFY says very little.
>
> Most, if not all languages provide the ability to enter in comments for
> a reason.
Even better, the OTHER call to MD_Update(&m,buf,j) -- the
*really important* one occurring around line 247 that didn't have
#ifndef PURIFY -- really should have had some comment like:

/* Add content of passed-in buffer to PRNG to supply entropy */

best regards,

--
Kevin B. McCarty <[hidden email]>
WWW: http://www.starplot.org/
WWW: http://people.debian.org/~kmccarty/
GPG: public key ID 4F83C751


signature.asc (260 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Thanks to Debian OpenSSL developers

Steffen Schulz
In reply to this post by Yves-Alexis Perez-2
On 080516 at 08:00, Yves-Alexis Perez wrote:
> On jeu, 2008-05-15 at 23:38 +0200, Steffen Schulz wrote:
> > or what its worth...I see 3.5 problems that accumulated into this
> > mess:
> >
> > - OpenSSL is complex and critical but the code is little documented.
> >   Code pieces like the ones in question should have warning-labels
> >   printed all over them and a distinguished place and interface.
> There was a #ifndef PURIFY just before the instruction commented by #if
> 0.

Exactly.

The comments in openssl(or mozilla/NSS, for that matter) are helpful
only to those who already understand what the code does. But even then
the programmer still doesn't know what the author *intended*.

actual code <-> authors intention <-> correct behaviour(iso/rfc/best pr.)

These are three different things and I tend to comment sensitive code
at two levels to link these.

> > - There are published algorithms for good PRNGs, no need to help
> >   yourself with adding unintialized memory and praying the OS does a
> >   good job already.
> You talk about the first instruction which doesn't add entropy in every
> case, but doesn't hurt (except the lintian warning).
> The problem was with the second one, which added the entropy pool the
> content of a now initialized buffer.

As I said, several problems accumulated. A clear description in high
and low level of the implementation of an approved PRNG could have
prevented $tool to complain about code quality and $hacker to fix the
wrong stuff.

Making the code more accessible also makes it easier to build up
expertise at 3rd parties, so you don't have to rely on the 2 core
devs to only approve code that doesn't mess up any unspoken rules.

> > - I don't know how much of an effort was made, to get the fix into
> >   upstream, but it seems it wasn't enough. Not enough to get a
> >   sufficient level of peer review. Instead it was decided to manage
> >   yet another debian-specific patch.
> There wasn't any fix, that's why.  The “#ifdef PURIFY” was there for a
> reason.

I mean, trying to take the (bad) patch into upstream would have
produced more peer review and most likely the error would have been
discovered.  (Either that or debian wouldn't be the only affected
distro, win-win ;))

To me thats another aspect why security failed here. And its an aspect
that could be covered by policy in Debian so that it is less likely to
happen again. It's the only thing Debian can do by itself, it can't
influence openssl comment and development practise.

I'm not saying 'you should do this and that', I'm just trying to
discuss what measures could limit the rate such errors occur at.


/Steffen
--
Bildet Olsenbanden!


--
To UNSUBSCRIBE, email to [hidden email]
with a subject of "unsubscribe". Trouble? Contact [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Thanks to Debian OpenSSL developers

Bodo Moeller
In reply to this post by Steffen Schulz-2
On Thu, May 15, 2008 at 11:38:59PM +0200, Steffen Schulz <[hidden email]> wrote:

> - There are published algorithms for good PRNGs, no need to help
>   yourself with adding unintialized memory and praying the OS does a
>   good job already.

Nothing in OpenSSL has ever *relied* on uninitialized memory.  The
only job expected from the OS is providing something readable, such
as (initially) a bunch of 0x00 bytes.

OpenSSL merely *accepts* uninitialized memory, since it sometimes may
very well contain some information dating back from whatever happened
previously during the program execution that couldn't be predicted by
adversaries, in which case this memory would be a good source of
entropy.  In its internal entropy estimation tally, OpenSSL never
assumes that uninitialized memory contributes anything -- this is just
opportunistic randomness if you will.

This much, by the way, should be very clear to anyone who has read the
OpenSSL PRNG's source code comments ;-)  Anyone who'd look at the
calling code responsible for the Valgrind warning would have found
a comment regarding this peculiar behavior.  An attempt to understand
what is going on locally based on just a single line, however, clearly
is doomed.  But even looking just at the single function would have
shown that the modified version of ssleay_rand_add() doesn't ever
dereference or pass the "buf" pointer; this should strike as odd even
if you don't read any of the comments.

Of course, mistakes can always happen anyway, and to anyone.  The
motto "never fix a bug you don't understand" will only help you out if
you are aware that you don't understand the bug -- not if you think
you understand, but actually misunderstand.

Bodo


--
To UNSUBSCRIBE, email to [hidden email]
with a subject of "unsubscribe". Trouble? Contact [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Thanks to Debian OpenSSL developers

Goswin von Brederlow-2
Bodo Moeller <[hidden email]> writes:

> This much, by the way, should be very clear to anyone who has read the
> OpenSSL PRNG's source code comments ;-)  Anyone who'd look at the
> calling code responsible for the Valgrind warning would have found
> a comment regarding this peculiar behavior.  An attempt to understand
> what is going on locally based on just a single line, however, clearly
> is doomed.  But even looking just at the single function would have
> shown that the modified version of ssleay_rand_add() doesn't ever
> dereference or pass the "buf" pointer; this should strike as odd even
> if you don't read any of the comments.

And that is exactly why I can't understand how this patch was ever
made nor commented as good on the openssl ML.

It might (and everyone having done security audits knows it is) be
hard to understand the implications of commenting out that line and
grasp the catastrophic effect it has without hindsight. But just
looking at the problem the patch was ment to address and that one
function it is clear that the patch is wrong.

Before you could judge if commenting that line is harmfull or not you
would have to trace every single instance of the function being called
and decide if the buf passed as argument is uninitialized there. Only
when you checked all invocations and prooved to yourself that they all
pass an uninitialized buffer would commenting out that line be an
option. But if you identified all the callers then why not fix them?

You never solve an uninitialized variable problem in a function that
gets that variable as argument. You fix the caller of that function.

> Of course, mistakes can always happen anyway, and to anyone.  The
> motto "never fix a bug you don't understand" will only help you out if
> you are aware that you don't understand the bug -- not if you think
> you understand, but actually misunderstand.

I totaly get how you get off by one errors or + and - swapped or
similar. But this mis-fix was already wrong on how it was attempting
to solve the issue in questions. That's why I don't get how at least 2
people messed up like that.

That it also has such a catastrophic effect doesn't even play into
that. To see the effect you have to understand a lot about the code,
intention and algorithms involed. And like most security issues that
is rather complicated and mistake prone. So let me repeated it. I'm
not appaled that nobody saw the effect. I'm shocked that nobody saw
that the way the patch tries to fix uninitialized memory access is
totaly wrong.

> Bodo

MfG
        Goswin


--
To UNSUBSCRIBE, email to [hidden email]
with a subject of "unsubscribe". Trouble? Contact [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Thanks to Debian OpenSSL developers

s. keeling-2
In reply to this post by Izak Burger
Izak Burger <[hidden email]>:
>  On Thu, May 15, 2008 at 9:58 PM, Guido Hennecke
> <[hidden email]> wrote:
> >  In Germany we say: "Wer nichts macht, macht auch nichts verkehrt".
>
>  Which means: he who does nothing makes no mistakes. (For those who
>  don't understand German)

Danke.

   "Behold, the turtle.  He makes progress when he sticks his neck out."


--
Any technology distinguishable from magic is insufficiently advanced.
(*)    http://blinkynet.net/comp/uip5.html      Linux Counter #80292
- -    http://www.faqs.org/rfcs/rfc1855.html    Please, don't Cc: me.


--
To UNSUBSCRIBE, email to [hidden email]
with a subject of "unsubscribe". Trouble? Contact [hidden email]