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

Sandboxing JavaScript runtime environment #520

Merged
merged 2 commits into from
Dec 3, 2015
Merged

Sandboxing JavaScript runtime environment #520

merged 2 commits into from
Dec 3, 2015

Conversation

alterakey
Copy link
Member

Hello,
This is my first attempt to secure the JS validator facility.

I think JS based validators are good to have, but at the same time I feel it's better to have runtime enviroments confined somehow. I have written some working exploits for RedPen 1.3 or later; if you are interested in, I'll show you.

Changes are:

  • Disable intrinsic Java bridging facilities in order to execute validators
  • Disable intrinsic function to load JS or enable Rhino compatibility in order to execute validators

Please review and hopefully merge. Keep up good work.
Best,
-t

@@ -224,6 +224,7 @@ protected String getLocalizedErrorMessage(String key, Object... args) {
"var addLocalizedError = Function.prototype.bind.call(redpenToBeBound.addLocalizedError, redpenToBeBound);" +
"var addLocalizedErrorFromToken = Function.prototype.bind.call(redpenToBeBound.addLocalizedErrorFromToken, redpenToBeBound);" +
"var addLocalizedErrorWithPosition = Function.prototype.bind.call(redpenToBeBound.addLocalizedErrorWithPosition, redpenToBeBound);");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding "java = undefined; javax = undefined; Java = undefined; load = undefined; redpenToBeBound = undefined;"); to this section, instead of introducing appllySandbox method and UnconfinedScript?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to do so, but that will break a test.

In redpen-core/src/test/java/cc/redpen/validator/JavaScriptValidatorTest.java, we have:

@Test
public void testJSLiteralValidator() throws RedPenException, IOException {
    ...
    validator.scripts.add(new ...Script(validator, "testScript.js",
            "function preValidateSentence(sentence) {" +
                    // add function names to "calledFunctions" list upon function calls for the later assertions
                    // the following script is using Nashorn's lobal object "Java".type to access static member:
                    // http://docs.oracle.com/javase/8/docs/technotes/guides/scripting/nashorn/api.html
                    "Java.type('cc.redpen.validator.JavaScriptValidatorTest').calledFunctions.add('preValidateSentence');}" +
     ...

So the test function needs the bridging facility for a legitimate purpose.

That's why I introduced UnconfinedScript.

@alterakey
Copy link
Member Author

Okay, now it's gone.

@takahi-i
Copy link
Member

takahi-i commented Dec 2, 2015

👍

@yusuke
Copy link
Contributor

yusuke commented Dec 2, 2015

What I understand is that this fix will prevent attacker from running malicious code on RedPen using JavaScriptValidator & JavaScript - Java bridging mechanism.

As the prerequisite, the attacker needs to get an access to the local file system. In that case, they can run any script even without JavaScriptValidator.

I mean, is this for attackers who have write access to the $REDPEN_HOME/js directory, but don't have any permission to execute scripts on that machine?

takahi-i added a commit that referenced this pull request Dec 3, 2015
Sandboxing JavaScript runtime environment
@takahi-i takahi-i merged commit 0d9cb74 into redpen-cc:master Dec 3, 2015
@alterakey
Copy link
Member Author

Please consider on the redpen-server. It is meant to be run by a user like a web server (and redpen-server doesn’t have a privilege separation mechanism yet.)

Then someone places a validator to somewhere untrusted users can write, say the /tmp, /var/www/…, /home/ftp/… , and instructs redpen-server to run it — with the privileges of the redpen-server user. With the word whatever I mean it — arbitrary code execution.

This is what I recognize as a vulnerability. It is going to be a problem even if RedPen restrict its script directory — what if people can run arbitrary code on some cloud-based services using RedPen?

Rhino/Nashorn engines under Java 8u40 has no real protection against the problem (*1); this patch should provide only marginal one. If you cannot accept the patch, you should at least clearly warn users not to run JS validators from untrusted sources, and/or to restrict access of the redpen-server (and/or JS validator directory) to trusted parties only.

*1: http://bugs.java.com/bugdatabase/view_bug.do?bug_id=8050078

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 this pull request may close these issues.

3 participants