C++: Use a `TaintTracking::Configuration` in three more queries #8382

MathiasVP · 2022-03-09T12:12:22Z

Initially, I was also going to modernize the sanitizers used in the three queries, but I realized that this code should be changed anyway once we merged the ongoing work on range analysis. So for now I've copied the ad-hoc sanitizers into these three queries to not change their behavior.

…on'.

…x-validation'.

…ation-size'.

geoffw0

Looks good, I've made a few comments and questions. I will also do some kind of run on LGTM of each query before approving this to merge.

cpp/ql/src/Critical/OverflowDestination.ql

cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql

geoffw0 · 2022-03-09T14:17:17Z

LGTM runs...

cpp/overflow-destination

before: https://lgtm.com/query/7063513232700019020/
after: https://lgtm.com/query/5954408493628011040/

cpp/unclear-array-index-validation

before: https://lgtm.com/query/7161980796863928292/
after: https://lgtm.com/query/1344744053190000681/

cpp/uncontrolled-allocation-size

before: https://lgtm.com/query/8782095103494040182/
after: https://lgtm.com/query/5576401620571102812/

MathiasVP · 2022-03-09T14:17:41Z

LGTM runs...

cpp/overflow-destination

before: https://lgtm.com/query/7063513232700019020/

after: https://lgtm.com/query/5954408493628011040/

cpp/unclear-array-index-validation

before: https://lgtm.com/query/7161980796863928292/

after: https://lgtm.com/query/1344744053190000681/

cpp/uncontrolled-allocation-size

before: https://lgtm.com/query/8782095103494040182/

after: https://lgtm.com/query/5576401620571102812/

Thanks a lot! I was just in the process of creating diff queries myself.

geoffw0 · 2022-03-09T14:20:11Z

They're not diff queries - I decided that viewing the difference in presentation (path-problem) was more important in this case.

MathiasVP · 2022-03-09T14:22:31Z

They're not diff queries - I decided that viewing the difference in presentation (path-problem) was more important in this case.

Yeah, that's fair. I stripped out the path-problem stuff in cpp/overflow-destination and created a diff like this: https://lgtm.com/query/2343590706647795193/, but I'm totally okay with just using two runs and comparing manually.

geoffw0 · 2022-03-09T18:02:37Z

On cpp/overflow-destination:

paths show where taint comes from, and for this query taint is only used to 'filter' results to the most dangerous cases - so it isn't as relevant as for other queries. Still nice to have.
2 new results for gpac/gpac; LGTM.
3 lost results for alibaba/AliSQL (the last 3 in page0zip.cc)
6 new results for arvidn/libtorrent; they're displaying as a table not as alerts, I think they're outside the source location?
1 new result for kamailio/kamalio (the one in parser_carrierroute.c); LGTM.
1 new result for haiku/haiku (the one in pool.c); LGTM.
1 new result for pure-data; LGTM.
2 new results for libretro-uae (in dr_flac.h and stdstring.c); this first is a little weak IMO but not wrong, the second LGTM.
1 lost result for lsds/sgx-lkl (the one in symbol.c)
overall the new results look good, I suspect they're due to wider sources; the lost results are sparse and don't concern me.
this requires a change note, and I think we can claim wider results as well as paths.

MathiasVP · 2022-03-09T18:05:40Z

Thanks for looking at the result changes for cpp/overflow-destination! I can take a look at the other ones tomorrow.

overall the new results look good, I suspect they're due to wider sources; the lost results are sparse and don't concern me.

It may also be due to the removal of the barriers on "unpredictable" instructions. If we like the results I don't think we should include that barrier to remove these results.

this requires a change note, and I think we can claim wider results as well as paths.

Indeed, this should have a change note. If nothing else, the change note should mention that these queries are now path-problem queries. I'll write the change note first thing tomorrow if we like the result changes on the other two queries.

geoffw0 · 2022-03-09T18:17:07Z

On cpp/uncontrolled-allocation-size:

