search

demba90 / owasp-esapi-java

Automatically exported from code.google.com/p/owasp-esapi-java
Other
0 stars 0 forks source link

StringUtils.union broken which has minor impact on CSRF Protection and random file name generation #323

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago 
The ESAPI for Java implementation of SecureRandom does not provide a random 
sequence of numbers. Because ESAPI uses a singleton to hold one instance of 
SecureRandom, the resulting random numbers are a predictable sequence. This 
also impacts the CSRF protection mechanism which depends on good random number 
generation.

To fix this, just use a new instance of SecureRandom each time instead of the 
ESAPI random number or CSRF calls.

(Thanks for David Rook for reporting this in Feb 2012)

Original issue reported on code.google.com by j...@manico.net on 6 Apr 2014 at 10:39

GoogleCodeExporter commented 8 years ago 
[deleted comment]
GoogleCodeExporter commented 8 years ago 
The original bug report came in 2/23/2012.  Here are some notes from my email 
archives:

***

I think I see the problem. ESAPI is not (necessarily) seeding the SecureRandom 
class depending on what API's you are using.

nextBytes forces a new seed if one does not exist, but the other helper API's 
(getInt and the like) do not.

I think we need to force a seed at instantiation time to fix this.

Original comment by j...@manico.net on 7 Apr 2014 at 3:15

GoogleCodeExporter commented 8 years ago 
The original email and private report was addressed to Chris Schmidt, Jeff 
Williams, Kevin Wall and John Steven and was titled "Why ESAPI's secure random 
is predictable....".

Original comment by j...@manico.net on 7 Apr 2014 at 3:16

GoogleCodeExporter commented 8 years ago

Original comment by kevin.w.wall@gmail.com on 9 Apr 2014 at 3:42

GoogleCodeExporter commented 8 years ago 
Changed priority from Medium to Critical.

Original comment by kevin.w.wall@gmail.com on 9 Apr 2014 at 4:23

GoogleCodeExporter commented 8 years ago 
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 comment by planetlevel on 28 Jun 2014 at 2:18

GoogleCodeExporter commented 8 years ago 
Nice work Jeff. The bug was actually first introduced to ESAPI-Java in August 
2009 here: https://code.google.com/p/owasp-esapi-java/source/detail?r=586. The 
specific diff is here: 
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.ja
va

You right, the random skew is very minor. The bigger impact is to anyone using 
StringUtilities.union(char[]... list) for important purposes. I reduced this to 
Priority-Medium (at best) and renamed the finding to be more accurate.

Very solid find, Jeff.

Original comment by j...@manico.net on 5 Jul 2014 at 5:57