[Esapi-user] [Esapi-dev] ESAPI Random Number Generation Broken

Jim Manico jim.manico at owasp.org
Mon Jun 30 01:47:46 UTC 2014


I am not in a board voting block when I have this discussion with you, this
is not a board level discussion, so please do not bring in or blame the
board for this discussion. I am also a volunteer, just like you.

I am upset that the ESAPI team "blew off" this "poor CSRF token entropy"
finding in 2011 when I was asked not to report Rook's finding even when his
results were repeatable. It led to me leaving the project at the time. This
is not just you, it was a group decision that I disagreed with. I did wait
two years before disclosure and I'm glad to see it fixed.

But you are right, getRandomString is •not• flawed - it's the calls to it
in a few places, and I spoke in error earlier.

Jeff, other reasons I've been "against" ESAPI as a production library is
because (1) it was pushed out very early when we all knew it was not ready
for prime time, I believe I used the words "Machiavellian release" at the
time and I stand by that. Even Kevin Wall wanted to stop the production
release. (2) It's no longer being maintained, this is a big problem.

As a security library I feel we need to be held to a much higher standard.
So yes your volunteerism is stellar and you are have absolutely no
responsibility to keep working on it. But let's call it what it is, and
excellent beta security library not at all production class. This has been
echoed by other dev's such as Dinis Cruz and John Steven. Major security
companies have done deep review of ESAPI because they •wanted• to use it,
but the problems were way to deep like the use of singletons for
everything, very poor password storage, and other issues.

OWASP volunteers are in the process of building a significant project
review platform to make it easier to quantify project quality. Nothing is
stopping you from working on ESAPI all you want and I'll dance a jig and
celebrate your work every-time you make a checkin, but I want to be in
integrity with the Java community by calling ESAPI what it is - beta at
best. When we pushed out ESAPI as production class I feel we lied to the
Java community and I don't want to keep that charade up.

--
Jim Manico
OWASP Volunteer
@Manicode
(808) 652-3805

On Jun 30, 2014, at 8:58 AM, Jeff Williams <jeff.williams at aspectsecurity.com>
wrote:

 Jim, all I've ever done is try to investigate whether there is a real
problem here or not. I don't believe security should be practiced by
histrionics.  Understanding the problem is step one.  Since this was
reported you've argued vehemently that one thing or another was to blame
even when they couldn't possibly explain the facts.  Now I actually get
around to digging in, figuring out what is going on, fixing it, and I'm the
bad guy?  This isn't how OWASP was and it's not how OWASP should be.  The
board should focus on making a great platform that supports contributors
like me.

--Jeff


On Jun 29, 2014, at 6:16 PM, "Jim Manico" <jim.manico at owasp.org> wrote:

  Jeff,

 The reported 2011 histogram from burp says otherwise. Even though the root
cause was the union function, it had a major impact on random number
generation. But I agree the CVE should be very accurate and this is
something •we• should work on.

 I also feel that this •is• a big deal. FACT: ESAPI is a security library
and the random number generation was made significantly •weaker• than pure
SecureRandom by itself. If a developer just used the raw SecureRandom they
would had been better off than using our •security• library since 2009!

 Squashing this bug and telling me not to report it in 2011 was an
incredibly wrong move in my opinion and these "compromises" lead to false
promises at best and deep insecurity at worst.

--
Jim Manico
@Manicode
(808) 652-3805

On Jun 30, 2014, at 6:08 AM, Jeff Williams <jeff.williams at owasp.org> wrote:

  If a CVE gets created, it should be very accurate. This is not and has
never been a problem with the ESAPI Randomizer.  A character set was
created wrong, with one instance of each digit and 4 of each upper case and
lower case letter.  This slightly skews the distribution, making digits
less likely to be randomly chosen.  Whoever writes it up should do the math
to see exactly how much weaker that makes a token.  Most tokens have more
than enough length for them to remain unguessable even with this bias.

--Jeff


On Jun 28, 2014, at 2:24 AM, Jim Manico <jim.manico at owasp.org> wrote:

  The real culprit is r586 so I think this bug was introduced for the
initial 2.0 release.


https://code.google.com/p/owasp-esapi-java/source/diff?spec=svn586&r=586&format=side&path=/branches/2.0_quality/src/main/java/org/owasp/esapi/StringUtilities.java&old_path=/branches/2.0_quality/src/main/java/org/owasp/esapi/StringUtilities.java&old=569

What else did this bug impact?

--
Jim Manico
@Manicode
(808) 652-3805

On Jun 28, 2014, at 7:11 AM, "Kevin W. Wall" <kevin.w.wall at gmail.com> wrote:

  Jim,

 For the sake of the CVE, all you need to tell them of what versions
 of ESAPI are vulnerable and what version it is fixed in, not the SVN rev#.
 And we probably didn't revert to the original version because the