1 new result in vim/vim (line 741); very long path but looks plausible.
1 new result in tesseract-ocr/tesseract (in bitvec.h); similar to the above.
2 lost results in aws/s2n-tls; perhaps we don't support / model fscanf as a source?
- ~~TODO: if this is true, create an issue for it and note the results we expect to probably recover.~~ --- created issue https://github.com/github/codeql-c-team/issues/896
2 new results in gpac/gpac; look OK I think. str2ulong is terrifying, perhaps we didn't track taint through that previously???
10 new results in alibaba/AliSQL (all but the one in my_malloc.c); seem plausible.
1 new result in apache/trafficserver (CacheTool.cc); the path goes through a bunch of largely irrelevant template code; the reinterpret_cast on line 739 is the key; looks correct and impressive!
I stopped reading at this point; there are quite a lot of new results but they seem good to me.

geoffw0 · 2022-03-09T18:22:23Z

It may also be due to the removal of the barriers on "unpredictable" instructions. If we like the results I don't think we should include that barrier to remove these results.

You might be right, specifically the case:

// don't use dataflow through calls to pure functions if two or more operands
// are unpredictable

which might mean we now get taint flowing through things like pointer differences (endOfBuffer - pointerIntoBuffer) where we might not have done before. I think I'm OK with the results like that I saw for cpp/overflow-destination.

geoffw0 · 2022-03-09T18:38:18Z

On cpp/unclear-array-index-validation:

this is the nosiest of the three queries and has got noisier (results in 83 -> 143 of 184 projects)
- that's a bit of a warning sign that something may be wrong with the query.
- though it lacks a @precision tag, and is probably not used all that much.
  - I'd be happy if you want to give it @precision low. That's neater and I don't think will mean it suddenly gets run in one of our standard suites?
2 new results in bitcoin/bitcoin
- in uint256.cpp, I think taint is flowing through a char type that may not be appropriate
  - were there any barriers based on char / small types previously?
- in system.cpp, are we tainting i from i < argc? If not, I'm not sure what's going on.
I'd like to improve this one a bit.

MathiasVP · 2022-03-10T09:00:33Z

I'd be happy if you want to give it @precision low. That's neater and I don't think will mean it suddenly gets run in one of our standard suites?

Indeed, I think we should give it a @precision low. I can do that in this PR.

2 new results in bitcoin/bitcoin

in uint256.cpp, I think taint is flowing through a char type that may not be appropriate

were there any barriers based on char / small types previously?

in system.cpp, are we tainting i from i < argc? If not, I'm not sure what's going on.

I'd like to improve this one a bit.

There are no barriers based on small types in DefaultTaintTracking, no. With that said, that might be a reasonable approach to filtering out some FPs from this query.

I might start by porting over the unpredictable instruction barrier to just this one query. I don't want to do too much work on improving the FP rate of a @precision low query, but I think we can improve it a lot with reasonable little work.

…ex-validation' (and not the array expression).

MathiasVP · 2022-03-10T11:24:14Z

I'd be happy if you want to give it @precision low. That's neater and I don't think will mean it suddenly gets run in one of our standard suites?

Fixed in 693eca2.

MathiasVP · 2022-03-10T11:27:15Z

@geoffw0 all the new results on cpp/unclear-array-index-validation happened because I incorrectly changed the sink in the query to be the entire array expression (instead of the offset expression) here). I've fixed this in 0d3e47b.

We now get a much more manageable difference. Here's a diff query and here are all the results. I think they look good!

geoffw0

Yep, looks much better. 👍

There are still false positives for that query to do with characters being checked against (presumably) 256-long arrays, but I don't think we need to fix that as part of this work.

MathiasVP · 2022-03-10T13:43:05Z

DCA showed a giant number of new results on cpp/unclear-array-index-validation (likely due to the bug I introduced in 2328898, but which I then fixed in 0d3e47b). I'll start another DCA run on the HEAD of this PR and merge it if the results look better.

geoffw0

Still 👍

MathiasVP · 2022-03-14T11:19:33Z

I've created the internal PR with the required changes.

…pp/unclear-array-index-validation' to prevent an explosion of new results.

geoffw0

Changes LGTM.

geoffw0 · 2022-03-15T09:43:50Z

cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql

+      nodeIsBarrierEqualityCandidate(node, access, checkedVar)
+    )
+    or
+    // Don't use dataflow into binary instructions if both operands are unpredictable


I do wonder what case this is supposed to cover besides subtraction ... but the existence of the case below as well suggests there's more to it.

