Avoid crashtracking temp-script path hijack#11613
Conversation
- TempLocationManager: POSIX ownership+0700 check on pre-existing dirs - Initializer: add isOwnedAndPrivate() helper (null-safe) - CrashUploaderScriptInitializer/OOMENotifierScriptInitializer: drop world-wide perms, validate pre-existing dirs/scripts, create new dirs with 0700 via Files.createDirectories+PosixFilePermissions - Add acceptance tests for both initializers and TempLocationManager Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ade7027697
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What Does This Do
Adds POSIX ownership and permission validation to the crashtracking temp-directory tree so that a pre-planted attacker-controlled script or directory cannot be silently reused.
TempLocationManager: validates every pre-existing directory frombaseTempDirdown totempDiris owned by the current JVM user and has no group/world bits (0700); throwsIllegalStateException+ emits aProfilerFlareLoggermessage on failure.Initializer: addsisOwnedAndPrivate()helper (POSIX-gated; usesTempLocationManagerto resolve the expected JVM process owner without string-based username lookup).CrashUploaderScriptInitializer/OOMENotifierScriptInitializer: drops the world-widesetReadable/setWritable/setExecutable(true, false)calls; creates new script directories with owner-only permissions viamkdirs()+setReadable/setWritable/setExecutable(true, true); validates ownership and permissions before reusing any pre-existing directory or script file.Motivation
Crashtracking is enabled by default. The agent sets HotSpot
OnError/OnOutOfMemoryErrorto execute scripts under a predictable temp path (<java.io.tmpdir>/ddprof_<user>/pid_<pid>/). A local attacker who pre-creates that directory and plantsdd_crash_uploader.shordd_oome_notifier.shthere can cause the JVM crash/OOME handler to execute attacker-controlled code as the instrumented service user.Additional Notes
Two sibling issues were fixed in the same pass:
CrashUploaderScriptInitializer: world-wide perms on script dir + unchecked reuse of pre-existing scriptOOMENotifierScriptInitializer: sameConfigManageris covered transitively — once the parent directory has a validated trust boundary, config files written into it are safe.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: APMSP-3192