linux-kernel - Re: [PATCH v8] fs: clear file privilege bits when mmap writing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20160115101759.GC15950@quack.suse.cz>
Date:	Fri, 15 Jan 2016 11:17:59 +0100
From:	Jan Kara <jack@...e.cz>
To:	Konstantin Khlebnikov <koct9i@...il.com>
Cc:	Kees Cook <keescook@...omium.org>, Jan Kara <jack@...e.cz>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Andy Lutomirski <luto@...capital.net>,
	yalin wang <yalin.wang2010@...il.com>,
	Willy Tarreau <w@....eu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux-MM <linux-mm@...ck.org>,
	LKML <linux-kernel@...r.kernel.org>, mfasheh@...e.de,
	ocfs2-devel@....oracle.com
Subject: Re: [PATCH v8] fs: clear file privilege bits when mmap writing

On Thu 14-01-16 10:35:17, Konstantin Khlebnikov wrote:
> On Wed, Jan 13, 2016 at 11:33 PM, Kees Cook <keescook@...omium.org> wrote:
> > On Wed, Jan 13, 2016 at 12:23 PM, Konstantin Khlebnikov
> > <koct9i@...il.com> wrote:
> >> On Wed, Jan 13, 2016 at 7:09 PM, Kees Cook <keescook@...omium.org> wrote:
> >>> On Wed, Jan 13, 2016 at 1:03 AM, Jan Kara <jack@...e.cz> wrote:
> >>>> On Tue 12-01-16 11:09:04, Kees Cook wrote:
> >>>>> Normally, when a user can modify a file that has setuid or setgid bits,
> >>>>> those bits are cleared when they are not the file owner or a member
> >>>>> of the group. This is enforced when using write and truncate but not
> >>>>> when writing to a shared mmap on the file. This could allow the file
> >>>>> writer to gain privileges by changing a binary without losing the
> >>>>> setuid/setgid/caps bits.
> >>>>>
> >>>>> Changing the bits requires holding inode->i_mutex, so it cannot be done
> >>>>> during the page fault (due to mmap_sem being held during the fault).
> >>>>> Instead, clear the bits if PROT_WRITE is being used at mmap open time,
> >>>>> or added at mprotect time.
> >>>>>
> >>>>> Since we can't do the check in the right place inside mmap (due to
> >>>>> holding mmap_sem), we have to do it before holding mmap_sem, which
> >>>>> means duplicating some checks, which have to be available to the non-MMU
> >>>>> builds too.
> >>>>>
> >>>>> When walking VMAs during mprotect, we need to drop mmap_sem (while
> >>>>> holding a file reference) and restart the walk after clearing privileges.
> >>>>
> >>>> ...
> >>>>
> >>>>> @@ -375,6 +376,7 @@ SYSCALL_DEFINE3(mprotect, unsigned long, start, size_t, len,
> >>>>>
> >>>>>       vm_flags = calc_vm_prot_bits(prot);
> >>>>>
> >>>>> +restart:
> >>>>>       down_write(&current->mm->mmap_sem);
> >>>>>
> >>>>>       vma = find_vma(current->mm, start);
> >>>>> @@ -416,6 +418,28 @@ SYSCALL_DEFINE3(mprotect, unsigned long, start, size_t, len,
> >>>>>                       goto out;
> >>>>>               }
> >>>>>
> >>>>> +             /*
> >>>>> +              * If we're adding write permissions to a shared file,
> >>>>> +              * we must clear privileges (like done at mmap time),
> >>>>> +              * but we have to juggle the locks to avoid holding
> >>>>> +              * mmap_sem while holding i_mutex.
> >>>>> +              */
> >>>>> +             if ((vma->vm_flags & VM_SHARED) && vma->vm_file &&
> >>>>> +                 (newflags & VM_WRITE) && !(vma->vm_flags & VM_WRITE) &&
> >>>>> +                 !IS_NOSEC(file_inode(vma->vm_file))) {
> >>>>
> >>>> This code assumes that IS_NOSEC gets set for inode once file_remove_privs()
> >>>> is called. However that is not true for two reasons:
> >>>>
> >>>> 1) When you are root, SUID bit doesn't get cleared and thus you cannot set
> >>>> IS_NOSEC.
> >>>>
> >>>> 2) Some filesystems do not have MS_NOSEC set and for those IS_NOSEC is
> >>>> never true.
> >>>>
> >>>> So in these cases you'll loop forever.
> >>>
> >>> UUuugh.
> >>>
> >>>>
> >>>> You can check SUID bits without i_mutex so that could be done without
> >>>> dropping mmap_sem but you cannot easily call security_inode_need_killpriv()
> >>>> without i_mutex as that checks extended attributes (IMA) and that needs
> >>>> i_mutex to be held to avoid races with someone else changing the attributes
> >>>> under you.
> >>>
> >>> Yeah, that's why I changed this from Konstantin's original suggestion.
> >>>
> >>>> Honestly, I don't see a way of implementing this in mprotect() which would
> >>>> be reasonably elegant.
> >>>
> >>> Konstantin, any thoughts here?
> >>
> >> Getxattr works fine without i_mutex: sys_getxattr/vfs_getxattr doesn't lock it.
> >> If somebody changes xattrs under us we'll end up in race anyway.
> >> But this still safe: setxattrs are sychronized.
> >
> > So I can swap my IS_NOSEC for your original file_needs_remove_privs()?
> > Are the LSM hooks expecting to be called under mm_sem? (Looks like
> > only common_caps implements that, though.)
> 
> getxattr should nests inside mmap_sem safely: it has sort of
> "readpage" semantics,
> actually ext4 uses it when inlines content of tiny files into xattr.

First, sorry Kees for misleading you. Somehow I missed that i_mutex is not
actually acquired for getxattr() calls.

I have checked and lots of filesystems have dedicated xattr semaphore which
should be safe to nest inside mmap_sem. There are filesystems like ocfs2 or
gfs2 which use their equivalent of i_mutex for xattr locking so there we
would create lock inversion when calling file_needs_remove_privs() from
under mmap_sem.

That being said at least OCFS2 has other issues with this xattr locking
scheme and they are working on changing things AFAIK. Mark can you perhaps
comment?

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR