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

[Jan Kara]

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.

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.

Honestly, I don't see a way of implementing this in mprotect() which would
be reasonably elegant.

								Honza

-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR