add a sanitizer that checks that the string does not start with "--"

Author: erik-krogh

go/ql/lib/semmle/go/security/CommandInjectionCustomizations.qll

@@ -52,4 +52,21 @@ module CommandInjection {
    * A call to a regexp match function, considered as a barrier guard for command injection.
    */
   class RegexpCheckBarrierAsSanitizer extends Sanitizer instanceof RegexpCheckBarrier { }
+
+  private predicate noDoubleDashPrefixCheck(DataFlow::Node hasPrefixNode, Expr e, boolean branch) {
+    exists(StringOps::HasPrefix hasPrefix | hasPrefix = hasPrefixNode |
+      e = hasPrefix.getBaseString().asExpr() and
+      hasPrefix.getSubstring().asExpr().getStringValue() = "--" and
+      branch = false
+    )
+  }
+
+  /**
+   * A call that confirms that the string does not start with `--`, considered as a barrier guard for command injection.
+   */
+  class NoDoubleDashPrefixSanitizer extends Sanitizer {
+    NoDoubleDashPrefixSanitizer() {
+      this = DataFlow::BarrierGuard<noDoubleDashPrefixCheck/3>::getABarrierNode()
+    }
+  }
 }

go/ql/test/query-tests/Security/CWE-078/CommandInjection.expected

@@ -14,6 +14,8 @@ edges
 | GitSubcommands.go:10:13:10:27 | call to Query | GitSubcommands.go:14:30:14:36 | tainted | provenance |  |
 | GitSubcommands.go:10:13:10:27 | call to Query | GitSubcommands.go:15:35:15:41 | tainted | provenance |  |
 | GitSubcommands.go:10:13:10:27 | call to Query | GitSubcommands.go:16:36:16:42 | tainted | provenance |  |
+| GitSubcommands.go:32:13:32:19 | selection of URL | GitSubcommands.go:32:13:32:27 | call to Query | provenance | MaD:735 |
+| GitSubcommands.go:32:13:32:27 | call to Query | GitSubcommands.go:37:32:37:38 | tainted | provenance |  |
 | SanitizingDoubleDash.go:9:13:9:19 | selection of URL | SanitizingDoubleDash.go:9:13:9:27 | call to Query | provenance | MaD:735 |
 | SanitizingDoubleDash.go:9:13:9:27 | call to Query | SanitizingDoubleDash.go:13:25:13:31 | tainted | provenance |  |
 | SanitizingDoubleDash.go:9:13:9:27 | call to Query | SanitizingDoubleDash.go:14:23:14:33 | slice expression | provenance |  |
@@ -123,6 +125,9 @@ nodes
 | GitSubcommands.go:14:30:14:36 | tainted | semmle.label | tainted |
 | GitSubcommands.go:15:35:15:41 | tainted | semmle.label | tainted |
 | GitSubcommands.go:16:36:16:42 | tainted | semmle.label | tainted |
+| GitSubcommands.go:32:13:32:19 | selection of URL | semmle.label | selection of URL |
+| GitSubcommands.go:32:13:32:27 | call to Query | semmle.label | call to Query |
+| GitSubcommands.go:37:32:37:38 | tainted | semmle.label | tainted |
 | SanitizingDoubleDash.go:9:13:9:19 | selection of URL | semmle.label | selection of URL |
 | SanitizingDoubleDash.go:9:13:9:27 | call to Query | semmle.label | call to Query |
 | SanitizingDoubleDash.go:13:15:13:32 | array literal [array] | semmle.label | array literal [array] |
@@ -212,6 +217,7 @@ subpaths
 | GitSubcommands.go:14:30:14:36 | tainted | GitSubcommands.go:10:13:10:19 | selection of URL | GitSubcommands.go:14:30:14:36 | tainted | This command depends on a $@. | GitSubcommands.go:10:13:10:19 | selection of URL | user-provided value |
 | GitSubcommands.go:15:35:15:41 | tainted | GitSubcommands.go:10:13:10:19 | selection of URL | GitSubcommands.go:15:35:15:41 | tainted | This command depends on a $@. | GitSubcommands.go:10:13:10:19 | selection of URL | user-provided value |
 | GitSubcommands.go:16:36:16:42 | tainted | GitSubcommands.go:10:13:10:19 | selection of URL | GitSubcommands.go:16:36:16:42 | tainted | This command depends on a $@. | GitSubcommands.go:10:13:10:19 | selection of URL | user-provided value |
+| GitSubcommands.go:37:32:37:38 | tainted | GitSubcommands.go:32:13:32:19 | selection of URL | GitSubcommands.go:37:32:37:38 | tainted | This command depends on a $@. | GitSubcommands.go:32:13:32:19 | selection of URL | user-provided value |
 | SanitizingDoubleDash.go:14:23:14:33 | slice expression | SanitizingDoubleDash.go:9:13:9:19 | selection of URL | SanitizingDoubleDash.go:14:23:14:33 | slice expression | This command depends on a $@. | SanitizingDoubleDash.go:9:13:9:19 | selection of URL | user-provided value |
 | SanitizingDoubleDash.go:40:23:40:30 | arrayLit | SanitizingDoubleDash.go:9:13:9:19 | selection of URL | SanitizingDoubleDash.go:40:23:40:30 | arrayLit | This command depends on a $@. | SanitizingDoubleDash.go:9:13:9:19 | selection of URL | user-provided value |
 | SanitizingDoubleDash.go:54:23:54:30 | arrayLit | SanitizingDoubleDash.go:9:13:9:19 | selection of URL | SanitizingDoubleDash.go:54:23:54:30 | arrayLit | This command depends on a $@. | SanitizingDoubleDash.go:9:13:9:19 | selection of URL | user-provided value |

go/ql/test/query-tests/Security/CWE-078/GitSubcommands.go

@@ -1,8 +1,8 @@
 package main
-
 import (
 	"net/http"
 	"os/exec"
+	"strings"
 )
 
 // BAD: using git subcommands that are vulnerable to arbitrary remote command execution
@@ -26,3 +26,14 @@ func gitSubcommandsGood(req *http.Request) {
 	exec.Command("git", "merge", tainted)
 	exec.Command("git", "add", tainted)
 }
+
+// BAD: using git subcommands that are vulnerable to arbitrary remote command execution
+func gitSubcommandsGood2(req *http.Request) {
+	tainted := req.URL.Query()["cmd"][0]
+
+	if !strings.HasPrefix(tainted, "--") {
+		exec.Command("git", "clone", tainted) // GOOD, `tainted` cannot start with "--"
+	} else {
+		exec.Command("git", "clone", tainted) // BAD, `tainted` can start with "--"
+	}
+}