[Esapi-user] ESAPI Random Number Generation Broken

Jim Manico jim.manico at owasp.org
Sat Jun 28 06:24:23 UTC 2014


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


More information about the Esapi-user mailing list