Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1366598 > unrolled thread

[PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies Scott Bauer <sbauer@eng.utah.edu> - 2016-03-29 22:00 +0200
    [PATCH v4 1/4] SROP Mitigation: Architecture independent code for signal cookies Scott Bauer <sbauer@eng.utah.edu> - 2016-03-29 22:00 +0200
      Re: [PATCH v4 1/4] SROP Mitigation: Architecture independent code for  signal cookies Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-30 01:10 +0200
      Re: [PATCH v4 1/4] SROP Mitigation: Architecture independent code for signal cookies ebiederm@xmission.com (Eric W. Biederman) - 2016-03-31 22:40 +0200
        Re: [PATCH v4 1/4] SROP Mitigation: Architecture independent code for  signal cookies Linus Torvalds <torvalds@linux-foundation.org> - 2016-04-01 00:10 +0200
          Re: [PATCH v4 1/4] SROP Mitigation: Architecture independent code for signal cookies ebiederm@xmission.com (Eric W. Biederman) - 2016-04-01 00:30 +0200
    Re: [PATCH v4 3/4] Sysctl: SROP Mitigation: Add Sysctl argument to  disable SROP. Andi Kleen <ak@linux.intel.com> - 2016-03-29 22:00 +0200
      Re: [PATCH v4 3/4] Sysctl: SROP Mitigation: Add Sysctl argument to  disable SROP. Scotty Bauer <sbauer@eng.utah.edu> - 2016-03-29 22:50 +0200
        Re: [PATCH v4 3/4] Sysctl: SROP Mitigation: Add Sysctl argument to  disable SROP. Andi Kleen <ak@linux.intel.com> - 2016-03-29 23:00 +0200
    [PATCH v4 3/4] Sysctl: SROP Mitigation: Add Sysctl argument to disable SROP. Scott Bauer <sbauer@eng.utah.edu> - 2016-03-29 22:00 +0200
    [PATCH v4 2/4] x86: SROP Mitigation: Implement Signal Cookies Scott Bauer <sbauer@eng.utah.edu> - 2016-03-29 22:00 +0200
    Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies Andy Lutomirski <luto@amacapital.net> - 2016-03-29 23:30 +0200
      Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies Andy Lutomirski <luto@amacapital.net> - 2016-03-29 23:40 +0200
        Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-30 00:40 +0200
          Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies Scotty Bauer <sbauer@eng.utah.edu> - 2016-03-30 01:20 +0200
            Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies ebiederm@xmission.com (Eric W. Biederman) - 2016-03-31 22:40 +0200
              Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies Cyrill Gorcunov <gorcunov@gmail.com> - 2016-04-01 15:00 +0200
        Re: [kernel-hardening] Re: [PATCH v4 0/4] SROP Mitigation:  Sigreturn Cookies Daniel Micay <danielmicay@gmail.com> - 2016-03-30 01:00 +0200
      Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies Scotty Bauer <sbauer@eng.utah.edu> - 2016-03-29 23:40 +0200
    Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-30 01:00 +0200
      Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-30 01:00 +0200
      Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies Andy Lutomirski <luto@amacapital.net> - 2016-03-30 01:10 +0200
      Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies Scotty Bauer <sbauer@eng.utah.edu> - 2016-03-30 01:20 +0200
        Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-30 01:30 +0200
          Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies Scotty Bauer <sbauer@eng.utah.edu> - 2016-03-30 01:40 +0200

Page 1 of 2  [1] 2  Next page →

#1366598 — [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies

Sigreturn-oriented programming is a new attack vector in userland
where an attacker crafts a fake signal frame on the stack and calls
sigreturn. The kernel will extract the fake signal frame, which
contains attacker controlled "saved" registers. The kernel will then
transfer control to the attacker controlled userland instruction pointer.

To prevent SROP attacks the kernel needs to know or be able to dervive
whether a sigreturn it is processing is in response to a legitimate
signal the kernel previously delivered. 

Further information and test code can be found in Documentation/security
and this excellent article:
http://lwn.net/Articles/676803/

These patches implement the necessary changes to generate a cookie
which will be placed above signal frame upon signal delivery to userland.
The cookie is generated using a per-process random value xor'd with
the address where the cookie will be stored on the stack.

Upon a sigreturn the kernel will extract the cookie from userland,
recalculate what the original cookie should be and verify that the two
do not differ. If the two differ the kernel will terminate the process
with a SIGSEGV.

This prevents SROP by adding a value that the attacker cannot guess,
but the kernel can verify. Therefore an attacker cannot use sigreturn as
a method to control the flow of a process.


Version	changes:

v3->v4
	Removed	ambiguous __user annotation, added Documentation
	and test code.

v2->v3
	Changed	cookie calculation from	using restored regs->sp	to
	using frame pointer from before	restoration.

v1->v2
	Miscellaneous nits and code cleanup.

Scott Bauer (4):
      SROP Mitigation: Architecture independent code for signal cookies
      x86: SROP Mitigation: Implement Signal Cookies
      Sysctl: SROP Mitigation: Add Sysctl argument to disable SROP.
      Documentation: SROP Mitigation: Add documentation for SROP cookies


 Documentation/security/srop-cookies.txt | 203 ++++++++++++++++++++++++++++++++
 arch/x86/ia32/ia32_signal.c             |  65 +++++++++-
 arch/x86/include/asm/fpu/signal.h       |   2 +
 arch/x86/kernel/fpu/signal.c            |  10 ++
 arch/x86/kernel/signal.c                |  83 +++++++++++--
 fs/exec.c                               |   3 +
 include/linux/sched.h                   |   7 ++
 include/linux/signal.h                  |   3 +
 kernel/signal.c                         |  49 ++++++++
 kernel/sysctl.c                         |   8 ++
 10 files changed, 418 insertions(+), 15 deletions(-)

[toc] | [next] | [standalone]

#1366600 — [PATCH v4 1/4] SROP Mitigation: Architecture independent code for signal cookies

This patch adds a per-process secret to the task struct which
will be used during signal delivery and during a sigreturn.
Also, logic is added in signal.c to generate, place, extract,
clear and verify the signal cookie.

Cc: Abhiram Balasubramanian <abhiram@cs.utah.edu>
Signed-off-by: Scott Bauer <sbauer@plzdonthack.me>
Signed-off-by: Scott Bauer <sbauer@eng.utah.edu>
---
 fs/exec.c              |  3 +++
 include/linux/sched.h  |  7 +++++++
 include/linux/signal.h |  2 ++
 kernel/signal.c        | 40 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+)

diff --git a/fs/exec.c b/fs/exec.c
index c4010b8..5d55c01 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -57,6 +57,7 @@
 #include <linux/oom.h>
 #include <linux/compat.h>
 #include <linux/vmalloc.h>
+#include <linux/random.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
@@ -1231,6 +1232,8 @@ void setup_new_exec(struct linux_binprm * bprm)
 	/* This is the point of no return */
 	current->sas_ss_sp = current->sas_ss_size = 0;
 
+	get_random_bytes(&current->sig_cookie, sizeof(current->sig_cookie));
+
 	if (uid_eq(current_euid(), current_uid()) && gid_eq(current_egid(), current_gid()))
 		set_dumpable(current->mm, SUID_DUMP_USER);
 	else
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 60bba7e..1828fb8 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1502,6 +1502,13 @@ struct task_struct {
 	unsigned long stack_canary;
 #endif
 	/*
+	 * Canary value for signal frames placed on user stack.
+	 * This helps mitigate "Signal Return oriented program"
+	 * exploits in userland.
+	 */
+	unsigned long sig_cookie;
+
+	/*
 	 * pointers to (original) parent process, youngest child, younger sibling,
 	 * older sibling, respectively.  (p->father can be replaced with
 	 * p->real_parent->pid)
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 92557bb..fae0618 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -280,6 +280,8 @@ extern int get_signal(struct ksignal *ksig);
 extern void signal_setup_done(int failed, struct ksignal *ksig, int stepping);
 extern void exit_signals(struct task_struct *tsk);
 extern void kernel_sigaction(int, __sighandler_t);
+extern int set_sigcookie(unsigned long __user *location);
+extern int verify_clear_sigcookie(unsigned long __user *sig_cookie_ptr);
 
 static inline void allow_signal(int sig)
 {
diff --git a/kernel/signal.c b/kernel/signal.c
index aa9bf00..1e4f65c 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2430,6 +2430,46 @@ out:
 	}
 }
 
+static unsigned long gen_sigcookie(unsigned long __user *location)
+{
+
+	unsigned long sig_cookie;
+	sig_cookie = (unsigned long) location ^ current->sig_cookie;
+
+	return sig_cookie;
+}
+
+int set_sigcookie(unsigned long __user *location)
+{
+
+	unsigned long sig_cookie = gen_sigcookie(location);
+
+	return put_user(sig_cookie, location);
+}
+
+int verify_clear_sigcookie(unsigned long __user *sig_cookie_ptr)
+{
+	unsigned long user_cookie;
+	unsigned long calculated_cookie;
+
+	if (get_user(user_cookie, sig_cookie_ptr))
+		return 1;
+
+	calculated_cookie = gen_sigcookie(sig_cookie_ptr);
+
+	if (user_cookie != calculated_cookie) {
+		pr_warn("Signal protector does not match what kernel set it to"\
+			". Possible exploit attempt or buggy program!\n");
+		return 1;
+
+	}
+
+	user_cookie = 0;
+	return put_user(user_cookie, sig_cookie_ptr);
+}
+
+EXPORT_SYMBOL(verify_clear_sigcookie);
+EXPORT_SYMBOL(set_sigcookie);
 EXPORT_SYMBOL(recalc_sigpending);
 EXPORT_SYMBOL_GPL(dequeue_signal);
 EXPORT_SYMBOL(flush_signals);
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1366771 — Re: [PATCH v4 1/4] SROP Mitigation: Architecture independent code for signal cookies

On Tue, Mar 29, 2016 at 2:53 PM, Scott Bauer <sbauer@eng.utah.edu> wrote:
> @@ -1231,6 +1232,8 @@ void setup_new_exec(struct linux_binprm * bprm)
>         /* This is the point of no return */
>         current->sas_ss_sp = current->sas_ss_size = 0;
>
> +       get_random_bytes(&current->sig_cookie, sizeof(current->sig_cookie));
> +

This should probably just be

     current->sig_cookie = get_random_long();

instead. That will use hardware random numbers if available, and be
*much* faster.

I realize that some people don't like the hardware random number
generators because they don't trust them, but quite frankly, for
something like this it's fine. If the attacker is in collusion with
the hardware manufacturer, you have way bigger problems than a SROP
attack.

                 Linus

[toc] | [prev] | [next] | [standalone]

#1368660 — Re: [PATCH v4 1/4] SROP Mitigation: Architecture independent code for signal cookies

Scott Bauer <sbauer@eng.utah.edu> writes:

> This patch adds a per-process secret to the task struct which
> will be used during signal delivery and during a sigreturn.
> Also, logic is added in signal.c to generate, place, extract,
> clear and verify the signal cookie.
>
> Cc: Abhiram Balasubramanian <abhiram@cs.utah.edu>
> Signed-off-by: Scott Bauer <sbauer@plzdonthack.me>
> Signed-off-by: Scott Bauer <sbauer@eng.utah.edu>

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 60bba7e..1828fb8 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1502,6 +1502,13 @@ struct task_struct {
>  	unsigned long stack_canary;
>  #endif
>  	/*
> +	 * Canary value for signal frames placed on user stack.
> +	 * This helps mitigate "Signal Return oriented program"
> +	 * exploits in userland.
> +	 */
> +	unsigned long sig_cookie;


I suspect we want this to be a per-mm attribute rather than a per-thread
attribute.

Otherwise you are breaking anything that uses a N-M threading model.
Which I suspect means that this implementation choice breaks all go
programs on linux.

> +	/*
>  	 * pointers to (original) parent process, youngest child, younger sibling,
>  	 * older sibling, respectively.  (p->father can be replaced with
>  	 * p->real_parent->pid)

Eric

[toc] | [prev] | [next] | [standalone]

#1368852 — Re: [PATCH v4 1/4] SROP Mitigation: Architecture independent code for signal cookies

On Thu, Mar 31, 2016 at 3:25 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> I suspect we want this to be a per-mm attribute rather than a per-thread
> attribute.
>
> Otherwise you are breaking anything that uses a N-M threading model.
> Which I suspect means that this implementation choice breaks all go
> programs on linux.

That sounds like a good point, but wouldn't it make more conceptual
sense to make it part of "struct sighand_struct" instead?

That is also shared for threads.

                Linus

[toc] | [prev] | [next] | [standalone]

#1368862 — Re: [PATCH v4 1/4] SROP Mitigation: Architecture independent code for signal cookies

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, Mar 31, 2016 at 3:25 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>>
>> I suspect we want this to be a per-mm attribute rather than a per-thread
>> attribute.
>>
>> Otherwise you are breaking anything that uses a N-M threading model.
>> Which I suspect means that this implementation choice breaks all go
>> programs on linux.
>
> That sounds like a good point, but wouldn't it make more conceptual
> sense to make it part of "struct sighand_struct" instead?
>
> That is also shared for threads.

Good point.  Given this is a signal handling feature
struct sighand_struct is the natural place to put this.

Eric

[toc] | [prev] | [next] | [standalone]

#1366602 — Re: [PATCH v4 3/4] Sysctl: SROP Mitigation: Add Sysctl argument to disable SROP.

On Tue, Mar 29, 2016 at 01:53:26PM -0600, Scott Bauer wrote:
> This patch adds a sysctl argument to disable SROP protection.

Sysctl needs to be documented in Documentation/sysctl/

Also negated sysctl is weird, normally they are positive (enable-xxx)

-Andi

[toc] | [prev] | [next] | [standalone]

#1366691 — Re: [PATCH v4 3/4] Sysctl: SROP Mitigation: Add Sysctl argument to disable SROP.

On 03/29/2016 01:59 PM, Andi Kleen wrote:
> On Tue, Mar 29, 2016 at 01:53:26PM -0600, Scott Bauer wrote:
>> This patch adds a sysctl argument to disable SROP protection.
> 
> Sysctl needs to be documented in Documentation/sysctl/
> 
> Also negated sysctl is weird, normally they are positive (enable-xxx)
> 

Sure, I can change it. This may be a dumb question: I want SROP to be enabled by default, and thus the new
enable-xxx will be initialized to 1, that's fine, right?

[toc] | [prev] | [next] | [standalone]

#1366709 — Re: [PATCH v4 3/4] Sysctl: SROP Mitigation: Add Sysctl argument to disable SROP.

On Tue, Mar 29, 2016 at 02:46:03PM -0600, Scotty Bauer wrote:
> 
> 
> 
> On 03/29/2016 01:59 PM, Andi Kleen wrote:
> > On Tue, Mar 29, 2016 at 01:53:26PM -0600, Scott Bauer wrote:
> >> This patch adds a sysctl argument to disable SROP protection.
> > 
> > Sysctl needs to be documented in Documentation/sysctl/
> > 
> > Also negated sysctl is weird, normally they are positive (enable-xxx)
> > 
> 
> Sure, I can change it. This may be a dumb question: I want SROP to be enabled by default, and thus the new
> enable-xxx will be initialized to 1, that's fine, right?

Yes that's fine.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

[toc] | [prev] | [next] | [standalone]

#1366604 — [PATCH v4 3/4] Sysctl: SROP Mitigation: Add Sysctl argument to disable SROP.

This patch adds a sysctl argument to disable SROP protection.

Cc: Abhiram Balasubramanian <abhiram@cs.utah.edu>
Signed-off-by: Scott Bauer <sbauer@plzdonthack.me>
Signed-off-by: Scott Bauer <sbauer@eng.utah.edu>
---
 include/linux/signal.h |  1 +
 kernel/signal.c        | 13 +++++++++++--
 kernel/sysctl.c        |  8 ++++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/include/linux/signal.h b/include/linux/signal.h
index fae0618..cd7f152 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -9,6 +9,7 @@ struct task_struct;
 
 /* for sysctl */
 extern int print_fatal_signals;
+extern int srop_disabled;
 /*
  * Real Time signals may be queued.
  */
diff --git a/kernel/signal.c b/kernel/signal.c
index 1e4f65c..fbe61d6 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -52,6 +52,7 @@
 static struct kmem_cache *sigqueue_cachep;
 
 int print_fatal_signals __read_mostly;
+int srop_disabled __read_mostly;
 
 static void __user *sig_handler(struct task_struct *t, int sig)
 {
@@ -2452,18 +2453,26 @@ int verify_clear_sigcookie(unsigned long __user *sig_cookie_ptr)
 	unsigned long user_cookie;
 	unsigned long calculated_cookie;
 
+	if (srop_disabled)
+		goto out;
+
 	if (get_user(user_cookie, sig_cookie_ptr))
 		return 1;
 
 	calculated_cookie = gen_sigcookie(sig_cookie_ptr);
 
 	if (user_cookie != calculated_cookie) {
-		pr_warn("Signal protector does not match what kernel set it to"\
-			". Possible exploit attempt or buggy program!\n");
+		pr_warn("kernel/signal.c: Signal protector does not match what"\
+			" kernel set it to.\n"				\
+			"Possible exploit attempt or buggy program!\nIf you"\
+			" believe this is an error you can disable SROP "\
+			" Protection by #echo 1 > /proc/sys/kernel/"\
+			"disable-srop-protection\n");
 		return 1;
 
 	}
 
+out:
 	user_cookie = 0;
 	return put_user(user_cookie, sig_cookie_ptr);
 }
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 725587f..7886634 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -536,6 +536,14 @@ static struct ctl_table kern_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec,
 	},
+	{
+		.procname	= "disable-srop-protection",
+		.data		= &srop_disabled,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec,
+
+	},
 #ifdef CONFIG_SPARC
 	{
 		.procname	= "reboot-cmd",
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1366605 — [PATCH v4 2/4] x86: SROP Mitigation: Implement Signal Cookies

This patch adds SROP mitigation logic to the x86 signal delivery
and sigreturn code. The cookie is placed in the unused alignment
space above the saved FP state, if it exists. If there is no FP
state to save then the cookie is placed in the alignment space above
the sigframe.

Cc: Abhiram Balasubramanian <abhiram@cs.utah.edu>
Signed-off-by: Scott Bauer <sbauer@plzdonthack.me>
Signed-off-by: Scott Bauer <sbauer@eng.utah.edu>
---
 arch/x86/ia32/ia32_signal.c       | 65 +++++++++++++++++++++++++++---
 arch/x86/include/asm/fpu/signal.h |  2 +
 arch/x86/kernel/fpu/signal.c      | 10 +++++
 arch/x86/kernel/signal.c          | 83 ++++++++++++++++++++++++++++++++++-----
 4 files changed, 145 insertions(+), 15 deletions(-)

diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 0552884..85ec799 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -68,7 +68,8 @@
 }
 
 static int ia32_restore_sigcontext(struct pt_regs *regs,
-				   struct sigcontext_32 __user *sc)
+				   struct sigcontext_32 __user *sc,
+				   void **user_cookie_ptr)
 {
 	unsigned int tmpflags, err = 0;
 	void __user *buf;
@@ -105,6 +106,16 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 		buf = compat_ptr(tmp);
 	} get_user_catch(err);
 
