diff --git a/MODULE.bazel b/MODULE.bazel index d5c2c8784e26..16b4a4691f8a 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -27,7 +27,7 @@ bazel_dep(name = "abseil-cpp", version = "20260107.1", repo_name = "absl") bazel_dep(name = "nlohmann_json", version = "3.11.3", repo_name = "json") bazel_dep(name = "fmt", version = "12.1.0-codeql.1") bazel_dep(name = "rules_kotlin", version = "2.2.2-codeql.1") -bazel_dep(name = "gazelle", version = "0.47.0") +bazel_dep(name = "gazelle", version = "0.50.0") bazel_dep(name = "rules_dotnet", version = "0.21.5-codeql.1") bazel_dep(name = "googletest", version = "1.17.0.bcr.2") bazel_dep(name = "rules_rust", version = "0.69.0") diff --git a/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql b/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql index a8bd8a5f93dc..00f601fd5daf 100644 --- a/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql +++ b/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql @@ -26,10 +26,23 @@ string permissionsForJob(Job job) { "{" + concat(string permission | permission = jobNeedsPermission(job) | permission, ", ") + "}" } +predicate jobHasPermissions(Job job) { + exists(job.getPermissions()) + or + exists(job.getEnclosingWorkflow().getPermissions()) + or + // The workflow is reusable and cannot be triggered in any other way; check callers + exists(ReusableWorkflow r | r = job.getEnclosingWorkflow() | + not exists(Event e | e = r.getOn().getAnEvent() | e.getName() != "workflow_call") and + forall(Job caller | caller = job.getEnclosingWorkflow().(ReusableWorkflow).getACaller() | + jobHasPermissions(caller) + ) + ) +} + from Job job, string permissions where - not exists(job.getPermissions()) and - not exists(job.getEnclosingWorkflow().getPermissions()) and + not jobHasPermissions(job) and // exists a trigger event that is not a workflow_call exists(Event e | e = job.getATriggerEvent() and diff --git a/actions/ql/src/Security/CWE-829/ArtifactPoisoningCritical.ql b/actions/ql/src/Security/CWE-829/ArtifactPoisoningCritical.ql index 24ecb4b03397..be49de830c33 100644 --- a/actions/ql/src/Security/CWE-829/ArtifactPoisoningCritical.ql +++ b/actions/ql/src/Security/CWE-829/ArtifactPoisoningCritical.ql @@ -20,6 +20,6 @@ from ArtifactPoisoningFlow::PathNode source, ArtifactPoisoningFlow::PathNode sin where ArtifactPoisoningFlow::flowPath(source, sink) and event = getRelevantEventInPrivilegedContext(sink.getNode()) -select sink.getNode(), source, sink, - "Potential artifact poisoning in $@, which may be controlled by an external user ($@).", sink, - sink.getNode().toString(), event, event.getName() +select source.getNode(), source, sink, + "Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@).", + event, event.getName() diff --git a/actions/ql/src/Security/CWE-829/ArtifactPoisoningMedium.ql b/actions/ql/src/Security/CWE-829/ArtifactPoisoningMedium.ql index d2aff7da95ff..49dc856e5665 100644 --- a/actions/ql/src/Security/CWE-829/ArtifactPoisoningMedium.ql +++ b/actions/ql/src/Security/CWE-829/ArtifactPoisoningMedium.ql @@ -20,6 +20,5 @@ from ArtifactPoisoningFlow::PathNode source, ArtifactPoisoningFlow::PathNode sin where ArtifactPoisoningFlow::flowPath(source, sink) and inNonPrivilegedContext(sink.getNode().asExpr()) -select sink.getNode(), source, sink, - "Potential artifact poisoning in $@, which may be controlled by an external user.", sink, - sink.getNode().toString() +select source.getNode(), source, sink, + "Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user." diff --git a/actions/ql/src/change-notes/2026-04-02-alert-msg-poisoning.md b/actions/ql/src/change-notes/2026-04-02-alert-msg-poisoning.md new file mode 100644 index 000000000000..e2340f446a71 --- /dev/null +++ b/actions/ql/src/change-notes/2026-04-02-alert-msg-poisoning.md @@ -0,0 +1,4 @@ +--- +category: majorAnalysis +--- +* Fixed alert messages in `actions/artifact-poisoning/critical` and `actions/artifact-poisoning/medium` as they previously included a redundant placeholder in the alert message that would on occasion contain a long block of yml that makes the alert difficult to understand. Also clarify the wording to make it clear that it is not the artifact that is being poisoned, but instead a potentially untrusted artifact that is consumed. Also change the alert location to be the source, to align more with other queries reporting an artifact (e.g. zipslip) which is more useful. \ No newline at end of file diff --git a/actions/ql/src/change-notes/2026-04-02-permissions.md b/actions/ql/src/change-notes/2026-04-02-permissions.md new file mode 100644 index 000000000000..2672a30ef870 --- /dev/null +++ b/actions/ql/src/change-notes/2026-04-02-permissions.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The query `actions/missing-workflow-permissions` no longer produces false positive results on reusable workflows where all callers set permissions. \ No newline at end of file diff --git a/actions/ql/test/query-tests/Security/CWE-275/.github/workflows/perms11.yml b/actions/ql/test/query-tests/Security/CWE-275/.github/workflows/perms11.yml new file mode 100644 index 000000000000..717cdabc3025 --- /dev/null +++ b/actions/ql/test/query-tests/Security/CWE-275/.github/workflows/perms11.yml @@ -0,0 +1,9 @@ +on: + workflow_call: + +jobs: + build: + name: Build and test + runs-on: ubuntu-latest + steps: + - uses: actions/deploy-pages diff --git a/actions/ql/test/query-tests/Security/CWE-275/.github/workflows/perms12.yml b/actions/ql/test/query-tests/Security/CWE-275/.github/workflows/perms12.yml new file mode 100644 index 000000000000..25ac1f532481 --- /dev/null +++ b/actions/ql/test/query-tests/Security/CWE-275/.github/workflows/perms12.yml @@ -0,0 +1,11 @@ +on: + workflow_dispatch: + +permissions: + contents: read + id-token: write + pages: write + +jobs: + call-workflow: + uses: ./.github/workflows/perms11.yml diff --git a/actions/ql/test/query-tests/Security/CWE-829/ArtifactPoisoningCritical.expected b/actions/ql/test/query-tests/Security/CWE-829/ArtifactPoisoningCritical.expected index 2d29cd9b79b4..3c5f6bf93e98 100644 --- a/actions/ql/test/query-tests/Security/CWE-829/ArtifactPoisoningCritical.expected +++ b/actions/ql/test/query-tests/Security/CWE-829/ArtifactPoisoningCritical.expected @@ -55,21 +55,21 @@ nodes | .github/workflows/test25.yml:39:14:40:45 | ./gradlew buildScanPublishPrevious\n | semmle.label | ./gradlew buildScanPublishPrevious\n | subpaths #select -| .github/workflows/artifactpoisoning11.yml:38:11:38:77 | ./sonarcloud-data/x.py build -j$(nproc) --compiler gcc --skip-build | .github/workflows/artifactpoisoning11.yml:13:9:32:6 | Uses Step | .github/workflows/artifactpoisoning11.yml:38:11:38:77 | ./sonarcloud-data/x.py build -j$(nproc) --compiler gcc --skip-build | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning11.yml:38:11:38:77 | ./sonarcloud-data/x.py build -j$(nproc) --compiler gcc --skip-build | ./sonarcloud-data/x.py build -j$(nproc) --compiler gcc --skip-build | .github/workflows/artifactpoisoning11.yml:4:3:4:14 | workflow_run | workflow_run | -| .github/workflows/artifactpoisoning12.yml:38:11:38:25 | python foo/x.py | .github/workflows/artifactpoisoning12.yml:13:9:32:6 | Uses Step | .github/workflows/artifactpoisoning12.yml:38:11:38:25 | python foo/x.py | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning12.yml:38:11:38:25 | python foo/x.py | python foo/x.py | .github/workflows/artifactpoisoning12.yml:4:3:4:14 | workflow_run | workflow_run | -| .github/workflows/artifactpoisoning21.yml:19:14:20:21 | sh foo/cmd\n | .github/workflows/artifactpoisoning21.yml:13:9:18:6 | Uses Step | .github/workflows/artifactpoisoning21.yml:19:14:20:21 | sh foo/cmd\n | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning21.yml:19:14:20:21 | sh foo/cmd\n | sh foo/cmd\n | .github/workflows/artifactpoisoning21.yml:4:3:4:14 | workflow_run | workflow_run | -| .github/workflows/artifactpoisoning22.yml:18:14:18:19 | sh cmd | .github/workflows/artifactpoisoning22.yml:13:9:17:6 | Uses Step | .github/workflows/artifactpoisoning22.yml:18:14:18:19 | sh cmd | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning22.yml:18:14:18:19 | sh cmd | sh cmd | .github/workflows/artifactpoisoning22.yml:4:3:4:14 | workflow_run | workflow_run | -| .github/workflows/artifactpoisoning31.yml:19:14:19:22 | ./foo/cmd | .github/workflows/artifactpoisoning31.yml:13:9:15:6 | Run Step | .github/workflows/artifactpoisoning31.yml:19:14:19:22 | ./foo/cmd | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning31.yml:19:14:19:22 | ./foo/cmd | ./foo/cmd | .github/workflows/artifactpoisoning31.yml:4:3:4:14 | workflow_run | workflow_run | -| .github/workflows/artifactpoisoning32.yml:17:14:18:20 | ./bar/cmd\n | .github/workflows/artifactpoisoning32.yml:13:9:16:6 | Run Step | .github/workflows/artifactpoisoning32.yml:17:14:18:20 | ./bar/cmd\n | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning32.yml:17:14:18:20 | ./bar/cmd\n | ./bar/cmd\n | .github/workflows/artifactpoisoning32.yml:4:3:4:14 | workflow_run | workflow_run | -| .github/workflows/artifactpoisoning33.yml:17:14:18:20 | ./bar/cmd\n | .github/workflows/artifactpoisoning33.yml:13:9:16:6 | Run Step | .github/workflows/artifactpoisoning33.yml:17:14:18:20 | ./bar/cmd\n | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning33.yml:17:14:18:20 | ./bar/cmd\n | ./bar/cmd\n | .github/workflows/artifactpoisoning33.yml:4:3:4:14 | workflow_run | workflow_run | -| .github/workflows/artifactpoisoning34.yml:20:14:22:23 | npm install\nnpm run lint\n | .github/workflows/artifactpoisoning34.yml:13:9:16:6 | Run Step | .github/workflows/artifactpoisoning34.yml:20:14:22:23 | npm install\nnpm run lint\n | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning34.yml:20:14:22:23 | npm install\nnpm run lint\n | npm install\nnpm run lint\n | .github/workflows/artifactpoisoning34.yml:4:3:4:14 | workflow_run | workflow_run | -| .github/workflows/artifactpoisoning41.yml:22:14:22:22 | ./foo/cmd | .github/workflows/artifactpoisoning41.yml:13:9:21:6 | Run Step | .github/workflows/artifactpoisoning41.yml:22:14:22:22 | ./foo/cmd | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning41.yml:22:14:22:22 | ./foo/cmd | ./foo/cmd | .github/workflows/artifactpoisoning41.yml:4:3:4:14 | workflow_run | workflow_run | -| .github/workflows/artifactpoisoning42.yml:22:14:22:18 | ./cmd | .github/workflows/artifactpoisoning42.yml:13:9:21:6 | Run Step | .github/workflows/artifactpoisoning42.yml:22:14:22:18 | ./cmd | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning42.yml:22:14:22:18 | ./cmd | ./cmd | .github/workflows/artifactpoisoning42.yml:4:3:4:14 | workflow_run | workflow_run | -| .github/workflows/artifactpoisoning71.yml:17:14:18:40 | sed -f config foo.md > bar.md\n | .github/workflows/artifactpoisoning71.yml:9:9:16:6 | Uses Step | .github/workflows/artifactpoisoning71.yml:17:14:18:40 | sed -f config foo.md > bar.md\n | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning71.yml:17:14:18:40 | sed -f config foo.md > bar.md\n | sed -f config foo.md > bar.md\n | .github/workflows/artifactpoisoning71.yml:4:5:4:16 | workflow_run | workflow_run | -| .github/workflows/artifactpoisoning81.yml:31:14:31:27 | python test.py | .github/workflows/artifactpoisoning81.yml:28:9:31:6 | Uses Step | .github/workflows/artifactpoisoning81.yml:31:14:31:27 | python test.py | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning81.yml:31:14:31:27 | python test.py | python test.py | .github/workflows/artifactpoisoning81.yml:3:5:3:23 | pull_request_target | pull_request_target | -| .github/workflows/artifactpoisoning92.yml:28:9:29:6 | Uses Step | .github/actions/download-artifact-2/action.yaml:6:7:25:4 | Uses Step | .github/workflows/artifactpoisoning92.yml:28:9:29:6 | Uses Step | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning92.yml:28:9:29:6 | Uses Step | Uses Step | .github/workflows/artifactpoisoning92.yml:3:3:3:14 | workflow_run | workflow_run | -| .github/workflows/artifactpoisoning92.yml:29:14:29:26 | make snapshot | .github/actions/download-artifact-2/action.yaml:6:7:25:4 | Uses Step | .github/workflows/artifactpoisoning92.yml:29:14:29:26 | make snapshot | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning92.yml:29:14:29:26 | make snapshot | make snapshot | .github/workflows/artifactpoisoning92.yml:3:3:3:14 | workflow_run | workflow_run | -| .github/workflows/artifactpoisoning96.yml:18:14:18:24 | npm install | .github/workflows/artifactpoisoning96.yml:13:9:18:6 | Uses Step | .github/workflows/artifactpoisoning96.yml:18:14:18:24 | npm install | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning96.yml:18:14:18:24 | npm install | npm install | .github/workflows/artifactpoisoning96.yml:2:3:2:14 | workflow_run | workflow_run | -| .github/workflows/artifactpoisoning101.yml:17:14:19:59 | PR_NUMBER=$(./get_pull_request_number.sh pr_number.txt)\necho "PR_NUMBER=$PR_NUMBER" >> $GITHUB_OUTPUT \n | .github/workflows/artifactpoisoning101.yml:10:9:16:6 | Uses Step | .github/workflows/artifactpoisoning101.yml:17:14:19:59 | PR_NUMBER=$(./get_pull_request_number.sh pr_number.txt)\necho "PR_NUMBER=$PR_NUMBER" >> $GITHUB_OUTPUT \n | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning101.yml:17:14:19:59 | PR_NUMBER=$(./get_pull_request_number.sh pr_number.txt)\necho "PR_NUMBER=$PR_NUMBER" >> $GITHUB_OUTPUT \n | PR_NUMBER=$(./get_pull_request_number.sh pr_number.txt)\necho "PR_NUMBER=$PR_NUMBER" >> $GITHUB_OUTPUT \n | .github/workflows/artifactpoisoning101.yml:4:3:4:21 | pull_request_target | pull_request_target | -| .github/workflows/test18.yml:36:15:40:58 | Uses Step | .github/workflows/test18.yml:12:15:33:12 | Uses Step | .github/workflows/test18.yml:36:15:40:58 | Uses Step | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/test18.yml:36:15:40:58 | Uses Step | Uses Step | .github/workflows/test18.yml:3:5:3:16 | workflow_run | workflow_run | -| .github/workflows/test25.yml:39:14:40:45 | ./gradlew buildScanPublishPrevious\n | .github/workflows/test25.yml:22:9:32:6 | Uses Step: downloadBuildScan | .github/workflows/test25.yml:39:14:40:45 | ./gradlew buildScanPublishPrevious\n | Potential artifact poisoning in $@, which may be controlled by an external user ($@). | .github/workflows/test25.yml:39:14:40:45 | ./gradlew buildScanPublishPrevious\n | ./gradlew buildScanPublishPrevious\n | .github/workflows/test25.yml:2:3:2:14 | workflow_run | workflow_run | +| .github/actions/download-artifact-2/action.yaml:6:7:25:4 | Uses Step | .github/actions/download-artifact-2/action.yaml:6:7:25:4 | Uses Step | .github/workflows/artifactpoisoning92.yml:28:9:29:6 | Uses Step | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning92.yml:3:3:3:14 | workflow_run | workflow_run | +| .github/actions/download-artifact-2/action.yaml:6:7:25:4 | Uses Step | .github/actions/download-artifact-2/action.yaml:6:7:25:4 | Uses Step | .github/workflows/artifactpoisoning92.yml:29:14:29:26 | make snapshot | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning92.yml:3:3:3:14 | workflow_run | workflow_run | +| .github/workflows/artifactpoisoning11.yml:13:9:32:6 | Uses Step | .github/workflows/artifactpoisoning11.yml:13:9:32:6 | Uses Step | .github/workflows/artifactpoisoning11.yml:38:11:38:77 | ./sonarcloud-data/x.py build -j$(nproc) --compiler gcc --skip-build | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning11.yml:4:3:4:14 | workflow_run | workflow_run | +| .github/workflows/artifactpoisoning12.yml:13:9:32:6 | Uses Step | .github/workflows/artifactpoisoning12.yml:13:9:32:6 | Uses Step | .github/workflows/artifactpoisoning12.yml:38:11:38:25 | python foo/x.py | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning12.yml:4:3:4:14 | workflow_run | workflow_run | +| .github/workflows/artifactpoisoning21.yml:13:9:18:6 | Uses Step | .github/workflows/artifactpoisoning21.yml:13:9:18:6 | Uses Step | .github/workflows/artifactpoisoning21.yml:19:14:20:21 | sh foo/cmd\n | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning21.yml:4:3:4:14 | workflow_run | workflow_run | +| .github/workflows/artifactpoisoning22.yml:13:9:17:6 | Uses Step | .github/workflows/artifactpoisoning22.yml:13:9:17:6 | Uses Step | .github/workflows/artifactpoisoning22.yml:18:14:18:19 | sh cmd | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning22.yml:4:3:4:14 | workflow_run | workflow_run | +| .github/workflows/artifactpoisoning31.yml:13:9:15:6 | Run Step | .github/workflows/artifactpoisoning31.yml:13:9:15:6 | Run Step | .github/workflows/artifactpoisoning31.yml:19:14:19:22 | ./foo/cmd | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning31.yml:4:3:4:14 | workflow_run | workflow_run | +| .github/workflows/artifactpoisoning32.yml:13:9:16:6 | Run Step | .github/workflows/artifactpoisoning32.yml:13:9:16:6 | Run Step | .github/workflows/artifactpoisoning32.yml:17:14:18:20 | ./bar/cmd\n | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning32.yml:4:3:4:14 | workflow_run | workflow_run | +| .github/workflows/artifactpoisoning33.yml:13:9:16:6 | Run Step | .github/workflows/artifactpoisoning33.yml:13:9:16:6 | Run Step | .github/workflows/artifactpoisoning33.yml:17:14:18:20 | ./bar/cmd\n | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning33.yml:4:3:4:14 | workflow_run | workflow_run | +| .github/workflows/artifactpoisoning34.yml:13:9:16:6 | Run Step | .github/workflows/artifactpoisoning34.yml:13:9:16:6 | Run Step | .github/workflows/artifactpoisoning34.yml:20:14:22:23 | npm install\nnpm run lint\n | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning34.yml:4:3:4:14 | workflow_run | workflow_run | +| .github/workflows/artifactpoisoning41.yml:13:9:21:6 | Run Step | .github/workflows/artifactpoisoning41.yml:13:9:21:6 | Run Step | .github/workflows/artifactpoisoning41.yml:22:14:22:22 | ./foo/cmd | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning41.yml:4:3:4:14 | workflow_run | workflow_run | +| .github/workflows/artifactpoisoning42.yml:13:9:21:6 | Run Step | .github/workflows/artifactpoisoning42.yml:13:9:21:6 | Run Step | .github/workflows/artifactpoisoning42.yml:22:14:22:18 | ./cmd | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning42.yml:4:3:4:14 | workflow_run | workflow_run | +| .github/workflows/artifactpoisoning71.yml:9:9:16:6 | Uses Step | .github/workflows/artifactpoisoning71.yml:9:9:16:6 | Uses Step | .github/workflows/artifactpoisoning71.yml:17:14:18:40 | sed -f config foo.md > bar.md\n | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning71.yml:4:5:4:16 | workflow_run | workflow_run | +| .github/workflows/artifactpoisoning81.yml:28:9:31:6 | Uses Step | .github/workflows/artifactpoisoning81.yml:28:9:31:6 | Uses Step | .github/workflows/artifactpoisoning81.yml:31:14:31:27 | python test.py | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning81.yml:3:5:3:23 | pull_request_target | pull_request_target | +| .github/workflows/artifactpoisoning96.yml:13:9:18:6 | Uses Step | .github/workflows/artifactpoisoning96.yml:13:9:18:6 | Uses Step | .github/workflows/artifactpoisoning96.yml:18:14:18:24 | npm install | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning96.yml:2:3:2:14 | workflow_run | workflow_run | +| .github/workflows/artifactpoisoning101.yml:10:9:16:6 | Uses Step | .github/workflows/artifactpoisoning101.yml:10:9:16:6 | Uses Step | .github/workflows/artifactpoisoning101.yml:17:14:19:59 | PR_NUMBER=$(./get_pull_request_number.sh pr_number.txt)\necho "PR_NUMBER=$PR_NUMBER" >> $GITHUB_OUTPUT \n | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/artifactpoisoning101.yml:4:3:4:21 | pull_request_target | pull_request_target | +| .github/workflows/test18.yml:12:15:33:12 | Uses Step | .github/workflows/test18.yml:12:15:33:12 | Uses Step | .github/workflows/test18.yml:36:15:40:58 | Uses Step | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/test18.yml:3:5:3:16 | workflow_run | workflow_run | +| .github/workflows/test25.yml:22:9:32:6 | Uses Step: downloadBuildScan | .github/workflows/test25.yml:22:9:32:6 | Uses Step: downloadBuildScan | .github/workflows/test25.yml:39:14:40:45 | ./gradlew buildScanPublishPrevious\n | Potential artifact poisoning; the artifact being consumed has contents that may be controlled by an external user ($@). | .github/workflows/test25.yml:2:3:2:14 | workflow_run | workflow_run | diff --git a/cpp/ql/integration-tests/query-suite/cpp-code-scanning.qls.expected b/cpp/ql/integration-tests/query-suite/cpp-code-scanning.qls.expected index 57d240fd7958..1b6db01b9033 100644 --- a/cpp/ql/integration-tests/query-suite/cpp-code-scanning.qls.expected +++ b/cpp/ql/integration-tests/query-suite/cpp-code-scanning.qls.expected @@ -7,10 +7,12 @@ ql/cpp/ql/src/Diagnostics/ExtractedFiles.ql ql/cpp/ql/src/Diagnostics/ExtractionWarnings.ql ql/cpp/ql/src/Diagnostics/FailedExtractorInvocations.ql ql/cpp/ql/src/Likely Bugs/Arithmetic/BadAdditionOverflowCheck.ql +ql/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql ql/cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck.ql ql/cpp/ql/src/Likely Bugs/Conversion/CastArrayPointerArithmetic.ql ql/cpp/ql/src/Likely Bugs/Format/SnprintfOverflow.ql ql/cpp/ql/src/Likely Bugs/Format/WrongNumberOfFormatArguments.ql +ql/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql ql/cpp/ql/src/Likely Bugs/Memory Management/AllocaInLoop.ql ql/cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow.ql ql/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql @@ -28,6 +30,7 @@ ql/cpp/ql/src/Security/CWE/CWE-120/VeryLikelyOverrunWrite.ql ql/cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql ql/cpp/ql/src/Security/CWE/CWE-134/UncontrolledFormatString.ql ql/cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql +ql/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql ql/cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql ql/cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.ql ql/cpp/ql/src/Security/CWE/CWE-311/CleartextFileWrite.ql diff --git a/cpp/ql/lib/semmle/code/cpp/commons/Printf.qll b/cpp/ql/lib/semmle/code/cpp/commons/Printf.qll index d189dd36f87c..624465761c2c 100644 --- a/cpp/ql/lib/semmle/code/cpp/commons/Printf.qll +++ b/cpp/ql/lib/semmle/code/cpp/commons/Printf.qll @@ -459,6 +459,13 @@ class FormatLiteral extends Literal instanceof StringLiteral { */ int getConvSpecOffset(int n) { result = this.getFormat().indexOf("%", n, 0) } + /** + * Gets the nth conversion specifier string. + */ + private string getConvSpecString(int n) { + n >= 0 and result = "%" + this.getFormat().splitAt("%", n + 1) + } + /* * Each of these predicates gets a regular expressions to match each individual * parts of a conversion specifier. @@ -524,22 +531,20 @@ class FormatLiteral extends Literal instanceof StringLiteral { int n, string spec, string params, string flags, string width, string prec, string len, string conv ) { - exists(int offset, string fmt, string rst, string regexp | - offset = this.getConvSpecOffset(n) and - fmt = this.getFormat() and - rst = fmt.substring(offset, fmt.length()) and + exists(string convSpec, string regexp | + convSpec = this.getConvSpecString(n) and regexp = this.getConvSpecRegexp() and ( - spec = rst.regexpCapture(regexp, 1) and - params = rst.regexpCapture(regexp, 2) and - flags = rst.regexpCapture(regexp, 3) and - width = rst.regexpCapture(regexp, 4) and - prec = rst.regexpCapture(regexp, 5) and - len = rst.regexpCapture(regexp, 6) and - conv = rst.regexpCapture(regexp, 7) + spec = convSpec.regexpCapture(regexp, 1) and + params = convSpec.regexpCapture(regexp, 2) and + flags = convSpec.regexpCapture(regexp, 3) and + width = convSpec.regexpCapture(regexp, 4) and + prec = convSpec.regexpCapture(regexp, 5) and + len = convSpec.regexpCapture(regexp, 6) and + conv = convSpec.regexpCapture(regexp, 7) or - spec = rst.regexpCapture(regexp, 1) and - not exists(rst.regexpCapture(regexp, 2)) and + spec = convSpec.regexpCapture(regexp, 1) and + not exists(convSpec.regexpCapture(regexp, 2)) and params = "" and flags = "" and width = "" and @@ -554,12 +559,10 @@ class FormatLiteral extends Literal instanceof StringLiteral { * Gets the nth conversion specifier (including the initial `%`). */ string getConvSpec(int n) { - exists(int offset, string fmt, string rst, string regexp | - offset = this.getConvSpecOffset(n) and - fmt = this.getFormat() and - rst = fmt.substring(offset, fmt.length()) and + exists(string convSpec, string regexp | + convSpec = this.getConvSpecString(n) and regexp = this.getConvSpecRegexp() and - result = rst.regexpCapture(regexp, 1) + result = convSpec.regexpCapture(regexp, 1) ) } diff --git a/cpp/ql/lib/semmle/code/cpp/commons/Scanf.qll b/cpp/ql/lib/semmle/code/cpp/commons/Scanf.qll index f032ba4749e6..98280a522cfd 100644 --- a/cpp/ql/lib/semmle/code/cpp/commons/Scanf.qll +++ b/cpp/ql/lib/semmle/code/cpp/commons/Scanf.qll @@ -194,6 +194,13 @@ class ScanfFormatLiteral extends Expr { ) } + /** + * Gets the nth conversion specifier string. + */ + private string getConvSpecString(int n) { + n >= 0 and result = "%" + this.getFormat().splitAt("%", n + 1) + } + /** * Gets the regular expression to match each individual part of a conversion specifier. */ @@ -227,16 +234,14 @@ class ScanfFormatLiteral extends Expr { * specifier. */ predicate parseConvSpec(int n, string spec, string width, string len, string conv) { - exists(int offset, string fmt, string rst, string regexp | - offset = this.getConvSpecOffset(n) and - fmt = this.getFormat() and - rst = fmt.substring(offset, fmt.length()) and + exists(string convSpec, string regexp | + convSpec = this.getConvSpecString(n) and regexp = this.getConvSpecRegexp() and ( - spec = rst.regexpCapture(regexp, 1) and - width = rst.regexpCapture(regexp, 2) and - len = rst.regexpCapture(regexp, 3) and - conv = rst.regexpCapture(regexp, 4) + spec = convSpec.regexpCapture(regexp, 1) and + width = convSpec.regexpCapture(regexp, 2) and + len = convSpec.regexpCapture(regexp, 3) and + conv = convSpec.regexpCapture(regexp, 4) ) ) } diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qll index 8b71f140b01b..fb2108c2ac58 100644 --- a/cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qll +++ b/cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qll @@ -6,11 +6,15 @@ * * The extensible relations have the following columns: * - Sources: - * `namespace; type; subtypes; name; signature; ext; output; kind` + * `namespace; type; subtypes; name; signature; ext; output; kind; provenance` * - Sinks: - * `namespace; type; subtypes; name; signature; ext; input; kind` + * `namespace; type; subtypes; name; signature; ext; input; kind; provenance` * - Summaries: - * `namespace; type; subtypes; name; signature; ext; input; output; kind` + * `namespace; type; subtypes; name; signature; ext; input; output; kind; provenance` + * - Barriers: + * `namespace; type; subtypes; name; signature; ext; output; kind; provenance` + * - BarrierGuards: + * `namespace; type; subtypes; name; signature; ext; input; acceptingValue; kind; provenance` * * The interpretation of a row is similar to API-graphs with a left-to-right * reading. @@ -87,11 +91,23 @@ * value, and * - flow from the _second_ indirection of the 0th argument to the first * indirection of the return value, etc. - * 8. The `kind` column is a tag that can be referenced from QL to determine to + * 8. The `acceptingValue` column of barrier guard models specifies the condition + * under which the guard blocks flow. It can be one of "true" or "false". In + * the future "no-exception", "not-zero", "null", "not-null" may be supported. + * 9. The `kind` column is a tag that can be referenced from QL to determine to * which classes the interpreted elements should be added. For example, for * sources "remote" indicates a default remote flow source, and for summaries * "taint" indicates a default additional taint step and "value" indicates a * globally applicable value-preserving step. + * 10. The `provenance` column is a tag to indicate the origin and verification of a model. + * The format is {origin}-{verification} or just "manual" where the origin describes + * the origin of the model and verification describes how the model has been verified. + * Some examples are: + * - "df-generated": The model has been generated by the model generator tool. + * - "df-manual": The model has been generated by the model generator and verified by a human. + * - "manual": The model has been written by hand. + * This information is used in a heuristic for dataflow analysis to determine, if a + * model or source code should be used for determining flow. */ import cpp @@ -931,13 +947,13 @@ private module Cached { private predicate barrierGuardChecks(IRGuardCondition g, Expr e, boolean gv, TKindModelPair kmp) { exists( - SourceSinkInterpretationInput::InterpretNode n, Public::AcceptingValue acceptingvalue, + SourceSinkInterpretationInput::InterpretNode n, Public::AcceptingValue acceptingValue, string kind, string model | - isBarrierGuardNode(n, acceptingvalue, kind, model) and + isBarrierGuardNode(n, acceptingValue, kind, model) and n.asNode().asExpr() = e and kmp = TMkPair(kind, model) and - gv = convertAcceptingValue(acceptingvalue).asBooleanValue() and + gv = convertAcceptingValue(acceptingValue).asBooleanValue() and n.asNode().(Private::ArgumentNode).getCall().asCallInstruction() = g ) } @@ -954,14 +970,14 @@ private module Cached { ) { exists( SourceSinkInterpretationInput::InterpretNode interpretNode, - Public::AcceptingValue acceptingvalue, string kind, string model, int indirectionIndex, + Public::AcceptingValue acceptingValue, string kind, string model, int indirectionIndex, Private::ArgumentNode arg | - isBarrierGuardNode(interpretNode, acceptingvalue, kind, model) and + isBarrierGuardNode(interpretNode, acceptingValue, kind, model) and arg = interpretNode.asNode() and arg.asIndirectExpr(indirectionIndex) = e and kmp = MkKindModelPairIntPair(TMkPair(kind, model), indirectionIndex) and - gv = convertAcceptingValue(acceptingvalue).asBooleanValue() and + gv = convertAcceptingValue(acceptingValue).asBooleanValue() and arg.getCall().asCallInstruction() = g ) } diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/internal/ExternalFlowExtensions.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/internal/ExternalFlowExtensions.qll index 1a572c221d9f..22c74c2aa714 100644 --- a/cpp/ql/lib/semmle/code/cpp/dataflow/internal/ExternalFlowExtensions.qll +++ b/cpp/ql/lib/semmle/code/cpp/dataflow/internal/ExternalFlowExtensions.qll @@ -33,7 +33,7 @@ extensible predicate barrierModel( */ extensible predicate barrierGuardModel( string namespace, string type, boolean subtypes, string name, string signature, string ext, - string input, string acceptingvalue, string kind, string provenance, QlBuiltins::ExtensionId madId + string input, string acceptingValue, string kind, string provenance, QlBuiltins::ExtensionId madId ); /** diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/internal/FlowSummaryImpl.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/internal/FlowSummaryImpl.qll index cce1b80e7fcb..d91dc41febeb 100644 --- a/cpp/ql/lib/semmle/code/cpp/dataflow/internal/FlowSummaryImpl.qll +++ b/cpp/ql/lib/semmle/code/cpp/dataflow/internal/FlowSummaryImpl.qll @@ -162,13 +162,13 @@ module SourceSinkInterpretationInput implements } predicate barrierGuardElement( - Element e, string input, Public::AcceptingValue acceptingvalue, string kind, + Element e, string input, Public::AcceptingValue acceptingValue, string kind, Public::Provenance provenance, string model ) { exists( string package, string type, boolean subtypes, string name, string signature, string ext | - barrierGuardModel(package, type, subtypes, name, signature, ext, input, acceptingvalue, kind, + barrierGuardModel(package, type, subtypes, name, signature, ext, input, acceptingValue, kind, provenance, model) and e = interpretElement(package, type, subtypes, name, signature, ext) ) diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql b/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql index 6747d177c80e..b05bd637dc2d 100644 --- a/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql +++ b/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql @@ -5,7 +5,7 @@ * @kind problem * @problem.severity warning * @security-severity 8.1 - * @precision medium + * @precision high * @id cpp/integer-multiplication-cast-to-long * @tags reliability * security diff --git a/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql b/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql index 7f0a4833cb59..5842b9474f74 100644 --- a/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql +++ b/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql @@ -5,7 +5,7 @@ * @kind problem * @problem.severity error * @security-severity 7.5 - * @precision medium + * @precision high * @id cpp/wrong-type-format-argument * @tags reliability * correctness diff --git a/cpp/ql/src/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.qhelp b/cpp/ql/src/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.qhelp index 6ff60d383419..90a98e1bf573 100644 --- a/cpp/ql/src/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.qhelp +++ b/cpp/ql/src/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.qhelp @@ -14,6 +14,9 @@ function may behave unpredictably.

This may indicate a misspelled function name, or that the required header containing the function declaration has not been included.

+

Note: This query is not compatible with build mode: none databases, and produces +no results on those databases.

+

Provide an explicit declaration of the function before invoking it.

@@ -26,4 +29,4 @@ the function declaration has not been included.

  • SEI CERT C Coding Standard: DCL31-C. Declare identifiers before using them
    •  - \ No newline at end of file + diff --git a/cpp/ql/src/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.ql b/cpp/ql/src/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.ql index 6a55557cf70b..00b29efbd0f2 100644 --- a/cpp/ql/src/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.ql +++ b/cpp/ql/src/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.ql @@ -5,7 +5,7 @@ * may lead to unpredictable behavior. * @kind problem * @problem.severity warning - * @precision medium + * @precision high * @id cpp/implicit-function-declaration * @tags correctness * maintainability @@ -17,6 +17,11 @@ import TooFewArguments import TooManyArguments import semmle.code.cpp.commons.Exclusions +/* + * This query is not compatible with build mode: none databases, and produces + * no results on those databases. + */ + predicate locInfo(Locatable e, File file, int line, int col) { e.getFile() = file and e.getLocation().getStartLine() = line and @@ -39,6 +44,7 @@ predicate isCompiledAsC(File f) { from FunctionDeclarationEntry fdeIm, FunctionCall fc where isCompiledAsC(fdeIm.getFile()) and + not any(Compilation c).buildModeNone() and not isFromMacroDefinition(fc) and fdeIm.isImplicit() and sameLocation(fdeIm, fc) and diff --git a/cpp/ql/src/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.qll b/cpp/ql/src/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.qll index 2dced5d8d844..dbb457db505e 100644 --- a/cpp/ql/src/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.qll +++ b/cpp/ql/src/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.qll @@ -79,9 +79,7 @@ private predicate hasZeroParamDecl(Function f) { // True if this file (or header) was compiled as a C file private predicate isCompiledAsC(File f) { - f.compiledAsC() - or - exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f) + exists(File src | src.compiledAsC() | src.getAnIncludedFile*() = f) } predicate mistypedFunctionArguments(FunctionCall fc, Function f, Parameter p) { diff --git a/cpp/ql/src/Likely Bugs/Underspecified Functions/TooFewArguments.qll b/cpp/ql/src/Likely Bugs/Underspecified Functions/TooFewArguments.qll index 218a54b36c51..fd323513a49e 100644 --- a/cpp/ql/src/Likely Bugs/Underspecified Functions/TooFewArguments.qll +++ b/cpp/ql/src/Likely Bugs/Underspecified Functions/TooFewArguments.qll @@ -28,9 +28,7 @@ private predicate hasZeroParamDecl(Function f) { /* Holds if this file (or header) was compiled as a C file. */ private predicate isCompiledAsC(File f) { - f.compiledAsC() - or - exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f) + exists(File src | src.compiledAsC() | src.getAnIncludedFile*() = f) } /** Holds if `fc` is a call to `f` with too few arguments. */ diff --git a/cpp/ql/src/Likely Bugs/Underspecified Functions/TooManyArguments.qll b/cpp/ql/src/Likely Bugs/Underspecified Functions/TooManyArguments.qll index 7fba78b5550e..ab2a98ae3a55 100644 --- a/cpp/ql/src/Likely Bugs/Underspecified Functions/TooManyArguments.qll +++ b/cpp/ql/src/Likely Bugs/Underspecified Functions/TooManyArguments.qll @@ -19,9 +19,7 @@ private predicate hasZeroParamDecl(Function f) { // True if this file (or header) was compiled as a C file private predicate isCompiledAsC(File f) { - f.compiledAsC() - or - exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f) + exists(File src | src.compiledAsC() | src.getAnIncludedFile*() = f) } predicate tooManyArguments(FunctionCall fc, Function f) { diff --git a/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql b/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql index 3f330807304f..7d9ef88adea1 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql +++ b/cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql @@ -6,7 +6,7 @@ * @kind problem * @problem.severity warning * @security-severity 7.8 - * @precision medium + * @precision high * @tags reliability * security * external/cwe/cwe-190 diff --git a/cpp/ql/src/change-notes/2026-03-23-implicit-function-declaration.md b/cpp/ql/src/change-notes/2026-03-23-implicit-function-declaration.md new file mode 100644 index 000000000000..8c2c431ec24c --- /dev/null +++ b/cpp/ql/src/change-notes/2026-03-23-implicit-function-declaration.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The "Implicit function declaration" (`cpp/implicit-function-declaration`) query no longer produces results on `build mode: none` databases. These results were found to be very noisy and fundamentally imprecise in this mode. diff --git a/cpp/ql/src/change-notes/2026-04-02-comparison-with-wider-type.md b/cpp/ql/src/change-notes/2026-04-02-comparison-with-wider-type.md new file mode 100644 index 000000000000..c84e1dba404c --- /dev/null +++ b/cpp/ql/src/change-notes/2026-04-02-comparison-with-wider-type.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The "Comparison of narrow type with wide type in loop condition" (`cpp/comparison-with-wider-type`) query has been upgraded to `high` precision. This query will now run in the default code scanning suite. diff --git a/cpp/ql/src/change-notes/2026-04-02-implicit-function-declaration.md b/cpp/ql/src/change-notes/2026-04-02-implicit-function-declaration.md new file mode 100644 index 000000000000..dd0dbd4bc7d9 --- /dev/null +++ b/cpp/ql/src/change-notes/2026-04-02-implicit-function-declaration.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The "Implicit function declaration" (`cpp/implicit-function-declaration`) query has been upgraded to `high` precision. diff --git a/cpp/ql/src/change-notes/2026-04-02-integer-multiplication-cast-to-long.md b/cpp/ql/src/change-notes/2026-04-02-integer-multiplication-cast-to-long.md new file mode 100644 index 000000000000..cd6796b408f0 --- /dev/null +++ b/cpp/ql/src/change-notes/2026-04-02-integer-multiplication-cast-to-long.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The "Multiplication result converted to larger type" (`cpp/integer-multiplication-cast-to-long`) query has been upgraded to `high` precision. This query will now run in the default code scanning suite. diff --git a/cpp/ql/src/change-notes/2026-04-02-wrong-type-format-argument.md b/cpp/ql/src/change-notes/2026-04-02-wrong-type-format-argument.md new file mode 100644 index 000000000000..f8b9085dacc6 --- /dev/null +++ b/cpp/ql/src/change-notes/2026-04-02-wrong-type-format-argument.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The "Wrong type of arguments to formatting function" (`cpp/wrong-type-format-argument`) query has been upgraded to `high` precision. This query will now run in the default code scanning suite. diff --git a/csharp/ql/lib/semmle/code/csharp/Conversion.qll b/csharp/ql/lib/semmle/code/csharp/Conversion.qll index ec7ef9cac952..fd2c680e9c7f 100644 --- a/csharp/ql/lib/semmle/code/csharp/Conversion.qll +++ b/csharp/ql/lib/semmle/code/csharp/Conversion.qll @@ -232,14 +232,9 @@ private module Identity { */ pragma[nomagic] private predicate convTypeArguments(Type fromTypeArgument, Type toTypeArgument, int i) { - exists(int j | - fromTypeArgument = getTypeArgumentRanked(_, _, i) and - toTypeArgument = getTypeArgumentRanked(_, _, j) and - i <= j and - j <= i - | - convIdentity(fromTypeArgument, toTypeArgument) - ) + fromTypeArgument = getTypeArgumentRanked(_, _, pragma[only_bind_into](i)) and + toTypeArgument = getTypeArgumentRanked(_, _, pragma[only_bind_into](i)) and + convIdentity(fromTypeArgument, toTypeArgument) } pragma[nomagic] @@ -929,19 +924,16 @@ private module Variance { private predicate convTypeArguments( TypeArgument fromTypeArgument, TypeArgument toTypeArgument, int i, TVariance v ) { - exists(int j | - fromTypeArgument = getTypeArgumentRanked(_, _, i, _) and - toTypeArgument = getTypeArgumentRanked(_, _, j, _) and - i <= j and - j <= i - | + fromTypeArgument = getTypeArgumentRanked(_, _, pragma[only_bind_into](i), _) and + toTypeArgument = getTypeArgumentRanked(_, _, pragma[only_bind_into](i), _) and + ( convIdentity(fromTypeArgument, toTypeArgument) and v = TNone() or - convRefTypeTypeArgumentOut(fromTypeArgument, toTypeArgument, j) and + convRefTypeTypeArgumentOut(fromTypeArgument, toTypeArgument, i) and v = TOut() or - convRefTypeTypeArgumentIn(toTypeArgument, fromTypeArgument, j) and + convRefTypeTypeArgumentIn(toTypeArgument, fromTypeArgument, i) and v = TIn() ) } diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowImplSpecific.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowImplSpecific.qll index af104d777b87..ab1e75b3d0fc 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowImplSpecific.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowImplSpecific.qll @@ -29,4 +29,8 @@ module CsharpDataFlow implements InputSig { predicate neverSkipInPathGraph(Node n) { exists(n.(AssignableDefinitionNode).getDefinition().getTargetAccess()) } + + DataFlowType getSourceContextParameterNodeType(Node p) { + exists(p) and result.isSourceContextParameterType() + } } diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll index 5554b8cdbd79..ae530b2d451a 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll @@ -1179,7 +1179,8 @@ private module Cached { cached newtype TDataFlowType = TGvnDataFlowType(Gvn::GvnType t) or - TDelegateDataFlowType(Callable lambda) { lambdaCreationExpr(_, lambda) } + TDelegateDataFlowType(Callable lambda) { lambdaCreationExpr(_, lambda) } or + TSourceContextParameterType() } import Cached @@ -2394,6 +2395,8 @@ class DataFlowType extends TDataFlowType { Callable asDelegate() { this = TDelegateDataFlowType(result) } + predicate isSourceContextParameterType() { this = TSourceContextParameterType() } + /** * Gets an expression that creates a delegate of this type. * @@ -2412,6 +2415,9 @@ class DataFlowType extends TDataFlowType { result = this.asGvnType().toString() or result = this.asDelegate().toString() + or + this.isSourceContextParameterType() and + result = "" } } @@ -2469,6 +2475,11 @@ private predicate compatibleTypesDelegateLeft(DataFlowType dt1, DataFlowType dt2 ) } +pragma[nomagic] +private predicate compatibleTypesSourceContextParameterTypeLeft(DataFlowType dt1, DataFlowType dt2) { + dt1.isSourceContextParameterType() and not exists(dt2.asDelegate()) +} + /** * Holds if `t1` and `t2` are compatible, that is, whether data can flow from * a node of type `t1` to a node of type `t2`. @@ -2499,6 +2510,10 @@ predicate compatibleTypes(DataFlowType dt1, DataFlowType dt2) { compatibleTypesDelegateLeft(dt2, dt1) or dt1.asDelegate() = dt2.asDelegate() + or + compatibleTypesSourceContextParameterTypeLeft(dt1, dt2) + or + compatibleTypesSourceContextParameterTypeLeft(dt2, dt1) } pragma[nomagic] @@ -2511,6 +2526,8 @@ predicate typeStrongerThan(DataFlowType t1, DataFlowType t2) { uselessTypebound(t2) or compatibleTypesDelegateLeft(t1, t2) + or + compatibleTypesSourceContextParameterTypeLeft(t1, t2) } /** diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll index 024e9cf119d5..f8cec8c4d9f6 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll @@ -4,13 +4,17 @@ * Provides classes and predicates for dealing with MaD flow models specified * in data extensions and CSV format. * - * The CSV specification has the following columns: + * The extensible relations have the following columns: * - Sources: * `namespace; type; subtypes; name; signature; ext; output; kind; provenance` * - Sinks: * `namespace; type; subtypes; name; signature; ext; input; kind; provenance` * - Summaries: * `namespace; type; subtypes; name; signature; ext; input; output; kind; provenance` + * - Barriers: + * `namespace; type; subtypes; name; signature; ext; output; kind; provenance` + * - BarrierGuards: + * `namespace; type; subtypes; name; signature; ext; input; acceptingValue; kind; provenance` * - Neutrals: * `namespace; type; name; signature; kind; provenance` * A neutral is used to indicate that a callable is neutral with respect to flow (no summary), source (is not a source) or sink (is not a sink). @@ -69,14 +73,17 @@ * - "Field[f]": Selects the contents of field `f`. * - "Property[p]": Selects the contents of property `p`. * - * 8. The `kind` column is a tag that can be referenced from QL to determine to + * 8. The `acceptingValue` column of barrier guard models specifies the condition + * under which the guard blocks flow. It can be one of "true" or "false". In + * the future "no-exception", "not-zero", "null", "not-null" may be supported. + * 9. The `kind` column is a tag that can be referenced from QL to determine to * which classes the interpreted elements should be added. For example, for * sources "remote" indicates a default remote flow source, and for summaries * "taint" indicates a default additional taint step and "value" indicates a * globally applicable value-preserving step. For neutrals the kind can be `summary`, * `source` or `sink` to indicate that the neutral is neutral with respect to * flow (no summary), source (is not a source) or sink (is not a sink). - * 9. The `provenance` column is a tag to indicate the origin and verification of a model. + * 10. The `provenance` column is a tag to indicate the origin and verification of a model. * The format is {origin}-{verification} or just "manual" where the origin describes * the origin of the model and verification describes how the model has been verified. * Some examples are: @@ -230,11 +237,11 @@ module ModelValidation { result = "Unrecognized provenance description \"" + provenance + "\" in " + pred + " model." ) or - exists(string acceptingvalue | - barrierGuardModel(_, _, _, _, _, _, _, acceptingvalue, _, _, _) and - invalidAcceptingValue(acceptingvalue) and + exists(string acceptingValue | + barrierGuardModel(_, _, _, _, _, _, _, acceptingValue, _, _, _) and + invalidAcceptingValue(acceptingValue) and result = - "Unrecognized accepting value description \"" + acceptingvalue + + "Unrecognized accepting value description \"" + acceptingValue + "\" in barrier guard model." ) } @@ -482,13 +489,13 @@ private module Cached { private predicate barrierGuardChecks(Guard g, Expr e, GuardValue gv, TKindModelPair kmp) { exists( - SourceSinkInterpretationInput::InterpretNode n, AcceptingValue acceptingvalue, string kind, + SourceSinkInterpretationInput::InterpretNode n, AcceptingValue acceptingValue, string kind, string model | - isBarrierGuardNode(n, acceptingvalue, kind, model) and + isBarrierGuardNode(n, acceptingValue, kind, model) and n.asNode().asExpr() = e and kmp = TMkPair(kind, model) and - gv = convertAcceptingValue(acceptingvalue) + gv = convertAcceptingValue(acceptingValue) | g.(Call).getAnArgument() = e or g.(QualifiableExpr).getQualifier() = e ) diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlowExtensions.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlowExtensions.qll index 3461f0a51863..cd438ece284d 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlowExtensions.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlowExtensions.qll @@ -33,7 +33,7 @@ extensible predicate barrierModel( */ extensible predicate barrierGuardModel( string namespace, string type, boolean subtypes, string name, string signature, string ext, - string input, string acceptingvalue, string kind, string provenance, QlBuiltins::ExtensionId madId + string input, string acceptingValue, string kind, string provenance, QlBuiltins::ExtensionId madId ); /** diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/FlowSummaryImpl.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/FlowSummaryImpl.qll index ec6bbf81fee1..a7ab18a62901 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/FlowSummaryImpl.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/FlowSummaryImpl.qll @@ -253,13 +253,13 @@ module SourceSinkInterpretationInput implements } predicate barrierGuardElement( - Element e, string input, Public::AcceptingValue acceptingvalue, string kind, + Element e, string input, Public::AcceptingValue acceptingValue, string kind, Public::Provenance provenance, string model ) { exists( string namespace, string type, boolean subtypes, string name, string signature, string ext | - barrierGuardModel(namespace, type, subtypes, name, signature, ext, input, acceptingvalue, + barrierGuardModel(namespace, type, subtypes, name, signature, ext, input, acceptingValue, kind, provenance, model) and e = interpretElement(namespace, type, subtypes, name, signature, ext, _) ) diff --git a/csharp/ql/test/utils/modelgenerator/dataflow/Summaries.cs b/csharp/ql/test/utils/modelgenerator/dataflow/Summaries.cs index b59513504d9d..4c85b397ac1f 100644 --- a/csharp/ql/test/utils/modelgenerator/dataflow/Summaries.cs +++ b/csharp/ql/test/utils/modelgenerator/dataflow/Summaries.cs @@ -536,6 +536,12 @@ public void Apply(Action a, object o) { a(o); } + + private void CallApply() + { + // Test that this call to `Apply` does not interfere with the flow summaries generated for `Apply` + Apply(x => x, null); + } } public static class HigherOrderExtensionMethods diff --git a/docs/codeql/reusables/supported-versions-compilers.rst b/docs/codeql/reusables/supported-versions-compilers.rst index ddda7065cf1c..3891332f4575 100644 --- a/docs/codeql/reusables/supported-versions-compilers.rst +++ b/docs/codeql/reusables/supported-versions-compilers.rst @@ -11,23 +11,23 @@ Microsoft extensions (up to VS 2022), Arm Compiler 5 [5]_","``.cpp``, ``.c++``, ``.cxx``, ``.hpp``, ``.hh``, ``.h++``, ``.hxx``, ``.c``, ``.cc``, ``.h``" - C#,C# up to 13,"Microsoft Visual Studio up to 2019 with .NET up to 4.8, + C#,C# up to 14 [6]_,"Microsoft Visual Studio up to 2019 with .NET up to 4.8, .NET Core up to 3.1 - .NET 5, .NET 6, .NET 7, .NET 8, .NET 9","``.sln``, ``.slnx``, ``.csproj``, ``.cs``, ``.cshtml``, ``.xaml``" + .NET 5, .NET 6, .NET 7, .NET 8, .NET 9, .NET 10 [6]_","``.sln``, ``.slnx``, ``.csproj``, ``.cs``, ``.cshtml``, ``.xaml``" GitHub Actions,"Not applicable",Not applicable,"``.github/workflows/*.yml``, ``.github/workflows/*.yaml``, ``**/action.yml``, ``**/action.yaml``" Go (aka Golang), "Go up to 1.26", "Go 1.11 or more recent", ``.go`` - Java,"Java 7 to 26 [6]_","javac (OpenJDK and Oracle JDK), + Java,"Java 7 to 26 [7]_","javac (OpenJDK and Oracle JDK), - Eclipse compiler for Java (ECJ) [7]_",``.java`` + Eclipse compiler for Java (ECJ) [8]_",``.java`` Kotlin,"Kotlin 1.8.0 to 2.3.2\ *x*","kotlinc",``.kt`` - JavaScript,ECMAScript 2022 or lower,Not applicable,"``.js``, ``.jsx``, ``.mjs``, ``.es``, ``.es6``, ``.htm``, ``.html``, ``.xhtm``, ``.xhtml``, ``.vue``, ``.hbs``, ``.ejs``, ``.njk``, ``.json``, ``.yaml``, ``.yml``, ``.raml``, ``.xml`` [8]_" - Python [9]_,"2.7, 3.5, 3.6, 3.7, 3.8, 3.9, 3.10, 3.11, 3.12, 3.13",Not applicable,``.py`` - Ruby [10]_,"up to 3.3",Not applicable,"``.rb``, ``.erb``, ``.gemspec``, ``Gemfile``" - Rust [11]_,"Rust editions 2021 and 2024","Rust compiler","``.rs``, ``Cargo.toml``" - Swift [12]_ [13]_,"Swift 5.4-6.2","Swift compiler","``.swift``" - TypeScript [14]_,"2.6-5.9",Standard TypeScript compiler,"``.ts``, ``.tsx``, ``.mts``, ``.cts``" + JavaScript,ECMAScript 2022 or lower,Not applicable,"``.js``, ``.jsx``, ``.mjs``, ``.es``, ``.es6``, ``.htm``, ``.html``, ``.xhtm``, ``.xhtml``, ``.vue``, ``.hbs``, ``.ejs``, ``.njk``, ``.json``, ``.yaml``, ``.yml``, ``.raml``, ``.xml`` [9]_" + Python [10]_,"2.7, 3.5, 3.6, 3.7, 3.8, 3.9, 3.10, 3.11, 3.12, 3.13",Not applicable,``.py`` + Ruby [11]_,"up to 3.3",Not applicable,"``.rb``, ``.erb``, ``.gemspec``, ``Gemfile``" + Rust [12]_,"Rust editions 2021 and 2024","Rust compiler","``.rs``, ``Cargo.toml``" + Swift [13]_ [14]_,"Swift 5.4-6.2","Swift compiler","``.swift``" + TypeScript [15]_,"2.6-5.9",Standard TypeScript compiler,"``.ts``, ``.tsx``, ``.mts``, ``.cts``" .. container:: footnote-group @@ -36,12 +36,13 @@ .. [3] Objective-C, Objective-C++, C++/CLI, and C++/CX are not supported. .. [4] Support for the clang-cl compiler is preliminary. .. [5] Support for the Arm Compiler (armcc) is preliminary. - .. [6] Builds that execute on Java 7 to 26 can be analyzed. The analysis understands standard language features in Java 8 to 26; "preview" and "incubator" features are not supported. Source code using Java language versions older than Java 8 are analyzed as Java 8 code. - .. [7] ECJ is supported when the build invokes it via the Maven Compiler plugin or the Takari Lifecycle plugin. - .. [8] JSX and Flow code, YAML, JSON, HTML, and XML files may also be analyzed with JavaScript files. - .. [9] The extractor requires Python 3 to run. To analyze Python 2.7 you should install both versions of Python. - .. [10] Requires glibc 2.17. - .. [11] Requires ``rustup`` and ``cargo`` to be installed. Features from nightly toolchains are not supported. - .. [12] Support for the analysis of Swift requires macOS. - .. [13] Embedded Swift is not supported. - .. [14] TypeScript analysis is performed by running the JavaScript extractor with TypeScript enabled. This is the default. + .. [6] Support for .NET 10 is preliminary and code that uses language features new to C# 14 is not yet fully supported for extraction and analysis. + .. [7] Builds that execute on Java 7 to 26 can be analyzed. The analysis understands standard language features in Java 8 to 26; "preview" and "incubator" features are not supported. Source code using Java language versions older than Java 8 are analyzed as Java 8 code. + .. [8] ECJ is supported when the build invokes it via the Maven Compiler plugin or the Takari Lifecycle plugin. + .. [9] JSX and Flow code, YAML, JSON, HTML, and XML files may also be analyzed with JavaScript files. + .. [10] The extractor requires Python 3 to run. To analyze Python 2.7 you should install both versions of Python. + .. [11] Requires glibc 2.17. + .. [12] Requires ``rustup`` and ``cargo`` to be installed. Features from nightly toolchains are not supported. + .. [13] Support for the analysis of Swift requires macOS. + .. [14] Embedded Swift is not supported. + .. [15] TypeScript analysis is performed by running the JavaScript extractor with TypeScript enabled. This is the default. diff --git a/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll b/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll index e1170aeda244..f0dc0cf0ca2b 100644 --- a/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll +++ b/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll @@ -4,13 +4,17 @@ * Provides classes and predicates for dealing with flow models specified * in data extensions and CSV format. * - * The CSV specification has the following columns: + * The extensible relations have the following columns: * - Sources: * `package; type; subtypes; name; signature; ext; output; kind; provenance` * - Sinks: * `package; type; subtypes; name; signature; ext; input; kind; provenance` * - Summaries: * `package; type; subtypes; name; signature; ext; input; output; kind; provenance` + * - Barriers: + * `package; type; subtypes; name; signature; ext; output; kind; provenance` + * - BarrierGuards: + * `package; type; subtypes; name; signature; ext; input; acceptingValue; kind; provenance` * - Neutrals: * `package; type; name; signature; kind; provenance` * A neutral is used to indicate that a callable is neutral with respect to flow (no summary), source (is not a source) or sink (is not a sink). @@ -78,11 +82,23 @@ * - "MapValue": Selects a value in a map. * - "Dereference": Selects the value referenced by a pointer. * - * 8. The `kind` column is a tag that can be referenced from QL to determine to + * 8. The `acceptingValue` column of barrier guard models specifies the condition + * under which the guard blocks flow. It can be one of "true" or "false". In + * the future "no-exception", "not-zero", "null", "not-null" may be supported. + * 9. The `kind` column is a tag that can be referenced from QL to determine to * which classes the interpreted elements should be added. For example, for * sources "remote" indicates a default remote flow source, and for summaries * "taint" indicates a default additional taint step and "value" indicates a * globally applicable value-preserving step. + * 10. The `provenance` column is a tag to indicate the origin and verification of a model. + * The format is {origin}-{verification} or just "manual" where the origin describes + * the origin of the model and verification describes how the model has been verified. + * Some examples are: + * - "df-generated": The model has been generated by the model generator tool. + * - "df-manual": The model has been generated by the model generator and verified by a human. + * - "manual": The model has been written by hand. + * This information is used in a heuristic for dataflow analysis to determine, if a + * model or source code should be used for determining flow. */ overlay[local?] module; @@ -250,11 +266,11 @@ module ModelValidation { result = "Unrecognized provenance description \"" + provenance + "\" in " + pred + " model." ) or - exists(string acceptingvalue | - barrierGuardModel(_, _, _, _, _, _, _, acceptingvalue, _, _, _) and - invalidAcceptingValue(acceptingvalue) and + exists(string acceptingValue | + barrierGuardModel(_, _, _, _, _, _, _, acceptingValue, _, _, _) and + invalidAcceptingValue(acceptingValue) and result = - "Unrecognized accepting value description \"" + acceptingvalue + + "Unrecognized accepting value description \"" + acceptingValue + "\" in barrier guard model." ) } @@ -462,13 +478,13 @@ private module Cached { private predicate barrierGuardChecks(DataFlow::Node g, Expr e, boolean gv, TKindModelPair kmp) { exists( - SourceSinkInterpretationInput::InterpretNode n, Public::AcceptingValue acceptingvalue, + SourceSinkInterpretationInput::InterpretNode n, Public::AcceptingValue acceptingValue, string kind, string model | - isBarrierGuardNode(n, acceptingvalue, kind, model) and + isBarrierGuardNode(n, acceptingValue, kind, model) and n.asNode().asExpr() = e and kmp = TMkPair(kind, model) and - gv = convertAcceptingValue(acceptingvalue) + gv = convertAcceptingValue(acceptingValue) | g.asExpr().(CallExpr).getAnArgument() = e // TODO: qualifier? ) diff --git a/go/ql/lib/semmle/go/dataflow/internal/ExternalFlowExtensions.qll b/go/ql/lib/semmle/go/dataflow/internal/ExternalFlowExtensions.qll index 5d43cf674c1c..ab2a241e14a6 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/ExternalFlowExtensions.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/ExternalFlowExtensions.qll @@ -35,7 +35,7 @@ extensible predicate barrierModel( */ extensible predicate barrierGuardModel( string package, string type, boolean subtypes, string name, string signature, string ext, - string input, string acceptingvalue, string kind, string provenance, QlBuiltins::ExtensionId madId + string input, string acceptingValue, string kind, string provenance, QlBuiltins::ExtensionId madId ); /** diff --git a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll index 240665bd492d..ff727286c3b4 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll @@ -174,13 +174,13 @@ module SourceSinkInterpretationInput implements } predicate barrierGuardElement( - Element e, string input, Public::AcceptingValue acceptingvalue, string kind, + Element e, string input, Public::AcceptingValue acceptingValue, string kind, Public::Provenance provenance, string model ) { exists( string package, string type, boolean subtypes, string name, string signature, string ext | - barrierGuardModel(package, type, subtypes, name, signature, ext, input, acceptingvalue, kind, + barrierGuardModel(package, type, subtypes, name, signature, ext, input, acceptingValue, kind, provenance, model) and e = interpretElement(package, type, subtypes, name, signature, ext) ) diff --git a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll index 1536c81aa083..a6a9347ca03a 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll @@ -4,13 +4,17 @@ * Provides classes and predicates for dealing with flow models specified * in data extensions and CSV format. * - * The CSV specification has the following columns: + * The extensible relations have the following columns: * - Sources: * `package; type; subtypes; name; signature; ext; output; kind; provenance` * - Sinks: * `package; type; subtypes; name; signature; ext; input; kind; provenance` * - Summaries: * `package; type; subtypes; name; signature; ext; input; output; kind; provenance` + * - Barriers: + * `package; type; subtypes; name; signature; ext; output; kind; provenance` + * - BarrierGuards: + * `package; type; subtypes; name; signature; ext; input; acceptingValue; kind; provenance` * - Neutrals: * `package; type; name; signature; kind; provenance` * A neutral is used to indicate that a callable is neutral with respect to flow (no summary), source (is not a source) or sink (is not a sink). @@ -69,14 +73,17 @@ * in the given range. The range is inclusive at both ends. * - "ReturnValue": Selects the return value of a call to the selected element. * - "Element": Selects the collection elements of the selected element. - * 8. The `kind` column is a tag that can be referenced from QL to determine to + * 8. The `acceptingValue` column of barrier guard models specifies the condition + * under which the guard blocks flow. It can be one of "true" or "false". In + * the future "no-exception", "not-zero", "null", "not-null" may be supported. + * 9. The `kind` column is a tag that can be referenced from QL to determine to * which classes the interpreted elements should be added. For example, for * sources "remote" indicates a default remote flow source, and for summaries * "taint" indicates a default additional taint step and "value" indicates a * globally applicable value-preserving step. For neutrals the kind can be `summary`, * `source` or `sink` to indicate that the neutral is neutral with respect to * flow (no summary), source (is not a source) or sink (is not a sink). - * 9. The `provenance` column is a tag to indicate the origin and verification of a model. + * 10. The `provenance` column is a tag to indicate the origin and verification of a model. * The format is {origin}-{verification} or just "manual" where the origin describes * the origin of the model and verification describes how the model has been verified. * Some examples are: @@ -358,11 +365,11 @@ module ModelValidation { result = "Unrecognized provenance description \"" + provenance + "\" in " + pred + " model." ) or - exists(string acceptingvalue | - barrierGuardModel(_, _, _, _, _, _, _, acceptingvalue, _, _, _) and - invalidAcceptingValue(acceptingvalue) and + exists(string acceptingValue | + barrierGuardModel(_, _, _, _, _, _, _, acceptingValue, _, _, _) and + invalidAcceptingValue(acceptingValue) and result = - "Unrecognized accepting value description \"" + acceptingvalue + + "Unrecognized accepting value description \"" + acceptingValue + "\" in barrier guard model." ) } @@ -583,13 +590,13 @@ private module Cached { private predicate barrierGuardChecks(Guard g, Expr e, GuardValue gv, TKindModelPair kmp) { exists( - SourceSinkInterpretationInput::InterpretNode n, AcceptingValue acceptingvalue, string kind, + SourceSinkInterpretationInput::InterpretNode n, AcceptingValue acceptingValue, string kind, string model | - isBarrierGuardNode(n, acceptingvalue, kind, model) and + isBarrierGuardNode(n, acceptingValue, kind, model) and n.asNode().asExpr() = e and kmp = TMkPair(kind, model) and - gv = convertAcceptingValue(acceptingvalue) + gv = convertAcceptingValue(acceptingValue) | g.(Call).getAnArgument() = e or g.(MethodCall).getQualifier() = e ) diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/ExternalFlowExtensions.qll b/java/ql/lib/semmle/code/java/dataflow/internal/ExternalFlowExtensions.qll index be474ad45352..3c6b003876de 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/ExternalFlowExtensions.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/ExternalFlowExtensions.qll @@ -35,7 +35,7 @@ extensible predicate barrierModel( */ extensible predicate barrierGuardModel( string package, string type, boolean subtypes, string name, string signature, string ext, - string input, string acceptingvalue, string kind, string provenance, QlBuiltins::ExtensionId madId + string input, string acceptingValue, string kind, string provenance, QlBuiltins::ExtensionId madId ); /** diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll b/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll index 64fa30c7d914..453b7ccae11c 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll @@ -282,7 +282,7 @@ module SourceSinkInterpretationInput implements } predicate barrierGuardElement( - Element e, string input, Public::AcceptingValue acceptingvalue, string kind, + Element e, string input, Public::AcceptingValue acceptingValue, string kind, Public::Provenance provenance, string model ) { exists( @@ -290,7 +290,7 @@ module SourceSinkInterpretationInput implements SourceOrSinkElement baseBarrier, string originalInput | barrierGuardModel(namespace, type, subtypes, name, signature, ext, originalInput, - acceptingvalue, kind, provenance, model) and + acceptingValue, kind, provenance, model) and baseBarrier = interpretElement(namespace, type, subtypes, name, signature, ext, _) and ( e = baseBarrier and input = originalInput diff --git a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll index 60fe40e716d0..155fb4b7c786 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll @@ -10,6 +10,10 @@ * `type, path, kind` * - Summaries: * `type, path, input, output, kind` + * - Barriers: + * `type, path, kind` + * - BarrierGuards: + * `type, path, acceptingValue, kind` * - Types: * `type1, type2, path` * @@ -42,7 +46,8 @@ * 3. The `input` and `output` columns specify how data enters and leaves the element selected by the * first `(type, path)` tuple. Both strings are `.`-separated access paths * of the same syntax as the `path` column. - * 4. The `kind` column is a tag that can be referenced from QL to determine to + * 4. The `acceptingValue` column of barrier guard models specifies which branch of the guard is blocking flow. It can be "true" or "false". + * 5. The `kind` column is a tag that can be referenced from QL to determine to * which classes the interpreted elements should be added. For example, for * sources `"remote"` indicates a default remote flow source, and for summaries * `"taint"` indicates a default additional taint step and `"value"` indicates a @@ -355,11 +360,11 @@ private predicate barrierModel(string type, string path, string kind, string mod /** Holds if a barrier guard model exists for the given parameters. */ private predicate barrierGuardModel( - string type, string path, string branch, string kind, string model + string type, string path, string acceptingValue, string kind, string model ) { // No deprecation adapter for barrier models, they were not around back then. exists(QlBuiltins::ExtensionId madId | - Extensions::barrierGuardModel(type, path, branch, kind, madId) and + Extensions::barrierGuardModel(type, path, acceptingValue, kind, madId) and model = "MaD:" + madId.toString() ) } @@ -783,16 +788,16 @@ module ModelOutput { } /** - * Holds if a barrier model contributed `barrier` with the given `kind` for the given `branch`. + * Holds if a barrier model contributed `barrier` with the given `kind` for the given `acceptingValue`. */ cached - API::Node getABarrierGuardNode(string kind, boolean branch, string model) { - exists(string type, string path, string branch_str | - branch = true and branch_str = "true" + API::Node getABarrierGuardNode(string kind, boolean acceptingValue, string model) { + exists(string type, string path, string acceptingValue_str | + acceptingValue = true and acceptingValue_str = "true" or - branch = false and branch_str = "false" + acceptingValue = false and acceptingValue_str = "false" | - barrierGuardModel(type, path, branch_str, kind, model) and + barrierGuardModel(type, path, acceptingValue_str, kind, model) and result = getNodeFromPath(type, path) ) } @@ -856,12 +861,12 @@ module ModelOutput { API::Node getABarrierNode(string kind) { result = getABarrierNode(kind, _) } /** - * Holds if an external model contributed `barrier-guard` with the given `kind` and `branch`. + * Holds if an external model contributed `barrier-guard` with the given `kind` and `acceptingValue`. * * INTERNAL: Do not use. */ - API::Node getABarrierGuardNode(string kind, boolean branch) { - result = getABarrierGuardNode(kind, branch, _) + API::Node getABarrierGuardNode(string kind, boolean acceptingValue) { + result = getABarrierGuardNode(kind, acceptingValue, _) } /** diff --git a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsExtensions.qll b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsExtensions.qll index 2a644aabb95d..8d8a4f5fd880 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsExtensions.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsExtensions.qll @@ -33,11 +33,11 @@ extensible predicate barrierModel( * of the given `kind` and `madId` is the data extension row number. * `path` is assumed to lead to a parameter of a call (possibly `self`), and * the call is guarding the parameter. - * `branch` is either `true` or `false`, indicating which branch of the guard - * is protecting the parameter. + * `acceptingValue` is either `true` or `false`, indicating which branch of + * the guard is protecting the parameter. */ extensible predicate barrierGuardModel( - string type, string path, string branch, string kind, QlBuiltins::ExtensionId madId + string type, string path, string acceptingValue, string kind, QlBuiltins::ExtensionId madId ); /** diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/MissingRateLimiting.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/MissingRateLimiting.qll index 5f4ad1b3d73e..8dd9c4831446 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/MissingRateLimiting.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/MissingRateLimiting.qll @@ -191,3 +191,21 @@ class RouteHandlerLimitedByRateLimiterFlexible extends RateLimitingMiddleware in private class FastifyRateLimiter extends RateLimitingMiddleware { FastifyRateLimiter() { this = DataFlow::moduleImport("fastify-rate-limit") } } + +/** + * An options object with a `rateLimit` config passed to a Fastify shorthand route method, + * such as `fastify.post('/path', { config: { rateLimit: { ... } } }, handler)`. + */ +private class FastifyPerRouteRateLimit extends RateLimitingMiddleware { + FastifyPerRouteRateLimit() { + exists(Fastify::RouteSetup setup | + not setup.getMethodName() = ["route", "addHook"] and + setup.getNumArgument() >= 3 and + this.flowsTo(setup.getArgument(1)) + | + exists(this.getAPropertySource("config").getAPropertySource("rateLimit")) + or + exists(this.getAPropertySource("rateLimit")) + ) + } +} diff --git a/javascript/ql/src/change-notes/2026-04-13-fastify-per-route-rate-limit.md b/javascript/ql/src/change-notes/2026-04-13-fastify-per-route-rate-limit.md new file mode 100644 index 000000000000..56d523885248 --- /dev/null +++ b/javascript/ql/src/change-notes/2026-04-13-fastify-per-route-rate-limit.md @@ -0,0 +1,5 @@ +--- +category: minorAnalysis +--- +* The query `js/missing-rate-limiting` now takes Fastify per-route + rate limiting into account. diff --git a/javascript/ql/test/query-tests/Security/CWE-770/MissingRateLimit/MissingRateLimiting.expected b/javascript/ql/test/query-tests/Security/CWE-770/MissingRateLimit/MissingRateLimiting.expected index 3d7bc2954eba..8d197d6e37f6 100644 --- a/javascript/ql/test/query-tests/Security/CWE-770/MissingRateLimit/MissingRateLimiting.expected +++ b/javascript/ql/test/query-tests/Security/CWE-770/MissingRateLimit/MissingRateLimiting.expected @@ -9,3 +9,4 @@ | tst.js:64:25:64:63 | functio ... req); } | This route handler performs $@, but is not rate-limited. | tst.js:64:46:64:60 | verifyUser(req) | authorization | | tst.js:76:25:76:53 | catchAs ... ndler1) | This route handler performs $@, but is not rate-limited. | tst.js:14:40:14:46 | login() | authorization | | tst.js:88:24:88:40 | expensiveHandler1 | This route handler performs $@, but is not rate-limited. | tst.js:14:40:14:46 | login() | authorization | +| tst.js:112:28:112:44 | expensiveHandler1 | This route handler performs $@, but is not rate-limited. | tst.js:14:40:14:46 | login() | authorization | diff --git a/javascript/ql/test/query-tests/Security/CWE-770/MissingRateLimit/tst.js b/javascript/ql/test/query-tests/Security/CWE-770/MissingRateLimit/tst.js index 4f778afef684..5b4312bbbe0e 100644 --- a/javascript/ql/test/query-tests/Security/CWE-770/MissingRateLimit/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-770/MissingRateLimit/tst.js @@ -88,3 +88,25 @@ const fastifyApp = require('fastify')(); fastifyApp.get('/foo', expensiveHandler1); // $ Alert fastifyApp.register(require('fastify-rate-limit')); fastifyApp.get('/bar', expensiveHandler1); + +// Fastify per-route rate limiting via config.rateLimit +const fastifyApp2 = require('fastify')(); +fastifyApp2.register(require('@fastify/rate-limit')); + +fastifyApp2.post('/login', { + config: { + rateLimit: { + max: 3, + timeWindow: '1 minute' + } + } +}, expensiveHandler1); // OK - has per-route rateLimit config + +fastifyApp2.post('/signup', { + rateLimit: { + max: 5, + timeWindow: '1 minute' + } +}, expensiveHandler1); // OK - has per-route rateLimit directly in options + +fastifyApp2.post('/other', expensiveHandler1); // $ Alert - no rate limiting diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/ClassInstanceFlow.qll b/python/ql/lib/semmle/python/dataflow/new/internal/ClassInstanceFlow.qll new file mode 100644 index 000000000000..2b907c243d26 --- /dev/null +++ b/python/ql/lib/semmle/python/dataflow/new/internal/ClassInstanceFlow.qll @@ -0,0 +1,138 @@ +/** + * Provides a reusable data-flow configuration for tracking class instances + * through global data-flow with full path support. + * + * This module is designed for quality queries that check whether instances + * of certain classes reach operations that require a specific interface + * (e.g., `__contains__`, `__iter__`, `__hash__`). + * + * The configuration uses two flow states: + * - `TrackingClass`: tracking a reference to the class itself + * - `TrackingInstance`: tracking an instance of the class + * + * At instantiation points (e.g., `cls()`), the state transitions from + * `TrackingClass` to `TrackingInstance`. Sinks are only matched in the + * `TrackingInstance` state. + */ + +private import python +import semmle.python.dataflow.new.DataFlow +private import semmle.python.dataflow.new.internal.DataFlowDispatch +private import semmle.python.ApiGraphs + +/** A flow state for tracking class references and their instances. */ +abstract class ClassInstanceFlowState extends string { + bindingset[this] + ClassInstanceFlowState() { any() } +} + +/** A state signifying that the tracked value is a reference to the class itself. */ +class TrackingClass extends ClassInstanceFlowState { + TrackingClass() { this = "TrackingClass" } +} + +/** A state signifying that the tracked value is an instance of the class. */ +class TrackingInstance extends ClassInstanceFlowState { + TrackingInstance() { this = "TrackingInstance" } +} + +/** + * Signature module for parameterizing `ClassInstanceFlow` per query. + */ +signature module ClassInstanceFlowSig { + /** Holds if `cls` is a class whose instances should be tracked to sinks. */ + predicate isRelevantClass(Class cls); + + /** Holds if `sink` is a location where reaching instances indicate a violation. */ + predicate isInstanceSink(DataFlow::Node sink); + + /** + * Holds if an `isinstance` check against `checkedType` should act as a barrier, + * suppressing alerts when the instance has been verified to have the expected interface. + */ + predicate isGuardType(DataFlow::Node checkedType); +} + +/** + * Constructs a global data-flow configuration for tracking instances of + * relevant classes from their definition to violation sinks. + */ +module ClassInstanceFlow { + /** + * Holds if `guard` is an `isinstance` call checking `node` against a type + * that should suppress the alert. + */ + private predicate isinstanceGuard(DataFlow::GuardNode guard, ControlFlowNode node, boolean branch) { + exists(DataFlow::CallCfgNode isinstance_call | + isinstance_call = API::builtin("isinstance").getACall() and + isinstance_call.getArg(0).asCfgNode() = node and + ( + Sig::isGuardType(isinstance_call.getArg(1)) + or + // Also handle tuples of types: isinstance(x, (T1, T2)) + Sig::isGuardType(DataFlow::exprNode(isinstance_call.getArg(1).asExpr().(Tuple).getAnElt())) + ) and + guard = isinstance_call.asCfgNode() and + branch = true + ) + } + + private module Config implements DataFlow::StateConfigSig { + class FlowState = ClassInstanceFlowState; + + predicate isSource(DataFlow::Node source, FlowState state) { + exists(ClassExpr ce | + Sig::isRelevantClass(ce.getInnerScope()) and + source.asExpr() = ce and + state instanceof TrackingClass + ) + } + + predicate isSink(DataFlow::Node sink, FlowState state) { + Sig::isInstanceSink(sink) and + state instanceof TrackingInstance + } + + predicate isBarrier(DataFlow::Node node) { + node = DataFlow::BarrierGuard::getABarrierNode() + } + + /** + * Holds if `call` is inside a branch that is guarded by a condition + * depending on a parameter of the enclosing function. In such cases, + * the instantiation is contextual — it only happens for certain argument + * values — and we cannot determine from the call site whether it will + * actually execute. + */ + private predicate parameterGuardedCall(CallNode call) { + exists(ConditionBlock guard, DataFlow::ParameterNode param, DataFlow::Node guardSubExpr | + guard.controls(call.getBasicBlock(), _) and + param.getScope() = call.getScope() and + guardSubExpr.asCfgNode() = guard.getLastNode().getAChild*() and + DataFlow::localFlow(param, guardSubExpr) + ) + } + + predicate isAdditionalFlowStep( + DataFlow::Node nodeFrom, FlowState stateFrom, DataFlow::Node nodeTo, FlowState stateTo + ) { + // Instantiation: class reference at the call function position + // flows to the call result as an instance. + stateFrom instanceof TrackingClass and + stateTo instanceof TrackingInstance and + exists(CallNode call | + nodeFrom.asCfgNode() = call.getFunction() and + nodeTo.asCfgNode() = call and + // Exclude decorator applications, where the result is a proxy + // rather than a typical instance. + not call.getNode() = any(FunctionExpr fe).getADecoratorCall() and + // Exclude instantiations guarded by parameter-dependent conditions, + // since we cannot determine from the call site whether the guard + // will be satisfied. + not parameterGuardedCall(call) + ) + } + } + + module Flow = DataFlow::GlobalWithState; +} diff --git a/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll b/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll index 60fe40e716d0..155fb4b7c786 100644 --- a/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll +++ b/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll @@ -10,6 +10,10 @@ * `type, path, kind` * - Summaries: * `type, path, input, output, kind` + * - Barriers: + * `type, path, kind` + * - BarrierGuards: + * `type, path, acceptingValue, kind` * - Types: * `type1, type2, path` * @@ -42,7 +46,8 @@ * 3. The `input` and `output` columns specify how data enters and leaves the element selected by the * first `(type, path)` tuple. Both strings are `.`-separated access paths * of the same syntax as the `path` column. - * 4. The `kind` column is a tag that can be referenced from QL to determine to + * 4. The `acceptingValue` column of barrier guard models specifies which branch of the guard is blocking flow. It can be "true" or "false". + * 5. The `kind` column is a tag that can be referenced from QL to determine to * which classes the interpreted elements should be added. For example, for * sources `"remote"` indicates a default remote flow source, and for summaries * `"taint"` indicates a default additional taint step and `"value"` indicates a @@ -355,11 +360,11 @@ private predicate barrierModel(string type, string path, string kind, string mod /** Holds if a barrier guard model exists for the given parameters. */ private predicate barrierGuardModel( - string type, string path, string branch, string kind, string model + string type, string path, string acceptingValue, string kind, string model ) { // No deprecation adapter for barrier models, they were not around back then. exists(QlBuiltins::ExtensionId madId | - Extensions::barrierGuardModel(type, path, branch, kind, madId) and + Extensions::barrierGuardModel(type, path, acceptingValue, kind, madId) and model = "MaD:" + madId.toString() ) } @@ -783,16 +788,16 @@ module ModelOutput { } /** - * Holds if a barrier model contributed `barrier` with the given `kind` for the given `branch`. + * Holds if a barrier model contributed `barrier` with the given `kind` for the given `acceptingValue`. */ cached - API::Node getABarrierGuardNode(string kind, boolean branch, string model) { - exists(string type, string path, string branch_str | - branch = true and branch_str = "true" + API::Node getABarrierGuardNode(string kind, boolean acceptingValue, string model) { + exists(string type, string path, string acceptingValue_str | + acceptingValue = true and acceptingValue_str = "true" or - branch = false and branch_str = "false" + acceptingValue = false and acceptingValue_str = "false" | - barrierGuardModel(type, path, branch_str, kind, model) and + barrierGuardModel(type, path, acceptingValue_str, kind, model) and result = getNodeFromPath(type, path) ) } @@ -856,12 +861,12 @@ module ModelOutput { API::Node getABarrierNode(string kind) { result = getABarrierNode(kind, _) } /** - * Holds if an external model contributed `barrier-guard` with the given `kind` and `branch`. + * Holds if an external model contributed `barrier-guard` with the given `kind` and `acceptingValue`. * * INTERNAL: Do not use. */ - API::Node getABarrierGuardNode(string kind, boolean branch) { - result = getABarrierGuardNode(kind, branch, _) + API::Node getABarrierGuardNode(string kind, boolean acceptingValue) { + result = getABarrierGuardNode(kind, acceptingValue, _) } /** diff --git a/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModelsExtensions.qll b/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModelsExtensions.qll index 2a644aabb95d..8d8a4f5fd880 100644 --- a/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModelsExtensions.qll +++ b/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModelsExtensions.qll @@ -33,11 +33,11 @@ extensible predicate barrierModel( * of the given `kind` and `madId` is the data extension row number. * `path` is assumed to lead to a parameter of a call (possibly `self`), and * the call is guarding the parameter. - * `branch` is either `true` or `false`, indicating which branch of the guard - * is protecting the parameter. + * `acceptingValue` is either `true` or `false`, indicating which branch of + * the guard is protecting the parameter. */ extensible predicate barrierGuardModel( - string type, string path, string branch, string kind, QlBuiltins::ExtensionId madId + string type, string path, string acceptingValue, string kind, QlBuiltins::ExtensionId madId ); /** diff --git a/python/ql/src/Expressions/ContainsNonContainer.ql b/python/ql/src/Expressions/ContainsNonContainer.ql index de8c38795675..ad2e10de017b 100644 --- a/python/ql/src/Expressions/ContainsNonContainer.ql +++ b/python/ql/src/Expressions/ContainsNonContainer.ql @@ -1,7 +1,7 @@ /** * @name Membership test with a non-container * @description A membership test, such as 'item in sequence', with a non-container on the right hand side will raise a 'TypeError'. - * @kind problem + * @kind path-problem * @tags quality * reliability * correctness @@ -12,25 +12,47 @@ */ import python -private import LegacyPointsTo +import semmle.python.dataflow.new.DataFlow +private import semmle.python.dataflow.new.internal.DataFlowDispatch +private import semmle.python.dataflow.new.internal.ClassInstanceFlow +private import semmle.python.ApiGraphs -predicate rhs_in_expr(ControlFlowNode rhs, Compare cmp) { - exists(Cmpop op, int i | cmp.getOp(i) = op and cmp.getComparator(i) = rhs.getNode() | +predicate rhs_in_expr(Expr rhs, Compare cmp) { + exists(Cmpop op, int i | cmp.getOp(i) = op and cmp.getComparator(i) = rhs | op instanceof In or op instanceof NotIn ) } +module ContainsNonContainerSig implements ClassInstanceFlowSig { + predicate isRelevantClass(Class cls) { + not DuckTyping::isContainer(cls) and + not DuckTyping::hasUnresolvedBase(getADirectSuperclass*(cls)) and + not exists(CallNode setattr_call | + setattr_call.getFunction().(NameNode).getId() = "setattr" and + setattr_call.getArg(0).(NameNode).getId() = cls.getName() and + setattr_call.getScope() = cls.getScope() + ) + } + + predicate isInstanceSink(DataFlow::Node sink) { rhs_in_expr(sink.asExpr(), _) } + + predicate isGuardType(DataFlow::Node checkedType) { + checkedType = + API::builtin(["list", "tuple", "set", "frozenset", "dict", "str", "bytes", "bytearray"]) + .getAValueReachableFromSource() + } +} + +module ContainsNonContainerFlow = ClassInstanceFlow; + +import ContainsNonContainerFlow::Flow::PathGraph + from - ControlFlowNodeWithPointsTo non_seq, Compare cmp, Value v, ClassValue cls, ControlFlowNode origin + ContainsNonContainerFlow::Flow::PathNode source, ContainsNonContainerFlow::Flow::PathNode sink, + ClassExpr ce where - rhs_in_expr(non_seq, cmp) and - non_seq.pointsTo(_, v, origin) and - v.getClass() = cls and - not Types::failedInference(cls, _) and - not cls.hasAttribute("__contains__") and - not cls.hasAttribute("__iter__") and - not cls.hasAttribute("__getitem__") and - not cls = ClassValue::nonetype() and - not cls = Value::named("types.MappingProxyType") -select cmp, "This test may raise an Exception as the $@ may be of non-container class $@.", origin, - "target", cls, cls.getName() + ContainsNonContainerFlow::Flow::flowPath(source, sink) and + source.getNode().asExpr() = ce +select sink.getNode(), source, sink, + "This test may raise an Exception as the $@ may be of non-container class $@.", source.getNode(), + "target", ce.getInnerScope(), ce.getInnerScope().getName() diff --git a/python/ql/src/Statements/ModificationOfLocals.ql b/python/ql/src/Statements/ModificationOfLocals.ql index e4791a410f7a..f32ddcf78849 100644 --- a/python/ql/src/Statements/ModificationOfLocals.ql +++ b/python/ql/src/Statements/ModificationOfLocals.ql @@ -12,10 +12,10 @@ */ import python -private import LegacyPointsTo +private import semmle.python.ApiGraphs -predicate originIsLocals(ControlFlowNodeWithPointsTo n) { - n.pointsTo(_, _, Value::named("locals").getACall()) +predicate originIsLocals(ControlFlowNode n) { + API::builtin("locals").getReturn().getAValueReachableFromSource().asCfgNode() = n } predicate modification_of_locals(ControlFlowNode f) { @@ -37,5 +37,8 @@ where // in module level scope `locals() == globals()` // see https://docs.python.org/3/library/functions.html#locals // FP report in https://github.com/github/codeql/issues/6674 - not a.getScope() instanceof ModuleScope + not a.getScope() instanceof Module and + // in class level scope `locals()` reflects the class namespace, + // so modifications do take effect. + not a.getScope() instanceof Class select a, "Modification of the locals() dictionary will have no effect on the local variables." diff --git a/python/ql/src/Statements/UnreachableCode.ql b/python/ql/src/Statements/UnreachableCode.ql index 55582ed2f061..200e073cff6b 100644 --- a/python/ql/src/Statements/UnreachableCode.ql +++ b/python/ql/src/Statements/UnreachableCode.ql @@ -13,7 +13,7 @@ */ import python -private import LegacyPointsTo +private import semmle.python.ApiGraphs predicate typing_import(ImportingStmt is) { exists(Module m | @@ -34,11 +34,7 @@ predicate unique_yield(Stmt s) { /** Holds if `contextlib.suppress` may be used in the same scope as `s` */ predicate suppression_in_scope(Stmt s) { exists(With w | - w.getContextExpr() - .(Call) - .getFunc() - .(ExprWithPointsTo) - .pointsTo(Value::named("contextlib.suppress")) and + w.getContextExpr() = API::moduleImport("contextlib").getMember("suppress").getACall().asExpr() and w.getScope() = s.getScope() ) } diff --git a/python/ql/src/Statements/UnusedExceptionObject.ql b/python/ql/src/Statements/UnusedExceptionObject.ql index 9a6a3650b7e6..890cdc963aca 100644 --- a/python/ql/src/Statements/UnusedExceptionObject.ql +++ b/python/ql/src/Statements/UnusedExceptionObject.ql @@ -12,11 +12,49 @@ */ import python -private import LegacyPointsTo +private import semmle.python.dataflow.new.internal.DataFlowDispatch +private import semmle.python.dataflow.new.internal.Builtins +private import semmle.python.ApiGraphs -from Call call, ClassValue ex +/** + * Holds if `cls` is a user-defined exception class, i.e. it transitively + * extends one of the builtin exception base classes. + */ +predicate isUserDefinedExceptionClass(Class cls) { + cls.getABase() = + API::builtin(["BaseException", "Exception"]).getAValueReachableFromSource().asExpr() + or + isUserDefinedExceptionClass(getADirectSuperclass(cls)) +} + +/** + * Gets the name of a builtin exception class. + */ +string getBuiltinExceptionName() { + result = Builtins::getBuiltinName() and + ( + result.matches("%Error") or + result.matches("%Exception") or + result.matches("%Warning") or + result = + ["GeneratorExit", "KeyboardInterrupt", "StopIteration", "StopAsyncIteration", "SystemExit"] + ) +} + +/** + * Holds if `call` is an instantiation of an exception class. + */ +predicate isExceptionInstantiation(Call call) { + exists(Class cls | + classTracker(cls).asExpr() = call.getFunc() and + isUserDefinedExceptionClass(cls) + ) + or + call.getFunc() = API::builtin(getBuiltinExceptionName()).getAValueReachableFromSource().asExpr() +} + +from Call call where - call.getFunc().(ExprWithPointsTo).pointsTo(ex) and - ex.getASuperType() = ClassValue::exception() and + isExceptionInstantiation(call) and exists(ExprStmt s | s.getValue() = call) select call, "Instantiating an exception, but not raising it, has no effect." diff --git a/python/ql/src/Statements/UseOfExit.ql b/python/ql/src/Statements/UseOfExit.ql index 437ff93b5371..2310a839f67b 100644 --- a/python/ql/src/Statements/UseOfExit.ql +++ b/python/ql/src/Statements/UseOfExit.ql @@ -12,10 +12,12 @@ */ import python -private import LegacyPointsTo +private import semmle.python.ApiGraphs from CallNode call, string name -where call.getFunction().(ControlFlowNodeWithPointsTo).pointsTo(Value::siteQuitter(name)) +where + name = ["exit", "quit"] and + call = API::builtin(name).getACall().asCfgNode() select call, "The '" + name + "' site.Quitter object may not exist if the 'site' module is not loaded or is modified." diff --git a/python/ql/test/query-tests/Expressions/general/ContainsNonContainer.expected b/python/ql/test/query-tests/Expressions/general/ContainsNonContainer.expected index cf6d78d0d36b..eca57f44333a 100644 --- a/python/ql/test/query-tests/Expressions/general/ContainsNonContainer.expected +++ b/python/ql/test/query-tests/Expressions/general/ContainsNonContainer.expected @@ -1,2 +1,22 @@ -| expressions_test.py:89:8:89:15 | Compare | This test may raise an Exception as the $@ may be of non-container class $@. | expressions_test.py:88:11:88:17 | ControlFlowNode for XIter() | target | expressions_test.py:77:1:77:20 | class XIter | XIter | -| expressions_test.py:91:8:91:19 | Compare | This test may raise an Exception as the $@ may be of non-container class $@. | expressions_test.py:88:11:88:17 | ControlFlowNode for XIter() | target | expressions_test.py:77:1:77:20 | class XIter | XIter | +edges +| expressions_test.py:77:1:77:20 | ControlFlowNode for ClassExpr | expressions_test.py:77:7:77:11 | ControlFlowNode for XIter | provenance | | +| expressions_test.py:77:7:77:11 | ControlFlowNode for XIter | expressions_test.py:88:11:88:15 | ControlFlowNode for XIter | provenance | | +| expressions_test.py:88:5:88:7 | ControlFlowNode for seq | expressions_test.py:89:13:89:15 | ControlFlowNode for seq | provenance | | +| expressions_test.py:88:5:88:7 | ControlFlowNode for seq | expressions_test.py:91:17:91:19 | ControlFlowNode for seq | provenance | | +| expressions_test.py:88:5:88:7 | ControlFlowNode for seq | expressions_test.py:91:17:91:19 | ControlFlowNode for seq | provenance | | +| expressions_test.py:88:11:88:15 | ControlFlowNode for XIter | expressions_test.py:88:11:88:17 | ControlFlowNode for XIter() | provenance | Config | +| expressions_test.py:88:11:88:17 | ControlFlowNode for XIter() | expressions_test.py:88:5:88:7 | ControlFlowNode for seq | provenance | | +nodes +| expressions_test.py:77:1:77:20 | ControlFlowNode for ClassExpr | semmle.label | ControlFlowNode for ClassExpr | +| expressions_test.py:77:7:77:11 | ControlFlowNode for XIter | semmle.label | ControlFlowNode for XIter | +| expressions_test.py:88:5:88:7 | ControlFlowNode for seq | semmle.label | ControlFlowNode for seq | +| expressions_test.py:88:11:88:15 | ControlFlowNode for XIter | semmle.label | ControlFlowNode for XIter | +| expressions_test.py:88:11:88:17 | ControlFlowNode for XIter() | semmle.label | ControlFlowNode for XIter() | +| expressions_test.py:89:13:89:15 | ControlFlowNode for seq | semmle.label | ControlFlowNode for seq | +| expressions_test.py:91:17:91:19 | ControlFlowNode for seq | semmle.label | ControlFlowNode for seq | +| expressions_test.py:91:17:91:19 | ControlFlowNode for seq | semmle.label | ControlFlowNode for seq | +subpaths +#select +| expressions_test.py:89:13:89:15 | ControlFlowNode for seq | expressions_test.py:77:1:77:20 | ControlFlowNode for ClassExpr | expressions_test.py:89:13:89:15 | ControlFlowNode for seq | This test may raise an Exception as the $@ may be of non-container class $@. | expressions_test.py:77:1:77:20 | ControlFlowNode for ClassExpr | target | expressions_test.py:77:1:77:20 | Class XIter | XIter | +| expressions_test.py:91:17:91:19 | ControlFlowNode for seq | expressions_test.py:77:1:77:20 | ControlFlowNode for ClassExpr | expressions_test.py:91:17:91:19 | ControlFlowNode for seq | This test may raise an Exception as the $@ may be of non-container class $@. | expressions_test.py:77:1:77:20 | ControlFlowNode for ClassExpr | target | expressions_test.py:77:1:77:20 | Class XIter | XIter | +| expressions_test.py:91:17:91:19 | ControlFlowNode for seq | expressions_test.py:77:1:77:20 | ControlFlowNode for ClassExpr | expressions_test.py:91:17:91:19 | ControlFlowNode for seq | This test may raise an Exception as the $@ may be of non-container class $@. | expressions_test.py:77:1:77:20 | ControlFlowNode for ClassExpr | target | expressions_test.py:77:1:77:20 | Class XIter | XIter | diff --git a/python/ql/test/query-tests/Expressions/general/expressions_test.py b/python/ql/test/query-tests/Expressions/general/expressions_test.py index 5e07b58e2041..f9d522d328b3 100644 --- a/python/ql/test/query-tests/Expressions/general/expressions_test.py +++ b/python/ql/test/query-tests/Expressions/general/expressions_test.py @@ -279,3 +279,62 @@ def local(): def apply(f): pass apply(foo)([1]) + +# Class used as a decorator: the runtime value at attribute access is the +# function's return value, not the decorator class instance. +class cached_property(object): + def __init__(self, func): + self.func = func + def __get__(self, obj, cls): + val = self.func(obj) + setattr(obj, self.func.__name__, val) + return val + +class MyForm(object): + @cached_property + def changed_data(self): + return [1, 2, 3] + +def test_decorator_class(form): + f = MyForm() + # OK: cached_property is a descriptor; the actual runtime value is a list. + if "name" in f.changed_data: + pass + +# Class with dynamically added methods via setattr: we cannot statically +# determine its full interface, so we should not flag it. +class DynamicProxy(object): + def __init__(self, args): + self._args = args + +for method_name in ["__contains__", "__iter__", "__len__"]: + def wrapper(self, *args, __method_name=method_name): + pass + setattr(DynamicProxy, method_name, wrapper) + +def test_dynamic_methods(): + proxy = DynamicProxy(()) + # OK: __contains__ is added dynamically via setattr. + if "name" in proxy: + pass + +# isinstance guard should suppress non-container warning +def guarded_contains(x): + obj = XIter() + if isinstance(obj, dict): + if x in obj: # OK: guarded by isinstance + pass + +def guarded_contains_tuple(x): + obj = XIter() + if isinstance(obj, (list, dict, set)): + if x in obj: # OK: guarded by isinstance with tuple of types + pass + +# Negated isinstance guard: early return when NOT a container +def guarded_contains_negated(x): + obj = XIter() + if not isinstance(obj, dict): + return + if x in obj: # OK: guarded by negated isinstance + early return + pass diff --git a/python/ql/test/query-tests/Statements/general/test.py b/python/ql/test/query-tests/Statements/general/test.py index eee63fa89e88..a5848f7c718d 100644 --- a/python/ql/test/query-tests/Statements/general/test.py +++ b/python/ql/test/query-tests/Statements/general/test.py @@ -174,3 +174,9 @@ def assert_ok(seq): # False positive. ODASA-8042. Fixed in PR #2401. class false_positive: e = (x for x in []) + +# In class-level scope `locals()` reflects the class namespace, +# so modifications do take effect. +class MyClass: + locals()['x'] = 43 # OK + y = x diff --git a/python/scripts/create-extractor-pack.sh b/python/scripts/create-extractor-pack.sh new file mode 100755 index 000000000000..ae9248f6ccfc --- /dev/null +++ b/python/scripts/create-extractor-pack.sh @@ -0,0 +1,67 @@ +#!/bin/bash +# +# Build a local Python extractor pack from source. +# +# Usage with the CodeQL CLI (run from the repository root): +# +# codeql database create -l python -s --search-path . +# codeql test run --search-path . python/ql/test/ +# +set -eux + +if [[ "$OSTYPE" == "linux-gnu"* ]]; then + platform="linux64" +elif [[ "$OSTYPE" == "darwin"* ]]; then + platform="osx64" +else + echo "Unknown OS" + exit 1 +fi + +cd "$(dirname "$0")/.." + +# Build the tsg-python Rust binary +(cd extractor/tsg-python && cargo build --release) +tsg_bin="extractor/tsg-python/target/release/tsg-python" + +# Generate python3src.zip from the Python extractor source. +# make_zips.py creates the zip in the source directory and then copies it to the +# given output directory. We use a temporary directory to avoid a same-file copy +# error, then move the zip back. +tmpdir=$(mktemp -d) +trap 'rm -rf "$tmpdir"' EXIT +(cd extractor && python3 make_zips.py "$tmpdir") +cp "$tmpdir/python3src.zip" extractor/python3src.zip + +# Assemble the extractor pack +rm -rf extractor-pack +mkdir -p extractor-pack/tools/${platform} + +# Root-level metadata and schema files +cp codeql-extractor.yml extractor-pack/ +cp ql/lib/semmlecode.python.dbscheme extractor-pack/ +cp ql/lib/semmlecode.python.dbscheme.stats extractor-pack/ + +# Python extractor engine files (into tools/) +cp extractor/python_tracer.py extractor-pack/tools/ +cp extractor/index.py extractor-pack/tools/ +cp extractor/setup.py extractor-pack/tools/ +cp extractor/convert_setup.py extractor-pack/tools/ +cp extractor/get_venv_lib.py extractor-pack/tools/ +cp extractor/imp.py extractor-pack/tools/ +cp extractor/LICENSE-PSF.md extractor-pack/tools/ +cp extractor/python3src.zip extractor-pack/tools/ +cp -r extractor/data extractor-pack/tools/ + +# Shell tool scripts (autobuild, pre-finalize, lgtm-scripts) +cp tools/autobuild.sh extractor-pack/tools/ +cp tools/autobuild.cmd extractor-pack/tools/ +cp tools/pre-finalize.sh extractor-pack/tools/ +cp tools/pre-finalize.cmd extractor-pack/tools/ +cp -r tools/lgtm-scripts extractor-pack/tools/ + +# Downgrades +cp -r downgrades extractor-pack/ + +# Platform-specific Rust binary +cp "${tsg_bin}" extractor-pack/tools/${platform}/tsg-python diff --git a/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModels.qll b/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModels.qll index 60fe40e716d0..155fb4b7c786 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModels.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModels.qll @@ -10,6 +10,10 @@ * `type, path, kind` * - Summaries: * `type, path, input, output, kind` + * - Barriers: + * `type, path, kind` + * - BarrierGuards: + * `type, path, acceptingValue, kind` * - Types: * `type1, type2, path` * @@ -42,7 +46,8 @@ * 3. The `input` and `output` columns specify how data enters and leaves the element selected by the * first `(type, path)` tuple. Both strings are `.`-separated access paths * of the same syntax as the `path` column. - * 4. The `kind` column is a tag that can be referenced from QL to determine to + * 4. The `acceptingValue` column of barrier guard models specifies which branch of the guard is blocking flow. It can be "true" or "false". + * 5. The `kind` column is a tag that can be referenced from QL to determine to * which classes the interpreted elements should be added. For example, for * sources `"remote"` indicates a default remote flow source, and for summaries * `"taint"` indicates a default additional taint step and `"value"` indicates a @@ -355,11 +360,11 @@ private predicate barrierModel(string type, string path, string kind, string mod /** Holds if a barrier guard model exists for the given parameters. */ private predicate barrierGuardModel( - string type, string path, string branch, string kind, string model + string type, string path, string acceptingValue, string kind, string model ) { // No deprecation adapter for barrier models, they were not around back then. exists(QlBuiltins::ExtensionId madId | - Extensions::barrierGuardModel(type, path, branch, kind, madId) and + Extensions::barrierGuardModel(type, path, acceptingValue, kind, madId) and model = "MaD:" + madId.toString() ) } @@ -783,16 +788,16 @@ module ModelOutput { } /** - * Holds if a barrier model contributed `barrier` with the given `kind` for the given `branch`. + * Holds if a barrier model contributed `barrier` with the given `kind` for the given `acceptingValue`. */ cached - API::Node getABarrierGuardNode(string kind, boolean branch, string model) { - exists(string type, string path, string branch_str | - branch = true and branch_str = "true" + API::Node getABarrierGuardNode(string kind, boolean acceptingValue, string model) { + exists(string type, string path, string acceptingValue_str | + acceptingValue = true and acceptingValue_str = "true" or - branch = false and branch_str = "false" + acceptingValue = false and acceptingValue_str = "false" | - barrierGuardModel(type, path, branch_str, kind, model) and + barrierGuardModel(type, path, acceptingValue_str, kind, model) and result = getNodeFromPath(type, path) ) } @@ -856,12 +861,12 @@ module ModelOutput { API::Node getABarrierNode(string kind) { result = getABarrierNode(kind, _) } /** - * Holds if an external model contributed `barrier-guard` with the given `kind` and `branch`. + * Holds if an external model contributed `barrier-guard` with the given `kind` and `acceptingValue`. * * INTERNAL: Do not use. */ - API::Node getABarrierGuardNode(string kind, boolean branch) { - result = getABarrierGuardNode(kind, branch, _) + API::Node getABarrierGuardNode(string kind, boolean acceptingValue) { + result = getABarrierGuardNode(kind, acceptingValue, _) } /** diff --git a/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModelsExtensions.qll b/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModelsExtensions.qll index 2a644aabb95d..8d8a4f5fd880 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModelsExtensions.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModelsExtensions.qll @@ -33,11 +33,11 @@ extensible predicate barrierModel( * of the given `kind` and `madId` is the data extension row number. * `path` is assumed to lead to a parameter of a call (possibly `self`), and * the call is guarding the parameter. - * `branch` is either `true` or `false`, indicating which branch of the guard - * is protecting the parameter. + * `acceptingValue` is either `true` or `false`, indicating which branch of + * the guard is protecting the parameter. */ extensible predicate barrierGuardModel( - string type, string path, string branch, string kind, QlBuiltins::ExtensionId madId + string type, string path, string acceptingValue, string kind, QlBuiltins::ExtensionId madId ); /** diff --git a/rust/ql/lib/codeql/files/FileSystem.qll b/rust/ql/lib/codeql/files/FileSystem.qll index cfab33b9a440..93ff7be604e0 100644 --- a/rust/ql/lib/codeql/files/FileSystem.qll +++ b/rust/ql/lib/codeql/files/FileSystem.qll @@ -45,13 +45,16 @@ extensible predicate additionalExternalFile(string relativePath); /** A file. */ class File extends Container, Impl::File { + pragma[nomagic] + private predicate isAdditionalExternalFile() { additionalExternalFile(this.getRelativePath()) } + /** * Holds if this file was extracted from the source code of the target project * (rather than another location such as inside a dependency). */ predicate fromSource() { exists(ExtractorStep s | s.getAction() = "Extract" and s.getFile() = this) and - not additionalExternalFile(this.getRelativePath()) + not this.isAdditionalExternalFile() } /** diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll index 27773758fc46..7c1fdd8cf781 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll @@ -1183,12 +1183,12 @@ private module Cached { exists( FlowSummaryImpl::Public::BarrierGuardElement b, FlowSummaryImpl::Private::SummaryComponentStack stack, - FlowSummaryImpl::Public::AcceptingValue acceptingvalue, string kind, string model + FlowSummaryImpl::Public::AcceptingValue acceptingValue, string kind, string model | - FlowSummaryImpl::Private::barrierGuardSpec(b, stack, acceptingvalue, kind, model) and + FlowSummaryImpl::Private::barrierGuardSpec(b, stack, acceptingValue, kind, model) and e = FlowSummaryImpl::StepsInput::getSinkNode(b, stack.headOfSingleton()).asExpr() and kmp = TMkPair(kind, model) and - gv = convertAcceptingValue(acceptingvalue) and + gv = convertAcceptingValue(acceptingValue) and g = b.getCall() ) } diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qll b/rust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qll index 4d28dd8de812..2b3ecf51fe40 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qll @@ -9,6 +9,13 @@ * `path; input; kind; provenance` * - Summaries: * `path; input; output; kind; provenance` + * - Barriers: + * `path; output; kind; provenance` + * - BarrierGuards: + * `path; input; acceptingValue; kind; provenance` + * - Neutrals: + * `path; kind; provenance` + * A neutral is used to indicate that a callable is neutral with respect to flow (no summary), source (is not a source) or sink (is not a sink). * * The interpretation of a row is similar to API-graphs with a left-to-right * reading. @@ -34,12 +41,15 @@ * - `Field[i]`: the `i`th element of a tuple. * - `Reference`: the referenced value. * - `Future`: the value being computed asynchronously. - * 3. The `kind` column is a tag that can be referenced from QL to determine to + * 3. The `acceptingValue` column of barrier guard models specifies which branch of the + * guard is blocking flow. It can be "true" or "false". In the future + * "no-exception", "not-zero", "null", "not-null" may be supported. + * 4. The `kind` column is a tag that can be referenced from QL to determine to * which classes the interpreted elements should be added. For example, for * sources `"remote"` indicates a default remote flow source, and for summaries * `"taint"` indicates a default additional taint step and `"value"` indicates a * globally applicable value-preserving step. - * 4. The `provenance` column is mainly used internally, and should be set to `"manual"` for + * 5. The `provenance` column is mainly used internally, and should be set to `"manual"` for * all custom models. */ @@ -114,11 +124,12 @@ extensible predicate barrierModel( * extension row number. * * The value referred to by `input` is assumed to lead to an argument of a call - * (possibly `self`), and the call is guarding the argument. `branch` is either `true` - * or `false`, indicating which branch of the guard is protecting the argument. + * (possibly `self`), and the call is guarding the argument. + * `acceptingValue` is either `true` or `false`, indicating which branch of + * the guard is protecting the parameter. */ extensible predicate barrierGuardModel( - string path, string input, string branch, string kind, string provenance, + string path, string input, string acceptingValue, string kind, string provenance, QlBuiltins::ExtensionId madId ); @@ -153,9 +164,9 @@ predicate interpretModelForTest(QlBuiltins::ExtensionId madId, string model) { model = "Barrier: " + path + "; " + output + "; " + kind ) or - exists(string path, string input, string branch, string kind | - barrierGuardModel(path, input, branch, kind, _, madId) and - model = "Barrier guard: " + path + "; " + input + "; " + branch + "; " + kind + exists(string path, string input, string acceptingValue, string kind | + barrierGuardModel(path, input, acceptingValue, kind, _, madId) and + model = "Barrier guard: " + path + "; " + input + "; " + acceptingValue + "; " + kind ) } @@ -265,10 +276,10 @@ private class FlowBarrierGuardFromModel extends FlowBarrierGuard::Range { } override predicate isBarrierGuard( - string input, string branch, string kind, Provenance provenance, string model + string input, string acceptingValue, string kind, Provenance provenance, string model ) { exists(QlBuiltins::ExtensionId madId | - barrierGuardModel(path, input, branch, kind, provenance, madId) and + barrierGuardModel(path, input, acceptingValue, kind, provenance, madId) and model = "MaD:" + madId.toString() ) } diff --git a/shared/dataflow/codeql/dataflow/DataFlow.qll b/shared/dataflow/codeql/dataflow/DataFlow.qll index 7f9c0194374b..cacd52cf8396 100644 --- a/shared/dataflow/codeql/dataflow/DataFlow.qll +++ b/shared/dataflow/codeql/dataflow/DataFlow.qll @@ -63,6 +63,35 @@ signature module InputSig { DataFlowType getNodeType(Node node); + /** + * Gets a special type to use for parameter node `p` belonging to callables with a + * source node where a source call context `FlowFeature` is used, if any. + * + * This can be used to prevent lambdas from being resolved, when a concrete call + * context is needed. Example: + * + * ```csharp + * void Foo(Action a) + * { + * var x = Source(); + * a(x); // (1) + * a = s => Sink(s); // (2) + * a(x); // (3) + * } + * + * void Bar() + * { + * Foo(s => Sink(s)); // (4) + * } + * ``` + * + * If a source call context flow feature is used, `a` can be assigned a special + * type that is incompatible with the type of _any_ lambda expression, which will + * prevent the call edge from (1) to (4). Note that the call edge from (3) to (2) + * will still be valid. + */ + default DataFlowType getSourceContextParameterNodeType(Node p) { none() } + predicate nodeIsHidden(Node node); class DataFlowExpr; diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll index 506774857d8e..ed0412d1cd4d 100644 --- a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll +++ b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll @@ -1103,6 +1103,16 @@ module MakeImpl Lang> { private module FwdTypeFlowInput implements TypeFlowInput { predicate enableTypeFlow = Param::enableTypeFlow/0; + pragma[nomagic] + predicate isParameterNodeInSourceCallContext(ParamNode p) { + hasSourceCallCtx() and + exists(Node source, DataFlowCallable c | + Config::isSource(pragma[only_bind_into](source), _) and + nodeEnclosingCallable(source, c) and + nodeEnclosingCallable(p, c) + ) + } + predicate relevantCallEdgeIn = PrevStage::relevantCallEdgeIn/2; predicate relevantCallEdgeOut = PrevStage::relevantCallEdgeOut/2; @@ -1410,6 +1420,8 @@ module MakeImpl Lang> { private module RevTypeFlowInput implements TypeFlowInput { predicate enableTypeFlow = Param::enableTypeFlow/0; + predicate isParameterNodeInSourceCallContext(ParamNode p) { none() } + predicate relevantCallEdgeIn(Call call, Callable c) { flowOutOfCallAp(call, c, _, _, _, _, _) } diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll index 51ebb3f8a730..962a58c26f94 100644 --- a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll +++ b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll @@ -1893,6 +1893,9 @@ module MakeImplCommon Lang> { signature module TypeFlowInput { predicate enableTypeFlow(); + /** Holds if `p` is a parameter of a callable with a source node that has a call context. */ + predicate isParameterNodeInSourceCallContext(ParamNode p); + /** Holds if the edge is possibly needed in the direction `call` to `c`. */ predicate relevantCallEdgeIn(Call call, Callable c); @@ -1953,6 +1956,9 @@ module MakeImplCommon Lang> { /** * Holds if a sequence of calls may propagate the value of `arg` to some * argument-to-parameter call edge that strengthens the static type. + * + * This predicate is a reverse flow computation, starting at calls that + * strengthen the type and then following relevant call edges backwards. */ pragma[nomagic] private predicate trackedArgTypeCand(ArgNode arg) { @@ -1987,6 +1993,9 @@ module MakeImplCommon Lang> { * Holds if `p` is part of a value-propagating call path where the * end-points have stronger types than the intermediate parameter and * argument nodes. + * + * This predicate is a forward flow computation, intersecting with the + * reverse flow computation done in `trackedArgTypeCand`. */ private predicate trackedParamType(ParamNode p) { exists(Call call1, Callable c1, ArgNode argOut, Call call2, Callable c2, ArgNode argIn | @@ -2013,6 +2022,8 @@ module MakeImplCommon Lang> { typeStrongerThanFilter(at, pt) ) or + Input::isParameterNodeInSourceCallContext(p) + or exists(ArgNode arg | trackedArgType(arg) and relevantCallEdge(_, _, arg, p) and @@ -2104,8 +2115,12 @@ module MakeImplCommon Lang> { * context. */ private predicate typeFlowParamType(ParamNode p, Type t, boolean cc) { - exists(Callable c | - Input::dataFlowNonCallEntry(c, cc) and + exists(Callable c | Input::dataFlowNonCallEntry(c, cc) | + cc = true and + nodeEnclosingCallable(p, c) and + t = getSourceContextParameterNodeType(p) + or + (cc = false or not exists(getSourceContextParameterNodeType(p))) and trackedParamWithType(p, t, c) ) or diff --git a/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll b/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll index 8b25c54bfa09..ce980724778b 100644 --- a/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll +++ b/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll @@ -388,11 +388,11 @@ module Make< /** * Holds if this element is a flow barrier guard of kind `kind`, for data - * flowing in as described by `input`, when `this` evaluates to `branch`. + * flowing in as described by `input`, when `this` evaluates to `acceptingValue`. */ pragma[nomagic] abstract predicate isBarrierGuard( - string input, string branch, string kind, Provenance provenance, string model + string input, string acceptingValue, string kind, Provenance provenance, string model ); } @@ -764,10 +764,10 @@ module Make< } private predicate isRelevantBarrierGuard( - BarrierGuardElement e, string input, string branch, string kind, Provenance provenance, - string model + BarrierGuardElement e, string input, string acceptingValue, string kind, + Provenance provenance, string model ) { - e.isBarrierGuard(input, branch, kind, provenance, model) and + e.isBarrierGuard(input, acceptingValue, kind, provenance, model) and ( provenance.isManual() or @@ -1588,11 +1588,11 @@ module Make< * Holds if `barrierGuard` is a relevant barrier guard element with input specification `inSpec`. */ predicate barrierGuardSpec( - BarrierGuardElement barrierGuard, SummaryComponentStack inSpec, string branch, string kind, - string model + BarrierGuardElement barrierGuard, SummaryComponentStack inSpec, string acceptingValue, + string kind, string model ) { exists(string input | - isRelevantBarrierGuard(barrierGuard, input, branch, kind, _, model) and + isRelevantBarrierGuard(barrierGuard, input, acceptingValue, kind, _, model) and External::interpretSpec(input, inSpec) ) } @@ -2189,10 +2189,10 @@ module Make< not exists(interpretComponent(c)) } - /** Holds if `acceptingvalue` is not a valid barrier guard accepting-value. */ - bindingset[acceptingvalue] - predicate invalidAcceptingValue(string acceptingvalue) { - not acceptingvalue instanceof AcceptingValue + /** Holds if `acceptingValue` is not a valid barrier guard accepting-value. */ + bindingset[acceptingValue] + predicate invalidAcceptingValue(string acceptingValue) { + not acceptingValue instanceof AcceptingValue } /** Holds if `provenance` is not a valid provenance value. */ @@ -2242,10 +2242,10 @@ module Make< /** * Holds if an external barrier guard specification exists for `n` with input - * specification `input`, accepting value `acceptingvalue`, and kind `kind`. + * specification `input`, accepting value `acceptingValue`, and kind `kind`. */ predicate barrierGuardElement( - Element n, string input, AcceptingValue acceptingvalue, string kind, + Element n, string input, AcceptingValue acceptingValue, string kind, Provenance provenance, string model ); @@ -2371,11 +2371,11 @@ module Make< } private predicate barrierGuardElementRef( - InterpretNode ref, SourceSinkAccessPath input, AcceptingValue acceptingvalue, string kind, + InterpretNode ref, SourceSinkAccessPath input, AcceptingValue acceptingValue, string kind, string model ) { exists(SourceOrSinkElement e | - barrierGuardElement(e, input, acceptingvalue, kind, _, model) and + barrierGuardElement(e, input, acceptingValue, kind, _, model) and if inputNeedsReferenceExt(input.getToken(0)) then e = ref.getCallTarget() else e = ref.asElement() @@ -2518,10 +2518,10 @@ module Make< * given kind in a MaD flow model. */ predicate isBarrierGuardNode( - InterpretNode node, AcceptingValue acceptingvalue, string kind, string model + InterpretNode node, AcceptingValue acceptingValue, string kind, string model ) { exists(InterpretNode ref, SourceSinkAccessPath input | - barrierGuardElementRef(ref, input, acceptingvalue, kind, model) and + barrierGuardElementRef(ref, input, acceptingValue, kind, model) and interpretInput(input, input.getNumToken(), ref, node) ) } diff --git a/shared/mad/codeql/mad/static/ModelsAsData.qll b/shared/mad/codeql/mad/static/ModelsAsData.qll index 84daaa9b6c86..4b58a23186ac 100644 --- a/shared/mad/codeql/mad/static/ModelsAsData.qll +++ b/shared/mad/codeql/mad/static/ModelsAsData.qll @@ -31,7 +31,7 @@ signature module ExtensionsSig { */ predicate barrierGuardModel( string namespace, string type, boolean subtypes, string name, string signature, string ext, - string input, string acceptingvalue, string kind, string provenance, + string input, string acceptingValue, string kind, string provenance, QlBuiltins::ExtensionId madId ); @@ -142,14 +142,14 @@ module ModelsAsData { or exists( string namespace, string type, boolean subtypes, string name, string signature, string ext, - string input, string acceptingvalue, string kind, string provenance + string input, string acceptingValue, string kind, string provenance | Extensions::barrierGuardModel(namespace, type, subtypes, name, signature, ext, input, - acceptingvalue, kind, provenance, madId) + acceptingValue, kind, provenance, madId) | model = "Barrier Guard: " + namespace + "; " + type + "; " + subtypes + "; " + name + "; " + - signature + "; " + ext + "; " + input + "; " + acceptingvalue + "; " + kind + "; " + + signature + "; " + ext + "; " + input + "; " + acceptingValue + "; " + kind + "; " + provenance ) or @@ -241,12 +241,12 @@ module ModelsAsData { /** Holds if a barrier guard model exists for the given parameters. */ predicate barrierGuardModel( string namespace, string type, boolean subtypes, string name, string signature, string ext, - string input, string acceptingvalue, string kind, string provenance, string model + string input, string acceptingValue, string kind, string provenance, string model ) { exists(string namespaceOrGroup, QlBuiltins::ExtensionId madId | namespace = getNamespace(namespaceOrGroup) and Extensions::barrierGuardModel(namespaceOrGroup, type, subtypes, name, signature, ext, input, - acceptingvalue, kind, provenance, madId) and + acceptingValue, kind, provenance, madId) and model = "MaD:" + madId.toString() ) } diff --git a/swift/ql/lib/codeql/swift/dataflow/internal/FlowSummaryImpl.qll b/swift/ql/lib/codeql/swift/dataflow/internal/FlowSummaryImpl.qll index c1ddb7f781f5..3a096fe3d576 100644 --- a/swift/ql/lib/codeql/swift/dataflow/internal/FlowSummaryImpl.qll +++ b/swift/ql/lib/codeql/swift/dataflow/internal/FlowSummaryImpl.qll @@ -168,7 +168,7 @@ module SourceSinkInterpretationInput implements } predicate barrierGuardElement( - Element n, string input, Public::AcceptingValue acceptingvalue, string kind, + Element n, string input, Public::AcceptingValue acceptingValue, string kind, Public::Provenance provenance, string model ) { none()