hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2025-12-17 01:03:14 +01:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
Go: Update the QHelp for `go/command-injection`.
This commit is contained in:
Erik Krogh Kristensen
2024-05-17 17:45:10 +02:00
committed by GitHub
parent b639f60fa6 384649b336
commit bfc95c6f13
13 changed files with 266 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files

23
go/ql/lib/semmle/go/security/CommandInjectionCustomizations.qll
View File

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

12
go/ql/src/Security/CWE-078/CommandInjection.go
View File

@@ -1,12 +0,0 @@
package main
import (
"net/http"
"os/exec"
)
func handler(req *http.Request) {
cmdName := req.URL.Query()["cmd"][0]
cmd := exec.Command(cmdName)
cmd.Run()
}

43
go/ql/src/Security/CWE-078/CommandInjection.qhelp
View File

@@ -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="CommandInjection.go"/>
<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 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>
<p>
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>

4
go/ql/src/Security/CWE-078/StoredCommand.qhelp
View File

@@ -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>

14
go/ql/src/Security/CWE-078/examples/CommandInjection.go Normal file
View File

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

28
go/ql/src/Security/CWE-078/examples/CommandInjectionGood.go Normal file
View File

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

23
go/ql/src/Security/CWE-078/examples/CommandInjectionGood2.go Normal file
View File

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

31
go/ql/src/Security/CWE-078/examples/CommandInjectionGood3.go Normal file
View File

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

0
go/ql/src/Security/CWE-078/StoredCommand.go → go/ql/src/Security/CWE-078/examples/StoredCommand.go
View File

0
go/ql/src/Security/CWE-078/StoredCommandGood.go → go/ql/src/Security/CWE-078/examples/StoredCommandGood.go
View File

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

