On Tue, 3 Nov 2015 10:20:38 -0800, Kees Cook wrote:
> On Mon, Nov 2, 2015 at 4:39 PM, Dirk Steinmetz
> <public@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > ---
> >
> > 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.
Thank you for that beautiful demonstration!
I agree. I was unaware that sgid-executables on foreign groups were already
considered as insecure, as the documentation states
"As a security measure, depending on the filesystem, the set-user-ID
and set-group-ID execution bits may be turned off if a file is written.
(On Linux this occurs if the writing process does not have the
CAP_FSETID capability.)" [1] -- I blindly assumed that this would be the
case for most filesystems and incorrectly derived that sgid with foreign
groups are a supported scenario. As you demonstrated, they are not, and
thus this patch and the sgid-issue discussed are irrelevant, as the
scenario is unsafe by design and not by error.
The only question remaining is whether the documentation should be
re-worded to prevent other people from doing the same mistake and assuming
the scenario would be supported.
However, I'm unsure how to word it properly; my best guess would be
"The set-user-ID and set-group-ID execution bits may be turned off if a
file is written. (On Linux, this is guaranteed to only happen if the
writing process does not have the CAP_FSETID capability. You should not
rely on this behavior as a security measure: any user having write access
to or owning the file should be considered as able to impersonate the
file's owner/group-owner)"
- Dirk
Reference:
[1] <http://man7.org/linux/man-pages/man2/chmod.2.html>
>
> -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
> >
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
