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

[erik-krogh]

Also fixed some FPs that appeared in the fix-examples in the new QHelp.

My friend likes this new QHelp.

An evaluation was very uneventful.
No change in results or performance.

[github-actions]

QHelp previews:

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

Command built from user-controlled sources

If a system command invocation is built from user-provided data without sufficient sanitization, a malicious user may be able to run commands to exfiltrate data or compromise the system.

Recommendation

Whenever possible, use hard-coded string literals for commands and avoid shell string interpreters like sh -c.

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.

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.

Example

In the following example, assume the function handler is an HTTP request handler in a web application, whose parameter req contains the request object:

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()
	// ...
}

The handler extracts the image file name from the request and uses the image name to construct a shell command that is executed using `sh -c`, which can lead to command injection.

It's better to avoid shell commands by using the exec.Command function directly, as shown in the following example:

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()
}

Alternatively, a regular expression can be used to ensure that the image name is safe to use in a shell command:

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()
}

Some commands, like git, can indirectly execute commands if an attacker specifies the flags given to the command.

To mitigate this risk, either add a -- argument to ensure subsequent arguments are not interpreted as flags, or verify that the argument does not start with "--".

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)
	}
}

References

• OWASP: Command Injection.
• Common Weakness Enumeration: CWE-78.

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

Command built from stored data

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.

Recommendation

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.

If this is not possible, then add sanitization code to verify that the user input is safe before using it.

Example

In the following example, the function run runs a command directly from the result of a query:

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()
}

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:

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()
}

References

• OWASP: Command Injection.
• Common Weakness Enumeration: CWE-78.

[owen-mc]

@erik-krogh I see that the formatting CI check failed and you fixed it. I worked on it a while ago to make it print out the files that needed to be reformatted, which it hadn't been doing. Did it work for you? Was it easy to see what had failed and what you needed to do?

[erik-krogh]

@erik-krogh I see that the formatting CI check failed and you fixed it. I worked on it a while ago to make it print out the files that needed to be reformatted, which it hadn't been doing. Did it work for you? Was it easy to see what had failed and what you needed to do?

Yes, that was helpful 👍

[owen-mc]

Generally this looks good. @atorralba should probably do a quick review as well, since I don't have much app sec experience.

[owen-mc]

We normally put imports at the top of the module (unless we're importing a module that has just been defined), so for consistency please move it up to line 14.

[owen-mc]

I don't think you need a return here, since log.Fatal will always exit the program. But I guess there's no harm in having it.

[atorralba]

@atorralba should probably do a quick review as well

The -- sanitizer I like — for argument injection. If I understand it correctly, it's now a sanitizer for the whole command injection query, so would something like this be marked as safe?

func handler(req *http.Request) {
    cmd := req.URL.Query()["cmd"][0]
    if strings.HasPrefix(cmd, "--") {
        log.Fatal("Invalid cmd")
    }
    cmd := exec.Command(cmd)
    cmd.Run()
}

If that's the case, this seems dangerous and prone to FNs.

The regex one is a tradeoff: there will be regexes that correctly sanitize the problem, and ones that not. With this sanitizer, we're prioritizing having less FPs in exchange of risking more FNs, which I'm not sure is what we want right now.

Ideally, we'd be able to analyze the regex to ensure that it either:

• Is an allow-list of characters that doesn't include whitespace, or
• Is a disallow-list of characters that includes whitespace.

Of course, this can get very complicated very quickly, and is probably overkill, so I'll leave it up to you if you want to merge as is, improve the sanitizer, or remove it entirely.

Maybe a decent alternative would be accepting less complex solutions that check the presence of whitespaces, like string.Contains and the like?

[erik-krogh]

The -- sanitizer I like — for argument injection. If I understand it correctly, it's now a sanitizer for the whole command injection query, so would something like this be marked as safe?

Yes, that example would spuriously be marked as safe.
And no, I don't think that's a problem.
The query doesn't currently use flow-labels, which would allow me to distinguish command-names and command-arguments, so my new sanitizer just sanitizes everything.

I don't think it's a problem that the sanitizer just sanitizes everything, because the -- prefix check only makes sense in the places where it actually sanitizes the value.
The example you showed does not really make sense, because a command-name will never start with --, so it wouldn't make sense for that check to be there.
If you look at how "--" prefix checks are used in the wild, you can see that it's used for parsing/sanitizing arguments.

So yes, FNs are possible, but I think those would be weird.

Of course, this can get very complicated very quickly, and is probably overkill, so I'll leave it up to you if you want to merge as is, improve the sanitizer, or remove it entirely.

We could do something nice, if Go had an implementation of the shared NFA libraries.
I could do a regular expression that tries to match regular expressions, but I don't think that would be food.
So I want to keep it as is.

Maybe a decent alternative would be accepting less complex solutions that check the presence of whitespaces, like string.Contains and the like?

Checking just for whitespace is not a valid sanitizer for anything related to command-injection? Can you give me an example of what you mean?

[atorralba]

Checking just for whitespace is not a valid sanitizer for anything related to command-injection? Can you give me an example of what you mean?

Yep, I know it's not enough, I was just referring to this part of the QHelp ⬆️ in case you wanted to do something more specific and simpler without using regex. But it wouldn't still be perfect of course.

[erik-krogh]

I wanted to mention which characters are dangerous, just to provide some context.
But deny-list based sanitizers for command injection are generally a bad idea, which is why I didn't do it that way in the example.

[atorralba]

In general, I won't challenge your decisions. As long as we're collectively aware of the shortcomings of these sanitizers, and decide they're worth it, then I have nothing against this PR. :)

[owen-mc]

It's definitely a thing we do elsewhere in the Go library to consider any regexp check a sanitizer without confirming that they've actually checked for the right thing. So I think it's fine to do here too.