[Esapi-user] ESAPI Random Number Generation Broken

Bruno Girin bruno at energydeck.com
Thu Apr 10 07:57:32 UTC 2014


On 10 April 2014 05:33, Jeff Williams <jeff.williams at aspectsecurity.com>wrote:

> Since most calls like this in Java are exclusive, we should just change
> the documentation.
>
>
> > On Apr 9, 2014, at 8:58 PM, "Kevin W. Wall" <kevin.w.wall at gmail.com>
> wrote:
> >
> >
> > Hmmm... my gut tells me that people who have actually used this
> > are more likely using it as  getRandomInteger(1, N) where N is
> > larger than 2; e.g., N = 1000, and that they have never noticed it.
> >
> > So I guess my own preference would be to fix it to match the existing
> > documentation, which is most people's expectations. (Hopefully,
> > there are not that many people using it who have begin to adjust
> > for its bugs. But that's something that we should note in the release
> notes.
>

I'll let you debate that one for now and concentrate on the other bits.


>
> I couldn't figure out how to (easily) map it to a uniform distribution
> w/out some
> complicated smoothing, so I figured 64 would work. Unfortunately, it
> breaks backward
> compatibility by adding to additional characters so if someone has a
> regex matcher
> against (say) CSRF tokens that looks something like this:
>
>        [A-Za-z0-9]{8}
>
> it would fail to match all possibilities because of the '_' and '.'
> characters that
> I added.
>
> However, unlike base64 encoding, this encoding gives you the exact #
> of characters
> you request. That coupled with the fact that the '/', '+', and '='
> that are used in
> base64 encoding are not safe for URLs, that might be an advantage.
>
>> So all this to say that I'm really not sure we can make getRandomString
>> un-biased in the general case.
>
> I agree. I suppose in theory we could do it, since it is should be
statistically
> possible (although some bias may remain; see the Javadoc for
> Random.nextInt() for instance), but practically speaking, I think it
> would way over complicate things. And if we had it wrong when it
> was relatively simple, my confidence in getting it correct when it
> was unduly complicated plummets close to zero. (Bruno: No intent
> to disparage your coding abilities it is what it is and we all know
> that complexity is an enemy of security.)

No worries about disparaging my coding abilities as it's a question of
maths :-) If you have a set of integer numbers that are evenly distributed
over a range [0:a] and you map them to integers in range [0:b], the result
will be evenly distributed if and only if b is a divisor of a. Depending on
how you do it, you will come up with a different bias: modulo will bias it
towards the bottom of the range while float division will result in evenly
distributed spikes.

Anyway, I completely agree that we should keep it simple. It might be worth
explicitly updating the Javadoc to warn that there will be bias if the
length of the target character set is not a power of 2.



>
>> #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
'.'?
>
> Dang. No, don't do that. Just change the DefaultUser to use a (new)
property
> from ESAPI.properties file. (It never really should have been hard coded
as '8'
> to start with.)  I think of CSRF tokens as being more associated with
> authenticators--maybe just because they are a secondary cryptographic
> token--but maybe something like
>
>    # Set this to 8 to make it backward compatible with the insecure
> setting in ESAPI 2.1.0 and earlier
>    Authenticator.CSRF.tokenlength=20
>
> You will have to also tweak SecurityConfigurator and
> DefaultSecurityConfigurator classes
> to add something to retrieve that property, and then call it from
> DefaultUser.resetCSRFToken.
>
> But I'd prefer that over forking DefaultUser. Doing that means that we
> have to have a
> way to define the user class of choice in ESAPI.properties (not
> present today). Seems
> like a lot of work for little return IMHO.

OK, no problem. I was thinking of using two properties:

Authenticate.CSRF.useOldAlgorithm=false
Authenticate.CSRF.tokenLength=20

So if the first one is true, is uses the old hard-coded values of 8
characters out of a set of 62 characters and if it's false, create a base
64 string out of random bytes to the specified length. Looking at the
Apache commons codec doc for the Base64 class last night, it actually has a
safe URL version that uses characters '-' and '_' instead of '+' and '/' so
I could use that to keep the code as short as possible.


>
> I really appreciate your helping out with this. Thanks so much!

No problem. I'm not sure when I'll have the time to do this but will try to
get it done over the weekend.

Bruno
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.owasp.org/pipermail/esapi-user/attachments/20140410/f0b8abc2/attachment.html>


More information about the Esapi-user mailing list