Update dataflow libraries and add support for CSV summary flow #574

sauyon · 2021-09-21T05:53:22Z

Currently I'm bodging SourceOrSinkNode by using AstNode because we don't have a class that includes both a call and a callee. (Other than data-flow nodes, which can't be used because it causes non-monotonic recursion.)

Possibly the easiest solution would be to add a Top class, but alternatively we could extract decls for all functions, or split data-flow nodes into to layers.

To resolve before merging:

Decide what to do with clearsContent
Revert type changes to getACallee (possible implementation available at Update dataflow libraries and add support for CSV summary flow #574 (comment))
Use CallNode.getRoot rather than .asExpr().getEnclosingCallable()
Investigate why Propagate data flow through receivers #34 (Update dataflow libraries and add support for CSV summary flow #574 (comment)) is reverted and whether this can be avoided
Diagnose missing sink bug at Update dataflow libraries and add support for CSV summary flow #574 (comment)
Run a DCA and dist-build to check for unpleasant side-effects
Give DataFlowCallable a user-facing name, since it is exposed to them in getACalleeIncludingExternals()

sauyon · 2021-09-21T05:53:49Z

(Needs testing, but I don't think this is testable without making one of those changes.)

smowton · 2021-09-22T11:02:45Z

Confirmed with some copying, pasting and diffing that 90f96e4 is only moving code, not making any substantial change.

ql/src/semmle/go/dataflow/FlowSummary.qll

ql/src/semmle/go/dataflow/internal/DataFlowPrivate.qll

smowton · 2021-09-22T13:58:28Z

There are also test failures to look at

smowton · 2021-09-22T13:59:24Z

I'd suggest we should add some actual models with this btw so we're not committing unused code -- perhaps port a small library that we already model by other means?

ql/src/semmle/go/dataflow/ExternalFlow.qll

smowton

To talk about at standup: can we make SourceOrSinkElement a newtype?

smowton · 2021-09-24T09:45:38Z

ql/src/semmle/go/dataflow/internal/FlowSummaryImplSpecific.qll

+class SourceOrSinkElement extends TSourceOrSinkElement {
+  Entity asEntity() { this = TEntityElement(result) }
+
+  AstNode asAstNode() { this = TAstElement(result) }


Looks like the only current user casts this straight to DataFlowCall? Could we use that instead of AstNode?

sauyon · 2021-09-28T08:19:34Z

This is currently still not in a working state, but I'm pretty stuck now; Steps::summaryLocalStep includes the step from the parameter to the summary return node, but partial flow seems to stop at the argument node. I believe that should just flow directly into the parameter node now, (the pair I'm looking for does show up in viableParamArg) but either that's not the case or it's being hidden somehow.

smowton

Looking into this now...

ql/src/semmle/go/dataflow/internal/DataFlowImplCommon.qll

smowton · 2021-09-28T13:54:26Z

@sauyon have pushed two fix commits that get the basic test passing:

The step into the callable needed to use Node.getEnclosingCallable to associate the parameter node with its callee, but that wasn't implemented for summary parameter nodes
The step out of the callable needed ReturnNode to include those defined inside callables

I haven't looked into within-summary read and store steps or callables with parameter side-effects yet, but this is at least enough to get the most basic test passing.

owen-mc · 2021-10-14T05:46:34Z

@smowton Do you know what the status of this is? Does it just need to be rebased, or are changes needed?

smowton · 2021-10-14T08:07:21Z

@sauyon could you give an update?

sauyon · 2021-10-14T15:04:15Z

I believe I was waiting on a review for the parts that do work (csv modeling).

Content flow isn't currently working. I was intending to get to that but I am currently at KubeCon so that may be a bit delayed.

owen-mc · 2021-11-09T06:33:00Z

It seems that it supports "ReturnValue", "ReturnValue[n]" and "ReturnValue[n1..n2]". I assume "ReturnValue" is a synonym for "ReturnValue[0]". I guess that makes sense, since most of the time functions return a (result, err) pair and taint would normally flow through the first of those. I was just thinking of FunctionInput::functionResult(), where it only works when there is only one return value, so you're forced to use FunctionInput::functionResult(0) if that's what you mean.

sauyon · 2021-11-24T11:10:12Z

It seems that it supports "ReturnValue", "ReturnValue[n]" and "ReturnValue[n1..n2]". I assume "ReturnValue" is a synonym for "ReturnValue[0]". I guess that makes sense, since most of the time functions return a (result, err) pair and taint would normally flow through the first of those. I was just thinking of FunctionInput::functionResult(), where it only works when there is only one return value, so you're forced to use FunctionInput::functionResult(0) if that's what you mean.

I forgot to address this; I think this is a good idea, but I think it was easier to make it work this way. I'll take a quick look after the variadic stuff and if it's easy I'll put it in, otherwise it's probably worth just doing later.

owen-mc · 2021-11-29T16:05:59Z

On further thought I think we should leave ReturnValue as a synonym for ReturnValue[0].

I've added some tests. I think this is ready to be re-reviewed now.

ql/lib/semmle/go/dataflow/ExternalFlow.qll

ql/lib/semmle/go/dataflow/FlowSummary.qll

ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll

smowton · 2021-12-01T12:20:52Z

ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll

-        or
-        exists(c.(MethodCallNode).getTarget().getBody())
-      )
+      pos = i


#34 suggests that commit "Track receiver arguments to interface calls" may have performance or accuracy consequences. To consider, can we get what we need without reverting the desired effects of that PR?

ql/test/library-tests/semmle/go/dataflow/ExternalFlow/test.go

ql/test/library-tests/semmle/go/dataflow/ExternalFlow/srcs.expected

smowton · 2021-12-01T12:31:25Z

ql/test/library-tests/semmle/go/dataflow/ExternalFlow/test.go

+	var b test.B
+
+	b.Sink1(arg)
+	b.SinkMethod().(io.Writer).Write(arg.([]byte))


No sink found on this line?

@sauyon and I had a quick look and this does seem to be a bug. He'll try and dig into it some more.

smowton · 2021-12-02T14:59:35Z

Note this PR currently contains a test, ExternalFlowVarArgs, which only passes in CI because CI tests the merge-product with main. It doesn't work on this branch.

This reverts commit b4742cc.

The documentation in ExternalFlow.qll does not specify that "ReturnValue" can be used as the input column.

The location of the dataflow libraries in codeql-go has changed and there is a new file to be synced.

# Conflicts: # ql/test/library-tests/semmle/go/dataflow/ExternalFlow/srcs.expected

PointerContent needs to have the PointerType specified as well

I removed asFunctionNode() because it would need an import, but it doesn't seem to be used anywhere.

This branch originally included a commit to enable flow through receivers when there is no function body. This was dropped, to be pursued later.

viableParamArg should be evaluated first.

…ith Configurations This is particularly important for ConversionWithoutBoundsCheckConfig which has 20 configs. By paring DataFlow::Node down to only those that have a local-flow successor, or only those with an isAdditionalFlowStep for some related configuration, the result size can be significantly reduced prior to taking the product against Configuration and finally paring down using config.fullBarrier etc. Saves about 1m20s per analysis on cockroachdb.

sauyon requested a review from a team as a code owner September 21, 2021 05:53

smowton reviewed Sep 22, 2021

View reviewed changes

ql/src/semmle/go/dataflow/FlowSummary.qll Show resolved Hide resolved

ql/src/semmle/go/dataflow/internal/DataFlowPrivate.qll Outdated Show resolved Hide resolved

smowton reviewed Sep 22, 2021

View reviewed changes

ql/src/semmle/go/dataflow/ExternalFlow.qll Outdated Show resolved Hide resolved

smowton reviewed Sep 22, 2021

View reviewed changes

sauyon force-pushed the dataflow-update branch from 1a0884a to 937df2e Compare September 22, 2021 16:20

sauyon added the no-change-note-required label Sep 22, 2021

sauyon force-pushed the dataflow-update branch from ef9ade2 to 71fdb1c Compare September 23, 2021 17:07

smowton reviewed Sep 24, 2021

View reviewed changes

smowton reviewed Sep 28, 2021

View reviewed changes

ql/src/semmle/go/dataflow/internal/DataFlowImplCommon.qll Show resolved Hide resolved

sauyon force-pushed the dataflow-update branch from b9de8e9 to 260b197 Compare September 29, 2021 15:13

sauyon force-pushed the dataflow-update branch 3 times, most recently from d448b2f to 4251f31 Compare October 8, 2021 11:34

sauyon force-pushed the dataflow-update branch from 4251f31 to 31fd94c Compare November 22, 2021 09:19

owen-mc force-pushed the dataflow-update branch 2 times, most recently from d11bd77 to d0c60dd Compare November 29, 2021 16:04

smowton reviewed Dec 1, 2021

View reviewed changes

owen-mc and others added 19 commits December 8, 2021 11:20

Do not allow "Argument" on its own

d717734

Move built-in models to ExternalFlow

9f763dd

Move type assertion into declared type

adf3dc0

Comment on why summaryDataFlowCall is none()

2d8fd71

Test models of flow through fields

e940a53

Diasble clearing content and add test for it

5ec0b09

Revert "Make getACallee return DataFlowCallable"

6110506

This reverts commit b4742cc.

Revert getACallee type change

3cf1459

Another instance of getEnclosingFunction -> getRoot

63b944a

Do not test ReturnValue as input for sink

16fdb9a

The documentation in ExternalFlow.qll does not specify that "ReturnValue" can be used as the input column.

Update sync-dataflow-libraries target in Makefile

1a299d2

The location of the dataflow libraries in codeql-go has changed and there is a new file to be synced.

Sync dataflow libraries again

01bfbde

Keep call to defaultTaintSanitizerGuard

578a31e

Allow for Return[i] specifications

b2f62b1

Do not allow "Argument" on its own

095fe6e

# Conflicts: # ql/test/library-tests/semmle/go/dataflow/ExternalFlow/srcs.expected

Update non-shared dataflow files to match sync

1a9ea38

Fix bad join order in BarrierGuard.guards/2

d703072

Fix accidental cartesian product

754c838

PointerContent needs to have the PointerType specified as well

Allow implicit taint reads through more content types

a6532b9

owen-mc force-pushed the dataflow-update branch from 1da7c1f to 537e4f4 Compare December 8, 2021 16:24

Give DataFlowCallable a user-facing name (Callable), move to Scopes.qll

a01f90b

I removed asFunctionNode() because it would need an import, but it doesn't seem to be used anywhere.

owen-mc force-pushed the dataflow-update branch from 537e4f4 to a01f90b Compare December 8, 2021 16:32

owen-mc and others added 4 commits December 8, 2021 15:28

Update expected test results with extra nodes

88e7c44

Update tests for no flow through receivers when no function body

06f889f

This branch originally included a commit to enable flow through receivers when there is no function body. This was dropped, to be pursued later.

Fix bad join order in getAFalsifiedGuard

b234ba7

viableParamArg should be evaluated first.

smowton approved these changes Dec 13, 2021

View reviewed changes

smowton merged commit 9309abf into github:main Dec 13, 2021

owen-mc mentioned this pull request Jan 11, 2022

Revert Function.getACall behaviour #660

Closed

owen-mc mentioned this pull request Jan 20, 2022

Refactor ReturnValue[n] in data flow libraries #673

Merged

Update dataflow libraries and add support for CSV summary flow #574

Update dataflow libraries and add support for CSV summary flow #574

Uh oh!

Conversation

sauyon commented Sep 21, 2021 • edited by smowton Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sauyon commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smowton commented Sep 22, 2021

Uh oh!

Uh oh!

Uh oh!

smowton commented Sep 22, 2021

Uh oh!

smowton commented Sep 22, 2021

Uh oh!

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

smowton Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

sauyon commented Sep 28, 2021

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smowton commented Sep 28, 2021

Uh oh!

owen-mc commented Oct 14, 2021

Uh oh!

smowton commented Oct 14, 2021

Uh oh!

sauyon commented Oct 14, 2021

Uh oh!

owen-mc commented Nov 9, 2021

Uh oh!

sauyon commented Nov 24, 2021

Uh oh!

owen-mc commented Nov 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smowton Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

smowton Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

owen-mc Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

smowton commented Dec 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sauyon commented Sep 21, 2021 •

edited by smowton

Loading

sauyon commented Sep 21, 2021 •

edited

Loading