Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(doc): Document public RandomStringUtils exploit #459

Closed
wants to merge 2 commits into from

Conversation

JLLeitschuh
Copy link

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

Document the public exploit that exists for `RandomStringUtils`.
@coveralls
Copy link

coveralls commented Sep 16, 2019

Coverage Status

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

@@ -34,7 +34,11 @@
* RandomStringGenerator</a> instead.</p>
*
* <p>Caveat: Instances of {@link Random}, upon which the implementation of this
* class relies, are not cryptographically secure.</p>
* class relies, are <b>not cryptographically secure</b>.
* Do not use this classes' default implementation of {@link Random} in security sensitive locations,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be

this class' default..

Or

These classes'

Or simply

Avoid using the default implementation in this classs if you need a cryptographically secure...

* class relies, are <b>not cryptographically secure</b>.
* Do not use this classes' default implementation of {@link Random} in security sensitive locations,
* for example password reset key generation, as all future values can be computed as proven by
* <a href="https://medium.com/@alex91ar/the-java-soothsayer-a-practical-application-for-insecure-randomness-c67b0cd148cd?source=friends_link&sk=3db1c41cc81a58f70ed05a7315191385">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with others on the issue with a link to medium, but I don't care much about monetization.

If we have a link to CVE, a paper published in some journal, a standard documentation from some site like ietf, or a wikipedia page, it would be preferrable IMHO.

If this is the only place with an explanation, or the best link to understand the issue, then we need to use a web.archive.org link to prevent it from disappearing after some years.

Copy link
Author

@JLLeitschuh JLLeitschuh Sep 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alex91ar Thoughts?
I have a CVE for JHipster. That's just a specific case though. This is a full POC.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you'd like I can port the article to somewhere else, just say where and I will!

@chtompki
Copy link
Member

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
Copy link
Author

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:
Kotlin/KEEP#184

@chtompki
Copy link
Member

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
Copy link
Author

@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:

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
Copy link
Member

@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
Copy link
Author

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

#459 (comment)

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

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

@garydgregory
Copy link
Member

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
Copy link

fche commented Sep 22, 2020

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
Copy link

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

@XenoAmess
Copy link
Contributor

XenoAmess commented Jun 14, 2021

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?

@iProdigy
Copy link

iProdigy commented Aug 7, 2024

No longer a concern given #1235

@garydgregory
Copy link
Member

We can close this I think unless someone thinks the git master docs can be further improved.

@garydgregory
Copy link
Member

Closing, no further comments, see current code in git master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants