sanity-check set-id interpreter script paths #1029
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The check is fairly stringent; we do it in a constructor, and we make sure that there actually is an open fd higher than 2 that matches the passed path. There is an argument to be made for trapping in the constructor here, and there is also an argument to be made for failing open like we did previously. We pick a middle ground: GetProgramExecutableName returns empty string. Logic being that ensuring the sanity of whatever set-id script mechanism user is running is above our pay grade, but the path TOCTOU between loader and binary is not.
By the logic of the previous commit, it's not our job to verify that the user's whacked out set-id configuration is sane, only that the path they passed does not refer to a different file now than it did when ape found it. This is generally true of the files under /dev/fd in our case. So we just check that the file is in /dev/fd/, that it starts with not a '.', and that it does not contain a '/'. Since we don't care whether there actually is an fd by that name, we can do the check when called and save a constructor.
Again, the details of the system's set-id interpreter script setup are above our pay grade; we only care whether the path can vary between us and the loader.
Blank line needed to prevent clang-format from making `DevFd` more than 80 characters long. Possible clang-format bug?
|
Tested on M1 Mac as so: % chmod u+s /usr/local/bin/ape
% chown root /usr/local/bin/ape
% ape - o/aarch64/examples/printargs.com printargs.com
[… GetProgramExecutableName = "" …]
% ape - /dev/fd/3 printargs.com 3<o/aarch64/examples/printargs.com
[… GetProgramExecutableName = "/dev/fd/3" …]And obviously confirmed that things like |
jart
approved these changes
Dec 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This actually implements proposal 2a, rolled back from #1024, of this:
https://gist.github.com/mrdomino/2222cab61715fd527e82e036ba4156b1
From the first commit: