Skip to content

Commit 761f9ca

Browse files
erik-krogherik-krogh
committed
make a new go/command-injection qhelp
1 parent e2a4c2a commit 761f9ca

File tree

6 files changed

+156
-12
lines changed

6 files changed

+156
-12
lines changed

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

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,26 +12,42 @@ a malicious user may be able to run commands to exfiltrate data or compromise th
1212

1313
<recommendation>
1414
<p>
15-
If possible, use hard-coded string literals to specify the command to run. Instead of interpreting
16-
user input directly as command names, examine the input and then choose among hard-coded string
17-
literals.
15+
If possible, use hard-coded string literals to specify the command to run, and avoid using
16+
shell string interpreters such as <code>sh -c</code> to execute shell commands.
1817
</p>
1918
<p>
20-
If this is not possible, then add sanitization code to verify that the user input is safe before
21-
using it.
19+
If given arguments as a single string, avoid simply splitting the string on
20+
whitespace. Arguments may contain quoted whitespace, causing them to split into
21+
multiple arguments.
22+
</p>
23+
<p>
24+
If this is not possible, then add sanitization code to verify that the user input is
25+
safe before using it, thereby avoiding characters that can change the meaning of the
26+
command such as spaces and quotes.
2227
</p>
2328
</recommendation>
2429

2530
<example>
2631
<p>
27-
In the following example, assume the function <code>handler</code> is an HTTP request handler in a
28-
web application, whose parameter <code>req</code> contains the request object:
32+
In the following example, assume the function <code>handler</code> is an HTTP request
33+
handler in a web application, whose parameter <code>req</code> contains the request object:
2934
</p>
3035
<sample src="examples/CommandInjection.go"/>
3136
<p>
32-
The handler extracts the name of a system command from the request object, and then runs it without
33-
any further checks, which can cause a command-injection vulnerability.
37+
The handler extracts the name of an image file from the request object, and then runs a command
38+
to process the image. The command is constructed by concatenating the image path and the output path,
39+
and then running it with <code>sh -c</code>. This can cause a command-injection vulnerability.
40+
</p>
41+
<p>
42+
It's better to avoid shell strings by using the <code>exec.Command</code> function directly,
43+
as shown in the following example:
44+
</p>
45+
<sample src="examples/CommandInjectionGood.go"/>
46+
<p>
47+
Alternatively, a regular expression can be used to ensure that the image name is safe to use
48+
in a shell command:
3449
</p>
50+
<sample src="examples/CommandInjectionGood2.go"/>
3551
</example>
3652
<references>
3753
<li>

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@ import (
66
)
77

88
func handler(req *http.Request) {
9-
cmdName := req.URL.Query()["cmd"][0]
10-
cmd := exec.Command(cmdName)
9+
imageName := req.URL.Query()["imageName"][0]
10+
outputPath = "/tmp/output.svg"
11+
cmd := exec.Command("sh", "-c", fmt.Sprintf("imagetool %s > %s", imageName, outputPath))
1112
cmd.Run()
12-
}
13+
// ...
14+
}

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
package main
2+
3+
import (
4+
"log"
5+
"net/http"
6+
"os"
7+
"os/exec"
8+
)
9+
10+
func handler(req *http.Request) {
11+
imageName := req.URL.Query()["imageName"][0]
12+
outputPath := "/tmp/output.svg"
13+
14+
// Create the output file
15+
outfile, err := os.Create(outputPath)
16+
if err != nil {
17+
log.Fatal(err)
18+
}
19+
defer outfile.Close()
20+
21+
// Prepare the command
22+
cmd := exec.Command("imagetool", imageName)
23+
24+
// Set the output to our file
25+
cmd.Stdout = outfile
26+
27+
cmd.Run()
28+
}

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
package main
2+
3+
import (
4+
"log"
5+
"net/http"
6+
"os/exec"
7+
"regexp"
8+
)
9+
10+
func handler(req *http.Request) {
11+
imageName := req.URL.Query()["imageName"][0]
12+
outputPath := "/tmp/output.svg"
13+
14+
// Validate the imageName with a regular expression
15+
validImageName := regexp.MustCompile(`^[a-zA-Z0-9_\-\.]+$`)
16+
if !validImageName.MatchString(imageName) {
17+
log.Fatal("Invalid image name")
18+
return
19+
}
20+
21+
cmd := exec.Command("sh", "-c", fmt.Sprintf("imagetool %s > %s", imageName, outputPath))
22+
cmd.Run()
23+
}

