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

Add setfsuid and setfsgid implementation for filesystem checks #1163

Merged
merged 1 commit into from Feb 29, 2020

Conversation

gliderkite
Copy link
Contributor

I noticed that the filesystem checks API setfsuid and setfsgid where missing (while available in libc). This PR adds the implementation for both of them.

@asomers
Copy link
Member

asomers commented Dec 15, 2019

According to the man page, those syscalls are obsolete. Do you have an actual use case for them? If not, then we should reject this PR in the interest of resisting bloat.

@gliderkite
Copy link
Contributor Author

As far as I understand the note you are referring to:

At the time when this system call was introduced, one process could send a signal to another process with the same effective user ID. This meant that if a privileged process changed its effective user ID for the purpose of file permission checking, then it could become vulnerable to receiving signals sent by another (unprivileged) process with the same user ID. The filesystem user ID attribute was thus added to allow a process to change its user ID for the purposes of file permission checking without at the same time becoming vulnerable to receiving unwanted signals. Since Linux 2.0, signal permission handling is different (see kill(2)), with the result that a process can change its effective user ID without being vulnerable to receiving signals from unwanted processes. Thus, setfsuid() is nowadays unneeded and should be avoided in new applications (likewise for setfsgid(2)).

states that setfsuid is not required anymore if the purpose was to protect the process from receiving signals from another process having the same effective UID, while it doesn't say that setfsuid should be avoided in general because obsolete.

I have a specific use case for this, that is I need to be able to check filesystem permissions per thread, and using seteuid would require to lock every thread with write access, not allowing parallel checks (source).

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Ok, that sounds like a reasonable use case. Could you please add a test and a CHANGELOG entry?

src/unistd.rs Outdated Show resolved Hide resolved
src/unistd.rs Outdated Show resolved Hide resolved
src/unistd.rs Outdated Show resolved Hide resolved
@gliderkite
Copy link
Contributor Author

Ok, that sounds like a reasonable use case. Could you please add a test and a CHANGELOG entry?

I added a new entry in the changelog, but regarding the tests I'm not sure what tests would you like to see. I couldn't find any test for any equivalent function such as seteuid or setegid for example.

@asomers
Copy link
Member

asomers commented Jan 28, 2020

I added a new entry in the changelog, but regarding the tests I'm not sure what tests would you like to see. I couldn't find any test for any equivalent function such as seteuid or setegid for example.

Anything that minimally exercises those syscalls. For example, creating a thread, setting its fs uid to nobody, and verifying that it gets EACCES when trying to read a 640 file.

@gliderkite
Copy link
Contributor Author

I added the test, but it fails on CI. At the moment I can only test it locally using --target x86_64-unknown-linux-gnu, which fails on CI, but it succeeds on my machine. I'll try to debug it by adding --nocapture to the CI tests when I have some time.

CHANGELOG.md Show resolved Hide resolved
src/unistd.rs Outdated Show resolved Hide resolved
src/unistd.rs Outdated
// spawn a new thread where to test setfsuid
thread::spawn(move || {
// set filesystem UID
let _ = setfsuid(nobody.uid);
Copy link
Member

Choose a reason for hiding this comment

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

To determine success, you should follow the procedure described in the man page. Call setfsuid(-1) and check whether the fsuid was actually changed. I suspect that when running on Travis, it won't be.

http://man7.org/linux/man-pages/man2/setfsuid.2.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an assertion for the fsuid change.

@gliderkite
Copy link
Contributor Author

gliderkite commented Feb 28, 2020

It looks like it failed on nightly the second run, where the first run passed and the only change only included the changelog. Could you please have a look @asomers?

@asomers
Copy link
Member

asomers commented Feb 28, 2020

It looks like a new Rust nightly has added some lints. I'll fix this in a separate PR, then you can rebase.

@asomers
Copy link
Member

asomers commented Feb 29, 2020

Actually, it looks like the failures were due to a bug in the compiler. I just restarted the build, it used a newer compiler version than the previous build (nightly-2020-02-28 vs nightly-2020-02-27), and it passed this time.

@asomers
Copy link
Member

asomers commented Feb 29, 2020

Please squash your commits. Then I'll merge.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Feb 29, 2020
1163: Add setfsuid and setfsgid implementation for filesystem checks r=asomers a=gliderkite

I noticed that the filesystem checks API `setfsuid` and `setfsgid` where missing (while available in `libc`). This PR adds the implementation for both of them.

Co-authored-by: Marco Conte <gliderkite@gmail.com>
@bors
Copy link
Contributor

bors bot commented Feb 29, 2020

Build failed

@asomers
Copy link
Member

asomers commented Feb 29, 2020

That failure happened deep within libstd. Perhaps a QEMU bug?

bors retry

@bors
Copy link
Contributor

bors bot commented Feb 29, 2020

Build succeeded

@bors bors bot merged commit 2a40283 into nix-rust:master Feb 29, 2020
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