Skip to content

Commit 1a99563

Browse files
asgerfasgerf
committed
JS: Restrict getInput to indirect command injection query
1 parent b9ad417 commit 1a99563

File tree

6 files changed

+43
-64
lines changed

6 files changed

+43
-64
lines changed

‎javascript/ql/lib/semmle/javascript/frameworks/ActionsLib.qll

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
*/
44

55
private import javascript
6+
private import semmle.javascript.security.dataflow.IndirectCommandInjectionCustomizations
67

78
private API::Node payload() {
89
result = API::moduleImport("@actions/github").getMember("context").getMember("payload")
@@ -51,11 +52,10 @@ private class GitHubActionsContextSource extends RemoteFlowSource {
5152
/**
5253
* A source of taint originating from user input.
5354
*
54-
* At the momemnt this is treated as a remote flow source, although it is not
55-
* always possible for an attacker to control this. In the future we might classify
56-
* this differently.
55+
* At the momemnt this is only treated as a taint source for the indirect-command injection
56+
* query.
5757
*/
58-
private class GitHubActionsInputSource extends RemoteFlowSource {
58+
private class GitHubActionsInputSource extends IndirectCommandInjection::Source {
5959
GitHubActionsInputSource() {
6060
this =
6161
API::moduleImport("@actions/core")
@@ -64,7 +64,7 @@ private class GitHubActionsInputSource extends RemoteFlowSource {
6464
.asSource()
6565
}
6666

67-
override string getSourceType() { result = "GitHub Actions user input" }
67+
override string describe() { result = "GitHub Actions user input" }
6868
}
6969

7070
private class ExecActionsCall extends SystemCommandExecution, DataFlow::CallNode {

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

Lines changed: 26 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,17 @@
11
nodes
2-
| actions.js:3:6:3:16 | process.env |
3-
| actions.js:3:6:3:16 | process.env |
4-
| actions.js:3:6:3:29 | process ... _DATA'] |
5-
| actions.js:3:6:3:29 | process ... _DATA'] |
6-
| actions.js:6:15:6:15 | e |
7-
| actions.js:7:10:7:10 | e |
8-
| actions.js:7:10:7:23 | e['TEST_DATA'] |
9-
| actions.js:7:10:7:23 | e['TEST_DATA'] |
10-
| actions.js:11:6:11:16 | process.env |
11-
| actions.js:11:6:11:16 | process.env |
2+
| actions.js:4:6:4:16 | process.env |
3+
| actions.js:4:6:4:16 | process.env |
4+
| actions.js:4:6:4:29 | process ... _DATA'] |
5+
| actions.js:4:6:4:29 | process ... _DATA'] |
6+
| actions.js:7:15:7:15 | e |
7+
| actions.js:8:10:8:10 | e |
8+
| actions.js:8:10:8:23 | e['TEST_DATA'] |
9+
| actions.js:8:10:8:23 | e['TEST_DATA'] |
10+
| actions.js:12:6:12:16 | process.env |
11+
| actions.js:12:6:12:16 | process.env |
12+
| actions.js:14:6:14:21 | getInput('data') |
13+
| actions.js:14:6:14:21 | getInput('data') |
14+
| actions.js:14:6:14:21 | getInput('data') |
1215
| command-line-parameter-command-injection.js:4:10:4:21 | process.argv |
1316
| command-line-parameter-command-injection.js:4:10:4:21 | process.argv |
1417
| command-line-parameter-command-injection.js:4:10:4:21 | process.argv |
@@ -222,15 +225,16 @@ nodes
222225
| command-line-parameter-command-injection.js:146:22:146:38 | program.pizzaType |
223226
| command-line-parameter-command-injection.js:146:22:146:38 | program.pizzaType |
224227
edges
225-
| actions.js:3:6:3:16 | process.env | actions.js:3:6:3:29 | process ... _DATA'] |
226-
| actions.js:3:6:3:16 | process.env | actions.js:3:6:3:29 | process ... _DATA'] |
227-
| actions.js:3:6:3:16 | process.env | actions.js:3:6:3:29 | process ... _DATA'] |
228-
| actions.js:3:6:3:16 | process.env | actions.js:3:6:3:29 | process ... _DATA'] |
229-
| actions.js:6:15:6:15 | e | actions.js:7:10:7:10 | e |
230-
| actions.js:7:10:7:10 | e | actions.js:7:10:7:23 | e['TEST_DATA'] |
231-
| actions.js:7:10:7:10 | e | actions.js:7:10:7:23 | e['TEST_DATA'] |
232-
| actions.js:11:6:11:16 | process.env | actions.js:6:15:6:15 | e |
233-
| actions.js:11:6:11:16 | process.env | actions.js:6:15:6:15 | e |
228+
| actions.js:4:6:4:16 | process.env | actions.js:4:6:4:29 | process ... _DATA'] |
229+
| actions.js:4:6:4:16 | process.env | actions.js:4:6:4:29 | process ... _DATA'] |
230+
| actions.js:4:6:4:16 | process.env | actions.js:4:6:4:29 | process ... _DATA'] |
231+
| actions.js:4:6:4:16 | process.env | actions.js:4:6:4:29 | process ... _DATA'] |
232+
| actions.js:7:15:7:15 | e | actions.js:8:10:8:10 | e |
233+
| actions.js:8:10:8:10 | e | actions.js:8:10:8:23 | e['TEST_DATA'] |
234+
| actions.js:8:10:8:10 | e | actions.js:8:10:8:23 | e['TEST_DATA'] |
235+
| actions.js:12:6:12:16 | process.env | actions.js:7:15:7:15 | e |
236+
| actions.js:12:6:12:16 | process.env | actions.js:7:15:7:15 | e |
237+
| actions.js:14:6:14:21 | getInput('data') | actions.js:14:6:14:21 | getInput('data') |
234238
| command-line-parameter-command-injection.js:4:10:4:21 | process.argv | command-line-parameter-command-injection.js:4:10:4:21 | process.argv |
235239
| command-line-parameter-command-injection.js:8:22:8:33 | process.argv | command-line-parameter-command-injection.js:8:22:8:36 | process.argv[2] |
236240
| command-line-parameter-command-injection.js:8:22:8:33 | process.argv | command-line-parameter-command-injection.js:8:22:8:36 | process.argv[2] |
@@ -419,8 +423,9 @@ edges
419423
| command-line-parameter-command-injection.js:146:22:146:38 | program.pizzaType | command-line-parameter-command-injection.js:146:10:146:38 | "cmd.sh ... zzaType |
420424
| command-line-parameter-command-injection.js:146:22:146:38 | program.pizzaType | command-line-parameter-command-injection.js:146:10:146:38 | "cmd.sh ... zzaType |
421425
#select
422-
| actions.js:3:6:3:29 | process ... _DATA'] | actions.js:3:6:3:16 | process.env | actions.js:3:6:3:29 | process ... _DATA'] | This command depends on an unsanitized $@. | actions.js:3:6:3:16 | process.env | environment variable |
423-
| actions.js:7:10:7:23 | e['TEST_DATA'] | actions.js:11:6:11:16 | process.env | actions.js:7:10:7:23 | e['TEST_DATA'] | This command depends on an unsanitized $@. | actions.js:11:6:11:16 | process.env | environment variable |
426+
| actions.js:4:6:4:29 | process ... _DATA'] | actions.js:4:6:4:16 | process.env | actions.js:4:6:4:29 | process ... _DATA'] | This command depends on an unsanitized $@. | actions.js:4:6:4:16 | process.env | environment variable |
427+
| actions.js:8:10:8:23 | e['TEST_DATA'] | actions.js:12:6:12:16 | process.env | actions.js:8:10:8:23 | e['TEST_DATA'] | This command depends on an unsanitized $@. | actions.js:12:6:12:16 | process.env | environment variable |
428+
| actions.js:14:6:14:21 | getInput('data') | actions.js:14:6:14:21 | getInput('data') | actions.js:14:6:14:21 | getInput('data') | This command depends on an unsanitized $@. | actions.js:14:6:14:21 | getInput('data') | GitHub Actions user input |
424429
| command-line-parameter-command-injection.js:4:10:4:21 | process.argv | command-line-parameter-command-injection.js:4:10:4:21 | process.argv | command-line-parameter-command-injection.js:4:10:4:21 | process.argv | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:4:10:4:21 | process.argv | command-line argument |
425430
| command-line-parameter-command-injection.js:8:10:8:36 | "cmd.sh ... argv[2] | command-line-parameter-command-injection.js:8:22:8:33 | process.argv | command-line-parameter-command-injection.js:8:10:8:36 | "cmd.sh ... argv[2] | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:8:22:8:33 | process.argv | command-line argument |
426431
| command-line-parameter-command-injection.js:11:14:11:20 | args[0] | command-line-parameter-command-injection.js:10:13:10:24 | process.argv | command-line-parameter-command-injection.js:11:14:11:20 | args[0] | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:10:13:10:24 | process.argv | command-line argument |

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { exec } from "@actions/exec";
2+
import { getInput } from "@actions/core";
23

34
exec(process.env['TEST_DATA']); // NOT OK
45
exec(process.env['GITHUB_ACTION']); // OK
@@ -9,3 +10,5 @@ function test(e) {
910
}
1011

1112
test(process.env);
13+
14+
exec(getInput('data')); // NOT OK

‎javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -13,16 +13,9 @@ nodes
1313
| NoSQLCodeInjection.js:22:36:22:43 | req.body |
1414
| NoSQLCodeInjection.js:22:36:22:43 | req.body |
1515
| NoSQLCodeInjection.js:22:36:22:48 | req.body.name |
16-
| actions.js:5:10:5:50 | github. ... message |
17-
| actions.js:5:10:5:50 | github. ... message |
18-
| actions.js:5:10:5:50 | github. ... message |
19-
| actions.js:6:10:6:33 | core.ge ... mbers') |
20-
| actions.js:6:10:6:33 | core.ge ... mbers') |
21-
| actions.js:6:10:6:33 | core.ge ... mbers') |
22-
| actions.js:7:10:7:42 | core.ge ... mbers') |
23-
| actions.js:7:10:7:42 | core.ge ... mbers') |
24-
| actions.js:7:10:7:53 | core.ge ... n('\\n') |
25-
| actions.js:7:10:7:53 | core.ge ... n('\\n') |
16+
| actions.js:4:10:4:50 | github. ... message |
17+
| actions.js:4:10:4:50 | github. ... message |
18+
| actions.js:4:10:4:50 | github. ... message |
2619
| angularjs.js:10:22:10:36 | location.search |
2720
| angularjs.js:10:22:10:36 | location.search |
2821
| angularjs.js:10:22:10:36 | location.search |
@@ -201,12 +194,7 @@ edges
201194
| NoSQLCodeInjection.js:22:36:22:43 | req.body | NoSQLCodeInjection.js:22:36:22:48 | req.body.name |
202195
| NoSQLCodeInjection.js:22:36:22:48 | req.body.name | NoSQLCodeInjection.js:22:24:22:48 | "name = ... dy.name |
203196
| NoSQLCodeInjection.js:22:36:22:48 | req.body.name | NoSQLCodeInjection.js:22:24:22:48 | "name = ... dy.name |
204-
| actions.js:5:10:5:50 | github. ... message | actions.js:5:10:5:50 | github. ... message |
205-
| actions.js:6:10:6:33 | core.ge ... mbers') | actions.js:6:10:6:33 | core.ge ... mbers') |
206-
| actions.js:7:10:7:42 | core.ge ... mbers') | actions.js:7:10:7:53 | core.ge ... n('\\n') |
207-
| actions.js:7:10:7:42 | core.ge ... mbers') | actions.js:7:10:7:53 | core.ge ... n('\\n') |
208-
| actions.js:7:10:7:42 | core.ge ... mbers') | actions.js:7:10:7:53 | core.ge ... n('\\n') |
209-
| actions.js:7:10:7:42 | core.ge ... mbers') | actions.js:7:10:7:53 | core.ge ... n('\\n') |
197+
| actions.js:4:10:4:50 | github. ... message | actions.js:4:10:4:50 | github. ... message |
210198
| angularjs.js:10:22:10:36 | location.search | angularjs.js:10:22:10:36 | location.search |
211199
| angularjs.js:13:23:13:37 | location.search | angularjs.js:13:23:13:37 | location.search |
212200
| angularjs.js:16:28:16:42 | location.search | angularjs.js:16:28:16:42 | location.search |
@@ -322,9 +310,7 @@ edges
322310
| NoSQLCodeInjection.js:18:24:18:37 | req.body.query | NoSQLCodeInjection.js:18:24:18:31 | req.body | NoSQLCodeInjection.js:18:24:18:37 | req.body.query | This code execution depends on a $@. | NoSQLCodeInjection.js:18:24:18:31 | req.body | user-provided value |
323311
| NoSQLCodeInjection.js:19:24:19:48 | "name = ... dy.name | NoSQLCodeInjection.js:19:36:19:43 | req.body | NoSQLCodeInjection.js:19:24:19:48 | "name = ... dy.name | This code execution depends on a $@. | NoSQLCodeInjection.js:19:36:19:43 | req.body | user-provided value |
324312
| NoSQLCodeInjection.js:22:24:22:48 | "name = ... dy.name | NoSQLCodeInjection.js:22:36:22:43 | req.body | NoSQLCodeInjection.js:22:24:22:48 | "name = ... dy.name | This code execution depends on a $@. | NoSQLCodeInjection.js:22:36:22:43 | req.body | user-provided value |
325-
| actions.js:5:10:5:50 | github. ... message | actions.js:5:10:5:50 | github. ... message | actions.js:5:10:5:50 | github. ... message | This code execution depends on a $@. | actions.js:5:10:5:50 | github. ... message | user-provided value |
326-
| actions.js:6:10:6:33 | core.ge ... mbers') | actions.js:6:10:6:33 | core.ge ... mbers') | actions.js:6:10:6:33 | core.ge ... mbers') | This code execution depends on a $@. | actions.js:6:10:6:33 | core.ge ... mbers') | user-provided value |
327-
| actions.js:7:10:7:53 | core.ge ... n('\\n') | actions.js:7:10:7:42 | core.ge ... mbers') | actions.js:7:10:7:53 | core.ge ... n('\\n') | This code execution depends on a $@. | actions.js:7:10:7:42 | core.ge ... mbers') | user-provided value |
313+
| actions.js:4:10:4:50 | github. ... message | actions.js:4:10:4:50 | github. ... message | actions.js:4:10:4:50 | github. ... message | This code execution depends on a $@. | actions.js:4:10:4:50 | github. ... message | user-provided value |
328314
| angularjs.js:10:22:10:36 | location.search | angularjs.js:10:22:10:36 | location.search | angularjs.js:10:22:10:36 | location.search | This code execution depends on a $@. | angularjs.js:10:22:10:36 | location.search | user-provided value |
329315
| angularjs.js:13:23:13:37 | location.search | angularjs.js:13:23:13:37 | location.search | angularjs.js:13:23:13:37 | location.search | This code execution depends on a $@. | angularjs.js:13:23:13:37 | location.search | user-provided value |
330316
| angularjs.js:16:28:16:42 | location.search | angularjs.js:16:28:16:42 | location.search | angularjs.js:16:28:16:42 | location.search | This code execution depends on a $@. | angularjs.js:16:28:16:42 | location.search | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected

Lines changed: 4 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -13,16 +13,9 @@ nodes
1313
| NoSQLCodeInjection.js:22:36:22:43 | req.body |
1414
| NoSQLCodeInjection.js:22:36:22:43 | req.body |
1515
| NoSQLCodeInjection.js:22:36:22:48 | req.body.name |
16-
| actions.js:5:10:5:50 | github. ... message |
17-
| actions.js:5:10:5:50 | github. ... message |
18-
| actions.js:5:10:5:50 | github. ... message |
19-
| actions.js:6:10:6:33 | core.ge ... mbers') |
20-
| actions.js:6:10:6:33 | core.ge ... mbers') |
21-
| actions.js:6:10:6:33 | core.ge ... mbers') |
22-
| actions.js:7:10:7:42 | core.ge ... mbers') |
23-
| actions.js:7:10:7:42 | core.ge ... mbers') |
24-
| actions.js:7:10:7:53 | core.ge ... n('\\n') |
25-
| actions.js:7:10:7:53 | core.ge ... n('\\n') |
16+
| actions.js:4:10:4:50 | github. ... message |
17+
| actions.js:4:10:4:50 | github. ... message |
18+
| actions.js:4:10:4:50 | github. ... message |
2619
| angularjs.js:10:22:10:36 | location.search |
2720
| angularjs.js:10:22:10:36 | location.search |
2821
| angularjs.js:10:22:10:36 | location.search |
@@ -205,12 +198,7 @@ edges
205198
| NoSQLCodeInjection.js:22:36:22:43 | req.body | NoSQLCodeInjection.js:22:36:22:48 | req.body.name |
206199
| NoSQLCodeInjection.js:22:36:22:48 | req.body.name | NoSQLCodeInjection.js:22:24:22:48 | "name = ... dy.name |
207200
| NoSQLCodeInjection.js:22:36:22:48 | req.body.name | NoSQLCodeInjection.js:22:24:22:48 | "name = ... dy.name |
208-
| actions.js:5:10:5:50 | github. ... message | actions.js:5:10:5:50 | github. ... message |
209-
| actions.js:6:10:6:33 | core.ge ... mbers') | actions.js:6:10:6:33 | core.ge ... mbers') |
210-
| actions.js:7:10:7:42 | core.ge ... mbers') | actions.js:7:10:7:53 | core.ge ... n('\\n') |
211-
| actions.js:7:10:7:42 | core.ge ... mbers') | actions.js:7:10:7:53 | core.ge ... n('\\n') |
212-
| actions.js:7:10:7:42 | core.ge ... mbers') | actions.js:7:10:7:53 | core.ge ... n('\\n') |
213-
| actions.js:7:10:7:42 | core.ge ... mbers') | actions.js:7:10:7:53 | core.ge ... n('\\n') |
201+
| actions.js:4:10:4:50 | github. ... message | actions.js:4:10:4:50 | github. ... message |
214202
| angularjs.js:10:22:10:36 | location.search | angularjs.js:10:22:10:36 | location.search |
215203
| angularjs.js:13:23:13:37 | location.search | angularjs.js:13:23:13:37 | location.search |
216204
| angularjs.js:16:28:16:42 | location.search | angularjs.js:16:28:16:42 | location.search |

‎javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/actions.js

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,5 @@
1-
const core = require('@actions/core');
21
const github = require('@actions/github');
32

43
function test() {
54
eval(github.context.payload.commits[1].message); // NOT OK
6-
eval(core.getInput('numbers')); // NOT OK
7-
eval(core.getMultilineInput('numbers').join('\n')); // NOT OK
85
}

0 commit comments

Comments
 (0)