Skip to content

Restrict current-user files cross-platform - #65

Merged
wesm merged 10 commits into
mainfrom
fix/private-current-user-files
Aug 8, 2026
Merged

Restrict current-user files cross-platform#65
wesm merged 10 commits into
mainfrom
fix/private-current-user-files

Conversation

@wesm

@wesm wesm commented Aug 8, 2026

Copy link
Copy Markdown
Member

Sensitive runtime files can predate a hardened caller or carry explicit permissions that a private parent does not override. Unix chmod semantics are not portable to Windows, where a current-user-owned file may still expose broad ACL entries.

This adds one fail-closed safe-file operation on an already-open, ownership-verified regular file: owner-only mode on Unix and a protected current-user/System/Administrators DACL on Windows. Callers retain path and format policy while sharing the security-sensitive platform logic. Platform-specific coverage pins both permission repairs.

@wesm @codex
Restrict current-user files cross-platform
a91e3d5 
Callers that inherit or reopen sensitive runtime files need to repair access on the verified handle, not assume Unix mode bits or a private parent are sufficient. This gives them one fail-closed primitive that preserves the current-user ownership check and installs native owner-only permissions, including a protected Windows DACL.

Generated with OpenAI Codex
Co-authored-by: OpenAI Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 8, 2026

Copy link
Copy Markdown

roborev: Combined Review (a91e3d5)

Code needs a Windows access-rights fix before merge.

Medium

  • safefileio/open_file_windows.go:68 — SetSecurityInfo requires WRITE_DAC, but neither OpenCurrentUserFile nor ordinary os.OpenFile requests that right, causing permission restriction to fail with access denied. The new O_RDWR test handle has the same issue. Obtain a validated handle with WRITE_DAC (for example, via ReOpenFile) before replacing the DACL, and update the test setup accordingly.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 1m21s

@wesm @codex
Reopen Windows files with DACL access
0eb5237 
An ordinary data handle does not imply authority to replace a Windows DACL, so permission repair could fail before securing the file. Reopening from the already-validated handle requests the exact ACL rights without reintroducing a path substitution race, and the Windows regression now uses the same capability boundary.

Generated with OpenAI Codex
Co-authored-by: OpenAI Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 8, 2026

Copy link
Copy Markdown

roborev: Combined Review (0eb5237)

Code needs one medium-severity security fix before approval.

Medium

  • safefileio/open_file_unix.go:63 — chmod(0600) may leave extended or NFSv4 ACL entries that grant other users access, particularly on macOS. Remove or validate ACLs through the open handle, or fail closed when current-user-only access cannot be guaranteed.

Reviewers: 2 done | Synthesis: codex, 5s | Total: 3m50s

@wesm @codex
Remove access ACLs before restricting files
d6234ca 
Mode bits alone do not revoke extended ACL grants, so a successful restriction could leave another principal with access. The safe-file contract now removes ACL policy through the verified handle on macOS and Linux, while unsupported Unix platforms fail instead of claiming privacy they cannot establish.

Generated with OpenAI Codex
Co-authored-by: OpenAI Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 8, 2026

Copy link
Copy Markdown

roborev: Combined Review (d6234ca)

Security review found one medium-severity issue.

Medium

  • macOS ACL removal failures may be silently accepted — safefileio/restrict_file_darwin.go:55

    acl_set_fd_np returns a 32-bit C int, but comparing its result with ^uintptr(0) can miss -1 represented as 0x00000000ffffffff. The function may then return success while an extended ACL still grants another local user access.

    Treat any nonzero result as failure, or use a typed int32 return and check for -1.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 5m23s

@wesm @codex
Reject every failed macOS ACL update
b0979b7 
The macOS ACL setter returns a C int, so a failure may appear in the uintptr syscall result as a zero-extended 32-bit minus one rather than an all-bits-set machine word. Treating zero as the only success value keeps file restriction fail-closed across ABI return representations.

