elf_set_personality()

elf_set_personality()

[Peter De Schrijver]

Hi,

Commit ID 5e143436d04465c937c1a242808a99c46393af3e
(fix personality flag propagation across an exec) makes a child process
inherit a number of personality flags from its parent. This includes the
READ_IMPLIES_EXEC flag. Unfortunately this causes problems when debugging
android applications using gdbserver. It appears that gdbserver on android
has an executable stack. This causes the kernel to set the READ_IMPLIES_EXEC
flag on the gdbserver process. So the child android process will also get this
flag. As soon as the android tries to mmap a readonly android shmem segment
(using the ashmem driver), ashmem will return EPERM, because the segment has
been exposed as readonly without exec permissions and the kernel will modify
any readonly mmap request into read and execute.

Why does the READ_IMPLIES_EXEC flag need to be propagated? Would it be
acceptable to not propagate it to child processes?

Thanks,

Peter.

elf_set_personality()

[Russell King - ARM Linux]

On Mon, Feb 27, 2012 at 02:36:12PM +0200, Peter De Schrijver wrote:
> Commit ID 5e143436d04465c937c1a242808a99c46393af3e
> (fix personality flag propagation across an exec) makes a child process
> inherit a number of personality flags from its parent. This includes the
> READ_IMPLIES_EXEC flag.

So, READ_IMPLIES_EXEC will only be leaked if it's already set.

> Unfortunately this causes problems when debugging
> android applications using gdbserver. It appears that gdbserver on android
> has an executable stack. This causes the kernel to set the READ_IMPLIES_EXEC
> flag on the gdbserver process.

Ok.

> So the child android process will also get this flag.

And that means that attempting to mmap() stuff will also get executable
protections in addition.

> As soon as the android tries to mmap a readonly android shmem segment
> (using the ashmem driver), ashmem will return EPERM, because the segment has
> been exposed as readonly without exec permissions and the kernel will modify
> any readonly mmap request into read and execute.

This sounds like a problem.  If you have two applications trying to use
the ashmem driver, one without READ_IMPLIES_EXEC and one with
READ_IMPLIES_EXEC, then it seems that ashmem will prevent the
READ_IMPLIES_EXEC one from using such regions.  That sounds like a
(different) bug to me.

> Why does the READ_IMPLIES_EXEC flag need to be propagated? Would it be
> acceptable to not propagate it to child processes?

It shouldn't propagate.  But I think it's uncovered a separate problem
in the ashmem driver.

elf_set_personality()

[Peter De Schrijver]

On Mon, Feb 27, 2012 at 02:04:53PM +0100, Russell King - ARM Linux wrote:
> On Mon, Feb 27, 2012 at 02:36:12PM +0200, Peter De Schrijver wrote:
> > Commit ID 5e143436d04465c937c1a242808a99c46393af3e
> > (fix personality flag propagation across an exec) makes a child process
> > inherit a number of personality flags from its parent. This includes the
> > READ_IMPLIES_EXEC flag.
> 
> So, READ_IMPLIES_EXEC will only be leaked if it's already set.
> 
> > Unfortunately this causes problems when debugging
> > android applications using gdbserver. It appears that gdbserver on android
> > has an executable stack. This causes the kernel to set the READ_IMPLIES_EXEC
> > flag on the gdbserver process.
> 
> Ok.
> 
> > So the child android process will also get this flag.
> 
> And that means that attempting to mmap() stuff will also get executable
> protections in addition.
> 
> > As soon as the android tries to mmap a readonly android shmem segment
> > (using the ashmem driver), ashmem will return EPERM, because the segment has
> > been exposed as readonly without exec permissions and the kernel will modify
> > any readonly mmap request into read and execute.
> 
> This sounds like a problem.  If you have two applications trying to use
> the ashmem driver, one without READ_IMPLIES_EXEC and one with
> READ_IMPLIES_EXEC, then it seems that ashmem will prevent the
> READ_IMPLIES_EXEC one from using such regions.  That sounds like a
> (different) bug to me.
> 

Good point. I don't know anything about the design ideas behind ashmem though.
Can anyone from android comment on this?

> > Why does the READ_IMPLIES_EXEC flag need to be propagated? Would it be
> > acceptable to not propagate it to child processes?
> 
> It shouldn't propagate.  But I think it's uncovered a separate problem

Ok. Should I write a patch for this? Seems easy enough to also clear the
READ_IMPLIES_EXEC bit when copying the child personality from the parent?

> in the ashmem driver.

Cheers,

Peter.

elf_set_personality()

[Robert Love]

On Mon, Feb 27, 2012 at 10:03 AM, Peter De Schrijver
<pdeschrijver@nvidia.com> wrote:
> On Mon, Feb 27, 2012 at 02:04:53PM +0100, Russell King - ARM Linux wrote:
> > This sounds like a problem. ?If you have two applications trying to use
> > the ashmem driver, one without READ_IMPLIES_EXEC and one with
> > READ_IMPLIES_EXEC, then it seems that ashmem will prevent the
> > READ_IMPLIES_EXEC one from using such regions. ?That sounds like a
> > (different) bug to me.
>
> Good point. I don't know anything about the design ideas behind ashmem
> though.
> Can anyone from android comment on this?

The problem is this code snippet:

    /* requested protection bits must match our allowed protection mask */
    if (unlikely((vma->vm_flags & ~asma->prot_mask) & PROT_MASK)) {
            ret = -EPERM;
	    goto out;
    }

Coupled with this snippet:

    /* does the application expect PROT_READ to imply PROT_EXEC? */
    if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
            prot |= PROT_EXEC;

One fix is to remove the second snippet altogether. I added it to be
diligent; Android doesn't have a specific use for it afaik.

An alternative is to keep the code as-is. Note the bug isn't quite as
described: It isn't the case that two processes, one with
READ_IMPLIES_EXEC and one without, will always fail to both map an
ashmem region. The failure case is when a process creates a region
PROT_READ & ~PROT_EXEC and then a second process with
READ_IMPLIES_EXEC tries to map the region PROT_READ with the implicit
PROT_EXEC. I'm not sure what to do here. This seems like a legit
reason to fail.

          Robert

elf_set_personality()

[Russell King - ARM Linux]

On Mon, Feb 27, 2012 at 10:20:23AM -0500, Robert Love wrote:
> On Mon, Feb 27, 2012 at 10:03 AM, Peter De Schrijver
> <pdeschrijver@nvidia.com> wrote:
> > On Mon, Feb 27, 2012 at 02:04:53PM +0100, Russell King - ARM Linux wrote:
> > > This sounds like a problem. ?If you have two applications trying to use
> > > the ashmem driver, one without READ_IMPLIES_EXEC and one with
> > > READ_IMPLIES_EXEC, then it seems that ashmem will prevent the
> > > READ_IMPLIES_EXEC one from using such regions. ?That sounds like a
> > > (different) bug to me.
> >
> > Good point. I don't know anything about the design ideas behind ashmem
> > though.
> > Can anyone from android comment on this?
> 
> The problem is this code snippet:
> 
>     /* requested protection bits must match our allowed protection mask */
>     if (unlikely((vma->vm_flags & ~asma->prot_mask) & PROT_MASK)) {
>             ret = -EPERM;
> 	    goto out;
>     }
> 
> Coupled with this snippet:
> 
>     /* does the application expect PROT_READ to imply PROT_EXEC? */
>     if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
>             prot |= PROT_EXEC;

You're missing another place - mm/mmap.c:

        /*
         * Does the application expect PROT_READ to imply PROT_EXEC?
         *
         * (the exception is when the underlying filesystem is noexec
         *  mounted, in which case we dont add PROT_EXEC.)
         */
        if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
                if (!(file && (file->f_path.mnt->mnt_flags & MNT_NOEXEC)))
                        prot |= PROT_EXEC;

So, a thread with READ_IMPLIES_EXEC with a mmap() containing PROT_READ
will always appear with PROT_READ | PROT_EXEC, which then gets
translated to VM_READ | VM_EXEC.

> One fix is to remove the second snippet altogether. I added it to be
> diligent; Android doesn't have a specific use for it afaik.
> 
> An alternative is to keep the code as-is. Note the bug isn't quite as
> described: It isn't the case that two processes, one with
> READ_IMPLIES_EXEC and one without, will always fail to both map an
> ashmem region. The failure case is when a process creates a region
> PROT_READ & ~PROT_EXEC and then a second process with
> READ_IMPLIES_EXEC tries to map the region PROT_READ with the implicit
> PROT_EXEC. I'm not sure what to do here. This seems like a legit
> reason to fail.

It seems that vanilla mmap() of a file does not deny a mapping containing
PROT_EXEC of a file with read-write-noexec permissions - it permits it,
but it does fail mapping a file PROT_WRITE which wasn't opened for write.

Moreover, it's not actually possible to prevent execution of code if
you have read permission - if you can read a mapping, you can copy it
into an executable mapping and then execute copied code.