Yeah, these two barriers block flow in ~100 examples on vim/vim. For this first case, it's most often subtraction/addition. The next case also handles stuff like getting the index of some character in a non-constant string (which they know is bounded due to invariants on the string). I imagine we could do something much more clever here if we want to, but that should probably be once we decide to invest effort in rewriting this query to something much more modern.

MathiasVP added 3 commits March 9, 2022 12:07

C++: Use a 'TaintTracking::Configuration' for 'cpp/overflow-destinati…

d7652f9

…on'.

C++: Use a 'TaintTracking::Configuration' for 'cpp/unclear-array-inde…

2328898

…x-validation'.

C++: Use a 'TaintTracking::Configuration' for 'cpp/uncontrolled-alloc…

8a8fb69

…ation-size'.

MathiasVP added the C++ label Mar 9, 2022

MathiasVP requested a review from a team as a code owner March 9, 2022 12:12

geoffw0 reviewed Mar 9, 2022

View reviewed changes

cpp/ql/src/Critical/OverflowDestination.ql Show resolved Hide resolved

cpp/ql/src/Critical/OverflowDestination.ql Show resolved Hide resolved

cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql Show resolved Hide resolved

C++: Actally convert 'cpp/overflow-destination' to a path-problem query.

f267696

MathiasVP added 3 commits March 10, 2022 10:17

C++: Give 'cpp/unclear-array-index-validation' precision low.

693eca2

C++: Pick the offset expression as the sink in 'cpp/unclear-array-ind…

0d3e47b

…ex-validation' (and not the array expression).

C++: Add change note.

bff10e8

github-actions bot added the documentation label Mar 10, 2022

geoffw0 previously approved these changes Mar 10, 2022

View reviewed changes

MathiasVP added 2 commits March 11, 2022 09:24

Merge branch 'main' into use-taint-configuration-in-three-more-queries

272e096

C++: Accept test changes.

3c17d90

MathiasVP dismissed geoffw0’s stale review via 3c17d90 March 11, 2022 09:30

geoffw0 previously approved these changes Mar 11, 2022

View reviewed changes

Merge branch 'main' into use-taint-configuration-in-three-more-queries

0da5d91

MathiasVP added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Mar 14, 2022

C++: Port the 'predictable' barrier from 'DefaultTaintTracking' to 'c…

50b7776

…pp/unclear-array-index-validation' to prevent an explosion of new results.

MathiasVP dismissed geoffw0’s stale review via 50b7776 March 14, 2022 13:14

MathiasVP added 2 commits March 14, 2022 13:38

C++: Use 'getAstVariable' now that 'getASTVariable' is deprecated.

7593eba

Merge branch 'main' into use-taint-configuration-in-three-more-queries

7e0e7d5

geoffw0 approved these changes Mar 15, 2022

View reviewed changes

MathiasVP merged commit 9642e59 into github:main Mar 15, 2022

C++: Use a TaintTracking::Configuration in three more queries #8382

C++: Use a TaintTracking::Configuration in three more queries #8382

Uh oh!

Conversation

MathiasVP commented Mar 9, 2022

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

geoffw0 commented Mar 9, 2022

Uh oh!

MathiasVP commented Mar 9, 2022

Uh oh!

geoffw0 commented Mar 9, 2022

Uh oh!

MathiasVP commented Mar 9, 2022

Uh oh!

geoffw0 commented Mar 9, 2022

Uh oh!

MathiasVP commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geoffw0 commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geoffw0 commented Mar 9, 2022

Uh oh!

geoffw0 commented Mar 9, 2022

Uh oh!

MathiasVP commented Mar 10, 2022

Uh oh!

MathiasVP commented Mar 10, 2022

Uh oh!

MathiasVP commented Mar 10, 2022

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

MathiasVP commented Mar 10, 2022

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

MathiasVP commented Mar 14, 2022

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

geoffw0 Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

MathiasVP Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

C++: Use a `TaintTracking::Configuration` in three more queries #8382

C++: Use a `TaintTracking::Configuration` in three more queries #8382

MathiasVP commented Mar 9, 2022 •

edited

Loading

geoffw0 commented Mar 9, 2022 •

edited

Loading