Skip to content

Commit 921198e

Browse files
am0o0am0o0
committed
add separate query for sinks that accepts data: URL
1 parent f6737b3 commit 921198e

File tree

13 files changed

+448
-263
lines changed

13 files changed

+448
-263
lines changed

‎.vscode/settings.json

Lines changed: 0 additions & 5 deletions
This file was deleted.

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

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -761,13 +761,6 @@ module NodeJSLib {
761761
}
762762
}
763763

764-
/**
765-
* The dynamic import expression input can be a `data:` URL which loads any module from that data
766-
*/
767-
class DynamicImport extends CodeInjection::Sink, DataFlow::ExprNode {
768-
DynamicImport() { this = any(DynamicImportExpr e).getSource().flow() }
769-
}
770-
771764
/**
772765
* A call to a method from module `child_process`.
773766
*/

‎javascript/ql/src/experimental/Security/CWE-094-dataURL/CodeInjection.inc.qhelp

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Directly evaluating user input (for example, an HTTP request parameter) as code without properly
9+
sanitizing the input first allows an attacker arbitrary code execution. This can occur when user
10+
input is treated as JavaScript, or passed to a framework which interprets it as an expression to be
11+
evaluated. Examples include AngularJS expressions or JQuery selectors.
12+
</p>
13+
</overview>
14+
15+
<recommendation>
16+
<p>
17+
Avoid including user input in any expression which may be dynamically evaluated. If user input must
18+
be included, use context-specific escaping before
19+
including it. It is important that the correct escaping is used for the type of evaluation that will
20+
occur.
21+
</p>
22+
</recommendation>
23+
24+
<example>
25+
<p>
26+
The following example shows part of the page URL being evaluated as JavaScript code on the server. This allows an
27+
attacker to provide JavaScript within the URL and send it to server. client side attacks need victim users interaction
28+
like clicking on a attacker provided URL.
29+
</p>
30+
31+
<sample src="examples/CodeInjection.js" />
32+
33+
</example>
34+
35+
<references>
36+
<li>
37+
OWASP:
38+
<a href="https://www.owasp.org/index.php/Code_Injection">Code Injection</a>.
39+
</li>
40+
<li>
41+
Wikipedia: <a href="https://en.wikipedia.org/wiki/Code_injection">Code Injection</a>.
42+
</li>
43+
<li>
44+
PortSwigger Research Blog:
45+
<a href="https://portswigger.net/research/server-side-template-injection">Server-Side Template Injection</a>.
46+
</li>
47+
</references>
48+
</qhelp>

‎javascript/ql/src/experimental/Security/CWE-094-dataURL/CodeInjection.qhelp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<include src="CodeInjection.inc.qhelp" />
6+
</qhelp>

‎javascript/ql/src/experimental/Security/CWE-094-dataURL/CodeInjection.ql

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
/**
2+
* @name Code injection
3+
* @description Interpreting unsanitized user input as code allows a malicious user arbitrary
4+
* code execution.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 9.3
8+
* @precision high
9+
* @id js/code-injection
10+
* @tags security
11+
* external/cwe/cwe-094
12+
* external/cwe/cwe-095
13+
* external/cwe/cwe-079
14+
* external/cwe/cwe-116
15+
*/
16+
17+
import javascript
18+
import DataFlow
19+
import DataFlow::PathGraph
20+
21+
abstract class Sanitizer extends DataFlow::Node { }
22+
23+
abstract class Sink extends DataFlow::Node { }
24+
25+
/** A non-first leaf in a string-concatenation. Seen as a sanitizer for dynamic import code injection. */
26+
class NonFirstStringConcatLeaf extends Sanitizer {
27+
NonFirstStringConcatLeaf() {
28+
exists(StringOps::ConcatenationRoot root |
29+
this = root.getALeaf() and
30+
not this = root.getFirstLeaf()
31+
)
32+
or
33+
exists(DataFlow::CallNode join |
34+
join = DataFlow::moduleMember("path", "join").getACall() and
35+
this = join.getArgument([1 .. join.getNumArgument() - 1])
36+
)
37+
}
38+
}
39+
40+
/**
41+
* The dynamic import expression input can be a `data:` URL which loads any module from that data
42+
*/
43+
class DynamicImport extends DataFlow::ExprNode {
44+
DynamicImport() { this = any(DynamicImportExpr e).getSource().flow() }
45+
}
46+
47+
/**
48+
* The dynamic import expression input can be a `data:` URL which loads any module from that data
49+
*/
50+
class WorkerThreads extends DataFlow::Node {
51+
WorkerThreads() {
52+
this = API::moduleImport("node:worker_threads").getMember("Worker").getParameter(0).asSink()
53+
}
54+
}
55+
56+
class WorkerThreadsLabel extends FlowLabel {
57+
WorkerThreadsLabel() { this = "worker_threads" }
58+
}
59+
60+
class DynamicImportLabel extends FlowLabel {
61+
DynamicImportLabel() { this = "DynamicImport" }
62+
}
63+
64+
/**
65+
* A taint-tracking configuration for reasoning about code injection vulnerabilities.
66+
*/
67+
class Configuration extends TaintTracking::Configuration {
68+
Configuration() { this = "CodeInjection" }
69+
70+
override predicate isSource(DataFlow::Node source, FlowLabel label) {
71+
source instanceof RemoteFlowSource and
72+
(label instanceof DynamicImportLabel or label instanceof WorkerThreadsLabel)
73+
}
74+
75+
override predicate isSink(DataFlow::Node sink, FlowLabel label) {
76+
sink instanceof DynamicImport and label instanceof DynamicImportLabel
77+
or
78+
sink instanceof WorkerThreads and label instanceof WorkerThreadsLabel
79+
}
80+
81+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
82+
83+
override predicate isAdditionalFlowStep(
84+
DataFlow::Node pred, DataFlow::Node succ, FlowLabel predlbl, FlowLabel succlbl
85+
) {
86+
exists(DataFlow::NewNode newUrl | succ = newUrl |
87+
newUrl = DataFlow::globalVarRef("URL").getAnInstantiation() and
88+
pred = newUrl.getArgument(0)
89+
) and
90+
predlbl instanceof WorkerThreadsLabel and
91+
succlbl instanceof WorkerThreadsLabel
92+
}
93+
}
94+
95+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
96+
where cfg.hasFlowPath(source, sink)
97+
select sink.getNode(), source, sink, sink.getNode() + " depends on a $@.", source.getNode(),
98+
"user-provided value"

