* [PATCH] signal: always clear sa_restorer on execve
@ 2013-03-11 19:11 Kees Cook
2013-03-11 19:28 ` Oleg Nesterov
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2013-03-11 19:11 UTC (permalink / raw)
To: linux-kernel
Cc: Al Viro, Oleg Nesterov, Andrew Morton, Eric W. Biederman,
Serge Hallyn, Emese Revfy, PaX Team
When the new signal handlers are set up for a fork, 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().
$ 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: stable@vger.kernel.org
---
kernel/signal.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/signal.c b/kernel/signal.c
index 2ec870a..8c8e3ca 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -485,6 +485,9 @@ flush_signal_handlers(struct task_struct *t, int force_default)
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++;
}
--
1.7.9.5
--
Kees Cook
Chrome OS Security
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] signal: always clear sa_restorer on execve
2013-03-11 19:11 [PATCH] signal: always clear sa_restorer on execve Kees Cook
@ 2013-03-11 19:28 ` Oleg Nesterov
2013-03-11 19:34 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2013-03-11 19:28 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Al Viro, Andrew Morton, Eric W. Biederman,
Serge Hallyn, Emese Revfy, PaX Team
On 03/11, Kees Cook wrote:
>
> When the new signal handlers are set up for a fork, 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().
I don't understand.
fork() should not change restorer/etc, and the child has the same address
space anyway. There is no any leak and the patch can't make any difference
in this case because flush_signal_handlers() is not called by fork().
> @@ -485,6 +485,9 @@ flush_signal_handlers(struct task_struct *t, int force_default)
> 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
However, exec sets SIG_DFL but keeps ->sa_restorer, so probably this
patch makes sense anyway.
Oleg.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] signal: always clear sa_restorer on execve
2013-03-11 19:28 ` Oleg Nesterov
@ 2013-03-11 19:34 ` Kees Cook
2013-03-11 19:38 ` Oleg Nesterov
2013-03-11 19:42 ` Eric W. Biederman
0 siblings, 2 replies; 6+ messages in thread
From: Kees Cook @ 2013-03-11 19:34 UTC (permalink / raw)
To: Oleg Nesterov
Cc: LKML, Al Viro, Andrew Morton, Eric W. Biederman, Serge Hallyn,
Emese Revfy, PaX Team
On Mon, Mar 11, 2013 at 12:28 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/11, Kees Cook wrote:
>>
>> When the new signal handlers are set up for a fork, 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().
>
> I don't understand.
>
> fork() should not change restorer/etc, and the child has the same address
> space anyway. There is no any leak and the patch can't make any difference
> in this case because flush_signal_handlers() is not called by fork().
I probably failed to explain this correctly. From the perspective of
what should be considered "secret", it only matters across the exec,
not the fork (since the VMAs haven't changed until the exec). But the
info leak is easy to see, and this patch fixes it. As you say, since
other things were reset, so should sa_restorer.
>
>> @@ -485,6 +485,9 @@ flush_signal_handlers(struct task_struct *t, int force_default)
>> 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
>
> However, exec sets SIG_DFL but keeps ->sa_restorer, so probably this
> patch makes sense anyway.
>
> Oleg.
>
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] signal: always clear sa_restorer on execve
2013-03-11 19:34 ` Kees Cook
@ 2013-03-11 19:38 ` Oleg Nesterov
2013-03-11 19:42 ` Eric W. Biederman
1 sibling, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2013-03-11 19:38 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Al Viro, Andrew Morton, Eric W. Biederman, Serge Hallyn,
Emese Revfy, PaX Team
On 03/11, Kees Cook wrote:
> On Mon, Mar 11, 2013 at 12:28 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 03/11, Kees Cook wrote:
> >>
> >> When the new signal handlers are set up for a fork, 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().
> >
> > I don't understand.
> >
> > fork() should not change restorer/etc, and the child has the same address
> > space anyway. There is no any leak and the patch can't make any difference
> > in this case because flush_signal_handlers() is not called by fork().
>
> I probably failed to explain this correctly. From the perspective of
> what should be considered "secret", it only matters across the exec,
> not the fork (since the VMAs haven't changed until the exec).
Exactly. So may be you should update the changelog? It really looks
as if the parent should "hide" something from the forked child.
> But the
> info leak is easy to see, and this patch fixes it. As you say, since
> other things were reset, so should sa_restorer.
Yes, I didn't argue with the patch itself.
Oleg.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] signal: always clear sa_restorer on execve
2013-03-11 19:34 ` Kees Cook
2013-03-11 19:38 ` Oleg Nesterov
@ 2013-03-11 19:42 ` Eric W. Biederman
2013-03-11 20:02 ` Kees Cook
1 sibling, 1 reply; 6+ messages in thread
From: Eric W. Biederman @ 2013-03-11 19:42 UTC (permalink / raw)
To: Kees Cook
Cc: Oleg Nesterov, LKML, Al Viro, Andrew Morton, Serge Hallyn,
Emese Revfy, PaX Team
Kees Cook <keescook@chromium.org> writes:
> On Mon, Mar 11, 2013 at 12:28 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>> On 03/11, Kees Cook wrote:
>>>
>>> When the new signal handlers are set up for a fork, 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().
>>
>> I don't understand.
>>
>> fork() should not change restorer/etc, and the child has the same address
>> space anyway. There is no any leak and the patch can't make any difference
>> in this case because flush_signal_handlers() is not called by fork().
>
> I probably failed to explain this correctly. From the perspective of
> what should be considered "secret", it only matters across the exec,
> not the fork (since the VMAs haven't changed until the exec). But the
> info leak is easy to see, and this patch fixes it. As you say, since
> other things were reset, so should sa_restorer.
At the very least please correct the explanation in your patch
description.
Too often I have had seen a confused patch description, indicate
confusion elsewhere in the patch. Let's make it easy for reviewers
and future bisectors to understand what is intended.
>>> @@ -485,6 +485,9 @@ flush_signal_handlers(struct task_struct *t, int force_default)
>>> 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
Also I am inclined to suggest this should be an inline function in a
header.
clear_sa_restorer(ka);
Just so we don't litter the code with #ifdefs.
Eric
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] signal: always clear sa_restorer on execve
2013-03-11 19:42 ` Eric W. Biederman
@ 2013-03-11 20:02 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2013-03-11 20:02 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Oleg Nesterov, LKML, Al Viro, Andrew Morton, Serge Hallyn,
Emese Revfy, PaX Team
On Mon, Mar 11, 2013 at 12:42 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Kees Cook <keescook@chromium.org> writes:
>
>> On Mon, Mar 11, 2013 at 12:28 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>>> On 03/11, Kees Cook wrote:
>>>>
>>>> When the new signal handlers are set up for a fork, 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().
>>>
>>> I don't understand.
>>>
>>> fork() should not change restorer/etc, and the child has the same address
>>> space anyway. There is no any leak and the patch can't make any difference
>>> in this case because flush_signal_handlers() is not called by fork().
>>
>> I probably failed to explain this correctly. From the perspective of
>> what should be considered "secret", it only matters across the exec,
>> not the fork (since the VMAs haven't changed until the exec). But the
>> info leak is easy to see, and this patch fixes it. As you say, since
>> other things were reset, so should sa_restorer.
>
> At the very least please correct the explanation in your patch
> description.
>
> Too often I have had seen a confused patch description, indicate
> confusion elsewhere in the patch. Let's make it easy for reviewers
> and future bisectors to understand what is intended.
Sounds good. I'll re-attempt and send a v2.
>>>> @@ -485,6 +485,9 @@ flush_signal_handlers(struct task_struct *t, int force_default)
>>>> 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
>
> Also I am inclined to suggest this should be an inline function in a
> header.
> clear_sa_restorer(ka);
>
> Just so we don't litter the code with #ifdefs.
Yeah, I took a look, and the code was already using the __ARCH_HAS...
ifdef, so I went with that. I can respin with some kind of
set_sa_restorer() for the other places sa_restorer is used.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-03-11 20:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-11 19:11 [PATCH] signal: always clear sa_restorer on execve Kees Cook
2013-03-11 19:28 ` Oleg Nesterov
2013-03-11 19:34 ` Kees Cook
2013-03-11 19:38 ` Oleg Nesterov
2013-03-11 19:42 ` Eric W. Biederman
2013-03-11 20:02 ` Kees Cook
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.