Skip to content

Commit b9ecf1a

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#3447 from erik-krogh/LibCmdInjection
Approved by asgerf, mchammer01
2 parents 9259dca + b79b25e commit b9ecf1a

31 files changed

+1267
-6
lines changed

‎change-notes/1.25/analysis-javascript.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
| Cross-site scripting through DOM (`js/xss-through-dom`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities where existing text from the DOM is used as HTML. Results are not shown on LGTM by default. |
3030
| Incomplete HTML attribute sanitization (`js/incomplete-html-attribute-sanitization`) | security, external/cwe/cwe-20, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities due to incomplete sanitization of HTML meta-characters. Results are shown on LGTM by default. |
3131
| Unsafe expansion of self-closing HTML tag (`js/unsafe-html-expansion`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities caused by unsafe expansion of self-closing HTML tags. |
32+
| Unsafe shell command constructed from library input (`js/shell-command-constructed-from-input`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights potential command injections due to a shell command being constructed from library inputs. Results are shown on LGTM by default. |
3233

3334
## Changes to existing queries
3435

‎javascript/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.qhelp

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
8+
Dynamically constructing a shell command with inputs from exported
9+
functions may inadvertently change the meaning of the shell command.
10+
11+
Clients using the exported function may use inputs containing
12+
characters that the shell interprets in a special way, for instance
13+
quotes and spaces.
14+
15+
This can result in the shell command misbehaving, or even
16+
allowing a malicious user to execute arbitrary commands on the system.
17+
</p>
18+
19+
20+
</overview>
21+
<recommendation>
22+
23+
<p>
24+
If possible, provide the dynamic arguments to the shell as an array
25+
using a safe API such as <code>child_process.execFile</code> to avoid
26+
interpretation by the shell.
27+
</p>
28+
29+
<p>
30+
Alternatively, if the shell command must be constructed
31+
dynamically, then add code to ensure that special characters
32+
do not alter the shell command unexpectedly.
33+
</p>
34+
35+
</recommendation>
36+
<example>
37+
38+
<p>
39+
The following example shows a dynamically constructed shell
40+
command that downloads a file from a remote URL.
41+
</p>
42+
43+
<sample src="examples/unsafe-shell-command-construction.js" />
44+
45+
<p>
46+
The shell command will, however, fail to work as intended if the
47+
input contains spaces or other special characters interpreted in a
48+
special way by the shell.
49+
</p>
50+
51+
<p>
52+
Even worse, a client might pass in user-controlled
53+
data, not knowing that the input is interpreted as a shell command.
54+
This could allow a malicious user to provide the input <code>http://example.org; cat /etc/passwd</code>
55+
in order to execute the command <code>cat /etc/passwd</code>.
56+
</p>
57+
58+
<p>
59+
To avoid such potentially catastrophic behaviors, provide the
60+
inputs from exported functions as an argument that does not
61+
get interpreted by a shell:
62+
</p>
63+
64+
<sample src="examples/unsafe-shell-command-construction_fixed.js" />
65+
66+
</example>
67+
<references>
68+
69+
<li>
70+
OWASP:
71+
<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
72+
</li>
73+
74+
</references>
75+
</qhelp>

‎javascript/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.ql

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @name Unsafe shell command constructed from library input
3+
* @description Using externally controlled strings in a command line may allow a malicious
4+
* user to change the meaning of the command.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @precision high
8+
* @id js/shell-command-constructed-from-input
9+
* @tags correctness
10+
* security
11+
* external/cwe/cwe-078
12+
* external/cwe/cwe-088
13+
*/
14+
15+
import javascript
16+
import semmle.javascript.security.dataflow.UnsafeShellCommandConstruction::UnsafeShellCommandConstruction
17+
import DataFlow::PathGraph
18+
19+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
20+
where cfg.hasFlowPath(source, sink) and sinkNode = sink.getNode()
21+
select sinkNode.getAlertLocation(), source, sink, "$@ based on libary input is later used in $@.",
22+
sinkNode.getAlertLocation(), sinkNode.getSinkType(), sinkNode.getCommandExecution(),
23+
"shell command"

‎javascript/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
var cp = require("child_process");
2+
3+
module.exports = function download(path, callback) {
4+
cp.exec("wget " + path, callback);
5+
}

‎javascript/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction_fixed.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
var cp = require("child_process");
2+
3+
module.exports = function download(path, callback) {
4+
cp.execFile("wget", [path], callback);
5+
}

‎javascript/ql/src/semmle/javascript/PackageExports.qll

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
/**
2+
* EXPERIMENTAL. This API may change in the future.
3+
*
4+
* Provides predicates for working with values exported from a package.
5+
*/
6+
7+
import javascript
8+
9+
/**
10+
* Gets the number of occurrences of "/" in `path`.
11+
*/
12+
bindingset[path]
13+
private int countSlashes(string path) { result = count(path.splitAt("/")) - 1 }
14+
15+
/**
16+
* Gets the topmost package.json that appears in the project.
17+
*
18+
* There can be multiple results if the there exists multiple package.json that are equally deeply nested in the folder structure.
19+
* Results are limited to package.json files that are at most nested 2 directories deep.
20+
*/
21+
PackageJSON getTopmostPackageJSON() {
22+
result =
23+
min(PackageJSON j |
24+
countSlashes(j.getFile().getRelativePath()) <= 3
25+
|
26+
j order by countSlashes(j.getFile().getRelativePath())
27+
)
28+
}
29+
30+
/**
31+
* Gets a value exported by the main module from the package.json `packageJSON`.
32+
* The value is either directly the `module.exports` value, a nested property of `module.exports`, or a method on an exported class.
33+
*/
34+
DataFlow::Node getAValueExportedBy(PackageJSON packageJSON) {
35+
result = getAnExportFromModule(packageJSON.getMainModule())
36+
or
37+
result = getAValueExportedBy(packageJSON).(DataFlow::PropWrite).getRhs()
38+
or
39+
exists(DataFlow::SourceNode callee |
40+
callee = getAValueExportedBy(packageJSON).(DataFlow::NewNode).getCalleeNode().getALocalSource()
41+
|
42+
result = callee.getAPropertyRead("prototype").getAPropertyWrite()
43+
or
44+
result = callee.(DataFlow::ClassNode).getAnInstanceMethod()
45+
)
46+
or
47+
result = getAValueExportedBy(packageJSON).getALocalSource()
48+
or
49+
result = getAValueExportedBy(packageJSON).(DataFlow::SourceNode).getAPropertyReference()
50+
or
51+
exists(Module mod |
52+
mod = getAValueExportedBy(packageJSON).getEnclosingExpr().(Import).getImportedModule()
53+
|
54+
result = getAnExportFromModule(mod)
55+
)
56+
or
57+
exists(DataFlow::ClassNode cla | cla = getAValueExportedBy(packageJSON) |
58+
result = cla.getAnInstanceMethod() or
59+
result = cla.getAStaticMethod() or
60+
result = cla.getConstructor()
61+
)
62+
}
63+
64+
/**
65+
* Gets an exported node from the module `mod`.
66+
*/
67+
private DataFlow::Node getAnExportFromModule(Module mod) {
68+
result.analyze().getAValue() = mod.(NodeModule).getAModuleExportsValue()
69+
or
70+
exists(ASTNode export | result.getEnclosingExpr() = export | mod.exports(_, export))
71+
}

‎javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -779,7 +779,8 @@ module TaintTracking {
779779
*/
780780
class AdHocWhitelistCheckSanitizer extends SanitizerGuardNode, DataFlow::CallNode {
781781
AdHocWhitelistCheckSanitizer() {
782-
getCalleeName().regexpMatch("(?i).*((?<!un)safe|whitelist|allow|(?<!un)auth(?!or\\b)).*") and
782+
getCalleeName()
783+
.regexpMatch("(?i).*((?<!un)safe|whitelist|(?<!in)valid|allow|(?<!un)auth(?!or\\b)).*") and
783784
getNumArgument() = 1
784785
}
785786

‎javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -449,10 +449,7 @@ module NodeJSLib {
449449

450450
private DataFlow::SourceNode fsModule(DataFlow::TypeTracker t) {
451451
exists(string moduleName |
452-
moduleName = "fs" or
453-
moduleName = "graceful-fs" or
454-
moduleName = "fs-extra" or
455-
moduleName = "original-fs"
452+
moduleName = ["mz/fs", "original-fs", "fs-extra", "graceful-fs", "fs"]
456453
|
457454
result = DataFlow::moduleImport(moduleName)
458455
or
@@ -621,6 +618,8 @@ module NodeJSLib {
621618

622619
ChildProcessMethodCall() {
623620
this = maybePromisified(DataFlow::moduleMember("child_process", methodName)).getACall()
621+
or
622+
this = DataFlow::moduleMember("mz/child_process", methodName).getACall()
624623
}
625624

626625
private DataFlow::Node getACommandArgument(boolean shell) {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ private DataFlow::SourceNode argumentList(SystemCommandExecution sys, DataFlow::
5252
result = pred.backtrack(t2, t)
5353
or
5454
t = t2.continue() and
55-
TaintTracking::arrayFunctionTaintStep(result, pred, _)
55+
TaintTracking::arrayFunctionTaintStep(any(DataFlow::Node n | result.flowsTo(n)), pred, _)
5656
)
5757
}
5858

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about shell command
3+
* constructed from library input vulnerabilities (CWE-078).
4+
*
5+
* Note, for performance reasons: only import this file if
6+
* `UnsafeShellCommandConstruction::Configuration` is needed, otherwise
7+
* `UnsafeShellCommandConstructionCustomizations` should be imported instead.
8+
*/
9+
10+
import javascript
11+
12+
/**
13+
* Classes and predicates for the shell command constructed from library input query.
14+
*/
15+
module UnsafeShellCommandConstruction {
16+
import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruction
17+
18+
/**
19+
* A taint-tracking configuration for reasoning about shell command constructed from library input vulnerabilities.
20+
*/
21+
class Configuration extends TaintTracking::Configuration {
22+
Configuration() { this = "UnsafeShellCommandConstruction" }
23+
24+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
25+
26+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
27+
28+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
29+
30+
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
31+
guard instanceof PathExistsSanitizerGuard or
32+
guard instanceof TaintTracking::AdHocWhitelistCheckSanitizer
33+
}
34+
}
35+
}

0 commit comments

Comments
 (0)