Message-ID: <CAGXu5j+i6PZNMwmseZc8G6V_5=5TK=dXP=bzLteK705bLdUU2A@mail.gmail.com>
Date:	Tue, 3 Nov 2015 10:20:38 -0800
From:	Kees Cook <keescook@...omium.org>
To:	Dirk Steinmetz <public@...tdrjgfuzkfg.com>
Cc:	Serge Hallyn <serge.hallyn@...ntu.com>,
	Seth Forshee <seth.forshee@...onical.com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Linux FS Devel <linux-fsdevel@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	"Eric W . Biederman" <ebiederm@...ssion.com>,
	Serge Hallyn <serge.hallyn@...onical.com>
Subject: Re: [RFC] namei: prevent sgid-hardlinks for unmapped gids

On Mon, Nov 2, 2015 at 4:39 PM, Dirk Steinmetz
<public@...tdrjgfuzkfg.com> wrote:
> 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.

How would such a file appear within the namespace? Wouldn't the gid
have to map to something inside the namespace?

>
> This change prevents hardlinking of sgid-executables within user
> namespaces, if the file is not owned by a mapped gid.

For clarity, this should say "... is not group-owned by a mapped git." correct?

> Signed-off-by: Dirk Steinmetz <public@...tdrjgfuzkfg.com>
> ---
>
> 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).

I believe this to be an unneeded check is the stated configuration
(setgid but without group ownership) is itself a security flaw. This
allows the user already to gain those group privileges even without
needing to pin the file or do anything:

setgid executable that reports euid and egid:

$ cat poof.c
#include <stdio.h>

int main(void)
{
    printf("%d:%d\n", geteuid(), getegid());
    return 0;
}
$ make poof
cc     poof.c   -o poof
$ sudo chgrp root poof && sudo chmod g+s poof
$ ls -la poof
-rwxr-s--- 1 keescook root 8658 Nov  3 10:14 poof
$ ./poof
149786:0

I am not a member of the 0 group:

$ id
uid=149786(keescook) gid=5000(eng)
groups=5000(eng),4(adm),20(dialout),21(fax),24(cdrom),25(floppy),26(tape),27(sudo),30(dip),44(video),46(plugdev),106(fuse),110(lpadmin),124(sambashare),129(pkcs11),133(libvirtd),999(logindev)

Now I mmap the file, and rewrite it (here I change the format string
from a : separator to a -, but we just just as easily injected code):

$ cat mod.c
#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/mman.h>

int main(void)
{
    int fd;
    struct stat info;
    unsigned char *ptr;
    off_t i;

    fd = open("poof", O_RDWR);
    fstat(fd, &info);
    ptr = mmap(NULL, info.st_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
    close(fd);

    for (i = 0; i < info.st_size; i++) {
        if (0 == strncmp(ptr + i, "%d:%d", 5)) {
            ptr[i + 2] = '-';
        }
    }
    munmap(ptr, info.st_size);

    system("./poof");

    return 0;
}
$ make mod
cc     mod.c   -o mod
$ ./mod
149786-0
$ ls -la poof
-rwxr-s--- 1 keescook root 8658 Nov  3 10:17 poof

So, I don't think this patch actually makes anything safer, though
there might be a namespace mapping element I've not understood.

-Kees

>
> 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
>



-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists