Skip to content

Further improvements to cpp/weak-cryptographic-algorithm #6099

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

Merged
merged 15 commits into from
Jun 21, 2021

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jun 17, 2021

Further improvements to cpp/weak-cryptographic-algorithm inspired by the findings of https://github.com/github/codeql-c-team/issues/550.

  • exclude results in template code as they are likely to be misplaced.
  • exclude results in function calls that don't seem to involve buffers.
  • adds a lot of new test cases based on real world stuff we've encountered.

I've also increased the query to @precision high in this PR - I think it's time to do this.

In the tests, we lose some false positive results and nearly as many true positives; I think this is an OK tradeoff as we're targeting @precision high.

In the real world, most of the results we lose are duplicate results in supporting code that isn't really where the encryption happens, leaving better results about the same algorithms in the same projects. In a few cases we lose something I'd rather have kept, but I think that's a necessary cost to make an essentially heuristic query precise enough for the code scanning suite.

Here's an LGTM run of the new version of the query on 20 projects: https://lgtm.com/query/78447995241338225/ (these are the same first 20 projects with results from the run in https://github.com/github/codeql-c-team/issues/550 so the density of data is high, and there's some discussion on that issue).

Performance is about 70% slower than before when I run locally. Most of this is because isFromTemplateInstantiation (and possibly isConstant?) calculate getEnclosingElement, which slows things down the first time it's computed.

CPP-Differences run: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/2088/

@geoffw0 geoffw0 added the C++ label Jun 17, 2021
@geoffw0 geoffw0 requested a review from a team as a code owner June 17, 2021 18:11
@geoffw0
Copy link
Contributor Author

geoffw0 commented Jun 21, 2021

CPP-Differences run: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/2088/

No changes, and analysis time seems unaffected as far as I can tell.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jun 21, 2021

I'll add a change note...

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jun 21, 2021

Added change note, fixed merge conflict (the @security-severity had been updated on main).

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

All of the changes LGTM!

Given that https://github.com/github/codeql/blob/main/cpp/change-notes/2021-05-19-weak-cryptographic-algorithm.md already documents the reduced FP rate, I think the change-note should just say something like "increased @precision of cpp/weak-cryptographic-algorithm to high". What do you think?

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jun 21, 2021

Given that https://github.com/github/codeql/blob/main/cpp/change-notes/2021-05-19-weak-cryptographic-algorithm.md already documents the reduced FP rate,

It does, but that was from separate changes a week or so ago. I decided it'd be simpler to write a new change note rather than figure out whether I can safely say that this change is covered by that note as well.

I've added a note about the @precision. That's an important detail I had missed out.

Co-authored-by: Mathias Vorreiter Pedersen <mathiasvp@github.com>
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM!

@MathiasVP MathiasVP merged commit 05389bb into github:main Jun 21, 2021
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