Skip to content

Commit e8c8360

Browse files
author
Max Schaefer
authored
Merge pull request #659 from esben-semmle/js/more-constant-string-usage
JS: replace StringLiteral with ConstantString in two queries
2 parents 54bb9d1 + 376ed7a commit e8c8360

File tree

6 files changed

+65
-15
lines changed

6 files changed

+65
-15
lines changed

‎javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ string metachar() {
2525
string getAMatchedString(Expr e) {
2626
result = getAMatchedConstant(e.(RegExpLiteral).getRoot()).getValue()
2727
or
28-
result = e.(StringLiteral).getValue()
28+
result = e.getStringValue()
2929
}
3030

3131
/** Gets a constant matched by `t`. */

‎javascript/ql/src/semmle/javascript/security/dataflow/CommandInjection.qll‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ module CommandInjection {
7777
override predicate isSource(DataFlow::Node nd) {
7878
nd instanceof DataFlow::ArrayCreationNode
7979
or
80-
exists (StringLiteral shell | shellCmd(shell, _) |
80+
exists (ConstantString shell | shellCmd(shell, _) |
8181
nd = DataFlow::valueNode(shell)
8282
)
8383
}
@@ -96,14 +96,14 @@ module CommandInjection {
9696
* That is, either `shell` is a Unix shell (`sh` or similar) and
9797
* `arg` is `"-c"`, or `shell` is `cmd.exe` and `arg` is `"/c"`.
9898
*/
99-
private predicate shellCmd(StringLiteral shell, string arg) {
100-
exists (string s | s = shell.getValue() |
99+
private predicate shellCmd(ConstantString shell, string arg) {
100+
exists (string s | s = shell.getStringValue() |
101101
(s = "sh" or s = "bash" or s = "/bin/sh" or s = "/bin/bash")
102102
and
103103
arg = "-c"
104104
)
105105
or
106-
exists (string s | s = shell.getValue().toLowerCase() |
106+
exists (string s | s = shell.getStringValue().toLowerCase() |
107107
(s = "cmd" or s = "cmd.exe")
108108
and
109109
(arg = "/c" or arg = "/C")
@@ -126,7 +126,7 @@ module CommandInjection {
126126
*/
127127
private predicate indirectCommandInjection(DataFlow::Node sink, SystemCommandExecution sys) {
128128
exists (ArgumentListTracking cfg, DataFlow::ArrayCreationNode args,
129-
StringLiteral shell, string dashC |
129+
ConstantString shell, string dashC |
130130
shellCmd(shell, dashC) and
131131
cfg.hasFlow(DataFlow::valueNode(shell), sys.getACommandArgument()) and
132132
cfg.hasFlow(args, sys.getArgumentList()) and

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

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,15 @@ nodes
2929
| child_process-test.js:44:30:44:33 | args |
3030
| child_process-test.js:46:9:46:12 | "sh" |
3131
| child_process-test.js:46:15:46:18 | args |
32-
| child_process-test.js:49:14:49:16 | cmd |
33-
| child_process-test.js:49:19:49:22 | args |
34-
| child_process-test.js:50:12:50:14 | cmd |
35-
| child_process-test.js:50:17:50:20 | args |
32+
| child_process-test.js:48:9:48:17 | args |
33+
| child_process-test.js:48:16:48:17 | [] |
34+
| child_process-test.js:50:15:50:17 | cmd |
35+
| child_process-test.js:51:17:51:32 | `/bin` + "/bash" |
36+
| child_process-test.js:51:35:51:38 | args |
37+
| child_process-test.js:55:14:55:16 | cmd |
38+
| child_process-test.js:55:19:55:22 | args |
39+
| child_process-test.js:56:12:56:14 | cmd |
40+
| child_process-test.js:56:17:56:20 | args |
3641
edges
3742
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:17:13:17:15 | cmd |
3843
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:18:17:18:19 | cmd |
@@ -44,6 +49,7 @@ edges
4449
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:25:21:25:23 | cmd |
4550
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:39:26:39:28 | cmd |
4651
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:43:15:43:17 | cmd |
52+
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:50:15:50:17 | cmd |
4753
| child_process-test.js:6:15:6:38 | url.par ... , true) | child_process-test.js:6:15:6:44 | url.par ... ).query |
4854
| child_process-test.js:6:15:6:44 | url.par ... ).query | child_process-test.js:6:15:6:49 | url.par ... ry.path |
4955
| child_process-test.js:6:15:6:49 | url.par ... ry.path | child_process-test.js:6:9:6:49 | cmd |
@@ -58,10 +64,12 @@ edges
5864
| child_process-test.js:41:9:41:17 | args | child_process-test.js:44:30:44:33 | args |
5965
| child_process-test.js:41:9:41:17 | args | child_process-test.js:46:15:46:18 | args |
6066
| child_process-test.js:41:16:41:17 | [] | child_process-test.js:41:9:41:17 | args |
61-
| child_process-test.js:46:9:46:12 | "sh" | child_process-test.js:49:14:49:16 | cmd |
62-
| child_process-test.js:46:15:46:18 | args | child_process-test.js:49:19:49:22 | args |
63-
| child_process-test.js:49:14:49:16 | cmd | child_process-test.js:50:12:50:14 | cmd |
64-
| child_process-test.js:49:19:49:22 | args | child_process-test.js:50:17:50:20 | args |
67+
| child_process-test.js:46:9:46:12 | "sh" | child_process-test.js:55:14:55:16 | cmd |
68+
| child_process-test.js:46:15:46:18 | args | child_process-test.js:55:19:55:22 | args |
69+
| child_process-test.js:48:9:48:17 | args | child_process-test.js:51:35:51:38 | args |
70+
| child_process-test.js:48:16:48:17 | [] | child_process-test.js:48:9:48:17 | args |
71+
| child_process-test.js:55:14:55:16 | cmd | child_process-test.js:56:12:56:14 | cmd |
72+
| child_process-test.js:55:19:55:22 | args | child_process-test.js:56:17:56:20 | args |
6573
#select
6674
| child_process-test.js:17:13:17:15 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:17:13:17:15 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
6775
| child_process-test.js:18:17:18:19 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:18:17:18:19 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
@@ -73,4 +81,5 @@ edges
7381
| child_process-test.js:25:13:25:31 | "foo" + cmd + "bar" | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:25:13:25:31 | "foo" + cmd + "bar" | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
7482
| child_process-test.js:39:5:39:31 | cp.spaw ... cmd ]) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:39:26:39:28 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
7583
| child_process-test.js:44:5:44:34 | cp.exec ... , args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:43:15:43:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
76-
| child_process-test.js:50:3:50:21 | cp.spawn(cmd, args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:43:15:43:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
84+
| child_process-test.js:51:5:51:39 | cp.exec ... , args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:50:15:50:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
85+
| child_process-test.js:56:3:56:21 | cp.spawn(cmd, args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:43:15:43:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-078/child_process-test.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,12 @@ var server = http.createServer(function(req, res) {
4444
cp.execFile("/bin/bash", args); // NOT OK
4545

4646
run("sh", args);
47+
48+
let args = [];
49+
args[0] = `-` + "c";
50+
args[1] = cmd;
51+
cp.execFile(`/bin` + "/bash", args); // NOT OK
52+
4753
});
4854

4955
function run(cmd, args) {

‎javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,3 +9,9 @@
99
| tst.js:37:20:37:23 | /"/g | This does not backslash-escape the backslash character. |
1010
| tst.js:41:20:41:22 | "/" | This replaces only the first occurrence of "/". |
1111
| tst.js:45:20:45:24 | "%25" | This replaces only the first occurrence of "%25". |
12+
| tst.js:49:20:49:22 | `'` | This replaces only the first occurrence of `'`. |
13+
| tst.js:53:20:53:22 | "'" | This replaces only the first occurrence of "'". |
14+
| tst.js:57:20:57:22 | `'` | This replaces only the first occurrence of `'`. |
15+
| tst.js:61:20:61:27 | "'" + "" | This replaces only the first occurrence of "'" + "". |
16+
| tst.js:65:20:65:22 | "'" | This replaces only the first occurrence of "'". |
17+
| tst.js:69:20:69:27 | "'" + "" | This replaces only the first occurrence of "'" + "". |

‎javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,29 @@ function bad11(s) {
4545
return s.replace("%25", "%"); // NOT OK
4646
}
4747

48+
function bad12(s) {
49+
return s.replace(`'`, ""); // NOT OK
50+
}
51+
52+
function bad13(s) {
53+
return s.replace("'", ``); // NOT OK
54+
}
55+
56+
function bad14(s) {
57+
return s.replace(`'`, ``); // NOT OK
58+
}
59+
60+
function bad15(s) {
61+
return s.replace("'" + "", ""); // NOT OK
62+
}
63+
64+
function bad16(s) {
65+
return s.replace("'", "" + ""); // NOT OK
66+
}
67+
68+
function bad17(s) {
69+
return s.replace("'" + "", "" + ""); // NOT OK
70+
}
4871

4972
function good1(s) {
5073
while (s.indexOf("'") > 0)
@@ -120,6 +143,12 @@ app.get('/some/path', function(req, res) {
120143
bad9(untrusted);
121144
bad10(untrusted);
122145
bad11(untrusted);
146+
bad12(untrusted);
147+
bad13(untrusted);
148+
bad14(untrusted);
149+
bad15(untrusted);
150+
bad16(untrusted);
151+
bad17(untrusted);
123152

124153
good1(untrusted);
125154
good2(untrusted);

0 commit comments

Comments
 (0)