C++: IR dataflow edges through outparams #2704

rdmarsh2 · 2020-01-28T01:11:59Z

This connects ReturnIndirectionInstructions to ReadSideEffectInstructions in the data flow library by adding a new case to TReturnKind. It doesn't cover flow into functions via indirect parameters - I think that will need some changes to the shared library to use a class similar to ReturnKind for argument IDs.

jbj

This looks promising! Please add tests to demonstrate that it works.

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Instruction.qll

jbj · 2020-01-28T08:39:26Z

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Instruction.qll

@@ -743,7 +743,7 @@ class ReturnValueInstruction extends ReturnInstruction {
  final Instruction getReturnValue() { result = getReturnValueOperand().getDef() }
 }

-class ReturnIndirectionInstruction extends Instruction {
+class ReturnIndirectionInstruction extends VariableInstruction {


Until now I thought that ReturnIndirectionInstruction was for simulating a read of a returned pointer. I can see now it's for simulating a read of a pointer parameter. Please add QLDoc.

jbj · 2020-01-31T15:40:02Z

I've got the input parameter indirection case covered in #2737, so it's fine that it's missing from this PR.

Update test expectations

rdmarsh2 · 2020-03-10T20:08:38Z

There's internal tests failing now. The changes in CWE-120 look incorrect, the others are restoring results that were lost when we enabled the IR. I'll look into the CWE-120 changes before I open an internal PR.

rdmarsh2 · 2020-03-10T20:58:11Z

It looks like the new false positives in the CWE-120 test come via the Chi chain on argv. I think everything is working as currently intended on the IR side. The easy patch would be to add accesses to argv as barriers in the query to prevent that chi chain flow. The harder solution would be to do an interprocedural analysis to identify functions that take a pointer to non-const but don't write to it and avoid generating side effect writes.

jbj · 2020-03-11T08:08:28Z

It looks like the new false positives in the CWE-120 test come via the Chi chain on argv. I think everything is working as currently intended on the IR side. The easy patch would be to add accesses to argv as barriers in the query to prevent that chi chain flow. The harder solution would be to do an interprocedural analysis to identify functions that take a pointer to non-const but don't write to it and avoid generating side effect writes.

I don't fully understand that problem. Can you turn it into a test case so we can see the IR? As I wrote in my initial review, I'd also like to see test cases that demonstrate that the new flow is working as intended.

rdmarsh2 · 2020-03-12T22:16:18Z

I added an example of that to ssa.cpp - you'll want to look at the unsound version for the moment. There's already tests for the dataflow portion that I added to cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp and cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp in earlier commits.

geoffw0 · 2020-03-18T14:26:44Z

This PR will fix 2 out of the 3 SAMATE test regressions identified in https://git.semmle.com/Semmle/code/pull/36464 (the CWE-78 and CWE-197 ones).

Is there anything I can do to help with this PR?

jbj · 2020-03-18T15:42:19Z

For the record, I believe these are the changes in CWE-120/.../UnboundedWrite.expected from the internal repo that show problematic flow from argv:

Diff:
--- expected
+++ actual
@@ -2,4 +2,9 @@
 | main.c:49:5:49:10 | call to strcpy | This 'call to strcpy' with input from $@ may overflow the destination. | main.c:113:15:113:18 | argv | argv |
 | main.c:49:5:49:10 | call to strcpy | This 'call to strcpy' with input from $@ may overflow the destination. | main.c:114:15:114:18 | argv | argv |
 | main.c:153:7:153:12 | call to strcpy | This 'call to strcpy' with input from $@ may overflow the destination. | main.c:149:20:149:25 | call to getenv | call to getenv |
+| main.c:181:5:181:11 | call to sprintf | This 'call to sprintf' with input from $@ may overflow the destination. | main.c:113:15:113:18 | argv | argv |
+| main.c:181:5:181:11 | call to sprintf | This 'call to sprintf' with input from $@ may overflow the destination. | main.c:114:15:114:18 | argv | argv |
+| main.c:181:5:181:11 | call to sprintf | This 'call to sprintf' with input from $@ may overflow the destination. | main.c:115:15:115:18 | argv | argv |
+| main.c:181:5:181:11 | call to sprintf | This 'call to sprintf' with input from $@ may overflow the destination. | main.c:116:15:116:18 | argv | argv |
+| main.c:181:5:181:11 | call to sprintf | This 'call to sprintf' with input from $@ may overflow the destination. | main.c:117:15:117:18 | argv | argv |
 | main.c:181:5:181:11 | call to sprintf | This 'call to sprintf' with input from $@ may overflow the destination. | main.c:180:12:180:15 | argv | argv |

jbj · 2020-03-18T15:53:30Z

Sorry for taking so long to follow up on this.

The additional results in CWE-120 don't look terrible to me, and I don't think they're blocking for this PR. The query still has the same primary-location results as before, and we can address the duplication of sources in a separate PR. All these result changes show that this is a disruptive PR that can uncover other latent bugs, so we'll want to run CPP-Differences. I'll trigger one now.

rdmarsh2 · 2020-04-03T00:30:51Z

I've been working my way through the CPP-Differences results with the path explanations from #3189. I'm seeing a lot of results where fields are getting conflated when a ReturnIndirection reads a chi node and then a BufferMayWriteSideEffect has a may-write to a whole allocation. I think #3118 might fix that.

rdmarsh2 · 2020-04-03T00:54:08Z

#3118 doesn't immediately fix those. I'll look at it some more tomorrow.

jbj · 2020-04-08T11:43:34Z

The CPP-Differences results have arrived: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1025/

jbj · 2020-04-08T12:13:03Z

@rdmarsh2 It looks like your latest merge from master accidentally added dozens of little test files from your working copy 😬. I recommend rebasing them away.

MathiasVP · 2020-04-08T14:58:32Z

I had a deeper look at the results now and came to a different set of conclusions:

g/openjdk/jdk

cpp/unbounded-write

5 new TP:
- socketTransport.c:215
- main.cpp:459
- socketTransport.c:123
- socketTransport.c:116
3 New TP that are excluded by jbj/ql@a715866:
- libproc_impl.c:179
- symtab.c:213
- symtab.c:240

These should be recoverable with field flow. Overall jbj/ql@a715866 reduces query results from 92 to 89.

cpp/path-injection

New TP (but excluded by jbj@a715866):
- symtab.c:222
- symtab.c:244
- symtab.c:232
- libproc_impl.c:184
  - This one should be recoverable with field flow.

Overall jbj/ql@a715866 reduces query results from 16 to 8. The remaining results look like true positives.

cpp/uncontrolled-allocation-size

New FP (but excluded by jbj@a715866):
- gifalloc.c:91
- gifalloc.c:79

Overall jbj/ql@a715866 reduces query results from 4 to 0. All of them seem like false positives.

cpp/uncontrolled-process-operation

New TP:
- java_md_solinux.c:550

jbj/ql@a715866 keeps the 2 query results. They both look like true positives.

g/torvalds/linux

cpp/path-injection

1 new TP:
- gen_init_cpio.c:445

Overall jbj/ql@a715866 reduces query results from 25 to 23. Both results look like true positives.

jbj · 2020-04-08T16:05:52Z

Here are my notes about the three snapshots I looked at. TL;DR: cherry-picking jbj@a715866 will fix all new FPs but move four TPs that should be recoverable with field flow.

Git

New TP from TaintedPath.ql on git/git in daemon.c:1476.
Everything else is FP, and it's fixed by the FieldAddressInstruction exclusion but not by merging from master

Wireshark

cpp/uncontrolled-allocation-size: 16 new FPs in PR head, all suppressed by merging from master.
cpp/path-injection:
- new TP in capture_sync.c, preserved by the conflation suppression
- Four new FP in lemon.c, all suppressed by merging from master

PHP

cpp/uncontrolled-allocation-size: all 155 results (all new FPs) fixed by the FieldAddressInstruction exclusion but not by merging from master
cpp/path-injection
- The FieldAddressInstruction exclusion removes TPs in pchar.c, util.c, main.c, and phpdbg_list.c, where field conflation plays the role of field flow and happens to match up the right field names. All results are essentially the same code, just in different main-routines. Can we recover these results with field flow?
- With output indirections + the FieldAddressInstruction exclusion we get two new results over master, and they are TP.

except if it's from a union. This prevents field conflation through buffers of `UnknownType`.

rdmarsh2 · 2020-04-08T22:29:33Z

NEW: cpp/unbounded-write (src/java.base/unix/native/libjli/java_md_solinux.c:395)

I think that characters from the getenv call do reach the source buffer of the sprintf, but the string is bounded after the call to snprintf (macro-expanded from JLI_Snprintf) in GetJVMPath. Probably a good place for a query-specific barrier.

NEW: cpp/uncontrolled-process-operation (src/java.base/unix/native/libjli/java_md_solinux.c:550)

Fixed by merging from the latest master.

Can we recover these results with field flow?

I think so, but I hit a join order issue with DataFlowImplCommon::Cached::FlowThrough::Final::parameterValueFlow0#ffff#cur_delta on my merge branch. It looks like the join ordering issue is introduced by something in #3118 but the tuple counts are much larger in the merge branch.

This would otherwise have lost a good qltest result at CWE-134/semmle/funcs/funcsLocal.c:58:9:58:10

rdmarsh2 · 2020-04-09T00:03:13Z

The UnknownType case for flow through partial reads is doing field conflation... I'll take that case out.

MathiasVP · 2020-04-10T09:10:37Z

I think so, but I hit a join order issue with DataFlowImplCommon::Cached::FlowThrough::Final::parameterValueFlow0#ffff#cur_delta on my merge branch. It looks like the join ordering issue is introduced by something in #3118 but the tuple counts are much larger in the merge branch.

I've just been hit by this join order issue as well. I'm investigating it now.

rdmarsh2 · 2020-04-10T18:10:58Z

Latest CPP-Differences finished: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1032

rdmarsh2 · 2020-04-10T22:52:08Z

Looking through the latest openjdk results, all the results that @MathiasVP mentioned going away with a cherry-pick of a715866 are back in the current PR after restoring flows through from the source value operand of partial loads of arrays and unions. Results on the other snapshots line up with the master merge and cherry pick at 9f40886

g/openjdk/jdk

cpp/unbounded-write

TPs restored as of 3b59118:
- libproc_impl.c:179
- symtab.c:213
- symtab.c:240
  These are all explained by partial loads through arrays.

cpp/path-injection

TPs restored as of 3b59118:
- symtab.c:222
- symtab.c:244
- symtab.c:232
- libproc_impl.c:184
  The results in symtab.c are all explained by partial loads through arrays. The result in libproc_impl.c flows through a field of array type in a struct that is allocated in the same function as the sink, and the flow goes through a call to strcpy. That might mean there's precise copies in the IR, but I need to do more digging to be sure.

cpp/uncontrolled-allocation-size

FPs restored as of 3b59118
- gifalloc.c:91
- gifalloc.c:79
  I think these are FPs but only because there's an implicit bound at this bitwise "and". I don't think we need to fix them as part of this PR.

rdmarsh2 · 2020-04-10T23:14:15Z

The cpp/path-injection result at libproc_impl.c:184 in openjdk comes from pointer/object conflation for modeled functions in DefaultTaintTracking.qll.

rdmarsh2 · 2020-04-10T23:22:26Z

I'm now convinced that none of these result changes come from new incorrect flow edges.

@MathiasVP @jbj I think this is mergeable once the QL changes are reviewed.

For submodule consistency

MathiasVP

LGTM!

rdmarsh2 added the C++ label Jan 28, 2020

rdmarsh2 requested review from a team as code owners January 28, 2020 01:12

jbj reviewed Jan 28, 2020

View reviewed changes

rdmarsh2 added 4 commits January 29, 2020 14:37

C++: IR dataflow edges through outparams

1a458aa

C++: fix merge issue

74ea9bc

Merge branch 'master' into rdmarsh/cpp/ir-flow-through-outparams

1472101

C++: add flow through partial loads in DTT

71d87be

Merge branch 'master' into rdmarsh/cpp/ir-flow-through-outparams

677f0f0

rdmarsh2 force-pushed the rdmarsh/cpp/ir-flow-through-outparams branch from 399cb85 to 677f0f0 Compare February 3, 2020 23:15

rdmarsh2 added 10 commits February 6, 2020 11:42

Merge branch 'master' into rdmarsh/cpp/ir-flow-through-outparams

6922074

C++: autoformat and accept test changes

2d3a742

Merge branch 'master' into rdmarsh/cpp/ir-flow-through-outparams

d1d19a7

Update test expectations

C++:autoformat

6809711

C++: accept test changes from extractor update

1d5971f

Merge branch 'master' into rdmarsh/cpp/ir-flow-through-outparams

adfe5f3

C++: Accept test output with IR enabled

ff876aa

Merge branch 'master' into rdmarsh/cpp/ir-flow-through-outparams

4333fe7

Merge branch 'master' into rdmarsh/cpp/ir-flow-through-outparams

bba6b23

C++: accept test changes

ba8ebe9

rdmarsh2 added 2 commits March 12, 2020 15:06

C++: add argv chi chain example to ssa test

a0823a2

Merge branch 'master' into rdmarsh/cpp/ir-flow-through-outparams

9f1833a

MathiasVP mentioned this pull request Apr 5, 2020

C++: IR field flow #3118

Merged

jbj added this to the 1.24 milestone Apr 7, 2020

rdmarsh2 and others added 3 commits April 8, 2020 12:32

Merge branch 'master' into rdmarsh/cpp/ir-flow-through-outparams

c38ccaa

C++: remove partial flow to IR loads

a0b26d6

C++: Don't allow taint out of a field read

9f40886

except if it's from a union. This prevents field conflation through buffers of `UnknownType`.

rdmarsh2 force-pushed the rdmarsh/cpp/ir-flow-through-outparams branch from b09ff54 to 9f40886 Compare April 8, 2020 21:11

rdmarsh2 added 3 commits April 8, 2020 16:30

C++: restore flow for non-class partial reads

b37c13d

This would otherwise have lost a good qltest result at CWE-134/semmle/funcs/funcsLocal.c:58:9:58:10

C++/C#: Document ReturnIndirectionInstruction::getParameter

7e299e7

C++: autoformat

1199ff9

rdmarsh2 added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Apr 8, 2020

C++: remove partial flow from UnknownType

3b59118

alexet changed the base branch from master to rc/1.24 April 9, 2020 16:03

rdmarsh2 added 2 commits April 13, 2020 12:02

Merge branch 'rc/1.24' into rdmarsh/cpp/ir-flow-through-outparams

a5e7db7

For submodule consistency

C++: accept minor test change

8779177

MathiasVP approved these changes Apr 14, 2020

View reviewed changes

MathiasVP merged commit 721e9d5 into github:rc/1.24 Apr 14, 2020

rdmarsh2 mentioned this pull request Apr 14, 2020

Merge rc/1.24 into master. #3264

Merged

C++: IR dataflow edges through outparams #2704

C++: IR dataflow edges through outparams #2704

Uh oh!

Conversation

rdmarsh2 commented Jan 28, 2020

Uh oh!

jbj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jbj Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

jbj commented Jan 31, 2020

Uh oh!

rdmarsh2 commented Mar 10, 2020

Uh oh!

rdmarsh2 commented Mar 10, 2020

Uh oh!

jbj commented Mar 11, 2020

Uh oh!

rdmarsh2 commented Mar 12, 2020

Uh oh!

geoffw0 commented Mar 18, 2020

Uh oh!

jbj commented Mar 18, 2020

Uh oh!

jbj commented Mar 18, 2020

Uh oh!

rdmarsh2 commented Apr 3, 2020

Uh oh!

rdmarsh2 commented Apr 3, 2020

Uh oh!

jbj commented Apr 8, 2020

Uh oh!

jbj commented Apr 8, 2020

Uh oh!

MathiasVP commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

g/openjdk/jdk

cpp/unbounded-write

cpp/path-injection

cpp/uncontrolled-allocation-size

cpp/uncontrolled-process-operation

g/torvalds/linux

cpp/path-injection

Uh oh!

jbj commented Apr 8, 2020

Git

Wireshark

PHP

Uh oh!

rdmarsh2 commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdmarsh2 commented Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MathiasVP commented Apr 10, 2020

Uh oh!

rdmarsh2 commented Apr 10, 2020

Uh oh!

rdmarsh2 commented Apr 10, 2020

g/openjdk/jdk

cpp/unbounded-write

cpp/path-injection

cpp/uncontrolled-allocation-size

Uh oh!

rdmarsh2 commented Apr 10, 2020

Uh oh!

rdmarsh2 commented Apr 10, 2020

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MathiasVP commented Apr 8, 2020 •

edited

Loading

rdmarsh2 commented Apr 8, 2020 •

edited

Loading

rdmarsh2 commented Apr 9, 2020 •

edited

Loading