How Boring Flaws Become Interesting

One of the great challenges for consumers of static analysis products, particularly desktop tools, is dealing with the large flaw counts. You have to wade through the findings to decide what to fix and when, which can be a daunting task. At Veracode, we continuously update our analysis engine to aggressively reduce false positives, thereby enabling our customers to more efficiently triage their results. Even so, it's not unusual for customers to ask for clarification on certain flaws as they prioritize fixes. The other day, we ran into an example that ended up being much more interesting than it appeared. The flaw category was Insecure Temporary Files, and the question was "should I really care about this?" The flaw we identified was in a Java application, and the offending line was something like this:

tmpFile = java.io.File.createTempFile(deploymentName, ".war");

I know what you're thinking. You think the rest of this post is about how createTempFile() uses java.util.Random instead of java.security.SecureRandom to generate filenames, and since Random is seeded with the system time, you can work backwards to figure out the seed and use it to predict all future temporary files. That's not it, so keep reading! We couldn't remember specifically what was so bad about createTempFile(), aside from using a non-cryptographic PRNG, so we checked the Java API for clues:

Creates a new empty file in the specified directory, using the given prefix and suffix strings to generate its name. ... To create the new file, the prefix and the suffix may first be adjusted to fit the limitations of the underlying platform. If the prefix is too long then it will be truncated, but its first three characters will always be preserved. If the suffix is too long then it too will be truncated, but if it begins with a period character ('.') then the period and the first three characters following it will always be preserved. Once these adjustments have been made the name of the new file will be generated by concatenating the prefix, five or more internally-generated characters, and the suffix.

This behavior was verified with a quick test program:

$ for i in `seq 1 10`; do java createTempFile; done
/tmp/prefix53363suffix
/tmp/prefix200suffix
/tmp/prefix53898suffix
/tmp/prefix26801suffix
/tmp/prefix13687suffix
/tmp/prefix2221suffix
/tmp/prefix28661suffix
/tmp/prefix61720suffix
/tmp/prefix23104suffix
/tmp/prefix29833suffix

OK, that looks about right. It does what it says it does. One of my colleagues quickly raised the question, what happens if the generated filename already exists? So he generated /tmp/prefix0suffix through /tmp/prefix65535suffix and ran the test program again.

$ for i in `seq 1 10`; do java createTempFile; done
/tmp/prefix65536suffix
/tmp/prefix65537suffix
/tmp/prefix65538suffix
/tmp/prefix65539suffix
/tmp/prefix65540suffix
/tmp/prefix65541suffix
/tmp/prefix65542suffix
/tmp/prefix65543suffix
/tmp/prefix65544suffix
/tmp/prefix65545suffix

Uh-oh, not good. So not only does createTempFile() use a pretty small search space, but when it exhausts that space, it degrades to being 100% predictable? Decompiling the relevant portion of JRE 1.6.0_07, we can see exactly how the filenames are constructed:

private static File generateFile(String s, String s1, File file)
    throws IOException
{
    if(counter == -1)
        counter = (new Random()).nextInt() & 0xffff;
    counter++;
    return new File(file, (new StringBuilder()).append(s).append(Integer.toString(counter)).append(s1).toString());
}

public static File createTempFile(String s, String s1, File file)
    throws IOException
{
    ...
    File file1;
    do
        file1 = generateFile(s, s2, file);
    while(!checkAndCreate(file1.getPath(), securitymanager));
    return file1;
}

What this tells us is that createTempFile() is actually worse than we thought. Notice that counter is only ever assigned a random value once. As soon as it has that first random value, it simply increments from that point forward. The reason we didn't get sequential output on our first test run was because we ran the test program 10 times, initializing counter each time. Had we put the loop inside the program, it would have generated a sequential list (try it yourself if you don't believe me). As luck would have it, Sun actually just fixed this problem in their latest release, Java 6 Update 11. Amazing that it went so long without being discovered. The updated function looks like this:

private static File generateFile(String s, String s1, File file)
    throws IOException
{
    long l = LazyInitialization.random.nextLong();
    if(l == 0x8000000000000000L)
        l = 0L;
    else
        l = Math.abs(l);
    return new File(file, (new StringBuilder()).append(s).append(Long.toString(l)).append(s1).toString());
}

If you're wondering, the same bug is present in IBM Java 6 SR2, but it's been fixed in SR3. Returning to the original question that led us down this rathole, we came to the conclusion that yes, these types of flaws ARE worth fixing. Predictability and security rarely go hand in hand.