+	/*
+	 * If there is fp state get cookie from the top of the fp state,
+	 * else get it from the top of the sig frame.
+	 */
+
+	if (tmp != 0)
+		*user_cookie_ptr = compat_ptr(tmp + fpu__getsize(1));
+	else
+		*user_cookie_ptr = NULL;
+
 	err |= fpu__restore_sig(buf, 1);
 
 	force_iret();
@@ -117,6 +128,7 @@ asmlinkage long sys32_sigreturn(void)
 	struct pt_regs *regs = current_pt_regs();
 	struct sigframe_ia32 __user *frame = (struct sigframe_ia32 __user *)(regs->sp-8);
 	sigset_t set;
+	void __user *user_cookie;
 
 	if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
 		goto badframe;
@@ -129,8 +141,16 @@ asmlinkage long sys32_sigreturn(void)
 
 	set_current_blocked(&set);
 
-	if (ia32_restore_sigcontext(regs, &frame->sc))
+	if (ia32_restore_sigcontext(regs, &frame->sc, &user_cookie))
 		goto badframe;
+
+	if (user_cookie == NULL)
+		user_cookie = compat_ptr(((unsigned long) frame) +
+					 sizeof(frame));
+
+	if (verify_clear_sigcookie(user_cookie))
+ 		goto badframe;
+
 	return regs->ax;
 
 badframe:
@@ -143,6 +163,7 @@ asmlinkage long sys32_rt_sigreturn(void)
 	struct pt_regs *regs = current_pt_regs();
 	struct rt_sigframe_ia32 __user *frame;
 	sigset_t set;
+	void __user *user_cookie;
 
 	frame = (struct rt_sigframe_ia32 __user *)(regs->sp - 4);
 
@@ -153,9 +174,17 @@ asmlinkage long sys32_rt_sigreturn(void)
 
 	set_current_blocked(&set);
 
-	if (ia32_restore_sigcontext(regs, &frame->uc.uc_mcontext))
+	if (ia32_restore_sigcontext(regs, &frame->uc.uc_mcontext, &user_cookie))
 		goto badframe;
 
+	if (user_cookie == NULL)
+		user_cookie = compat_ptr(((unsigned long) frame) +
+					 sizeof(*frame));
+
+	if (verify_clear_sigcookie(user_cookie))
+ 		goto badframe;
+
+
 	if (compat_restore_altstack(&frame->uc.uc_stack))
 		goto badframe;
 
@@ -213,7 +242,8 @@ static int ia32_setup_sigcontext(struct sigcontext_32 __user *sc,
  */
 static void __user *get_sigframe(struct ksignal *ksig, struct pt_regs *regs,
 				 size_t frame_size,
-				 void __user **fpstate)
+				 void __user **fpstate,
+				 void __user **cookie_ptr)
 {
 	struct fpu *fpu = &current->thread.fpu;
 	unsigned long sp;
@@ -230,11 +260,20 @@ static void __user *get_sigframe(struct ksignal *ksig, struct pt_regs *regs,
 		 ksig->ka.sa.sa_restorer)
 		sp = (unsigned long) ksig->ka.sa.sa_restorer;
 
+	/*
+	 * Allocate space for cookie above FP/Frame. It will sit in
+	 * the padding of the saved FP state, or if there is no FP
+	 * state it will sit in the padding of the sig frame.
+	 */
+	sp -= sizeof(unsigned long);
+
 	if (fpu->fpstate_active) {
 		unsigned long fx_aligned, math_size;
 
 		sp = fpu__alloc_mathframe(sp, 1, &fx_aligned, &math_size);
 		*fpstate = (struct _fpstate_32 __user *) sp;
+		*cookie_ptr = (void __user *) sp + math_size;
+
 		if (copy_fpstate_to_sigframe(*fpstate, (void __user *)fx_aligned,
 				    math_size) < 0)
 			return (void __user *) -1L;
@@ -244,6 +283,10 @@ static void __user *get_sigframe(struct ksignal *ksig, struct pt_regs *regs,
 	/* Align the stack pointer according to the i386 ABI,
 	 * i.e. so that on function entry ((sp + 4) & 15) == 0. */
 	sp = ((sp + 4) & -16ul) - 4;
+
+	if (!fpu->fpstate_active)
+		*cookie_ptr = (void __user *) (sp + frame_size);
+
 	return (void __user *) sp;
 }
 
@@ -254,6 +297,7 @@ int ia32_setup_frame(int sig, struct ksignal *ksig,
 	void __user *restorer;
 	int err = 0;
 	void __user *fpstate = NULL;
+	void __user *cookie_location;
 
 	/* copy_to_user optimizes that into a single 8 byte store */
 	static const struct {
@@ -266,7 +310,8 @@ int ia32_setup_frame(int sig, struct ksignal *ksig,
 		0x80cd,		/* int $0x80 */
 	};
 
-	frame = get_sigframe(ksig, regs, sizeof(*frame), &fpstate);
+	frame = get_sigframe(ksig, regs, sizeof(*frame),
+			     &fpstate, &cookie_location);
 
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
 		return -EFAULT;
@@ -274,6 +319,9 @@ int ia32_setup_frame(int sig, struct ksignal *ksig,
 	if (__put_user(sig, &frame->sig))
 		return -EFAULT;
 
+	if (set_sigcookie(cookie_location))
+		return -EFAULT;
+
 	if (ia32_setup_sigcontext(&frame->sc, fpstate, regs, set->sig[0]))
 		return -EFAULT;
 
@@ -332,6 +380,7 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
 	void __user *restorer;
 	int err = 0;
 	void __user *fpstate = NULL;
+	void __user *cookie_location;
 
 	/* __copy_to_user optimizes that into a single 8 byte store */
 	static const struct {
@@ -346,11 +395,15 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
 		0,
 	};
 
-	frame = get_sigframe(ksig, regs, sizeof(*frame), &fpstate);
+	frame = get_sigframe(ksig, regs, sizeof(*frame),
+			     &fpstate, &cookie_location);
 
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
 		return -EFAULT;
 
+	if (set_sigcookie(cookie_location))
+		return -EFAULT;
+
 	put_user_try {
 		put_user_ex(sig, &frame->sig);
 		put_user_ex(ptr_to_compat(&frame->info), &frame->pinfo);
diff --git a/arch/x86/include/asm/fpu/signal.h b/arch/x86/include/asm/fpu/signal.h
index 0e970d0..0c279cd 100644
--- a/arch/x86/include/asm/fpu/signal.h
+++ b/arch/x86/include/asm/fpu/signal.h
@@ -28,6 +28,8 @@ unsigned long
 fpu__alloc_mathframe(unsigned long sp, int ia32_frame,
 		     unsigned long *buf_fx, unsigned long *size);
 
+unsigned long fpu__getsize(int ia32_frame);
+
 extern void fpu__init_prepare_fx_sw_frame(void);
 
 #endif /* _ASM_X86_FPU_SIGNAL_H */
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 31c6a60..50535d7 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -344,6 +344,16 @@ static inline int xstate_sigframe_size(void)
 	return use_xsave() ? xstate_size + FP_XSTATE_MAGIC2_SIZE : xstate_size;
 }
 
+unsigned long fpu__getsize(int ia32_frame)
+{
+	unsigned long frame_size = xstate_sigframe_size();
+
+	if (ia32_frame && use_fxsr())
+		frame_size += sizeof(struct fregs_state);
+
+	return frame_size;
+}
+
 /*
  * Restore FPU state from a sigframe:
  */
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 548ddf7..87aa6f3 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -23,6 +23,7 @@
 #include <linux/user-return-notifier.h>
 #include <linux/uprobes.h>
 #include <linux/context_tracking.h>
+#include <linux/hash.h>
 
 #include <asm/processor.h>
 #include <asm/ucontext.h>
@@ -92,7 +93,8 @@ static void force_valid_ss(struct pt_regs *regs)
 
 static int restore_sigcontext(struct pt_regs *regs,
 			      struct sigcontext __user *sc,
-			      unsigned long uc_flags)
+			      unsigned long uc_flags,
+			      void **user_cookie_ptr)
 {
 	unsigned long buf_val;
 	void __user *buf;
@@ -146,6 +148,12 @@ static int restore_sigcontext(struct pt_regs *regs,
 		buf = (void __user *)buf_val;
 	} get_user_catch(err);
 
+	if (buf_val != 0)
+		*user_cookie_ptr = (void __user *)
+			(buf_val + fpu__getsize(config_enabled(CONFIG_X86_32)));
+	else
+		*user_cookie_ptr = NULL;
+
 	err |= fpu__restore_sig(buf, config_enabled(CONFIG_X86_32));
 
 	force_iret();
@@ -235,7 +243,7 @@ static unsigned long align_sigframe(unsigned long sp)
 
 static void __user *
 get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
-	     void __user **fpstate)
+	     void __user **fpstate, void __user **cookie_ptr)
 {
 	/* Default to using normal stack */
 	unsigned long math_size = 0;
@@ -262,14 +270,25 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
 		}
 	}
 
+	/*
+	 * Allocate space for cookie above FP/Frame. It will sit in
+	 * the padding of the saved FP state, or if there is no FP
+	 * state it will sit in the padding of the sig frame.
+	 */
+	sp -= sizeof(unsigned long);
+
 	if (fpu->fpstate_active) {
 		sp = fpu__alloc_mathframe(sp, config_enabled(CONFIG_X86_32),
 					  &buf_fx, &math_size);
 		*fpstate = (void __user *)sp;
+		*cookie_ptr = (void __user *)sp + math_size;
 	}
 
 	sp = align_sigframe(sp - frame_size);
 
+	if (!fpu->fpstate_active)
+		*cookie_ptr = (void __user *) (sp + frame_size);
+
 	/*
 	 * If we are on the alternate signal stack and would overflow it, don't.
 	 * Return an always-bogus address instead so we will die with SIGSEGV.
@@ -316,8 +335,10 @@ __setup_frame(int sig, struct ksignal *ksig, sigset_t *set,
 	void __user *restorer;
 	int err = 0;
 	void __user *fpstate = NULL;
+	void __user *cookie_location;
 
-	frame = get_sigframe(&ksig->ka, regs, sizeof(*frame), &fpstate);
+	frame = get_sigframe(&ksig->ka, regs, sizeof(*frame),
+			     &fpstate, &cookie_location);
 
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
 		return -EFAULT;
@@ -325,6 +346,9 @@ __setup_frame(int sig, struct ksignal *ksig, sigset_t *set,
 	if (__put_user(sig, &frame->sig))
 		return -EFAULT;
 
+	if (set_sigcookie(cookie_location))
+		return -EFAULT;
+
 	if (setup_sigcontext(&frame->sc, fpstate, regs, set->sig[0]))
 		return -EFAULT;
 
@@ -379,12 +403,17 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
 	void __user *restorer;
 	int err = 0;
 	void __user *fpstate = NULL;
+	void __user *cookie_location;
 
-	frame = get_sigframe(&ksig->ka, regs, sizeof(*frame), &fpstate);
+	frame = get_sigframe(&ksig->ka, regs, sizeof(*frame),
+			     &fpstate, &cookie_location);
 
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
 		return -EFAULT;
 
+	if (set_sigcookie(cookie_location))
+		return -EFAULT;
+
 	put_user_try {
 		put_user_ex(sig, &frame->sig);
 		put_user_ex(&frame->info, &frame->pinfo);
@@ -458,9 +487,11 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
 {
 	struct rt_sigframe __user *frame;
 	void __user *fp = NULL;
+	void __user *cookie_location;
 	int err = 0;
 
-	frame = get_sigframe(&ksig->ka, regs, sizeof(struct rt_sigframe), &fp);
+	frame = get_sigframe(&ksig->ka, regs, sizeof(struct rt_sigframe),
+			     &fp, &cookie_location);
 
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
 		return -EFAULT;
@@ -470,6 +501,9 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
 			return -EFAULT;
 	}
 
+	if (set_sigcookie(cookie_location))
+		return -EFAULT;
+
 	put_user_try {
 		/* Create the ucontext.  */
 		put_user_ex(frame_uc_flags(regs), &frame->uc.uc_flags);
@@ -541,8 +575,10 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
 	void __user *restorer;
 	int err = 0;
 	void __user *fpstate = NULL;
+	void __user *cookie_location;
 
-	frame = get_sigframe(&ksig->ka, regs, sizeof(*frame), &fpstate);
+	frame = get_sigframe(&ksig->ka, regs, sizeof(*frame),
+			     &fpstate, &cookie_location);
 
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
 		return -EFAULT;
@@ -552,6 +588,9 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
 			return -EFAULT;
 	}
 
+	if (set_sigcookie(cookie_location))
+		return -EFAULT;
+
 	put_user_try {
 		/* Create the ucontext.  */
 		put_user_ex(frame_uc_flags(regs), &frame->uc.uc_flags);
@@ -603,6 +642,7 @@ asmlinkage unsigned long sys_sigreturn(void)
 {
 	struct pt_regs *regs = current_pt_regs();
 	struct sigframe __user *frame;
+	void __user *user_cookie;
 	sigset_t set;
 
 	frame = (struct sigframe __user *)(regs->sp - 8);
@@ -620,8 +660,16 @@ asmlinkage unsigned long sys_sigreturn(void)
 	 * x86_32 has no uc_flags bits relevant to restore_sigcontext.
 	 * Save a few cycles by skipping the __get_user.
 	 */
-	if (restore_sigcontext(regs, &frame->sc, 0))
+	if (restore_sigcontext(regs, &frame->sc, 0, &user_cookie))
+		goto badframe;
+
+	if (user_cookie == NULL)
+		user_cookie = (void __user *)
+			((unsigned long) frame) + sizeof(*frame);
+
+	if (verify_clear_sigcookie(user_cookie))
 		goto badframe;
+
 	return regs->ax;
 
 badframe:
@@ -635,6 +683,7 @@ asmlinkage long sys_rt_sigreturn(void)
 {
 	struct pt_regs *regs = current_pt_regs();
 	struct rt_sigframe __user *frame;
+	void __user *user_cookie;
 	sigset_t set;
 	unsigned long uc_flags;
 
@@ -648,7 +697,14 @@ asmlinkage long sys_rt_sigreturn(void)
 
 	set_current_blocked(&set);
 
-	if (restore_sigcontext(regs, &frame->uc.uc_mcontext, uc_flags))
+	if (restore_sigcontext(regs, &frame->uc.uc_mcontext, uc_flags, &user_cookie))
+		goto badframe;
+
+	if (user_cookie == NULL)
+		user_cookie = (void __user *)
+			((unsigned long) frame) + sizeof(*frame);
+
+	if (verify_clear_sigcookie(user_cookie))
 		goto badframe;
 
 	if (restore_altstack(&frame->uc.uc_stack))
@@ -834,6 +890,7 @@ asmlinkage long sys32_x32_rt_sigreturn(void)
 {
 	struct pt_regs *regs = current_pt_regs();
 	struct rt_sigframe_x32 __user *frame;
+	void __user *user_cookie;
 	sigset_t set;
 	unsigned long uc_flags;
 
@@ -848,7 +905,15 @@ asmlinkage long sys32_x32_rt_sigreturn(void)
 
 	set_current_blocked(&set);
 
-	if (restore_sigcontext(regs, &frame->uc.uc_mcontext, uc_flags))
+	if (restore_sigcontext(regs, &frame->uc.uc_mcontext,
+			       uc_flags, &user_cookie))
+		goto badframe;
+
+	if (user_cookie == NULL)
+		user_cookie = (void __user *)
+			((unsigned long) frame) + sizeof(*frame);
+
+	if (verify_clear_sigcookie(user_cookie))
 		goto badframe;
 
 	if (compat_restore_altstack(&frame->uc.uc_stack))
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1366717

On Tue, Mar 29, 2016 at 12:53 PM, Scott Bauer <sbauer@eng.utah.edu> wrote:
> Sigreturn-oriented programming is a new attack vector in userland
> where an attacker crafts a fake signal frame on the stack and calls
> sigreturn. The kernel will extract the fake signal frame, which
> contains attacker controlled "saved" registers. The kernel will then
> transfer control to the attacker controlled userland instruction pointer.
>
> To prevent SROP attacks the kernel needs to know or be able to dervive
> whether a sigreturn it is processing is in response to a legitimate
> signal the kernel previously delivered.
>
> Further information and test code can be found in Documentation/security
> and this excellent article:
> http://lwn.net/Articles/676803/
>
> These patches implement the necessary changes to generate a cookie
> which will be placed above signal frame upon signal delivery to userland.
> The cookie is generated using a per-process random value xor'd with
> the address where the cookie will be stored on the stack.
>
> Upon a sigreturn the kernel will extract the cookie from userland,
> recalculate what the original cookie should be and verify that the two
> do not differ. If the two differ the kernel will terminate the process
> with a SIGSEGV.
>
> This prevents SROP by adding a value that the attacker cannot guess,
> but the kernel can verify. Therefore an attacker cannot use sigreturn as
> a method to control the flow of a process.
>

Has anyone verified that this doesn't break CRIU cross-machine (or
cross-boot) migration and that this doesn't break dosemu?  You're
changing the ABI here.

--Andy

>
> Version changes:
>
> v3->v4
>         Removed ambiguous __user annotation, added Documentation
>         and test code.
>
> v2->v3
>         Changed cookie calculation from using restored regs->sp to
>         using frame pointer from before restoration.
>
> v1->v2
>         Miscellaneous nits and code cleanup.
>
> Scott Bauer (4):
>       SROP Mitigation: Architecture independent code for signal cookies
>       x86: SROP Mitigation: Implement Signal Cookies
>       Sysctl: SROP Mitigation: Add Sysctl argument to disable SROP.
>       Documentation: SROP Mitigation: Add documentation for SROP cookies
>
>
>  Documentation/security/srop-cookies.txt | 203 ++++++++++++++++++++++++++++++++
>  arch/x86/ia32/ia32_signal.c             |  65 +++++++++-
>  arch/x86/include/asm/fpu/signal.h       |   2 +
>  arch/x86/kernel/fpu/signal.c            |  10 ++
>  arch/x86/kernel/signal.c                |  83 +++++++++++--
>  fs/exec.c                               |   3 +
>  include/linux/sched.h                   |   7 ++
>  include/linux/signal.h                  |   3 +
>  kernel/signal.c                         |  49 ++++++++
>  kernel/sysctl.c                         |   8 ++
>  10 files changed, 418 insertions(+), 15 deletions(-)
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

#1366725

On Tue, Mar 29, 2016 at 2:36 PM, Scotty Bauer <sbauer@eng.utah.edu> wrote:
>
>
> On 03/29/2016 03:29 PM, Andy Lutomirski wrote:
>> On Tue, Mar 29, 2016 at 12:53 PM, Scott Bauer <sbauer@eng.utah.edu> wrote:
>>> Sigreturn-oriented programming is a new attack vector in userland
>>> where an attacker crafts a fake signal frame on the stack and calls
>>> sigreturn. The kernel will extract the fake signal frame, which
>>> contains attacker controlled "saved" registers. The kernel will then
>>> transfer control to the attacker controlled userland instruction pointer.
>>>
>>> To prevent SROP attacks the kernel needs to know or be able to dervive
>>> whether a sigreturn it is processing is in response to a legitimate
>>> signal the kernel previously delivered.
>>>
>>> Further information and test code can be found in Documentation/security
>>> and this excellent article:
>>> http://lwn.net/Articles/676803/
>>>
>>> These patches implement the necessary changes to generate a cookie
>>> which will be placed above signal frame upon signal delivery to userland.
>>> The cookie is generated using a per-process random value xor'd with
>>> the address where the cookie will be stored on the stack.
>>>
>>> Upon a sigreturn the kernel will extract the cookie from userland,
>>> recalculate what the original cookie should be and verify that the two
>>> do not differ. If the two differ the kernel will terminate the process
>>> with a SIGSEGV.
>>>
>>> This prevents SROP by adding a value that the attacker cannot guess,
>>> but the kernel can verify. Therefore an attacker cannot use sigreturn as
>>> a method to control the flow of a process.
>>>
>>
>> Has anyone verified that this doesn't break CRIU cross-machine (or
>> cross-boot) migration and that this doesn't break dosemu?  You're
>> changing the ABI here.
>>
>
> I haven't yet I'll do that to verify it breaks -- I'm pretty sure under some
> conditions it will break CRIU. That's why we added the sysctl to turn it off.
> Should I have mentioned this in the main commit that it possibly breaks CRIU/DOSEMU?
> I went ahead and added that to the Documentation.
>
>

Then there's an unanswered question: is this patch acceptable given
that it's an ABI break?  Security fixes are sometimes an exception to
the "no ABI breaks" rule, but it's by no means an automatic exception.

--Andy

[toc] | [prev] | [next] | [standalone]

#1366757

On Tue, Mar 29, 2016 at 4:38 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> Then there's an unanswered question: is this patch acceptable given
> that it's an ABI break?  Security fixes are sometimes an exception to
> the "no ABI breaks" rule, but it's by no means an automatic exception.

So there isn't any "no ABI break" rule - there is only a "does it
break real applications" rule.

(This can also be re-stated as: "Talk is cheap", aka "reality trumps
documentation".

Documentation is meaningless if it doesn't match reality, and what we
actually *do* is what matters.

So the ABI isn't about some theoretical interface documentation, the
ABI is about what people use and have tested.

On the one hand, that means that that our ABI is _stricter_ than any
documentatiuon, and that "but we can make this change that breaks app
XYZ, because XYZ is depending on undocumented behavior" is not an
acceptable excuse.

But on the other hand it *also* means that since the ABI is about real
programs, not theoretical issues, we can also change things as long as
we don't actually break anything that people can notice and depend
on).

And while *acute* security holes will be fixed regardless of ABI
issues, something like this that is only hardening rather than fixing
a particular security hole, really needs to not break any
applications.

Because if it does break anything, it needs to be turned off by
default. That's a hard rule. And since that would be largely defeating
the whole point o fthe series, I think we really need to have made
sure nothing breaks before a patch series like this can be accepted.

That said, if this is done right, I don't think it will break
anything. CRIU may indeed be a special case, but CRIU isn't really a
normal application, and the CRIU people may need to turn this off
explicitly, if it does break.

But yes, dosemu needs to be tested, and needs to just continue
working. But does dosemu actually create a signal stack, as opposed to
just playing with one that has been created for it? I thought it was
just the latter case, which should be ok even with a magic cookie in
there.

                   Linus

[toc] | [prev] | [next] | [standalone]

#1366781

On 03/29/2016 04:34 PM, Linus Torvalds wrote:
> On Tue, Mar 29, 2016 at 4:38 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> Then there's an unanswered question: is this patch acceptable given
>> that it's an ABI break?  Security fixes are sometimes an exception to
>> the "no ABI breaks" rule, but it's by no means an automatic exception.
> 
> So there isn't any "no ABI break" rule - there is only a "does it
> break real applications" rule.
> 
> (This can also be re-stated as: "Talk is cheap", aka "reality trumps
> documentation".
> 
> Documentation is meaningless if it doesn't match reality, and what we
> actually *do* is what matters.
> 
> So the ABI isn't about some theoretical interface documentation, the
> ABI is about what people use and have tested.
> 
> On the one hand, that means that that our ABI is _stricter_ than any
> documentatiuon, and that "but we can make this change that breaks app
> XYZ, because XYZ is depending on undocumented behavior" is not an
> acceptable excuse.
> 
> But on the other hand it *also* means that since the ABI is about real
> programs, not theoretical issues, we can also change things as long as
> we don't actually break anything that people can notice and depend
> on).
> 
> And while *acute* security holes will be fixed regardless of ABI
> issues, something like this that is only hardening rather than fixing
> a particular security hole, really needs to not break any
> applications.
> 
> Because if it does break anything, it needs to be turned off by
> default. That's a hard rule. And since that would be largely defeating
> the whole point o fthe series, I think we really need to have made
> sure nothing breaks before a patch series like this can be accepted.
> 
> That said, if this is done right, I don't think it will break
> anything. CRIU may indeed be a special case, but CRIU isn't really a
> normal application, and the CRIU people may need to turn this off
> explicitly, if it does break.
> 
> But yes, dosemu needs to be tested, and needs to just continue
> working. But does dosemu actually create a signal stack, as opposed to
> just playing with one that has been created for it? I thought it was
> just the latter case, which should be ok even with a magic cookie in
> there.
> 
>                    Linus
> 


For what it's worth this series is breaking CRIU, I just tested:

root@node0:/mnt/criu# criu restore -vvvv -o restore.log --shell-job
root@node0:/mnt/criu# tail -3 /var/log/syslog
Mar 29 17:12:08 localhost kernel: [ 3554.625535] Possible exploit attempt or buggy program!
Mar 29 17:12:08 localhost kernel: [ 3554.625535] If you believe this is an error you can disable SROP  Protection by #echo 1 > /proc/sys/kernel/disable-srop-protection
Mar 29 17:12:08 localhost kernel: [ 3554.625545] test_[25305] bad frame in rt_sigreturn frame:000000000001e540 ip:7f561542cf20 sp:7ffe004ecfd8 orax:ffffffffffffffff in libc-2.19.so[7f561536c000+1bb0]
root@node0:/mnt/criu# echo 1 > /proc/sys/kernel/disable-srop-protection 
root@node0:/mnt/criu# criu restore -vvvv -o restore.log --shell-job
slept for one second
slept for one second
slept for one second
slept for one second
root@node0:/mnt/criu# 


I'm working on getting dosemu up and running-- are there any other applications
off the top of your head that I should be testing with?

[toc] | [prev] | [next] | [standalone]

#1368645

Cc' the Criu list to attempt to give them a heads up.

Scotty Bauer <sbauer@eng.utah.edu> writes:

> On 03/29/2016 04:34 PM, Linus Torvalds wrote:
>> On Tue, Mar 29, 2016 at 4:38 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>
>>> Then there's an unanswered question: is this patch acceptable given
>>> that it's an ABI break?  Security fixes are sometimes an exception to
>>> the "no ABI breaks" rule, but it's by no means an automatic exception.
>> 
>> So there isn't any "no ABI break" rule - there is only a "does it
>> break real applications" rule.
>> 
>> (This can also be re-stated as: "Talk is cheap", aka "reality trumps
>> documentation".
>> 
>> Documentation is meaningless if it doesn't match reality, and what we
>> actually *do* is what matters.
>> 
>> So the ABI isn't about some theoretical interface documentation, the
>> ABI is about what people use and have tested.
>> 
>> On the one hand, that means that that our ABI is _stricter_ than any
>> documentatiuon, and that "but we can make this change that breaks app
>> XYZ, because XYZ is depending on undocumented behavior" is not an
>> acceptable excuse.
>> 
>> But on the other hand it *also* means that since the ABI is about real
>> programs, not theoretical issues, we can also change things as long as
>> we don't actually break anything that people can notice and depend
>> on).
>> 
>> And while *acute* security holes will be fixed regardless of ABI
>> issues, something like this that is only hardening rather than fixing
>> a particular security hole, really needs to not break any
>> applications.
>> 
>> Because if it does break anything, it needs to be turned off by
>> default. That's a hard rule. And since that would be largely defeating
>> the whole point o fthe series, I think we really need to have made
>> sure nothing breaks before a patch series like this can be accepted.
>> 
>> That said, if this is done right, I don't think it will break
>> anything. CRIU may indeed be a special case, but CRIU isn't really a
>> normal application, and the CRIU people may need to turn this off
>> explicitly, if it does break.
>> 
>> But yes, dosemu needs to be tested, and needs to just continue
>> working. But does dosemu actually create a signal stack, as opposed to
>> just playing with one that has been created for it? I thought it was
>> just the latter case, which should be ok even with a magic cookie in
>> there.
>> 
>>                    Linus
>> 
>
>
> For what it's worth this series is breaking CRIU, I just tested:
>
> root@node0:/mnt/criu# criu restore -vvvv -o restore.log --shell-job
> root@node0:/mnt/criu# tail -3 /var/log/syslog
> Mar 29 17:12:08 localhost kernel: [ 3554.625535] Possible exploit attempt or buggy program!
> Mar 29 17:12:08 localhost kernel: [ 3554.625535] If you believe this is an error you can disable SROP  Protection by #echo 1 > /proc/sys/kernel/disable-srop-protection
> Mar 29 17:12:08 localhost kernel: [ 3554.625545] test_[25305] bad frame in rt_sigreturn frame:000000000001e540 ip:7f561542cf20 sp:7ffe004ecfd8 orax:ffffffffffffffff in libc-2.19.so[7f561536c000+1bb0]
> root@node0:/mnt/criu# echo 1 > /proc/sys/kernel/disable-srop-protection 
> root@node0:/mnt/criu# criu restore -vvvv -o restore.log --shell-job
> slept for one second
> slept for one second
> slept for one second
> slept for one second
> root@node0:/mnt/criu#

Which means that if checkpoint/restart is going to continue to be
something that is possible in Linux it should be possible to
save/restore the per process sig_cookie.  Perhaps with a prctl?

This should be addressed as part of this patchset as making that
information too easily accessible/changable will defeat the security
guarantees.  Making it too difficult to do destroys the ability to
migrate a process from one kernel to another.  As the existence of CRIU
attests it is desirable to have a checkpoint/restart capability in the
kernel.

> I'm working on getting dosemu up and running-- are there any other applications
> off the top of your head that I should be testing with?

There are a set of POSIX functions setcontext, getcontext, makecontext
and swapcontext that to the best of my knowledge deal in signal stacks.
Although I don't know that they use sigreturn.  Anything that makes use
of those is potentially affected.

Perhaps you can find binaries that care by looking for libraries and
executables that import those elf symbols.  glibc certainly provides
them.


Eric

[toc] | [prev] | [next] | [standalone]

#1369271

On Thu, Mar 31, 2016 at 03:22:38PM -0500, Eric W. Biederman wrote:
> 
> Cc' the Criu list to attempt to give them a heads up.

Thanks Eric! I managed to miss this thread (I try to scan
lkml descussions one a day in my inbox, but this one somehow
escaped, thank you!)

> Scotty Bauer <sbauer@eng.utah.edu> writes:
...
> >> Because if it does break anything, it needs to be turned off by
> >> default. That's a hard rule. And since that would be largely defeating
> >> the whole point o fthe series, I think we really need to have made
> >> sure nothing breaks before a patch series like this can be accepted.
> >> 
> >> That said, if this is done right, I don't think it will break
> >> anything. CRIU may indeed be a special case, but CRIU isn't really a
> >> normal application, and the CRIU people may need to turn this off
> >> explicitly, if it does break.
> >> 
> >> But yes, dosemu needs to be tested, and needs to just continue
> >> working. But does dosemu actually create a signal stack, as opposed to
> >> just playing with one that has been created for it? I thought it was
> >> just the latter case, which should be ok even with a magic cookie in
> >> there.
...
> > For what it's worth this series is breaking CRIU, I just tested:
> >
> > root@node0:/mnt/criu# criu restore -vvvv -o restore.log --shell-job
> > root@node0:/mnt/criu# tail -3 /var/log/syslog
> > Mar 29 17:12:08 localhost kernel: [ 3554.625535] Possible exploit attempt or buggy program!
> > Mar 29 17:12:08 localhost kernel: [ 3554.625535] If you believe this is an error you can disable SROP  Protection by #echo 1 > /proc/sys/kernel/disable-srop-protection
> > Mar 29 17:12:08 localhost kernel: [ 3554.625545] test_[25305] bad frame in rt_sigreturn frame:000000000001e540 ip:7f561542cf20 sp:7ffe004ecfd8 orax:ffffffffffffffff in libc-2.19.so[7f561536c000+1bb0]
> > root@node0:/mnt/criu# echo 1 > /proc/sys/kernel/disable-srop-protection 
> > root@node0:/mnt/criu# criu restore -vvvv -o restore.log --shell-job
> > slept for one second
> > slept for one second
> > slept for one second
> > slept for one second
> > root@node0:/mnt/criu#
> 
> Which means that if checkpoint/restart is going to continue to be
> something that is possible in Linux it should be possible to
> save/restore the per process sig_cookie.  Perhaps with a prctl?

Yes please. Currently (together with other aims) we're trying to
remove "root-only" requirement from criu, so user would be able
to c/r non-privileged processes without sudo/su. Thus I presume
such prctl will be cap-sysadmin only and we will have to run some
suid'ed daemon for it or something.

> This should be addressed as part of this patchset as making that
> information too easily accessible/changable will defeat the security
> guarantees.  Making it too difficult to do destroys the ability to
> migrate a process from one kernel to another.  As the existence of CRIU
> attests it is desirable to have a checkpoint/restart capability in the
> kernel.

To change sigframe an attacked process must have had some code
already injected and this cookie guard will help but not _that_
much I think.

> > I'm working on getting dosemu up and running-- are there any other applications
> > off the top of your head that I should be testing with?
> 
> There are a set of POSIX functions setcontext, getcontext, makecontext
> and swapcontext that to the best of my knowledge deal in signal stacks.
> Although I don't know that they use sigreturn.  Anything that makes use
> of those is potentially affected.
> 
> Perhaps you can find binaries that care by looking for libraries and
> executables that import those elf symbols.  glibc certainly provides
> them.

	Cyrill

[toc] | [prev] | [next] | [standalone]

#1366768 — Re: [kernel-hardening] Re: [PATCH v4 0/4] SROP Mitigation: Sigreturn Cookies

[Multipart message — attachments visible in raw view] — view raw

> Then there's an unanswered question: is this patch acceptable given
> that it's an ABI break?  Security fixes are sometimes an exception to
> the "no ABI breaks" rule, but it's by no means an automatic exception.
> 
> --Andy

It seems this could be worked around in general. Processes can have a
bit tracking whether this is enabled, and CRIU can save/restore it. It
would just leave it off for resuming old saved processes.

Should CRIU really be covered by the kernel's ABI guarantee though? It
seems like this was meant to be extensible, so it's adding an extra ABI
guarantee that wasn't there before. It makes sense to freeze this ABI
for CRIU, but a version field should be added first in one final ABI
break if it's not too late.

[toc] | [prev] | [next] | [standalone]

#1366727

On 03/29/2016 03:29 PM, Andy Lutomirski wrote:
> On Tue, Mar 29, 2016 at 12:53 PM, Scott Bauer <sbauer@eng.utah.edu> wrote:
>> Sigreturn-oriented programming is a new attack vector in userland
>> where an attacker crafts a fake signal frame on the stack and calls
>> sigreturn. The kernel will extract the fake signal frame, which
>> contains attacker controlled "saved" registers. The kernel will then
>> transfer control to the attacker controlled userland instruction pointer.
>>
>> To prevent SROP attacks the kernel needs to know or be able to dervive
>> whether a sigreturn it is processing is in response to a legitimate
>> signal the kernel previously delivered.
>>
>> Further information and test code can be found in Documentation/security
>> and this excellent article:
>> http://lwn.net/Articles/676803/
>>
>> These patches implement the necessary changes to generate a cookie
>> which will be placed above signal frame upon signal delivery to userland.
>> The cookie is generated using a per-process random value xor'd with
>> the address where the cookie will be stored on the stack.
>>
>> Upon a sigreturn the kernel will extract the cookie from userland,
>> recalculate what the original cookie should be and verify that the two
>> do not differ. If the two differ the kernel will terminate the process
>> with a SIGSEGV.
>>
>> This prevents SROP by adding a value that the attacker cannot guess,
>> but the kernel can verify. Therefore an attacker cannot use sigreturn as
>> a method to control the flow of a process.
>>
> 
> Has anyone verified that this doesn't break CRIU cross-machine (or
> cross-boot) migration and that this doesn't break dosemu?  You're
> changing the ABI here.
> 

I haven't yet I'll do that to verify it breaks -- I'm pretty sure under some
conditions it will break CRIU. That's why we added the sysctl to turn it off. 
Should I have mentioned this in the main commit that it possibly breaks CRIU/DOSEMU?
I went ahead and added that to the Documentation.

[toc] | [prev] | [next] | [standalone]

#1366766

On Tue, Mar 29, 2016 at 2:53 PM, Scott Bauer <sbauer@eng.utah.edu> wrote:
>
> These patches implement the necessary changes to generate a cookie
> which will be placed above signal frame upon signal delivery to userland.
> The cookie is generated using a per-process random value xor'd with
> the address where the cookie will be stored on the stack.

Side note: wouldn't it be better to make the cookie something that
doesn't make it trivial to figure out the random value in case you
already have access to a signal stack?

Maybe there could be a stronger variation of this that makes the
cookie be something like a single md5 round (not a full md5).
Something fast, and not necessarily secure, but something that needs
more than one single CPU instruction to figure out.

So you could do 4 32

 - the random value
 - the low 32 bits of the address of the cookie
 - the low 32 bits of the return point stack and instruction pointer

Yes, yes, md5 is not cryptographically secure, and making it a single
iteration rather than the full four makes it even less so, but if the
attacker can generate long arbitrary code, then the whole SROP is
pointless to begin with, no?

In contrast, with the plain xor, the SROP would be a trivial operation
if you can just force it to happen within the context of a signal, so
that you can just re-use the signal return stack as-is. But mixing in
the returning IP and SP would make it *much* harder to use the
sigreturn as an attack vector.

I realize that this would likely need to be a separate and non-default
extra hardening mode, because there are *definitely* applications that
take signals and then update the return address (maybe single-stepping
over instructions etc). But for a *lot* of applications, signal return
implies changing no signal state at all, and mixing in the returning
IP and SP would seem to be a fundamentally stronger cookie.

No?

                 Linus

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web