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

Ruby: Noisiness of rb/weak-cryptographic-algorithm / MD5 detection #11107

Closed
SampsonCrowley opened this issue Nov 3, 2022 · 3 comments · Fixed by #11129
Closed

Ruby: Noisiness of rb/weak-cryptographic-algorithm / MD5 detection #11107

SampsonCrowley opened this issue Nov 3, 2022 · 3 comments · Fixed by #11129
Labels
question Further information is requested

Comments

@SampsonCrowley
Copy link

SampsonCrowley commented Nov 3, 2022

Description of the false positive
https://github.com/github/codeql/blob/a520de3986987baf4c5f846bd82bf68536ae042c/ruby/ql/src/queries/security/cwe-327/BrokenCryptoAlgorithm.ql

This flags every single use of MD5 as a cryptography problem.

MD5 exists for a reason an it's entirely inappropriate to flag any and every usage of it as a cryptographic usage

It is intended to be a lighter weight, simpler algorithm. Using it at all should not be a flag. there are plenty of legitimate use cases that have nothing to do with security

example:

this sorting algorithm has nothing to do with security and absolutely does not need the heavier implementation of an SHA1 hash

Screenshot 2022-11-03 at 9 54 13 AM

@SampsonCrowley SampsonCrowley added the question Further information is requested label Nov 3, 2022
@smowton smowton changed the title General issue Ruby: should we flag all uses of MD5? Nov 3, 2022
@DarrenKipu
Copy link

Confirmed - we have extreme noise from this cluttering up every new branch we create.

@turbo turbo changed the title Ruby: should we flag all uses of MD5? Ruby: Noisiness of rb/weak-cryptographic-algorithm / MD5 detection Nov 3, 2022
@turbo
Copy link
Member

turbo commented Nov 3, 2022

Hi all 👋,

Thank you for reporting this. We are aware of an increased amount of alerts caused by recent changes to this query, and a currently working on a fix. We realize this may be disruptive to your workflow at this time, so this has a high priority for us.

If you need guidance on dismissing the existing alerts or exclusion logic, please let us know and someone from our team will assist here.

Thanks

@aibaars
Copy link
Contributor

aibaars commented Nov 4, 2022

This PR should address the issue #11119

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants