[Esapi-user] ESAPI Random Number Generation Broken

Dan Anderson dan-anderson at cox.net
Tue Apr 8 08:16:21 UTC 2014

>1. IF it is clearly advertised in BIG letters the changelog (maybe for a
major version only ?),

If the default is not backwards compatible (and I agree that it should not
be) - I think we're, by convention (http://semver.org/), supposed to call
it a major?  (Not that we necessarily follow this convention - but I think
it makes sense.)

>I'm not a security expert, but being for a "simple" CSRF token (and not
the key of any crypto algo), is it really that bad ?

And yeah, with a very clear notice that describes the potential impact of
each use case as well as how to enable the backwards compatibility for
each.  It would be nice to be controllable by use case - as Jim mentions:
CSRF tokens, key creation, random access map.  CSRF tokens don't seem like
they pack the worst possible impact from this(?) to me either (given the
other controls that should be in place around CSRF tokens (TLS, not in URL,
etc)) and I suspect that CSRF tokens are the biggest deployed use case.


On Tue, Apr 8, 2014 at 2:29 AM, Olivier Jaquemet <
olivier.jaquemet at jalios.com> wrote:

>  Hi Kevin, Jim,
> My opinion regarding Kevin question on breaking backward compatibility :
> 1. IF it is clearly advertised in BIG letters the changelog (maybe for a
> major version only ?),
> 2. AND there is still a way to generate random string of some specific
> chars (eg alpha numerics)
> I don't think it would be that bad to breaking backward compatibility for
> improved randomness and security
> Olivier
> PS : probably inspired by ESAPI code, I have been using the ESAPI
> randomizer in the same way to generated a random CSRF token in our webapp :
>    ESAPI.randomizer().getRandomString(16,
> EncoderConstants.CHAR_ALPHANUMERICS);
> I'm not a security expert, but being for a "simple" CSRF token (and not
> the key of any crypto algo), is it really that bad ?
> On 08/04/2014 07:53, Jim Manico wrote:
> PS: As I mentioned earlier in this thread, the DefaultUser class uses the
> same API for CSRF token generation: getRandomString (and is only set to an
> 8 character "random" string for some reason). This is most certainly
> broken. Small sized token and poor randomness. Ouch.
>            public String resetCSRFToken() {
>                   csrfToken = ESAPI.randomizer().getRandomString(8,
> EncoderConstants.CHAR_ALPHANUMERICS);
>                   return csrfToken;
>           }
> --
> Jim Manico
> @Manicode
> (808) 652-3805
> On Apr 7, 2014, at 10:09 PM, "Kevin W. Wall" <kevin.w.wall at gmail.com>
> wrote:
>  Okay, I ran some more tests tonight. I thought, I'm going to make this
> worse that what we have it now...I decided to use java.util.Random
> rather than java.security.SecureRandom. I wanted to see how much
> worse it was.
> I ran it using the ESAPI encoding method that DefaultRandomizer
> uses. I got an effective 2 bits of entropy at the 1% significance level.
> That was for 32-character length strings consisting solely of A-Z, a-z,
> and 0-9 and it used Random.nextInt(int) similar to how DefaultRandomizer
> uses SecureRandom.nextInt(int).
> Next I changed it to use Random.nextByte(32) and base64 encoded that.
> I ran through the Burp test again, only telling it to first base64-decode
> the "tokens". This time it said there were 218-bits of entropy (out of
> a possible 256 bits) at a 1% significance level. Much better, right?
> Then I used the exact same strings, but told it NOT to base64-decode
> things, but because I wanted to compare 32 character strings and
> the base64-encode strings were 44 characters, I truncated all 20K
> lines after the first 32 characters. Running through Burp this way
> gave 174-bits at the 1% significance. Lower than before, but better
> than the ESAPI encoding.
> So, either we are dealing with a rather biased Random.nextInt(int)
> method (the Javadoc for this methods alludes to some bias, but it
> is not clear how much), or ...my guess we are doing something stupid
> with the way we are encoding it. (Well, I know we are doing something
> stupid because we are making way more calls then we need to.)
> The call in DefaultRandomizer is normally to SecureRandom.nextInt(32)
> or 64 [in David Rook's tests; I've been using 32 just to make Burp run
> faster], but the way we are creating that string is one character position
> at a time, so for a 32 char "random" string, we are calling
> SecureRandom.nextInt(32) 32 times, then using the resulting 'int'
> to index into a character set array that consists of all upper case alpha,
> lower case alpha, and digits. From an efficiency standpoint, this of
> course is rather insane.
> So here is a question for all of you... How important to you is it
> to maintain an exact backward compatibility of format in terms of
> characters used?
> I can "fix" this in a secure manner by changing it to call
>     byte[] b = new byte[32];
>     SecureRandom.nextBytes(b)
> and then just base64 encode 'b'. That gets us pretty close to
> what we want. (For a 32-byte array [44 chars after base64
> encoding], Burp shows 231 bits of entropy [of a possible 256
> bits].) And while that may not be "fixed" (myself, I still consider
> it broke as would every cryptographer), it's still within the levels
> of acceptable security for most things. [Note: If I create a new
> SecureRandom instance each time it is called, the entropy
> increases from 231 bits to 239 bits, so reseeding seems to
> help...A LITTLE, but it is not the root cause of this problem.]
> Now, I suppose they may be things that break if the format of
> these "random" strings change from what we are using today
> in DefaultRandomizer to using base64 encoded strings (e.g.,
> someone might be doing regex matching), so I don't toss this
> idea out their casually. But IMO, I would have never invented my
> own encoding method to start with. Use hex encoding, or any
> radix R encoding or URL encoding, or whatever. But it seems
> that the current ESAPI encoding it a contributor to why this
> is an epic fail in terms of the Burp statistical analysis. While it
> looks good in theory, it apparently fails in practice. I wish I had
> a better explanation of why, but I don't.
> But to me, better to salvage something rather than having a
> critical component remain broken.  But, OTOH, I wouldn't
> be breaking my code so I'm a little biased. (I intentionally
> didn't use the ESAPI.randomizer() stuff in the ESAPI crypto
> because I didn't want to tightly couple the implementations
> or even interfaces needlessly. (If that decision had been made
> in other ESAPI components, we would have an easier job
> at getting rid of some of these dependencies, but that's another
> story for another time.)
> Thoughts?
> -kevin
> P.S.- If any of you have a firm grounding in statistics, perhaps you
>         could help us analyze this. I had some theories, but eventually
>         shot them down by further experimentation.
> --
> Blog: http://off-the-wall-security.blogspot.com/
> NSA: All your crypto bit are belong to us.
> _______________________________________________
> Esapi-user mailing listEsapi-user at lists.owasp.orghttps://lists.owasp.org/mailman/listinfo/esapi-user
> --
> Olivier Jaquemet <olivier.jaquemet at jalios.com> <olivier.jaquemet at jalios.com>
> Ingénieur R&D Jalios S.A. - http://www.jalios.com/
> @OlivierJaquemet +33671116490
> _______________________________________________
> Esapi-user mailing list
> Esapi-user at lists.owasp.org
> https://lists.owasp.org/mailman/listinfo/esapi-user
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.owasp.org/pipermail/esapi-user/attachments/20140408/aa7ccce0/attachment.html>

More information about the Esapi-user mailing list