TRUNK-6713: Switch to saner DaemonThread protection scheme#6349
TRUNK-6713: Switch to saner DaemonThread protection scheme#6349ibacher wants to merge 2 commits into
Conversation
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6349 +/- ##
============================================
- Coverage 59.56% 59.44% -0.12%
+ Complexity 9563 9558 -5
============================================
Files 731 731
Lines 38385 38404 +19
Branches 5598 5589 -9
============================================
- Hits 22863 22830 -33
- Misses 13479 13539 +60
+ Partials 2043 2035 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@claude review |
There was a problem hiding this comment.
Pull request overview
This PR refactors daemon-privilege “guard” checks away from stack inspection (StackWalker) toward a capability/token-style approach (Daemon.CallerKey) that is distributed to known collaborators (e.g., HibernateContextDAO, ModuleFactory, JobRequestHandlerAdapter). It also introduces a new way to temporarily treat the current thread as a daemon context to support synchronous startup paths.
Changes:
- Introduces
Daemon.CallerKeyand replaces stack-based caller validation with key-based validation for guarded entry points (module startup, user creation, service startup, scheduled tasks). - Adds
Daemon.runInDaemonContext(...)and updatesWebDaemon.startOpenmrs(...)to use it so startup can schedule a daemon task from the servlet container thread. - Updates trusted collaborators and unit tests to pass/receive the new caller key.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/main/java/org/openmrs/web/WebDaemon.java | Uses the new “daemon context” API to schedule startup work from the container thread. |
| api/src/test/java/org/openmrs/api/context/DaemonTest.java | Updates tests for the new guarded method signatures and new error-message parameterization. |
| api/src/main/java/org/openmrs/scheduler/jobrunr/JobRequestHandlerAdapter.java | Receives and supplies the CallerKey when invoking guarded scheduled-task execution. |
| api/src/main/java/org/openmrs/module/ModuleFactory.java | Supplies a CallerKey when invoking guarded module startup. |
| api/src/main/java/org/openmrs/api/db/hibernate/HibernateContextDAO.java | Supplies a CallerKey when invoking guarded user creation. |
| api/src/main/java/org/openmrs/api/context/ServiceContext.java | Supplies a package-scoped CallerKey when invoking guarded service startup. |
| api/src/main/java/org/openmrs/api/context/Daemon.java | Adds the CallerKey mechanism, distributes it to collaborators, and adds runInDaemonContext. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static void runInDaemonContext(Runnable runnable) { | ||
| Boolean previous = isDaemonThread.get(); | ||
| isDaemonThread.set(Boolean.TRUE); | ||
| try { | ||
| runnable.run(); | ||
| } finally { | ||
| if (previous == null) { | ||
| isDaemonThread.remove(); | ||
| } else { | ||
| var callerClass = possibleFrame.get(); | ||
| return Daemon.class.equals(callerClass) || "org.openmrs.web.WebDaemon".equals(callerClass.getName()); | ||
| isDaemonThread.set(previous); | ||
| } | ||
| } else { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Confirmed exploitable, and worse than the description above suggests: Context.authenticate() and Context.hasPrivilege() (Context.java lines 336, 360, 669, 680, 738) short-circuit to full Daemon-user authority whenever Daemon.isDaemonThread() is true, with no further checks. So today, any code, on any thread, can do:
Daemon.runInDaemonContext(() -> {
// isDaemonThread() is now true on this thread; every privilege check and
// Context.authenticate() call is bypassed as "Daemon user is always authenticated"
Daemon.runNewDaemonTask(() -> attackerCode()); // runs for real on the daemon pool, same bypass applies there
});with zero use of CallerKey, no need to already be a daemon thread, and no stack/class check at all — runInDaemonContext just unconditionally flips the flag for whoever calls it. That's a strictly larger hole than the StackWalker fallback this PR removes: the old isDaemonThread() only granted this transitively when the two-frames-up caller actually was Daemon/WebDaemon by class identity, which arbitrary code couldn't spoof. A fix consistent with the rest of this PR would be to gate runInDaemonContext behind a CallerKey too, with WebDaemon registered as a collaborator (push-based, since it's a different package) the same way HibernateContextDAO/ModuleFactory/JobRequestHandlerAdapter are.
| /** | ||
| * Inner class passed to expected callers allowed to create DaemonThreads. | ||
| * | ||
| * @since 3.0.0, 2.9.0, 2.8.9 | ||
| */ |
| /** | ||
| * The capability that proves to {@link Daemon} that user creation is being requested from this | ||
| * class. | ||
| */ | ||
| private static Daemon.CallerKey daemonCallerKey; | ||
|
|
| /** | ||
| * The capability that proves to {@link Daemon} that user creation is being requested from this | ||
| * class. | ||
| */ | ||
| private static Daemon.CallerKey daemonCallerKey; | ||
|
|
| /** | ||
| * The capability that proves to {@link Daemon} that user creation is being requested from this | ||
| * class. | ||
| */ | ||
| private static Daemon.CallerKey daemonCallerKey; | ||
|
|
Description of what I changed
This switches from using a StackWalker and the general stack-based hunting we were doing to guard methods to handing out a token to the known-good callers, mirror the DaemonToken. This token, however, allows direct access to APIs other than spawning new threads, for performance critical parts of the application.
This will be backported, but the shape of the backport will vary a bit.
Issue I worked on
see https://issues.openmrs.org/browse/TRUNK-6713
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
./mvnw clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master