Classcache must honor current security context.

[rPraml]

Hello, we use different security contexts when executing javascript.
In different security contexts, different methods are allowed to call. We distinguish between restricted (=safe) and unrestricted (=dangerous) methods.

We noticed, that the ClassCache uses only the class as key and caches the visible methods on first invocation.
If this was done in a restricted context, it is not possible to call some unrestricted methods in an unrestricted context later, as the cache will be hit.
The same is, if the class was loaded in an unrestricted securitycontext, you can call unrestricted methods in a restricted context.

With this PR, the cache takes the current security context (in detail, the java.security.Permissions) into account, when caching classes.

Please review and give me feedback.

[gbrail]

This looks basically good to me. I don't regularly work with the security framework so I'll assume that you know it well enough to ensure that ".equals" is a good way to compare security contexts.

Before we merge this, I really would like you to write a test. It can use a mock security context, or FWIW it can just use an arbitrary object as the "security context" but I think that we need something that covers the code here, including the testing in various cases.

Thanks!

[rPraml]

I can write a test for this

[rPraml]

I've added a testcase for the code change. 0e01d89
The test may look complex, but I'm working on improving the classShutter, so I will provide more test cases based on this code piece

/edit: have to investigate why test runs in eclipse but not gradle

[rPraml]

This looks basically good to me. I don't regularly work with the security framework so I'll assume that you know it well enough to ensure that ".equals" is a good way to compare security contexts.

The standard implementation of java.lang.SecurityManager.getSecurityContext delegates to AccessController.getContext() and this will return a java.security.AccessControlContext that has a good ".equals" method to compare them. It is also only necessary to store the context, if the script runs in "restricted" environment. If there is no security manager, or if there are "all" permissions, we use null as key to distinguish between them.

[gbrail]

I think that this looks good -- thanks for writing complete tests. I just have one tiny nit (below) and it's ready to merge. Thanks!

[gbrail]

Thanks!

[pKrav75]

Hello,
Sorry to come back to this topic, which has been closed for 2 years.
We have been using Rhino intensively for over 10 years with a SecurityManager and adapted permissions. Our java code very frequently calls Rhino with Java variable passing. The JS itself allocates Java variables. In early summer 2023, we switched to version 1.7.14 and noticed a significant performance degradation (factor 4 or 5). On closer examination, we found that the majority of execution time is spent here:

"https-jsse-nio-8444-exec-6" #103 daemon prio=5 os_prio=0 cpu=59474.36ms elapsed=2071.12s tid=0x00007fd120872860 nid=0xe7a runnable [0x00007fcf425f7000] java.lang.Thread.State: RUNNABLE at java.lang.Throwable.fillInStackTrace([email protected]/Native Method) at java.lang.Throwable.fillInStackTrace([email protected]/Throwable.java:798) - locked <0x0000000658ee8940> (a java.security.AccessControlException) at java.lang.Throwable.<init>([email protected]/Throwable.java:271) at java.lang.Exception.<init>([email protected]/Exception.java:67) at java.lang.RuntimeException.<init>([email protected]/RuntimeException.java:63) at java.lang.SecurityException.<init>([email protected]/SecurityException.java:52) at java.security.AccessControlException.<init>([email protected]/AccessControlException.java:79) at java.security.AccessControlContext.checkPermission([email protected]/AccessControlContext.java:485) at org.mozilla.javascript.JavaMembers.getSecurityContext(JavaMembers.java:837) at org.mozilla.javascript.JavaMembers.lookupClass(JavaMembers.java:773) at org.mozilla.javascript.NativeJavaObject.initMembers(NativeJavaObject.java:63) at org.mozilla.javascript.NativeJavaObject.<init>(NativeJavaObject.java:53) at org.mozilla.javascript.NativeJavaObject.<init>(NativeJavaObject.java:44)

Apparently most of the time is spent handling the exception introduced by this PR :

`private static Object getSecurityContext() {
Object sec = null;
SecurityManager sm = System.getSecurityManager();
if (sm != null) {
sec = sm.getSecurityContext();
if (sec instanceof AccessControlContext) {
try {
((AccessControlContext) sec).checkPermission(allPermission);
// if we have allPermission, we do not need to store the
// security object in the cache key
return null;
} catch (SecurityException e) {
}
}
}

    return sec;
}`

Indeed, we never run with allPermession, and it rightly seems that the check is performed as soon as a java variable is accessed.

As far as we're concerned, the Security Manager's permissioning doesn't change during the entire JVM runtime, so why isn't this test performed once for all?
Is it possible to propose a PR in this direction?

[rPraml]

Hello @pKrav75
thanks for your feedback.

Yes, I see, that handling the exception may consume a lot of time.

The idea behind the code was, that all JavaMember.lookupClasses are put in the same slot, if the current AccessControlContext has all permissions. Unfortunately, it is only possible to detect this, if checkPermission is called.

As far as we're concerned, the Security Manager's permissioning doesn't change during the entire JVM runtime, so why isn't this test performed once for all?

IMHO each invocation has to get the current AccessControlContext from the SecurityManager. It depends on the current call stack, so the check looks OK for me.
It may be possible to cache the result somehow to speed up the tests

Is it possible to propose a PR in this direction?

Sure, but I think the SecurityManager will be removed in Rhino 2.0 because it is deprecated and I don't know, if it is planned to make a 1.7.x release (maybe @gbrail has a schedule for this?)

So I don't know, if a PR will get merged.
It would be also OK for me to revert that change, because we do not use a security manager any more.

It's sad that the SecurityManager is deprecated, but I can understand the JDK developers because the SecurityManager concept is very complicated and doesn't offer 100% security (Please read related issues #1045 #972 #1363 and #1353)

I know that's not exactly the answer you want to hear, but you have to face reality and live with the fact that a security concept based on the SecurityManager no longer has a future. (It was also no easy task for us)

[pKrav75]

Thank you for your prompt reply.
I was actually expecting something of the sort, especially regarding the SecurityManager deprecation.
Unfortunately the migration path is far from obvious for us, and in the short (medium ?) term we have to support our distributions. So I'm interested to hear more from @gbrail about a possible 1.7.X release, which might see this change reverted.

Thanks again for your responsiveness :-)