Add StoredCommand query

change-notes/2020-11-11-stored-command.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* A new query "Command built from stored data" (`go/stored-command`) has been added. The query detects command executions that contain data from a database or a similar possibly user-controllable source.

ql/src/Security/CWE-078/StoredCommand.go

@@ -0,0 +1,16 @@
+package main
+
+import (
+	"database/sql"
+	"os/exec"
+)
+
+var db *sql.DB
+
+func run(query string) {
+	rows, _ := db.Query(query)
+	var cmdName string
+	rows.Scan(&cmdName)
+	cmd := exec.Command(cmdName)
+	cmd.Run()
+}

ql/src/Security/CWE-078/StoredCommand.qhelp

@@ -0,0 +1,45 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+If a system command invocation is built from stored data without sufficient sanitization, and that
+data is stored from a user input, a malicious user may be able to run commands to exfiltrate data or
+compromise the system.
+</p>
+</overview>
+
+<recommendation>
+<p>
+If possible, use hard-coded string literals to specify the command to run. Instead of interpreting
+stored input directly as command names, examine the input and then choose among hard-coded string
+literals.
+</p>
+<p>
+If this is not possible, then add sanitization code to verify that the user input is safe before
+using it.
+</p>
+</recommendation>
+
+<example>
+<p>
+In the following example, the function <code>run</code> runs a command directly from the result of a
+query:
+</p>
+<sample src="StoredCommand.go"/>
+<p>
+The function extracts the name of a system command from the database query, and then runs it without
+any further checks, which can cause a command-injection vulnerability. A possible solution is to
+ensure that commands are checked against a whitelist:
+</p>
+<sample src="StoredCommandGood.go"/>
+</example>
+
+<references>
+<li>
+OWASP: <a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
+</li>
+</references>
+</qhelp>

ql/src/Security/CWE-078/StoredCommand.ql

@@ -0,0 +1,20 @@
+/**
+ * @name Command built from stored data
+ * @description Building a system command from stored data that is user-controlled
+ *              can lead to execution of malicious code by the user.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision low
+ * @id go/stored-command
+ * @tags security
+ *       external/cwe/cwe-078
+ */
+
+import go
+import semmle.go.security.StoredCommand
+import DataFlow::PathGraph
+
+from StoredCommand::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "This command depends on $@.", source.getNode(),
+  "a stored value"

ql/src/Security/CWE-078/StoredCommandGood.go

@@ -0,0 +1,18 @@
+package main
+
+import (
+	"database/sql"
+	"os/exec"
+)
+
+var db *sql.DB
+
+func run(query string) {
+	rows, _ := db.Query(query)
+	var cmdName string
+	rows.Scan(&cmdName)
+	if cmdName == "mybinary1" || cmdName == "mybinary2" {
+		cmd := exec.Command(cmdName)
+	}
+	cmd.Run()
+}

ql/src/semmle/go/security/StoredCommand.qll

@@ -0,0 +1,42 @@
+/**
+ * Provides a taint tracking configuration for reasoning about command
+ * injection vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if
+ * `StoredCommand::Configuration` is needed, otherwise
+ * `StoredCommandCustomizations` should be imported instead.
+ */
+
+import go
+import StoredXssCustomizations
+import CommandInjectionCustomizations
+
+/**
+ * Provides a taint tracking configuration for reasoning about command
+ * injection vulnerabilities.
+ */
+module StoredCommand {
+  /**
+   * A taint-tracking configuration for reasoning about command-injection vulnerabilities.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "StoredCommand" }
+
+    override predicate isSource(DataFlow::Node source) {
+      source instanceof StoredXss::Source and
+      // exclude file names, since those are not generally an issue
+      not source instanceof StoredXss::FileNameSource
+    }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof CommandInjection::Sink }
+
+    override predicate isSanitizer(DataFlow::Node node) {
+      super.isSanitizer(node) or
+      node instanceof CommandInjection::Sanitizer
+    }
+
+    override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+      guard instanceof CommandInjection::SanitizerGuard
+    }
+  }
+}

ql/test/query-tests/Security/CWE-078/StoredCommand.expected

@@ -0,0 +1,7 @@
+edges
+| StoredCommand.go:11:2:11:27 | ... := ...[0] : pointer type | StoredCommand.go:14:22:14:28 | cmdName |
+nodes
+| StoredCommand.go:11:2:11:27 | ... := ...[0] : pointer type | semmle.label | ... := ...[0] : pointer type |
+| StoredCommand.go:14:22:14:28 | cmdName | semmle.label | cmdName |
+#select
+| StoredCommand.go:14:22:14:28 | cmdName | StoredCommand.go:11:2:11:27 | ... := ...[0] : pointer type | StoredCommand.go:14:22:14:28 | cmdName | This command depends on $@. | StoredCommand.go:11:2:11:27 | ... := ...[0] | a stored value |

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

@@ -0,0 +1,16 @@
+package main
+
+import (
+	"database/sql"
+	"os/exec"
+)
+
+var db *sql.DB
+
+func run(query string) {
+	rows, _ := db.Query(query)
+	var cmdName string
+	rows.Scan(&cmdName)
+	cmd := exec.Command(cmdName)
+	cmd.Run()
+}

ql/test/query-tests/Security/CWE-078/StoredCommand.qlref

@@ -0,0 +1 @@
+Security/CWE-078/StoredCommand.ql