Go: Make flow configurations use new data flow API #13820

owen-mc · 2023-07-26T09:29:59Z

I highly recommend reviewing commit by commit (and spreading it out over several sessions). All configurations have tests (normally of the related query). There are no changes in results, though sometimes there are changes in exact PathNodes and edges between them.

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

owen-mc · 2023-07-27T06:15:57Z

~~I will rebase on main when #13822 is merged, which will fix the sync files check.~~

mbg

Good work updating all of this! ✨ The refactoring generally looks reasonable. There are a few specific comments I had while looking over this and two more broad ones:

For queries where the expected test output changed and you haven't done so yet, it would be good to note why it changed in the commit description or in the PR description.
Most/all of the old data/taint flow configurations had docs comments explaining their purpose. However, the new ConfigSig implementations and data/taint flow configurations mostly do not have matching docs comments. In general, it would be nice to have more documentation/comments.

There also seem to a good few ConfigSigs which are basically the same as a result of the [..]Customizations approach along the lines of:

private module Config implements DataFlow::ConfigSig {
    predicate isSource(DataFlow::Node source) { source instanceof Source }

    predicate isSink(DataFlow::Node sink) { sink instanceof Sink }

    predicate isBarrier(DataFlow::Node node) { node instanceof Barrier }
}

Would it be possible (in the future, not as part of this PR) to refactor this so that we have a generic implementation that we can just parameterise over the module that's providing the Source, Sink, and Barrier?

mbg · 2023-08-01T06:38:00Z

go/ql/test/query-tests/Security/CWE-190/AllocationSizeOverflow.expected

For queries where the expected test output has changed, it would be good to include some discussion in the PR description why this has changed.

It generally isn't obvious why. I know one cause is when there were more than one config in scope in the old version. I'm not sure how much value there is in tracking it down. I'm going to bet that in the corresponding PRs for java and C# they have similar changes and they haven't looked into what caused them. (Of course if there was a change in the results it would be different. It's just because the only change is in path nodes and edges.)

go/ql/lib/semmle/go/security/AllocationSizeOverflow.qll

mbg · 2023-08-01T07:08:21Z

go/ql/test/library-tests/semmle/go/frameworks/Encoding/jsoniter.expected

How come we get the edges here but didn't before?

mbg · 2023-08-01T07:08:57Z

go/ql/test/library-tests/semmle/go/frameworks/Gorestful/gorestful.expected

How come we get the edges here but didn't before?

mbg · 2023-08-01T07:20:05Z

go/ql/test/query-tests/Security/CWE-918/RequestForgery.expected

Why the differences?

mbg · 2023-08-01T10:36:13Z

go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll

@@ -148,6 +150,115 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
  }
 }

+/** Flow state for ConversionWithoutBoundsCheckConfig. */
+newtype MyFlowState =


Should this have a less generic name, e.g. IntegerConversionState?

mbg · 2023-08-01T10:44:09Z

go/ql/test/library-tests/semmle/go/frameworks/Spew/TaintFlows.ql

+    source =
+      any(Function f | f.getName() = ["getUntrustedString", "getUntrustedStruct"])
+          .getACall()
+          .getResult()


I note that the test previously ranged over all of UntrustedFlowSource but now is an inlined UntrustedSource. Do we miss out anything important by not having this range over all of UntrustedFlowSource anymore?

I don't think so - we're just trying to match the two functions used as sources in the test files. I can only imagine was written like this to be more like a real source definition.

mbg · 2023-08-01T10:46:41Z

go/ql/test/library-tests/semmle/go/frameworks/Protobuf/TaintFlows.ql

+    exists(Function fn | fn.hasQualifiedName(_, ["getUntrustedString", "getUntrustedBytes"]) |
+      source = fn.getACall().getResult()
+    )


This previously ranged over all of UntrustedFlowSource, but is now more limited. Do we miss out on anything as a result of this change?

As above, I don't think it matters for tests.

mbg · 2023-08-01T10:48:54Z

go/ql/test/library-tests/semmle/go/frameworks/K8sIoApiCoreV1/main.go

@@ -9,86 +9,88 @@ import (
 //go:generate depstubber -vendor k8s.io/apimachinery/pkg/runtime ProtobufMarshaller,ProtobufReverseMarshaller

 func source() interface{} {
-	return make([]byte, 1, 1)
+	return make([]byte, 1)


What's the reason for this change?

My IDE told me it was better to do it this way. Presumably it's a built-in Go linter of some kind.

mbg · 2023-08-01T10:50:46Z

go/ql/test/library-tests/semmle/go/frameworks/EvanphxJsonPatch/TaintFlows.ql

    )
  }
+
+  int fieldFlowBranchLimit() { result = 1000 }


Could you add a comment noting why you added this here for this particular configuration?

It isn't needed. I've removed it. I must have added it by mistake, probably copying from somewhere else.

owen-mc · 2023-08-07T11:10:41Z

I believe I've addressed all comments except explaining the changes in test expectations.

owen-mc · 2023-08-09T21:06:58Z

I've added explanations of test result changes in the relevant commit messages. I had to force-push to do this, so I rebased on main at the same time. There were no changes to the commits except the commit messages.

VS Code's JSON formatter removed it automatically. It turns out that the easiest way to keep it is to use the `files.insertFinalNewline` setting, which the JSON formatter obeys.

We preserve all old QLDocs, but move them from the config to the Flow module. This makes more sense than the Config module, which is often private, and is generally not directly accessed.

Co-authored-by: Michael B. Gale <[email protected]>

mbg

🚢

