-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
JS: Dynamic import as code injection sink #14293
Merged
erik-krogh
merged 15 commits into
github:main
from
am0o0:amammad-js-CodeInjection_dynamic_import
Jun 20, 2024
Merged
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
06114d9
V1
am0o0 344869f
change commandExecution sink to CodeInjection sink
am0o0 f6737b3
fix FP
am0o0 921198e
add separate query for sinks that accepts data: URL
am0o0 75f0fc4
fix a mistake
am0o0 f41bc1f
revert nodeJSLib
am0o0 41e7b91
fix flowLabels
am0o0 00b6e1f
fix tests
am0o0 1567168
remove unused flowLable, update path query alert message
am0o0 3f41a42
remove unused classes
am0o0 e3e59e0
Merge branch 'github:main' into amammad-js-CodeInjection_dynamic_import
am0o0 d27a378
change query-id to avoid duplicate ids
am0o0 8258e37
use PascalCase for URLConstructorLabel
am0o0 9db334d
update select statement, update test cases
am0o0 bb03a9f
format the query file
am0o0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
48 changes: 48 additions & 0 deletions
48
javascript/ql/src/experimental/Security/CWE-094-dataURL/CodeInjection.inc.qhelp
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
6
javascript/ql/src/experimental/Security/CWE-094-dataURL/CodeInjection.qhelp
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
89
javascript/ql/src/experimental/Security/CWE-094-dataURL/CodeInjection.ql
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
* @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, sink.getNode() + "This command line depends on a $@.", | ||
source.getNode(), "user-provided value" |
14 changes: 14 additions & 0 deletions
14
javascript/ql/src/experimental/Security/CWE-094-dataURL/examples/CodeInjection.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) | ||
}); | ||
|
52 changes: 52 additions & 0 deletions
52
javascript/ql/test/experimental/Security/CWE-094-dataURL/CodeInjection.expected
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
WARNING: Unused class Sink (/home/am/CodeQL-home/codeql-repo-amammad/javascript/ql/src/experimental/Security/CWE-094-dataURL/CodeInjection.ql:23,16-20) | ||
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 | payloadURL 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 | payloadURL 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 | payload 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 | payload + sth depends on a $@. | test.js:17:21:17:44 | req.que ... rameter | user-provided value | |
1 change: 1 addition & 0 deletions
1
javascript/ql/test/experimental/Security/CWE-094-dataURL/CodeInjection.qlref
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
22
javascript/ql/test/experimental/Security/CWE-094-dataURL/test.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
}); | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't have the same query-id as the query in the standard suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ this is still an issue.
And now the CI is complaining about it.