[
https://issues.apache.org/jira/browse/LOG4J2-3236?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17792214#comment-17792214
]
Matt Sicker commented on LOG4J2-3236:
-------------------------------------
Note that this must be limited to the 2.x branch as we've removed
{{SecurityManager}} support in 3.x due to the removal of {{SecurityManager}}
beginning after Java 21 (plus, you can't invoke {{System::setSecurityManager}}
anymore).
> 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 the potential for
> this issue has existed for a long time, it seems to manifest in slightly
> different and more problematic ways in recent log4j versions.
> 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.10#820010)
