From 96ece8802c094c75aa84ac7bdbb40c62d03de0f4 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Mon, 3 Apr 2023 10:30:15 +0200 Subject: [PATCH] Fixes #6184 - JEP-411 will deprecate/remove the SecurityManager from the JVM. Removed usages of `SecurityManager` and `AccessControlller.doPrivileged()`. In places where they are still necessary, now using reflection. Signed-off-by: Simone Bordet --- .../jaspi/DefaultAuthConfigFactory.java | 52 +++++--- .../jetty/server/handler/ContextHandler.java | 62 +++++---- .../logging/JettyLoggerConfiguration.java | 41 +++--- .../org/eclipse/jetty/util/MemoryUtils.java | 14 +-- .../java/org/eclipse/jetty/util/TypeUtil.java | 4 +- .../util/thread/PrivilegedThreadFactory.java | 92 ++++++++++---- .../eclipse/jetty/xml/XmlConfiguration.java | 119 +++++++++--------- 7 files changed, 222 insertions(+), 162 deletions(-) diff --git a/jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/DefaultAuthConfigFactory.java b/jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/DefaultAuthConfigFactory.java index bc687e1b69b8..2d67d34d1ae6 100644 --- a/jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/DefaultAuthConfigFactory.java +++ b/jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/DefaultAuthConfigFactory.java @@ -60,9 +60,7 @@ public AuthConfigProvider getConfigProvider(String layer, String appContext, Reg @Override public String registerConfigProvider(String className, Map properties, String layer, String appContext, String description) { - SecurityManager sm = System.getSecurityManager(); - if (sm != null) - sm.checkPermission(new AuthPermission("registerAuthConfigProvider")); + checkPermission("registerAuthConfigProvider"); String key = getKey(layer, appContext); AuthConfigProvider configProvider = createConfigProvider(className, properties); @@ -76,9 +74,7 @@ public String registerConfigProvider(String className, Map properties, String la @Override public String registerConfigProvider(AuthConfigProvider provider, String layer, String appContext, String description) { - SecurityManager sm = System.getSecurityManager(); - if (sm != null) - sm.checkPermission(new AuthPermission("registerAuthConfigProvider")); + checkPermission("registerAuthConfigProvider"); String key = getKey(layer, appContext); DefaultRegistrationContext context = new DefaultRegistrationContext(provider, layer, appContext, description, false); @@ -91,9 +87,7 @@ public String registerConfigProvider(AuthConfigProvider provider, String layer, @Override public boolean removeRegistration(String registrationID) { - SecurityManager sm = System.getSecurityManager(); - if (sm != null) - sm.checkPermission(new AuthPermission("removeAuthRegistration")); + checkPermission("removeAuthRegistration"); DefaultRegistrationContext registrationContext = _registrations.remove(registrationID); if (registrationContext == null) @@ -106,9 +100,7 @@ public boolean removeRegistration(String registrationID) @Override public String[] detachListener(RegistrationListener listener, String layer, String appContext) { - SecurityManager sm = System.getSecurityManager(); - if (sm != null) - sm.checkPermission(new AuthPermission("detachAuthListener")); + checkPermission("detachAuthListener"); List registrationIds = new ArrayList<>(); for (DefaultRegistrationContext registration : _registrations.values()) @@ -145,13 +137,43 @@ public RegistrationContext getRegistrationContext(String registrationID) @Override public void refresh() { - SecurityManager sm = System.getSecurityManager(); - if (sm != null) - sm.checkPermission(new AuthPermission("refreshAuth")); + checkPermission("refreshAuth"); // TODO: maybe we should re-construct providers created from classname. } + private static void checkPermission(String permission) + { + try + { + Object securityManager = getSecurityManager(); + if (securityManager == null) + return; + securityManager.getClass().getMethod("checkPermission") + .invoke(securityManager, new AuthPermission(permission)); + } + catch (SecurityException x) + { + throw x; + } + catch (Throwable ignored) + { + } + } + + private static Object getSecurityManager() + { + try + { + // Use reflection to work with Java versions that have and don't have SecurityManager. + return System.class.getMethod("getSecurityManager").invoke(null); + } + catch (Throwable ignored) + { + return null; + } + } + private static String getKey(String layer, String appContext) { return layer + "/" + appContext; diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index 5816266ab657..21a328eb2f1d 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -20,8 +20,6 @@ import java.net.URI; import java.net.URL; import java.net.URLClassLoader; -import java.security.AccessController; -import java.security.PrivilegedAction; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -219,7 +217,7 @@ private enum ProtectedTargetType private int _maxFormKeys = Integer.getInteger(MAX_FORM_KEYS_KEY, DEFAULT_MAX_FORM_KEYS); private int _maxFormContentSize = Integer.getInteger(MAX_FORM_CONTENT_SIZE_KEY, DEFAULT_MAX_FORM_CONTENT_SIZE); private boolean _compactPath = false; - private boolean _usingSecurityManager = System.getSecurityManager() != null; + private boolean _usingSecurityManager = getSecurityManager() != null; private final List _programmaticListeners = new CopyOnWriteArrayList<>(); private final List _servletContextListeners = new CopyOnWriteArrayList<>(); @@ -326,7 +324,7 @@ public boolean isUsingSecurityManager() public void setUsingSecurityManager(boolean usingSecurityManager) { - if (usingSecurityManager && System.getSecurityManager() == null) + if (usingSecurityManager && getSecurityManager() == null) throw new IllegalStateException("No security manager"); _usingSecurityManager = usingSecurityManager; } @@ -2114,6 +2112,19 @@ public void clearAliasChecks() _aliasChecks.clear(); } + private static Object getSecurityManager() + { + try + { + // Use reflection to work with Java versions that have and don't have SecurityManager. + return System.class.getMethod("getSecurityManager").invoke(null); + } + catch (Throwable ignored) + { + return null; + } + } + /** * Context. *