‎javascript/ql/src/experimental/Security/CWE-094-dataURL/examples/CodeInjection.js

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
const { Worker } = require('node:worker_threads');
2+
var app = require('express')();
3+
4+
app.post('/path', async function (req, res) {
5+
const payload = req.query.queryParameter // like: payload = 'data:text/javascript,console.log("hello!");//'
6+
const payloadURL = new URL(payload)
7+
new Worker(payloadURL);
8+
});
9+
10+
app.post('/path2', async function (req, res) {
11+
const payload = req.query.queryParameter // like: payload = 'data:text/javascript,console.log("hello!");//'
12+
await import(payload)
13+
});
14+

‎javascript/ql/test/experimental/Security/CWE-094-dataURL/CodeInjection.expected

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
WARNING: Unused class Sink (/home/am/CodeQL-home/codeql-repo-amammad/javascript/ql/src/experimental/Security/CWE-094-dataURL/CodeInjection.ql:23,16-20)
2+
nodes
3+
| test.js:18:11:18:44 | payload |
4+
| test.js:18:21:18:44 | req.que ... rameter |
5+
| test.js:18:21:18:44 | req.que ... rameter |
6+
| test.js:20:18:20:24 | payload |
7+
| test.js:20:18:20:24 | payload |
8+
edges
9+
| test.js:18:11:18:44 | payload | test.js:20:18:20:24 | payload |
10+
| test.js:18:11:18:44 | payload | test.js:20:18:20:24 | payload |
11+
| test.js:18:21:18:44 | req.que ... rameter | test.js:18:11:18:44 | payload |
12+
| test.js:18:21:18:44 | req.que ... rameter | test.js:18:11:18:44 | payload |
13+
#select
14+
| test.js:20:18:20:24 | payload | test.js:18:21:18:44 | req.que ... rameter | test.js:20:18:20:24 | payload | payload depends on a $@. | test.js:18:21:18:44 | req.que ... rameter | user-provided value |

‎javascript/ql/test/experimental/Security/CWE-094-dataURL/CodeInjection.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/Security/CWE-094-dataURL/CodeInjection.ql

‎javascript/ql/test/experimental/Security/CWE-094-dataURL/test.js

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
const { Worker } = require('node:worker_threads');
2+
var app = require('express')();
3+
4+
app.post('/path', async function (req, res) {
5+
const payload = req.query.queryParameter // like: payload = 'data:text/javascript,console.log("hello!");//'
6+
let payloadURL = new URL(payload + sth)
7+
// NOT OK
8+
new Worker(payloadURL);
9+
// NOT OK
10+
payloadURL = new URL(payload + sth)
11+
new Worker(payloadURL);
12+
// OK
13+
payloadURL = new URL(sth + payload)
14+
new Worker(payloadURL);
15+
});
16+
17+
app.post('/path2', async function (req, res) {
18+
const payload = req.query.queryParameter // like: payload = 'data:text/javascript,console.log("hello!");//'
19+
// NOT OK
20+
await import(payload)
21+
// NOT OK
22+
await import(payload + sth)
23+
// OK
24+
await import(sth + payload)
25+
});
26+

‎javascript/ql/test/library-tests/frameworks/NodeJSLib/CodeInjectionSink.qll

Lines changed: 0 additions & 4 deletions
This file was deleted.

0 commit comments

Comments
 (0)