From 7c1154725118d1de6897402dc5d647e11eab4669 Mon Sep 17 00:00:00 2001 From: Patrick Doyle Date: Fri, 31 Jan 2025 11:01:05 -0500 Subject: [PATCH 1/6] Entitlement IT cases for reflection --- .../qa/test/DummyImplementations.java | 5 ++--- .../qa/test/RestEntitlementsCheckAction.java | 17 +++++++++++++++++ .../entitlement/qa/EntitlementsTestRule.java | 1 + 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/libs/entitlement/qa/entitlement-test-plugin/src/main/java/org/elasticsearch/entitlement/qa/test/DummyImplementations.java b/libs/entitlement/qa/entitlement-test-plugin/src/main/java/org/elasticsearch/entitlement/qa/test/DummyImplementations.java index 6564e0eed41e1..2169b60df21c5 100644 --- a/libs/entitlement/qa/entitlement-test-plugin/src/main/java/org/elasticsearch/entitlement/qa/test/DummyImplementations.java +++ b/libs/entitlement/qa/entitlement-test-plugin/src/main/java/org/elasticsearch/entitlement/qa/test/DummyImplementations.java @@ -52,10 +52,9 @@ *

* A bit like Mockito but way more painful. */ -class DummyImplementations { - - static class DummyLocaleServiceProvider extends LocaleServiceProvider { +public class DummyImplementations { + public static class DummyLocaleServiceProvider extends LocaleServiceProvider { @Override public Locale[] getAvailableLocales() { throw unexpected(); diff --git a/libs/entitlement/qa/entitlement-test-plugin/src/main/java/org/elasticsearch/entitlement/qa/test/RestEntitlementsCheckAction.java b/libs/entitlement/qa/entitlement-test-plugin/src/main/java/org/elasticsearch/entitlement/qa/test/RestEntitlementsCheckAction.java index dfca49d122673..2581593b730f3 100644 --- a/libs/entitlement/qa/entitlement-test-plugin/src/main/java/org/elasticsearch/entitlement/qa/test/RestEntitlementsCheckAction.java +++ b/libs/entitlement/qa/entitlement-test-plugin/src/main/java/org/elasticsearch/entitlement/qa/test/RestEntitlementsCheckAction.java @@ -96,6 +96,9 @@ static CheckAction alwaysDenied(CheckedRunnable action) { private static final Map checkActions = Stream.concat( Stream.>of( + entry("static_reflection", deniedToPlugins(RestEntitlementsCheckAction::staticMethodNeverEntitledViaReflection)), + entry("nonstatic_reflection", deniedToPlugins(RestEntitlementsCheckAction::nonstaticMethodNeverEntitledViaReflection)), + entry("constructor_reflection", deniedToPlugins(RestEntitlementsCheckAction::constructorNeverEntitledViaReflection)), entry("runtime_exit", deniedToPlugins(RestEntitlementsCheckAction::runtimeExit)), entry("runtime_halt", deniedToPlugins(RestEntitlementsCheckAction::runtimeHalt)), entry("system_exit", deniedToPlugins(RestEntitlementsCheckAction::systemExit)), @@ -338,6 +341,11 @@ private static void systemExit() { System.exit(123); } + private static void staticMethodNeverEntitledViaReflection() throws Exception { + Method systemExit = System.class.getMethod("exit", int.class); + systemExit.invoke(null, 123); + } + private static void createClassLoader() throws IOException { try (var classLoader = new URLClassLoader("test", new URL[0], RestEntitlementsCheckAction.class.getClassLoader())) { logger.info("Created URLClassLoader [{}]", classLoader.getName()); @@ -348,6 +356,11 @@ private static void processBuilder_start() throws IOException { new ProcessBuilder("").start(); } + private static void nonstaticMethodNeverEntitledViaReflection() throws Exception { + Method processBuilderStart = ProcessBuilder.class.getMethod("start"); + processBuilderStart.invoke(new ProcessBuilder("")); + } + private static void processBuilder_startPipeline() throws IOException { ProcessBuilder.startPipeline(List.of()); } @@ -386,6 +399,10 @@ private static void setHttpsConnectionProperties() { new DummyLocaleServiceProvider(); } + private static void constructorNeverEntitledViaReflection() throws Exception { + DummyLocaleServiceProvider.class.getConstructor().newInstance(); + } + private static void breakIteratorProvider$() { new DummyBreakIteratorProvider(); } diff --git a/libs/entitlement/qa/src/javaRestTest/java/org/elasticsearch/entitlement/qa/EntitlementsTestRule.java b/libs/entitlement/qa/src/javaRestTest/java/org/elasticsearch/entitlement/qa/EntitlementsTestRule.java index 33d5eeca595ab..719283cbbc2bb 100644 --- a/libs/entitlement/qa/src/javaRestTest/java/org/elasticsearch/entitlement/qa/EntitlementsTestRule.java +++ b/libs/entitlement/qa/src/javaRestTest/java/org/elasticsearch/entitlement/qa/EntitlementsTestRule.java @@ -56,6 +56,7 @@ protected void before() throws Throwable { .systemProperty("es.entitlements.enabled", "true") .systemProperty("es.entitlements.testdir", () -> testDir.getRoot().getAbsolutePath()) .setting("xpack.security.enabled", "false") + .setting("logger.org.elasticsearch.entitlement", "TRACE") .build(); ruleChain = RuleChain.outerRule(testDir).around(tempDirSetup).around(cluster); } From b0c20973cb1d937f967b17214cdfac0c439851b7 Mon Sep 17 00:00:00 2001 From: Patrick Doyle Date: Fri, 31 Jan 2025 11:10:13 -0500 Subject: [PATCH 2/6] EntitlementBootstrap selfTest using reflection --- .../bootstrap/EntitlementBootstrap.java | 34 +++++++++++++++---- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java index e7312103f9921..f7463949f4ed9 100644 --- a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java +++ b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java @@ -22,8 +22,10 @@ import org.elasticsearch.logging.Logger; import java.io.IOException; +import java.lang.reflect.InvocationTargetException; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.attribute.FileAttribute; import java.util.Map; import java.util.function.Function; @@ -144,19 +146,33 @@ private static String findAgentJar() { * @throws IllegalStateException if the entitlements system can't prevent an unauthorized action of our choosing */ private static void selfTest() { - ensureCannotStartProcess(); - ensureCanCreateTempFile(); + ensureCannotStartProcess(false); + ensureCannotStartProcess(true); + ensureCanCreateTempFile(false); + ensureCanCreateTempFile(true); } - private static void ensureCannotStartProcess() { + private static void ensureCannotStartProcess(boolean useReflection) { try { // The command doesn't matter; it doesn't even need to exist - new ProcessBuilder("").start(); + ProcessBuilder builder = new ProcessBuilder(""); + if (useReflection) { + try { + var start = ProcessBuilder.class.getMethod("start"); + start.invoke(builder); + } catch (InvocationTargetException e) { + throw e.getCause(); + } + } else { + builder.start(); + } } catch (NotEntitledException e) { logger.debug("Success: Entitlement protection correctly prevented process creation"); return; } catch (IOException e) { throw new IllegalStateException("Failed entitlement protection self-test", e); + } catch (Throwable e) { + throw new IllegalStateException("Error during entitlement protection self-test", e); } throw new IllegalStateException("Entitlement protection self-test was incorrectly permitted"); } @@ -165,9 +181,15 @@ private static void ensureCannotStartProcess() { * Originally {@code Security.selfTest}. */ @SuppressForbidden(reason = "accesses jvm default tempdir as a self-test") - private static void ensureCanCreateTempFile() { + private static void ensureCanCreateTempFile(boolean useReflection) { try { - Path p = Files.createTempFile(null, null); + Path p; + if (useReflection) { + p = (Path) Files.class.getMethod("createTempFile", String.class, String.class, FileAttribute[].class) + .invoke(null, null, null, new FileAttribute[0]); + } else { + p = Files.createTempFile(null, null); + } p.toFile().deleteOnExit(); // Make an effort to clean up the file immediately; also, deleteOnExit leaves the file if the JVM exits abnormally. From 7b234c9f3090ea8151935fefda0474995acbe6ec Mon Sep 17 00:00:00 2001 From: Patrick Doyle Date: Fri, 31 Jan 2025 11:36:41 -0500 Subject: [PATCH 3/6] Remove errant logging setting --- .../org/elasticsearch/entitlement/qa/EntitlementsTestRule.java | 1 - 1 file changed, 1 deletion(-) diff --git a/libs/entitlement/qa/src/javaRestTest/java/org/elasticsearch/entitlement/qa/EntitlementsTestRule.java b/libs/entitlement/qa/src/javaRestTest/java/org/elasticsearch/entitlement/qa/EntitlementsTestRule.java index 719283cbbc2bb..33d5eeca595ab 100644 --- a/libs/entitlement/qa/src/javaRestTest/java/org/elasticsearch/entitlement/qa/EntitlementsTestRule.java +++ b/libs/entitlement/qa/src/javaRestTest/java/org/elasticsearch/entitlement/qa/EntitlementsTestRule.java @@ -56,7 +56,6 @@ protected void before() throws Throwable { .systemProperty("es.entitlements.enabled", "true") .systemProperty("es.entitlements.testdir", () -> testDir.getRoot().getAbsolutePath()) .setting("xpack.security.enabled", "false") - .setting("logger.org.elasticsearch.entitlement", "TRACE") .build(); ruleChain = RuleChain.outerRule(testDir).around(tempDirSetup).around(cluster); } From 1357446cb1cb1f133d9df628361abed9609987e5 Mon Sep 17 00:00:00 2001 From: Patrick Doyle Date: Fri, 31 Jan 2025 11:49:27 -0500 Subject: [PATCH 4/6] Lambdas instead of booleans --- .../elasticsearch/core/CheckedSupplier.java | 18 +++++++ .../bootstrap/EntitlementBootstrap.java | 48 ++++++++----------- 2 files changed, 39 insertions(+), 27 deletions(-) create mode 100644 libs/core/src/main/java/org/elasticsearch/core/CheckedSupplier.java diff --git a/libs/core/src/main/java/org/elasticsearch/core/CheckedSupplier.java b/libs/core/src/main/java/org/elasticsearch/core/CheckedSupplier.java new file mode 100644 index 0000000000000..5d3831881f285 --- /dev/null +++ b/libs/core/src/main/java/org/elasticsearch/core/CheckedSupplier.java @@ -0,0 +1,18 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +package org.elasticsearch.core; + +/** + * A {@link java.util.function.Supplier}-like interface which allows throwing checked exceptions. + */ +@FunctionalInterface +public interface CheckedSupplier { + T get() throws E; +} diff --git a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java index f7463949f4ed9..b0bcb36a986d6 100644 --- a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java +++ b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java @@ -14,6 +14,8 @@ import com.sun.tools.attach.AttachNotSupportedException; import com.sun.tools.attach.VirtualMachine; +import org.elasticsearch.core.CheckedConsumer; +import org.elasticsearch.core.CheckedSupplier; import org.elasticsearch.core.SuppressForbidden; import org.elasticsearch.entitlement.initialization.EntitlementInitialization; import org.elasticsearch.entitlement.runtime.api.NotEntitledException; @@ -146,33 +148,31 @@ private static String findAgentJar() { * @throws IllegalStateException if the entitlements system can't prevent an unauthorized action of our choosing */ private static void selfTest() { - ensureCannotStartProcess(false); - ensureCannotStartProcess(true); - ensureCanCreateTempFile(false); - ensureCanCreateTempFile(true); + ensureCannotStartProcess(ProcessBuilder::start); + ensureCanCreateTempFile(() -> Files.createTempFile(null, null)); + + // Try again with reflection + ensureCannotStartProcess(pb -> { + try { + var start = ProcessBuilder.class.getMethod("start"); + start.invoke(pb); + } catch (InvocationTargetException e) { + throw (Exception)e.getCause(); + } + }); + ensureCanCreateTempFile(() -> (Path) Files.class.getMethod("createTempFile", String.class, String.class, FileAttribute[].class) + .invoke(null, null, null, new FileAttribute[0])); } - private static void ensureCannotStartProcess(boolean useReflection) { + private static void ensureCannotStartProcess(CheckedConsumer startProcess) { try { // The command doesn't matter; it doesn't even need to exist - ProcessBuilder builder = new ProcessBuilder(""); - if (useReflection) { - try { - var start = ProcessBuilder.class.getMethod("start"); - start.invoke(builder); - } catch (InvocationTargetException e) { - throw e.getCause(); - } - } else { - builder.start(); - } + startProcess.accept(new ProcessBuilder("")); } catch (NotEntitledException e) { logger.debug("Success: Entitlement protection correctly prevented process creation"); return; - } catch (IOException e) { + } catch (Exception e) { throw new IllegalStateException("Failed entitlement protection self-test", e); - } catch (Throwable e) { - throw new IllegalStateException("Error during entitlement protection self-test", e); } throw new IllegalStateException("Entitlement protection self-test was incorrectly permitted"); } @@ -181,15 +181,9 @@ private static void ensureCannotStartProcess(boolean useReflection) { * Originally {@code Security.selfTest}. */ @SuppressForbidden(reason = "accesses jvm default tempdir as a self-test") - private static void ensureCanCreateTempFile(boolean useReflection) { + private static void ensureCanCreateTempFile(CheckedSupplier createTempFile) { try { - Path p; - if (useReflection) { - p = (Path) Files.class.getMethod("createTempFile", String.class, String.class, FileAttribute[].class) - .invoke(null, null, null, new FileAttribute[0]); - } else { - p = Files.createTempFile(null, null); - } + Path p = createTempFile.get(); p.toFile().deleteOnExit(); // Make an effort to clean up the file immediately; also, deleteOnExit leaves the file if the JVM exits abnormally. From 5ea71b49f4f4c5778d92e25473242e58a90da0d8 Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Fri, 31 Jan 2025 16:56:05 +0000 Subject: [PATCH 5/6] [CI] Auto commit changes from spotless --- .../entitlement/bootstrap/EntitlementBootstrap.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java index b0bcb36a986d6..bdf0083dc3bcd 100644 --- a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java +++ b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java @@ -157,11 +157,13 @@ private static void selfTest() { var start = ProcessBuilder.class.getMethod("start"); start.invoke(pb); } catch (InvocationTargetException e) { - throw (Exception)e.getCause(); + throw (Exception) e.getCause(); } }); - ensureCanCreateTempFile(() -> (Path) Files.class.getMethod("createTempFile", String.class, String.class, FileAttribute[].class) - .invoke(null, null, null, new FileAttribute[0])); + ensureCanCreateTempFile( + () -> (Path) Files.class.getMethod("createTempFile", String.class, String.class, FileAttribute[].class) + .invoke(null, null, null, new FileAttribute[0]) + ); } private static void ensureCannotStartProcess(CheckedConsumer startProcess) { From 9fcdb668269ad4c591c8a257960e30c11dd4090c Mon Sep 17 00:00:00 2001 From: Patrick Doyle Date: Fri, 31 Jan 2025 12:01:38 -0500 Subject: [PATCH 6/6] Refactor: Extract lambdas to method refs --- .../bootstrap/EntitlementBootstrap.java | 37 +++++++++++-------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java index b0bcb36a986d6..b433c69289d94 100644 --- a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java +++ b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java @@ -149,19 +149,11 @@ private static String findAgentJar() { */ private static void selfTest() { ensureCannotStartProcess(ProcessBuilder::start); - ensureCanCreateTempFile(() -> Files.createTempFile(null, null)); + ensureCanCreateTempFile(EntitlementBootstrap::createTempFile); // Try again with reflection - ensureCannotStartProcess(pb -> { - try { - var start = ProcessBuilder.class.getMethod("start"); - start.invoke(pb); - } catch (InvocationTargetException e) { - throw (Exception)e.getCause(); - } - }); - ensureCanCreateTempFile(() -> (Path) Files.class.getMethod("createTempFile", String.class, String.class, FileAttribute[].class) - .invoke(null, null, null, new FileAttribute[0])); + ensureCannotStartProcess(EntitlementBootstrap::reflectiveStartProcess); + ensureCanCreateTempFile(EntitlementBootstrap::reflectiveCreateTempFile); } private static void ensureCannotStartProcess(CheckedConsumer startProcess) { @@ -177,10 +169,6 @@ private static void ensureCannotStartProcess(CheckedConsumer throw new IllegalStateException("Entitlement protection self-test was incorrectly permitted"); } - /** - * Originally {@code Security.selfTest}. - */ - @SuppressForbidden(reason = "accesses jvm default tempdir as a self-test") private static void ensureCanCreateTempFile(CheckedSupplier createTempFile) { try { Path p = createTempFile.get(); @@ -200,5 +188,24 @@ private static void ensureCanCreateTempFile(CheckedSupplier createTempF logger.debug("Success: Entitlement protection correctly permitted temp file creation"); } + @SuppressForbidden(reason = "accesses jvm default tempdir as a self-test") + private static Path createTempFile() throws Exception { + return Files.createTempFile(null, null); + } + + private static void reflectiveStartProcess(ProcessBuilder pb) throws Exception { + try { + var start = ProcessBuilder.class.getMethod("start"); + start.invoke(pb); + } catch (InvocationTargetException e) { + throw (Exception) e.getCause(); + } + } + + private static Path reflectiveCreateTempFile() throws Exception { + return (Path) Files.class.getMethod("createTempFile", String.class, String.class, FileAttribute[].class) + .invoke(null, null, null, new FileAttribute[0]); + } + private static final Logger logger = LogManager.getLogger(EntitlementBootstrap.class); }