From 8255ffad43d0864a9a312d0deb821fd144747ea4 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 29 Dec 2023 14:55:15 -0600 Subject: [PATCH 1/2] Fix use of AccessController in LoaderUtil This fixes issue #2129 where `AccessController::doPrivileged` is needlessly invoked when no `SecurityManager` is installed. --- .../apache/logging/log4j/util/LoaderUtil.java | 38 ++++++++++--------- ...fix_security_manager_use_in_LoaderUtil.xml | 27 +++++++++++++ 2 files changed, 48 insertions(+), 17 deletions(-) create mode 100644 src/changelog/.2.x.x/fix_security_manager_use_in_LoaderUtil.xml 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 dc7fd4ee00b..1b8cc72b9f4 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 @@ -65,13 +65,10 @@ public final class LoaderUtil { } catch (final SecurityException ignored) { try { // let's see if we can obtain that permission - AccessController.doPrivileged( - (PrivilegedAction) () -> { - AccessController.checkPermission(GET_CLASS_LOADER); - return null; - }, - null, - GET_CLASS_LOADER); + runPrivilegedActionWithGetClassLoaderPermission((PrivilegedAction) () -> { + AccessController.checkPermission(GET_CLASS_LOADER); + return null; + }); getClassLoaderDisabled = false; } catch (final SecurityException ignore) { // no chance @@ -108,7 +105,7 @@ public static ClassLoader getClassLoader(final Class class1, final Class c } return isChild(loader1, loader2) ? loader1 : loader2; }; - return AccessController.doPrivileged(action, null, GET_CLASS_LOADER); + return runActionInvolvingGetClassLoaderPermission(action); } /** @@ -143,22 +140,29 @@ private static boolean isChild(final ClassLoader loader1, final ClassLoader load * @return the current thread's ClassLoader, a fallback loader, or null if no fallback can be determined */ public static ClassLoader getThreadContextClassLoader() { - if (GET_CLASS_LOADER_DISABLED) { - // we can at least get this class's ClassLoader regardless of security context - // however, if this is null, there's really no option left at this point - try { - return getThisClassLoader(); - } catch (final SecurityException ignored) { - return null; - } + try { + return GET_CLASS_LOADER_DISABLED + ? getThisClassLoader() + : runActionInvolvingGetClassLoaderPermission(TCCL_GETTER); + } catch (final SecurityException ignored) { + return null; } - return AccessController.doPrivileged(TCCL_GETTER, null, GET_CLASS_LOADER); } private static ClassLoader getThisClassLoader() { return LoaderUtil.class.getClassLoader(); } + private static T runActionInvolvingGetClassLoaderPermission(final PrivilegedAction action) { + return System.getSecurityManager() != null + ? runPrivilegedActionWithGetClassLoaderPermission(action) + : action.run(); + } + + private static T runPrivilegedActionWithGetClassLoaderPermission(final PrivilegedAction action) { + return AccessController.doPrivileged(action, null, GET_CLASS_LOADER); + } + private static class ThreadContextClassLoaderGetter implements PrivilegedAction { @Override public ClassLoader run() { diff --git a/src/changelog/.2.x.x/fix_security_manager_use_in_LoaderUtil.xml b/src/changelog/.2.x.x/fix_security_manager_use_in_LoaderUtil.xml new file mode 100644 index 00000000000..94bb2ea47b8 --- /dev/null +++ b/src/changelog/.2.x.x/fix_security_manager_use_in_LoaderUtil.xml @@ -0,0 +1,27 @@ + + + + + + Fixed use of `SecurityManager` in `LoaderUtil` where `AccessController::doPrivileged` should only be invoked when + a `SecurityManager` is installed. Some runtimes do not seem to have this method available. + + From 6dafba368bae291f390d42731d5b6675446f9431 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Sat, 30 Dec 2023 17:49:16 -0600 Subject: [PATCH 2/2] Add test and use older API Turns out Android doesn't support Java 8 all that much. --- .../util/LoaderUtilSecurityManagerTest.java | 47 ++++++++++++ .../apache/logging/log4j/util/LoaderUtil.java | 71 ++++++++----------- src/site/_release-notes/_2.x.x.adoc | 5 ++ 3 files changed, 81 insertions(+), 42 deletions(-) create mode 100644 log4j-api-test/src/test/java/org/apache/logging/log4j/util/LoaderUtilSecurityManagerTest.java diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/util/LoaderUtilSecurityManagerTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/util/LoaderUtilSecurityManagerTest.java new file mode 100644 index 00000000000..beb7cc345b6 --- /dev/null +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/util/LoaderUtilSecurityManagerTest.java @@ -0,0 +1,47 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.logging.log4j.util; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertFalse; + +import java.security.Permission; +import org.apache.logging.log4j.test.junit.SecurityManagerTestRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.jupiter.api.parallel.ResourceLock; + +@ResourceLock("java.lang.SecurityManager") +public class LoaderUtilSecurityManagerTest { + @Rule + public final SecurityManagerTestRule rule = new SecurityManagerTestRule(new TestSecurityManager()); + + private static class TestSecurityManager extends SecurityManager { + @Override + public void checkPermission(final Permission perm) { + if (perm.equals(LoaderUtil.GET_CLASS_LOADER)) { + throw new SecurityException("disabled"); + } + } + } + + @Test + public void canGetClassLoaderThroughPrivileges() { + assertFalse(LoaderUtil.GET_CLASS_LOADER_DISABLED); + assertDoesNotThrow(() -> LoaderUtil.getClassLoader(LoaderUtilSecurityManagerTest.class, String.class)); + } +} 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 1b8cc72b9f4..51ad5675eac 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 @@ -50,36 +50,31 @@ public final class LoaderUtil { // wants to use PropertiesUtil, but then PropertiesUtil wants to use LoaderUtil. private static Boolean ignoreTCCL; - private static final RuntimePermission GET_CLASS_LOADER = new RuntimePermission("getClassLoader"); - private static final boolean GET_CLASS_LOADER_DISABLED; - - private static final PrivilegedAction TCCL_GETTER = new ThreadContextClassLoaderGetter(); - - static { - if (System.getSecurityManager() != null) { - boolean getClassLoaderDisabled; + static final RuntimePermission GET_CLASS_LOADER = new RuntimePermission("getClassLoader"); + static final LazyBoolean GET_CLASS_LOADER_DISABLED = new LazyBoolean(() -> { + if (System.getSecurityManager() == null) { + return false; + } + try { + AccessController.checkPermission(GET_CLASS_LOADER); + // seems like we'll be ok + return false; + } catch (final SecurityException ignored) { try { - AccessController.checkPermission(GET_CLASS_LOADER); - // seems like we'll be ok - getClassLoaderDisabled = false; - } catch (final SecurityException ignored) { - try { - // let's see if we can obtain that permission - runPrivilegedActionWithGetClassLoaderPermission((PrivilegedAction) () -> { - AccessController.checkPermission(GET_CLASS_LOADER); - return null; - }); - getClassLoaderDisabled = false; - } catch (final SecurityException ignore) { - // no chance - getClassLoaderDisabled = true; - } + // let's see if we can obtain that permission + AccessController.doPrivileged((PrivilegedAction) () -> { + AccessController.checkPermission(GET_CLASS_LOADER); + return null; + }); + return false; + } catch (final SecurityException ignore) { + // no chance + return true; } - GET_CLASS_LOADER_DISABLED = getClassLoaderDisabled; - } else { - GET_CLASS_LOADER_DISABLED = false; } - } + }); + + private static final PrivilegedAction TCCL_GETTER = new ThreadContextClassLoaderGetter(); private LoaderUtil() {} @@ -97,7 +92,7 @@ public static ClassLoader getClassLoader(final Class class1, final Class c PrivilegedAction action = () -> { final ClassLoader loader1 = class1 == null ? null : class1.getClassLoader(); final ClassLoader loader2 = class2 == null ? null : class2.getClassLoader(); - final ClassLoader referenceLoader = GET_CLASS_LOADER_DISABLED + final ClassLoader referenceLoader = GET_CLASS_LOADER_DISABLED.getAsBoolean() ? getThisClassLoader() : Thread.currentThread().getContextClassLoader(); if (isChild(referenceLoader, loader1)) { @@ -105,7 +100,7 @@ public static ClassLoader getClassLoader(final Class class1, final Class c } return isChild(loader1, loader2) ? loader1 : loader2; }; - return runActionInvolvingGetClassLoaderPermission(action); + return runPrivileged(action); } /** @@ -141,9 +136,7 @@ private static boolean isChild(final ClassLoader loader1, final ClassLoader load */ public static ClassLoader getThreadContextClassLoader() { try { - return GET_CLASS_LOADER_DISABLED - ? getThisClassLoader() - : runActionInvolvingGetClassLoaderPermission(TCCL_GETTER); + return GET_CLASS_LOADER_DISABLED.getAsBoolean() ? getThisClassLoader() : runPrivileged(TCCL_GETTER); } catch (final SecurityException ignored) { return null; } @@ -153,14 +146,8 @@ private static ClassLoader getThisClassLoader() { return LoaderUtil.class.getClassLoader(); } - private static T runActionInvolvingGetClassLoaderPermission(final PrivilegedAction action) { - return System.getSecurityManager() != null - ? runPrivilegedActionWithGetClassLoaderPermission(action) - : action.run(); - } - - private static T runPrivilegedActionWithGetClassLoaderPermission(final PrivilegedAction action) { - return AccessController.doPrivileged(action, null, GET_CLASS_LOADER); + private static T runPrivileged(final PrivilegedAction action) { + return System.getSecurityManager() != null ? AccessController.doPrivileged(action) : action.run(); } private static class ThreadContextClassLoaderGetter implements PrivilegedAction { @@ -171,7 +158,7 @@ public ClassLoader run() { return contextClassLoader; } final ClassLoader thisClassLoader = getThisClassLoader(); - if (thisClassLoader != null || GET_CLASS_LOADER_DISABLED) { + if (thisClassLoader != null || GET_CLASS_LOADER_DISABLED.getAsBoolean()) { return thisClassLoader; } return ClassLoader.getSystemClassLoader(); @@ -476,7 +463,7 @@ static Collection findUrlResources(final String resource, final boo final ClassLoader[] candidates = { useTccl ? getThreadContextClassLoader() : null, LoaderUtil.class.getClassLoader(), - GET_CLASS_LOADER_DISABLED ? null : ClassLoader.getSystemClassLoader() + GET_CLASS_LOADER_DISABLED.getAsBoolean() ? null : ClassLoader.getSystemClassLoader() }; // @formatter:on final Collection resources = new LinkedHashSet<>(); diff --git a/src/site/_release-notes/_2.x.x.adoc b/src/site/_release-notes/_2.x.x.adoc index fd378052224..f8efa5c3eee 100644 --- a/src/site/_release-notes/_2.x.x.adoc +++ b/src/site/_release-notes/_2.x.x.adoc @@ -38,6 +38,11 @@ This releases contains ... * Deprecated the `RingBufferLogEventHandler` class for removal from the public API in 3.x. +[#release-notes-2-x-x-fixed] +=== Fixed + +* Fixed use of `SecurityManager` in `LoaderUtil` where `AccessController::doPrivileged` should only be invoked when a `SecurityManager` is installed. Some runtimes do not seem to have this method available. (https://github.com/apache/logging-log4j2/issues/2129[2129]) + [#release-notes-2-x-x-updated] === Updated