diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll index 8a3dfb7b2a7..ac222741c02 100644 --- a/ql/lib/codeql/actions/Ast.qll +++ b/ql/lib/codeql/actions/Ast.qll @@ -89,6 +89,24 @@ module Utils { ) } + bindingset[line] + predicate extractPathAssignment(string line, string value) { + exists(string path | + // single path assignment + path = + line.regexpCapture("(echo|Write-Output)\\s+(.*)>>\\s*(\"|')?\\$(\\{)?GITHUB_PATH(\\})?(\"|')?", + 2) and + value = trimQuotes(path) + or + // workflow command assignment + path = + line.regexpCapture("(echo|Write-Output)\\s+(\"|')?::add-path::(.*)(\"|')?", 3) + .regexpReplaceAll("^\"", "") + .regexpReplaceAll("\"$", "") and + value = trimQuotes(path) + ) + } + predicate writeToGitHubEnv(Run run, string key, string value) { extractLineAssignment(run.getScript().splitAt("\n"), "ENV", key, value) or extractMultilineAssignment(run.getScript(), "ENV", key, value) @@ -98,6 +116,10 @@ module Utils { extractLineAssignment(run.getScript().splitAt("\n"), "OUTPUT", key, value) or extractMultilineAssignment(run.getScript(), "OUTPUT", key, value) } + + predicate writeToGitHubPath(Run run, string value) { + extractPathAssignment(run.getScript().splitAt("\n"), value) + } } class AstNode instanceof AstNodeImpl { diff --git a/ql/lib/codeql/actions/security/EnvPathInjectionQuery.qll b/ql/lib/codeql/actions/security/EnvPathInjectionQuery.qll new file mode 100644 index 00000000000..a5cf2d600f0 --- /dev/null +++ b/ql/lib/codeql/actions/security/EnvPathInjectionQuery.qll @@ -0,0 +1,68 @@ +private import actions +private import codeql.actions.TaintTracking +private import codeql.actions.dataflow.ExternalFlow +import codeql.actions.dataflow.FlowSources +private import codeql.actions.security.ArtifactPoisoningQuery +import codeql.actions.DataFlow + +predicate envPathInjectionFromExprSink(DataFlow::Node sink) { + exists(Expression expr, Run run, string value | + Utils::writeToGitHubPath(run, value) and + expr = sink.asExpr() and + run.getAnScriptExpr() = expr and + value.indexOf(expr.getExpression()) > 0 + ) +} + +predicate envPathInjectionFromFileSink(DataFlow::Node sink) { + exists(Run run, UntrustedArtifactDownloadStep step, string value | + sink.asExpr() = run and + step.getAFollowingStep() = run and + Utils::writeToGitHubPath(run, value) and + // TODO: add support for other commands like `<`, `jq`, ... + value.regexpMatch(["\\$\\(", "`"] + ["cat\\s+", "<"] + ".*" + ["`", "\\)"]) + ) +} + +/** + * Holds if a Run step declares an environment variable, uses it to declare a PATH env var. + * e.g. + * env: + * BODY: ${{ github.event.comment.body }} + * run: | + * echo "$BODY" >> $GITHUB_PATH + */ +predicate envPathInjectionFromEnvSink(DataFlow::Node sink) { + exists(Run run, Expression expr, string varname, string value | + sink.asExpr().getInScopeEnvVarExpr(varname) = expr and + run = sink.asExpr() and + Utils::writeToGitHubPath(run, value) and + ( + value = ["$" + varname, "${" + varname + "}", "$ENV{" + varname + "}"] + or + value.matches("$(echo %") and value.indexOf(varname) > 0 + ) + ) +} + +private class EnvPathInjectionSink extends DataFlow::Node { + EnvPathInjectionSink() { + envPathInjectionFromExprSink(this) or + envPathInjectionFromFileSink(this) or + envPathInjectionFromEnvSink(this) or + externallyDefinedSink(this, "envpath-injection") + } +} + +/** + * A taint-tracking configuration for unsafe user input + * that is used to construct and evaluate an environment variable. + */ +private module EnvPathInjectionConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + predicate isSink(DataFlow::Node sink) { sink instanceof EnvPathInjectionSink } +} + +/** Tracks flow of unsafe user input that is used to construct and evaluate the PATH environment variable. */ +module EnvPathInjectionFlow = TaintTracking::Global; diff --git a/ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll b/ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll index 7d95188cc8c..0ae333a56f5 100644 --- a/ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll +++ b/ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll @@ -10,7 +10,7 @@ predicate envVarInjectionFromExprSink(DataFlow::Node sink) { Utils::writeToGitHubEnv(run, key, value) and expr = sink.asExpr() and run.getAnScriptExpr() = expr and - value.indexOf(expr.getRawExpression()) > 0 + value.indexOf(expr.getExpression()) > 0 ) } diff --git a/ql/src/Security/CWE-077/EnvPathInjection.ql b/ql/src/Security/CWE-077/EnvPathInjection.ql new file mode 100644 index 00000000000..19b4cf6c01b --- /dev/null +++ b/ql/src/Security/CWE-077/EnvPathInjection.ql @@ -0,0 +1,32 @@ +/** + * @name PATH Enviroment Variable built from user-controlled sources + * @description Building the PATH environment variable from user-controlled sources may alter the execution of following system commands + * @kind path-problem + * @problem.severity warning + * @security-severity 5.0 + * @precision high + * @id actions/envpath-injection + * @tags actions + * security + * external/cwe/cwe-077 + * external/cwe/cwe-020 + */ + +import actions +import codeql.actions.security.EnvPathInjectionQuery +import EnvPathInjectionFlow::PathGraph + +from EnvPathInjectionFlow::PathNode source, EnvPathInjectionFlow::PathNode sink +where + EnvPathInjectionFlow::flowPath(source, sink) and + ( + exists(source.getNode().asExpr().getEnclosingCompositeAction()) + or + exists(Workflow w | + w = source.getNode().asExpr().getEnclosingWorkflow() and + not w.isPrivileged() + ) + ) +select sink.getNode(), source, sink, + "Potential PATH environment variable injection in $@, which may be controlled by an external user.", + sink, sink.getNode().toString() diff --git a/ql/src/Security/CWE-077/PrivilegedEnvPathInjection.ql b/ql/src/Security/CWE-077/PrivilegedEnvPathInjection.ql new file mode 100644 index 00000000000..e9f55d1cbb2 --- /dev/null +++ b/ql/src/Security/CWE-077/PrivilegedEnvPathInjection.ql @@ -0,0 +1,28 @@ +/** + * @name PATH Enviroment Variable built from user-controlled sources + * @description Building the PATH environment variable from user-controlled sources may alter the execution of following system commands + * @kind path-problem + * @problem.severity error + * @security-severity 9 + * @precision high + * @id actions/privileged-envpath-injection + * @tags actions + * security + * external/cwe/cwe-077 + * external/cwe/cwe-020 + */ + +import actions +import codeql.actions.security.EnvPathInjectionQuery +import EnvPathInjectionFlow::PathGraph + +from EnvPathInjectionFlow::PathNode source, EnvPathInjectionFlow::PathNode sink +where + EnvPathInjectionFlow::flowPath(source, sink) and + exists(Workflow w | + w = source.getNode().asExpr().getEnclosingWorkflow() and + w.isPrivileged() + ) +select sink.getNode(), source, sink, + "Potential privileged PATH environment variable injection in $@, which may be controlled by an external user.", + sink, sink.getNode().toString() diff --git a/ql/test/query-tests/Security/CWE-077/.github/workflows/path1.yml b/ql/test/query-tests/Security/CWE-077/.github/workflows/path1.yml new file mode 100644 index 00000000000..d22f09c03bd --- /dev/null +++ b/ql/test/query-tests/Security/CWE-077/.github/workflows/path1.yml @@ -0,0 +1,33 @@ +name: Pull Request Open + +on: + pull_request_target: + +jobs: + test: + runs-on: ubuntu-latest + steps: + + - run: echo "${{ github.event.pull_request.title }}" >> $GITHUB_PATH + - env: + PATHINJ: ${{ github.event.pull_request.title }} + run: echo $(echo "$PATHINJ") >> $GITHUB_PATH + - env: + PATHINJ: ${{ github.event.pull_request.title }} + run: echo $PATHINJ >> $GITHUB_PATH + - env: + PATHINJ: ${{ github.event.pull_request.title }} + run: echo ${PATHINJ} >> $GITHUB_PATH + - uses: dawidd6/action-download-artifact@v2 + with: + name: artifact_name + path: foo + - run: echo "$(cat foo/bar)" >> $GITHUB_PATH + - env: + ACTIONS_ALLOW_UNSECURE_COMMANDS: true + PATHINJ: ${{ github.event.pull_request.title }} + run: echo "::add-path::$PATHINJ" + + + + diff --git a/ql/test/query-tests/Security/CWE-077/EnvPathInjection.actual b/ql/test/query-tests/Security/CWE-077/EnvPathInjection.actual new file mode 100644 index 00000000000..6d9801ccd81 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-077/EnvPathInjection.actual @@ -0,0 +1,10 @@ +edges +| .github/workflows/path1.yml:21:9:25:6 | Uses Step | .github/workflows/path1.yml:25:9:26:6 | Run Step | +| .github/workflows/path1.yml:21:9:25:6 | Uses Step | .github/workflows/path1.yml:26:9:29:41 | Run Step | +nodes +| .github/workflows/path1.yml:11:21:11:58 | github.event.pull_request.title | semmle.label | github.event.pull_request.title | +| .github/workflows/path1.yml:21:9:25:6 | Uses Step | semmle.label | Uses Step | +| .github/workflows/path1.yml:25:9:26:6 | Run Step | semmle.label | Run Step | +| .github/workflows/path1.yml:26:9:29:41 | Run Step | semmle.label | Run Step | +subpaths +#select diff --git a/ql/test/query-tests/Security/CWE-077/EnvPathInjection.qlref b/ql/test/query-tests/Security/CWE-077/EnvPathInjection.qlref new file mode 100644 index 00000000000..ab36454942e --- /dev/null +++ b/ql/test/query-tests/Security/CWE-077/EnvPathInjection.qlref @@ -0,0 +1 @@ +Security/CWE-077/EnvPathInjection.ql