Skip to content

macOS: validate target of privileged SetFileOwner (restrict to disk device nodes)#1758

Open
damianrickard wants to merge 1 commit into
veracrypt:masterfrom
damianrickard:fix/macos-setfileowner-validation
Open

macOS: validate target of privileged SetFileOwner (restrict to disk device nodes)#1758
damianrickard wants to merge 1 commit into
veracrypt:masterfrom
damianrickard:fix/macos-setfileowner-validation

Conversation

@damianrickard

Copy link
Copy Markdown

Summary

Defense-in-depth hardening for the macOS privileged helper. The elevated CoreService handler for SetFileOwnerRequest passed the client-supplied path straight to chown() running as root, with no validation — unlike the adjacent ExecuteMacOSXAPFSFormatterRequest handler, which strictly validates its device argument via IsMacOSXFormatterDevicePath().

Background

On macOS, when the user is not an administrator, the volume creation / format flows temporarily transfer ownership of the target disk device node to the invoking user through the root-elevated service (VolumeCreator, GraphicUserInterface, TextUserInterface, MacOSXFormatterDevice.h). Every legitimate caller of the elevated SetFileOwner targets a /dev/[r]diskN[sM] device.

The service side, however, performed no validation — it would chown() any path to any uid as root. Because chown() follows symlinks, a symlink planted at the target (or a crafted IPC request) could redirect the root-owned chown to an arbitrary path.

Reachability is limited (the service channel is private to the authenticated session, and the real callers only ever target devices), so this is hardening rather than a directly weaponizable issue. But the asymmetry with the already-validated formatter handler is worth closing.

Change

Add ValidateMacOSXSetFileOwnerTarget() in CoreService.cpp, invoked before the elevated chown:

  • Require the strict device-path form already used by the formatter (IsMacOSXFormatterDevicePath: exactly /dev/disk<N> or /dev/disk<N>s<M>, plus the rdisk variants, with no trailing characters).
  • lstat() the path (not stat) and require S_ISBLK or S_ISCHR, so a symlink is rejected outright rather than followed.

No behavior change for legitimate flows — all macOS callers already target device nodes. Other platforms are unaffected (#ifdef TC_MACOSX).

Testing

Built on Apple Silicon (arm64, macOS); volume create + mount still succeed. The validator accepts /dev/diskN (block) and /dev/rdiskN (character) device nodes and rejects non-device paths and symlinks.

The privileged CoreService handler for SetFileOwnerRequest passed the
client-supplied path straight to chown() as root with no validation --
unlike the adjacent APFS formatter handler, which strictly validates its
device argument. Every legitimate macOS caller of the elevated
SetFileOwner targets a real disk device node (/dev/[r]diskN[sM]), so a
crafted IPC request, or a symlink planted at the target, could otherwise
make the root process change ownership of an arbitrary path.

Validate the target service-side: require the strict device-path form
already used by the formatter, and lstat() it to confirm a block or
character device (rejecting symlinks rather than following them) before
the chown.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@idrassi

idrassi commented Jun 13, 2026

Copy link
Copy Markdown
Member

@damianrickard
Thanks for this, good catch, and the framing is right. That being, I can't merge this PR in its current state like the previous PR:

The commit is marked as co-authored by Claude Fable 5: While I'm not against the use of AI for development, commits should be authored by humans only to take full responsibility and ownership of the changes. co-ownership with AI is not accepted. Please amend the commit to remove the Claude Fable 5 co-author.

Once the commit is amended to remove Claude Fable 5 co-author (if you take full responsibility and ownership of the changes), I will merge this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants