-
Notifications
You must be signed in to change notification settings - Fork 847
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
EXPERIMENTAL: Remove SecurityManager #1353
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR contains some thoughts. Maybe more code can be removed/simplified (like SecureCaller)
@@ -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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No security context -> We can revert this change #1019
@@ -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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CHECKME: Would we need a protectionDomain at all?
if (protectionDomain == null) { | ||
protectionDomain = JavaAdapter.class.getProtectionDomain(); | ||
} | ||
ProtectionDomain protectionDomain = JavaAdapter.class.getProtectionDomain(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CHECKME: can this be simplified?
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CHECKME: Do we need a VMBridge for java 11/17?
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. |
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 As minimum JVM version is now 11, there will be also no need for JavaMembers / JavaMembers_jdk11 |
Converted as draft as still considered experimental by the author |
@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? |
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
Why shouldn't we remove it
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?