[
https://issues.apache.org/jira/browse/NIFI-1240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15036923#comment-15036923
]
ASF GitHub Bot commented on NIFI-1240:
--------------------------------------
Github user apiri commented on the pull request:
https://github.com/apache/nifi/pull/138#issuecomment-161473916
Good catch and certainly makes a lot of sense.
Code change looks good and certainly in line with the Javadoc for
SecureRandom and makes a lot of sense to that end. All tests pass and this
should not affect any backwards compatibility.
Only area is that of the starred imports that were likely done by your IDE.
These violate the checkstyle setup for the project (you can run this via mvn
<goals> -Pcontrib-check).
If adjusting these to be single imports is okay with you, I can make the
adjustment and close out. Otherwise if you would like to do so, feel free to
push up another commit to the branch you made the PR from.
Thanks for the contribution.
> SecureRandom is improperly seeded with current time
> ---------------------------------------------------
>
> Key: NIFI-1240
> URL: https://issues.apache.org/jira/browse/NIFI-1240
> Project: Apache NiFi
> Issue Type: Bug
> Components: Core Framework
> Affects Versions: 0.4.0
> Reporter: Andy LoPresto
> Assignee: Andy LoPresto
> Priority: Critical
> Labels: easyfix, security
> Fix For: 0.4.0
>
> Original Estimate: 1h
> Remaining Estimate: 1h
>
> In PasswordBasedEncryptor.java, java.security.SecureRandom is used to
> generate a salt for key derivation. However, the SecureRandom instance is
> seeded by System.getCurrentTimeInMillis(), which is not random and is
> predictable. Instead, we should allow SecureRandom to seed itself by calling
> SecureRandom.nextBytes().
> The instance accessor should also explicitly specify "SUN" as the
> cryptographic service provider to avoid default CSP issues.
> "First, while it is good that the code explicitly specifies the instance of
> SecureRandom to be SHA1PRNG (because a call to .getInstance() will return
> whatever the Java properties specify), to be completely explicit, it should
> be .getInstance("SHA1PRNG", "SUN") because the Java cryptographic service
> provider (CSP) should be selected. On most systems this will default to Sun,
> but it can conceivably cause issues if a different CSP is prioritized.
> Second, seeding the SecureRandom with the current time is most definitely not
> random and is predictable. SecureRandom.nextBytes() actually self-seeds if
> the instance had not previously been seeded, and this manual seeding is
> decreasing the entropy used. These two issues will be resolved in an upcoming
> release, but are not related to the encryption issue we are addressing now."
> The fix is very simple. I have searched the project and this is the only use
> of SecureRandom which is manually seeded.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
