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.

Sign up for GitHub

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

Jump to bottom

Fix ACL checks for NFS kernel server #13221

Merged
merged 3 commits into from
Mar 18, 2022
Merged

Fix ACL checks for NFS kernel server #13221

merged 3 commits into from
Mar 18, 2022

Conversation

ghost
Copy link

@ghost ghost commented Mar 15, 2022

Motivation and Context

For Linux NFS kernel server ops, fsuid and fsgid in
cred are populated with ids that operation is
being performed as, but euid and egid remain 0.

In Linux when setresuid(2) and setresgid(2) are
called, the fsuid and fsgid are set to the euid
and egid respectively.

The use of fsuid / fsgid is prescribed in Linux kernel documentation under the section "Open File Credentials" in Documentation/security/credentials.rst.

Description

This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@ghost
Copy link
Author

ghost commented Mar 15, 2022

CC @anodos325

@ghost
Copy link
Author

ghost commented Mar 15, 2022

Fixes #13217

@ghost ghost added the Status: Code Review Needed Ready for review and testing label Mar 15, 2022
@behlendorf behlendorf mentioned this pull request Mar 15, 2022
Closed
sdimitro
sdimitro approved these changes Mar 16, 2022
View reviewed changes
Copy link
Contributor

@sdimitro sdimitro left a comment

Choose a reason for hiding this comment

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

This definitely fixes #13217 for me. Have you given any thought to Brian's feedback here: #13217 (comment)

@ghost
Copy link
Author

ghost commented Mar 17, 2022

I'm testing out changing most uses of crgetuid/crgetgid to crgetfsuid/crgetfsgid, except for the ioctls/delegation. There are also a ton of unused/unimplemented SPL cred functions (especially on the FreeBSD side) I'm cleaning out while here.

@ghost
Copy link
Author

ghost commented Mar 17, 2022

Yeah I'll go with Brian's suggestion instead. The fsuid/fsgid will work for the ioctls/delegation, and it's cleaner to keep crgetuid/crgetgid and expunge crgetfsuid/crgetfsgid. Having both is more confusing than it needs to be.

@anodos325
Fix ACL checks for NFS kernel server
a43f573 
For Linux NFS kernel server ops, fsuid and fsgid in
cred are populated with ids that operation is
being performed as, but euid and egid remain 0.

In Linux when setresuid(2) and setresgid(2) are
called, the fsuid and fsgid are set to the euid
and egid respectively.

This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Signed-off-by: Andrew Walker <awalker@ixsystems.com>
@ghost
Copy link
Author

ghost commented Mar 17, 2022 •
edited by ghost
Loading

  • Incorporated feedback to fully eliminate crgetfsuid/crgetfsgid
  • Also removed FreeBSD's kidmap.h and unused cred and sid functions from SPL

@sdimitro
Copy link
Contributor

That's great! Thanks for being thorough @freqlabs !

Ryan Moeller added 2 commits March 17, 2022 17:07
Clean out unused/unimplemented cred interfaces
f2aae33 
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
FreeBSD: Remove unused kidmap/sid functions
e3937f3 
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
@ghost
Copy link
Author

ghost commented Mar 17, 2022

  • Failed to save an edit whoops

@mmaybee mmaybee added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Mar 17, 2022
@mmaybee mmaybee merged commit d42979c into openzfs:master Mar 18, 2022
@ghost ghost deleted the f_cred branch March 18, 2022 15:48
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Mar 18, 2022
@behlendorf
Fix ACL checks for NFS kernel server
8b17d39 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Mar 19, 2022
@behlendorf
Fix ACL checks for NFS kernel server
a07cd5c 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
behlendorf added a commit that referenced this pull request Mar 21, 2022
@behlendorf
Fix ACL checks for NFS kernel server
847d030 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Co-authored-by: Ryan Moeller <freqlabs@FreeBSD.org>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes #13221
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
@nicman23
Fix ACL checks for NFS kernel server
deee808 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
@nicman23
Fix ACL checks for NFS kernel server
48a3f3c 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 2, 2022
@lundman
Fix ACL checks for NFS kernel server
4ce3279 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
beren12 pushed a commit to beren12/zfs that referenced this pull request Sep 19, 2022
@beren12
Fix ACL checks for NFS kernel server
5a41652 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@andrewc12
Fix ACL checks for NFS kernel server
9f070a0 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@andrewc12
Fix ACL checks for NFS kernel server
51475e1 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@andrewc12
Fix ACL checks for NFS kernel server
83c5605 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@andrewc12
Fix ACL checks for NFS kernel server
7590fe8 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@andrewc12
Fix ACL checks for NFS kernel server
d33d622 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@andrewc12
Fix ACL checks for NFS kernel server
1e5332f 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@andrewc12
Fix ACL checks for NFS kernel server
b19a05b 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@andrewc12
Fix ACL checks for NFS kernel server
f3360d9 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@andrewc12
Fix ACL checks for NFS kernel server
5efb9c0 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@andrewc12
Fix ACL checks for NFS kernel server
5709c0c 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@andrewc12
Fix ACL checks for NFS kernel server
e78f364 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@andrewc12
Fix ACL checks for NFS kernel server
ec9395e 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 23, 2022
@andrewc12
Fix ACL checks for NFS kernel server
f81bf65 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
snajpa pushed a commit to vpsfreecz/zfs that referenced this pull request Oct 22, 2022
@snajpa
Fix ACL checks for NFS kernel server
b98e5b5 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
snajpa pushed a commit to vpsfreecz/zfs that referenced this pull request Oct 22, 2022
@snajpa
Fix ACL checks for NFS kernel server
fd96653 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
snajpa pushed a commit to vpsfreecz/zfs that referenced this pull request Oct 23, 2022
@snajpa
Fix ACL checks for NFS kernel server
4a4f657 
This PR changes ZFS ACL checks to evaluate
fsuid / fsgid rather than euid / egid to avoid
accidentally granting elevated permissions to
NFS clients.

Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Andrew Walker <awalker@ixsystems.com>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#13221
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@don-brady don-brady Awaiting requested review from don-brady

Assignees

@mmaybee mmaybee

Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sdimitro @behlendorf @mmaybee @anodos325