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

Java: Improve weak crypto query #17869

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jcogs33
Copy link
Contributor

@jcogs33 jcogs33 commented Oct 30, 2024

Description

Removes weak hash algorithms such as MD5 and SHA1 from java/weak-cryptographic-algorithm and adds these cases to java/potentially-weak-cryptographic-algorithm instead.

The removal of weak hash algorithms from java/weak-cryptographic-algorithm aligns with other languages, e.g. #11129.

Consideration

Let me know if there is any concern with changing the behavior of rankedInsecureAlgorithm instead of doing the exclusion for java/weak-cryptographic-algorithm directly in BrokenAlgoLiteral? I chose to do the exclusion in rankedInsecureAlgorithm since doing so automatically adds the weak hash cases to java/potentially-weak-cryptographic-algorithm due to this code.

Note that there were some overlapping results between the two queries for cases where a weak hash algorithm string literal is used as the default value in a getProperty call (e.g. the "MD5" value in this test case). In order to avoid duplicate results for these cases in the java/potentially-weak-cryptographic-algorithm query, I've added an exclusion to InsecureAlgoLiteral.

@jcogs33
Copy link
Contributor Author

jcogs33 commented Nov 3, 2024

DCA alert changes look reasonable.

@jcogs33 jcogs33 changed the title [DRAFT] Java: Improve weak crypto query Java: Improve weak crypto query Nov 3, 2024
@jcogs33 jcogs33 marked this pull request as ready for review November 3, 2024 23:34
@jcogs33 jcogs33 requested a review from a team as a code owner November 3, 2024 23:34
Copy link
Contributor

@egregius313 egregius313 left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

2 participants