From 4cc56dea06ea02d30d9319ba70650817e80ad101 Mon Sep 17 00:00:00 2001 From: Chris Hegarty <62058229+ChrisHegarty@users.noreply.github.com> Date: Sat, 18 Dec 2021 15:47:17 +0000 Subject: [PATCH] Tolerate unprivileged log4j getClassLoaders calls (#81840) Tolerate unprivileged log4j getClassLoaders calls, as if (but not exactly) like they were wrapped in doPriv. This is precautionary step as security permission exceptions have been observed during testing. This change also reverts changes to tests in the log4j 2.15 Upgrade #81709, as they should no longer be needed, given this change and changes in #81851. No explicit new test has been added in this PR, but the code in question is exercised extensively by existing tests, since the policy is set in the test framework. The test reverts mentioned above confirm that the changes are working as expected. This change is a workaround to the issue raised in log4j: https://issues.apache.org/jira/browse/LOG4J2-3236 --- .../org/elasticsearch/bootstrap/ESPolicy.java | 25 +++++++++++++++++++ .../audit/logfile/LoggingAuditTrailTests.java | 9 +------ .../sql/qa/server/multi-node/build.gradle | 5 ---- .../sql/qa/server/security/build.gradle | 3 --- .../sql/qa/server/single-node/build.gradle | 4 --- 5 files changed, 26 insertions(+), 20 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java b/server/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java index 2f824aee56afb..2ac6637021848 100644 --- a/server/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java +++ b/server/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java @@ -20,8 +20,10 @@ import java.security.Permissions; import java.security.Policy; import java.security.ProtectionDomain; +import java.util.Arrays; import java.util.Collections; import java.util.Map; +import java.util.Optional; import java.util.function.Predicate; /** custom policy for union of static and dynamic permissions */ @@ -58,6 +60,25 @@ final class ESPolicy extends Policy { this.plugins = plugins; } + private static final Predicate JDK_BOOT = f -> f.getClassLoaderName() == null; + private static final Predicate ES_BOOTSTRAP = f -> f.getClassName().startsWith("org.elasticsearch.bootstrap"); + private static final Predicate IS_LOG4J = f -> "org.apache.logging.log4j.util.LoaderUtil".equals(f.getClassName()) + && "getClassLoaders".equals(f.getMethodName()); + + /** + * Returns true if the top of the call stack has: + * 1) Only frames belonging from the JDK's boot loader or org.elasticsearch.bootstrap, followed directly by + * 2) org.apache.logging.log4j.util.LoaderUtil.getClassLoaders + */ + private static boolean isLoaderUtilGetClassLoaders() { + Optional frame = Arrays.stream(Thread.currentThread().getStackTrace()) + .dropWhile(JDK_BOOT.or(ES_BOOTSTRAP)) + .limit(1) + .findFirst() + .filter(IS_LOG4J); + return frame.isPresent(); + } + @Override @SuppressForbidden(reason = "fast equals check is desired") public boolean implies(ProtectionDomain domain, Permission permission) { @@ -101,6 +122,10 @@ public boolean implies(ProtectionDomain domain, Permission permission) { return true; } + if (permission instanceof RuntimePermission && "getClassLoader".equals(permission.getName()) && isLoaderUtilGetClassLoaders()) { + return true; + } + // otherwise defer to template + dynamic file permissions return template.implies(domain, permission) || dynamic.implies(permission) || system.implies(domain, permission); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java index 46f4b4697dd3e..e41447b1a077e 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java @@ -129,8 +129,6 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; -import java.security.AccessController; -import java.security.PrivilegedAction; import java.time.Clock; import java.util.ArrayList; import java.util.Arrays; @@ -249,12 +247,7 @@ public static void lookupPatternLayout() throws Exception { assertThat(properties.getProperty("appender.audit_rolling.layout.type"), is("PatternLayout")); final String patternLayoutFormat = properties.getProperty("appender.audit_rolling.layout.pattern"); assertThat(patternLayoutFormat, is(notNullValue())); - patternLayout = AccessController.doPrivileged( - (PrivilegedAction) () -> PatternLayout.newBuilder() - .withPattern(patternLayoutFormat) - .withCharset(StandardCharsets.UTF_8) - .build() - ); + patternLayout = PatternLayout.newBuilder().withPattern(patternLayoutFormat).withCharset(StandardCharsets.UTF_8).build(); customAnonymousUsername = randomAlphaOfLength(8); reservedRealmEnabled = randomBoolean(); } diff --git a/x-pack/plugin/sql/qa/server/multi-node/build.gradle b/x-pack/plugin/sql/qa/server/multi-node/build.gradle index 499a3907cc580..acdd38750404f 100644 --- a/x-pack/plugin/sql/qa/server/multi-node/build.gradle +++ b/x-pack/plugin/sql/qa/server/multi-node/build.gradle @@ -12,8 +12,3 @@ testClusters.matching { it.name == "integTest" }.configureEach { setting 'xpack.license.self_generated.type', 'trial' plugin ':x-pack:qa:freeze-plugin' } - -tasks.named("integTest").configure { - // Disabled because of log4j Security Manager permission issues in CLI tools - systemProperty 'tests.security.manager', 'false' -} diff --git a/x-pack/plugin/sql/qa/server/security/build.gradle b/x-pack/plugin/sql/qa/server/security/build.gradle index b3fb75eca027b..d7216d25d52b0 100644 --- a/x-pack/plugin/sql/qa/server/security/build.gradle +++ b/x-pack/plugin/sql/qa/server/security/build.gradle @@ -54,9 +54,6 @@ subprojects { "${-> testClusters.integTest.singleNode().getAuditLog()}" nonInputProperties.systemProperty 'tests.audit.yesterday.logfile', "${-> testClusters.integTest.singleNode().getAuditLog().getParentFile()}/integTest_audit-${new Date().format('yyyy-MM-dd')}-1.json.gz" - - // Disabled because of log4j Security Manager permission issues in CLI tools - systemProperty 'tests.security.manager', 'false' } tasks.named("testingConventions").configure { enabled = false } diff --git a/x-pack/plugin/sql/qa/server/single-node/build.gradle b/x-pack/plugin/sql/qa/server/single-node/build.gradle index 945fa65c048ae..8a603ceaa6739 100644 --- a/x-pack/plugin/sql/qa/server/single-node/build.gradle +++ b/x-pack/plugin/sql/qa/server/single-node/build.gradle @@ -5,7 +5,3 @@ testClusters.matching { it.name == "integTest" }.configureEach { plugin ':x-pack:qa:freeze-plugin' } -tasks.named("integTest").configure { - // Disabled because of log4j Security Manager permission issues in CLI tools - systemProperty 'tests.security.manager', 'false' -}