Skip to content
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

CVE-2022-23923 #57

Open
eric-hemasystems opened this issue May 24, 2022 · 2 comments
Open

CVE-2022-23923 #57

eric-hemasystems opened this issue May 24, 2022 · 2 comments

Comments

@eric-hemasystems
Copy link

eric-hemasystems commented May 24, 2022

I was recently notified by Github regarding CVE-2022-23923. The actual description of the issue is a bit odd so wanted to get clarification on its impact. It is described with:

All versions of package jailed are vulnerable to Sandbox Bypass via an exported alert() method which can access the main application. Exported methods are stored in the application.remote object.

This seems to imply the issue is only present when a remote method named alert is exported to the jailed script. The issue seems to be imported from the Sync vulnerability database and that actually includes a POC of the vulnerability. My understanding of the POC leads me to believe the description of the vulnerability is not accurate. It feels like the person who wrote the description is not the same as the person who wrote the POC and the description writer did not understand the POC. The alert was just used to present the exfiltrated data for demonstration purposes and has nothing to do really with the vulnerability.

My understanding of the POC is it is a variation on the known vulnerability with jailed when running on Node.js and documented on the README. It is getting a Function constructor on an object provided by the main context from within the context created by Node.js's vm.runInNewContext. Then using that to eval code to get access to require via process.mainModule. From there it can do any number of things (in the POC's case it is reading from the filesystem).

What makes this POC different from #33 is that it is not using objects passed to the sandbox to get access to this function constructor but instead using this.constructor which I am thinking is the constructor for the global object that Node provides the new context? If this is correct I don't think the proposed fix to @gpascualg did in #37 would be sufficient as the Function constructor is not gained via one of the exposed objects passed to the new vm context.

I'm posting this issue hoping to get feedback from others to confirm my understand which is:

This is just a variation on #33 and therefore represents no more risk than was already documented in the README. jailed on Node.js continues to be broken (only now the proposed fix may be insufficient) but jailed on the web is still valid as it's isolation is handled entirely differently and the constructs used in the POC (process.mainModule and require) are not present for JS running in a web browser.

@marke-apexit
Copy link

Is this post suggesting this security issue is only relevant if you export alert() to 'jailed?' And the security vulnerability can be managed by not allowing user-defined exports, only user-defined code?

Could 'jailed' fix the CVE by scrubbing the exports and failing if alert() is in the exports?

@eric-hemasystems
Copy link
Author

Is this post suggesting this security issue is only relevant if you export alert() to 'jailed?'

No. See above where it says:

The alert was just used to present the exfiltrated data for demonstration purposes and has nothing to do really with the vulnerability.


And the security vulnerability can be managed by not allowing user-defined exports, only user-defined code?

No. See above where it says:

I don't think the proposed fix to @gpascualg did in #37 would be sufficient as the Function constructor is not gained via one of the exposed objects passed to the new vm context.


Could 'jailed' fix the CVE by scrubbing the exports and failing if alert() is in the exports?

Since the sandbox escape uses a built-in Node object, this.constructor, and therefore there is nothing we are passing in I don't think there is a way to prevent the escape. Node.js would have to be changed itself.


Final result is:

  • The isolation provided by jailed when used with Node.js is broken.
  • This has been true for a while and was documented on the README. The only thing CVE-2022-23923 did was demonstrate a variant of the original that makes the previous attempts to fix it not workable.
  • jailed is still useful and a sandbox secure when executed in the browser

If you need to run JavaScript server-side I might suggest try using a different JS engine such as Spidermonkey compiled to wasm run via wasmer. You can either just use the wasmer bin directly or use library bindings to it. Here is a module I made in Ruby that allows me to safely execute JS code server-side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants