make a new go/command-injection qhelp

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

@@ -12,26 +12,42 @@ 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.
+If possible, use hard-coded string literals to specify the command to run, and avoid using 
+shell string interpreters such as <code>sh -c</code> to execute shell commands. 
 </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, then add sanitization code to verify that the user input is 
+safe before using it, thereby avoiding characters that can change the meaning of the 
+command such as spaces and quotes.
 </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 name of a system command from the request object, and then runs it without
-any further checks, which can cause a command-injection vulnerability.
+The handler extracts the name of an image file from the request object, and then runs a command
+to process the image. The command is constructed by concatenating the image path and the output path, 
+and then running it with <code>sh -c</code>. This can cause a command-injection vulnerability.
 </p>
+<p>
+It's better to avoid shell strings 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"/>
 </example>
 <references>
 <li>

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

@@ -6,7 +6,9 @@ import (
 )
 
 func handler(req *http.Request) {
-	cmdName := req.URL.Query()["cmd"][0]
-	cmd := exec.Command(cmdName)
+	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/CommandInjectionGood.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/CommandInjectionGood2.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/test/query-tests/Security/CWE-078/CommandInjection.expected

@@ -1,6 +1,16 @@
 edges
 | ArgumentInjection.go:9:10:9:16 | selection of URL | ArgumentInjection.go:9:10:9:24 | call to Query | provenance | MaD:735 |
 | ArgumentInjection.go:9:10:9:24 | call to Query | ArgumentInjection.go:10:31:10:34 | path | provenance |  |
+| CommandInjection2.go:13:15:13:21 | selection of URL | CommandInjection2.go:13:15:13:29 | call to Query | provenance | MaD:735 |
+| CommandInjection2.go:13:15:13:29 | call to Query | CommandInjection2.go:15:67:15:75 | imageName | provenance |  |
+| CommandInjection2.go:15:34:15:88 | []type{args} [array] | CommandInjection2.go:15:34:15:88 | call to Sprintf | provenance | MaD:245 |
+| CommandInjection2.go:15:67:15:75 | imageName | CommandInjection2.go:15:34:15:88 | []type{args} [array] | provenance |  |
+| CommandInjection2.go:15:67:15:75 | imageName | CommandInjection2.go:15:34:15:88 | call to Sprintf | provenance | FunctionModel |
+| CommandInjection2.go:41:18:41:24 | selection of URL | CommandInjection2.go:41:18:41:32 | call to Query | provenance | MaD:735 |
+| CommandInjection2.go:41:18:41:32 | call to Query | CommandInjection2.go:51:70:51:78 | imageName | provenance |  |
+| CommandInjection2.go:51:37:51:91 | []type{args} [array] | CommandInjection2.go:51:37:51:91 | call to Sprintf | provenance | MaD:245 |
+| CommandInjection2.go:51:70:51:78 | imageName | CommandInjection2.go:51:37:51:91 | []type{args} [array] | provenance |  |
+| CommandInjection2.go:51:70:51:78 | imageName | CommandInjection2.go:51:37:51:91 | call to Sprintf | provenance | FunctionModel |
 | CommandInjection.go:9:13:9:19 | selection of URL | CommandInjection.go:9:13:9:27 | call to Query | provenance | MaD:735 |
 | CommandInjection.go:9:13:9:27 | call to Query | CommandInjection.go:10:22:10:28 | cmdName | provenance |  |
 | GitSubcommands.go:10:13:10:19 | selection of URL | GitSubcommands.go:10:13:10:27 | call to Query | provenance | MaD:735 |
@@ -103,6 +113,16 @@ nodes
 | ArgumentInjection.go:9:10:9:16 | selection of URL | semmle.label | selection of URL |
 | ArgumentInjection.go:9:10:9:24 | call to Query | semmle.label | call to Query |
 | ArgumentInjection.go:10:31:10:34 | path | semmle.label | path |
+| CommandInjection2.go:13:15:13:21 | selection of URL | semmle.label | selection of URL |
+| CommandInjection2.go:13:15:13:29 | call to Query | semmle.label | call to Query |
+| CommandInjection2.go:15:34:15:88 | []type{args} [array] | semmle.label | []type{args} [array] |
+| CommandInjection2.go:15:34:15:88 | call to Sprintf | semmle.label | call to Sprintf |
+| CommandInjection2.go:15:67:15:75 | imageName | semmle.label | imageName |
+| CommandInjection2.go:41:18:41:24 | selection of URL | semmle.label | selection of URL |
+| CommandInjection2.go:41:18:41:32 | call to Query | semmle.label | call to Query |
+| CommandInjection2.go:51:37:51:91 | []type{args} [array] | semmle.label | []type{args} [array] |
+| CommandInjection2.go:51:37:51:91 | call to Sprintf | semmle.label | call to Sprintf |
+| CommandInjection2.go:51:70:51:78 | imageName | semmle.label | imageName |
 | CommandInjection.go:9:13:9:19 | selection of URL | semmle.label | selection of URL |
 | CommandInjection.go:9:13:9:27 | call to Query | semmle.label | call to Query |
 | CommandInjection.go:10:22:10:28 | cmdName | semmle.label | cmdName |
@@ -195,6 +215,8 @@ nodes
 subpaths
 #select
 | ArgumentInjection.go:10:31:10:34 | path | ArgumentInjection.go:9:10:9:16 | selection of URL | ArgumentInjection.go:10:31:10:34 | path | This command depends on a $@. | ArgumentInjection.go:9:10:9:16 | selection of URL | user-provided value |
+| CommandInjection2.go:15:34:15:88 | call to Sprintf | CommandInjection2.go:13:15:13:21 | selection of URL | CommandInjection2.go:15:34:15:88 | call to Sprintf | This command depends on a $@. | CommandInjection2.go:13:15:13:21 | selection of URL | user-provided value |
+| CommandInjection2.go:51:37:51:91 | call to Sprintf | CommandInjection2.go:41:18:41:24 | selection of URL | CommandInjection2.go:51:37:51:91 | call to Sprintf | This command depends on a $@. | CommandInjection2.go:41:18:41:24 | selection of URL | user-provided value |
 | CommandInjection.go:10:22:10:28 | cmdName | CommandInjection.go:9:13:9:19 | selection of URL | CommandInjection.go:10:22:10:28 | cmdName | This command depends on a $@. | CommandInjection.go:9:13:9:19 | selection of URL | user-provided value |
 | GitSubcommands.go:12:31:12:37 | tainted | GitSubcommands.go:10:13:10:19 | selection of URL | GitSubcommands.go:12:31:12:37 | tainted | This command depends on a $@. | GitSubcommands.go:10:13:10:19 | selection of URL | user-provided value |
 | GitSubcommands.go:13:31:13:37 | tainted | GitSubcommands.go:10:13:10:19 | selection of URL | GitSubcommands.go:13:31:13:37 | tainted | This command depends on a $@. | GitSubcommands.go:10:13:10:19 | selection of URL | user-provided value |

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

@@ -0,0 +1,53 @@
+package main
+
+import (
+	"net/http"
+	"os/exec"
+	"log"
+    "os"
+    "regexp"
+	"fmt"
+)
+
+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)) // NOT OK - correctly flagged
+  	cmd.Run()
+  // ...
+}
+
+func handler2(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) // OK - and not flagged
+
+    // Set the output to our file
+    cmd.Stdout = outfile
+
+    cmd.Run()
+}
+
+func handler3(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)) // OK - but falsely flagged
+    cmd.Run()
+}