Quoting Dirk Steinmetz ([email protected]): > In order to hardlink to a sgid-executable, it is sufficient to be the > file's owner. When hardlinking within an unprivileged user namespace, the > users of that namespace could thus use hardlinks to pin setgid binaries > owned by themselves (or any mapped uid, with CAP_FOWNER) and a gid outside > of the namespace. This is a possible security risk. > > This change prevents hardlinking of sgid-executables within user > namespaces, if the file is not owned by a mapped gid. > > Signed-off-by: Dirk Steinmetz <[email protected]>
Hey, Hoping this gets a close review by Kees, but this looks good to me, thanks! Acked-by: Serge E. Hallyn <[email protected]> > --- > > MISSING: Documentation/sysctl/fs.txt not updated, as this patch is > intended for discussion. > > If there are no further misunderstandings on my side, this patch is what > Serge and I agree on (modulo my not-that-much-linux-kernel-experience > codestyle, feel free to suggest improvements!). > > The new condition for sgid-executables is equivalent to > > inode_owner_or_capable(inode) && kgid_has_mapping(ns, inode->i_gid) > which, as recommended by Serge, does not change the behaviour for the init > namespace. It fixes the problem of pinning parent namespace's gids. > > However, I think the "same" security issue is also valid within any > namespace, for regular users pinning other gids within the same namespace. > I already presented an example for that in a previous mail: > - A file has the setgid and user/group executable bits set, and is owned > by user:group. > - The user 'user' is not in the group 'group', and does not have any > capabilities. > - The user 'user' hardlinks the file. The permission check will succeed, > as the user is the owner of the file. > - The file is replaced with a newer version (for example fixing a security > issue) > - Now user can still use the hardlink-pinned version to execute the file > as 'user:group' (and for example exploit the security issue). > > To prevent that, the condition would need to be changed to something like > inode_group_or_capable, resembling inode_owner_or_capable, but checking > that the caller is in the group the inode belongs to or has some > capability (for consistency with former behaviour, CAP_FOWNER? for > consistency with the documentation, CAP_FSETID?). However, this would > change userland behaviour outside of userns. Thus my main question: > Is the scenario above bad enough to change userland behaviour? > > I'd apprechiate your comments. > > - Dirk > > > Diffstat: > fs/namei.c | 47 ++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 38 insertions(+), 9 deletions(-) > > diff --git a/fs/namei.c b/fs/namei.c > index 29fc6a6..9c6c2e2 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -913,18 +913,19 @@ static inline int may_follow_link(struct nameidata *nd) > } > > /** > - * safe_hardlink_source - Check for safe hardlink conditions > + * safe_hardlink_source_uid - Check for safe hardlink conditions not > dependent > + * on the inode's group. These conditions may be overridden by inode > ownership > + * or CAP_FOWNER with respect to the inode's uid > * @inode: the source inode to hardlink from > * > * Return false if at least one of the following conditions: > * - inode is not a regular file > * - inode is setuid > - * - inode is setgid and group-exec > * - access failure for read and write > * > * Otherwise returns true. > */ > -static bool safe_hardlink_source(struct inode *inode) > +static bool safe_hardlink_source_uid(struct inode *inode) > { > umode_t mode = inode->i_mode; > > @@ -936,10 +937,6 @@ static bool safe_hardlink_source(struct inode *inode) > if (mode & S_ISUID) > return false; > > - /* Executable setgid files should not get pinned to the filesystem. */ > - if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) > - return false; > - > /* Hardlinking to unreadable or unwritable sources is dangerous. */ > if (inode_permission(inode, MAY_READ | MAY_WRITE)) > return false; > @@ -948,30 +945,62 @@ static bool safe_hardlink_source(struct inode *inode) > } > > /** > + * safe_hardlink_source_gid - Check for safe hardlink conditions dependent > + * on the inode's group. These conditions may be overridden by inode > ownership > + * or CAP_FOWNER with respect to the inode's gid > + * @inode: the source inode to hardlink from > + * > + * Return false if inode is setgid and group-exec > + * > + * Otherwise returns true. > + */ > +static bool safe_hardlink_source_gid(struct inode *inode) > +{ > + umode_t mode = inode->i_mode; > + > + /* Executable setgid files should not get pinned to the filesystem. */ > + if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) > + return false; > + > + return true; > +} > + > +/** > * may_linkat - Check permissions for creating a hardlink > * @link: the source to hardlink from > * > * Block hardlink when all of: > * - sysctl_protected_hardlinks enabled > * - fsuid does not match inode > - * - hardlink source is unsafe (see safe_hardlink_source() above) > + * - hardlink source is unsafe (see safe_hardlink_source_*() above) > * - not CAP_FOWNER in a namespace with the inode owner uid mapped > + * (and inode gid mapped, if hardlink conditions depending on the inode's > + * group are not satisfied) > * > * Returns 0 if successful, -ve on error. > */ > static int may_linkat(struct path *link) > { > struct inode *inode; > + struct user_namespace *ns; > + bool owner; > + bool safe_uid; > + bool safe_gid; > > if (!sysctl_protected_hardlinks) > return 0; > > inode = link->dentry->d_inode; > + ns = current_user_ns(); > > /* Source inode owner (or CAP_FOWNER) can hardlink all they like, > * otherwise, it must be a safe source. > */ > - if (inode_owner_or_capable(inode) || safe_hardlink_source(inode)) > + owner = inode_owner_or_capable(inode); > + safe_uid = safe_hardlink_source_uid(inode) || owner; > + safe_gid = safe_hardlink_source_gid(inode) || > + (owner && kgid_has_mapping(ns, inode->i_gid)); > + if (safe_uid && safe_gid) > return 0; > > audit_log_link_denied("linkat", link); > -- > 2.1.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
