[LOG4J2-3236] Improve privileged access to parent class loader in LoaderUtil

• Type: Bug
• Status: Open
• Priority: Major
• Resolution: Unresolved
• Affects Version/s: 3.0.0, 2.16.0
• Component/s: Core
• Assignee: Unassigned
• Reporter: Chris Hegarty
• Created: 15/Dec/21 16:04
• Updated: 01/Dec/23 22:08

Description

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:

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

$ 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 "*", "*";
};

Run the test prog with the policy set:

$ 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

Proposed fix:

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

[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