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

KAFKA-7856: Fix vulnerability CWE-331 #6184

Closed
wants to merge 1 commit into from
Closed

Conversation

tbaldo
Copy link

@tbaldo tbaldo commented Jan 22, 2019

fix vulnerability pointed by CWE.
Cryptographic Issues by Insufficient Entropy (CWE-331 - Flaw medium,SANS
TOP 25)
This change should be promoted to new versions.

fix vulnerability pointed by CWE.
Cryptographic Issues by Insufficient Entropy (CWE-331 - Flaw medium,SANS
TOP 25)
This change should be promoted to new versions.
@tbaldo tbaldo changed the title Fix vulnerability CWE-331 KAFKA-7856 Fix vulnerability CWE-331 Jan 22, 2019
@tbaldo tbaldo changed the title KAFKA-7856 Fix vulnerability CWE-331 KAFKA-7856: Fix vulnerability CWE-331 Jan 22, 2019
@cmccabe
Copy link
Contributor

cmccabe commented Jan 22, 2019

I don't see how this is a vulnerability. The random number generator is only being used to adjust the jitter which is added to the credential refresh period. An attacker doesn't gain anything by guessing this sequence of numbers, as best as I can tell.

@vasartori
Copy link

vasartori commented Jan 22, 2019

hi @cmccabe
This security flaw is a low vulnerability but if you submit kafka client to security scan (like veracode ) a flaw will be appointed.
But this small issue has a "plus" in your security score because it is listed in top 25 of CWE/SANS Most Dangerous Software Errors (http://cwe.mitre.org/top25/)

@cmccabe
Copy link
Contributor

cmccabe commented Jan 22, 2019

Static analysis tools are not perfect and they often warn about things which are not real problems. In this particular case, I don't see why the random numbers used for random delays need to be cryptographically secure. Can you explain how this is a vulnerability?

@tbaldo
Copy link
Author

tbaldo commented Jan 23, 2019

Hi @cmccabe
I agree with you. Static analysis is not very reliable.
Maybe the subtitle is not accurate. I think this could be fix cryptographic issue, not a vulnerability.
See the report show to us. https://imgur.com/a/HCradBj.

Another alternative is use the method setSeed from Random class. What do you think about this approach?

@cmccabe
Copy link
Contributor

cmccabe commented Jan 23, 2019

Maybe the subtitle is not accurate. I think this could be fix cryptographic issue, not a vulnerability

I'm not sure I follow. What's the cryptographic issue here?

Another alternative is use the method setSeed from Random class. What do you think about this approach?

The JavaDoc for Random#Random says:

Creates a new random number generator. This constructor sets the seed of the random number generator to a value very likely to be distinct from any other invocation of this constructor.

Why would we need to initialize the seed on our own here?

@tbaldo
Copy link
Author

tbaldo commented Jan 23, 2019

Why would we need to initialize the seed on our own here?

Because the number generated by Random has a low entropy and when we add setSeed the number is less predictable.
When we use SecureRandom (in PR) the number generate has more digits and entropy is better.

Use SecureRandom has low performance? This will impact in kafka?

@cmccabe
Copy link
Contributor

cmccabe commented Jan 24, 2019

Because the number generated by Random has a low entropy and when we add setSeed the number is less predictable.

Random#Random already seeds the RNG with the current time. From the jdk8 source code:

    /**
     * Creates a new random number generator. This constructor sets
     * the seed of the random number generator to a value very likely
     * to be distinct from any other invocation of this constructor.
     */
    public Random() {
        this(seedUniquifier() ^ System.nanoTime());
    }

Re-seeding it with the time will not help.

When we use SecureRandom (in PR) the number generate has more digits and entropy is better.

You haven't explained why we need more entropy here.

@cmccabe
Copy link
Contributor

cmccabe commented Jan 24, 2019

There seems to be no security issue here.

@cmccabe cmccabe closed this Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants