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

DefaultRandomizer.java use SecureRandom.getInstance(algorithm) to get SecureRandom instance Generating pseudo-random numbers #668

Open
JerryDevis opened this issue Mar 18, 2022 · 12 comments

Comments

@JerryDevis
Copy link

JerryDevis commented Mar 18, 2022

SecureRandom.getInstance(SHA1) is to generate a pseudorandom number sequence by continuously performing SHA1.

The right way:
Use SecureRandom.getInstanceStrong() to generate random numbers.

@xeno6696
Copy link
Collaborator

@JerryDevis Thanks.

We will track this issue here, but we are tied to Java 1.7 until July 2022 and the end of Oracle advanced support for Java 1.7.

@xeno6696
Copy link
Collaborator

Method referenced doesn't exist prior to 1.8: https://docs.oracle.com/javase/8/docs/api/java/security/SecureRandom.html#getInstanceStrong--

@xeno6696
Copy link
Collaborator

Also, probably worth asking you @kwwall about configurations regarding /dev/random and /dev/urandom
I've worked in government environments where despite security concerns, the lack of entropy on headless servers made /dev/random the preferred solution. If you check the warning in org.owasp.esapi.util.FileTestUtils the unit tests specifically call Random instead of SecureRandom so I believe the behavior that @JerryDevis is seeing is unique to using the library on Java 1.8. The note in the unit test reads

	/*
		Rational for switching from SecureRandom to Random:
		
		This is used for generating filenames for temporary
		directories. Originally this was using SecureRandom for
		this to make /tmp races harder. This is not necessary as
		mkdir always returns false if if the directory already
		exists.
		
		Additionally, SecureRandom for some reason on Linux
		is appears to be reading from /dev/random instead of
		/dev/urandom. As such, the many calls for temporary
		directories in the unit tests quickly depletes the
		entropy pool causing unit test runs to block until more
		entropy is collected (this is why moving the mouse speeds
		up unit tests).
	*/

So it seems that we are probably fine when the library is running on JVMs < 1.8, but we begin to drift as implementations swap out underneath us.

@kwwall
Copy link
Contributor

kwwall commented Mar 18, 2022

@xeno6696 - First off, I didn't think the race condition necessarily involved mkdir. I thought it could also apply to creating sym links, although I may be thinking of something else.

@JerryDevis - Secondly, if you don't like the way your default JCE behaves for SecureRandom, then change your JCE to something else such as BouncyCastle.

The problem though that I have with this GH issue is that the "ask" is unclear. It's unclear if this gripe is about the fact that the SunJCE SecureRandom uses SHA1PRNG instead of something else (like SHA256PRNG), which really is not an issue as SHA1 is fine as a PRNG. Or is it that it uses /dev/random (which might block) vs /dev/urandom which doesn't block, or something else? I honestly and not sure which particular thing that you are objecting to and what exactly you propose ESAPI do about it so a more detailed explanation would be welcomed by all.

@xeno6696
Copy link
Collaborator

Pulled this from some backchannel talk.

"We do not want this", at least on Linux. (I can't speak for the details on Windows.)

Here's why. On older Linux kernels (which lots of folks still run), I just did a quick experiment calling SecureRandom.getInstanceStrong() with JDK 8. Specifically, I ran the test on a Linux 4.15 kernel and Linux blocked for 15-20 seconds almost every time ran the tiny test program, and just to gather 20 bytes on a system that has not been rebooted since 10 days (last kernel update), so a system that should have plenty of entropy in the kernel entropy pools.

So I plan on closing this as "won't fix" unless @kwwall you think this is something we can loop back to after awhile? I'm guessing we just don't want to make this change until 1.) We can divorce from Java 1.7 and 2.) We can be reasonably assured that those linux versions are behind us.

I'm not sure how long 2 would take.

@kwwall
Copy link
Contributor

kwwall commented Mar 19, 2022

The only way that we can do this in a way that the ESAPI community will accept is to write it in a manner where:

  1. The default behavior (that is SecureRandom.getInstance("SHA1PRNG")) remains as it is today.
  2. We provide an option via a new properties in the ESAPI.properties file where someone can alter the behavior to specif that it use SecureRandom.getInstanceStrong() instead.

But otherwise, there will be hell to pay with older Linux kernels if we cause the programs using ESAPI to block. But I can pretty much assure you that there likely are some using older Linux kernels, especially for embedded systems, IoT, etc. so I don't think the assumption that those older Linux kernels are safely behind us.

@xeno6696
Copy link
Collaborator

It's been awhile. I got /dev/random and /dev/urandom confused. (O_o)

@JerryDevis
Copy link
Author

@kwwall @xeno6696 I think. The DRBG NID_aes_256_ctr can be used(SP800SecureRandomBuilder). The following conditions must be met: Set the seed for the first time, set the seed callback function, set the seed at an interval, and obtain the seed from /dev/random.

@kwwall
Copy link
Contributor

kwwall commented Mar 21, 2022

Two questions for @JerryDevis:

  1. Does this work for ALL operating systems, machine architectures, and JDK implementations?
  2. If the answer to question 1 is "yes", can you please provide a reference link to implementation so we don't have to chase it down. (Note: If this answer to this is "no", I'm not sure I want to do this.)

Thanks, -kevin
P.S.- DRBG stands for Deterministic Random Bit Generator, which still an PRNG, not a TRNG. And while it is a cryptographically secure RNG, it is still a pseudo-random number generator, so even if we make this change, DefaultRandomizer will still be generating pseudo-random numbers, not true random numbers. You probably are aware of that, but the title of this issue may mislead some.

@kwwall
Copy link
Contributor

kwwall commented Apr 28, 2022

@JerryDevis - Now that we are on Java 8 (ESAPI 2.4.0.0 was the first such release), we can carry this forward, but need you to address the above questions.

@JerryDevis
Copy link
Author

Two questions for @JerryDevis:

  1. Does this work for ALL operating systems, machine architectures, and JDK implementations?
  2. If the answer to question 1 is "yes", can you please provide a reference link to implementation so we don't have to chase it down. (Note: If this answer to this is "no", I'm not sure I want to do this.)

Thanks, -kevin P.S.- DRBG stands for Deterministic Random Bit Generator, which still an PRNG, not a TRNG. And while it is a cryptographically secure RNG, it is still a pseudo-random number generator, so even if we make this change, DefaultRandomizer will still be generating pseudo-random numbers, not true random numbers. You probably are aware of that, but the title of this issue may mislead some.

I can't prove question one ,this answer to this is "no"。

@noloader
Copy link
Contributor

It's unclear if this gripe is about the fact that the SunJCE SecureRandom uses SHA1PRNG instead of something else (like SHA256PRNG), which really is not an issue as SHA1 is fine as a PRNG.

Yes, this. SHA-1 is fine as a PRF.

SHA-1 has lost some of its collision resistance. I believe Marc Steven's HashClash has it down to 2^63 (from a theoretical 2^80). But collision resistance is not a property of random number generation.

On the other hand, if the hash lost preimage resistance and the attacker could reverse the output of the hash back to the original message, then that would be bad. The attacker could learn the state of the prng, and secret state is an assumption of a prng's security model. But SHA-1 has not lost preimage resistance.

There are other properties like uniform distribution, but it is still believed SHA-1 behaves as an ideal hash. That is, it does not produce a stream of weak bits when properly keyed.

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

No branches or pull requests

4 participants