[Esapi-user] ESAPI Random Number Generation Broken

Bruno Girin bruno at energydeck.com
Wed Apr 9 22:45:59 UTC 2014


I had a look at the code and I am slightly confused with regards to the
root problem so Jim if you could send me the original email from David
Rook, it would be very useful as it would give me a bit of context.

Now for the things I found out by trying to break down the problem in
individual pieces.

#1 Bug 217 - getRandomInteger(1, 2) always returns 1

This is because the [Secure]Random.nextInt(n) method returns a number from
0 (inclusive) to n (exclusive) and the DefaultRandomizer implementation
doesn't take that into account. So as suggested in the bug report, there
are two ways to do this: either fix the code or fix the documentation. I
can do either.

This is unrelated to the 2 points below as getRandomString doesn't use
getRandomInteger.

#2 getRandomString(int length, char[] characterSet) potentially skewed

At the moment, this method creates the string one character at a time by
selecting a character at random in the characterSet array. So in the case
of the current implementation of CSRF tokens, it calls
secureRandom.nextInt(62) 8 times.

Kevin, your suggested implementation does this instead:

    byte[] nonce = new byte[ n ];
    secureRandom.nextBytes(nonce);
    StringBuilder result = new StringBuilder(length);
    for( int i = 0; i < nonce.length; i++ ) {
        char c = characterSet[ Math.abs( (nonce[i] % characterSet.length) )
];
        result.append( c );
    }
    return result.toString();

So instead of calling secureRandom.nextInt(characterSet.length) to avoid a
potential bias, we call secureRandom.nextBytes(). So far so good. However,
this code introduces a bias in the loop through the modulo operator:

nonce[i] % characterSet.length

Assuming secureRandom.nextBytes() is not biased, the bytes in the nonce
will be evenly distributed between -128 and 127. Taking a modulo of those
bytes will therefore introduce bias if characterSet.length is not a power
of 2.

So all this to say that I'm really not sure we can make getRandomString
un-biased in the general case.

#3 CSRF tokens

Those have a number of problems as identified by Kevin: irrespective of
bias, an 8 character string with each character taken out of a set of 62
has limited entropy. The only way to fix that is in DefaultUser rather than
DefaultRandomizer so shall I create a new User implementation that returns
longer CSRF tokens using a variation on base 64 encoding using '_' and '.'?

Cheers,

Bruno



On 9 April 2014 18:15, Kevin W. Wall <kevin.w.wall at gmail.com> wrote:

> Base64 encoding is out because +, /, and = are all "reserved" characters
> wrt URIs. I picked '_' and '.', because they are safe "unreserved"
> characters that don't need to be URL encoding. That's important because
> some people add their CSRF tokens as extra path info in the URL so we don't
> want to use any characters that requires URL encoding. Unfortunately,
> adding anything could break backward compatibility,  so I think Jim's
> suggestion of forking DefaultRandomizer into something like
> SecureDefaultRandomizer (or whatever) is the best choice.
>
> If you want to work on this, that would be great. Just attach your code to
> the Google Issue for this (# 323).
>
> Thanks,
> -kevin
> Sent from my Droid; please excuse typos.
> On Apr 9, 2014 8:24 AM, "Bruno Girin" <bruno at energydeck.com> wrote:
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.owasp.org/pipermail/esapi-user/attachments/20140409/77181d77/attachment.html>


More information about the Esapi-user mailing list