Skip to content

Commit 983999f

Browse files
authored
Improve InnocuousThread permission checks handling backport(#91704) (#91862)
Improve InnocuousThread permission checks handling (https://github.com/elastic/elasticsearch/pull/91704[)](https://github.com/elastic/elasticsearch/commit/6a3855112cf04aade4955664646b18fe1405eb48) 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. backport(#91704) closes #91650
1 parent 549e7a9 commit 983999f

File tree

4 files changed

+41
-5
lines changed

4 files changed

+41
-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: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1096,4 +1096,22 @@ private List<String> listPlugins() {
10961096
final Installation.Executables bin = installation.executables();
10971097
return Arrays.stream(sh.run(bin.pluginTool + " list").stdout.split("\n")).collect(Collectors.toList());
10981098
}
1099+
1100+
public void test600Interrupt() throws Exception {
1101+
waitForElasticsearch(installation);
1102+
final Result containerLogs = getContainerLogs();
1103+
1104+
assertThat("Container logs should contain starting ...", containerLogs.stdout, containsString("starting ..."));
1105+
1106+
final List<String> processes = Arrays.stream(sh.run("pgrep java").stdout.split(System.lineSeparator()))
1107+
.collect(Collectors.toList());
1108+
int maxPid = processes.stream().map(i -> Integer.parseInt(i.trim())).max(Integer::compareTo).get();
1109+
1110+
sh.run("kill -int " + maxPid); // send ctrl+c to all java processes
1111+
final Result containerLogsAfter = getContainerLogs();
1112+
1113+
assertThat("Container logs should contain stopping ...", containerLogsAfter.stdout, containsString("stopping ..."));
1114+
assertThat("No errors stdout", containerLogsAfter.stdout, not(containsString("java.security.AccessControlException:")));
1115+
assertThat("No errors stderr", containerLogsAfter.stderr, not(containsString("java.security.AccessControlException:")));
1116+
}
10991117
}

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ static boolean isFatalUncaught(Throwable e) {
5353

5454
void onFatalUncaught(final String threadName, final Throwable t) {
5555
final String message = "fatal error in thread [" + threadName + "], exiting";
56-
logger.error(message, t);
56+
logErrorMessage(t, message);
5757
Terminal.DEFAULT.errorPrintln(message);
5858
t.printStackTrace(Terminal.DEFAULT.getErrorWriter());
5959
// Without a final flush, the stacktrace may not be shown before ES exits
@@ -64,13 +64,20 @@ void onFatalUncaught(final String threadName, final Throwable t) {
6464

6565
void onNonFatalUncaught(final String threadName, final Throwable t) {
6666
final String message = "uncaught exception in thread [" + threadName + "]";
67-
logger.error(message, t);
67+
logErrorMessage(t, message);
6868
Terminal.DEFAULT.errorPrintln(message);
6969
t.printStackTrace(Terminal.DEFAULT.getErrorWriter());
7070
// Without a final flush, the stacktrace may not be shown if ES goes on to exit
7171
Terminal.DEFAULT.flush();
7272
}
7373

74+
private void logErrorMessage(Throwable t, String message) {
75+
AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
76+
logger.error(message, t);
77+
return null;
78+
});
79+
}
80+
7481
void halt(int status) {
7582
AccessController.doPrivileged(new PrivilegedHaltAction(status));
7683
}

0 commit comments

Comments
 (0)