Fix #2075 Slow startup because of low entropy for PRNG #2318
Conversation
…Strong() Signed-off-by: Takahiro Nagao <[email protected]>
|
Can anyone review this patch? |
|
Hello @rfelcman, as you were the last one touching that file, could you please review this Pullrequest? It would be awesome if we could fix this soon as it affects our CI-Builds and also may affect Cloud Applications. |
| } catch (NoSuchAlgorithmException e) { | ||
| throw new RuntimeException(e); | ||
| String useStrongRNG = PrivilegedAccessHelper.getSystemProperty(SystemProperties.SECURITY_ENCRYPTOR_USE_STRONG_RANDOM_NUMBER_GENERATOR); | ||
| if (Boolean.parseBoolean(useStrongRNG)) { |
There was a problem hiding this comment.
It will change default behavior into less secure mode by default.
But I'd like rephrase it and change condition content. If users needs it e.g. due performance reasons, they could enable it by system property like
SECURITY_ENCRYPTOR_USE_NON_BLOCKING_RANDOM_NUMBER_GENERATOR = "eclipselink.security.encryptor.non.blocking.random.number.generator"; -> random = new SecureRandom();
For me in this case security is higher, than performance so I'd like keep as a default random = SecureRandom.getInstanceStrong(); and conditionally random = new SecureRandom();
There was a problem hiding this comment.
I understand your concerns.
But IMHO, this pull request does not immediately lower the security level, because it does not change the cipher (AES GCM) for encryption, and using new SecureRandom() as random source is still secure.
From the viewpoint of availability, the impact of stall (almost hang in some environments) due to blocking algorithms cannot be ignored.
If a blocking algorithm is used by default, users might encounter the stall in a production environment, which can cause business losses.
There are several related discussions discouraging use of SecureRandom.getInstanceStrong() in server-side applications:
- https://www.blackduck.com/blog/proper-use-of-javas-securerandom.html
- https://security.docs.wso2.com/en/latest/security-guidelines/secure-engineering-guidelines/secure-coding-guidlines/general-recommendations-for-secure-coding/#java-specific-recommendations_13
- Recommend
new SecureRandomoverSecureRandom.getInstanceStrongfor HTTPS server akka/akka#20908
Should we still use SecureRandom.getInstanceStrong()?
There was a problem hiding this comment.
OK I accepted Your arguments. I don't like blocked production systems too. Lets use new SecureRandom as a default.
| } catch (NoSuchAlgorithmException e) { | ||
| throw new RuntimeException(e); | ||
| String useStrongRNG = PrivilegedAccessHelper.getSystemProperty(SystemProperties.SECURITY_ENCRYPTOR_USE_STRONG_RANDOM_NUMBER_GENERATOR); | ||
| if (Boolean.parseBoolean(useStrongRNG)) { |
There was a problem hiding this comment.
I'd like see there validation for input see e.g.
https://github.com/eclipse-ee4j/eclipselink/blob/master/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/asm/ASMFactory.java#L268
and throw throw ValidationException.incorrect..... to point user what is wrong.
There was a problem hiding this comment.
Thank you for your advice.
I tried to find examples of boolean value validation to meet your expectation, but I could not find a good example...
- https://github.com/eclipse-ee4j/eclipselink/blob/5.0.0-B04/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/helper/ConversionManager.java#L115
- https://github.com/eclipse-ee4j/eclipselink/blob/5.0.0-B04/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/helper/Helper.java#L1133
- https://github.com/eclipse-ee4j/eclipselink/blob/5.0.0-B04/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/mappings/UnidirectionalOneToManyMapping.java#L401
- https://github.com/eclipse-ee4j/eclipselink/blob/5.0.0-B04/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/helper/ConcurrencyUtil.java#L68-L73
For example, java.lang.Boolean.parseBoolean() can be used to parse boolean values, but the method cannot be used for validation since it does not throw exceptions.
Validation could be achieved as in if (!"true".equalsIgnoreCase(value) && !"false".equalsIgnoreCase(value)) { throw ... }, but such code is not pretty.
Could you suggest a good way to validate boolean values if you have?
There was a problem hiding this comment.
Sorry I don't have anything better. Similar pattern "true".equals.... is used sometimes inside EclipseLink e.g.
https://github.com/eclipse-ee4j/eclipselink/blob/master/jpa/org.eclipse.persistence.jpa/src/main/java/org/eclipse/persistence/internal/jpa/EntityManagerImpl.java#L275
Just use this "not pretty" pattern and throw org.eclipse.persistence.exceptions.ValidationException#invalidBooleanValueForProperty.
There was a problem hiding this comment.
I have reflected your comment now. Please check it.
If an invalid value is specified, an exception like below is thrown.
Exception [EclipseLink-7278] (Eclipse Persistence Services - 5.0.0.v202412160411-a5af5837377bcad2adc674a0682717d24a436f69): org.eclipse.persistence.exceptions.ValidationException
Exception Description: The specified boolean value [aaa] for the persistence property [eclipselink.security.encryptor.use.strong.random.number.generator] is invalid, the value must either be "true" or "false".
I'm not sure the reason why the original implementation is creating new SecureRandom instance (i.e. calling In my recognition, there is at least no security problem with creating new SecureRandom instance every time because of non-deterministic nature of SecureRandom. |
Signed-off-by: Takahiro Nagao <[email protected]>
|
It's going to master branch (5.0.x ) EclipseLink version. If You need it in older versions please make a backports. |
…clipse-ee4j#2318) * Fix hang-up due to blocking PRNG returned by SecureRandom.getInstanceStrong() Fixes eclipse-ee4j#2075 (cherry picked from commit 4ef3517) Signed-off-by: Patrick Schmitt <[email protected]>
…clipse-ee4j#2318) * Fix hang-up due to blocking PRNG returned by SecureRandom.getInstanceStrong() Fixes eclipse-ee4j#2075 (cherry picked from commit 4ef3517) Signed-off-by: Zuplyx <[email protected]>
Fixes #2075.
To address hang-up due to blocking PRNG returned by
SecureRandom.getInstanceStrong(), this pull request introduces a system property (eclipselink.security.encryptor.use.strong.random.number.generator) to switchSecureRandom.getInstanceStrong()andnew SecureRandom().new SecureRandom()returns the default PRNG of Java platform [1].Since
SecureRandom.getInstanceStrong()may return a blocking PRNG and is not recommended in server-side code (see [2]), we prefer to usenew SecureRandom()as the default setting.Users can change the behavior by configuring the system property if they want.
[1] https://docs.oracle.com/en/java/javase/21/security/java-cryptography-architecture-jca-reference-guide.html#GUID-AEB77CD8-D28F-4BBE-B9E5-160B5DC35D36
[2] https://www.blackduck.com/blog/proper-use-of-javas-securerandom.html