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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
273 changes: 273 additions & 0 deletions include/sys/policy.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,273 @@
/*
* CDDL HEADER START
*
* The contents of this file are subject to the terms of the
* Common Development and Distribution License (the "License").
* You may not use this file except in compliance with the License.
*
* You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
* or http://www.opensolaris.org/os/licensing.
* See the License for the specific language governing permissions
* and limitations under the License.
*
* When distributing Covered Code, include this CDDL HEADER in each
* file and include the License file at usr/src/OPENSOLARIS.LICENSE.
* If applicable, add the following below this CDDL HEADER, with the
* fields enclosed by brackets "[]" replaced with your own identifying
* information: Portions Copyright [yyyy] [name of copyright owner]
*
* CDDL HEADER END
*/

#ifndef _SYS_POLICY_H
#define _SYS_POLICY_H

#include <sys/types.h>
#include <sys/xvattr.h>
#include <sys/zfs_znode.h>
#include <sys/cred.h>
#include <sys/vnode.h>
#include <linux/kmod.h>
#include <linux/security.h>

/*
* The passed credentials cannot be used because the Linux kernel does
* not expose an interface to check arbitrary credentials. The available
* capable() interface assumes the current user. Luckily, the credentials
* passed by zfs are almost always obtained be CRED() which is defined to
* be the current user credentials. So the credentials should be the same.
*
* The major exception to this is that zfs will occasionally pass kcred
* for certain operations such as zil replay. Now as long as zfs uses
* kcred only in the right contexts this should also be OK. The worst
* case here is that we mistakenly deny an operation.
*/
static inline int
spl_capable(cred_t *c, int capability)
{
return capable(capability) ? 0 : EACCES;
}

static inline int
secpolicy_fs_unmount(cred_t *c, struct vfsmount *mnt)
{
return spl_capable(c, CAP_SYS_ADMIN);
}

static inline int
secpolicy_sys_config(cred_t *c, int checkonly)
{
return spl_capable(c, CAP_SYS_ADMIN);
}

static inline int
secpolicy_nfs(cred_t *c)
{
return spl_capable(c, CAP_SYS_ADMIN);
}

static inline int
secpolicy_zfs(cred_t *c)
{
return spl_capable(c, CAP_SYS_ADMIN);
}

static inline int
secpolicy_zinject(cred_t *c)
{
return spl_capable(c, CAP_SYS_ADMIN);
}

static inline int
secpolicy_vnode_setids_setgids(cred_t *c, gid_t gid)
{
if (groupmember(gid, c))
return 0;

return spl_capable(c, CAP_FSETID);
}

static inline int
secpolicy_vnode_setid_retain(cred_t *c, int is_setuid_root)
{
return spl_capable(c, CAP_FSETID);
}

static inline int
secpolicy_setid_setsticky_clear(struct inode *ip, vattr_t *attr,
vattr_t *oldattr, cred_t *c)
{
boolean_t requires_extrapriv = B_FALSE;

if ((attr->va_mode & S_ISGID) && !groupmember(oldattr->va_gid, c))
requires_extrapriv = B_TRUE;

if ((attr->va_mode & S_ISUID) && !(oldattr->va_uid == crgetuid(c)))
requires_extrapriv = B_TRUE;

if (requires_extrapriv == B_FALSE)
return 0;

return spl_capable(c, CAP_FSETID);
}

static inline int
secpolicy_setid_clear(vattr_t *v, cred_t *c)
{
int err;

err = spl_capable(c, CAP_FSETID);
if (err)
return err;

if (v->va_mode & (S_ISUID | S_ISGID)) {
v->va_mask |= AT_MODE;
v->va_mode &= ~(S_ISUID | S_ISGID);
}

return err;
}

static inline int
secpolicy_vnode_any_access(cred_t *c , struct inode *ip, uid_t owner)
{
if (crgetuid(c) == owner)
return 0;

if (spl_capable(c, CAP_DAC_OVERRIDE) == 0)
return 0;

if (spl_capable(c, CAP_DAC_READ_SEARCH) == 0)
return 0;

if (spl_capable(c, CAP_FOWNER) == 0)
return 0;

return EACCES;
}

static inline int
secpolicy_vnode_access2(cred_t *c, struct inode *ip, uid_t owner,
mode_t curmode, mode_t wantedmode)
{
mode_t missing = ~curmode & wantedmode;

if (missing == 0)
return 0;

if ((missing & ~(S_IRUSR | S_IXUSR)) == 0) {
if (spl_capable(c, CAP_DAC_READ_SEARCH) == 0)
return 0;
}

return spl_capable(c, CAP_DAC_OVERRIDE);
}

static inline int
secpolicy_vnode_chown(cred_t *c, uid_t owner)
{
if (crgetuid(c) == owner)
Copy link
Contributor

Choose a reason for hiding this comment

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

According to comments in struct cred ( http://lxr.linux.no/#linux+v3.2/include/linux/cred.h#L131 )
shouldn't we change every crgetuid with crgetfsuid, as we are always checking for permissions related to filesystem? (same applies for gid).
I mean in the entire ZoL codebase (possibile exception: delegations) not only in this patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that seems the most correct. However, up until now we've relied pretty much exclusively on the VFS layer for this sort of permission checking and enforcement. Delegations should probably rely on crgetuid/crgetgid and the ACLs and attribute code should use crgetfsuid/crgetfsguid.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want, I'll take care of this. May I suggest using "effective UID" instead of "real UID" for delegations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By all means, if you could sort this out that would be ideal from my point of view. Frankly, your going to be able to give this a lot more careful thought than me in the near future.

Copy link
Contributor

Choose a reason for hiding this comment

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

maxximino/zfs@f3ddf21
maxximino/spl@6e9e846
I still have to test them deeply, however those patches look reasonably safe. (On successful end of tests I will make another pull request.)

return 0;

return spl_capable(c, CAP_FOWNER);
}

static inline int
secpolicy_vnode_setdac(cred_t *c, uid_t owner)
{
if (crgetuid(c) == owner)
return 0;

return spl_capable(c, CAP_DAC_OVERRIDE);
}

static inline int
secpolicy_vnode_remove(cred_t *c)
{
return spl_capable(c, CAP_FOWNER);
}

static inline int
secpolicy_vnode_setattr(cred_t *c, struct inode *ip, vattr_t *vap,
vattr_t *oldvap, int flags,
int (*zaccess)(void *, int, cred_t *), znode_t *znode)
{
int mask = vap->va_mask;
int err = 0;

if (mask & AT_MODE) {
err = secpolicy_vnode_setdac(c, oldvap->va_uid);
if (err)
return err;

err = secpolicy_setid_setsticky_clear(ip, vap, oldvap, c);
if (err)
return err;
} else {
vap->va_mode = oldvap->va_mode;
}

if (mask & AT_SIZE) {
if (S_ISDIR(ip->i_mode))
return (EISDIR);

err = zaccess(znode, S_IWUSR, c);
if (err)
return err;
}

if (mask & (AT_UID | AT_GID)) {
if (((mask & AT_UID) && vap->va_uid != oldvap->va_uid) ||
((mask & AT_GID) && vap->va_gid != oldvap->va_gid )) {
secpolicy_setid_clear(vap, c);
err = secpolicy_vnode_setdac(c, oldvap->va_uid);
if (err)
return err;
}
}

if (mask & (AT_ATIME | AT_MTIME)) {
/*
* If the caller is the owner or has appropriate capabilities,
* allow them to set every time. The Linux-specific constants
* mean the user has requested to write specific times instead
* of the current time.
*/
if ((secpolicy_vnode_setdac(c, oldvap->va_uid) != 0) &&
(mask & (ATTR_ATIME_SET | ATTR_MTIME_SET))) {
err = zaccess(znode, S_IWUSR, c);
if (err)
return err;
}
}

return err;
}

/* Unused */
static inline int
secpolicy_vnode_stky_modify(cred_t *c)
{
return EACCES;
}

static inline int
secpolicy_basic_link(cred_t *c)
{
return spl_capable(c, CAP_FOWNER);
}

/* Only used with ksid */
static inline int
secpolicy_vnode_create_gid(cred_t *c)
{
return EACCES;
}

static inline int
secpolicy_xvattr(xvattr_t *xv, uid_t owner, cred_t *c, mode_t mode)
{
return secpolicy_vnode_chown(c, owner);
}

#endif /* SYS_POLICY_H */
4 changes: 1 addition & 3 deletions module/zfs/zfs_acl.c
Original file line number Diff line number Diff line change
Expand Up @@ -1725,9 +1725,7 @@ zfs_acl_ids_create(znode_t *dzp, int flag, vattr_t *vap, cred_t *cr,
int error;
zfs_sb_t *zsb = ZTOZSB(dzp);
zfs_acl_t *paclp;
#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.

boolean_t need_chmod = B_TRUE;
boolean_t inherited = B_FALSE;

Expand Down
2 changes: 1 addition & 1 deletion module/zfs/zfs_vnops.c
Original file line number Diff line number Diff line change
Expand Up @@ -2364,7 +2364,7 @@ zfs_setattr(struct inode *ip, vattr_t *vap, int flags, cred_t *cr)
vattr_t oldva;
xvattr_t *tmpxvattr;
uint_t mask = vap->va_mask;
uint_t saved_mask;
uint_t saved_mask = 0;
int trim_mask = 0;
uint64_t new_mode;
uint64_t new_uid, new_gid;
Expand Down