Skip to content

Conversation

@mschwager
Copy link
Member

Most queries updated without problems. However, go/src/security/MissingMinVersionTLS/MissingMinVersionTLS.ql took some consideration.

First, the NamedType updates fixed this warning:

+WARNING: type 'NamedType' has been deprecated and may be removed in future (MissingMinVersionTLS.ql:90,8-17)
+WARNING: type 'NamedType' has been deprecated and may be removed in future (MissingMinVersionTLS.ql:96,8-17)

Second, the sink.(DataFlow::PostUpdateNode).getPreUpdateNode() changes were due to this breaking change in the codeql/go-all library: https://github.com/github/codeql/blob/codeql-cli/latest/go/ql/lib/CHANGELOG.md#500.

Specifically:

The shape of the Go data-flow graph has changed. Previously for code like x := def(); use1(x); use2(x), there would be edges from the definition of x to each use. Now there is an edge from the definition to the first use, then another from the first use to the second, and so on. This means that data-flow barriers work differently - flow will not reach any uses after the barrier node. Where this is not desired it may be necessary to add an additional flow step to propagate the flow forward. Additionally, when a variable may be subject to a side-effect, such as updating an array, passing a pointer to a function that might write through it or writing to a field of a struct, there is now a dedicated post-update node representing the variable after this side-effect has taken place. Previously post-update nodes were aliases for either a variable's definition, or were equal to the pre-update node. This led to backwards steps in the data-flow graph, which could cause false positives. For example, in the previous code there would be an edge from x in use2(x) back to the definition of x. If we define our sources as any argument of use2 and our sinks as any argument of use1 then this would lead to a false positive path. Now there are distinct post-update nodes and no backwards edge to the definition, so we will not find this false positive path.

Shoutout to @owen-mc for helping me out here.

TlsConfigCreationFlow::flow(source, sink) and
sink.asExpr() = v.getAReference() and
(
sink.asExpr() = v.getAReference() or
Copy link
Member Author

Choose a reason for hiding this comment

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

It's challenging that this logic is duplicated in two places. That also made this fix especially hard to track down because the Variable here has to stay in sync with the Variable in isSink 😕.

I can think of a number of improvements here, but I'll save those for a separate PR and another day. Particularly, I would think that this query should be a path-problem query and all the conditions in the where clause should instead be located in the dataflow ConfigSig 🤷‍♂️. Using characteristic predicates for the source/StructLit and sink/Variable may also clear things up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants