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

produceSecureBindings() doesn't seem to work #134

Closed
asilism opened this issue Jan 27, 2023 · 3 comments · Fixed by #136
Closed

produceSecureBindings() doesn't seem to work #134

asilism opened this issue Jan 27, 2023 · 3 comments · Fixed by #136

Comments

@asilism
Copy link

asilism commented Jan 27, 2023

Hi, recently I found that sandbox can load remote script by using load() function.
It can be secure problem in my project , so I checked option of sandbox.

I found allowLoadFunctions args is "false" by default.

I debugged NashornSandboxImpl, and there is ambiguous part of produceSecureBindings()

private void produceSecureBindings() {
		try {
			final StringBuilder sb = new StringBuilder();
			cached = scriptEngine.getBindings(ScriptContext.ENGINE_SCOPE);
			sanitizeBindings(cached);
			if (!allowExitFunctions) {
				sb.append("var quit=function(){};var exit=function(){};");
			}
			if (!allowPrintFunctions) {
				sb.append("var print=function(){};var echo = function(){};");
			}
			if (!allowReadFunctions) {
				sb.append("var readFully=function(){};").append("var readLine=function(){};");
			}
			if (!allowLoadFunctions) {
				sb.append("var load=function(){};var loadWithNewGlobal=function(){};");
			}
			if (!allowGlobalsObjects) {
				// Max 22nd of Feb 2018: I don't think these are strictly necessary since they
				// are only available in scripting mode
				sb.append("var $ARG=null;var $ENV=null;var $EXEC=null;");
				sb.append("var $OPTIONS=null;var $OUT=null;var $ERR=null;var $EXIT=null;");
			}
			scriptEngine.eval(sb.toString());

			resetEngineBindings();

			engineAsserted.set(true);

		} catch (final Exception e) {
			throw new RuntimeException(e);
		}
	}

It looks like doing method overide such as quit/load/print ... . but after that, call resetEngineBindings() again.
Doesn't it mean that all overrides are initialized?

I don't know if this is the real cause, but in my current sandbox it is still possible to execute load() function.
Is there something I'm missing?

mxro added a commit that referenced this issue Jan 29, 2023
@mxro
Copy link
Collaborator

mxro commented Jan 29, 2023

Thank you for raising this issue!

I tried to reproduce it in a unit test:

public class TestIssue134 {

	@Test
	public void test() throws ScriptCPUAbuseException, ScriptException {
		NashornSandbox sandbox = NashornSandboxes.create();

		sandbox.eval("load('classpath:TestIssue134.js')");
		sandbox.eval("load('somethingwrong')");
	}

}

4f3c8fa

It seems like the load function is just the overriden dummy.

When running the same test with the enabled load function as follows:

	@Test
	public void test() throws ScriptCPUAbuseException, ScriptException {
		NashornSandbox sandbox = NashornSandboxes.create();
		sandbox.allowLoadFunctions(true);
		sandbox.eval("load('classpath:TestIssue134.js')");
		sandbox.eval("load('somethingwrong')");
	}

We will get the exception 'I should never be called' from running the script.

Could you help me refine the test so it captures what you have encountered? Thank you!

@asilism
Copy link
Author

asilism commented Jan 31, 2023

@Test
	public void test() throws ScriptCPUAbuseException, ScriptException {
		NashornSandbox sandbox = NashornSandboxes.create();
		// create context for Engine scope binding
		ScriptContext context = new SimpleScriptContext();
		// actually, there is more source for binding
		// blah,blah, blah....
		sandbox.eval("load('classpath:TestIssue134.js')", context);
		sandbox.eval("load('somethingwrong')", context);
	}

I run eval() with new sciprt context, but that is not secure context.
Because the context isn't sanitize, and also allow/block function is based on context.
Is there any way to get sanitized context?
Now, I'm confused if this is a glitch or if I'm using sandbox incorrectly. 😂

thanks to every reply.

@mxro
Copy link
Collaborator

mxro commented Apr 6, 2023

I think technically the Sandbox worked as expected here.

However, I think your example shows that it is very easy to accidentally expose scripts. Therefore, I released a new version 0.3.0 that attempts to make it safer working with script contexts.

It will now work as follows:

        @Test
	public void test() throws ScriptCPUAbuseException, ScriptException {
		NashornSandbox sandbox = NashornSandboxes.create();
		// create context for Engine scope binding
		ScriptContext context = sandbox.createScriptContext();
		// actually, there is more source for binding
		// blah,blah, blah....
		sandbox.eval("load('classpath:TestIssue134.js')", context);
		sandbox.eval("load('somethingwrong')", context);
	}

Please let me know if this doesn't resolve your issue!

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

Successfully merging a pull request may close this issue.

2 participants