interface changed from union(char[], char[][) to union(char[]...).

 And if we do this, we will want a release that fixes it and if we
do that, we will have to find a new place to host the ESAPI jar / zip
file because Google Code no longer allows this and simply putting it
in Maven Central is not sufficient because there are still a lot of
 'ant' users out there.  So any suggestions where we should host the
download for ESAPI? Google suggests Google Drive but I'm open to other
options.

 -kevin


On Sat, Jun 28, 2014 at 2:03 AM, Jim Manico <jim.manico at owasp.org> wrote:

>  Jeff,
>
>  I see three versions of this code in the source over history. For the
> sake of the CVE report...
>
>  This was introduced at r387.
>
>       /**
>        * Union two character arrays.
>        *
>        * @param c1 the c1
>        * @param c2 the c2
>        * @return the char[]
>        */
>       public static char[] union(char[] c1, char[] c2) {
>           StringBuffer sb = new StringBuffer();
>           for (int i = 0; i < c1.length; i++) {
>               if (!contains(sb, c1[i]))
>                   sb.append(c1[i]);
>           }
>           for (int i = 0; i < c2.length; i++) {
>               if (!contains(sb, c2[i]))
>                   sb.append(c2[i]);
>           }
>           char[] c3 = new char[sb.length()];
>           sb.getChars(0, sb.length(), c3, 0);
>           Arrays.sort(c3);
>           return c3;
>       }
>
>    This was introduced at r722 during a merge:
>
>        /**
>        * Union multiple character arrays.
>        *
>        * @param list the char[]s to union
>        * @return the union of the char[]s
>        */
>       public static char[] union(char[]... list) {
>           StringBuilder sb = new StringBuilder();
>
>           for (char[] characters : list) {
>                   for (int i = 0; i < list.length; i++) {
>                       if (!contains(sb, characters[i]))
>                           sb.append(list[i]);
>                   }
>           }
>
>           char[] toReturn = new char[sb.length()];
>           sb.getChars(0, sb.length(), toReturn, 0);
>           Arrays.sort(toReturn);
>           return toReturn;
>       }
>
>
>  And this is your fix at r1943.
>
>
>       /**
>        * Union multiple character arrays.
>        *
>        * @param list the char[]s to union
>        * @return the union of the char[]s
>        */
>       public static char[] union(char[]... list) {
>           StringBuilder sb = new StringBuilder();
>
>                   for (char[] characters : list) {
>                           for ( char c : characters ) {
>                                   if ( !contains( sb, c ) )
>                                           sb.append( c );
>                           }
>                   }
>
>           char[] toReturn = new char[sb.length()];
>           sb.getChars(0, sb.length(), toReturn, 0);
>           Arrays.sort(toReturn);
>           return toReturn;
>       }
>
>    I'm wondering why we did not revert back to the original code. Can a
> few of you eyeball this sequence?
>
>  Aloha,
>
> --
> Jim Manico
> @Manicode
> (808) 652-3805
>
> On Jun 28, 2014, at 3:17 AM, Jeff Williams <
> jeff.williams at aspectsecurity.com> wrote:
>
>    Everyone,
>
> I found the problem.  The real problem.  There was a change introduced in
> StringUtilities r722 that broke the union() method.  This method was used
> to generate the EncoderConstants.CHAR_ALPHANUMERICS set used in the test
> case.
>
> I've checked in a fix and test cases to verify that it works.  I also
> added a very simple test case for getRandomString() that verifies that the
> method generates roughly the same number of each character across a bunch
> of generated strings.  Not perfect but at least sensitive enough to
> recognize if something is way off.
>
> The good news is that order has been restored to the universe, and our
> Burp test suite results are back to 'excellent'.  If you'd like to verify
> this yourself (and I strongly encourage you to do so) I included a small
> utility to generate random tokens as a main() method in RandomizerTest.
>
>    /**
>     * Run this class to generate a file named "tokens.txt" with 20,000
> random 20 character ALPHANUMERIC tokens.
>     * Use Burp Pro sequencer to load this file and run a series of
> randomness tests.
>     *
>     * NOTE: be careful not to include any CRLF characters (10 or 13 ASCII)
> because they'll create new tokens
>     * Check to be sure your analysis tool loads exactly 20,000 tokens of
> 20 characters each.
>     */
>    public static void main(String[] args) throws IOException {
>        FileWriter fw = new FileWriter("tokens.txt");
>        for (int i = 0; i < 20000; i++) {
>            String token = ESAPI.randomizer().getRandomString(20,
> EncoderConstants.CHAR_ALPHANUMERICS);
>            fw.write(token + "\n");
>        }
>        fw.close();
>    }
>
> Thanks to everyone who put some thought into the issue.
>
> --Jeff
>
>
> -----Original Message-----
> From: Jim Manico [mailto:jim.manico at owasp.org <jim.manico at owasp.org>]
> Sent: Thursday, June 26, 2014 1:30 AM
> To: Kevin W. Wall
> Cc: Jeff Williams; Bruno Girin; esapi-dev at lists.owasp.org;
> esapi-user at lists.owasp.org
> Subject: Re: [Esapi-user] ESAPI Random Number Generation Broken
>
> I'll track these issues on google code as soon as I get to a real
> computer. Everything you say makes perfect sense to me and I appreciate
> your time.
>
> I'm still going to chase down DJB's SecureRandom hack for our analysis.
> (Dr) Steven Murdoch is here in Cambridge and made that suggestion.
>
> Cheers, Kevin.
> --
> Jim Manico
> @Manicode
> (808) 652-3805
>
> On Jun 26, 2014, at 5:39 AM, "Kevin W. Wall" <kevin.w.wall at gmail.com>
> wrote:
>
>
>  On Wed, Jun 25, 2014 at 9:57 AM, Jim Manico <jim.manico at owasp.org> wrote:
>
>  PS: Java 8 improves upon this and provides a new API:
>
>  http://docs.oracle.com/javase/8/docs/api/java/security/SecureRandom.h
>
>  tml#getInstanceStrong-- which is what we should be using for "
>
>  high-value/long-lived secrets like RSA public/private keys". But even
>
>  then, using the same instance without reseeding with lead to a PRNG,
>
>  not a CRNG sequence.
>
>
>  That's all well and good, but even with JDK 7 and earlier,
>
> SecureRandom
>
> *is* a CSRNG. (Note that a CSRNG *is* a PRNG.)  The problems with
>
> SecureRandom very early own is that they just punted on the initial
>
> pseudo-random seed. Before JDK 1.4 (I think that's where it was
>
> changed), the first time they set the seed, they did some black magic
>
> like mixing together a few bits from the current time in milliseconds,
>
> the current amount of total and available memory in the Java heap, and
>
> added in how many times a thread could yield in 3 seconds.  Near boot
>
> time, all those things were very predictable. In 1.4 (maybe 1.3), it
>
> was changed to initialize the seed /dev/urandom if it was available
>
> and if not, I think it reverted to some other song and dance.  There
>
> is problems however with /dev/urandom shortly after boot time (and
>
> ESPECIALLY shortly after the *initial* system boot). Using /dev/random
>
> would be better, but unfortunately that will block. (Aside: At my
>
> previous job, we wrote an EntropyPool to seed things like SecureRandom
>
> that would read from /dev/random by default or alternately
>
> /dev/urandom. There was a warning in the Javadoc if would block and to
>
> use the weaker entropy setting if that was a concern. It was never a
>
> problem until one time when an application had a new release and they
>
> started requesting [for some unknown reason] about 10k worth of data
>
> from this EntropyPool all at once. And it was in an /etc/init.d script
>
> that started their application in WebLogic Server. Result was they had
>
> a 20+ minute startup delay until 10k bytes could be collected from
>
> /dev/random. They called me in the middle of the night to 'yell' at
>
> me. I nicely told them to RTFM. Sigh.)
>
>
>  Anyway, my guess is that getInstanceStrong() method will allow you to
>
> specify things like "use /dev/random".  However, I agree with Thomas
>
> Ptacek's comment that if it really matters use an Operating
>
> System-level CSRNG and not a userspace CSRNG. There are of course
>
> reasons why you might not want to. Always using /dev/urandom is a
>
> reasonable compromise, but there are still edge cases where you can
>
> get burned by using /dev/urandom rather than /dev/random. But unless
>
> you are protecting nuclear launch codes (and seriously, you BETTER not
>
> be doing that with Java since you can't guarentee that you clear
>
> memory), /dev/urandom will probably suffice.
>
>
>  Also, one thing that I picked up on the Cigital blog post
>
> (http://www.cigital.com/justice-league-blog/2009/08/14/proper-use-of-j
>
> avas-securerandom/) was that an attacker could potentially hide a call
>
> like this
>
>
>    System.setProperty("securerandom.source", "/dev/zero");
>
>
>  in some 3rd party library that you are using and then your are toast.
>
> Of course, the same is true if you don't specify that you want the Sun
>
> provider as in:
>
>
>    SecureRandom csrng = SecureRandom.getInstance("SHA1PRNG", "SUN");
>
>
>  as someone could dynamically insert their own tainted provider for
>
> SecureRandom into a 3rd party library you are using and again you are
>
> screwed. (Unless of course you are using a Java SecurityManager and an
>
> appropriately locked-down security policy which I am sure that you
>
> *all* are doing, right. Cough, cough.)
>
>
>  Anyhow, we need to do something about those two things in ESAPI...
>
> especially the first since it would be really subtle. Anyone care to
>
> write up a Google issue to that effect to remind me?
>
>
>  Cheers,
>
> -kevin
>
> --
>
> Blog: http://off-the-wall-security.blogspot.com/
>
> NSA: All your crypto bit are belong to us.
>
>


-- 
Blog: http://off-the-wall-security.blogspot.com/
NSA: All your crypto bit are belong to us.

  _______________________________________________
Esapi-dev mailing list
Esapi-dev at lists.owasp.org
https://lists.owasp.org/mailman/listinfo/esapi-dev

   _______________________________________________
Esapi-dev mailing list
Esapi-dev at lists.owasp.org
https://lists.owasp.org/mailman/listinfo/esapi-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.owasp.org/pipermail/esapi-user/attachments/20140630/73a162c5/attachment.html>


More information about the Esapi-user mailing list