NetBSD Crypto Bug: sizeof v. sizeof()

[wil...@wilbur.25thandclement.com]

This bad code[1] compromised[2] NetBSD's CPRNG implementation:

rnd_extract_data(key + r, sizeof(key - r),
RND_EXTRACT_ANY);

One might argue this bug stemmed from using the sizeof() idiom instead of
stylistically treating sizeof as a regular operator.


1: http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/kern/subr_cprng.c.diff?r1=1.14&r2=1.15&only_with_tag=MAIN&f=h
2: http://www.theregister.co.uk/2013/03/26/netbsd_crypto_bug/

[Eric Sosman]

On 5/23/2013 9:20 PM, wil...@wilbur.25thandClement.com wrote:
> This bad code[1] compromised[2] NetBSD's CPRNG implementation:
>
> rnd_extract_data(key + r, sizeof(key - r),
> RND_EXTRACT_ANY);
>
> One might argue this bug stemmed from using the sizeof() idiom instead of
> stylistically treating sizeof as a regular operator.
>
>
> 1: http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/kern/subr_cprng.c.diff?r1=1.14&r2=1.15&only_with_tag=MAIN&f=h

It doesn't look like a problem with sizeof per se, but more
like a problem with incorrect grouping. At a guess (looking only
at the diffs, not motivated to go hunt up the complete source and
acquaint myself with the conventions, expectations, context, culture,
and street argot thereof), it looks like they wanted `sizeof(key) - r'
instead of `sizeof(key - r)' -- but that's far from certain, as the
diffs are considerably more voluminous than "a quick sizeof fix"
would warrant. Looks like either (1) other fixes were mixed in,
or (2) "the sizeof problem" was only a small part of the picture.

Anyhow, this isn't the first example of faulty code ever to
be seen under the, er, Sun. Flawless software is fairly rare ...

> 2: http://www.theregister.co.uk/2013/03/26/netbsd_crypto_bug/

Not informative; don't know why you included it.

--
Eric Sosman
eso...@comcast-dot-net.invalid

[Ian Pilcher]

On 05/23/2013 08:20 PM, wil...@wilbur.25thandClement.com wrote:
> This bad code[1] compromised[2] NetBSD's CPRNG implementation:
>
> rnd_extract_data(key + r, sizeof(key - r),
> RND_EXTRACT_ANY);
>
> One might argue this bug stemmed from using the sizeof() idiom instead of
> stylistically treating sizeof as a regular operator.

That seems like a construct for a which a compiler might want to issue a
warning.

--
========================================================================
Ian Pilcher arequ...@gmail.com
Sometimes there's nothing left to do but crash and burn...or die trying.
========================================================================

[christian.bau]

On May 24, 3:00 pm, Ian Pilcher <arequip...@gmail.com> wrote:

> That seems like a construct for a which a compiler might want to issue a
> warning.

I think I have seen warnings were sizeof (some pointer) was used when
you would have expected sizeof (some array).

I know one compiler where

int* p = calloc (x, y);

wants one of the arguments to be sizeof (int). Doesn't actually care
which one.

[Jorgen Grahn]

On Fri, 2013-05-24, christian.bau wrote:
> On May 24, 3:00 pm, Ian Pilcher <arequip...@gmail.com> wrote:
>
>> That seems like a construct for a which a compiler might want to issue a
>> warning.
>

> I think I have seen warnings [where] sizeof (some pointer) was used when

> you would have expected sizeof (some array).

I have certainly seen the bug it tries to protect against ...

void baz(char foo[42])
{ {
char foo[42];
bar(foo, sizeof foo); bar(foo, sizeof foo); /* bug */
} }

It's rather easy to trigger while modifying existing code, and it's
surprising to people who have never seen it before.

And yet I use sizeof array (and sizeof array/sizeof array[0]) a lot.
The alternatives are often worse IMO.

/Jorgen

--
// Jorgen Grahn <grahn@ Oo o. . .
\X/ snipabacken.se> O o .