From da36924bb1098a89adcb425dc5532e966ca90ff2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Tue, 30 Jul 2024 10:26:41 +0200 Subject: [PATCH] feat(queries): Add Output Clobbering query --- ql/lib/codeql/actions/dataflow/FlowSteps.qll | 2 +- .../security/OutputClobberingQuery.qll | 103 ++++++++++++++---- .../Security/CWE-077/OutputClobberingHigh.ql | 37 +++++++ .../CWE-094/OutputClobberingMedium.ql | 31 ------ .../CWE-077/.github/workflows/output1.yml | 38 +++++++ .../CWE-077/OutputClobberingHigh.expected | 12 ++ .../CWE-077/OutputClobberingHigh.qlref | 1 + 7 files changed, 168 insertions(+), 56 deletions(-) create mode 100644 ql/src/Security/CWE-077/OutputClobberingHigh.ql delete mode 100644 ql/src/Security/CWE-094/OutputClobberingMedium.ql create mode 100644 ql/test/query-tests/Security/CWE-077/.github/workflows/output1.yml create mode 100644 ql/test/query-tests/Security/CWE-077/OutputClobberingHigh.expected create mode 100644 ql/test/query-tests/Security/CWE-077/OutputClobberingHigh.qlref diff --git a/ql/lib/codeql/actions/dataflow/FlowSteps.qll b/ql/lib/codeql/actions/dataflow/FlowSteps.qll index e16bc00f8ea..5d0d45c26c1 100644 --- a/ql/lib/codeql/actions/dataflow/FlowSteps.qll +++ b/ql/lib/codeql/actions/dataflow/FlowSteps.qll @@ -113,7 +113,7 @@ predicate envToRunStep(DataFlow::Node pred, DataFlow::Node succ) { run.getInScopeEnvVarExpr(var_name) = pred.asExpr() and succ.asExpr() = run.getScriptScalar() and ( - envToSpecialFile(["GITHUB_ENV", "GITHUB_PATH"], var_name, run, _) or + envToSpecialFile(["GITHUB_ENV", "GITHUB_OUTPUT", "GITHUB_PATH"], var_name, run, _) or envToArgInjSink(var_name, run, _) ) ) diff --git a/ql/lib/codeql/actions/security/OutputClobberingQuery.qll b/ql/lib/codeql/actions/security/OutputClobberingQuery.qll index f1811ed5762..a67be6e3562 100644 --- a/ql/lib/codeql/actions/security/OutputClobberingQuery.qll +++ b/ql/lib/codeql/actions/security/OutputClobberingQuery.qll @@ -1,43 +1,98 @@ private import actions private import codeql.actions.TaintTracking private import codeql.actions.dataflow.ExternalFlow -private import codeql.actions.security.CodeInjectionQuery private import codeql.actions.security.ArtifactPoisoningQuery -import codeql.actions.dataflow.FlowSources +private import codeql.actions.dataflow.FlowSteps import codeql.actions.DataFlow +import codeql.actions.dataflow.FlowSources -abstract class OutputClobberingSource extends Step { } +abstract class OutputClobberingSink extends DataFlow::Node { } -class RunOutputClobbering extends OutputClobberingSource, Run { - RunOutputClobbering() { - exists(UntrustedArtifactDownloadStep download, string script | - download.getAFollowingStep() = this and - this.getScript() = script and - exists(int i | - script.splitAt("\n", i).matches(["%GITHUB_OUTPUT%", "%::set-output name%"]) and - i < count(string line | line = script.splitAt("\n") | line) - 1 +/** + * Holds if a Run step declares an environment variable with contents from a local file. + * e.g. + * run: | + * echo "sha=$(cat test-results/sha-number)" >> $GITHUB_OUTPUT + * echo "sha=$(> $GITHUB_OUTPUT + */ +class OutputClobberingFromFileReadSink extends OutputClobberingSink { + OutputClobberingFromFileReadSink() { + exists(Run run, UntrustedArtifactDownloadStep step, string content, string key, string value | + this.asExpr() = run.getScriptScalar() and + step.getAFollowingStep() = run and + writeToGitHubOutput(run, content) and + extractVariableAndValue(content, key, value) and + // there is a different output variable in the same script + // TODO: key2/value2 should be declared before key/value + exists(string content2, string key2 | + writeToGitHubOutput(run, content2) and + extractVariableAndValue(content2, key2, _) and + not key2 = key + ) and + ( + outputsPartialFileContent(value) + or + // e.g. + // FOO=$(cat test-results/sha-number) + // echo "FOO=$FOO" >> $GITHUB_OUTPUT + exists(string line, string var_name, string var_value | + run.getScript().splitAt("\n") = line + | + var_name = line.regexpCapture("([a-zA-Z0-9\\-_]+)=(.*)", 1) and + var_value = line.regexpCapture("([a-zA-Z0-9\\-_]+)=(.*)", 2) and + outputsPartialFileContent(var_value) and + ( + value.matches("%$" + ["", "{", "ENV{"] + var_name + "%") + or + value.matches("$(echo %") and value.indexOf(var_name) > 0 + ) + ) ) ) } } /** - * A taint-tracking configuration for unsafe user input - * that is used to construct and evaluate a code script. + * Holds if a Run step declares an environment variable, uses it to declare env var. + * e.g. + * env: + * BODY: ${{ github.event.comment.body }} + * run: | + * echo "FOO=$BODY" >> $GITHUB_OUTPUT */ -private module OutputClobberingConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node source) { source.asExpr() instanceof OutputClobberingSource } - - predicate isSink(DataFlow::Node sink) { sink instanceof CodeInjectionSink } - - predicate isAdditionalFlowStep(DataFlow::Node prev, DataFlow::Node succ) { - exists(StepsExpression e | - e.getTarget() = prev.asExpr() and - prev.asExpr() instanceof OutputClobberingSource and - succ.asExpr() = e +class OutputClobberingFromEnvVarSink extends OutputClobberingSink { + OutputClobberingFromEnvVarSink() { + exists(Run run, string var_name, string key | + envToSpecialFile("GITHUB_OUTPUT", var_name, run, key) and + // there is a different output variable in the same script + // TODO: key2/value2 should be declared before key/value + exists(string content2, string key2 | + writeToGitHubOutput(run, content2) and + extractVariableAndValue(content2, key2, _) and + not key2 = key + ) and + exists(run.getInScopeEnvVarExpr(var_name)) and + run.getScriptScalar() = this.asExpr() ) } } -/** Tracks flow of unsafe user input that is used to construct and evaluate a code script. */ +class OutputClobberingFromMaDSink extends OutputClobberingSink { + OutputClobberingFromMaDSink() { madSink(this, "output-clobbering") } +} + +/** + * A taint-tracking configuration for unsafe user input + * that is used to construct and evaluate an environment variable. + */ +private module OutputClobberingConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + source instanceof RemoteFlowSource and + not source.(RemoteFlowSource).getSourceType() = "branch" + } + + predicate isSink(DataFlow::Node sink) { sink instanceof OutputClobberingSink } +} + +/** Tracks flow of unsafe user input that is used to construct and evaluate an environment variable. */ module OutputClobberingFlow = TaintTracking::Global; diff --git a/ql/src/Security/CWE-077/OutputClobberingHigh.ql b/ql/src/Security/CWE-077/OutputClobberingHigh.ql new file mode 100644 index 00000000000..a7016a50c58 --- /dev/null +++ b/ql/src/Security/CWE-077/OutputClobberingHigh.ql @@ -0,0 +1,37 @@ +/** + * @name Output Clobbering + * @description A Step output can be clobbered which may allow an attacker to manipulate the expected and trusted values of a variable. + * @kind path-problem + * @problem.severity error + * @security-severity 7.3 + * @precision high + * @id actions/output-clobbering/high + * @tags actions + * security + * experimental + * external/cwe/cwe-094 + * external/cwe/cwe-095 + * external/cwe/cwe-116 + */ + +import actions +import codeql.actions.security.OutputClobberingQuery +import codeql.actions.dataflow.ExternalFlow +import OutputClobberingFlow::PathGraph + +from OutputClobberingFlow::PathNode source, OutputClobberingFlow::PathNode sink +where + OutputClobberingFlow::flowPath(source, sink) and + inPrivilegedContext(sink.getNode().asExpr()) and + // exclude paths to file read sinks from non-artifact sources + ( + not source.getNode().(RemoteFlowSource).getSourceType() = "artifact" + or + source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and + ( + sink.getNode() instanceof OutputClobberingFromFileReadSink or + madSink(sink.getNode(), "output-clobbering") + ) + ) +select sink.getNode(), source, sink, "Potential clobbering of a step output in $@.", sink, + sink.getNode().toString() diff --git a/ql/src/Security/CWE-094/OutputClobberingMedium.ql b/ql/src/Security/CWE-094/OutputClobberingMedium.ql deleted file mode 100644 index 7094a7891da..00000000000 --- a/ql/src/Security/CWE-094/OutputClobberingMedium.ql +++ /dev/null @@ -1,31 +0,0 @@ -/** - * @name Output Clobbering - * @description A Step output can be clobbered which may allow an attacker to manipulate the expected and trusted values of a variable. - * @kind path-problem - * @problem.severity warning - * @security-severity 5.0 - * @precision medium - * @id actions/output-clobbering/medium - * @tags actions - * security - * external/cwe/cwe-094 - * external/cwe/cwe-095 - * external/cwe/cwe-116 - */ - -import actions -import codeql.actions.security.OutputClobberingQuery -import OutputClobberingFlow::PathGraph - -from OutputClobberingFlow::PathNode source, OutputClobberingFlow::PathNode sink -where - OutputClobberingFlow::flowPath(source, sink) and - inPrivilegedContext(sink.getNode().asExpr()) and - // exclude cases where the sink is a JS script and the expression uses toJson - not exists(UsesStep script | - script.getCallee() = "actions/github-script" and - script.getArgumentExpr("script") = sink.getNode().asExpr() and - exists(getAToJsonReferenceExpression(sink.getNode().asExpr().(Expression).getExpression(), _)) - ) -select sink.getNode(), source, sink, "Potential output clobbering leading to code injection in $@.", - sink, sink.getNode().asExpr().(Expression).getRawExpression() diff --git a/ql/test/query-tests/Security/CWE-077/.github/workflows/output1.yml b/ql/test/query-tests/Security/CWE-077/.github/workflows/output1.yml new file mode 100644 index 00000000000..df583724998 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-077/.github/workflows/output1.yml @@ -0,0 +1,38 @@ +on: + issue_comment: +jobs: + test1: + runs-on: ubuntu-latest + steps: + - id: clob1 + env: + BODY: ${{ github.event.comment.body }} + run: | + # VULNERABLE + echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT + echo "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT + - id: clob2 + run: | + echo ${{ steps.clob1.outputs.OUTPUT_1 }} + echo ${{ steps.clob1.outputs.OUTPUT_2 }} + test2: + runs-on: ubuntu-latest + steps: + - id: clob1 + env: + BODY: ${{ github.event.comment.body }} + run: | + # NOT VULNERABLE + echo "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT + test3: + runs-on: ubuntu-latest + steps: + - name: Download artifact + uses: dawidd6/action-download-artifact@v6 + with: + run_id: ${{ github.event.workflow_run.id }} + name: pr_number + - id: clob1 + run: | + echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT + echo "OUTPUT_2=$(> $GITHUB_OUTPUT diff --git a/ql/test/query-tests/Security/CWE-077/OutputClobberingHigh.expected b/ql/test/query-tests/Security/CWE-077/OutputClobberingHigh.expected new file mode 100644 index 00000000000..ea3261450ec --- /dev/null +++ b/ql/test/query-tests/Security/CWE-077/OutputClobberingHigh.expected @@ -0,0 +1,12 @@ +edges +| .github/workflows/output1.yml:9:18:9:49 | github.event.comment.body | .github/workflows/output1.yml:10:14:13:50 | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | provenance | | +| .github/workflows/output1.yml:30:9:35:6 | Uses Step | .github/workflows/output1.yml:36:14:38:58 | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(> $GITHUB_OUTPUT\n | provenance | | +nodes +| .github/workflows/output1.yml:9:18:9:49 | github.event.comment.body | semmle.label | github.event.comment.body | +| .github/workflows/output1.yml:10:14:13:50 | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | semmle.label | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | +| .github/workflows/output1.yml:30:9:35:6 | Uses Step | semmle.label | Uses Step | +| .github/workflows/output1.yml:36:14:38:58 | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(> $GITHUB_OUTPUT\n | semmle.label | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(> $GITHUB_OUTPUT\n | +subpaths +#select +| .github/workflows/output1.yml:10:14:13:50 | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | .github/workflows/output1.yml:9:18:9:49 | github.event.comment.body | .github/workflows/output1.yml:10:14:13:50 | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | Potential clobbering of a step output in $@. | .github/workflows/output1.yml:10:14:13:50 | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | +| .github/workflows/output1.yml:36:14:38:58 | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(> $GITHUB_OUTPUT\n | .github/workflows/output1.yml:30:9:35:6 | Uses Step | .github/workflows/output1.yml:36:14:38:58 | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(> $GITHUB_OUTPUT\n | Potential clobbering of a step output in $@. | .github/workflows/output1.yml:36:14:38:58 | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(> $GITHUB_OUTPUT\n | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(> $GITHUB_OUTPUT\n | diff --git a/ql/test/query-tests/Security/CWE-077/OutputClobberingHigh.qlref b/ql/test/query-tests/Security/CWE-077/OutputClobberingHigh.qlref new file mode 100644 index 00000000000..5af047eec9e --- /dev/null +++ b/ql/test/query-tests/Security/CWE-077/OutputClobberingHigh.qlref @@ -0,0 +1 @@ +Security/CWE-077/OutputClobberingHigh.ql