On Tue, Dec 30, 2014 at 10:40 AM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Wed, Dec 24, 2014 at 1:39 PM, Richard Weinberger <richard@xxxxxx> wrote:
>> While exploring the offset2lib attack I remembered that
>> grsecurity has an interesting feature to make such attacks
>> much harder. Exploits can brute stack canaries often very easily
>> if the target is a forking server like sshd or Apache httpd.
>> The problem is that after fork() the child has by definition
>> exactly the same memory as the parent and therefore also the same
>> stack canaries.
>> The attacker can guess the stack canaries byte by byte.
>> After 256 times 7 forks() a good exploit can find the correct
>> canary value.
>>
>> The basic idea behind this patch is to delay fork() if a child died
>> due to a fatal error.
>> Currently it delays fork() by 30 seconds if the parent tries to fork()
>> within 60 seconds after a child died due to a fatal error.
>>
>> I'm sure you'll hate this patch but I want to find out how much you hate it
>> and whether there is a little chance to get it mainline in a modified form.
>> Later I'd make it depend on a new Kconfig option and off by default
>> and the timing constants changeable via sysctl.
>>
>> Credit for the concept goes to grsecurity folks, I'll take the flames.
>>
>> Signed-off-by: Richard Weinberger <richard@xxxxxx>
>> ---
>> fs/coredump.c | 10 ++++++++++
>> include/linux/sched.h | 1 +
>> kernel/fork.c | 7 +++++++
>> 3 files changed, 18 insertions(+)
>>
>> diff --git a/fs/coredump.c b/fs/coredump.c
>> index b5c86ff..d7730c8 100644
>> --- a/fs/coredump.c
>> +++ b/fs/coredump.c
>> @@ -512,6 +512,7 @@ void do_coredump(const siginfo_t *siginfo)
>> bool need_nonrelative = false;
>> bool core_dumped = false;
>> static atomic_t core_dump_count = ATOMIC_INIT(0);
>> + int sig = siginfo->si_signo;
>> struct coredump_params cprm = {
>> .siginfo = siginfo,
>> .regs = signal_pt_regs(),
>> @@ -526,6 +527,15 @@ void do_coredump(const siginfo_t *siginfo)
>>
>> audit_core_dumps(siginfo->si_signo);
>>
>> + if (sig == SIGSEGV || sig == SIGBUS || sig == SIGKILL || sig == SIGILL) {
>
> I think we should add SIGSYS to this list.
>
>> + rcu_read_lock();
>> + read_lock(&tasklist_lock);
>> + if (current->real_parent && (current->flags & PF_FORKNOEXEC))
>> + current->real_parent->brute_expires = get_seconds() + (30 * 60);
>> + read_unlock(&tasklist_lock);
>> + rcu_read_unlock();
>> + }
>> +
>> binfmt = mm->binfmt;
>> if (!binfmt || !binfmt->core_dump)
>> goto fail;
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 8db31ef..c616735 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -1701,6 +1701,7 @@ struct task_struct {
>> #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
>> unsigned long task_state_change;
>> #endif
>> + unsigned long brute_expires;
>> };
>>
>> /* Future-safe accessor for struct task_struct's cpus_allowed. */
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> index 4dc2dda..178c80e 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -74,6 +74,7 @@
>> #include <linux/uprobes.h>
>> #include <linux/aio.h>
>> #include <linux/compiler.h>
>> +#include <linux/delay.h>
>>
>> #include <asm/pgtable.h>
>> #include <asm/pgalloc.h>
>> @@ -352,6 +353,8 @@ static struct task_struct *dup_task_struct(struct task_struct *orig)
>> tsk->splice_pipe = NULL;
>> tsk->task_frag.page = NULL;
>>
>> + tsk->brute_expires = 0;
>> +
>> account_kernel_stack(ti, 1);
>>
>> return tsk;
>> @@ -1669,6 +1672,10 @@ long do_fork(unsigned long clone_flags,
>> if (clone_flags & CLONE_PARENT_SETTID)
>> put_user(nr, parent_tidptr);
>>
>> + if (unlikely(current->brute_expires) && time_before(get_seconds(),
>> + current->brute_expires))
>> + msleep(30 * 1000);
>> +
>> if (clone_flags & CLONE_VFORK) {
>> p->vfork_done = &vfork;
>> init_completion(&vfork);
>> --
>> 2.1.0
>>
>
> Instead of open-coding the checks here, maybe it'd make sense to
> extract the "attach" and "check" logic into explicit functions that
> can be CONFIG stubbed out? This is how grsec handles it via their
> gr_handle_brute_* functions.
>
> Regardless, I'm for adding this feature to mainline. :)
>
If this is going to go into mainline, I think it needs better
configurability (the timeouts, etc, should be settable by sysctl and
possibly prctl), and I expect we'll need a prctl to turn it off. I
can imagine programs (e.g. anything Zygote-like) that will want to
turn it off because it'll do more harm than good.
But I like the concept, too.
--Andy
> -Kees
>
> --
> Kees Cook
> Chrome OS Security
--
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
