Python : Arbitrary code execution due to Js2Py · Pull Request #16771 · github/codeql

ghost · 2024-06-15T15:23:55Z

Js2Py is a Javascript to Python translation library written in Python. It allows users to invoke JavaScript code directly from Python. The Js2Py interpreter by default exposes the entire standard library to it's users. This can lead to security issues if a malicious input were directly.

This PR includes a CodeQL query along with a qhelp and testcases to detect cases where an untrusted input flows to an Js2Py eval call.

This query successfully detects CVE-2023-0297 in pyload/pyloadalong with it's fix. The databases can be downloaded from the links bellow.

https://file.io/qrMEjSJJoTq1
https://filetransfer.io/data-package/a02eab7V#link

ghsecuritylab · 2024-06-15T16:02:39Z

Hello porcupineyhairs 👋
You have submitted this pull request as a bug bounty report in the github/securitylab repository and therefore this pull request has been put into draft state to give time for the GitHub Security Lab to assess the PR. When GitHub Security Lab has finished assessing your pull request, it will be marked automatically as Ready for review. Until then, please don't change the draft state.

In the meantime, feel free to make changes to the pull request. If you'd like to maximize payout for your this and future submissions, here are a few general guidelines, that we might take into consideration when reviewing a submission.

the submission models widely-used frameworks/libraries
the vulnerability modeled in the submission is impactful
the submission finds new true positive vulnerabilities
the submission finds very few false positives
code in the submission is easy to read and will be easy to maintain
documentation is written clearly, highlighting the impact of the issue it finds and is written without grammatical or other errors. The code samples clearly show the vulnerability
the submission includes tests, change note etc.

Please note that these are guidelines, not rules. Since we have a lot of different types of submissions, the guidelines might vary for each submission.

Happy hacking!

sylwia-budzynska · 2024-07-01T11:22:12Z

@github/codeql-python 👋 This submission is ready for review.

RasmusWL

Thanks for your contribution 💪

I've made a few suggestions. If you accept the one for the alert message, you will also need to update the .expected files.

I had a brief look to understand what js2py.disable_pyimport() actually does (src), and found an interesting comment highlighting that even when using this, you should still consider using the library with untrusted input as insecure. I think this is important to highlight in the qhelp.

RasmusWL · 2024-07-02T08:36:06Z

python/ql/src/experimental/Security/CWE-094/Js2Py.qhelp

+        </p>
+    </overview>
+    <recommendation>
+        <p> This vulnerability can be prevented either by preventing an untrusted user input to lfow


Suggested change

<p> This vulnerability can be prevented either by preventing an untrusted user input to lfow

<p> This vulnerability can be prevented either by preventing an untrusted user input to flow

RasmusWL · 2024-07-02T08:36:16Z

python/ql/src/experimental/Security/CWE-094/Js2Py.qhelp

+    </overview>
+    <recommendation>
+        <p> This vulnerability can be prevented either by preventing an untrusted user input to lfow
+            to an <code>eval_js</code> call. Or, thee impact of this vulnerability can be


Suggested change

to an <code>eval_js</code> call. Or, thee impact of this vulnerability can be

to an <code>eval_js</code> call. Or, the impact of this vulnerability can be

RasmusWL · 2024-07-02T08:36:44Z

python/ql/src/experimental/Security/CWE-094/Js2Py.ql

+ * @description Passing user supplied arguments to a Javascript to Python translation engine such as Js2Py can lead to remote code execution.
+ * @severity high
+ * @kind path-problem
+ * @id python/js2py-rce


convention is to use py/ as the prefix

Suggested change

* @id python/js2py-rce

* @id py/js2py-rce

RasmusWL · 2024-07-02T08:43:50Z

python/ql/src/experimental/Security/CWE-094/Js2Py.ql

