Re: + signal-always-clear-sa_restorer-on-execve.patch added to -mm tree

Re: + signal-always-clear-sa_restorer-on-execve.patch added to -mm tree

[Kees Cook]

Hi,

A note for backporters: you'll likely want to change
__ARCH_HAS_SA_RESTORER to SA_RESTORER, since the former was recently
introduced. If not, this will apply but not actually do any good.

-Kees

On Mon, Mar 11, 2013 at 1:22 PM,  <akpm@linux-foundation.org> wrote:
>
> The patch titled
>      Subject: signal: always clear sa_restorer on execve
> has been added to the -mm tree.  Its filename is
>      signal-always-clear-sa_restorer-on-execve.patch
>
> Before you just go and hit "reply", please:
>    a) Consider who else should be cc'ed
>    b) Prefer to cc a suitable mailing list as well
>    c) Ideally: find the original patch on the mailing list and do a
>       reply-to-all to that, adding suitable additional cc's
>
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
>
> The -mm tree is included into linux-next and is updated
> there every 3-4 working days
>
> ------------------------------------------------------
> From: Kees Cook <keescook@chromium.org>
> Subject: signal: always clear sa_restorer on execve
>
> When the new signal handlers are set up, the location of sa_restorer is
> not cleared, leaking a parent process's address space location to
> children.  This allows for a potential bypass of the parent's ASLR by
> examining the sa_restorer value returned when calling sigaction().
>
> Based on what should be considered "secret" about addresses, it only
> matters across the exec not the fork (since the VMAs haven't changed until
> the exec).  But since exec sets SIG_DFL and keeps sa_restorer, this is
> where it should be fixed.
>
> Given the few uses of sa_restorer, a "set" function was not written since
> this would be the only use.  Instead, we use __ARCH_HAS_SA_RESTORER, as
> already done in other places.
>
> Example of the leak before applying this patch:
>
> $ cat /proc/$$/maps
> ...
> 7fb9f3083000-7fb9f3238000 r-xp 00000000 fd:01 404469 .../libc-2.15.so
> ...
> $ ./leak
> ...
> 7f278bc74000-7f278be29000 r-xp 00000000 fd:01 404469 .../libc-2.15.so
> ...
> 1 0 (nil) 0x7fb9f30b94a0
> 2 4000000 (nil) 0x7f278bcaa4a0
> 3 4000000 (nil) 0x7f278bcaa4a0
> 4 0 (nil) 0x7fb9f30b94a0
> ...
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Reported-by: Emese Revfy <re.emese@gmail.com>
> Cc: Emese Revfy <re.emese@gmail.com>
> Cc: PaX Team <pageexec@freemail.hu>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>
> Cc: Julien Tinnes <jln@google.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
>  kernel/signal.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff -puN kernel/signal.c~signal-always-clear-sa_restorer-on-execve kernel/signal.c
> --- a/kernel/signal.c~signal-always-clear-sa_restorer-on-execve
> +++ a/kernel/signal.c
> @@ -485,6 +485,9 @@ flush_signal_handlers(struct task_struct
>                 if (force_default || ka->sa.sa_handler != SIG_IGN)
>                         ka->sa.sa_handler = SIG_DFL;
>                 ka->sa.sa_flags = 0;
> +#ifdef __ARCH_HAS_SA_RESTORER
> +               ka->sa.sa_restorer = NULL;
> +#endif
>                 sigemptyset(&ka->sa.sa_mask);
>                 ka++;
>         }
> _
>
> Patches currently in -mm which might be from keescook@chromium.org are
>
> linux-next.patch
> binfmt_elfc-use-get_random_int-to-fix-entropy-depleting.patch
> signal-always-clear-sa_restorer-on-execve.patch
>



--
Kees Cook
Chrome OS Security

Re: + signal-always-clear-sa_restorer-on-execve.patch added to -mm tree

[Andrew Morton]

On Mon, 11 Mar 2013 13:37:53 -0700 Kees Cook <keescook@chromium.org> wrote:

> ...
>

(pop toasting undone)

> > Subject: signal: always clear sa_restorer on execve
> >
> > When the new signal handlers are set up, the location of sa_restorer is
> > not cleared, leaking a parent process's address space location to
> > children.  This allows for a potential bypass of the parent's ASLR by
> > examining the sa_restorer value returned when calling sigaction().
> >
> > Based on what should be considered "secret" about addresses, it only
> > matters across the exec not the fork (since the VMAs haven't changed until
> > the exec).  But since exec sets SIG_DFL and keeps sa_restorer, this is
> > where it should be fixed.
>
> A note for backporters: you'll likely want to change
> __ARCH_HAS_SA_RESTORER to SA_RESTORER, since the former was recently
> introduced. If not, this will apply but not actually do any good.

I added this to the changelog, but I fear people won't read it!  Is
there any clever way in which we can have one patch which will work OK
in both old and new kernels?  I can't think of one...

Re: + signal-always-clear-sa_restorer-on-execve.patch added to -mm tree

[Kees Cook]

On Mon, Mar 11, 2013 at 2:01 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 11 Mar 2013 13:37:53 -0700 Kees Cook <keescook@chromium.org> wrote:
>
>> ...
>>
>
> (pop toasting undone)
>
>> > Subject: signal: always clear sa_restorer on execve
>> >
>> > When the new signal handlers are set up, the location of sa_restorer is
>> > not cleared, leaking a parent process's address space location to
>> > children.  This allows for a potential bypass of the parent's ASLR by
>> > examining the sa_restorer value returned when calling sigaction().
>> >
>> > Based on what should be considered "secret" about addresses, it only
>> > matters across the exec not the fork (since the VMAs haven't changed until
>> > the exec).  But since exec sets SIG_DFL and keeps sa_restorer, this is
>> > where it should be fixed.
>>
>> A note for backporters: you'll likely want to change
>> __ARCH_HAS_SA_RESTORER to SA_RESTORER, since the former was recently
>> introduced. If not, this will apply but not actually do any good.
>
> I added this to the changelog, but I fear people won't read it!  Is
> there any clever way in which we can have one patch which will work OK
> in both old and new kernels?  I can't think of one...

Using just SA_RESTORER will work in both cases, but isn't "correct"
going forward. :(

-Kees

-- 
Kees Cook
Chrome OS Security

Re: + signal-always-clear-sa_restorer-on-execve.patch added to -mm tree

[Greg KH]

On Mon, Mar 11, 2013 at 02:01:30PM -0700, Andrew Morton wrote:
> On Mon, 11 Mar 2013 13:37:53 -0700 Kees Cook <keescook@chromium.org> wrote:
> 
> > ...
> >
> 
> (pop toasting undone)
> 
> > > Subject: signal: always clear sa_restorer on execve
> > >
> > > When the new signal handlers are set up, the location of sa_restorer is
> > > not cleared, leaking a parent process's address space location to
> > > children.  This allows for a potential bypass of the parent's ASLR by
> > > examining the sa_restorer value returned when calling sigaction().
> > >
> > > Based on what should be considered "secret" about addresses, it only
> > > matters across the exec not the fork (since the VMAs haven't changed until
> > > the exec).  But since exec sets SIG_DFL and keeps sa_restorer, this is
> > > where it should be fixed.
> >
> > A note for backporters: you'll likely want to change
> > __ARCH_HAS_SA_RESTORER to SA_RESTORER, since the former was recently
> > introduced. If not, this will apply but not actually do any good.
> 
> I added this to the changelog, but I fear people won't read it!  Is
> there any clever way in which we can have one patch which will work OK
> in both old and new kernels?  I can't think of one...

I'll store it in my stable inbox and will hope to remember it when it
hits Linus's tree...

thanks,

greg k-h

Re: + signal-always-clear-sa_restorer-on-execve.patch added to -mm tree

[Andrew Morton]

On Mon, 11 Mar 2013 14:03:20 -0700 Kees Cook <keescook@chromium.org> wrote:

> On Mon, Mar 11, 2013 at 2:01 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Mon, 11 Mar 2013 13:37:53 -0700 Kees Cook <keescook@chromium.org> wrote:
> >
> >> ...
> >>
> >
> > (pop toasting undone)
> >
> >> > Subject: signal: always clear sa_restorer on execve
> >> >
> >> > When the new signal handlers are set up, the location of sa_restorer is
> >> > not cleared, leaking a parent process's address space location to
> >> > children.  This allows for a potential bypass of the parent's ASLR by
> >> > examining the sa_restorer value returned when calling sigaction().
> >> >
> >> > Based on what should be considered "secret" about addresses, it only
> >> > matters across the exec not the fork (since the VMAs haven't changed until
> >> > the exec).  But since exec sets SIG_DFL and keeps sa_restorer, this is
> >> > where it should be fixed.
> >>
> >> A note for backporters: you'll likely want to change
> >> __ARCH_HAS_SA_RESTORER to SA_RESTORER, since the former was recently
> >> introduced. If not, this will apply but not actually do any good.
> >
> > I added this to the changelog, but I fear people won't read it!  Is
> > there any clever way in which we can have one patch which will work OK
> > in both old and new kernels?  I can't think of one...
> 
> Using just SA_RESTORER will work in both cases, but isn't "correct"
> going forward. :(

That's easy.

patch #1: use SA_RESTORER, cc stable (please promise me this will work OK)
patch #2: switch to __ARCH_HAS_SA_RESTORER, no cc stable

I'm assuming this is all for 3.10, btw.  If you think it should be in
3.9 then you need to write scarier changelogs.

Re: + signal-always-clear-sa_restorer-on-execve.patch added to -mm tree

[Kees Cook]

On Mon, Mar 11, 2013 at 2:22 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 11 Mar 2013 14:03:20 -0700 Kees Cook <keescook@chromium.org> wrote:
>
>> On Mon, Mar 11, 2013 at 2:01 PM, Andrew Morton
>> <akpm@linux-foundation.org> wrote:
>> > On Mon, 11 Mar 2013 13:37:53 -0700 Kees Cook <keescook@chromium.org> wrote:
>> >
>> >> ...
>> >>
>> >
>> > (pop toasting undone)
>> >
>> >> > Subject: signal: always clear sa_restorer on execve
>> >> >
>> >> > When the new signal handlers are set up, the location of sa_restorer is
>> >> > not cleared, leaking a parent process's address space location to
>> >> > children.  This allows for a potential bypass of the parent's ASLR by
>> >> > examining the sa_restorer value returned when calling sigaction().
>> >> >
>> >> > Based on what should be considered "secret" about addresses, it only
>> >> > matters across the exec not the fork (since the VMAs haven't changed until
>> >> > the exec).  But since exec sets SIG_DFL and keeps sa_restorer, this is
>> >> > where it should be fixed.
>> >>
>> >> A note for backporters: you'll likely want to change
>> >> __ARCH_HAS_SA_RESTORER to SA_RESTORER, since the former was recently
>> >> introduced. If not, this will apply but not actually do any good.
>> >
>> > I added this to the changelog, but I fear people won't read it!  Is
>> > there any clever way in which we can have one patch which will work OK
>> > in both old and new kernels?  I can't think of one...
>>
>> Using just SA_RESTORER will work in both cases, but isn't "correct"
>> going forward. :(
>
> That's easy.
>
> patch #1: use SA_RESTORER, cc stable (please promise me this will work OK)

I don't have the ability to build all the architectures, but it seems
like the best flag based on code review.

> patch #2: switch to __ARCH_HAS_SA_RESTORER, no cc stable
>
> I'm assuming this is all for 3.10, btw.  If you think it should be in
> 3.9 then you need to write scarier changelogs.

Well, I'm not sure it needs to be scarier; it's a serious local ASLR
info leak. I was hoping it could go into 3.9 since the expectation is
for it to go to stable, so why not put it in 3.9 now?

Do you want me to spin up the 2-patch solution or is it too much of a hack?

-Kees

-- 
Kees Cook
Chrome OS Security

Re: + signal-always-clear-sa_restorer-on-execve.patch added to -mm tree

[Andrew Morton]

On Mon, 11 Mar 2013 14:33:18 -0700 Kees Cook <keescook@chromium.org> wrote:

> > patch #1: use SA_RESTORER, cc stable (please promise me this will work OK)
> 
> I don't have the ability to build all the architectures, but it seems
> like the best flag based on code review.

SA_RESTORER looks OK back to 3.5 (at least).

> > patch #2: switch to __ARCH_HAS_SA_RESTORER, no cc stable
> >
> > I'm assuming this is all for 3.10, btw.  If you think it should be in
> > 3.9 then you need to write scarier changelogs.
> 
> Well, I'm not sure it needs to be scarier; it's a serious local ASLR
> info leak. I was hoping it could go into 3.9 since the expectation is
> for it to go to stable, so why not put it in 3.9 now?

OK.

> Do you want me to spin up the 2-patch solution or is it too much of a hack?

Is OK, I have it all lined up.