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.

Sign up for GitHub

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

Jump to bottom

EXPERIMENTAL: Remove SecurityManager #1353

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

EXPERIMENTAL: Remove SecurityManager #1353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions spotbugs-exclude.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@
<Class name="org.mozilla.javascript.DToA" />
<Bug pattern="FE_FLOATING_POINT_EQUALITY" />
</Match>
<Match>
<!-- Ignore security-manager relevant bugs -->
<Bug pattern="DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED" />
</Match>
<Match>
<!-- A bigger task to take this on across the codebase -->
<Bug pattern="DM_DEFAULT_ENCODING" />
Expand Down
35 changes: 2 additions & 33 deletions src/org/mozilla/javascript/ClassCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

import java.io.Serializable;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.ConcurrentHashMap;

/**
Expand All @@ -22,42 +21,12 @@ public class ClassCache implements Serializable {
private static final long serialVersionUID = -8866246036237312215L;
private static final Object AKEY = "ClassCache";
private volatile boolean cachingIsEnabled = true;
private transient Map<CacheKey, JavaMembers> classTable;
private transient Map<Class, JavaMembers> classTable;
private transient Map<JavaAdapter.JavaAdapterSignature, Class<?>> classAdapterCache;
private transient Map<Class<?>, Object> interfaceAdapterCache;
private int generatedClassSerial;
private Scriptable associatedScope;

/**
* CacheKey is a combination of class and securityContext. This is required when classes are
* loaded from different security contexts
*/
static class CacheKey {
final Class<?> cls;
final Object sec;
/** Constructor. */
public CacheKey(Class<?> cls, Object securityContext) {
this.cls = cls;
this.sec = securityContext;
}

@Override
public int hashCode() {
int result = cls.hashCode();
if (sec != null) {
result = sec.hashCode() * 31;
}
return result;
}

@Override
public boolean equals(Object obj) {
return (obj instanceof CacheKey)
&& Objects.equals(this.cls, ((CacheKey) obj).cls)
&& Objects.equals(this.sec, ((CacheKey) obj).sec);
}
}

/**
* Search for ClassCache object in the given scope. The method first calls {@link
* ScriptableObject#getTopLevelScope(Scriptable scope)} to get the top most scope and then tries
Expand Down Expand Up @@ -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() {
Copy link
Contributor Author

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

if (classTable == null) {
// Use 1 as concurrency level here and for other concurrent hash maps
// as we don't expect high levels of sustained concurrent writes.
Expand Down
28 changes: 9 additions & 19 deletions src/org/mozilla/javascript/ContextFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@

package org.mozilla.javascript;

import java.security.AccessController;
import java.security.PrivilegedAction;

/**
* Factory class that Rhino runtime uses to create new {@link Context} instances. A <code>
* ContextFactory</code> can also notify listeners about context creation and release.
Expand Down Expand Up @@ -333,18 +330,11 @@ protected org.mozilla.javascript.xml.XMLLib.Factory getE4xImplementationFactory(

/**
* Create class loader for generated classes. This method creates an instance of the default
* implementation of {@link GeneratedClassLoader}. Rhino uses this interface to load generated
* JVM classes when no {@link SecurityController} is installed. Application can override the
* method to provide custom class loading.
* implementation of {@link GeneratedClassLoader}. Application can override the method to
* provide custom class loading.
*/
protected GeneratedClassLoader createClassLoader(final ClassLoader parent) {
return AccessController.doPrivileged(
new PrivilegedAction<DefiningClassLoader>() {
@Override
public DefiningClassLoader run() {
return new DefiningClassLoader(parent);
}
});
return new DefiningClassLoader(parent);
}