@@ -1,14 +1,26 @@
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:15:41:21 | selection of URL | CommandInjection2.go:41:15:41:29 | call to Query | provenance | MaD:735 |
| CommandInjection2.go:41:15:41:29 | call to Query | CommandInjection2.go:44:67:44:75 | imageName | provenance | |
| CommandInjection2.go:44:34:44:88 | []type{args} [array] | CommandInjection2.go:44:34:44:88 | call to Sprintf | provenance | MaD:245 |
| CommandInjection2.go:44:67:44:75 | imageName | CommandInjection2.go:44:34:44:88 | []type{args} [array] | provenance | |
| CommandInjection2.go:44:67:44:75 | imageName | CommandInjection2.go:44:34:44:88 | 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 |
| GitSubcommands.go:10:13:10:27 | call to Query | GitSubcommands.go:12:31:12:37 | tainted | provenance | |
| GitSubcommands.go:10:13:10:27 | call to Query | GitSubcommands.go:13:31:13:37 | tainted | provenance | |
| 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:11:13:11:19 | selection of URL | GitSubcommands.go:11:13:11:27 | call to Query | provenance | MaD:735 |
| GitSubcommands.go:11:13:11:27 | call to Query | GitSubcommands.go:13:31:13:37 | tainted | provenance | |
| GitSubcommands.go:11:13:11:27 | call to Query | GitSubcommands.go:14:31:14:37 | tainted | provenance | |
| GitSubcommands.go:11:13:11:27 | call to Query | GitSubcommands.go:15:30:15:36 | tainted | provenance | |
| GitSubcommands.go:11:13:11:27 | call to Query | GitSubcommands.go:16:35:16:41 | tainted | provenance | |
| GitSubcommands.go:11:13:11:27 | call to Query | GitSubcommands.go:17:36:17:42 | tainted | provenance | |
| GitSubcommands.go:33:13:33:19 | selection of URL | GitSubcommands.go:33:13:33:27 | call to Query | provenance | MaD:735 |
| GitSubcommands.go:33:13:33:27 | call to Query | GitSubcommands.go:38:32:38: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 | |
@@ -103,16 +115,29 @@ 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:15:41:21 | selection of URL | semmle.label | selection of URL |
| CommandInjection2.go:41:15:41:29 | call to Query | semmle.label | call to Query |
| CommandInjection2.go:44:34:44:88 | []type{args} [array] | semmle.label | []type{args} [array] |
| CommandInjection2.go:44:34:44:88 | call to Sprintf | semmle.label | call to Sprintf |
| CommandInjection2.go:44:67:44:75 | 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 |
| GitSubcommands.go:10:13:10:19 | selection of URL | semmle.label | selection of URL |
| GitSubcommands.go:10:13:10:27 | call to Query | semmle.label | call to Query |
| GitSubcommands.go:12:31:12:37 | tainted | semmle.label | tainted |
| GitSubcommands.go:11:13:11:19 | selection of URL | semmle.label | selection of URL |
| GitSubcommands.go:11:13:11:27 | call to Query | semmle.label | call to Query |
| GitSubcommands.go:13:31:13:37 | tainted | semmle.label | tainted |
| 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:14:31:14:37 | tainted | semmle.label | tainted |
| GitSubcommands.go:15:30:15:36 | tainted | semmle.label | tainted |
| GitSubcommands.go:16:35:16:41 | tainted | semmle.label | tainted |
| GitSubcommands.go:17:36:17:42 | tainted | semmle.label | tainted |
| GitSubcommands.go:33:13:33:19 | selection of URL | semmle.label | selection of URL |
| GitSubcommands.go:33:13:33:27 | call to Query | semmle.label | call to Query |
| GitSubcommands.go:38:32:38: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] |
@@ -195,12 +220,15 @@ 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:44:34:44:88 | call to Sprintf | CommandInjection2.go:41:15:41:21 | selection of URL | CommandInjection2.go:44:34:44:88 | call to Sprintf | This command depends on a $@. | CommandInjection2.go:41:15:41:21 | 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 |
| 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:13:31:13:37 | tainted | GitSubcommands.go:11:13:11:19 | selection of URL | GitSubcommands.go:13:31:13:37 | tainted | This command depends on a $@. | GitSubcommands.go:11:13:11:19 | selection of URL | user-provided value |
| GitSubcommands.go:14:31:14:37 | tainted | GitSubcommands.go:11:13:11:19 | selection of URL | GitSubcommands.go:14:31:14:37 | tainted | This command depends on a $@. | GitSubcommands.go:11:13:11:19 | selection of URL | user-provided value |
| GitSubcommands.go:15:30:15:36 | tainted | GitSubcommands.go:11:13:11:19 | selection of URL | GitSubcommands.go:15:30:15:36 | tainted | This command depends on a $@. | GitSubcommands.go:11:13:11:19 | selection of URL | user-provided value |
| GitSubcommands.go:16:35:16:41 | tainted | GitSubcommands.go:11:13:11:19 | selection of URL | GitSubcommands.go:16:35:16:41 | tainted | This command depends on a $@. | GitSubcommands.go:11:13:11:19 | selection of URL | user-provided value |
| GitSubcommands.go:17:36:17:42 | tainted | GitSubcommands.go:11:13:11:19 | selection of URL | GitSubcommands.go:17:36:17:42 | tainted | This command depends on a $@. | GitSubcommands.go:11:13:11:19 | selection of URL | user-provided value |
| GitSubcommands.go:38:32:38:38 | tainted | GitSubcommands.go:33:13:33:19 | selection of URL | GitSubcommands.go:38:32:38:38 | tainted | This command depends on a $@. | GitSubcommands.go:33:13:33: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 |

55
go/ql/test/query-tests/Security/CWE-078/CommandInjection2.go Normal file
View File

@@ -0,0 +1,55 @@
package main
import (
"fmt"
"log"
"net/http"
"os"
"os/exec"
"regexp"
)
func handlerExample(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 handlerExample2(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 handlerExample3(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()
// Validate the imageName with a regular expression
validImageName := regexp.MustCompile(`^[a-zA-Z0-9_\-\.]+$`)
if !validImageName.MatchString(imageName) {
log.Fatal("Invalid image name")
}
cmd2 := exec.Command("sh", "-c", fmt.Sprintf("imagetool %s > %s", imageName, outputPath)) // OK
cmd2.Run()
}

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

@@ -3,6 +3,7 @@ package main
import (
"net/http"
"os/exec"
"strings"
)
// BAD: using git subcommands that are vulnerable to arbitrary remote command execution
@@ -26,3 +27,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 "--"
}
}