So, I don't think there's any reason to prevent PROT_EXEC in a hard and
fast manner.  If you don't want to go that far, what about:

	prot_mask = PROT_MASK;
	if (current->personality & READ_IMPLIES_EXEC)
		prot_mask &= ~PROT_EXEC;
	if (vma->vm_flags & ~asma->prot_mask & prot_mask) {
		ret = -EPERM;
		goto out;
	}

which would mean !READ_IMPLIES_EXEC threads would get the full permission
checking, but a READ_IMPLIES_EXEC thread would still be able to attach
to a r/w only shared mapping.

elf_set_personality()

[Robert Love]

On Mon, Feb 27, 2012 at 11:41 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> It seems that vanilla mmap() of a file does not deny a mapping containing
> PROT_EXEC of a file with read-write-noexec permissions - it permits it,
> but it does fail mapping a file PROT_WRITE which wasn't opened for write.
>
> Moreover, it's not actually possible to prevent execution of code if
> you have read permission - if you can read a mapping, you can copy it
> into an executable mapping and then execute copied code.
>
> So, I don't think there's any reason to prevent PROT_EXEC in a hard and
> fast manner. ?If you don't want to go that far, what about:
>
> ? ? ? ?prot_mask = PROT_MASK;
> ? ? ? ?if (current->personality & READ_IMPLIES_EXEC)
> ? ? ? ? ? ? ? ?prot_mask &= ~PROT_EXEC;
> ? ? ? ?if (vma->vm_flags & ~asma->prot_mask & prot_mask) {
> ? ? ? ? ? ? ? ?ret = -EPERM;
> ? ? ? ? ? ? ? ?goto out;
> ? ? ? ?}
>
> which would mean !READ_IMPLIES_EXEC threads would get the full permission
> checking, but a READ_IMPLIES_EXEC thread would still be able to attach
> to a r/w only shared mapping.

ashmem should behave however vanilla mmap behaves. There is no reason
to be different and I'd prefer to consolidate or at least copy the
code.

It is unfortunate that each implementer of fops->mmap needs to
manually handle the READ_IMPLIES_EXEC case.

        Robert

elf_set_personality()

[Russell King - ARM Linux]

On Mon, Feb 27, 2012 at 12:09:00PM -0500, Robert Love wrote:
> On Mon, Feb 27, 2012 at 11:41 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > It seems that vanilla mmap() of a file does not deny a mapping containing
> > PROT_EXEC of a file with read-write-noexec permissions - it permits it,
> > but it does fail mapping a file PROT_WRITE which wasn't opened for write.
> >
> > Moreover, it's not actually possible to prevent execution of code if
> > you have read permission - if you can read a mapping, you can copy it
> > into an executable mapping and then execute copied code.
> >
> > So, I don't think there's any reason to prevent PROT_EXEC in a hard and
> > fast manner. ?If you don't want to go that far, what about:
> >
> > ? ? ? ?prot_mask = PROT_MASK;
> > ? ? ? ?if (current->personality & READ_IMPLIES_EXEC)
> > ? ? ? ? ? ? ? ?prot_mask &= ~PROT_EXEC;
> > ? ? ? ?if (vma->vm_flags & ~asma->prot_mask & prot_mask) {
> > ? ? ? ? ? ? ? ?ret = -EPERM;
> > ? ? ? ? ? ? ? ?goto out;
> > ? ? ? ?}
> >
> > which would mean !READ_IMPLIES_EXEC threads would get the full permission
> > checking, but a READ_IMPLIES_EXEC thread would still be able to attach
> > to a r/w only shared mapping.
> 
> ashmem should behave however vanilla mmap behaves.

Well, it's fairly clear that it doesn't, because it has more stringent
permission checks than vanilla mmap.

> There is no reason to be different and I'd prefer to consolidate or at
> least copy the code.
> 
> It is unfortunate that each implementer of fops->mmap needs to
> manually handle the READ_IMPLIES_EXEC case.

That would mean ignoring PROT_EXEC entirely.

elf_set_personality()

[Robert Love]

On Mon, Feb 27, 2012 at 12:16 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>> There is no reason to be different and I'd prefer to consolidate or at
>> least copy the code.
>>
>> It is unfortunate that each implementer of fops->mmap needs to
>> manually handle the READ_IMPLIES_EXEC case.
>
> That would mean ignoring PROT_EXEC entirely.

Right. I was agreeing with your first suggestion.

As goes vanilla mmap, so goes ashmem.

I no longer work on Android, but I can put together a patch.

      Robert