Go: mass enable diff-informed data flow - #19660
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
Enables diff-informed data flow tracking in Go QL security queries by adding the required predicate to each DataFlow configuration.
- Adds observeDiffInformedIncrementalMode() returning any() to all DataFlow::ConfigSig modules
- Ensures incremental, diff-based analysis is enabled for each relevant security check
Reviewed Changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.
Show a summary per file| File | Description |
|---|---|
| go/ql/src/Security/CWE-352/ConstantOauth2State.ql | Add observeDiffInformedIncrementalMode predicate |
| go/ql/src/Security/CWE-326/InsufficientKeySize.ql | Add observeDiffInformedIncrementalMode predicate |
| go/ql/src/Security/CWE-209/StackTraceExposure.ql | Add observeDiffInformedIncrementalMode predicate |
| go/ql/src/Security/CWE-020/SuspiciousCharacterInRegexp.ql | Add observeDiffInformedIncrementalMode predicate |
| go/ql/src/Security/CWE-020/MissingRegexpAnchor.ql | Add observeDiffInformedIncrementalMode predicate |
| go/ql/src/Security/CWE-020/IncompleteHostnameRegexp.ql | Add observeDiffInformedIncrementalMode predicate |
| go/ql/lib/semmle/go/security/ZipSlip.qll | Add observeDiffInformedIncrementalMode predicate |
| go/ql/lib/semmle/go/security/XPathInjection.qll | Add observeDiffInformedIncrementalMode predicate |
| go/ql/lib/semmle/go/security/UnsafeUnzipSymlink.qll | Add observeDiffInformedIncrementalMode predicate |
| go/ql/lib/semmle/go/security/UncontrolledAllocationSize.qll | Add observeDiffInformedIncrementalMode predicate |
| go/ql/lib/semmle/go/security/TaintedPath.qll | Add observeDiffInformedIncrementalMode predicate |
| go/ql/lib/semmle/go/security/StringBreak.qll | Add observeDiffInformedIncrementalMode predicate |
| go/ql/lib/semmle/go/security/StoredXss.qll | Add observeDiffInformedIncrementalMode predicate |
| go/ql/lib/semmle/go/security/StoredCommand.qll | Add observeDiffInformedIncrementalMode predicate |
| go/ql/lib/semmle/go/security/SqlInjection.qll | Add observeDiffInformedIncrementalMode predicate |
| go/ql/lib/semmle/go/security/OpenUrlRedirect.qll | Add observeDiffInformedIncrementalMode predicate |
| go/ql/lib/semmle/go/security/MissingJwtSignatureCheck.qll | Add observeDiffInformedIncrementalMode predicate |
| go/ql/lib/semmle/go/security/LogInjection.qll | Add observeDiffInformedIncrementalMode predicate |
| go/ql/lib/semmle/go/security/ExternalAPIs.qll | Add observeDiffInformedIncrementalMode predicate |
| go/ql/lib/semmle/go/security/CleartextLogging.qll | Add observeDiffInformedIncrementalMode predicate |
go/ql/src/Security/CWE-352/ConstantOauth2State.ql:44
- No tests were added to verify the new observeDiffInformedIncrementalMode predicate. Include tests to confirm that diff-informed incremental data flow is actually activated.
predicate observeDiffInformedIncrementalMode() { any() }
Sorry, something went wrong.
|
It turns out that some of the generated changes in the PRs were not correct, e.g. because they should have also generated a getASelected{Source,Sink}Location() override but didn't (see Chuan-kai's comment here). So for now I'm putting them back in Draft until I make sure (via the patch script) that we are correctly handling all 3 documented query patterns, starting with the simplest one (both source and sink are used as location sources). If you have already started reviewing the PRs, thank you (also for your patience) and stay tuned for an update as to what has changed in the meantime! |
Sorry, something went wrong.
|
Update: no changes since last time I opened the PR. It turns out that it's sound (but not optimally performant) to leave getASelected{Source,Sink}Location() un-overridden, specifically in case of a select clause containing only one of source or sink but not both. The patch script currently does not differentiate between that case and the one in which both source and sink are present in the select clause. So I will re-open these PRs as they are, and generate an appropriate getASelected{Source,Sink}Location() override in a follow-up round of PRs. |
Sorry, something went wrong.
An auto-generated patch that enables diff-informed data flow in the obvious cases. Builds on github#18345 and github/codeql-patch#88
|
Note, according to the follow-up PR, 5 of these queries (MissingRegexpAnchor.ql, HtmlTemplateEscapingBypassXss.ql, InsufficientKeySize.ql, PamAuthBypass.ql, DivideByZero.ql) have a missing source/sink in their select clauses; the other ones should have both. |
Sorry, something went wrong.
michaelnebel
left a comment
There was a problem hiding this comment.
LGTM!
Sorry, something went wrong.
An auto-generated patch that enables diff-informed data flow in the obvious cases.
Builds on #18345 and https://github.com/github/codeql-patch/pull/88