+private API::Node js2py() { result = API::moduleImport("js2py") }
+
+private API::Node js2evaljs() { result = js2py().getMember(["eval_js", "eval_js6"]) }
+
+private class DisablePyImportCall extends API::CallNode, DataFlow::CallCfgNode {
+  DisablePyImportCall() { this = js2py().getMember("disable_pyimport").getACall() }
+}
+
+class EvalJsCall extends API::CallNode, DataFlow::CallCfgNode {
+  EvalJsCall() { this = js2evaljs().getACall() }
+}
+
+module Js2PyFlowConfiguration implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
+
+  predicate isSink(DataFlow::Node node) { exists(EvalJsCall e | e.getArg(_) = node) }
+}
+
+module Js2PyFlow = TaintTracking::Global<Js2PyFlowConfiguration>;
+
+import Js2PyFlow::PathGraph
+
+from Js2PyFlow::PathNode source, Js2PyFlow::PathNode sink
+where
+  Js2PyFlow::flowPath(source, sink) and
+  not exists(DisablePyImportCall c)


I think the following looks cleaner, although it does the same.

NOTE: I've also added support for EvalJs method call.

Suggested change

private API::Node js2py() { result = API::moduleImport("js2py") }

private API::Node js2evaljs() { result = js2py().getMember(["eval_js", "eval_js6"]) }

private class DisablePyImportCall extends API::CallNode, DataFlow::CallCfgNode {

DisablePyImportCall() { this = js2py().getMember("disable_pyimport").getACall() }

}

class EvalJsCall extends API::CallNode, DataFlow::CallCfgNode {

EvalJsCall() { this = js2evaljs().getACall() }

}

module Js2PyFlowConfiguration implements DataFlow::ConfigSig {

predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }

predicate isSink(DataFlow::Node node) { exists(EvalJsCall e | e.getArg(_) = node) }

}

module Js2PyFlow = TaintTracking::Global<Js2PyFlowConfiguration>;

import Js2PyFlow::PathGraph

from Js2PyFlow::PathNode source, Js2PyFlow::PathNode sink

where

Js2PyFlow::flowPath(source, sink) and

not exists(DisablePyImportCall c)

module Js2PyFlowConfiguration implements DataFlow::ConfigSig {

predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }

predicate isSink(DataFlow::Node node) {

API::moduleImport("js2py").getMember(["eval_js", "eval_js6", "EvalJs"]).getACall().getArg(_) = node

}

}

module Js2PyFlow = TaintTracking::Global<Js2PyFlowConfiguration>;

import Js2PyFlow::PathGraph

from Js2PyFlow::PathNode source, Js2PyFlow::PathNode sink

where

Js2PyFlow::flowPath(source, sink) and

not exists(API::moduleImport("js2py").getMember("disable_pyimport").getACall())

RasmusWL · 2024-07-02T08:44:17Z

python/ql/src/experimental/Security/CWE-094/Js2Py.ql

+where
+  Js2PyFlow::flowPath(source, sink) and
+  not exists(DisablePyImportCall c)
+select sink, source, sink, "This can lead to arbitrary code execution"


This alert message is more in-line with our other alert messages

Suggested change

select sink, source, sink, "This can lead to arbitrary code execution"

select sink, source, sink, "This input to Js2Py depends on a $@.", source.getNode(),

"user-provided value"

RasmusWL · 2024-07-02T08:53:46Z

python/ql/src/experimental/Security/CWE-094/Js2Py.qhelp

+    <recommendation>
+        <p> This vulnerability can be prevented either by preventing an untrusted user input to lfow
+            to an <code>eval_js</code> call. Or, thee impact of this vulnerability can be
+            significantly reduced by disabling imports from the interepreted code. </p>


Suggested change

significantly reduced by disabling imports from the interepreted code. </p>

significantly reduced by disabling imports from the interepreted code (note that in a <a href="https://github.com/PiotrDabkowski/Js2Py/issues/45#issuecomment-258724406">comment</a> the author of the library highlights that Js2Py is still insecure with this option).</p>

github-actions · 2024-07-02T09:01:10Z

QHelp previews:

python/ql/src/experimental/Security/CWE-094/Js2Py.qhelp

JavaScript code execution.

Passing untrusted inputs to a JavaScript interpreter like `Js2Py` can lead to arbitrary code execution.

Recommendation

This vulnerability can be prevented either by preventing an untrusted user input to flow to an eval_js call. Or, the impact of this vulnerability can be significantly reduced by disabling imports from the interepreted code (note that in a comment the author of the library highlights that Js2Py is still insecure with this option).

Example

In the example below, the Javascript code being evaluated is controlled by the user and hence leads to arbitrary code execution.

@bp.route("/bad")
def bad():
    jk = flask.request.form["jk"]
    jk = eval_js(f"{jk} f()")

This can be fixed by disabling imports before evaluating the user passed buffer.

@bp.route("/good")
def good():
    # disable python imports to prevent execution of malicious code 
    js2py.disable_pyimport()
    jk = flask.request.form["jk"]
    jk = eval_js(f"{jk} f()")

python/ql/src/experimental/Security/CWE-094/Js2Py.ql

ghost · 2024-07-03T13:35:07Z

@RasmusWL Changes done!

Js2Py is a Javascript to Python translation library written in Python. It allows users to invoke JavaScript code directly from Python. The Js2Py interpreter by default exposes the entire standard library to it's users. This can lead to security issues if a malicious input were directly. This PR includes a CodeQL query along with a qhelp and testcases to detect cases where an untrusted input flows to an Js2Py eval call. This query successfully detects CVE-2023-0297 in `pyload/pyload`along with it's fix. The databases can be downloaded from the links bellow. ``` https://file.io/qrMEjSJJoTq1 https://filetransfer.io/data-package/a02eab7V#link ```

RasmusWL · 2024-07-04T12:13:00Z

I would strongly advise to just push additional commits to your branch/PR next time. By doing so, it becomes very easy for me to see the changes you made after reading my review. With the force push, I have to remember what the code looked like before and try to figure out what has changed 🤔

ghost · 2024-07-04T12:15:22Z

@RasmusWL Sorry about that. I made a couple of mistakes while merging so I had to force push again.

RasmusWL

Thanks 👍

Before promoting this, I think we should expand tests a bit to also include js2py.disable_pyimport()

ghost self-requested a review as a code owner June 15, 2024 15:23

github-actions bot added documentation Python labels Jun 15, 2024

ghost mentioned this pull request Jun 15, 2024

Python : Arbitrary code execution due to Js2Py github/securitylab#832

Closed

2 tasks

ghsecuritylab marked this pull request as draft June 15, 2024 16:02

ghsecuritylab marked this pull request as ready for review July 1, 2024 11:22

RasmusWL added the external-contribution label Jul 1, 2024

RasmusWL self-assigned this Jul 2, 2024

RasmusWL requested changes Jul 2, 2024

View reviewed changes

github-advanced-security AI found potential problems Jul 2, 2024

View reviewed changes

python/ql/src/experimental/Security/CWE-094/Js2Py.ql Fixed Show fixed Hide fixed

python/ql/src/experimental/Security/CWE-094/Js2Py.ql Fixed Show fixed Hide fixed

Porcupiney Hairs and others added 3 commits July 3, 2024 19:06

Python: Fixup qhelp

8d1113c

Python: Update query metadata

0a32f9f

Python: Fix .expected

5ecde38

RasmusWL approved these changes Jul 11, 2024

View reviewed changes

RasmusWL merged commit f41d2a8 into github:main Jul 11, 2024

ghost deleted the js2py branch July 11, 2024 15:47

	<p> This vulnerability can be prevented either by preventing an untrusted user input to lfow
	<p> This vulnerability can be prevented either by preventing an untrusted user input to flow

	to an <code>eval_js</code> call. Or, thee impact of this vulnerability can be
	to an <code>eval_js</code> call. Or, the impact of this vulnerability can be

	select sink, source, sink, "This can lead to arbitrary code execution"
	select sink, source, sink, "This input to Js2Py depends on a $@.", source.getNode(),
	"user-provided value"

	significantly reduced by disabling imports from the interepreted code. </p>
	significantly reduced by disabling imports from the interepreted code (note that in a <a href="https://github.com/PiotrDabkowski/Js2Py/issues/45#issuecomment-258724406">comment</a> the author of the library highlights that Js2Py is still insecure with this option).</p>

Conversation

ghost commented Jun 15, 2024

Uh oh!

ghsecuritylab commented Jun 15, 2024

Uh oh!

sylwia-budzynska commented Jul 1, 2024

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

RasmusWL Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

RasmusWL Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

RasmusWL Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

RasmusWL Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

RasmusWL Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

RasmusWL Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JavaScript code execution.

Recommendation

Example

Uh oh!

Uh oh!

Uh oh!

ghost commented Jul 3, 2024

Uh oh!

RasmusWL commented Jul 4, 2024

Uh oh!

ghost commented Jul 4, 2024

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Jul 2, 2024 •

edited

Loading