owen-mc requested a review from a team as a code owner July 26, 2023 09:30

github-actions bot added the Go label Jul 26, 2023

owen-mc force-pushed the go/refactor-data-flow-configurations branch from 85baa81 to 1d2c268 Compare July 26, 2023 09:34

github-advanced-security bot found potential problems Jul 26, 2023

View reviewed changes

go/ql/test/library-tests/semmle/go/dataflow/GuardingFunctions/test.ql Fixed Show fixed Hide fixed

owen-mc added the no-change-note-required This PR does not need a change note label Jul 26, 2023

owen-mc force-pushed the go/refactor-data-flow-configurations branch from 1d2c268 to 028eb9b Compare July 26, 2023 09:51

owen-mc marked this pull request as draft July 26, 2023 10:25

owen-mc force-pushed the go/refactor-data-flow-configurations branch from 028eb9b to 9919221 Compare July 26, 2023 21:18

github-actions bot added C# C++ Java Python Ruby Swift labels Jul 26, 2023

owen-mc force-pushed the go/refactor-data-flow-configurations branch from 9919221 to 355f9c6 Compare July 27, 2023 06:13

github-actions bot removed C# C++ Java Python Ruby Swift labels Jul 27, 2023

owen-mc marked this pull request as ready for review July 27, 2023 06:14

owen-mc mentioned this pull request Aug 1, 2023

Go: Refactor to use new module API for DataFlow configurations #12835

Closed

owen-mc force-pushed the go/refactor-data-flow-configurations branch 2 times, most recently from d8ed7fe to 9376ead Compare August 1, 2023 07:04

mbg reviewed Aug 1, 2023

View reviewed changes

owen-mc force-pushed the go/refactor-data-flow-configurations branch from 825b716 to 9612740 Compare August 9, 2023 21:05

owen-mc added 20 commits August 10, 2023 15:49

Make SensitiveConditionBypass use new API

9b19cde

Make CorsMisconfiguration use new API

b00e447

Make InsecureTLS use new API

442dfc1

Make BadRedirectCheck use new API

6c0c8d6

Make IncorrectIntegerConversion use new API

8db3e4a

Make taint tracking tests use config from InlineFlowTest

663fb2c

Make taint tracking tests use InlineFlowTest

c11da5b

Make taint tracking tests use new API

b6b7e15

Note deprecation in QLDoc for LogInjection

81d4149

Remove unnecessary import

046e517

Keep newline at the end of identical-files.json

0399251

VS Code's JSON formatter removed it automatically. It turns out that the easiest way to keep it is to use the `files.insertFinalNewline` setting, which the JSON formatter obeys.

Revert edit to deprecated class

6b4bf12

Use more descriptive names for merged path graphs

e6c8a0b

Do not make unnecessary changes

e33d303

Improve QLDoc for Config::FlowState in StringBreak

089ea01

Remove output from running query like a test

2578ef4

Update for recent changes to DsnInjection

36b1a0d

Give MyFlowstate a less generic name

0928fa6

Remove unnecessary fieldFlowBranchLimit

94c15f7

Improve inaccurate deprecation comments

08e1e8a

owen-mc force-pushed the go/refactor-data-flow-configurations branch 2 times, most recently from 0bd3ce8 to 19dbe79 Compare August 11, 2023 08:54

owen-mc and others added 2 commits August 11, 2023 10:06

Put QLDoc on data flow and taint tracking modules

b7dfa23

We preserve all old QLDocs, but move them from the config to the Flow module. This makes more sense than the Config module, which is often private, and is generally not directly accessed.

Apply suggestions from code review

35a300f

Co-authored-by: Michael B. Gale <[email protected]>

owen-mc force-pushed the go/refactor-data-flow-configurations branch from 19dbe79 to 35a300f Compare August 11, 2023 09:06

mbg approved these changes Aug 11, 2023

View reviewed changes

owen-mc merged commit c10d03e into github:main Aug 11, 2023
10 checks passed

owen-mc deleted the go/refactor-data-flow-configurations branch August 11, 2023 09:49

RasmusWL mentioned this pull request Aug 28, 2023

Python: Use new dataflow API #14068

Merged

owen-mc mentioned this pull request Nov 10, 2023

Go: Add Improper LDAP Authentication query (CWE-287) #13366

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go: Make flow configurations use new data flow API #13820

Go: Make flow configurations use new data flow API #13820

owen-mc commented Jul 26, 2023 •

edited

Loading

owen-mc commented Jul 27, 2023 •

edited

Loading

mbg left a comment

mbg Aug 1, 2023

owen-mc Aug 1, 2023

mbg Aug 1, 2023

mbg Aug 1, 2023

mbg Aug 1, 2023

mbg Aug 1, 2023

mbg Aug 1, 2023

owen-mc Aug 2, 2023

mbg Aug 1, 2023

owen-mc Aug 2, 2023

mbg Aug 1, 2023

owen-mc Aug 1, 2023

mbg Aug 1, 2023

owen-mc Aug 2, 2023

owen-mc commented Aug 7, 2023

owen-mc commented Aug 9, 2023

mbg left a comment

Go: Make flow configurations use new data flow API #13820

Go: Make flow configurations use new data flow API #13820

Conversation

owen-mc commented Jul 26, 2023 • edited Loading

owen-mc commented Jul 27, 2023 • edited Loading

mbg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owen-mc commented Aug 7, 2023

owen-mc commented Aug 9, 2023

mbg left a comment

Choose a reason for hiding this comment

owen-mc commented Jul 26, 2023 •

edited

Loading

owen-mc commented Jul 27, 2023 •

edited

Loading