[Esapi-user] ESAPI Random Number Generation Broken

Jim Manico jim.manico at owasp.org
Sat Jun 28 06:03:43 UTC 2014


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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.owasp.org/pipermail/esapi-user/attachments/20140628/d8cc299e/attachment-0001.html>


More information about the Esapi-user mailing list