@@ -2561,11 +2572,9 @@ public ClassLoader getClassLoader() { // check to see if the classloader of the caller is the same as the context // classloader, or a parent of it, as required by the javadoc specification. - - // Wrap in a PrivilegedAction so that only Jetty code will require the - // "createSecurityManager" permission, not also application code that calls this method. - Caller caller = AccessController.doPrivileged((PrivilegedAction)Caller::new); - ClassLoader callerLoader = caller.getCallerClassLoader(2); + ClassLoader callerLoader = StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE) + .getCallerClass() + .getClassLoader(); while (callerLoader != null) { if (callerLoader == _classLoader) @@ -2573,11 +2582,31 @@ public ClassLoader getClassLoader() else callerLoader = callerLoader.getParent(); } - System.getSecurityManager().checkPermission(new RuntimePermission("getClassLoader")); + checkPermission(); return _classLoader; } } + private void checkPermission() + { + try + { + // Use reflection to work with Java versions that still have the SecurityManager. + Object securityManager = getSecurityManager(); + if (securityManager == null) + return; + securityManager.getClass().getMethod("checkPermission") + .invoke(securityManager, new RuntimePermission("getClassLoader")); + } + catch (SecurityException x) + { + throw x; + } + catch (Throwable ignored) + { + } + } + @Override public JspConfigDescriptor getJspConfigDescriptor() { @@ -3103,17 +3132,4 @@ public static interface ContextScopeListener extends EventListener */ void exitScope(Context context, Request request); } - - private static class Caller extends SecurityManager - { - public ClassLoader getCallerClassLoader(int depth) - { - if (depth < 0) - return null; - Class[] classContext = getClassContext(); - if (classContext.length <= depth) - return null; - return classContext[depth].getClassLoader(); - } - } } diff --git a/jetty-slf4j-impl/src/main/java/org/eclipse/jetty/logging/JettyLoggerConfiguration.java b/jetty-slf4j-impl/src/main/java/org/eclipse/jetty/logging/JettyLoggerConfiguration.java index dd95a72819dc..e51024b03f06 100644 --- a/jetty-slf4j-impl/src/main/java/org/eclipse/jetty/logging/JettyLoggerConfiguration.java +++ b/jetty-slf4j-impl/src/main/java/org/eclipse/jetty/logging/JettyLoggerConfiguration.java @@ -16,8 +16,6 @@ import java.io.IOException; import java.io.InputStream; import java.net.URL; -import java.security.AccessController; -import java.security.PrivilegedAction; import java.util.Arrays; import java.util.Locale; import java.util.Properties; @@ -161,29 +159,26 @@ public TimeZone getTimeZone(String key) */ public JettyLoggerConfiguration load(ClassLoader loader) { - return AccessController.doPrivileged((PrivilegedAction)() -> + // First see if the jetty-logging.properties object exists in the classpath. + // * This is an optional feature used by embedded mode use, and test cases to allow for early + // * configuration of the Log class in situations where access to the System.properties are + // * either too late or just impossible. + load(readProperties(loader, "jetty-logging.properties")); + + // Next see if an OS specific jetty-logging.properties object exists in the classpath. + // This really for setting up test specific logging behavior based on OS. + String osName = System.getProperty("os.name"); + if (osName != null && osName.length() > 0) { - // First see if the jetty-logging.properties object exists in the classpath. - // * This is an optional feature used by embedded mode use, and test cases to allow for early - // * configuration of the Log class in situations where access to the System.properties are - // * either too late or just impossible. - load(readProperties(loader, "jetty-logging.properties")); - - // Next see if an OS specific jetty-logging.properties object exists in the classpath. - // This really for setting up test specific logging behavior based on OS. - String osName = System.getProperty("os.name"); - if (osName != null && osName.length() > 0) - { - // NOTE: cannot use jetty-util's StringUtil.replace() as it may initialize logging itself. - osName = osName.toLowerCase(Locale.ENGLISH).replace(' ', '-'); - load(readProperties(loader, "jetty-logging-" + osName + ".properties")); - } + // NOTE: cannot use jetty-util's StringUtil.replace() as it may initialize logging itself. + osName = osName.toLowerCase(Locale.ENGLISH).replace(' ', '-'); + load(readProperties(loader, "jetty-logging-" + osName + ".properties")); + } - // Now load the System.properties as-is into the properties, - // these values will override any key conflicts in properties. - load(System.getProperties()); - return this; - }); + // Now load the System.properties as-is into the properties, + // these values will override any key conflicts in properties. + load(System.getProperties()); + return this; } public String getString(String key, String defValue) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/MemoryUtils.java b/jetty-util/src/main/java/org/eclipse/jetty/util/MemoryUtils.java index aa024f549ed9..4b53a983c7f5 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/MemoryUtils.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/MemoryUtils.java @@ -13,9 +13,6 @@ package org.eclipse.jetty.util; -import java.security.AccessController; -import java.security.PrivilegedAction; - /** * MemoryUtils provides an abstraction over memory properties and operations. */ @@ -25,18 +22,11 @@ public class MemoryUtils static { - final int defaultValue = 64; + int defaultValue = 64; int value = defaultValue; try { - value = Integer.parseInt(AccessController.doPrivileged(new PrivilegedAction() - { - @Override - public String run() - { - return System.getProperty("org.eclipse.jetty.util.cacheLineBytes", String.valueOf(defaultValue)); - } - })); + value = Integer.parseInt(System.getProperty("org.eclipse.jetty.util.cacheLineBytes", String.valueOf(defaultValue))); } catch (Exception ignored) { diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/TypeUtil.java b/jetty-util/src/main/java/org/eclipse/jetty/util/TypeUtil.java index e983e56e3702..be8ce1200cf4 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/TypeUtil.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/TypeUtil.java @@ -26,9 +26,7 @@ import java.net.URI; import java.net.URISyntaxException; import java.net.URL; -import java.security.AccessController; import java.security.CodeSource; -import java.security.PrivilegedAction; import java.security.ProtectionDomain; import java.util.ArrayList; import java.util.Arrays; @@ -671,7 +669,7 @@ public static URI getCodeSourceLocation(Class clazz) { try { - ProtectionDomain domain = AccessController.doPrivileged((PrivilegedAction)() -> clazz.getProtectionDomain()); + ProtectionDomain domain = clazz.getProtectionDomain(); if (domain != null) { CodeSource source = domain.getCodeSource(); diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java index 5c506c7abfc1..2cf95cd043bb 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java @@ -13,39 +13,85 @@ package org.eclipse.jetty.util.thread; -import java.security.AccessController; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; import java.security.PrivilegedAction; +import java.security.ProtectionDomain; import java.util.function.Supplier; /** - * Convenience class to ensure that a new Thread is created - * inside a privileged block. - * - * This prevents the Thread constructor - * from pinning the caller's context classloader. This happens - * when the Thread constructor takes a snapshot of the current - * calling context - which contains ProtectionDomains that may - * reference the context classloader - and remembers it for the - * lifetime of the Thread. + *

Convenience {@link Thread} factory that ensure threads are + * created without referencing any web application {@link ClassLoader}.

+ *

Up to Java 17, the {@code Thread} constructor was taking a + * snapshot of the calling context, which may contain a {@link ProtectionDomain} + * that references a web application {@code ClassLoader} + * (for example if the creation of the {@code Thread} was triggered + * by some operation performed by the web application). + * The {@code Thread} might then be pooled and prevent the + * web application {@code ClassLoader} to be garbage collected + * when the web application is undeployed. + * For this reason, {@code Thread}s must be created in a privileged + * action, which restricts the calling context to just the caller + * frame, not all the frames in the stack.

+ *

Since Java 18 and the removal of the Java security manager + * and related classes by JEP 411, {@code Thread}s do not retain + * the calling context, so there is no need to create them in a + * privileged action.

*/ class PrivilegedThreadFactory { + private static final MethodHandle privileged = lookup(); + private static MethodHandle lookup() + { + try + { + // Use reflection to work with Java versions that have and don't have AccessController. + Class klass = ClassLoader.getPlatformClassLoader().loadClass("java.security.AccessController"); + MethodHandles.Lookup lookup = MethodHandles.lookup(); + return lookup.findStatic(klass, "doPrivileged", MethodType.methodType(Object.class, PrivilegedAction.class)); + } + catch (Throwable x) + { + return null; + } + } + /** - * Use a Supplier to make a new thread, calling it within - * a privileged block to prevent classloader pinning. - * - * @param newThreadSupplier a Supplier to create a fresh thread - * @return a new thread, protected from classloader pinning. + *

Creates a new {@link Thread} from the given {@link Supplier}, + * without retaining the calling context.

+ * + * @param supplier the {@link Supplier} that creates the {@link Thread} + * @return a new {@link Thread} without retaining the calling context */ - static T newThread(Supplier newThreadSupplier) + static T newThread(Supplier supplier) { - return AccessController.doPrivileged(new PrivilegedAction() + // Keep this method short and inlineable. + MethodHandle methodHandle = privileged; + if (methodHandle == null) + return supplier.get(); + return privilegedNewThread(methodHandle, supplier); + } + + @SuppressWarnings("unchecked") + private static T privilegedNewThread(MethodHandle privileged, Supplier supplier) + { + try + { + PrivilegedAction action = supplier::get; + return (T)privileged.invoke(action); + } + catch (RuntimeException | Error x) + { + throw x; + } + catch (Throwable x) { - @Override - public T run() - { - return newThreadSupplier.get(); - } - }); + throw new RuntimeException(x); + } + } + + private PrivilegedThreadFactory() + { } } diff --git a/jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java b/jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java index ca48f84d7e5b..76e143d5a459 100644 --- a/jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java +++ b/jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java @@ -30,8 +30,6 @@ import java.net.UnknownHostException; import java.nio.file.Path; import java.nio.file.Paths; -import java.security.AccessController; -import java.security.PrivilegedExceptionAction; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -493,7 +491,7 @@ public void configure(Object obj, XmlParser.Node cfg, int i) throws Exception } catch (Exception e) { - LOG.warn("Config error {} at {} in {}", e.toString(), node, _configuration); + LOG.warn("Config error {} at {} in {}", e, node, _configuration); throw e; } } @@ -1815,86 +1813,81 @@ public static void main(final String... args) throws Exception { try { - AccessController.doPrivileged((PrivilegedExceptionAction)() -> - { - Properties properties = new Properties(); - properties.putAll(System.getProperties()); + Properties properties = new Properties(); + properties.putAll(System.getProperties()); - // For all arguments, load properties - if (LOG.isDebugEnabled()) - LOG.debug("args={}", Arrays.asList(args)); - for (String arg : args) + // For all arguments, load properties + if (LOG.isDebugEnabled()) + LOG.debug("args={}", Arrays.asList(args)); + for (String arg : args) + { + if (arg.indexOf('=') >= 0) { - if (arg.indexOf('=') >= 0) + int i = arg.indexOf('='); + properties.put(arg.substring(0, i), arg.substring(i + 1)); + } + else if (arg.toLowerCase(Locale.ENGLISH).endsWith(".properties")) + { + try (InputStream inputStream = Resource.newResource(arg).getInputStream()) { - int i = arg.indexOf('='); - properties.put(arg.substring(0, i), arg.substring(i + 1)); - } - else if (arg.toLowerCase(Locale.ENGLISH).endsWith(".properties")) - { - try (InputStream inputStream = Resource.newResource(arg).getInputStream()) - { - properties.load(inputStream); - } + properties.load(inputStream); } } + } - // For all arguments, parse XMLs - XmlConfiguration last = null; - List objects = new ArrayList<>(args.length); - for (String arg : args) + // For all arguments, parse XMLs + XmlConfiguration last = null; + List objects = new ArrayList<>(args.length); + for (String arg : args) + { + if (!arg.toLowerCase(Locale.ENGLISH).endsWith(".properties") && (arg.indexOf('=') < 0)) { - if (!arg.toLowerCase(Locale.ENGLISH).endsWith(".properties") && (arg.indexOf('=') < 0)) + XmlConfiguration configuration = new XmlConfiguration(Resource.newResource(arg)); + if (last != null) + configuration.getIdMap().putAll(last.getIdMap()); + if (properties.size() > 0) { - XmlConfiguration configuration = new XmlConfiguration(Resource.newResource(arg)); - if (last != null) - configuration.getIdMap().putAll(last.getIdMap()); - if (properties.size() > 0) - { - Map props = new HashMap<>(); - properties.entrySet().stream() - .forEach(objectObjectEntry -> props.put(objectObjectEntry.getKey().toString(), - String.valueOf(objectObjectEntry.getValue()))); - configuration.getProperties().putAll(props); - } - - Object obj = configuration.configure(); - if (obj != null && !objects.contains(obj)) - objects.add(obj); - last = configuration; + Map props = new HashMap<>(); + properties.forEach((key, value) -> props.put(key.toString(), + String.valueOf(value))); + configuration.getProperties().putAll(props); } + + Object obj = configuration.configure(); + if (obj != null && !objects.contains(obj)) + objects.add(obj); + last = configuration; } + } - if (LOG.isDebugEnabled()) - LOG.debug("objects={}", objects); + if (LOG.isDebugEnabled()) + LOG.debug("objects={}", objects); - // For all objects created by XmlConfigurations, start them if they are lifecycles. - List started = new ArrayList<>(objects.size()); - for (Object obj : objects) + // For all objects created by XmlConfigurations, start them if they are lifecycles. + List started = new ArrayList<>(objects.size()); + for (Object obj : objects) + { + if (obj instanceof LifeCycle) { - if (obj instanceof LifeCycle) + LifeCycle lc = (LifeCycle)obj; + if (!lc.isRunning()) { - LifeCycle lc = (LifeCycle)obj; - if (!lc.isRunning()) + lc.start(); + if (lc.isStarted()) + started.add(lc); + else { - lc.start(); - if (lc.isStarted()) - started.add(lc); - else + // Failed to start a component, so stop all started components + Collections.reverse(started); + for (LifeCycle slc : started) { - // Failed to start a component, so stop all started components - Collections.reverse(started); - for (LifeCycle slc : started) - { - slc.stop(); - } - break; + slc.stop(); } + break; } } } - return null; - }); + } } catch (Error | Exception e) {