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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALYGNiNn7m=AS9Zopa1g+TT5key=bEi7pKzGz3LzazQfm28qUw@mail.gmail.com>
Date:	Thu, 14 Jan 2016 10:35:17 +0300
From:	Konstantin Khlebnikov <koct9i@...il.com>
To:	Kees Cook <keescook@...omium.org>
Cc:	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>
Subject: Re: [PATCH v8] fs: clear file privilege bits when mmap writing

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.

>
> -Kees
>
> --
> Kees Cook
> Chrome OS & Brillo Security