Skip to content

C++: New query for SSL result conflation #7242

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 8 commits into from
Nov 30, 2021
Merged

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Nov 25, 2021

This is the first of two new queries for CWE-295: Improper Certificate Validation.

Results:

  • on all LGTM C/C++ projects; very thin (only 1 result).
  • this is disappointing, but the query does cover 2 of the 5 examples given in the CWE definition.

Precision:

  • provisionally set to @medium, it's difficult to judge without seeing more real world results.

Performance:

  • tested locally on kamailio_kamailio and chakra-core (performance is great with a warmed up cache)
  • no timeouts on the above LGTM run.

@geoffw0 geoffw0 added the C++ label Nov 25, 2021
@geoffw0 geoffw0 requested a review from a team as a code owner November 25, 2021 15:53
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! Could we upgrade this query to precision high right away? It's certainly not very noisy!

@MathiasVP
Copy link
Contributor

Actually. Do you mind running DCA to make sure we don't get any re-evaluation in the suite @geoffw0?

@geoffw0
Copy link
Contributor Author

geoffw0 commented Nov 29, 2021

Will do...

@geoffw0
Copy link
Contributor Author

geoffw0 commented Nov 29, 2021

It's certainly not very noisy!

I'd like to tune it to detect a bit more (support more libraries and more variants of logic / flow), but the truth may be that the mistake simply isn't as common as the CWE examples led us to believe.

I'm a bit reluctant to increase precision to high without more real world evidence (I guess it wouldn't do much harm though).

@geoffw0
Copy link
Contributor Author

geoffw0 commented Nov 29, 2021

DCA shows no significant change in performance.

@MathiasVP
Copy link
Contributor

Great! Merging! 🎉

@MathiasVP MathiasVP merged commit f4555ed into github:main Nov 30, 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