[ https://issues.apache.org/jira/browse/LOG4J2-3236?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17460079#comment-17460079 ]
Chris Hegarty commented on LOG4J2-3236: --------------------------------------- Additionally, or alternatively, log4j could decided to catch-and-silently-discard security exceptions and just return the class loader(s) accessible to the caller. I'm happy to contribute the code for this, and/or start a PR, once an approach is agreed. > Improve privileged access to parent class loader in LoaderUtil > -------------------------------------------------------------- > > Key: LOG4J2-3236 > URL: https://issues.apache.org/jira/browse/LOG4J2-3236 > Project: Log4j 2 > Issue Type: Bug > Components: Core > Affects Versions: 3.0.0, 2.16.0 > Reporter: Chris Hegarty > Priority: Major > > During upgrade of log4j in Elasticsearch [1] (from 2.11.1 to 2.15+) it has > been noticed that there are a number of calls in LoaderUtil to > `ClassLoader::getParent`. These calls are not encapsulated in `doPrivileged` > so require an application to grant `RuntimePermission "getClassLoader"` to > many more parts of the system than should be required. This is a significant > issue for code running with a dynamic security manager (first not enabled, > then later enabled, then subsequently replaced. And with different permission > sets granted to different code bases on the stack). > While there are other areas of the log4j code base that do not appear to give > consideration to running with a security manager, LoadUtil does (to some > extent). What is proposed here is a small change that complements the use of > doPrivileged in LoaderUtil to extend it to all `ClassLoader::getParent` > calls, thus allowing an application to grant log4j that permission ( without > requiring the caller of the logger to also require permission). The changes > are also sympathetic to the fact that the security manager is dynamic, and > should be checked during the operation (rather than its presence and > permissions stored statically). > The remained of the details provided here demonstrate the issue, and also a > proposed minimal solution. > Minimal contrived test case: > {code:java} > package com.example; > import org.apache.logging.log4j.*; > import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilder; > import > org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilderFactory; > import org.apache.logging.log4j.core.config.builder.impl.BuiltConfiguration; > import org.apache.logging.log4j.core.config.Configurator; > public class Example { > private static final Logger LOGGER = LogManager.getLogger(); > public static void main(String... args) { > System.setSecurityManager(new SecurityManager()); > configureStatusLogger(); > } > private static void configureStatusLogger() { > ConfigurationBuilder<BuiltConfiguration> builder = > ConfigurationBuilderFactory.newConfigurationBuilder(); > builder.setStatusLevel(Level.ERROR); > Configurator.initialize(builder.build()); > } > {code} > {code:java} > $ cat java.policy > // replace with the location of your log4j-api jar file > grant codeBase > "file:/Users/chegar/git/logging-log4j2/log4j-api/target/log4j-api-3.0.0-SNAPSHOT.jar" > { > permission java.lang.RuntimePermission "getClassLoader"; > }; > grant { > // Permissions to allow the test to complete silently, not strictly > // relevant to the crux of the test. > permission javax.management.MBeanServerPermission "createMBeanServer"; > permission javax.management.MBeanPermission "*", "*"; > }; > {code} > Run the test prog with the policy set: > {code:java} > $ java -cp ...: -Djava.security.policy=java.policy com.example.Example > .... > Caused by: java.security.AccessControlException: access denied > ("java.lang.RuntimePermission" "getClassLoader") > at > java.base/java.security.AccessControlContext.checkPermission(AccessControlContext.java:472) > at > java.base/java.security.AccessController.checkPermission(AccessController.java:1036) > at > java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:408) > at > java.base/java.lang.ClassLoader.checkClassLoaderPermission(ClassLoader.java:2049) > at java.base/java.lang.ClassLoader.getParent(ClassLoader.java:1799) > at > org.apache.logging.log4j.util.LoaderUtil.getClassLoaders(LoaderUtil.java:159) > at > org.apache.logging.log4j.core.util.WatchManager.getEventServices(WatchManager.java:161) > at > org.apache.logging.log4j.core.util.WatchManager.<init>(WatchManager.java:137) > at > org.apache.logging.log4j.core.config.AbstractConfiguration.<init>(AbstractConfiguration.java:138) > at > org.apache.logging.log4j.core.config.builder.impl.BuiltConfiguration.<init>(BuiltConfiguration.java:55) > ... 10 more > {code} > Proposed fix: > {code:java} > $ git diff > diff --git > a/log4j-api/src/main/java/org/apache/logging/log4j/util/LoaderUtil.java > b/log4j-api/src/main/java/org/apache/logging/log4j/util/LoaderUtil.java > index 67944307e..c1afec3f3 100644 > --- a/log4j-api/src/main/java/org/apache/logging/log4j/util/LoaderUtil.java > +++ b/log4j-api/src/main/java/org/apache/logging/log4j/util/LoaderUtil.java > @@ -44,8 +44,6 @@ public final class LoaderUtil { > public static final String IGNORE_TCCL_PROPERTY = "log4j.ignoreTCL"; > public static final String FORCE_TCL_ONLY_PROPERTY = > "log4j.forceTCLOnly"; > > - private static final SecurityManager SECURITY_MANAGER = > System.getSecurityManager(); > - > // this variable must be lazily loaded; otherwise, we get a nice > circular class loading problem where LoaderUtil > // wants to use PropertiesUtil, but then PropertiesUtil wants to use > LoaderUtil. > private static Boolean ignoreTCCL; > @@ -57,11 +55,15 @@ public final class LoaderUtil { > private static final PrivilegedAction<ClassLoader> TCCL_GETTER = new > ThreadContextClassLoaderGetter(); > > static { > - if (SECURITY_MANAGER != null) { > + final SecurityManager sm = System.getSecurityManager(); > + if (sm != null) { > boolean getClassLoaderDisabled; > try { > - SECURITY_MANAGER.checkPermission(new > RuntimePermission("getClassLoader")); > - getClassLoaderDisabled = false; > + PrivilegedAction<Boolean> pa = () -> { > + sm.checkPermission(new > RuntimePermission("getClassLoader")); > + return false; > + }; > + getClassLoaderDisabled = AccessController.doPrivileged(pa); > } catch (final SecurityException ignored) { > getClassLoaderDisabled = true; > } > @@ -108,7 +110,7 @@ public final class LoaderUtil { > // however, if this is null, there's really no option left at > this point > return LoaderUtil.class.getClassLoader(); > } > - return SECURITY_MANAGER == null ? TCCL_GETTER.run() : > AccessController.doPrivileged(TCCL_GETTER); > + return System.getSecurityManager() == null ? TCCL_GETTER.run() : > AccessController.doPrivileged(TCCL_GETTER); > } > > /** > @@ -121,9 +123,9 @@ public final class LoaderUtil { > */ > private static boolean isChild(final ClassLoader loader1, final > ClassLoader loader2) { > if (loader1 != null && loader2 != null) { > - ClassLoader parent = loader1.getParent(); > + ClassLoader parent = getParentLoader(loader1); > while (parent != null && parent != loader2) { > - parent = parent.getParent(); > + parent = getParentLoader(parent); > } > // once parent is null, we're at the system CL, which would > indicate they have separate ancestry > return parent != null; > @@ -146,6 +148,19 @@ public final class LoaderUtil { > } > } > > + private static ClassLoader privilegedGetParentLoader(ClassLoader loader) > { > + PrivilegedAction<ClassLoader> pa = () -> loader.getParent(); > + return AccessController.doPrivileged(pa); > + } > + > + private static ClassLoader getParentLoader(ClassLoader loader) { > + if (System.getSecurityManager() == null) { > + return loader.getParent(); > + } else { > + return privilegedGetParentLoader(loader); > + } > + } > + > public static ClassLoader[] getClassLoaders() { > final Collection<ClassLoader> classLoaders = new LinkedHashSet<>(); > final ClassLoader tcl = getThreadContextClassLoader(); > @@ -156,7 +171,7 @@ public final class LoaderUtil { > if (layer == null) { > if (!isForceTccl()) { > accumulateClassLoaders(LoaderUtil.class.getClassLoader(), > classLoaders); > - accumulateClassLoaders(tcl == null ? null : tcl.getParent(), > classLoaders); > + accumulateClassLoaders(tcl == null ? null : > getParentLoader(tcl), classLoaders); > final ClassLoader systemClassLoader = > ClassLoader.getSystemClassLoader(); > if (systemClassLoader != null) { > classLoaders.add(systemClassLoader); > @@ -191,7 +206,7 @@ public final class LoaderUtil { > private static void accumulateClassLoaders(final ClassLoader loader, > final Collection<ClassLoader> loaders) { > // Some implementations may use null to represent the bootstrap > class loader. > if (loader != null && loaders.add(loader)) { > - accumulateClassLoaders(loader.getParent(), loaders); > + accumulateClassLoaders(getParentLoader(loader), loaders); > } > } > {code} > [1] > https://github.com/elastic/elasticsearch/pull/47298#issuecomment-540754371 > <<< this is a link to the first observed failure, but similar have been > observer in more recent upgrade attempts, for example during > https://github.com/elastic/elasticsearch/pull/81709 -- This message was sent by Atlassian Jira (v8.20.1#820001)