‎go/ql/test/query-tests/Security/CWE-078/CommandInjection.expected

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,16 @@
11
edges
22
| ArgumentInjection.go:9:10:9:16 | selection of URL | ArgumentInjection.go:9:10:9:24 | call to Query | provenance | MaD:735 |
33
| ArgumentInjection.go:9:10:9:24 | call to Query | ArgumentInjection.go:10:31:10:34 | path | provenance | |
4+
| CommandInjection2.go:13:15:13:21 | selection of URL | CommandInjection2.go:13:15:13:29 | call to Query | provenance | MaD:735 |
5+
| CommandInjection2.go:13:15:13:29 | call to Query | CommandInjection2.go:15:67:15:75 | imageName | provenance | |
6+
| CommandInjection2.go:15:34:15:88 | []type{args} [array] | CommandInjection2.go:15:34:15:88 | call to Sprintf | provenance | MaD:245 |
7+
| CommandInjection2.go:15:67:15:75 | imageName | CommandInjection2.go:15:34:15:88 | []type{args} [array] | provenance | |
8+
| CommandInjection2.go:15:67:15:75 | imageName | CommandInjection2.go:15:34:15:88 | call to Sprintf | provenance | FunctionModel |
9+
| CommandInjection2.go:41:18:41:24 | selection of URL | CommandInjection2.go:41:18:41:32 | call to Query | provenance | MaD:735 |
10+
| CommandInjection2.go:41:18:41:32 | call to Query | CommandInjection2.go:51:70:51:78 | imageName | provenance | |
11+
| CommandInjection2.go:51:37:51:91 | []type{args} [array] | CommandInjection2.go:51:37:51:91 | call to Sprintf | provenance | MaD:245 |
12+
| CommandInjection2.go:51:70:51:78 | imageName | CommandInjection2.go:51:37:51:91 | []type{args} [array] | provenance | |
13+
| CommandInjection2.go:51:70:51:78 | imageName | CommandInjection2.go:51:37:51:91 | call to Sprintf | provenance | FunctionModel |
414
| CommandInjection.go:9:13:9:19 | selection of URL | CommandInjection.go:9:13:9:27 | call to Query | provenance | MaD:735 |
515
| CommandInjection.go:9:13:9:27 | call to Query | CommandInjection.go:10:22:10:28 | cmdName | provenance | |
616
| 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
103113
| ArgumentInjection.go:9:10:9:16 | selection of URL | semmle.label | selection of URL |
104114
| ArgumentInjection.go:9:10:9:24 | call to Query | semmle.label | call to Query |
105115
| ArgumentInjection.go:10:31:10:34 | path | semmle.label | path |
116+
| CommandInjection2.go:13:15:13:21 | selection of URL | semmle.label | selection of URL |
117+
| CommandInjection2.go:13:15:13:29 | call to Query | semmle.label | call to Query |
118+
| CommandInjection2.go:15:34:15:88 | []type{args} [array] | semmle.label | []type{args} [array] |
119+
| CommandInjection2.go:15:34:15:88 | call to Sprintf | semmle.label | call to Sprintf |
120+
| CommandInjection2.go:15:67:15:75 | imageName | semmle.label | imageName |
121+
| CommandInjection2.go:41:18:41:24 | selection of URL | semmle.label | selection of URL |
122+
| CommandInjection2.go:41:18:41:32 | call to Query | semmle.label | call to Query |
123+
| CommandInjection2.go:51:37:51:91 | []type{args} [array] | semmle.label | []type{args} [array] |
124+
| CommandInjection2.go:51:37:51:91 | call to Sprintf | semmle.label | call to Sprintf |
125+
| CommandInjection2.go:51:70:51:78 | imageName | semmle.label | imageName |
106126
| CommandInjection.go:9:13:9:19 | selection of URL | semmle.label | selection of URL |
107127
| CommandInjection.go:9:13:9:27 | call to Query | semmle.label | call to Query |
108128
| CommandInjection.go:10:22:10:28 | cmdName | semmle.label | cmdName |
@@ -195,6 +215,8 @@ nodes
195215
subpaths
196216
#select
197217
| 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 |
218+
| 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 |
219+
| 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 |
198220
| 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 |
199221
| 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 |
200222
| 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

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
package main
2+
3+
import (
4+
"net/http"
5+
"os/exec"
6+
"log"
7+
"os"
8+
"regexp"
9+
"fmt"
10+
)
11+
12+
func handler(req *http.Request) {
13+
imageName := req.URL.Query()["imageName"][0]
14+
outputPath = "/tmp/output.svg"
15+
cmd := exec.Command("sh", "-c", fmt.Sprintf("imagetool %s > %s", imageName, outputPath)) // NOT OK - correctly flagged
16+
cmd.Run()
17+
// ...
18+
}
19+
20+
func handler2(req *http.Request) {
21+
imageName := req.URL.Query()["imageName"][0]
22+
outputPath := "/tmp/output.svg"
23+
24+
// Create the output file
25+
outfile, err := os.Create(outputPath)
26+
if err != nil {
27+
log.Fatal(err)
28+
}
29+
defer outfile.Close()
30+
31+
// Prepare the command
32+
cmd := exec.Command("imagetool", imageName) // OK - and not flagged
33+
34+
// Set the output to our file
35+
cmd.Stdout = outfile
36+
37+
cmd.Run()
38+
}
39+
40+
func handler3(req *http.Request) {
41+
imageName := req.URL.Query()["imageName"][0]
42+
outputPath := "/tmp/output.svg"
43+
44+
// Validate the imageName with a regular expression
45+
validImageName := regexp.MustCompile(`^[a-zA-Z0-9_\-\.]+$`)
46+
if !validImageName.MatchString(imageName) {
47+
log.Fatal("Invalid image name")
48+
return
49+
}
50+
51+
cmd := exec.Command("sh", "-c", fmt.Sprintf("imagetool %s > %s", imageName, outputPath)) // OK - but falsely flagged
52+
cmd.Run()
53+
}

0 commit comments

Comments
 (0)