EXPERIMENTAL: Remove SecurityManager #1353

rPraml · 2023-07-06T14:30:17Z

Hello, I want to discuss, if it is already time to remove the dependency to SecurityManager from Rhino.

There was already a discussion, where I was involved: #972 (comment)

and the last days I found some time to update our fork.

Why should we remove the dependency to securityManager

because it is deprecated since Java 9
because it is deprecated for removal since java 17

Why shouldn't we remove it

because there are JVMs that still have an active security-manager
because people rely on a complex security policy, when executing javascript (at least we did this, but we removed it from our application code, because we realized that the concept is very hard to understand and there is no 100% security)

This PR is radical and removes all code pieces, that relate to SecurityManager.

(Note: I've made an earlier PR, where I moved all stuff to a "SecurityBridge". See: #1068)

Maybe you can provide some feedback, what will be a good way to go here?

rPraml

This PR contains some thoughts. Maybe more code can be removed/simplified (like SecureCaller)

rPraml · 2023-07-07T05:51:27Z

src/org/mozilla/javascript/ClassCache.java

@@ -132,7 +101,7 @@ public synchronized void setCachingEnabled(boolean enabled) {
    }

    /** @return a map from classes to associated JavaMembers objects */
-    Map<CacheKey, JavaMembers> getClassCacheMap() {
+    Map<Class, JavaMembers> getClassCacheMap() {


No security context -> We can revert this change #1019

rPraml · 2023-07-07T05:52:10Z

src/org/mozilla/javascript/DefiningClassLoader.java

@@ -23,8 +23,7 @@ public Class<?> defineClass(String name, byte[] data) {
        // Use our own protection domain for the generated classes.
        // TODO: we might want to use a separate protection domain for classes
        // compiled from scripts, based on where the script was loaded from.
-        return super.defineClass(
-                name, data, 0, data.length, SecurityUtilities.getProtectionDomain(getClass()));
+        return super.defineClass(name, data, 0, data.length, getClass().getProtectionDomain());


CHECKME: Would we need a protectionDomain at all?

rPraml · 2023-07-07T05:52:41Z

src/org/mozilla/javascript/JavaAdapter.java

-            if (protectionDomain == null) {
-                protectionDomain = JavaAdapter.class.getProtectionDomain();
-            }
+            ProtectionDomain protectionDomain = JavaAdapter.class.getProtectionDomain();


CHECKME: can this be simplified?

rPraml · 2023-07-07T05:53:08Z

src/org/mozilla/javascript/JavaMembers.java

@@ -319,8 +318,7 @@ private void discoverAccessibleMethods(
                                if (isPublic(mods) || isProtected(mods) || includePrivate) {
                                    MethodSignature sig = new MethodSignature(method);
                                    if (!map.containsKey(sig)) {
-                                        if (includePrivate && !method.isAccessible())
-                                            method.setAccessible(true);
+                                        VMBridge.instance.tryToMakeAccessible(method);


CHECKME: Do we need a VMBridge for java 11/17?

gbrail · 2023-08-04T18:13:58Z

I think that we should do this after the 1.7.15 release -- at that point I want to reorganize the source files (git seems to be smart enough to make that less of a nightmare), upgrade the build to require Java 11 minimum, and then this can be one of the first things that we address.

rPraml · 2024-08-22T12:05:34Z

Hello @gbrail, @p-bakker

I've resolved the merge conflicts here, but I will still consider this as experimental:

Maybe we shoud first decide, if we still need for VMBridge / VMBridge_jdk18, repectively JavaMembers / JavaMembers_jdk11

I don't know, if there are still use cases, where someone needs its own VMBridge, but trying to load VMBridge_custom from org.mozilla.javascript will not work in a modular java (see related issue #1076)
-> either we decide to remove custom VMBridges completely
-> or load it with ServiceLoader

As minimum JVM version is now 11, there will be also no need for JavaMembers / JavaMembers_jdk11

p-bakker · 2024-09-08T07:24:43Z

Converted as draft as still considered experimental by the author

p-bakker · 2024-09-10T05:45:33Z

@rPraml in the mentioned discussion you said your company currently heavily relies on the SecurityManager to prevent untrusted JavaScript code from doing stuff that is not allowed.

Have you come up with an alternative approach achieving the same that didn't rely on the SecurityManager?

rPraml · 2024-09-24T14:19:17Z

@p-bakker
The short story:

We do not longer rely on SecurityManager and I would agree, if it will be removed from rhino (in 2.0?)

Long story:

In the beginning we tried to allow the end user to edit our JavaScript code, where we have modelled a lot of our business logic.
This JavaScript code was (more or less) protected by ClassShutter and a whiteList of classes/packages that we have classified as "secure"

We found ways to bypass these mechansims (e. g. trustedObject.getClass().getClassLoader().loadClass("non.allowed.Clazz")) and began to experiment with a SecurtityManager for rhino.

Debugging and fixing one security issue brings the next to light... (it was a pain, to find a good balance, to enable features and yet offer enough security)

With the knowledge of JEP411, we abandoned the idea that we could ever provide a JavaScript user interface where the end user cannot compromise the system.

So in our current application, we have forbidden to view/edit JavaScript code to the end user

p-bakker · 2024-09-24T17:51:54Z

Right, was afraid of that. I guess the only way to allow untrusted script to run 'securely' is to disable Java Interop completely and create custom host objects (Scriptable's) that expose whatever domain logic you want to expose

And 'securily' in brackets, because:

a rogue script could consume a ton of resources, starving the system
unless you properly seal everything, including (hidden) prototypes etc (see Secure EcmaScript/SES) and execute in a new or user/tenant/... specific Realm, there scripts can interfere with eachother
who knows what other attack vectors there might be...

p-bakker · 2024-09-24T17:57:54Z

Wrt to removing the SecurityManager: while it has been delayed for removal, to my knowledge is not clear yet in which Java version it'll actually be removed, no?

I propose to leave it in Rhino at least until after the next release, as the next release is sharing up to be a significant improvement on the EcmaScript-compatibility side (amongst other improvements) and I rather not lock Rhino users out of those goodies if they currently depend on the SecurityManager

What I think we could do though is mark the relevant methods as deprecated, include a note about it in the release notes and maybe write some documentation about security with Rhino in general and about moving away from the SecurityManager specifically

rPraml · 2024-09-25T14:15:42Z

In https://openjdk.org/jeps/411 they said

In feature releases after Java 18, we will degrade other Security Manager APIs so that they remain in place but with limited or no functionality. For example, we may revise AccessController::doPrivileged simply to run the given action, or revise System::getSecurityManager always to return null

But as far as I see, this hasn't been done yet. There are no relevant changes in
https://github.com/openjdk/jdk/commits/master/src/java.base/share/classes/java/lang/SecurityManager.java
https://github.com/openjdk/jdk/commits/master/src/java.base/share/classes/java/security/AccessController.java

There is a comment in https://bugs.openjdk.org/browse/JDK-8264713 where someone asked for a date.

The only thing I noticed, that some testcases print the warning

WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by org.mozilla.javascript.tests.SecurityControllerTest

on the console since some time.

EXPERIMENTAL: Remove SecurityManager

732cadb

rPraml commented Jul 7, 2023

View reviewed changes

rPraml mentioned this pull request Sep 5, 2023

Classcache must honor current security context. #1019

Merged

p-bakker added the Java Compatibility Issues related to Rhino being compatible to (new) Java releases label Oct 25, 2023

rPraml added 3 commits August 22, 2024 13:11

Merge branch 'master-upstream' into remove-security-manager

3c4d6e6

spotless

0c32b2d

fix compile error

47ec21c

rPraml mentioned this pull request Aug 22, 2024

Move SecurityManager relevant parts to SecurityBridge #1068

Closed

p-bakker marked this pull request as draft September 8, 2024 07:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EXPERIMENTAL: Remove SecurityManager #1353

EXPERIMENTAL: Remove SecurityManager #1353

rPraml commented Jul 6, 2023 •

edited by p-bakker

Loading

rPraml left a comment

rPraml Jul 7, 2023

rPraml Jul 7, 2023

rPraml Jul 7, 2023

rPraml Jul 7, 2023

gbrail commented Aug 4, 2023

rPraml commented Aug 22, 2024

p-bakker commented Sep 8, 2024

p-bakker commented Sep 10, 2024

rPraml commented Sep 24, 2024

p-bakker commented Sep 24, 2024

p-bakker commented Sep 24, 2024

rPraml commented Sep 25, 2024

EXPERIMENTAL: Remove SecurityManager #1353

Are you sure you want to change the base?

EXPERIMENTAL: Remove SecurityManager #1353

Conversation

rPraml commented Jul 6, 2023 • edited by p-bakker Loading

rPraml left a comment

Choose a reason for hiding this comment

rPraml Jul 7, 2023

Choose a reason for hiding this comment

rPraml Jul 7, 2023

Choose a reason for hiding this comment

rPraml Jul 7, 2023

Choose a reason for hiding this comment

rPraml Jul 7, 2023

Choose a reason for hiding this comment

gbrail commented Aug 4, 2023

rPraml commented Aug 22, 2024

p-bakker commented Sep 8, 2024

p-bakker commented Sep 10, 2024

rPraml commented Sep 24, 2024

p-bakker commented Sep 24, 2024

p-bakker commented Sep 24, 2024

rPraml commented Sep 25, 2024

rPraml commented Jul 6, 2023 •

edited by p-bakker

Loading