/**
Expand Down Expand Up @@ -495,9 +485,9 @@ public final <T> T call(ContextAction<T> action) {
* }
* </pre>
*
* Instead of using <code>enterContext()</code>, <code>exit()</code> pair consider using {@link
* #call(ContextAction)} which guarantees proper association of Context instances with the
* current thread. With this method the above example becomes:
* <p>Instead of using <code>enterContext()</code>, <code>exit()</code> pair consider using
* {@link #call(ContextAction)} which guarantees proper association of Context instances with
* the current thread. With this method the above example becomes:
*
* <pre>
* ContextFactory.call(new ContextAction() {
Expand All @@ -519,8 +509,8 @@ public Context enterContext() {
}

/**
* @deprecated use {@link #enterContext()} instead
* @return a Context associated with the current thread
* @deprecated use {@link #enterContext()} instead
*/
@Deprecated
public final Context enter() {
Expand All @@ -542,10 +532,10 @@ public final void exit() {
*
* @param cx a Context to associate with the thread if possible
* @return a Context associated with the current thread
* @see #enterContext()
* @see #call(ContextAction)
* @throws IllegalStateException if <code>cx</code> is already associated with a different
* thread
* @see #enterContext()
* @see #call(ContextAction)
*/
public final Context enterContext(Context cx) {
return Context.enter(cx, this);
Expand Down
3 changes: 1 addition & 2 deletions src/org/mozilla/javascript/DefiningClassLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Contributor Author

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?

}

@Override
Expand Down
4 changes: 2 additions & 2 deletions src/org/mozilla/javascript/Interpreter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -903,7 +903,7 @@ public String getSourcePositionFromStack(Context cx, int[] linep) {
public String getPatchedStack(RhinoException ex, String nativeStackTrace) {
String tag = "org.mozilla.javascript.Interpreter.interpretLoop";
StringBuilder sb = new StringBuilder(nativeStackTrace.length() + 1000);
String lineSeparator = SecurityUtilities.getSystemProperty("line.separator");
String lineSeparator = System.getProperty("line.separator");

CallFrame[] array = (CallFrame[]) ex.interpreterStackInfo;
int[] linePC = ex.interpreterLineData;
Expand Down Expand Up @@ -961,7 +961,7 @@ public String getPatchedStack(RhinoException ex, String nativeStackTrace) {
public List<String> getScriptStack(RhinoException ex) {
ScriptStackElement[][] stack = getScriptStackElements(ex);
List<String> list = new ArrayList<>(stack.length);
String lineSeparator = SecurityUtilities.getSystemProperty("line.separator");
String lineSeparator = System.getProperty("line.separator");
for (ScriptStackElement[] group : stack) {
StringBuilder sb = new StringBuilder();
for (ScriptStackElement elem : group) {
Expand Down
5 changes: 1 addition & 4 deletions src/org/mozilla/javascript/JavaAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,10 +488,7 @@ static Class<?> loadAdapterClass(String className, byte[] classBytes) {
Class<?> domainClass = SecurityController.getStaticSecurityDomainClass();
if (domainClass == CodeSource.class || domainClass == ProtectionDomain.class) {
// use the calling script's security domain if available
ProtectionDomain protectionDomain = SecurityUtilities.getScriptProtectionDomain();
if (protectionDomain == null) {
protectionDomain = JavaAdapter.class.getProtectionDomain();
}
ProtectionDomain protectionDomain = JavaAdapter.class.getProtectionDomain();
Copy link
Contributor Author

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?

if (domainClass == CodeSource.class) {
staticDomain = protectionDomain == null ? null : protectionDomain.getCodeSource();
} else {
Expand Down
35 changes: 7 additions & 28 deletions src/org/mozilla/javascript/JavaMembers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import java.lang.reflect.Member;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.security.AccessControlContext;
import java.security.AllPermission;
import java.security.Permission;
import java.util.ArrayList;
Expand Down Expand Up @@ -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);
Copy link
Contributor Author

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?

map.put(sig, method);
}
}
Expand Down Expand Up @@ -658,7 +656,7 @@ private Field[] getAccessibleFields(boolean includeProtected, boolean includePri
for (Field field : declared) {
int mod = field.getModifiers();
if (includePrivate || isPublic(mod) || isProtected(mod)) {
if (!field.isAccessible()) field.setAccessible(true);
VMBridge.instance.tryToMakeAccessible(field);
fieldsList.add(field);
}
}
Expand Down Expand Up @@ -770,17 +768,16 @@ static JavaMembers lookupClass(
Scriptable scope, Class<?> dynamicType, Class<?> staticType, boolean includeProtected) {
JavaMembers members;
ClassCache cache = ClassCache.get(scope);
Map<ClassCache.CacheKey, JavaMembers> ct = cache.getClassCacheMap();
Map<Class, JavaMembers> ct = cache.getClassCacheMap();

Class<?> cl = dynamicType;
Object secCtx = getSecurityContext();
for (; ; ) {
members = ct.get(new ClassCache.CacheKey(cl, secCtx));
members = ct.get(cl);
if (members != null) {
if (cl != dynamicType) {
// member lookup for the original class failed because of
// missing privileges, cache the result so we don't try again
ct.put(new ClassCache.CacheKey(dynamicType, secCtx), members);
ct.put(dynamicType, members);
}
return members;
}
Expand Down Expand Up @@ -811,11 +808,11 @@ static JavaMembers lookupClass(
}

if (cache.isCachingEnabled()) {
ct.put(new ClassCache.CacheKey(cl, secCtx), members);
ct.put(cl, members);
if (cl != dynamicType) {
// member lookup for the original class failed because of
// missing privileges, cache the result so we don't try again
ct.put(new ClassCache.CacheKey(dynamicType, secCtx), members);
ct.put(dynamicType, members);
}
}
return members;
Expand All @@ -830,24 +827,6 @@ private static JavaMembers createJavaMembers(
}
}

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;
}

RuntimeException reportMemberNotFound(String memberName) {
return Context.reportRuntimeErrorById(
"msg.java.member.not.found", cl.getName(), memberName);
Expand Down
28 changes: 1 addition & 27 deletions src/org/mozilla/javascript/LazilyLoadedCtor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@

import java.io.Serializable;
import java.lang.reflect.InvocationTargetException;
import java.security.AccessController;
import java.security.PrivilegedAction;

/**
* Avoid loading classes unless they are used.
Expand All @@ -26,27 +24,16 @@ public final class LazilyLoadedCtor implements Serializable {
private final String propertyName;
private final String className;
private final boolean sealed;
private final boolean privileged;
private Object initializedValue;
private int state;

public LazilyLoadedCtor(
ScriptableObject scope, String propertyName, String className, boolean sealed) {
this(scope, propertyName, className, sealed, false);
}

LazilyLoadedCtor(
ScriptableObject scope,
String propertyName,
String className,
boolean sealed,
boolean privileged) {
ScriptableObject scope, String propertyName, String className, boolean sealed) {

this.scope = scope;
this.propertyName = propertyName;
this.className = className;
this.sealed = sealed;
this.privileged = privileged;
this.state = STATE_BEFORE_INIT;

scope.addLazilyInitializedValue(propertyName, 0, this, ScriptableObject.DONTENUM);
Expand Down Expand Up @@ -77,19 +64,6 @@ Object getValue() {
}

private Object buildValue() {
if (privileged) {
return AccessController.doPrivileged(
new PrivilegedAction<Object>() {
@Override
public Object run() {
return buildValue0();
}
});
}
return buildValue0();
}

private Object buildValue0() {
Class<? extends Scriptable> cl = cast(Kit.classOrNull(className));
if (cl != null) {
try {
Expand Down
Loading