Validation: reproduced the old false-success result by invoking ACL removal on a closed descriptor.

Generated with OpenAI Codex
Co-authored-by: OpenAI Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 8, 2026

Copy link
Copy Markdown

roborev: Combined Review (b0979b7)

The change has one high-severity access-control race and one medium-severity cross-platform test failure.

High

  • ACL removal can broaden file access — safefileio/restrict_file_linux.go:21, safefileio/restrict_file_darwin.go:33

    ACLs are removed before applying mode 0600. A local user denied by an ACL but permitted by the underlying group/other mode bits could race this interval and retain an open file descriptor. Apply Chmod(0600) before removing ACLs, then optionally reapply it afterward.

Medium

  • Unix-wide test fails on supported BSD builds — safefileio/private_dir_unix_test.go:107

    The test expects success, while BSD and other supported Unix implementations intentionally return “unsupported.” Move the success test to a linux || darwin-tagged file and separately test unsupported platforms.

Reviewers: 2 done | Synthesis: codex, 5s | Total: 5m30s

@wesm @codex
Narrow Unix modes before removing ACLs
6fd027f 
Removing a deny ACL while group or other mode bits remain open creates a brief access window even if the final state is private. Supported Unix implementations now share a fail-closed sequence that narrows mode bits before ACL work and reapplies them afterward.\n\nPlatform-specific tests also distinguish Darwin/Linux enforcement from the intentional unsupported-Unix result instead of imposing one contract across incompatible implementations.

Generated with OpenAI Codex
Co-authored-by: OpenAI Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 8, 2026

Copy link
Copy Markdown

roborev: Combined Review (6fd027f)

Review verdict: One high-severity and one medium-severity permission-hardening issue found.

High

  • safefileio/restrict_file_linux.go:18 — CIFS/SMB ACLs remain unrestricted. The implementation removes only POSIX and NFSv4 ACLs. On CIFS mounts, chmod(0600) may succeed without changing server permissions, and ignored ENOTSUP results can leave a broad system.cifs_acl DACL intact. Detect CIFS/SMB and securely replace and verify its server DACL, or return an unsupported-filesystem error.

Medium

  • safefileio/restrict_file_supported_unix.go:14 — Successful permission changes are not verified. Filesystems may ignore or clamp chmod, leaving the file publicly accessible despite a success result. Stat the file after both permission changes and require mode 0600, particularly before removing ACLs.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 7m8s

@wesm @codex
Reject unverifiable Unix file restrictions
8c0a565 
A successful local chmod is not sufficient evidence that a filesystem enforced private access, and SMB-family mounts retain a server-side DACL that these helpers cannot safely rewrite. Fail before mutation on those Linux filesystems and require an exact mode readback around ACL removal so callers never receive a success-shaped result for ambiguous permissions.

Validation: executed the complete safefileio test binary inside a Linux container in addition to the repository and cross-build suites.

Generated with OpenAI Codex
Co-authored-by: OpenAI Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 8, 2026

Copy link
Copy Markdown

roborev: Combined Review (8c0a565)

Verdict: Two medium-severity security issues require attention.

Medium

  • safefileio/restrict_file_linux.go:26 — On 32-bit Linux, signed Statfs_t.Type sign-extends CIFS/SMB2 magic values, bypassing filesystem rejection. Normalize through uint32 before comparison and test sign-extended values.

  • safefileio/restrict_file_supported_unix.go:24, safefileio/open_file_windows.go:72 — Restricting permissions does not revoke existing attacker-held handles. Reject broadly accessible files, or securely create and atomically replace them instead of repairing them in place.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 6m23s

@wesm @codex
Validate private files without repairing access
8101d25 
Permission repair cannot revoke a descriptor another process already obtained while a file was broad. Replace the unreleased mutation API with handle-bound validation so broad mode bits, access ACLs, Windows DACLs, and SMB-family filesystems fail without changing the object; callers must create a private replacement instead.\n\nNormalizing Linux filesystem magic through uint32 also keeps CIFS and SMB2 rejection effective when 32-bit statfs values are sign-extended.

Validation: reproduced the sign-extension bypass in a Linux test binary and executed the complete updated safefileio suite inside Linux.

Generated with OpenAI Codex
Co-authored-by: OpenAI Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 8, 2026

Copy link
Copy Markdown

roborev: Combined Review (8101d25)

Code is generally sound, but one medium-severity Darwin ACL validation issue should be fixed.

Medium

  • safefileio/private_file_darwin.go:42 — errno is thread-local, but the goroutine may migrate between __error and acl_get_fd_np. This could misclassify an ACL inspection failure as ENOENT and accept the file. Capture errno from the same PureGo call (for example, with purego.SyscallN) or lock the goroutine to its OS thread across both calls.

Reviewers: 2 done | Synthesis: codex, 5s | Total: 5m8s

@wesm @codex
Keep Darwin ACL errno thread-local
07828b5 
The ACL probe and libc errno pointer must refer to the same OS thread; otherwise goroutine migration can turn an inspection failure into a false no-ACL result. Pinning the short native-call sequence preserves the validation-only contract while retaining typed PureGo function signatures.

Generated with OpenAI Codex
Co-authored-by: OpenAI Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 8, 2026

Copy link
Copy Markdown

roborev: Combined Review (07828b5)

Review verdict: one high-severity Windows security issue must be addressed.

High

  • safefileio/private_dir_windows.go:203 — Unprotected file DACL permits post-validation access injection. File validation disables the SE_DACL_PROTECTED check. An attacker controlling the containing directory could add an inheritable ACE after validation, causing Windows to propagate new access to the sensitive file. Require protected file DACLs, or securely bind validation to a protected, trusted parent. Callers should replace unprotected files with files created using an explicitly protected DACL. (Microsoft documentation)

Reviewers: 2 done | Synthesis: codex, 12s | Total: 9m46s

@wesm @codex
Require protected Windows file DACLs
9ac7634 
A DACL limited to trusted principals is still mutable through inheritance while it remains unprotected. Treat inheritable files as unsafe so validation cannot be invalidated by later ACE propagation from an attacker-controlled parent; recovery continues to require a separately created protected replacement.

Generated with OpenAI Codex
Co-authored-by: OpenAI Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 8, 2026

Copy link
Copy Markdown

roborev: Combined Review (9ac7634)

High-severity issue found: Linux validation may falsely accept AFS files as private.

High

  • safefileio/private_file_linux.go:37 — AFS directory ACLs bypass validation. AFS can grant access through containing-directory ACLs despite 0600 file mode. Since only SMB/CIFS is rejected and file xattrs do not expose directory ACLs, validation may incorrectly succeed and expose secrets. Reject AFS as unsupported or reliably validate its effective ACL.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 6m24s

@wesm @codex
Reject Linux filesystems with external access policy
e492735 
AFS and similar network or user-space filesystems can grant access outside local file modes and access ACLs, so treating them as supported can falsely validate secret-bearing files as private. Classify these filesystems through statfs and fail closed while retaining uint32 normalization for 32-bit Linux.

Validated with the safefileio suite in a Linux container, native vet and tests, Linux 386/arm cross-builds, and Windows/Darwin cross-builds.

Generated with OpenAI Codex

Co-authored-by: OpenAI Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 8, 2026

Copy link
Copy Markdown

roborev: Combined Review (e492735)

Code review passed: no Medium, High, or Critical findings.

Reviewers: 2 done | Synthesis: codex, 5s | Total: 4m36s

@wesm
wesm merged commit 7d77ff6 into main Aug 8, 2026
11 checks passed
@wesm
wesm deleted the fix/private-current-user-files branch August 8, 2026 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@wesm