Diff-informed queries: phase 3 (non-trivial locations) by d10c · Pull Request #19957 · github/codeql · GitHub

This PR enables diff-informed mode on queries that select a location other than dataflow source or sink. This entails adding a non-trivial location override that returns the locations that are actually selected.

Prior work includes PRs like #19663, #19759, and #19817. This PR uses the same patch script as those PRs to find candidate queries to convert to diff-enabled. This is the final step in mass-enabling diff-informed queries on all the languages.

Commit-by-commit reviewing is recommended.

I have split the commits that add/modify tests from the ones that enable/disable diff-informed queries.
If the commit modifies a .qll file, in the commit message I've included links to the queries that depend on that .qll for easier reviewing.
Feel free to delegate parts of the review to others who may be more specialized in certain languages.

Potentially tricky cases:

[TEST] C++: CWE-020/ExternalAPI: add tests based on qlhelp: I wasn't able to get alerts to show up for the qhelp-based tests; can someone from the language team chip in to what a useful test would look like? Similar case with the Python TimingAttackAgainstHash test. In contrast, in [TEST] Java: CWE-020/ExternalAPI I was able to get alerts just from the qhelp example, and in [TEST] C++: CleartextSqliteDatabase I was able to get alerts from a mostly Copilot-written test.
[DIFF-INFORMED] C++: (IR) ExternalAPIs: config used in two queries, but one of them without a location; I guess that's fine?
[DIFF-INFORMED] C++: SSLResultConflation: query uses a secondary config (i.e. calls the same config twice), but this does pass tests, unlike the other queries with secondary configs. Can I get a sanity check here? Let me know if you find other cases of secondary configs that are passing tests.
[DIFF-INFORMED] Java: LogInjection: disabled because of mysterious OOM during --check-diff-informed locally and in CI. Should create a follow-up issue.
I'm not sure how important it is to have the location overrides be as precise as possible by including all the clauses from the where, or if it's good enough to overapproximate and just pass the --check-diff-informed tests.

DCA results: some slowdowns on Python, but they don't seem to be related to these changes. Java had some timeout-related failures, so I'll restart that. Overall, no negative performance impact on empty-diff.

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/java/ql/src/Security/CWE/CWE-089/SqlConcatenated.ql#L27

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/java/ql/src/Security/CWE/CWE-078/ExecTaintedEnvironment.ql#L22

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql#L56

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/java/ql/src/Security/CWE/CWE-501/TrustBoundaryViolation.ql#L18

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql#L21

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/javascript/ql/src/Security/CWE-078/IndirectCommandInjection.ql#L25

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/javascript/ql/src/Security/CWE-078/ShellCommandInjectionFromEnvironment.ql#L26

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/javascript/ql/src/experimental/Security/CWE-099/EnvValueAndKeyInjection.ql#L68

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/ruby/ql/src/queries/security/cwe-020/MissingFullAnchor.ql#L18

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/swift/ql/src/queries/Security/CWE-311/CleartextStorageDatabase.ql#L33

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/swift/ql/src/queries/Security/CWE-312/CleartextStoragePreferences.ql#L32

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/swift/ql/src/queries/Security/CWE-757/InsecureTLS.ql#L18

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/swift/ql/src/queries/Security/CWE-079/UnsafeWebViewFetch.ql#L24

This is a very large diff for a single PR. Would it be a lot of extra work to split this into one PR per language? Then it will also be possible to merge a bit at the time (there might be issues with getting DCA running successfully for this many languages and it will be a shame to block merging changes while waiting for DCA/review for other languages).

Sure, I can split it off into separate PRs per language.

Sure, I can split it off into separate PRs per language.

Thank you - that will be really appreciated. You also structured the commits very nicely, so it seems doable 😄

github-actions Bot added C# JS C++ Java Python Go Ruby Rust Pull requests that update Rust code Swift Actions Analysis of GitHub Actions labels Jul 2, 2025

github-advanced-security AI found potential problems Jul 2, 2025

View reviewed changes

d10c force-pushed the d10c/diff-informed-phase-3 branch from ff3a4b9 to 95fe462 Compare July 3, 2025 15:50

github-advanced-security AI found potential problems Jul 3, 2025

View reviewed changes

Comment thread actions/ql/lib/codeql/actions/security/EnvPathInjectionQuery.qll Fixed

d10c force-pushed the d10c/diff-informed-phase-3 branch 3 times, most recently from aff62c2 to 6d0ae3a Compare July 4, 2025 14:20

github-advanced-security AI found potential problems Jul 4, 2025

View reviewed changes

Comment thread ...erimental/query-tests/Security/CWE-208/TimingAttackAgainstHash/TimingAttackAgainstHash.qlref Dismissed

d10c force-pushed the d10c/diff-informed-phase-3 branch 2 times, most recently from c871f5e to 276c7f0 Compare July 7, 2025 09:42

github-advanced-security AI found potential problems Jul 7, 2025

View reviewed changes

Comment thread go/ql/lib/semmle/go/security/HardcodedCredentials.qll Fixed

Comment thread go/ql/lib/semmle/go/security/HardcodedCredentials.qll Fixed

d10c force-pushed the d10c/diff-informed-phase-3 branch 2 times, most recently from cb2db2f to c70036d Compare July 8, 2025 15:30

github-advanced-security AI found potential problems Jul 8, 2025

View reviewed changes

Comment thread java/ql/lib/semmle/code/java/security/SqlConcatenatedQuery.qll Fixed

Comment thread java/ql/lib/semmle/code/java/security/TempDirLocalInformationDisclosureQuery.qll Fixed

Comment thread java/ql/lib/semmle/code/java/security/UnsafeCertTrustQuery.qll Fixed

