Skip to content

Commit

Permalink
Tolerate unprivileged log4j getClassLoaders calls (elastic#81840)
Browse files Browse the repository at this point in the history
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 elastic#81709,
as they should no longer be needed, given this change and changes in elastic#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
  • Loading branch information
ChrisHegarty authored Dec 18, 2021
1 parent 0763439 commit 4cc56de
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 20 deletions.
25 changes: 25 additions & 0 deletions server/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -58,6 +60,25 @@ final class ESPolicy extends Policy {
this.plugins = plugins;
}

private static final Predicate<StackTraceElement> JDK_BOOT = f -> f.getClassLoaderName() == null;
private static final Predicate<StackTraceElement> ES_BOOTSTRAP = f -> f.getClassName().startsWith("org.elasticsearch.bootstrap");
private static final Predicate<StackTraceElement> 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<StackTraceElement> 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) {
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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>) () -> PatternLayout.newBuilder()
.withPattern(patternLayoutFormat)
.withCharset(StandardCharsets.UTF_8)
.build()
);
patternLayout = PatternLayout.newBuilder().withPattern(patternLayoutFormat).withCharset(StandardCharsets.UTF_8).build();
customAnonymousUsername = randomAlphaOfLength(8);
reservedRealmEnabled = randomBoolean();
}
Expand Down
5 changes: 0 additions & 5 deletions x-pack/plugin/sql/qa/server/multi-node/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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'
}
3 changes: 0 additions & 3 deletions x-pack/plugin/sql/qa/server/security/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
4 changes: 0 additions & 4 deletions x-pack/plugin/sql/qa/server/single-node/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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'
}

0 comments on commit 4cc56de

Please sign in to comment.