Chris Hegarty created LOG4J2-3236:
-------------------------------------
Summary: 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: 2.16.0, 3.0.0
Reporter: Chris Hegarty
During upgrade of log4j in Elasticsearch (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}
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