d10c force-pushed the d10c/diff-informed-phase-3 branch from c70036d to 08c4cc2 Compare July 9, 2025 16:47

github-advanced-security AI found potential problems Jul 9, 2025

View reviewed changes

Comment thread cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql Fixed

d10c force-pushed the d10c/diff-informed-phase-3 branch 2 times, most recently from 857b583 to 3e3e856 Compare July 11, 2025 12:49

d10c force-pushed the d10c/diff-informed-phase-3 branch 2 times, most recently from 443655d to 5112a9c Compare July 16, 2025 15:36

d10c added 24 commits July 17, 2025 10:56

[DIFF-INFORMED] Java: SqlConcatenated

5228767

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/java/ql/src/Security/CWE/CWE-089/SqlConcatenated.ql#L27

[DIFF-INFORMED] Java: TaintedEnvironmentVariable

3197508

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/java/ql/src/Security/CWE/CWE-078/ExecTaintedEnvironment.ql#L22

[DIFF-INFORMED] Java: TempDirLocalInformationDisclosure

69e93eb

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql#L56

[DIFF-INFORMED] Java: TrustBoundaryViolation

f10210c

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/java/ql/src/Security/CWE/CWE-501/TrustBoundaryViolation.ql#L18

[DIFF-INFORMED] Java: UnsafeCertTrust

5e05aeb

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/java/ql/src/Security/CWE/CWE-273/UnsafeCertTrust.ql#L21

[DIFF-INFORMED] Java: AndroidWebViewSettingsAllowsContentAccess

06add01

[DIFF-INFORMED] JS: IndirectCommandInjection

89dd8a8

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/javascript/ql/src/Security/CWE-078/IndirectCommandInjection.ql#L25

[DIFF-INFORMED] JS: ShellCommandInjectionFromEnvironment

2a5eae2

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/javascript/ql/src/Security/CWE-078/ShellCommandInjectionFromEnvironment.ql#L26

[DIFF-INFORMED] JS: EnvValueAndKeyInjection

81bbcda

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/javascript/ql/src/experimental/Security/CWE-099/EnvValueAndKeyInjection.ql#L68

[DIFF-INFORMED] JS: decodeJwtWithoutVerification

272a97d

[DIFF-INFORMED] Python: (Possible)TimingAttackAgainstHash

e7b6ad2

[DIFF-INFORMED] Ruby: MissingFullAnchor

9f72a2f

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/ruby/ql/src/queries/security/cwe-020/MissingFullAnchor.ql#L18

[DIFF-INFORMED] Rust: RegexInjection

2ba8234

[DIFF-INFORMED] Rust: TaintedPath

2afffd3

[DIFF-INFORMED] Rust: SqlInjection

7e4f37f

[DIFF-INFORMED] Rust: CleartextTransmission

b3f76ce

[DIFF-INFORMED] Rust: CleartextLogging

d25c8cd

[DIFF-INFORMED] Rust: UncontrolledAllocationSize

ca1848e

[DIFF-INFORMED] Rust: AccessAfterLifetime

ab6be38

[DIFF-INFORMED] Rust: AccessInvalidPointer

a0215ae

[DIFF-INFORMED] Swift: CleartextStorageDatabase

162781d

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/swift/ql/src/queries/Security/CWE-311/CleartextStorageDatabase.ql#L33

[DIFF-INFORMED] Swift: CleartextStoragePreferences

37e6e2c

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/swift/ql/src/queries/Security/CWE-312/CleartextStoragePreferences.ql#L32

[DIFF-INFORMED] Swift: InsecureTLS

649df5b

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/swift/ql/src/queries/Security/CWE-757/InsecureTLS.ql#L18

[DIFF-INFORMED] Swift: UnsafeWebViewFetch

085720a

https://github.com/d10c/codeql/blob/d10c/diff-informed-phase-3/swift/ql/src/queries/Security/CWE-079/UnsafeWebViewFetch.ql#L24

d10c force-pushed the d10c/diff-informed-phase-3 branch from 5112a9c to 085720a Compare July 17, 2025 08:57

d10c mentioned this pull request Jul 21, 2025

C#: Diff-informed queries: phase 3 (non-trivial locations) #20074

Merged

d10c closed this Nov 10, 2025

Diff-informed queries: phase 3 (non-trivial locations) by d10c · Pull Request #19957 · github/codeql · GitHub

Uh oh!

Diff-informed queries: phase 3 (non-trivial locations) - #19957

Diff-informed queries: phase 3 (non-trivial locations)#19957
d10c wants to merge 96 commits into
github:mainfrom
d10c:d10c/diff-informed-phase-3

d10c commented Jul 2, 2025 •
edited
Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

d10c commented Jul 15, 2025

Uh oh!

michaelnebel commented Jul 17, 2025

Uh oh!

d10c commented Jul 17, 2025

Uh oh!

michaelnebel commented Jul 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Diff-informed queries: phase 3 (non-trivial locations) by d10c · Pull Request #19957 · github/codeql · GitHub

Uh oh!

Diff-informed queries: phase 3 (non-trivial locations) - #19957

Diff-informed queries: phase 3 (non-trivial locations)#19957d10c wants to merge 96 commits intogithub:maingithub/codeql:mainfrom d10c:d10c/diff-informed-phase-3d10c/codeql:d10c/diff-informed-phase-3Copy head branch name to clipboard

Conversation

d10c commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

d10c commented Jul 15, 2025

Uh oh!

michaelnebel commented Jul 17, 2025

Uh oh!

d10c commented Jul 17, 2025

Uh oh!

michaelnebel commented Jul 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Diff-informed queries: phase 3 (non-trivial locations)#19957
d10c wants to merge 96 commits into
github:mainfrom
d10c:d10c/diff-informed-phase-3

d10c commented Jul 2, 2025 •
edited
Loading