Skip to content

Commit 3448751

Browse files
RasmusWLRasmusWL
committed
JS: Consolidate command-line argument modeling
Such that we can reuse the existing modeling, but have it globally applied as a threat-model as well. I Basically just moved the modeling. One important aspect is that this changes is that the previously query-specific `argsParseStep` is now a globally applied taint-step. This seems reasonable, if someone applied the argument parsing to any user-controlled string, it seems correct to propagate that taint for _any_ query.
1 parent 412e841 commit 3448751

File tree

5 files changed

+186
-122
lines changed

5 files changed

+186
-122
lines changed

‎javascript/ql/lib/javascript.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ import semmle.javascript.frameworks.Classnames
8181
import semmle.javascript.frameworks.ClassValidator
8282
import semmle.javascript.frameworks.ClientRequests
8383
import semmle.javascript.frameworks.ClosureLibrary
84+
import semmle.javascript.frameworks.CommandLineArguments
8485
import semmle.javascript.frameworks.CookieLibraries
8586
import semmle.javascript.frameworks.Credentials
8687
import semmle.javascript.frameworks.CryptoLibraries

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

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
/** Provides modeling for parsed command line arguments. */
2+
3+
import javascript
4+
5+
/**
6+
* An object containing command-line arguments, potentially parsed by a library.
7+
*
8+
* Extend this class to refine existing API models. If you want to model new APIs,
9+
* extend `CommandLineArguments::Range` instead.
10+
*/
11+
class CommandLineArguments extends ThreatModelSource instanceof CommandLineArguments::Range { }
12+
13+
/** Provides a class for modeling new sources of remote user input. */
14+
module CommandLineArguments {
15+
/**
16+
* An object containing command-line arguments, potentially parsed by a library.
17+
*
18+
* Extend this class to model new APIs. If you want to refine existing API models,
19+
* extend `CommandLineArguments` instead.
20+
*/
21+
abstract class Range extends ThreatModelSource::Range {
22+
override string getThreatModel() { result = "commandargs" }
23+
24+
override string getSourceType() { result = "CommandLineArguments" }
25+
}
26+
}
27+
28+
/** A read of `process.argv`, considered as a threat-model source. */
29+
private class ProcessArgv extends CommandLineArguments::Range {
30+
ProcessArgv() {
31+
// `process.argv[0]` and `process.argv[1]` are paths to `node` and `main`, and
32+
// therefore should not be considered a threat-source... However, we don't have an
33+
// easy way to exclude them, so we need to allow them.
34+
this = NodeJSLib::process().getAPropertyRead("argv")
35+
}
36+
37+
override string getSourceType() { result = "process.argv" }
38+
}
39+
40+
private class DefaultModels extends CommandLineArguments::Range {
41+
DefaultModels() {
42+
// `require('get-them-args')(...)` => `{ unknown: [], a: ... b: ... }`
43+
this = DataFlow::moduleImport("get-them-args").getACall()
44+
or
45+
// `require('optimist').argv` => `{ _: [], a: ... b: ... }`
46+
this = DataFlow::moduleMember("optimist", "argv")
47+
or
48+
// `require("arg")({...spec})` => `{_: [], a: ..., b: ...}`
49+
this = DataFlow::moduleImport("arg").getACall()
50+
or
51+
// `(new (require(argparse)).ArgumentParser({...spec})).parse_args()` => `{a: ..., b: ...}`
52+
this =
53+
API::moduleImport("argparse")
54+
.getMember("ArgumentParser")
55+
.getInstance()
56+
.getMember("parse_args")
57+
.getACall()
58+
or
59+
// `require('command-line-args')({...spec})` => `{a: ..., b: ...}`
60+
this = DataFlow::moduleImport("command-line-args").getACall()
61+
or
62+
// `require('meow')(help, {...spec})` => `{a: ..., b: ....}`
63+
this = DataFlow::moduleImport("meow").getACall()
64+
or
65+
// `require("dashdash").createParser(...spec)` => `{a: ..., b: ...}`
66+
this =
67+
[
68+
API::moduleImport("dashdash"),
69+
API::moduleImport("dashdash").getMember("createParser").getReturn()
70+
].getMember("parse").getACall()
71+
or
72+
// `require('commander').myCmdArgumentName`
73+
this = commander().getAMember().asSource()
74+
or
75+
// `require('commander').opt()` => `{a: ..., b: ...}`
76+
this = commander().getMember("opts").getACall()
77+
}
78+
}
79+
80+
/**
81+
* A step for propagating taint through command line parsing,
82+
* such as `var succ = require("minimist")(pred)`.
83+
*/
84+
private class ArgsParseStep extends TaintTracking::SharedTaintStep {
85+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
86+
exists(DataFlow::CallNode call |
87+
call = DataFlow::moduleMember("args", "parse").getACall() or
88+
call = DataFlow::moduleImport(["yargs-parser", "minimist", "subarg"]).getACall()
89+
|
90+
succ = call and
91+
pred = call.getArgument(0)
92+
)
93+
}
94+
}
95+
96+
/**
97+
* Gets a Command instance from the `commander` library.
98+
*/
99+
private API::Node commander() {
100+
result = API::moduleImport("commander")
101+
or
102+
// `require("commander").program === require("commander")`
103+
result = commander().getMember("program")
104+
or
105+
result = commander().getMember("Command").getInstance()
106+
or
107+
// lots of chainable methods
108+
result = commander().getAMember().getReturn()
109+
}
110+
111+
/**
112+
* Gets an instance of `yargs`.
113+
* Either directly imported as a module, or through some chained method call.
114+
*/
115+
private DataFlow::SourceNode yargs() {
116+
result = DataFlow::moduleImport("yargs")
117+
or
118+
// script used to generate list of chained methods: https://gist.github.com/erik-krogh/f8afe952c0577f4b563a993e613269ba
119+
exists(string method |
120+
not method =
121+
// the methods that does not return a chained `yargs` object.
122+
[
123+
"getContext", "getDemandedOptions", "getDemandedCommands", "getDeprecatedOptions",
124+
"_getParseContext", "getOptions", "getGroups", "getStrict", "getStrictCommands",
125+
"getExitProcess", "locale", "getUsageInstance", "getCommandInstance"
126+
]
127+
|
128+
result = yargs().getAMethodCall(method)
129+
)
130+
}
131+
132+
/**
133+
* An array of command line arguments (`argv`) parsed by the `yargs` library.
134+
*/
135+
private class YargsArgv extends CommandLineArguments::Range {
136+
YargsArgv() {
137+
this = yargs().getAPropertyRead("argv")
138+
or
139+
this = yargs().getAMethodCall("parse") and
140+
this.(DataFlow::MethodCallNode).getNumArgument() = 0
141+
}
142+
}

‎javascript/ql/lib/semmle/javascript/security/dataflow/IndirectCommandInjectionCustomizations.qll

Lines changed: 3 additions & 118 deletions
Original file line numberDiff line numberDiff line change
@@ -25,21 +25,6 @@ module IndirectCommandInjection {
2525
*/
2626
abstract class Sanitizer extends DataFlow::Node { }
2727

28-
/**
29-
* A source of user input from the command-line, considered as a flow source for command injection.
30-
*/
31-
private class CommandLineArgumentsArrayAsSource extends Source instanceof CommandLineArgumentsArray
32-
{ }
33-
34-
/**
35-
* An array of command-line arguments.
36-
*/
37-
class CommandLineArgumentsArray extends DataFlow::SourceNode {
38-
CommandLineArgumentsArray() {
39-
this = DataFlow::globalVarRef("process").getAPropertyRead("argv")
40-
}
41-
}
42-
4328
/**
4429
* A read of `process.env`, considered as a flow source for command injection.
4530
*/
@@ -82,109 +67,9 @@ module IndirectCommandInjection {
8267
}
8368

8469
/**
85-
* An object containing parsed command-line arguments, considered as a flow source for command injection.
70+
* An object containing command-line arguments, considered as a flow source for command injection.
8671
*/
87-
class ParsedCommandLineArgumentsAsSource extends Source {
88-
ParsedCommandLineArgumentsAsSource() {
89-
// `require('get-them-args')(...)` => `{ unknown: [], a: ... b: ... }`
90-
this = DataFlow::moduleImport("get-them-args").getACall()
91-
or
92-
// `require('optimist').argv` => `{ _: [], a: ... b: ... }`
93-
this = DataFlow::moduleMember("optimist", "argv")
94-
or
95-
// `require("arg")({...spec})` => `{_: [], a: ..., b: ...}`
96-
this = DataFlow::moduleImport("arg").getACall()
97-
or
98-
// `(new (require(argparse)).ArgumentParser({...spec})).parse_args()` => `{a: ..., b: ...}`
99-
this =
100-
API::moduleImport("argparse")
101-
.getMember("ArgumentParser")
102-
.getInstance()
103-
.getMember("parse_args")
104-
.getACall()
105-
or
106-
// `require('command-line-args')({...spec})` => `{a: ..., b: ...}`
107-
this = DataFlow::moduleImport("command-line-args").getACall()
108-
or
109-
// `require('meow')(help, {...spec})` => `{a: ..., b: ....}`
110-
this = DataFlow::moduleImport("meow").getACall()
111-
or
112-
// `require("dashdash").createParser(...spec)` => `{a: ..., b: ...}`
113-
this =
114-
[
115-
API::moduleImport("dashdash"),
116-
API::moduleImport("dashdash").getMember("createParser").getReturn()
117-
].getMember("parse").getACall()
118-
or
119-
// `require('commander').myCmdArgumentName`
120-
this = commander().getAMember().asSource()
121-
or
122-
// `require('commander').opt()` => `{a: ..., b: ...}`
123-
this = commander().getMember("opts").getACall()
124-
}
125-
}
126-
127-
/**
128-
* Holds if there is a command line parsing step from `pred` to `succ`.
129-
* E.g: `var succ = require("minimist")(pred)`.
130-
*/
131-
predicate argsParseStep(DataFlow::Node pred, DataFlow::Node succ) {
132-
exists(DataFlow::CallNode call |
133-
call = DataFlow::moduleMember("args", "parse").getACall() or
134-
call = DataFlow::moduleImport(["yargs-parser", "minimist", "subarg"]).getACall()
135-
|
136-
succ = call and
137-
pred = call.getArgument(0)
138-
)
139-
}
140-
141-
/**
142-
* Gets a Command instance from the `commander` library.
143-
*/
144-
private API::Node commander() {
145-
result = API::moduleImport("commander")
146-
or
147-
// `require("commander").program === require("commander")`
148-
result = commander().getMember("program")
149-
or
150-
result = commander().getMember("Command").getInstance()
151-
or
152-
// lots of chainable methods
153-
result = commander().getAMember().getReturn()
154-
}
155-
156-
/**
157-
* Gets an instance of `yargs`.
158-
* Either directly imported as a module, or through some chained method call.
159-
*/
160-
private DataFlow::SourceNode yargs() {
161-
result = DataFlow::moduleImport("yargs")
162-
or
163-
// script used to generate list of chained methods: https://gist.github.com/erik-krogh/f8afe952c0577f4b563a993e613269ba
164-
exists(string method |
165-
not method =
166-
// the methods that does not return a chained `yargs` object.
167-
[
168-
"getContext", "getDemandedOptions", "getDemandedCommands", "getDeprecatedOptions",
169-
"_getParseContext", "getOptions", "getGroups", "getStrict", "getStrictCommands",
170-
"getExitProcess", "locale", "getUsageInstance", "getCommandInstance"
171-
]
172-
|
173-
result = yargs().getAMethodCall(method)
174-
)
175-
}
176-
177-
/**
178-
* An array of command line arguments (`argv`) parsed by the `yargs` library.
179-
*/
180-
class YargsArgv extends Source {
181-
YargsArgv() {
182-
this = yargs().getAPropertyRead("argv")
183-
or
184-
this = yargs().getAMethodCall("parse") and
185-
this.(DataFlow::MethodCallNode).getNumArgument() = 0
186-
}
187-
}
72+
private class CommandLineArgumentsAsSource extends Source instanceof CommandLineArguments { }
18873

18974
/**
19075
* A command-line argument that effectively is system-controlled, and therefore not likely to be exploitable when used in the execution of another command.
@@ -193,7 +78,7 @@ module IndirectCommandInjection {
19378
SystemControlledCommandLineArgumentSanitizer() {
19479
// `process.argv[0]` and `process.argv[1]` are paths to `node` and `main`.
19580
exists(string index | index = "0" or index = "1" |
196-
this = any(CommandLineArgumentsArray a).getAPropertyRead(index)
81+
this = DataFlow::globalVarRef("process").getAPropertyRead("argv").getAPropertyRead(index)
19782
)
19883
}
19984
}

‎javascript/ql/lib/semmle/javascript/security/dataflow/IndirectCommandInjectionQuery.qll

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,4 @@ class Configuration extends TaintTracking::Configuration {
2828
override predicate isSink(DataFlow::Node sink) { this.isSinkWithHighlight(sink, _) }
2929

3030
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
31-
32-
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
33-
argsParseStep(pred, succ)
34-
}
3531
}

‎javascript/ql/test/library-tests/threat-models/sources/sources.js

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,3 +2,43 @@ import 'dummy';
22

33
var x = process.env['foo']; // $ threat-source=environment
44
SINK(x); // $ hasFlow
5+
6+
var y = process.argv[2]; // $ threat-source=commandargs
7+
SINK(y); // $ hasFlow
8+
9+
10+
// Accessing command line arguments using yargs
11+
// https://www.npmjs.com/package/yargs/v/17.7.2
12+
const yargs = require('yargs/yargs');
13+
const { hideBin } = require('yargs/helpers');
14+
const argv = yargs(hideBin(process.argv)).argv; // $ threat-source=commandargs
15+
16+
SINK(argv.foo); // $ MISSING: hasFlow
17+
18+
// older version
19+
// https://www.npmjs.com/package/yargs/v/7.1.2
20+
const yargsOld = require('yargs');
21+
const argvOld = yargsOld.argv; // $ threat-source=commandargs
22+
23+
SINK(argvOld.foo); // $ hasFlow
24+
25+
// Accessing command line arguments using yargs-parser
26+
const yargsParser = require('yargs-parser');
27+
const src = process.argv.slice(2); // $ threat-source=commandargs
28+
const parsedArgs = yargsParser(src);
29+
30+
SINK(parsedArgs.foo); // $ hasFlow
31+
32+
// Accessing command line arguments using minimist
33+
const minimist = require('minimist');
34+
const args = minimist(process.argv.slice(2)); // $ threat-source=commandargs
35+
36+
SINK(args.foo); // $ hasFlow
37+
38+
39+
// Accessing command line arguments using commander
40+
const { Command } = require('commander'); // $ SPURIOUS: threat-source=commandargs
41+
const program = new Command();
42+
program.parse(process.argv); // $ threat-source=commandargs
43+
44+
SINK(program.opts().foo); // $ hasFlow SPURIOUS: threat-source=commandargs

0 commit comments

Comments
 (0)