Better handling of EnvVar Injection and Argument Injection

Author: Alvaro Muñoz

ql/lib/codeql/actions/Bash.qll

@@ -220,9 +220,13 @@ class BashShellScript extends ShellScript {
   override string getCommand(int i) {
     // remove redirection
     result =
-      this.getCmd(i).regexpReplaceAll("(>|>>|2>|2>>|<|<<<)\\s*[\\{\\}\\$\"'_\\-0-9a-zA-Z]+$", "") and
+      this.getCmd(i)
+          .regexpReplaceAll("(>|>>|2>|2>>|<|<<<)\\s*[\\{\\}\\$\"'_\\-0-9a-zA-Z]+$", "")
+          .trim() and
     // exclude variable declarations
     not result.regexpMatch("^[a-zA-Z0-9\\-_]+=") and
+    // exclude comments
+    not result.trim().indexOf("#") = 0 and
     // exclude the following keywords
     not result =
       [
@@ -359,11 +363,11 @@ module Bash {
     exists(string regexp |
       // $(cmd)
       regexp = ".*\\$\\(([^)]+)\\).*" and
-      cmd = expr.regexpCapture(regexp, 1)
+      cmd = expr.regexpCapture(regexp, 1).trim()
       or
       // `cmd`
       regexp = ".*`([^`]+)`.*" and
-      cmd = expr.regexpCapture(regexp, 1)
+      cmd = expr.regexpCapture(regexp, 1).trim()
     )
   }
 
@@ -657,8 +661,8 @@ module Bash {
     exists(string cmd, string regex, int command_group, int argument_group |
       cmd = script.getACommand() and
       argumentInjectionSinksDataModel(regex, command_group, argument_group) and
-      argument = cmd.regexpCapture(regex, argument_group) and
-      command = cmd.regexpCapture(regex, command_group) and
+      argument = cmd.regexpCapture(regex, argument_group).trim() and
+      command = cmd.regexpCapture(regex, command_group).trim() and
       envReachingRunExpr(script, source, argument)
     )
   }
@@ -669,8 +673,8 @@ module Bash {
     exists(string cmd, string regex, int command_group, int argument_group |
       cmd = script.getACommand() and
       argumentInjectionSinksDataModel(regex, command_group, argument_group) and
-      argument = cmd.regexpCapture(regex, argument_group) and
-      command = cmd.regexpCapture(regex, command_group) and
+      argument = cmd.regexpCapture(regex, argument_group).trim() and
+      command = cmd.regexpCapture(regex, command_group).trim() and
       cmdReachingRunExpr(script, source, argument)
     )
   }

ql/lib/codeql/actions/config/Config.qll

@@ -47,10 +47,6 @@ predicate externallyTriggerableEventsDataModel(string event) {
 
 private string commandLauncher() { result = ["", "sudo\\s+", "su\\s+", "xvfb-run\\s+"] }
 
-private string commandPrefixDelimiter() { result = "(^|;|\\$\\(|`|\\||&&|\\|\\|)\\s*" }
-
-private string commandSuffixDelimiter() { result = "\\s*(;|\\||\\)|`|&&|\\|\\||$)" }
-
 /**
  * MaD models for poisonable commands
  * Fields:
@@ -59,9 +55,7 @@ private string commandSuffixDelimiter() { result = "\\s*(;|\\||\\)|`|&&|\\|\\||$
 predicate poisonableCommandsDataModel(string regexp) {
   exists(string sub_regexp |
     Extensions::poisonableCommandsDataModel(sub_regexp) and
-    // find regexp
-    regexp =
-      commandPrefixDelimiter() + commandLauncher() + sub_regexp + "(.*?)" + commandSuffixDelimiter()
+    regexp = commandLauncher() + sub_regexp + ".*"
   )
 }
 
@@ -74,10 +68,7 @@ predicate poisonableCommandsDataModel(string regexp) {
 predicate poisonableLocalScriptsDataModel(string regexp, int command_group) {
   exists(string sub_regexp |
     Extensions::poisonableLocalScriptsDataModel(sub_regexp, command_group) and
-    // capture regexp
-    regexp =
-      ".*" + commandPrefixDelimiter() + commandLauncher() + sub_regexp + commandSuffixDelimiter() +
-        ".*"
+    regexp = commandLauncher() + sub_regexp + ".*"
   )
 }
 
@@ -91,8 +82,7 @@ predicate poisonableLocalScriptsDataModel(string regexp, int command_group) {
 predicate argumentInjectionSinksDataModel(string regexp, int command_group, int argument_group) {
   exists(string sub_regexp |
     Extensions::argumentInjectionSinksDataModel(sub_regexp, command_group, argument_group) and
-    // capture regexp
-    regexp = ".*" + commandPrefixDelimiter() + sub_regexp // + commandSuffixDelimiter() +  ".*"
+    regexp = commandLauncher() + sub_regexp
   )
 }
 

ql/lib/codeql/actions/dataflow/FlowSources.qll

@@ -100,10 +100,10 @@ class GitCommandSource extends RemoteFlowSource, CommandSource {
       ) and
       this.asExpr() = run.getScript() and
       checkout.getAFollowingStep() = run and
-      run.getScript().getACommand() = cmd and
+      run.getScript().getAStmt() = cmd and
       cmd.indexOf("git") = 0 and
       untrustedGitCommandsDataModel(cmd_regex, flag) and
-      cmd.regexpMatch(cmd_regex)
+      cmd.regexpMatch(".*" + cmd_regex + ".*")
     )
   }
 

ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll

@@ -9,17 +9,17 @@ import codeql.actions.dataflow.FlowSources
 
 abstract class EnvVarInjectionSink extends DataFlow::Node { }
 
+string sanitizerCommand() {
+  result =
+    [
+      "tr\\s+(-d\\s*)?('|\")?.n('|\")?", // tr -d '\n' ' ', tr '\n' ' '
+      "tr\\s+-cd\\s+.*:alpha:", // tr -cd '[:alpha:_]'
+      "(head|tail)\\s+-n\\s+1" // head -n 1, tail -n 1
+    ]
+}
+
 /**
  * Holds if a Run step declares an environment variable with contents from a local file.
- * e.g.
- *    run: |
- *      cat test-results/.env >> $GITHUB_ENV
- *
- *      echo "sha=$(cat test-results/sha-number)" >> $GITHUB_ENV
- *      echo "sha=$(<test-results/sha-number)" >> $GITHUB_ENV
- *
- *      FOO=$(cat test-results/sha-number)
- *      echo "FOO=$FOO" >> $GITHUB_ENV
  */
 class EnvVarInjectionFromFileReadSink extends EnvVarInjectionSink {
   EnvVarInjectionFromFileReadSink() {
@@ -31,11 +31,19 @@ class EnvVarInjectionFromFileReadSink extends EnvVarInjectionSink {
       this.asExpr() = run.getScript() and
       step.getAFollowingStep() = run and
       (
-        exists(string cmd |
-          run.getScript().getACmdReachingGitHubEnvWrite(cmd, _) and
-          run.getScript().getAFileReadCommand() = cmd
+        // eg:
+        // echo "SHA=$(cat test-results/sha-number)" >> $GITHUB_ENV
+        // echo "SHA=$(<test-results/sha-number)" >> $GITHUB_ENV
+        // FOO=$(cat test-results/sha-number)
+        // echo "FOO=$FOO" >> $GITHUB_ENV
+        exists(string cmd, string var, string sanitizer |
+          run.getScript().getAFileReadCommand() = cmd and
+          run.getScript().getACmdReachingGitHubEnvWrite(cmd, var) and
+          run.getScript().getACmdReachingGitHubEnvWrite(sanitizer, var) and
+          not exists(sanitizer.regexpFind(sanitizerCommand(), _, _))
         )
         or
+        // eg: cat test-results/.env >> $GITHUB_ENV
         run.getScript().fileToGitHubEnv(_)
       )
     )
@@ -51,9 +59,18 @@ class EnvVarInjectionFromFileReadSink extends EnvVarInjectionSink {
  */
 class EnvVarInjectionFromCommandSink extends EnvVarInjectionSink {
   EnvVarInjectionFromCommandSink() {
-    exists(CommandSource source |
+    exists(CommandSource source, Run run, string var |
       this.asExpr() = source.getEnclosingRun().getScript() and
-      source.getEnclosingRun().getScript().getACmdReachingGitHubEnvWrite(source.getCommand(), _)
+      run = source.getEnclosingRun() and
+      run.getScript().getACmdReachingGitHubEnvWrite(source.getCommand(), var) and
+      (
+        not run.getScript().getACmdReachingGitHubEnvWrite(_, var)
+        or
+        exists(string sanitizer |
+          run.getScript().getACmdReachingGitHubEnvWrite(sanitizer, var) and
+          not exists(sanitizer.regexpFind(sanitizerCommand(), _, _))
+        )
+      )
     )
   }
 }
@@ -68,10 +85,18 @@ class EnvVarInjectionFromCommandSink extends EnvVarInjectionSink {
  */
 class EnvVarInjectionFromEnvVarSink extends EnvVarInjectionSink {
   EnvVarInjectionFromEnvVarSink() {
-    exists(Run run, string var_name |
+    exists(Run run, string var_name, string var |
       exists(run.getInScopeEnvVarExpr(var_name)) and
       run.getScript() = this.asExpr() and
-      run.getScript().getAnEnvReachingGitHubEnvWrite(var_name, _)
+      run.getScript().getAnEnvReachingGitHubEnvWrite(var_name, var) and
+      (
+        not run.getScript().getACmdReachingGitHubEnvWrite(_, var)
+        or
+        exists(string sanitizer |
+          run.getScript().getACmdReachingGitHubEnvWrite(sanitizer, var) and
+          not exists(sanitizer.regexpFind(sanitizerCommand(), _, _))
+        )
+      )
     )
   }
 }

ql/lib/codeql/actions/security/PoisonableSteps.qll

@@ -3,22 +3,15 @@ import codeql.actions.config.Config
 
 abstract class PoisonableStep extends Step { }
 
-private string dangerousActions() {
-  exists(string action |
-    poisonableActionsDataModel(action) and
-    result = action
-  )
-}
-
 class DangerousActionUsesStep extends PoisonableStep, UsesStep {
-  DangerousActionUsesStep() { this.getCallee() = dangerousActions() }
+  DangerousActionUsesStep() { poisonableActionsDataModel(this.getCallee()) }
 }
 
 class PoisonableCommandStep extends PoisonableStep, Run {
   PoisonableCommandStep() {
     exists(string regexp |
       poisonableCommandsDataModel(regexp) and
-      this.getScript().getACommand().regexpMatch("^" + regexp + ".*")
+      this.getScript().getACommand().regexpMatch(regexp)
     )
   }
 }

ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll

@@ -53,7 +53,7 @@ private module ActionsMutableRefCheckoutConfig implements DataFlow::ConfigSig {
   predicate isSink(DataFlow::Node sink) {
     exists(Uses uses |
       uses.getCallee() = "actions/checkout" and
-      uses.getArgumentExpr("ref") = sink.asExpr()
+      uses.getArgumentExpr(["ref", "repository"]) = sink.asExpr()
     )
   }
 
@@ -99,7 +99,7 @@ private module ActionsSHACheckoutConfig implements DataFlow::ConfigSig {
   predicate isSink(DataFlow::Node sink) {
     exists(Uses uses |
       uses.getCallee() = "actions/checkout" and
-      uses.getArgumentExpr("ref") = sink.asExpr()
+      uses.getArgumentExpr(["ref", "repository"]) = sink.asExpr()
     )
   }
 
@@ -199,7 +199,7 @@ class ActionsMutableRefCheckout extends MutableRefCheckoutStep instanceof UsesSt
     (
       exists(ActionsMutableRefCheckoutFlow::PathNode sink |
         ActionsMutableRefCheckoutFlow::flowPath(_, sink) and
-        sink.getNode().asExpr() = this.getArgumentExpr("ref")
+        sink.getNode().asExpr() = this.getArgumentExpr(["ref", "repository"])
       )
       or
       // heuristic base on the step id and field name
@@ -243,7 +243,7 @@ class ActionsSHACheckout extends SHACheckoutStep instanceof UsesStep {
     (
       exists(ActionsSHACheckoutFlow::PathNode sink |
         ActionsSHACheckoutFlow::flowPath(_, sink) and
-        sink.getNode().asExpr() = this.getArgumentExpr("ref")
+        sink.getNode().asExpr() = this.getArgumentExpr(["ref", "repository"])
       )
       or
       // heuristic base on the step id and field name

ql/lib/ext/config/argument_injection_sinks.yml

@@ -5,12 +5,11 @@ extensions:
     # https://gtfobins.github.io/
     # https://0xn3va.gitbook.io/cheat-sheets/web-application/command-injection/argument-injection
     data:
-      - ["(awk)\\s(.*?)", 2, 3]
-      - ["(curl)\\s(.*?)", 2, 3]
-      - ["(find)\\s(.*?)", 2, 3]
-      - ["(git)\\s(.*?)", 2, 3]
-      - ["(sed)\\s(.*?)", 2, 3]
-      - ["(tar)\\s(.*?)", 2, 3]
-      - ["(wget)\\s(.*?)", 2, 3]
-      - ["(zip)\\s(.*?)", 2, 3]
+      - ["(awk)\\s(.*?)", 1, 2]
+      - ["(find)\\s(.*?)", 1, 2]
+      - ["(git clone)\\s(.*?)", 1, 2]
+      - ["(sed)\\s(.*?)", 1, 2]
+      - ["(tar)\\s(.*?)", 1, 2]
+      - ["(wget)\\s(.*?)", 1, 2]
+      - ["(zip)\\s(.*?)", 1, 2]
 

ql/lib/ext/config/poisonable_steps.yml

@@ -63,12 +63,12 @@ extensions:
       extensible: poisonableLocalScriptsDataModel
     data:
       # TODO: It could also be in the form of `dir/cmd`
-      - ["(\\.\\/[a-zA-Z0-9\\-_\\./]+)(.*?)", 2]
-      - ["(\\.\\s+[a-zA-Z0-9\\-_\\./]+)(.*?)", 2] # eg: . venv/bin/activate
-      - ["(source|sh|bash|zsh|fish)\\s+(.*?)", 3]
-      - ["(node)\\s+(.*?)(\\.js|\\.ts)(.*?)", 3]
-      - ["(python)\\s+(.*?)\\.py(.*?)", 3]
-      - ["(ruby)\\s+(.*?)\\.rb(.*?)", 3]
-      - ["(go)\\s+(generate|run)\\s+(.*?)\\.go(.*?)", 4]
-      - ["(dotnet)\\s+(.*?)\\.csproj(.*?)", 3]
+      - ["(\\.\\/[^\\s]+)\\b", 1] # eg: ./venv/bin/activate
+      - ["(\\.\\s+[^\\s]+)\\b", 1] # eg: . venv/bin/activate
+      - ["(source|sh|bash|zsh|fish)\\s+([^\\s]+)\\b", 2]
+      - ["(node)\\s+([^\\s]+)(\\.js|\\.ts)\\b", 2]
+      - ["(python)\\s+([^\\s]+)\\.py\\b", 2]
+      - ["(ruby)\\s+([^\\s]+)\\.rb\\b", 2]
+      - ["(go)\\s+(generate|run)\\s+([^\\s]+)\\.go\\b", 3]
+      - ["(dotnet)\\s+([^\\s]+)\\.csproj\\b", 2]
 

ql/lib/ext/config/untrusted_git_commands.yml

@@ -4,29 +4,29 @@ extensions:
       extensible: untrustedGitCommandsDataModel 
     data:
       # FILES=$(git diff-tree --no-commit-id --name-only HEAD -r)
-      - [".*git\\b.*\\bdiff-tree\\b.*", "filename,multiline"]
+      - ["git\\b.*\\bdiff-tree\\b", "filename,multiline"]
       # CHANGES=$(git --no-pager diff --name-only $NAME | grep -v -f .droneignore);
       # CHANGES=$(git diff --name-only)
-      - [".*git\\b.*\\bdiff\\b.*", "filename,multiline"]
+      - ["git\\b.*\\bdiff\\b", "filename,multiline"]
       # COMMIT_MESSAGE=$(git log --format=%s -n 1)
-      - [".*git\\b.*\\blog\\b.*%s.*", "text,online"]
+      - ["git\\b.*\\blog\\b.*%s", "text,online"]
       # COMMIT_MESSAGE=$(git log --format=%B -n 1) 
-      - [".*git\\b.*\\blog\\b.*%B.*", "text,multiline"]
+      - ["git\\b.*\\blog\\b.*%B", "text,multiline"]
       # COMMIT_MESSAGE=$(git log --format=oneline)
-      - [".*git\\b.*\\blog\\b.*oneline.*", "text,oneline"]
+      - ["git\\b.*\\blog\\b.*oneline", "text,oneline"]
       # COMMIT_MESSAGE=$(git show -s --format=%B)
       # COMMIT_MESSAGE=$(git show -s --format=%s)
-      - [".*git\\b.*\\bshow\\b.*-s.*%s.*", "text,oneline"]
-      - [".*git\\b.*\\bshow\\b.*-s.*%B.*", "text,multiline"]
+      - ["git\\b.*\\bshow\\b.*-s.*%s", "text,oneline"]
+      - ["git\\b.*\\bshow\\b.*-s.*%B", "text,multiline"]
       # AUTHOR=$(git log -1 --pretty=format:'%an')
-      - [".*git\\b.*\\blog\\b.*%an.*", "username,oneline"]
+      - ["git\\b.*\\blog\\b.*%an", "username,oneline"]
       # AUTHOR=$(git show -s --pretty=%an)
-      - [".*git\\b.*\\bshow\\b.*%an.*", "username,oneline"]
+      - ["git\\b.*\\bshow\\b.*%an", "username,oneline"]
       # EMAIL=$(git log -1 --pretty=format:'%ae')
-      - [".*git\\b.*\\blog\\b.*%ae.*", "email,oneline"]
+      - ["git\\b.*\\blog\\b.*%ae", "email,oneline"]
       # EMAIL=$(git show -s --pretty=%ae)
-      - [".*git\\b.*\\bshow\\b.*%ae.*", "email,oneline"]
+      - ["git\\b.*\\bshow\\b.*%ae", "email,oneline"]
       # BRANCH=$(git branch --show-current)
-      - [".*git\\b.*\\bbranch\\b.*\\b--show-current\\b.*", "branch,oneline"]
+      - ["git\\b.*\\bbranch\\b.*\\b--show-current\\b", "branch,oneline"]
       # BRANCH=$(git rev-parse --abbrev-ref HEAD)
-      - [".*git\\b.*\\brev-parse\\b.*\\b--abbrev-ref\\b.*", "branch,oneline"]
+      - ["git\\b.*\\brev-parse\\b.*\\b--abbrev-ref\\b", "branch,oneline"]