StringUtils.union broken which has minor impact on CSRF Protection and random file name generation
Description
Environment
Activity
Max Gelman November 13, 2014 at 6:27 PM
From kevin.w.wall@gmail.com on April 08, 2014 21:23:57
Changed priority from Medium to Critical.
Labels: -Priority-Medium Priority-Critical
Max Gelman November 13, 2014 at 6:27 PM
From planetlevel on June 27, 2014 19:18:01
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
Status: Fixed
Max Gelman November 13, 2014 at 6:27 PM
From j...@manico.net on July 04, 2014 22:57:40
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.java 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.
Summary: StringUtils.union broken which has minor impact on CSRF Protection and random file name generation <span class="oldvalue"> (was: ESAPI Random Number Generation and CSRF Protection Broken) </span>
Labels: -Priority-Critical Priority-Medium
Max Gelman November 13, 2014 at 6:27 PM
From j...@manico.net on April 06, 2014 20:16:47
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....".
Max Gelman November 13, 2014 at 6:27 PM
From kevin.w.wall@gmail.com on April 08, 2014 20:42:03
Labels: Security OpSys-All Component-Randomizer
From j...@manico.net on April 06, 2014 18:39:56
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: http://code.google.com/p/owasp-esapi-java/issues/detail?id=323