Skip to content

C++: Add more barriers to cpp/tainted-arithmetic #6125

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jun 23, 2021
Merged

C++: Add more barriers to cpp/tainted-arithmetic #6125

merged 6 commits into from
Jun 23, 2021

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Jun 21, 2021 •
edited
Loading

This PR does two things:

  • First, it converts cpp/tainted-arithmetic to a path-problem query. This is mostly to help investigate results on LGTM. It seems like a good fit for this query anyway.
  • Second, it puts the bounded predicate (which was introduced in C++: Add more sanitizers to cpp/uncontrolled-arithmetic #5887 for cpp/uncontrolled-arithmetic) in a separate QL file so that it can also be used by cpp/tainted-arithmetic.

We should also be able to use it in cpp/uncontrolled-allocation-size once #6120 is merged (which this PR currently conflicts with).

In addition, as this query is kinda noisy I've added another barrier to block flow out of "small integer" types.

Since it's difficult to do an LGTM difference run I've just manually compared the "before state" with the "after state".

Before: https://lgtm.com/query/519140448821761310/
After: https://lgtm.com/query/2136072651063415759/

The only real noticeable difference is that we remove all the FPs on mldbai/giflib and adriweb/tivars_lib_cpp. These results are in fact removed by the e.getUnspecifiedType().(IntegralType).getSize() <= 1 barrier.

The two test failures are caused by the conversion to path-problem in the internal testcases. I've fixed them in a PR in the internal repo that should be merged with this PR.

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

geoffw0 commented Jun 22, 2021

LGTM, subject to resolving conflicts with #6120 after it is merged.

The only real noticeable difference is that we remove all the FPs on mldbai/giflib and adriweb/tivars_lib_cpp. These results are in fact removed by the e.getUnspecifiedType().(IntegralType).getSize() <= 1 barrier.

With 1 byte types are people just being more careful and deliberate about overflow?

I'm surprised the cases in mldbai/giflib at least aren't caught by bounded and the bitwise & case anyway.

@MathiasVP
Copy link
Contributor Author

I'm surprised the cases in mldbai/giflib at least aren't caught by bounded and the bitwise & case anyway.

Good point! I took another look at the results, and it turns out that the & case indeed removes the results related to the & cases.
However, there are a couple of results in the same project that ends at the expression Buf[0]--, which are removed by the barrier that blocks flow through small integers. As these will underflow to at most a value of 255, I think it's fine to remove these alerts.

@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Jun 23, 2021
@MathiasVP
Copy link
Contributor Author

This PR doesn't need a change-note as the change-note added in #5667 also perfectly covers the changes made in PR. 🎉

@MathiasVP MathiasVP added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Jun 23, 2021
@MathiasVP
Copy link
Contributor Author

LGTM, subject to resolving conflicts with #6120 after it is merged.

Conflicts have been resolved. I've also rebumped the internal PR's submodule.

@MathiasVP
Copy link
Contributor Author

The tests on this PR, and the internal one, are green now @geoffw0.

geoffw0
geoffw0 reviewed Jun 23, 2021
View reviewed changes
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

LGTM.

I suspect we may be able to remove the exclusion for 1-byte types at some point, when we have other improvements in place, but if it's doing good now I don't oppose it.

@MathiasVP @geoffw0
Update cpp/ql/src/Security/CWE/CWE-190/Bounded.qll
c444754 
Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
geoffw0
geoffw0 approved these changes Jun 23, 2021
View reviewed changes
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Thanks, 👍

@MathiasVP MathiasVP merged commit 9b8f558 into github:main Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoffw0 geoffw0 geoffw0 approved these changes

Assignees
No one assigned
Labels
C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR no-change-note-required This PR does not need a change note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MathiasVP @geoffw0