(doc): Document public RandomStringUtils exploit

[JLLeitschuh]

Document the public exploit that exists for RandomStringUtils.

This documentation update stems from my finding this vulnerability in the very popular code generator JHipster.

https://nvd.nist.gov/vuln/detail/CVE-2019-16303

[coveralls]

Coverage remained the same at 95.203% when pulling 5698827c25273748304b2b2e57ad7165e30f6d12 on JLLeitschuh:patch-1 into 29723e851ca56aa621165130049e588980c5a5e7 on apache:master.

[chtompki]

Did anyone note that in 3.9 we have the following at the top of the JavaDoc for RandomStringUtils

Caveat: Instances of Random, upon which the implementation of this class relies, are not cryptographically secure.

Please note that the Apache Commons project provides a component dedicated to pseudo-random number generation, namely Commons RNG, that may be a better choice for applications with more stringent requirements (performance and/or correctness).

which, both provides the point we're trying to make as well as a place to look for a solution to the insecurity of the randomness.

[JLLeitschuh]

Please note that the Apache Commons project provides a component dedicated to pseudo-random number generation, namely Commons RNG, that may be a better choice for applications with more stringent requirements (performance and/or correctness).

When I read this, I don't think this refers to 'security' at all. 'Correctness' & 'Performance' do not equate to providing security benefit when I read it.

If I had more time, I'd actually propose formally deprecating every single method on this class in favor of an API like this:

// Uses new Random()
RandomStringUtils.insecureAndPredictable().random(32);

// Uses new SecureRandom()
RandomStringUtils.secure().random(32);

RandomStringUtils.random(SecureRandom.getInstanceStrong()).random(32);

That way it's made very clear to the developer that they are making an explicit choice for security here instead of defaulting to insecure randomness.

I'm arguing this same exact point to the Kotlin team here: https://github.com/Kotlin/KEEP/issues/184

[chtompki]

I'm still mildly confused how:

https://github.com/apache/commons-lang/blob/commons-lang-3.9/src/main/java/org/apache/commons/lang3/RandomStringUtils.java#L36-L37

Caveat: Instances of Random, upon which the implementation of this class relies, are not cryptographically secure.

doesn't lead the reader to conclude the class is not to be used for the purpose of security and in turn minimally use SecureRandom (as stipulated in the java.util.Random javadoc, to which we've linked).

[JLLeitschuh]

@chtompki Because many people don't read the documentation. Especially on the top of classes.

I've found this class of vulnerability in other places because of similar issues around not reading the documentation:

• https://nvd.nist.gov/vuln/detail/CVE-2019-11808?cpeVersion=2.2

I've got 3 outstanding undisclosed vulnerabilities I've reported due to insecure RNG caused by this class.

The problem is, in security, defaults really matter. Unfortunately, by defaulting to insecure RNG, this class is exposing a lot of projects to this vulnerability.

Want some examples? Just GitHub search for "RandomStringUtils token" or "RandomStringUtils key" on github. You'll find tens of thousands of examples.

https://github.com/search?q=RandomStringUtils+token&type=Code

[garydgregory]

@chtompki Because many people don't read the documentation. Especially on the top of classes. uh? That's where this kind of information belongs IMO. "Because many people don't" also implies that many people do. So it's not saying much IMO ;-) Don't assume other folks' brain work like yours or or colleagues'.

My POV here is that this is Javadoc for a util class, we don't need to link to articles on a "proof" on reasons to not use it; if we want to discourage use cases in certain scenarios, we just say so and we're done. If there is a CVE to deal with, let's link to that CVE.

[JLLeitschuh]

Any thoughts on my proposal here around moving the use of insecure randomness to be a more explicit decision as I detailed above:

https://github.com/apache/commons-lang/pull/459#issuecomment-532330032

I've got two more public more CVE's I've had issued as a result of RandomStringUtils:

• CVE-2019-10754
• CVE-2019-10755

There are a couple more that aren't public yet.

[garydgregory]

Please see git master for a Javadoc update to this class which I believe emphasizes the main points brought up by this discussion. If someone would like to improve further upon that, then feel free to rebase this PR on master.

[fche]

Caveat: Instances of Random, upon which the implementation of this class relies, are not cryptographically secure.

doesn't lead the reader to conclude the class is not to be used for the purpose of security and in turn minimally use SecureRandom (as stipulated in the java.util.Random javadoc, to which we've linked).

Any chance of coming around to "let's be secure" as a default position?

[alex91ar]

So, would it be possible to create a Java CVE over this vulnerability? Thanks.

[XenoAmess]

Why not change to use java.security.SecureRandom here?

Is there be any reason for not using it instead?

What about adding a configuration boolean param about it?