Skip to content
This repository was archived by the owner on Jan 5, 2023. It is now read-only.

Conversation

dilanbhalla
Copy link

Hello! In this PR I have begun writing basic support for detecting dangerous crypto algorithms. Included is a simple dataflow query as well as a library that will hopefully eventually include a variety of dangerous algorithms (I based this off of the JavaScript library for crypto). So far I have included detection for a couple of dangerous algorithms (MD5 and DES) along with respective tests, and was just hoping I could get some feedback on this before expanding upon the library. Thanks!

@dilanbhalla
Copy link
Author

I am also getting a build error when checking if all QL and Go code is autoformatted, but I have checked multiple times that all of these files have been autoformatted. Is there something else that needs to be autoformatted as well that I am missing? Thanks!

@intrigus-lgtm
Copy link
Contributor

CryptoLibraries.qll has been inspired by https://github.com/github/codeql/blob/b25a4614de01a64884626483ca8d6389c642c114/javascript/ql/src/semmle/javascript/frameworks/CryptoLibraries.qll right?

@dilanbhalla
Copy link
Author

Hi @intrigus-lgtm yes that's correct

@dilanbhalla
Copy link
Author

@intrigus-lgtm https://github.com/github/codeql/blob/master/javascript/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql also provides context for how the library is used, if that helps!

Copy link
Contributor

@max-schaefer max-schaefer left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! This is certainly a query we'd like to have, and as an experimental contribution we hope to get it merged fairly quickly. I don't really understand a core part of the modelling, though; see my question below.

@intrigus-lgtm
Copy link
Contributor

Something went wrong with your last commit I think:

Files changed 866

@dilanbhalla
Copy link
Author

Hi @intrigus-lgtm, sorry about that I noticed this as soon as it pushed. Correcting it right now it should be fixed in just a minute.

Copy link
Contributor

@max-schaefer max-schaefer left a comment

Choose a reason for hiding this comment

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

Broadly LGTM, just a few remaining minor comments.

@dilanbhalla
Copy link
Author

Had a little build error there on the last submission, all fixed now though 👍

@max-schaefer max-schaefer merged commit d675daa into github:main Aug 14, 2020
@max-schaefer max-schaefer added the needs-polishing An external contribution that may need follow-up work. label Aug 14, 2020
@max-schaefer
Copy link
Contributor

Many thanks for your contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-polishing An external contribution that may need follow-up work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants