Skip to content

Commit

Permalink
RFC: user namespaces in policy.c
Browse files Browse the repository at this point in the history 
Honor the user namespace when checking whether we're allowed
to add the setgid bit to a file.
  • Loading branch information
@Blub
Blub committed Oct 30, 2017
1 parent f4ae39a commit dd97fcf
Showing 1 changed file with 15 additions and 14 deletions.
29 changes: 15 additions & 14 deletions module/zfs/policy.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,14 @@
* all other cases this function must fail and return the passed err.
*/
static int
priv_policy(const cred_t *cr, int capability, boolean_t all, int err)
priv_policy(const cred_t *cr, int capability, boolean_t all, int err, struct user_namespace *ns)

This comment has been minimized.

Sign in to view

Copy link
@behlendorf

behlendorf Oct 30, 2017

The current user namespace is available as part of the passed credential, cred.user_ns, this is in fact where current_user_ns() gets it from. You should be able to use that instead which ensures the same credential is used for the duration of the system call.
{
ASSERT3S(all, ==, B_FALSE);

if (cr != CRED() && (cr != kcred))
return (err);

if (!capable(capability))
if (!(ns ? ns_capable(ns, capability) : capable(capability)))

This comment has been minimized.

Sign in to view

Copy link
@behlendorf

behlendorf Oct 30, 2017

You should be able to unconditionally convert this to if (!ns_capable(cr->user_cred, capability)).

This comment has been minimized.

Sign in to view

Copy link
@Blub

Blub Oct 31, 2017

Author Owner

For some reason I only considered file operations initially when trying to find cases which behave differently.
But, doing this generally means the user can perform zpool and zfs command related operations such as zpool export tank or zfs create tank/foo.
I'd expect this to only work if the real-uid is zfs allowed to do this. But these permissions seem to not honor the uid mapping currently. So perhaps it's better to just change secpolicy_vnode_setids_setgids() directly for now and then work on the rest, which seems to be a bit more involved.

This comment has been minimized.

Sign in to view

Copy link
@Blub

Blub Oct 31, 2017

Author Owner

Ah, the checks do actually honor the mapping, but the allow/unallow commands themselves do not.
return (err);

return (0);
Expand All @@ -62,7 +62,7 @@ priv_policy(const cred_t *cr, int capability, boolean_t all, int err)
int
secpolicy_nfs(const cred_t *cr)
{
return (priv_policy(cr, CAP_SYS_ADMIN, B_FALSE, EPERM));
return (priv_policy(cr, CAP_SYS_ADMIN, B_FALSE, EPERM, NULL));
}

/*
Expand All @@ -71,7 +71,7 @@ secpolicy_nfs(const cred_t *cr)
int
secpolicy_sys_config(const cred_t *cr, boolean_t checkonly)
{
return (priv_policy(cr, CAP_SYS_ADMIN, B_FALSE, EPERM));
return (priv_policy(cr, CAP_SYS_ADMIN, B_FALSE, EPERM, NULL));
}

/*
Expand Down Expand Up @@ -102,10 +102,10 @@ secpolicy_vnode_any_access(const cred_t *cr, struct inode *ip, uid_t owner)
if (zpl_inode_owner_or_capable(ip))
return (0);

if (priv_policy(cr, CAP_DAC_OVERRIDE, B_FALSE, EPERM) == 0)
if (priv_policy(cr, CAP_DAC_OVERRIDE, B_FALSE, EPERM, NULL) == 0)
return (0);

if (priv_policy(cr, CAP_DAC_READ_SEARCH, B_FALSE, EPERM) == 0)
if (priv_policy(cr, CAP_DAC_READ_SEARCH, B_FALSE, EPERM, NULL) == 0)
return (0);

return (EPERM);
Expand All @@ -120,7 +120,7 @@ secpolicy_vnode_chown(const cred_t *cr, uid_t owner)
if (crgetfsuid(cr) == owner)
return (0);

return (priv_policy(cr, CAP_FOWNER, B_FALSE, EPERM));
return (priv_policy(cr, CAP_FOWNER, B_FALSE, EPERM, NULL));
}

/*
Expand All @@ -129,7 +129,7 @@ secpolicy_vnode_chown(const cred_t *cr, uid_t owner)
int
secpolicy_vnode_create_gid(const cred_t *cr)
{
return (priv_policy(cr, CAP_SETGID, B_FALSE, EPERM));
return (priv_policy(cr, CAP_SETGID, B_FALSE, EPERM, NULL));
}

/*
Expand All @@ -139,7 +139,7 @@ secpolicy_vnode_create_gid(const cred_t *cr)
int
secpolicy_vnode_remove(const cred_t *cr)
{
return (priv_policy(cr, CAP_FOWNER, B_FALSE, EPERM));
return (priv_policy(cr, CAP_FOWNER, B_FALSE, EPERM, NULL));
}

/*
Expand All @@ -152,7 +152,7 @@ secpolicy_vnode_setdac(const cred_t *cr, uid_t owner)
if (crgetfsuid(cr) == owner)
return (0);

return (priv_policy(cr, CAP_FOWNER, B_FALSE, EPERM));
return (priv_policy(cr, CAP_FOWNER, B_FALSE, EPERM, NULL));
}

/*
Expand All @@ -175,8 +175,9 @@ secpolicy_vnode_setid_retain(const cred_t *cr, boolean_t issuidroot)
int
secpolicy_vnode_setids_setgids(const cred_t *cr, gid_t gid)
{
struct user_namespace *ns = current_user_ns();
if (crgetfsgid(cr) != gid && !groupmember(gid, cr))
return (priv_policy(cr, CAP_FSETID, B_FALSE, EPERM));
return (kgid_has_mapping(ns, SGID_TO_KGID(gid)) && priv_policy(cr, CAP_FSETID, B_FALSE, EPERM, ns));

This comment has been minimized.

Sign in to view

Copy link
@behlendorf

behlendorf Oct 30, 2017

We need to preserve the returned errno here.

return (0);
}
Expand All @@ -188,7 +189,7 @@ secpolicy_vnode_setids_setgids(const cred_t *cr, gid_t gid)
int
secpolicy_zinject(const cred_t *cr)
{
return (priv_policy(cr, CAP_SYS_ADMIN, B_FALSE, EACCES));
return (priv_policy(cr, CAP_SYS_ADMIN, B_FALSE, EACCES, NULL));
}

/*
Expand All @@ -198,7 +199,7 @@ secpolicy_zinject(const cred_t *cr)
int
secpolicy_zfs(const cred_t *cr)
{
return (priv_policy(cr, CAP_SYS_ADMIN, B_FALSE, EACCES));
return (priv_policy(cr, CAP_SYS_ADMIN, B_FALSE, EACCES, NULL));
}

void
Expand All @@ -222,7 +223,7 @@ secpolicy_vnode_setid_modify(const cred_t *cr, uid_t owner)
if (crgetfsuid(cr) == owner)
return (0);

return (priv_policy(cr, CAP_FSETID, B_FALSE, EPERM));
return (priv_policy(cr, CAP_FSETID, B_FALSE, EPERM, NULL));
}

/*
Expand Down

0 comments on commit dd97fcf

Please sign in to comment.