From 4f3c8faef7e40260dd4312cd12fccbb7ac3dd26d Mon Sep 17 00:00:00 2001 From: Max Rohde Date: Mon, 30 Jan 2023 09:58:52 +1100 Subject: [PATCH 1/3] Adding unit test for #134 --- .project | 4 ++-- .../delight/nashornsandbox/TestIssue134.java | 22 +++++++++++++++++++ src/test/resources/TestIssue134.js | 2 ++ 3 files changed, 26 insertions(+), 2 deletions(-) create mode 100644 src/test/java/delight/nashornsandbox/TestIssue134.java create mode 100644 src/test/resources/TestIssue134.js diff --git a/.project b/.project index db1d7d8..1d35314 100644 --- a/.project +++ b/.project @@ -28,12 +28,12 @@ - 1602570532382 + 1675032386265 30 org.eclipse.core.resources.regexFilterMatcher - node_modules|.git|__CREATED_BY_JAVA_LANGUAGE_SERVER__ + node_modules|\.git|__CREATED_BY_JAVA_LANGUAGE_SERVER__ diff --git a/src/test/java/delight/nashornsandbox/TestIssue134.java b/src/test/java/delight/nashornsandbox/TestIssue134.java new file mode 100644 index 0000000..9dedb0c --- /dev/null +++ b/src/test/java/delight/nashornsandbox/TestIssue134.java @@ -0,0 +1,22 @@ +package delight.nashornsandbox; + +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +import javax.script.ScriptException; + +import org.junit.Test; + +import delight.nashornsandbox.exceptions.ScriptCPUAbuseException; + +public class TestIssue134 { + + @Test + public void test() throws ScriptCPUAbuseException, ScriptException { + NashornSandbox sandbox = NashornSandboxes.create(); + + sandbox.eval("load('classpath:TestIssue134.js')"); + sandbox.eval("load('somethingwrong')"); + } + +} diff --git a/src/test/resources/TestIssue134.js b/src/test/resources/TestIssue134.js new file mode 100644 index 0000000..12e9231 --- /dev/null +++ b/src/test/resources/TestIssue134.js @@ -0,0 +1,2 @@ +console.log('loaded'); +throw new Error('I should never be called!'); \ No newline at end of file From 074378b4ca8aefa3ee70a18ef824455cb0a1f78d Mon Sep 17 00:00:00 2001 From: Max Rohde <1448524+mxro@users.noreply.github.com> Date: Fri, 7 Apr 2023 06:36:22 +1000 Subject: [PATCH 2/3] Introducing wrapper for script context to avoid the danger of exposing vulernable script contexts --- pom.xml | 2 +- .../nashornsandbox/NashornSandbox.java | 14 +++-- .../nashornsandbox/SandboxScriptContext.java | 8 +++ .../internal/NashornSandboxImpl.java | 52 ++++++++++++++----- .../TestEvalWithScriptContext.java | 16 +++--- ...tEvalWithScriptContextWithNewBindings.java | 12 ++--- ...estEvalWithScriptContextWithVariables.java | 8 +-- .../java/delight/nashornsandbox/TestExit.java | 5 +- .../delight/nashornsandbox/TestIssue134.java | 10 ++-- src/test/resources/TestIssue134.js | 2 +- 10 files changed, 83 insertions(+), 46 deletions(-) create mode 100644 src/main/java/delight/nashornsandbox/SandboxScriptContext.java diff --git a/pom.xml b/pom.xml index 79f4697..cf4568d 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.javadelight delight-nashorn-sandbox - 0.2.5 + 0.3.0 A safe sandbox to execute JavaScript code from Nashorn. https://github.com/javadelight/delight-nashorn-sandbox diff --git a/src/main/java/delight/nashornsandbox/NashornSandbox.java b/src/main/java/delight/nashornsandbox/NashornSandbox.java index 7283924..4b1fd77 100644 --- a/src/main/java/delight/nashornsandbox/NashornSandbox.java +++ b/src/main/java/delight/nashornsandbox/NashornSandbox.java @@ -133,7 +133,7 @@ public interface NashornSandbox { * @throws ScriptException when script syntax error occurs * @see #setMaxCPUTime(long) */ - Object eval(String js,Bindings bindings) throws ScriptCPUAbuseException, ScriptException; + Object eval(String js, Bindings bindings) throws ScriptCPUAbuseException, ScriptException; /** * Evaluates the JavaScript string for a given script context @@ -145,7 +145,7 @@ public interface NashornSandbox { * @throws ScriptException when script syntax error occurs * @see #setMaxCPUTime(long) */ - Object eval(String js, ScriptContext scriptContext) throws ScriptCPUAbuseException, ScriptException; + Object eval(String js, SandboxScriptContext scriptContext) throws ScriptCPUAbuseException, ScriptException; /** @@ -159,7 +159,7 @@ public interface NashornSandbox { * @throws ScriptException when script syntax error occurs * @see #setMaxCPUTime(long) */ - Object eval(String js, ScriptContext scriptContext,Bindings bindings) throws ScriptCPUAbuseException, ScriptException; + Object eval(String js, SandboxScriptContext scriptContext,Bindings bindings) throws ScriptCPUAbuseException, ScriptException; /** * Obtains the value of the specified JavaScript variable. @@ -310,5 +310,11 @@ public interface NashornSandbox { Object eval(CompiledScript compiledScript) throws ScriptCPUAbuseException, ScriptException; Object eval(CompiledScript compiledScript, Bindings bindings) throws ScriptCPUAbuseException, ScriptException; Object eval(CompiledScript compiledScript, ScriptContext scriptContext) throws ScriptCPUAbuseException, ScriptException; - Object eval(CompiledScript compiledScript, ScriptContext scriptContext,Bindings bindings) throws ScriptCPUAbuseException, ScriptException; + Object eval(CompiledScript compiledScript, ScriptContext scriptContext, Bindings bindings) throws ScriptCPUAbuseException, ScriptException; + + /** + * Create a new secured script context + */ +SandboxScriptContext createScriptContext(); + } diff --git a/src/main/java/delight/nashornsandbox/SandboxScriptContext.java b/src/main/java/delight/nashornsandbox/SandboxScriptContext.java new file mode 100644 index 0000000..7b0d88a --- /dev/null +++ b/src/main/java/delight/nashornsandbox/SandboxScriptContext.java @@ -0,0 +1,8 @@ +package delight.nashornsandbox; + +import javax.script.ScriptContext; + +public interface SandboxScriptContext { + + public ScriptContext getContext(); +} \ No newline at end of file diff --git a/src/main/java/delight/nashornsandbox/internal/NashornSandboxImpl.java b/src/main/java/delight/nashornsandbox/internal/NashornSandboxImpl.java index 40e61f7..0f0a63f 100644 --- a/src/main/java/delight/nashornsandbox/internal/NashornSandboxImpl.java +++ b/src/main/java/delight/nashornsandbox/internal/NashornSandboxImpl.java @@ -1,6 +1,7 @@ package delight.nashornsandbox.internal; import delight.nashornsandbox.NashornSandbox; +import delight.nashornsandbox.SandboxScriptContext; import delight.nashornsandbox.SecuredJsCache; import delight.nashornsandbox.exceptions.ScriptCPUAbuseException; import delight.nashornsandbox.exceptions.ScriptMemoryAbuseException; @@ -14,6 +15,8 @@ import javax.script.ScriptContext; import javax.script.ScriptEngine; import javax.script.ScriptException; +import javax.script.SimpleScriptContext; + import java.io.Writer; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; @@ -125,9 +128,9 @@ public ScriptEngine createNashornScriptEngineFactory(String ... params) { private synchronized void assertScriptEngine() { try { if (!engineAsserted.get()) { - produceSecureBindings(); + produceSecureBindings(scriptEngine.getContext()); } else if (!engineBindingUnchanged()) { - resetEngineBindings(); + resetEngineBindings(scriptEngine.getContext()); } } catch (final Exception e) { throw new RuntimeException(e); @@ -146,10 +149,10 @@ private boolean engineBindingUnchanged() { return true; } - private void produceSecureBindings() { + private void produceSecureBindings(ScriptContext context) { try { final StringBuilder sb = new StringBuilder(); - cached = scriptEngine.getBindings(ScriptContext.ENGINE_SCOPE); + cached = context.getBindings(ScriptContext.ENGINE_SCOPE); sanitizeBindings(cached); if (!allowExitFunctions) { sb.append("var quit=function(){};var exit=function(){};"); @@ -169,9 +172,9 @@ private void produceSecureBindings() { 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()); + scriptEngine.eval(sb.toString(), context); - resetEngineBindings(); + resetEngineBindings(context); engineAsserted.set(true); @@ -180,11 +183,11 @@ private void produceSecureBindings() { } } - protected void resetEngineBindings() { + protected void resetEngineBindings(ScriptContext context) { final Bindings bindings = createBindings(); sanitizeBindings(bindings); bindings.putAll(cached); - scriptEngine.setBindings(bindings, ScriptContext.ENGINE_SCOPE); + context.setBindings(bindings, ScriptContext.ENGINE_SCOPE); } protected void sanitizeBindings(Bindings bindings) { @@ -206,7 +209,24 @@ protected void sanitizeBindings(Bindings bindings) { } } + + + @Override + public SandboxScriptContext createScriptContext() { + ScriptContext context = new SimpleScriptContext(); + produceSecureBindings(context); + return new SandboxScriptContext() { + + @Override + public ScriptContext getContext() { + return context; + } + + }; + } + + @Override public Object eval(final String js) throws ScriptCPUAbuseException, ScriptException { return eval(js, null, null); } @@ -217,12 +237,12 @@ public Object eval(String js, Bindings bindings) throws ScriptCPUAbuseException, } @Override - public Object eval(String js, ScriptContext scriptContext) throws ScriptCPUAbuseException, ScriptException { + public Object eval(String js, SandboxScriptContext scriptContext) throws ScriptCPUAbuseException, ScriptException { return eval(js, scriptContext, null); } @Override - public Object eval(final String js, final ScriptContext scriptContext, final Bindings bindings) + public Object eval(final String js, final SandboxScriptContext scriptContext, final Bindings bindings) throws ScriptCPUAbuseException, ScriptException { assertScriptEngine(); final JsSanitizer sanitizer = getSanitizer(); @@ -233,13 +253,17 @@ public Object eval(final String js, final ScriptContext scriptContext, final Bin if (scriptContext == null) { securedJs = blockAccessToEngine + sanitizer.secureJs(js); } else { - // Unfortunately, blocking access to the engine property inteferes with setting - // a script context - // needs further investigation + // Unfortunately, blocking access to the engine property interferes with setting + // a script context needs further investigation securedJs = sanitizer.secureJs(js); } final Bindings securedBindings = secureBindings(bindings); - EvaluateOperation op = new EvaluateOperation(securedJs, scriptContext, securedBindings); + EvaluateOperation op; + if (scriptContext != null) { + op = new EvaluateOperation(securedJs, scriptContext.getContext(), securedBindings); + } else { + op = new EvaluateOperation(securedJs, null, securedBindings); + } return executeSandboxedOperation(op); } diff --git a/src/test/java/delight/nashornsandbox/TestEvalWithScriptContext.java b/src/test/java/delight/nashornsandbox/TestEvalWithScriptContext.java index adf2e20..de89d25 100644 --- a/src/test/java/delight/nashornsandbox/TestEvalWithScriptContext.java +++ b/src/test/java/delight/nashornsandbox/TestEvalWithScriptContext.java @@ -17,12 +17,12 @@ public class TestEvalWithScriptContext { @Test public void test() throws ScriptCPUAbuseException, ScriptException { final NashornSandbox sandbox = NashornSandboxes.create(); - ScriptContext newContext1 = new SimpleScriptContext(); - Bindings engineScope1 = newContext1.getBindings(ScriptContext.ENGINE_SCOPE); + SandboxScriptContext newContext1 = sandbox.createScriptContext(); + Bindings engineScope1 = newContext1.getContext().getBindings(ScriptContext.ENGINE_SCOPE); engineScope1.put("y", 2); - ScriptContext newContext2 = new SimpleScriptContext(); - Bindings engineScope2 = newContext2.getBindings(ScriptContext.ENGINE_SCOPE); + SandboxScriptContext newContext2 = sandbox.createScriptContext(); + Bindings engineScope2 = newContext2.getContext().getBindings(ScriptContext.ENGINE_SCOPE); engineScope2.put("y", 4); final Object res1 = sandbox.eval("function cal() {var x = y + 1; return x;} cal();", newContext1); @@ -56,12 +56,12 @@ public void testWithCPUAndMemory() throws ScriptCPUAbuseException, ScriptExcepti sandbox.setMaxCPUTime(100); sandbox.setMaxMemory(1000 * 1024); sandbox.setExecutor(Executors.newSingleThreadExecutor()); - ScriptContext newContext1 = new SimpleScriptContext(); - Bindings engineScope1 = newContext1.getBindings(ScriptContext.ENGINE_SCOPE); + SandboxScriptContext newContext1 = sandbox.createScriptContext(); + Bindings engineScope1 = newContext1.getContext().getBindings(ScriptContext.ENGINE_SCOPE); engineScope1.put("y", 2); - ScriptContext newContext2 = new SimpleScriptContext(); - Bindings engineScope2 = newContext2.getBindings(ScriptContext.ENGINE_SCOPE); + SandboxScriptContext newContext2 = sandbox.createScriptContext(); + Bindings engineScope2 = newContext2.getContext().getBindings(ScriptContext.ENGINE_SCOPE); engineScope2.put("y", 4); final Object res1 = sandbox.eval("function cal() {var x = y + 1; return x;} cal();", newContext1); diff --git a/src/test/java/delight/nashornsandbox/TestEvalWithScriptContextWithNewBindings.java b/src/test/java/delight/nashornsandbox/TestEvalWithScriptContextWithNewBindings.java index 946c3f2..b403d8f 100644 --- a/src/test/java/delight/nashornsandbox/TestEvalWithScriptContextWithNewBindings.java +++ b/src/test/java/delight/nashornsandbox/TestEvalWithScriptContextWithNewBindings.java @@ -37,11 +37,11 @@ public void testWithNewSimpleBindings() throws ScriptCPUAbuseException, ScriptEx @Test public void testWithNewBindingsScriptContext() throws ScriptCPUAbuseException, ScriptException { final NashornSandbox sandbox = NashornSandboxes.create(); - ScriptContext newContext = new SimpleScriptContext(); + SandboxScriptContext newContext = sandbox.createScriptContext(); // Create new binding to override the ECMAScript Global properties Bindings newBinding = sandbox.createBindings(); newBinding.put("Date", "2112018"); - newContext.setBindings(newBinding, ScriptContext.ENGINE_SCOPE); + newContext.getContext().setBindings(newBinding, ScriptContext.ENGINE_SCOPE); final Object res = sandbox.eval("function method() { return parseInt(Date);} method();", newContext); Assert.assertTrue(res.equals(Double.valueOf("2112018.0")) || res.equals(Integer.valueOf(2112018))); @@ -50,14 +50,12 @@ public void testWithNewBindingsScriptContext() throws ScriptCPUAbuseException, S @Test public void testWithExistingBindings() throws ScriptCPUAbuseException, ScriptException { final NashornSandbox sandbox = NashornSandboxes.create(); - ScriptContext newContext = new SimpleScriptContext(); - Bindings newBinding = newContext.getBindings(ScriptContext.ENGINE_SCOPE); - // This will not be updated by using existing bindings, since Date is a - // ECMAScript "global" properties and it is being in ENGINE_SCOPE + SandboxScriptContext newContext = sandbox.createScriptContext(); + Bindings newBinding = newContext.getContext().getBindings(ScriptContext.ENGINE_SCOPE); newBinding.put("Date", "2112018"); final Object res = sandbox.eval("function method() { return parseInt(Date);} method();", newContext); - Assert.assertTrue(Double.isNaN((Double)res)); + Assert.assertTrue(res.equals(2112018)); } diff --git a/src/test/java/delight/nashornsandbox/TestEvalWithScriptContextWithVariables.java b/src/test/java/delight/nashornsandbox/TestEvalWithScriptContextWithVariables.java index 9c79d35..9b8a626 100644 --- a/src/test/java/delight/nashornsandbox/TestEvalWithScriptContextWithVariables.java +++ b/src/test/java/delight/nashornsandbox/TestEvalWithScriptContextWithVariables.java @@ -16,8 +16,8 @@ public class TestEvalWithScriptContextWithVariables { @Test public void test() throws ScriptCPUAbuseException, ScriptException { final NashornSandbox sandbox = NashornSandboxes.create(); - ScriptContext newContext1 = new SimpleScriptContext(); - ScriptContext newContext2 = new SimpleScriptContext(); + SandboxScriptContext newContext1 = sandbox.createScriptContext(); + SandboxScriptContext newContext2 = sandbox.createScriptContext(); sandbox.eval("function cal() {var x = 1; return x;}", newContext1); sandbox.eval("function cal() {var x = 2; return x;}", newContext2); @@ -36,8 +36,8 @@ public void testWithCPUAndMemory() throws ScriptCPUAbuseException, ScriptExcepti sandbox.setMaxCPUTime(100); sandbox.setMaxMemory(1000 * 1024); sandbox.setExecutor(Executors.newSingleThreadExecutor()); - ScriptContext newContext1 = new SimpleScriptContext(); - ScriptContext newContext2 = new SimpleScriptContext(); + SandboxScriptContext newContext1 = sandbox.createScriptContext(); + SandboxScriptContext newContext2 = sandbox.createScriptContext(); sandbox.eval("function cal() {var x = 1; return x;}", newContext1); sandbox.eval("function cal() {var x = 2; return x;}", newContext2); diff --git a/src/test/java/delight/nashornsandbox/TestExit.java b/src/test/java/delight/nashornsandbox/TestExit.java index 454e352..ef93553 100644 --- a/src/test/java/delight/nashornsandbox/TestExit.java +++ b/src/test/java/delight/nashornsandbox/TestExit.java @@ -34,11 +34,10 @@ public void testQuitWithBindings() throws ScriptCPUAbuseException, ScriptExcepti - @Test - @Ignore("This will fail as there is no confirmation on Script Contexts") + @Test(expected = javax.script.ScriptException.class) public void testQuitWithScriptContext() throws ScriptCPUAbuseException, ScriptException { final NashornSandbox sandbox = NashornSandboxes.create(); - sandbox.eval("quit();", new SimpleScriptContext()); + sandbox.eval("quit();", sandbox.createScriptContext()); } diff --git a/src/test/java/delight/nashornsandbox/TestIssue134.java b/src/test/java/delight/nashornsandbox/TestIssue134.java index 9dedb0c..320fc0b 100644 --- a/src/test/java/delight/nashornsandbox/TestIssue134.java +++ b/src/test/java/delight/nashornsandbox/TestIssue134.java @@ -3,7 +3,9 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import javax.script.ScriptContext; import javax.script.ScriptException; +import javax.script.SimpleScriptContext; import org.junit.Test; @@ -11,12 +13,12 @@ public class TestIssue134 { - @Test + @Test(expected = javax.script.ScriptException.class) public void test() throws ScriptCPUAbuseException, ScriptException { NashornSandbox sandbox = NashornSandboxes.create(); - - sandbox.eval("load('classpath:TestIssue134.js')"); - sandbox.eval("load('somethingwrong')"); + SandboxScriptContext context = sandbox.createScriptContext(); + sandbox.eval("load('classpath:TestIssue134.js')", context); + sandbox.eval("load('somethingwrong')", context); } } diff --git a/src/test/resources/TestIssue134.js b/src/test/resources/TestIssue134.js index 12e9231..7cb4cee 100644 --- a/src/test/resources/TestIssue134.js +++ b/src/test/resources/TestIssue134.js @@ -1,2 +1,2 @@ -console.log('loaded'); +print('loaded'); throw new Error('I should never be called!'); \ No newline at end of file From 264fc43fcdbfc298a2ccc6118e5cd95594f36603 Mon Sep 17 00:00:00 2001 From: Max Rohde <1448524+mxro@users.noreply.github.com> Date: Fri, 7 Apr 2023 06:38:15 +1000 Subject: [PATCH 3/3] Adding entry in readme for new version --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 0b4fa14..651e147 100644 --- a/README.md +++ b/README.md @@ -110,6 +110,7 @@ for JS evaluation and better handling of monitoring for threads for possible CPU ## Version History +- 0.3.0: Creating a wrapper for Script Context to be passed to eval to avoid accidental exposure. Resolves [Issue #134](https://github.com/javadelight/delight-nashorn-sandbox/issues/134) - 0.2.5: Support for pre-compiled scripts ([PR #119](https://github.com/javadelight/delight-nashorn-sandbox/pull/119) by [geoffreyharding](https://github.com/geoffreyharding)] - 0.2.4: Increased resiliency for killing threads under high load ([PR #118](https://github.com/javadelight/delight-nashorn-sandbox/pull/118) by [tellmewhattodo](https://github.com/tellmewhattodo)) - 0.2.0: Dynamically detects what version of the JDK the library is run with, and will either use the included Nashorn version or use Nashorn dependency ([Issue #109](https://github.com/javadelight/delight-nashorn-sandbox/issues/109), [PR #101](https://github.com/javadelight/delight-nashorn-sandbox/pull/111)).