浏览代码

Improve InnocuousThread permission checks handling (#91704)

on shutdown, the jdk's InnocuousThread can try to change a thread name. This requires "java.lang.RuntimePermission" "modifyThread" permission. However InnocuousThread doe not inherit any Access Control Context and therefore have no permissions. This results in AccessControlException.
This commit fixes this by skipping a check for modify thread permission if a thread is innocuous.
relates #91658 and #91650

When previously described AccessControlException is thrown, it is not being catched anywhere in the Elasticsearch, hence it ends up being handled by ElasticsearchUncaughtExceptionHandler#onNonFatalUncaught
This is being again being run by the thread [process reaper] which is an innocuous thread (jdk specific) and has no permissions. onNonFatalUncaught is trying to log a message, but this in turn requires java.lang.RuntimePermission" "getenv.*" permission. which is does not have. This again results in AccessControlException java.lang.RuntimePermission" "getenv.*"

We can fix this by executing with doPrivileged in ElasticsearchUncaughtExceptionHandler#onNonFatalUncaught and this will stop the Security Manager's walk and will have ES's global grant permissions.

closes #91650
Przemyslaw Gomulka 2 年之前
父节点
当前提交
72119f00af

+ 7 - 0
docs/changelog/91704.yaml

@@ -0,0 +1,7 @@
+pr: 91704
+summary: '`DoPrivileged` in `ElasticsearchEncaughtExceptionHandler` and check modify
+  thread'
+area: Infra/Core
+type: bug
+issues:
+ - 91650

+ 7 - 3
libs/secure-sm/src/main/java/org/elasticsearch/secure_sm/SecureSM.java

@@ -162,8 +162,12 @@ public class SecureSM extends SecurityManager {
     protected void checkThreadAccess(Thread t) {
         Objects.requireNonNull(t);
 
-        // first, check if we can modify threads at all.
-        checkPermission(MODIFY_THREAD_PERMISSION);
+        boolean targetThreadIsInnocuous = isInnocuousThread(t);
+        // we don't need to check if innocuous thread is modifying itself (like changes its name)
+        if (Thread.currentThread() != t || targetThreadIsInnocuous == false) {
+            // first, check if we can modify threads at all.
+            checkPermission(MODIFY_THREAD_PERMISSION);
+        }
 
         // check the threadgroup, if its our thread group or an ancestor, its fine.
         final ThreadGroup source = Thread.currentThread().getThreadGroup();
@@ -171,7 +175,7 @@ public class SecureSM extends SecurityManager {
 
         if (target == null) {
             return;    // its a dead thread, do nothing.
-        } else if (source.parentOf(target) == false && isInnocuousThread(t) == false) {
+        } else if (source.parentOf(target) == false && targetThreadIsInnocuous == false) {
             checkPermission(MODIFY_ARBITRARY_THREAD_PERMISSION);
         }
     }

+ 17 - 0
qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java

@@ -1221,4 +1221,21 @@ public class DockerTests extends PackagingTestCase {
         waitForElasticsearch(installation);
         assertTrue(readinessProbe(9399));
     }
+
+    public void test600Interrupt() {
+        waitForElasticsearch(installation, "elastic", PASSWORD);
+        final Result containerLogs = getContainerLogs();
+
+        assertThat("Container logs should contain starting ...", containerLogs.stdout(), containsString("starting ..."));
+
+        final List<ProcessInfo> infos = ProcessInfo.getProcessInfo(sh, "java");
+        final int maxPid = infos.stream().map(i -> i.pid()).max(Integer::compareTo).get();
+
+        sh.run("kill -int " + maxPid); // send ctrl+c to all java processes
+        final Result containerLogsAfter = getContainerLogs();
+
+        assertThat("Container logs should contain stopping ...", containerLogsAfter.stdout(), containsString("stopping ..."));
+        assertThat("No errors stdout", containerLogsAfter.stdout(), not(containsString("java.security.AccessControlException:")));
+        assertThat("No errors stderr", containerLogsAfter.stderr(), not(containsString("java.security.AccessControlException:")));
+    }
 }

+ 9 - 2
server/src/main/java/org/elasticsearch/bootstrap/ElasticsearchUncaughtExceptionHandler.java

@@ -52,12 +52,19 @@ class ElasticsearchUncaughtExceptionHandler implements Thread.UncaughtExceptionH
 
     void onFatalUncaught(final String threadName, final Throwable t) {
         final String message = "fatal error in thread [" + threadName + "], exiting";
-        logger.error(message, t);
+        logErrorMessage(t, message);
     }
 
     void onNonFatalUncaught(final String threadName, final Throwable t) {
         final String message = "uncaught exception in thread [" + threadName + "]";
-        logger.error(message, t);
+        logErrorMessage(t, message);
+    }
+
+    private void logErrorMessage(Throwable t, String message) {
+        AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
+            logger.error(message, t);
+            return null;
+        });
     }
 
     void halt(int status) {