Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Bash fix-permissions script with Go #1219

Merged
merged 3 commits into from Sep 20, 2023

Conversation

DrJosh9000
Copy link
Contributor

@DrJosh9000 DrJosh9000 commented Sep 19, 2023

  • Easier to test (❤️ table driven tests)
  • Can test more things - it tests that file ownership changed, for example

But perhaps most importantly: this change prevents shenanigans between validating the path and chown-ing files.

With the previous Bash script, a determined attacker could occasionally manage to replace parts of the path in between validating (that the args and the path are fine), and running chown(1) at the end.

Various chown flags won't help: -h changes what chown does to symlinks when it finds them, and -P prevents recursion traversing them, but you can always pass it a path where intermediate segments are symlinks, and it resolves those.

The new binary is based on openat2(2). This lovely syscall can simultaneously ensure (with various flags) that there are no symlinks, magic links, cross-device accesses, etc in the path being opened, and ensure that the (relative) path is treated as a subpath of a given file descriptor. Basing the access on a file descriptor defeats shenanigans, because file descriptors refer to inodes, not paths. An attacker can replace what the path resolves to as much as they like, but can't change which inode the file descriptor refers to once it is opened.

Aside from openat2, fchownat(2) works in a similar way for changing ownership. (fchownat is wrapped in a method called "Lchown" because AT_SYMLINK_NOFOLLOW makes it operate like lchown.)

These syscalls are wrapped into an io/fs.FS implementation I've called fdfs ("file descriptor file system"), which is used to drive fs.WalkDir. fs.WalkDir doesn't traverse symlinks when it finds them, with the exception of the root it starts walking from - but we eliminated that with openat2. That's very cute, but there's still a gap of time between walking a path (at that moment it has no intermediate symlinks) and passing it to fchownat (when it could have changed). So fdfs also implements a recursive lchown.

The script-level logic (a.k.a. "business logic") is in the fixer subpackage, for testability.

The main downside to openat2 is that it is Linux specific. There are probably ways to hold openat (original flavour) on other platforms to do similar things.

@DrJosh9000 DrJosh9000 force-pushed the pdp-1621-replace-bash-with-go branch 7 times, most recently from 5924943 to a2d4bc6 Compare September 19, 2023 05:56
* Easier to test
* Can test more things
* Prevents symlink shenanigans
Copy link
Contributor

@moskyb moskyb left a comment

Choose a reason for hiding this comment

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

LGTM - awesome work on this, deep in the syscall mines

@DrJosh9000 DrJosh9000 merged commit ca2183c into main Sep 20, 2023
1 check was pending
@DrJosh9000 DrJosh9000 deleted the pdp-1621-replace-bash-with-go branch September 20, 2023 03:17
DrJosh9000 added a commit that referenced this pull request Sep 20, 2023
Backport: Replace Bash fix-permissions script with Go #1219
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.

None yet

2 participants