JS: Dynamic import as code injection sink #14293

am0o0 · 2023-09-22T09:38:31Z

Dynamic import in nodejs support URLs starts with data: which is dangerous.
There is another nodejs API that accepts the data: URL which is:

const {Worker} = require('node:worker_threads');
new Worker(new URL('data:text/javascript,console.log("hello!");'))

but it needs to be a URL Type as input, not any string value that starts with data:, I'm not sure what is the best way to implement it.

am0o0 · 2023-09-28T10:41:48Z

Hi, I wanted to add two sinks, one of the sinks should have a new URL instance in one of the middle nodes, so I've used flow steps but it seems that I can't reach some sinks like this.

pwntester · 2023-10-02T09:02:49Z

@amammad Not sure flow labels are needed for this use case. Can you try removing them and checking if the taint flows through the string concatenations and URL construction (keeping the existing flow step)?

am0o0 · 2023-10-02T09:23:27Z

@pwntester I'm using labels because dynamic import doesn't need a URL construction but worker needs.
do you want to simply create two queries for each one? Also, how can I indicate on the taint configuration that we must have a URL construction as a middle dataflow node?

pwntester · 2023-10-02T13:11:05Z

Does the Worker constructor accept either a String or an URL and only the later is vulnerable? If thats the case, you need the Flow labels and affixing a label on the URL constructor that you can later check at the sink. Note that you can ignore that check for the imports sink but enforce it for the Worker one.

am0o0 · 2023-10-02T14:12:25Z

@pwntester
If the input is string then it should be a JS file path which then is not a code injection anymore, it can be arbitrary JS file execution.
If the input is a URL object then it can lead to code injection.

pwntester · 2023-10-03T08:39:50Z

@amammad Ok, in that case you need to use flow labels as you were doing. Sorry for the confusion. You can start use the regular isSource(DataFlow::Node src) which should affix the default implicit label (just an empty string) and then at the URL constructor taint step affix a new label representing that the taint tracking flowed through this specific taint step. At the sink, you can use the isSink(DataFlow::Node sink, FlowLabel sinkLbl) and check the label only for the Worker sink. I think something like that should work but have not tried it:

  override predicate isSource(DataFlow::Node source) {
    source instanceof RemoteFlowSource
  }
  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 = StandardFlowLabel and
    succlbl instanceof URLConstructorLabel
  }
  override predicate isSink(DataFlow::Node sink) {
    sink instanceof DynamicImport
  }
  override predicate isSink(DataFlow::Node sink, FlowLabel label) {
    sink instanceof WorkerThreads and label instanceof URLConstructorLabel
  }

am0o0 · 2023-10-08T09:03:51Z

@pwntester Thank you a lot! I can see the correct results now. you can see the tests and sanitizer also is working for both sinks.

erik-krogh

This looks OK for an experimental query, just a note about the query-id.

The solution that you guys figured out which flow-labels is nice 👍

Also, could you do a merge with main.
The CI hasn't run, and I think that might be because your branch is lacking a lot of progress from main.

erik-krogh · 2024-06-04T11:59:59Z

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

+ * @problem.severity error
+ * @security-severity 9.3
+ * @precision high
+ * @id js/code-injection


This shouldn't have the same query-id as the query in the standard suite.

^ this is still an issue.

And now the CI is complaining about it.

github-actions · 2024-06-06T11:12:18Z

QHelp previews:

javascript/ql/src/Security/CWE-094/CodeInjection.qhelp

Code injection

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.

Recommendation

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.

Example

The following example shows part of the page URL being evaluated as JavaScript code. This allows an attacker to provide JavaScript within the URL. If an attacker can persuade a user to click on a link to such a URL, the attacker can evaluate arbitrary JavaScript in the browser of the user to, for example, steal cookies containing session information.

eval(document.location.href.substring(document.location.href.indexOf("default=")+8))

The following example shows a Pug template being constructed from user input, allowing attackers to run arbitrary code via a payload such as #{global.process.exit(1)}.

const express = require('express')
var pug = require('pug');
const app = express()

app.post('/', (req, res) => {
    var input = req.query.username;
    var template = `
doctype
html
head
    title= 'Hello world'
body
    form(action='/' method='post')
        input#name.form-control(type='text)
        button.btn.btn-primary(type='submit') Submit
    p Hello `+ input
    var fn = pug.compile(template);
    var html = fn();
    res.send(html);
})

Below is an example of how to use a template engine without any risk of template injection. The user input is included via an interpolation expression #{username} whose value is provided as an option to the template, instead of being part of the template string itself:

const express = require('express')
var pug = require('pug');
const app = express()

app.post('/', (req, res) => {
    var input = req.query.username;
    var template = `
doctype
html
head
    title= 'Hello world'
body
    form(action='/' method='post')
        input#name.form-control(type='text)
        button.btn.btn-primary(type='submit') Submit
    p Hello #{username}`
    var fn = pug.compile(template);
    var html = fn({username: input});
    res.send(html);
})

References

OWASP: Code Injection.
Wikipedia: Code Injection.
PortSwigger Research Blog: Server-Side Template Injection.
Common Weakness Enumeration: CWE-94.
Common Weakness Enumeration: CWE-95.
Common Weakness Enumeration: CWE-79.
Common Weakness Enumeration: CWE-116.

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

Code injection

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.

Recommendation

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.

Example

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.

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)
});

References

OWASP: Code Injection.
Wikipedia: Code Injection.
PortSwigger Research Blog: Server-Side Template Injection.
Common Weakness Enumeration: CWE-94.
Common Weakness Enumeration: CWE-95.
Common Weakness Enumeration: CWE-79.
Common Weakness Enumeration: CWE-116.

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

Code injection with additional heuristic sources

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.

Recommendation

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.

Example

The following example shows part of the page URL being evaluated as JavaScript code. This allows an attacker to provide JavaScript within the URL. If an attacker can persuade a user to click on a link to such a URL, the attacker can evaluate arbitrary JavaScript in the browser of the user to, for example, steal cookies containing session information.

eval(document.location.href.substring(document.location.href.indexOf("default=")+8))

The following example shows a Pug template being constructed from user input, allowing attackers to run arbitrary code via a payload such as #{global.process.exit(1)}.

const express = require('express')
var pug = require('pug');
const app = express()

app.post('/', (req, res) => {
    var input = req.query.username;
    var template = `
doctype
html
head
    title= 'Hello world'
body
    form(action='/' method='post')
        input#name.form-control(type='text)
        button.btn.btn-primary(type='submit') Submit
    p Hello `+ input
    var fn = pug.compile(template);
    var html = fn();
    res.send(html);
})

Below is an example of how to use a template engine without any risk of template injection. The user input is included via an interpolation expression #{username} whose value is provided as an option to the template, instead of being part of the template string itself:

const express = require('express')
var pug = require('pug');
const app = express()

app.post('/', (req, res) => {
    var input = req.query.username;
    var template = `
doctype
html
head
    title= 'Hello world'
body
    form(action='/' method='post')
        input#name.form-control(type='text)
        button.btn.btn-primary(type='submit') Submit
    p Hello #{username}`
    var fn = pug.compile(template);
    var html = fn({username: input});
    res.send(html);
})

References

OWASP: Code Injection.
Wikipedia: Code Injection.
PortSwigger Research Blog: Server-Side Template Injection.
Common Weakness Enumeration: CWE-94.
Common Weakness Enumeration: CWE-95.
Common Weakness Enumeration: CWE-79.
Common Weakness Enumeration: CWE-116.

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

erik-krogh · 2024-06-07T12:14:29Z

The Security/CWE-094-dataURL/CodeInjection.qlref test is failing in the CI.

I get the below diff, can you look into that?

-| 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 |
+| test.js:7:16:7:25 | payloadURL | test.js:5:21:5:44 | req.que ... rameter | test.js:7:16:7:25 | payloadURL | payloadURLThis 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 | payloadURLThis 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 | payloadThis 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 | payload + sthThis command line depends on a $@. | test.js:17:21:17:44 | req.que ... rameter | user-provided value |

It's from the select part of your query.
The string you're constructing is sink.getNode() + "This command line depends on a $@.". I think you should remove that first sink.getNode() + part.

am0o0 · 2024-06-07T19:26:55Z

@erik-krogh sorry I should've run tests before pushing changes to remote.

erik-krogh · 2024-06-12T12:25:09Z

The format check is failing on javascript/ql/src/experimental/Security/CWE-094-dataURL/CodeInjection.ql.

You can run codeql query format -i <path-to-ql-file> to run the autoformatter.

am0o0 added 2 commits September 22, 2023 19:19

V1

06114d9

change commandExecution sink to CodeInjection sink

344869f

github-actions bot added the JS label Sep 22, 2023

This was referenced Sep 22, 2023

[JS]: New command execution sinks(Execa, shelljs and dynamic import) github/securitylab#788

Closed

JS: New Command Execution Sinks #14198

Closed

am0o0 and others added 2 commits September 25, 2023 21:09

fix FP

f6737b3

add separate query for sinks that accepts data: URL

921198e

github-actions bot added the documentation label Sep 28, 2023

am0o0 added 2 commits September 28, 2023 20:34

fix a mistake

75f0fc4

revert nodeJSLib

f41bc1f

am0o0 added 2 commits October 8, 2023 11:00

fix flowLabels

41e7b91

fix tests

00b6e1f

am0o0 added 2 commits October 8, 2023 11:06

remove unused flowLable, update path query alert message

1567168

remove unused classes

3f41a42

am0o0 marked this pull request as ready for review May 27, 2024 18:37

am0o0 requested a review from a team as a code owner May 27, 2024 18:37

sidshank added the external-contribution label May 28, 2024

erik-krogh reviewed Jun 4, 2024

View reviewed changes

Merge branch 'github:main' into amammad-js-CodeInjection_dynamic_import

e3e59e0

github-advanced-security bot found potential problems Jun 6, 2024

View reviewed changes

javascript/ql/src/experimental/Security/CWE-094-dataURL/CodeInjection.ql Fixed Show fixed Hide fixed

am0o0 added 2 commits June 6, 2024 13:59

change query-id to avoid duplicate ids

d27a378

use PascalCase for URLConstructorLabel

8258e37

update select statement, update test cases

9db334d

format the query file

bb03a9f

erik-krogh approved these changes Jun 20, 2024

View reviewed changes

erik-krogh merged commit 555d7e5 into github:main Jun 20, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS: Dynamic import as code injection sink #14293

JS: Dynamic import as code injection sink #14293

am0o0 commented Sep 22, 2023

am0o0 commented Sep 28, 2023

pwntester commented Oct 2, 2023 •

edited

Loading

am0o0 commented Oct 2, 2023

pwntester commented Oct 2, 2023 •

edited

Loading

am0o0 commented Oct 2, 2023

pwntester commented Oct 3, 2023

am0o0 commented Oct 8, 2023

erik-krogh left a comment

erik-krogh Jun 4, 2024

erik-krogh Jun 6, 2024

github-actions bot commented Jun 6, 2024

Code injection

Recommendation

Example

References

Code injection

Recommendation

Example

References

Code injection with additional heuristic sources

Recommendation

Example

References

erik-krogh commented Jun 7, 2024

am0o0 commented Jun 7, 2024

erik-krogh commented Jun 12, 2024

JS: Dynamic import as code injection sink #14293

JS: Dynamic import as code injection sink #14293

Conversation

am0o0 commented Sep 22, 2023

am0o0 commented Sep 28, 2023

pwntester commented Oct 2, 2023 • edited Loading

am0o0 commented Oct 2, 2023

pwntester commented Oct 2, 2023 • edited Loading

am0o0 commented Oct 2, 2023

pwntester commented Oct 3, 2023

am0o0 commented Oct 8, 2023

erik-krogh left a comment

Choose a reason for hiding this comment

erik-krogh Jun 4, 2024

Choose a reason for hiding this comment

erik-krogh Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot commented Jun 6, 2024

Code injection

Recommendation

Example

References

Code injection

Recommendation

Example

References

Code injection with additional heuristic sources

Recommendation

Example

References

erik-krogh commented Jun 7, 2024

am0o0 commented Jun 7, 2024

erik-krogh commented Jun 12, 2024

pwntester commented Oct 2, 2023 •

edited

Loading

pwntester commented Oct 2, 2023 •

edited

Loading