Skip to content

Commit eb032b9

Browse files
authored
Improve InnocuousThread permission checks handling backport(#91704) (#91849)
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 backport #91704
1 parent 495178f commit eb032b9

File tree

4 files changed

+40
-5
lines changed

4 files changed

+40
-5
lines changed

docs/changelog/91704.yaml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
pr: 91704
2+
summary: '`DoPrivileged` in `ElasticsearchEncaughtExceptionHandler` and check modify
3+
thread'
4+
area: Infra/Core
5+
type: bug
6+
issues:
7+
- 91650

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -162,16 +162,20 @@ private static boolean isInnocuousThread(Thread t) {
162162
protected void checkThreadAccess(Thread t) {
163163
Objects.requireNonNull(t);
164164

165-
// first, check if we can modify threads at all.
166-
checkPermission(MODIFY_THREAD_PERMISSION);
165+
boolean targetThreadIsInnocuous = isInnocuousThread(t);
166+
// we don't need to check if innocuous thread is modifying itself (like changes its name)
167+
if (Thread.currentThread() != t || targetThreadIsInnocuous == false) {
168+
// first, check if we can modify threads at all.
169+
checkPermission(MODIFY_THREAD_PERMISSION);
170+
}
167171

168172
// check the threadgroup, if its our thread group or an ancestor, its fine.
169173
final ThreadGroup source = Thread.currentThread().getThreadGroup();
170174
final ThreadGroup target = t.getThreadGroup();
171175

172176
if (target == null) {
173177
return; // its a dead thread, do nothing.
174-
} else if (source.parentOf(target) == false && isInnocuousThread(t) == false) {
178+
} else if (source.parentOf(target) == false && targetThreadIsInnocuous == false) {
175179
checkPermission(MODIFY_ARBITRARY_THREAD_PERMISSION);
176180
}
177181
}

qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1220,4 +1220,21 @@ public void test500Readiness() throws Exception {
12201220
waitForElasticsearch(installation);
12211221
assertTrue(readinessProbe(9399));
12221222
}
1223+
1224+
public void test600Interrupt() {
1225+
waitForElasticsearch(installation, "elastic", PASSWORD);
1226+
final Result containerLogs = getContainerLogs();
1227+
1228+
assertThat("Container logs should contain starting ...", containerLogs.stdout(), containsString("starting ..."));
1229+
1230+
final List<ProcessInfo> infos = ProcessInfo.getProcessInfo(sh, "java");
1231+
final int maxPid = infos.stream().map(i -> i.pid()).max(Integer::compareTo).get();
1232+
1233+
sh.run("kill -int " + maxPid); // send ctrl+c to all java processes
1234+
final Result containerLogsAfter = getContainerLogs();
1235+
1236+
assertThat("Container logs should contain stopping ...", containerLogsAfter.stdout(), containsString("stopping ..."));
1237+
assertThat("No errors stdout", containerLogsAfter.stdout(), not(containsString("java.security.AccessControlException:")));
1238+
assertThat("No errors stderr", containerLogsAfter.stderr(), not(containsString("java.security.AccessControlException:")));
1239+
}
12231240
}

server/src/main/java/org/elasticsearch/bootstrap/ElasticsearchUncaughtExceptionHandler.java

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,12 +52,19 @@ static boolean isFatalUncaught(Throwable e) {
5252

5353
void onFatalUncaught(final String threadName, final Throwable t) {
5454
final String message = "fatal error in thread [" + threadName + "], exiting";
55-
logger.error(message, t);
55+
logErrorMessage(t, message);
5656
}
5757

5858
void onNonFatalUncaught(final String threadName, final Throwable t) {
5959
final String message = "uncaught exception in thread [" + threadName + "]";
60-
logger.error(message, t);
60+
logErrorMessage(t, message);
61+
}
62+
63+
private void logErrorMessage(Throwable t, String message) {
64+
AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
65+
logger.error(message, t);
66+
return null;
67+
});
6168
}
6269

6370
void halt(int status) {

0 commit comments

Comments
 (0)