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

Implement secpolicy_* functions #619

Closed
wants to merge 1 commit into from

Conversation

behlendorf
Copy link
Contributor

Based on Linux POSIX style capabilities.

Signed-off-by: Brian Behlendorf behlendorf1@llnl.gov
Signed-off-by: Massimo Maggimassimo@mmmm.it

Based on Linux POSIX style capabilities.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Massimo Maggi<massimo@mmmm.it>
@behlendorf
Copy link
Contributor Author

Massimo Maggi's secpolicy implementation, changes from the original patch include. Please review!

  1. Style, updated to be consistent with the Solaris style guide. Consistency is good.

  2. Inverted the spl_capable() check logic in the following functions which looked like they were returning success which spl_capable() was actually failing: secpolicy_setid_clear, secpolicy_vnode_any_access, secpolicy_vnode_access2

  3. in_group_p() function replaced by groupmember for compatibility

  4. Move to the zfs code from the spl code, this header should be picked up first as the include but we will need to remove it's counterpart from the spl when this change is merged.

#ifdef HAVE_KSID
gid_t gid;
#endif /* HAVE_KSID */
gid_t gid = crgetgid(cr);
Copy link
Contributor

Choose a reason for hiding this comment

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

But in this context secpolicy_vnode_setids_setgids should not check if the current user can create a setgid of the requested gid? (checking vap->va_gid, which should be different for example when we are creating a file inside a setguid directory? See zpl_vap_init in zpl_inode.c)
The current user is always a groupmember of the current gid, so secpolicy_vnode_setids_setgids is always going to return 0.
I've tried to exploit this,unsuccessfully. I think that Linux VFS shields against this problem, as of now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then this should be vap->va_gid?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've solved the problem with commit maxximino/zfs@b2f0012 . Did you miss it (it was referenced in the text of previous pull request) or you've solved differently on purpose?
If you prefer to "approximate" with a oneliner, vap->va_gid looks more reasonable to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, yes I just missed the zfs side changes you had proposed. I was a bit rushed on Friday when looking at this originally.

@behlendorf
Copy link
Contributor Author

Closing as stale. But the patch is still available for reference when this does get implemented.

@FransUrbo
Copy link
Contributor

@maxximino @behlendorf Is there any progress on this? Combining this PR with maxximino/zfs@b2f0012 and maxximino/zfs@f3ddf21, is this enough to get this going?

@ilovezfs
Copy link
Contributor

ilovezfs commented Mar 9, 2015

FYI, I have implemented a simple version of this for OpenZFS on OS X. It checks for root privileges in the important cases, and otherwise allows read access:
openzfsonosx/spl#7

This will be merged to master within the next couple days.

@behlendorf
Copy link
Contributor Author

@ilovezfs interesting! Thanks for pointing us to your implementation.

@maxximino
Copy link
Contributor

@FransUrbo That was initially created for a Posix ACL implementation with NFSv4 on-disk format, which has been set aside for the current way (linux-specific encoding of the Posix ACL stored as xattr, exactly the same encoding used in all other linux filesystems). I think it can be useful also to implement delegation, should not be a huge effort. About namespaces/containers, I am not familiar enough with internals of UID namespaces to say something useful about that. About continuing this pull request, I would be really happy if someone does, but for the moment I can't. When I was working on Posix ACL, I hadn't hit problems due to this specific PR, but requires very diligent testing before merging: the worst possible outcome here is not a simple crash, but allowing operations / data access that should not be allowed.

pcd1193182 pushed a commit to pcd1193182/zfs that referenced this pull request Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Feature request or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants