Skip to content

Commit

Permalink
8297451: ProcessHandleImpl should assert privilege when modifying rea…
Browse files Browse the repository at this point in the history
…per thread

Reviewed-by: chegar, alanb
  • Loading branch information
rjernst authored and ChrisHegarty committed Nov 26, 2022
1 parent 99d3840 commit 50f9043
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 7 deletions.
25 changes: 21 additions & 4 deletions src/java.base/share/classes/java/lang/ProcessHandleImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ final class ProcessHandleImpl implements ProcessHandle {
ThreadFactory threadFactory = grimReaper -> {
Thread t = InnocuousThread.newSystemThread("process reaper", grimReaper,
stackSize, Thread.MAX_PRIORITY);
t.setDaemon(true);
privilegedThreadSetDaemon(t, true);
return t;
};

Expand All @@ -115,6 +115,22 @@ private static class ExitCompletion extends CompletableFuture<Integer> {
}
}

@SuppressWarnings("removal")
private static void privilegedThreadSetName(Thread thread, String name) {
AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
thread.setName(name);
return null;
});
}

@SuppressWarnings("removal")
private static void privilegedThreadSetDaemon(Thread thread, boolean on) {
AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
thread.setDaemon(on);
return null;
});
}

/**
* Returns a CompletableFuture that completes with process exit status when
* the process completes.
Expand All @@ -140,8 +156,9 @@ static CompletableFuture<Integer> completion(long pid, boolean shouldReap) {
processReaperExecutor.execute(new Runnable() {
// Use inner class to avoid lambda stack overhead
public void run() {
String threadName = Thread.currentThread().getName();
Thread.currentThread().setName("process reaper (pid " + pid + ")");
Thread t = Thread.currentThread();
String threadName = t.getName();
privilegedThreadSetName(t, "process reaper (pid " + pid + ")");
try {
int exitValue = waitForProcessExit0(pid, shouldReap);
if (exitValue == NOT_A_CHILD) {
Expand Down Expand Up @@ -172,7 +189,7 @@ public void run() {
completions.remove(pid, newCompletion);
} finally {
// Restore thread name
Thread.currentThread().setName(threadName);
privilegedThreadSetName(t, threadName);
}
}
});
Expand Down
18 changes: 15 additions & 3 deletions test/jdk/java/lang/ProcessBuilder/SecurityManagerClinit.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

/*
* @test
* @bug 6980747
* @bug 6980747 8297451
* @summary Check that Process-related classes have the proper
* doPrivileged blocks, and can be initialized with an adversarial
* security manager.
Expand Down Expand Up @@ -52,6 +52,17 @@ public boolean implies(ProtectionDomain pd, Permission p) {
}
}

// Security manager that unconditionally performs Thread Modify Access checks.
@SuppressWarnings("removal")
private static class TMACSecurityManager extends SecurityManager {
static final RuntimePermission MODIFY_THREAD_PERMISSION =
new RuntimePermission("modifyThread");
@Override
public void checkAccess(Thread thread) {
checkPermission(MODIFY_THREAD_PERMISSION);
}
}

public static void main(String[] args) throws Throwable {
String javaExe =
System.getProperty("java.home") +
Expand All @@ -60,10 +71,11 @@ public static void main(String[] args) throws Throwable {
final SimplePolicy policy =
new SimplePolicy
(new FilePermission("<<ALL FILES>>", "execute"),
new RuntimePermission("setSecurityManager"));
new RuntimePermission("setSecurityManager"),
new RuntimePermission("modifyThread"));
Policy.setPolicy(policy);

System.setSecurityManager(new SecurityManager());
System.setSecurityManager(new TMACSecurityManager());

try {
String[] cmd = { javaExe, "-version" };
Expand Down

3 comments on commit 50f9043

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisHegarty
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/backport jdk19u

@openjdk
Copy link

@openjdk openjdk bot commented on 50f9043 Nov 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisHegarty the backport was successfully created on the branch ChrisHegarty-backport-50f9043c in my personal fork of openjdk/jdk19u. To create a pull request with this backport targeting openjdk/jdk19u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 50f9043c from the openjdk/jdk repository.

The commit being backported was authored by Ryan Ernst on 26 Nov 2022 and was reviewed by Chris Hegarty and Alan Bateman.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk19u:

$ git fetch https://github.com/openjdk-bots/jdk19u ChrisHegarty-backport-50f9043c:ChrisHegarty-backport-50f9043c
$ git checkout ChrisHegarty-backport-50f9043c
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk19u ChrisHegarty-backport-50f9043c

Please sign in to comment.