Merge pull request #16510 from erik-krogh/go-command

Go: Update the QHelp for `go/command-injection`.

Author: erik-krogh

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

@@ -5,6 +5,7 @@
  */
 
 import go
+private import semmle.go.dataflow.barrierguardutil.RegexpCheck
 
 /**
  * Provides extension points for customizing the taint tracking configuration for reasoning about
@@ -45,4 +46,26 @@ module CommandInjection {
 
     override predicate doubleDashIsSanitizing() { exec.doubleDashIsSanitizing() }
   }
+
+  /**
+   * 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/src/Security/CWE-078/CommandInjection.go

@@ -12,26 +12,49 @@ a malicious user may be able to run commands to exfiltrate data or compromise th
 
 <recommendation>
 <p>
-If possible, use hard-coded string literals to specify the command to run. Instead of interpreting
-user input directly as command names, examine the input and then choose among hard-coded string
-literals.
+Whenever possible, use hard-coded string literals for commands and avoid shell 
+string interpreters like <code>sh -c</code>.
 </p>
 <p>
-If this is not possible, then add sanitization code to verify that the user input is safe before
-using it.
+If given arguments as a single string, avoid simply splitting the string on 
+whitespace. Arguments may contain quoted whitespace, causing them to split into 
+multiple arguments.
+</p>
+<p>
+If this is not possible, sanitize user input to avoid characters like spaces and
+various kinds of quotes that can alter the meaning of the command.
 </p>
 </recommendation>
 
 <example>
 <p>
-In the following example, assume the function <code>handler</code> is an HTTP request handler in a
-web application, whose parameter <code>req</code> contains the request object:
+In the following example, assume the function <code>handler</code> is an HTTP request
+handler in a web application, whose parameter <code>req</code> contains the request object:
+</p>
+<sample src="examples/CommandInjection.go"/>
+<p>
+The handler extracts the image file name from the request and uses the image name to construct a 
+shell command that is executed using <code>`sh -c`</code>, which can lead to command injection.
+</p>
+<p>
+It's better to avoid shell commands by using the <code>exec.Command</code> function directly, 
+as shown in the following example:
+</p>
+<sample src="examples/CommandInjectionGood.go"/>
+<p>
+Alternatively, a regular expression can be used to ensure that the image name is safe to use 
+in a shell command:
+</p>
+<sample src="examples/CommandInjectionGood2.go"/>
+<p>
+Some commands, like <code>git</code>, can indirectly execute commands if an attacker specifies
+the flags given to the command. 
 </p>
-<sample src="CommandInjection.go"/>
 <p>
-The handler extracts the name of a system command from the request object, and then runs it without
-any further checks, which can cause a command-injection vulnerability.
+To mitigate this risk, either add a <code>--</code> argument to ensure subsequent arguments are
+not interpreted as flags, or verify that the argument does not start with <code>"--"</code>.
 </p>
+<sample src="examples/CommandInjectionGood3.go"/>
 </example>
 <references>
 <li>

go/ql/src/Security/CWE-078/CommandInjection.qhelp

@@ -28,13 +28,13 @@ using it.
 In the following example, the function <code>run</code> runs a command directly from the result of a
 query:
 </p>
-<sample src="StoredCommand.go"/>
+<sample src="examples/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"/>
+<sample src="examples/StoredCommandGood.go"/>
 </example>
 
 <references>

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

@@ -0,0 +1,14 @@
+package main
+
+import (
+	"net/http"
+	"os/exec"
+)
+
+func handler(req *http.Request) {
+	imageName := req.URL.Query()["imageName"][0]
+	outputPath := "/tmp/output.svg"
+	cmd := exec.Command("sh", "-c", fmt.Sprintf("imagetool %s > %s", imageName, outputPath))
+	cmd.Run()
+	// ...
+}

go/ql/src/Security/CWE-078/examples/CommandInjection.go

@@ -0,0 +1,28 @@
+package main
+
+import (
+	"log"
+	"net/http"
+	"os"
+	"os/exec"
+)
+
+func handler(req *http.Request) {
+	imageName := req.URL.Query()["imageName"][0]
+	outputPath := "/tmp/output.svg"
+
+	// Create the output file
+	outfile, err := os.Create(outputPath)
+	if err != nil {
+		log.Fatal(err)
+	}
+	defer outfile.Close()
+
+	// Prepare the command
+	cmd := exec.Command("imagetool", imageName)
+
+	// Set the output to our file
+	cmd.Stdout = outfile
+
+	cmd.Run()
+}

go/ql/src/Security/CWE-078/examples/CommandInjectionGood.go

@@ -0,0 +1,23 @@
+package main
+
+import (
+	"log"
+	"net/http"
+	"os/exec"
+	"regexp"
+)
+
+func handler(req *http.Request) {
+	imageName := req.URL.Query()["imageName"][0]
+	outputPath := "/tmp/output.svg"
+
+	// Validate the imageName with a regular expression
+	validImageName := regexp.MustCompile(`^[a-zA-Z0-9_\-\.]+$`)
+	if !validImageName.MatchString(imageName) {
+		log.Fatal("Invalid image name")
+		return
+	}
+
+	cmd := exec.Command("sh", "-c", fmt.Sprintf("imagetool %s > %s", imageName, outputPath))
+	cmd.Run()
+}

go/ql/src/Security/CWE-078/examples/CommandInjectionGood2.go

@@ -0,0 +1,31 @@
+package main
+
+import (
+	"log"
+	"net/http"
+	"os/exec"
+	"strings"
+)
+
+func handler(req *http.Request) {
+	repoURL := req.URL.Query()["repoURL"][0]
+	outputPath := "/tmp/repo"
+
+	// Sanitize the repoURL to ensure it does not start with "--"
+	if strings.HasPrefix(repoURL, "--") {
+		log.Fatal("Invalid repository URL")
+	} else {
+		cmd := exec.Command("git", "clone", repoURL, outputPath)
+		err := cmd.Run()
+		if err != nil {
+			log.Fatal(err)
+		}
+	}
+
+	// Or: add "--" to ensure that the repoURL is not interpreted as a flag
+	cmd := exec.Command("git", "clone", "--", repoURL, outputPath)
+	err := cmd.Run()
+	if err != nil {
+		log.Fatal(err)
+	}
+}