Skip to content

JS: Dynamic import as code injection sink #14293

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
erik-krogh merged 15 commits into from
Jun 20, 2024
Merged

JS: Dynamic import as code injection sink #14293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
06114d9
V1
am0o0 Sep 22, 2023
344869f
change commandExecution sink to CodeInjection sink
am0o0 Sep 22, 2023
f6737b3
fix FP
am0o0 Sep 25, 2023
921198e
add separate query for sinks that accepts data: URL
am0o0 Sep 27, 2023
75f0fc4
fix a mistake
am0o0 Sep 28, 2023
f41bc1f
revert nodeJSLib
am0o0 Sep 28, 2023
41e7b91
fix flowLabels
am0o0 Oct 8, 2023
00b6e1f
fix tests
am0o0 Oct 8, 2023
1567168
remove unused flowLable, update path query alert message
am0o0 Oct 8, 2023
3f41a42
remove unused classes
am0o0 Oct 8, 2023
e3e59e0
Merge branch 'github:main' into amammad-js-CodeInjection_dynamic_import
am0o0 Jun 4, 2024
d27a378
change query-id to avoid duplicate ids
am0o0 Jun 6, 2024
8258e37
use PascalCase for URLConstructorLabel
am0o0 Jun 6, 2024
9db334d
update select statement, update test cases
am0o0 Jun 7, 2024
bb03a9f
format the query file
am0o0 Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions javascript/ql/src/experimental/Security/CWE-094-dataURL/CodeInjection.inc.qhelp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>

<overview>
<p>
Directly evaluating user input (for example, an HTTP request parameter) as code without properly
sanitizing the input first allows an attacker arbitrary code execution. This can occur when user
input is treated as JavaScript, or passed to a framework which interprets it as an expression to be
evaluated. Examples include AngularJS expressions or JQuery selectors.
</p>
</overview>

<recommendation>
<p>
Avoid including user input in any expression which may be dynamically evaluated. If user input must
be included, use context-specific escaping before
including it. It is important that the correct escaping is used for the type of evaluation that will
occur.
</p>
</recommendation>

<example>
<p>
The following example shows part of the page URL being evaluated as JavaScript code on the server. This allows an
attacker to provide JavaScript within the URL and send it to server. client side attacks need victim users interaction
like clicking on a attacker provided URL.
</p>

<sample src="examples/CodeInjection.js" />

</example>

<references>
<li>
OWASP:
<a href="https://www.owasp.org/index.php/Code_Injection">Code Injection</a>.
</li>
<li>
Wikipedia: <a href="https://en.wikipedia.org/wiki/Code_injection">Code Injection</a>.
</li>
<li>
PortSwigger Research Blog:
<a href="https://portswigger.net/research/server-side-template-injection">Server-Side Template Injection</a>.
</li>
</references>
</qhelp>
6 changes: 6 additions & 0 deletions javascript/ql/src/experimental/Security/CWE-094-dataURL/CodeInjection.qhelp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<include src="CodeInjection.inc.qhelp" />
</qhelp>
89 changes: 89 additions & 0 deletions javascript/ql/src/experimental/Security/CWE-094-dataURL/CodeInjection.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/**
* @name Code injection
* @description Interpreting unsanitized user input as code allows a malicious user arbitrary
* code execution.
* @kind path-problem
* @problem.severity error
* @security-severity 9.3
* @precision high
* @id js/code-injection-dynamic-import
* @tags security
* external/cwe/cwe-094
* external/cwe/cwe-095
* external/cwe/cwe-079
* external/cwe/cwe-116
*/

import javascript
import DataFlow
import DataFlow::PathGraph

abstract class Sanitizer extends DataFlow::Node { }

/** A non-first leaf in a string-concatenation. Seen as a sanitizer for dynamic import code injection. */
class NonFirstStringConcatLeaf extends Sanitizer {
NonFirstStringConcatLeaf() {
exists(StringOps::ConcatenationRoot root |
this = root.getALeaf() and
not this = root.getFirstLeaf()
)
or
exists(DataFlow::CallNode join |
join = DataFlow::moduleMember("path", "join").getACall() and
this = join.getArgument([1 .. join.getNumArgument() - 1])
)
}
}

/**
* The dynamic import expression input can be a `data:` URL which loads any module from that data
*/
class DynamicImport extends DataFlow::ExprNode {
DynamicImport() { this = any(DynamicImportExpr e).getSource().flow() }
}

/**
* The dynamic import expression input can be a `data:` URL which loads any module from that data
*/
class WorkerThreads extends DataFlow::Node {
WorkerThreads() {
this = API::moduleImport("node:worker_threads").getMember("Worker").getParameter(0).asSink()
}
}

class UrlConstructorLabel extends FlowLabel {
UrlConstructorLabel() { this = "UrlConstructorLabel" }
}

/**
* A taint-tracking configuration for reasoning about code injection vulnerabilities.
*/
class Configuration extends TaintTracking::Configuration {
Configuration() { this = "CodeInjection" }

override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }

override predicate isSink(DataFlow::Node sink) { sink instanceof DynamicImport }

override predicate isSink(DataFlow::Node sink, FlowLabel label) {
sink instanceof WorkerThreads and label instanceof UrlConstructorLabel
}

override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }

override predicate isAdditionalFlowStep(
DataFlow::Node pred, DataFlow::Node succ, FlowLabel predlbl, FlowLabel succlbl
) {
exists(DataFlow::NewNode newUrl | succ = newUrl |
newUrl = DataFlow::globalVarRef("URL").getAnInstantiation() and
pred = newUrl.getArgument(0)
) and
predlbl instanceof StandardFlowLabel and
succlbl instanceof UrlConstructorLabel
}
}

from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "This command line depends on a $@.", source.getNode(),
"user-provided value"
14 changes: 14 additions & 0 deletions javascript/ql/src/experimental/Security/CWE-094-dataURL/examples/CodeInjection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
const { Worker } = require('node:worker_threads');
var app = require('express')();

app.post('/path', async function (req, res) {
const payload = req.query.queryParameter // like: payload = 'data:text/javascript,console.log("hello!");//'
const payloadURL = new URL(payload)
new Worker(payloadURL);
});

app.post('/path2', async function (req, res) {
const payload = req.query.queryParameter // like: payload = 'data:text/javascript,console.log("hello!");//'
await import(payload)
});

51 changes: 51 additions & 0 deletions javascript/ql/test/experimental/Security/CWE-094-dataURL/CodeInjection.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
nodes
| test.js:5:11:5:44 | payload |
| test.js:5:21:5:44 | req.que ... rameter |
| test.js:5:21:5:44 | req.que ... rameter |
| test.js:6:9:6:43 | payloadURL |
| test.js:6:22:6:43 | new URL ... + sth) |
| test.js:6:30:6:36 | payload |
| test.js:6:30:6:42 | payload + sth |
| test.js:7:16:7:25 | payloadURL |
| test.js:7:16:7:25 | payloadURL |
| test.js:9:5:9:39 | payloadURL |
| test.js:9:18:9:39 | new URL ... + sth) |
| test.js:9:26:9:32 | payload |
| test.js:9:26:9:38 | payload + sth |
| test.js:10:16:10:25 | payloadURL |
| test.js:10:16:10:25 | payloadURL |
| test.js:17:11:17:44 | payload |
| test.js:17:21:17:44 | req.que ... rameter |
| test.js:17:21:17:44 | req.que ... rameter |
| test.js:18:18:18:24 | payload |
| test.js:18:18:18:24 | payload |
| test.js:19:18:19:24 | payload |
| test.js:19:18:19:30 | payload + sth |
| test.js:19:18:19:30 | payload + sth |
edges
| test.js:5:11:5:44 | payload | test.js:6:30:6:36 | payload |
| test.js:5:11:5:44 | payload | test.js:9:26:9:32 | payload |
| test.js:5:21:5:44 | req.que ... rameter | test.js:5:11:5:44 | payload |
| test.js:5:21:5:44 | req.que ... rameter | test.js:5:11:5:44 | payload |
| test.js:6:9:6:43 | payloadURL | test.js:7:16:7:25 | payloadURL |
| test.js:6:9:6:43 | payloadURL | test.js:7:16:7:25 | payloadURL |
| test.js:6:22:6:43 | new URL ... + sth) | test.js:6:9:6:43 | payloadURL |
| test.js:6:30:6:36 | payload | test.js:6:30:6:42 | payload + sth |
| test.js:6:30:6:42 | payload + sth | test.js:6:22:6:43 | new URL ... + sth) |
| test.js:9:5:9:39 | payloadURL | test.js:10:16:10:25 | payloadURL |
| test.js:9:5:9:39 | payloadURL | test.js:10:16:10:25 | payloadURL |
| test.js:9:18:9:39 | new URL ... + sth) | test.js:9:5:9:39 | payloadURL |
| test.js:9:26:9:32 | payload | test.js:9:26:9:38 | payload + sth |
| test.js:9:26:9:38 | payload + sth | test.js:9:18:9:39 | new URL ... + sth) |
| test.js:17:11:17:44 | payload | test.js:18:18:18:24 | payload |
| test.js:17:11:17:44 | payload | test.js:18:18:18:24 | payload |
| test.js:17:11:17:44 | payload | test.js:19:18:19:24 | payload |
| test.js:17:21:17:44 | req.que ... rameter | test.js:17:11:17:44 | payload |
| test.js:17:21:17:44 | req.que ... rameter | test.js:17:11:17:44 | payload |
| test.js:19:18:19:24 | payload | test.js:19:18:19:30 | payload + sth |
| test.js:19:18:19:24 | payload | test.js:19:18:19:30 | payload + sth |
#select
| test.js:7:16:7:25 | payloadURL | test.js:5:21:5:44 | req.que ... rameter | test.js:7:16:7:25 | payloadURL | This command line depends on a $@. | test.js:5:21:5:44 | req.que ... rameter | user-provided value |
| test.js:10:16:10:25 | payloadURL | test.js:5:21:5:44 | req.que ... rameter | test.js:10:16:10:25 | payloadURL | This command line depends on a $@. | test.js:5:21:5:44 | req.que ... rameter | user-provided value |
| test.js:18:18:18:24 | payload | test.js:17:21:17:44 | req.que ... rameter | test.js:18:18:18:24 | payload | This command line depends on a $@. | test.js:17:21:17:44 | req.que ... rameter | user-provided value |
| test.js:19:18:19:30 | payload + sth | test.js:17:21:17:44 | req.que ... rameter | test.js:19:18:19:30 | payload + sth | This command line depends on a $@. | test.js:17:21:17:44 | req.que ... rameter | user-provided value |
1 change: 1 addition & 0 deletions javascript/ql/test/experimental/Security/CWE-094-dataURL/CodeInjection.qlref
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
experimental/Security/CWE-094-dataURL/CodeInjection.ql
22 changes: 22 additions & 0 deletions javascript/ql/test/experimental/Security/CWE-094-dataURL/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
const { Worker } = require('node:worker_threads');
var app = require('express')();

app.post('/path', async function (req, res) {
const payload = req.query.queryParameter // like: payload = 'data:text/javascript,console.log("hello!");//'
let payloadURL = new URL(payload + sth) // NOT OK
new Worker(payloadURL);

payloadURL = new URL(payload + sth) // NOT OK
new Worker(payloadURL);

payloadURL = new URL(sth + payload) // OK
new Worker(payloadURL);
});

app.post('/path2', async function (req, res) {
const payload = req.query.queryParameter // like: payload = 'data:text/javascript,console.log("hello!");//'
await import(payload) // NOT OK
await import(payload + sth) // NOT OK
await import(sth + payload) // OK
});