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 <[email protected]>
Signed-off-by: Scott Bauer <[email protected]>
---
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 dcd4ac7..3de0a32 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -56,6 +56,7 @@
#include <linux/pipe_fs_i.h>
#include <linux/oom.h>
#include <linux/compat.h>
+#include <linux/random.h>
#include <asm/uaccess.h>
#include <asm/mmu_context.h>
@@ -1135,6 +1136,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(¤t->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 a10494a..556162f 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1497,6 +1497,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 0508544..00e4a16 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
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 <[email protected]>
Signed-off-by: Scott Bauer <[email protected]>
---
arch/x86/ia32/ia32_signal.c | 63 +++++++++++++++++++++++++---
arch/x86/include/asm/fpu/signal.h | 1 +
arch/x86/include/asm/sighandling.h | 5 ++-
arch/x86/kernel/fpu/signal.c | 10 +++++
arch/x86/kernel/signal.c | 86 +++++++++++++++++++++++++++++++++-----
5 files changed, 146 insertions(+), 19 deletions(-)
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 0552884..77ee2dd 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 **user_cookie)
{
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 = compat_ptr(tmp + fpu__getsize(1));
+ else
+ *user_cookie = 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,15 @@ 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:
@@ -142,6 +161,7 @@ asmlinkage long sys32_rt_sigreturn(void)
{
struct pt_regs *regs = current_pt_regs();
struct rt_sigframe_ia32 __user *frame;
+ void __user *user_cookie;
sigset_t set;
frame = (struct rt_sigframe_ia32 __user *)(regs->sp - 4);
@@ -153,7 +173,13 @@ 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 = (void __user *)(((unsigned long) frame) + sizeof(*frame));
+
+ if (verify_clear_sigcookie(user_cookie))
goto badframe;
if (compat_restore_altstack(&frame->uc.uc_stack))
@@ -213,7 +239,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)
{
struct fpu *fpu = ¤t->thread.fpu;
unsigned long sp;
@@ -230,11 +257,21 @@ 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 = (void __user *)sp + math_size;
+
if (copy_fpstate_to_sigframe(*fpstate, (void __user *)fx_aligned,
math_size) < 0)
return (void __user *) -1L;
@@ -244,6 +281,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 = (void __user *) (sp + frame_size);
+
return (void __user *) sp;
}
@@ -254,6 +295,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 +308,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 +317,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 +378,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 +393,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..a720b95 100644
--- a/arch/x86/include/asm/fpu/signal.h
+++ b/arch/x86/include/asm/fpu/signal.h
@@ -27,6 +27,7 @@ extern void convert_to_fxsr(struct task_struct *tsk,
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);
diff --git a/arch/x86/include/asm/sighandling.h b/arch/x86/include/asm/sighandling.h
index 89db467..971c8b2 100644
--- a/arch/x86/include/asm/sighandling.h
+++ b/arch/x86/include/asm/sighandling.h
@@ -12,8 +12,9 @@
X86_EFLAGS_ZF | X86_EFLAGS_AF | X86_EFLAGS_PF | \
X86_EFLAGS_CF | X86_EFLAGS_RF)
-void signal_fault(struct pt_regs *regs, void __user *frame, char *where);
-int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc);
+void signal_fault(struct pt_regs *regs, void __user *frame, const char *where);
+int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc,
+ void __user **user_cookie);
int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
struct pt_regs *regs, unsigned long mask);
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 cb6282c..b756def 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>
@@ -61,7 +62,9 @@
regs->seg = GET_SEG(seg) | 3; \
} while (0)
-int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc)
+
+int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc,
+ void __user **user_cookie)
{
unsigned long buf_val;
void __user *buf;
@@ -112,8 +115,14 @@ int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc)
buf = (void __user *)buf_val;
} get_user_catch(err);
- err |= fpu__restore_sig(buf, config_enabled(CONFIG_X86_32));
+ if (buf_val != 0)
+ *user_cookie = (void __user *)
+ (buf_val + fpu__getsize(config_enabled(CONFIG_X86_32)));
+ else
+ *user_cookie = NULL;
+
+ err |= fpu__restore_sig(buf, config_enabled(CONFIG_X86_32));
force_iret();
return err;
@@ -200,7 +209,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)
{
/* Default to using normal stack */
unsigned long math_size = 0;
@@ -227,14 +236,27 @@ 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 = (void __user *)sp + math_size;
}
sp = align_sigframe(sp - frame_size);
+ if (!fpu->fpstate_active)
+ *cookie = (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.
@@ -281,8 +303,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;
@@ -290,6 +314,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;
@@ -344,12 +371,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);
@@ -408,9 +440,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(*frame),
+ &fp, &cookie_location);
if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
return -EFAULT;
@@ -420,6 +454,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. */
if (cpu_has_xsave)
@@ -476,8 +513,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;
@@ -487,6 +526,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. */
if (cpu_has_xsave)
@@ -541,6 +583,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);
@@ -554,8 +597,15 @@ asmlinkage unsigned long sys_sigreturn(void)
set_current_blocked(&set);
- if (restore_sigcontext(regs, &frame->sc))
+ if (restore_sigcontext(regs, &frame->sc, &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:
@@ -569,6 +619,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;
frame = (struct rt_sigframe __user *)(regs->sp - sizeof(long));
@@ -579,7 +630,13 @@ asmlinkage long sys_rt_sigreturn(void)
set_current_blocked(&set);
- if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
+ if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &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))
@@ -740,7 +797,7 @@ void do_signal(struct pt_regs *regs)
restore_saved_sigmask();
}
-void signal_fault(struct pt_regs *regs, void __user *frame, char *where)
+void signal_fault(struct pt_regs *regs, void __user *frame, const char *where)
{
struct task_struct *me = current;
@@ -762,6 +819,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;
frame = (struct rt_sigframe_x32 __user *)(regs->sp - 8);
@@ -773,7 +831,13 @@ asmlinkage long sys32_x32_rt_sigreturn(void)
set_current_blocked(&set);
- if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
+ if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &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
This patch adds a sysctl argument to disable SROP protection.
Cc: Abhiram Balasubramanian <[email protected]>
Signed-off-by: Scott Bauer <[email protected]>
---
include/linux/signal.h | 2 ++
kernel/signal.c | 12 ++++++++++--
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..7e580d9 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -9,6 +9,8 @@ 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 00e4a16..dec4e20 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,6 +2453,9 @@ 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;
@@ -2459,13 +2463,17 @@ int verify_clear_sigcookie(unsigned long __user *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");
+ ". Possible exploit attempt or buggy program!\n If 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)
+ return put_user(user_cookie, sig_cookie_ptr);
}
EXPORT_SYMBOL(verify_clear_sigcookie);
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 97715fd..6c95172 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -524,6 +524,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
On Tue, Mar 8, 2016 at 12:47 PM, Scott Bauer <[email protected]> wrote:
> 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.
>
Potentially silly question: it's been a while since I read the SROP
paper, but would the technique be effectively mitigated if sigreturn
were to zero out the whole signal frame before returning to user mode?
On Tue, 8 Mar 2016 13:47:55 -0700
Scott Bauer <[email protected]> wrote:
> This patch adds a sysctl argument to disable SROP protection.
Shouldn't it be a sysctl to enable it irrevocably, otherwise if I have DAC
capability I can turn off SROP and attack something to get to higher
capability levels ?
(The way almost all distros are set up its kind of academic but for a
properly secured system it might matter).
Alan
> static int ia32_restore_sigcontext(struct pt_regs *regs,
> - struct sigcontext_32 __user *sc)
> + struct sigcontext_32 __user *sc,
> + void __user **user_cookie)
> {
> 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 = compat_ptr(tmp + fpu__getsize(1));
> + else
> + *user_cookie = NULL;
user_cookie is is __user, so shouldn't just be poking at it without
get/put_user ? It might fault if someone has engineered a bad stack frame.
Alan
Hi Scott,
[auto build test ERROR on tip/x86/core]
[also build test ERROR on v4.5-rc7]
[cannot apply to next-20160308]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Scott-Bauer/SROP-Mitigation-Architecture-independent-code-for-signal-cookies/20160309-045202
config: i386-tinyconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
Note: the linux-review/Scott-Bauer/SROP-Mitigation-Architecture-independent-code-for-signal-cookies/20160309-045202 HEAD 52ff6d1c09bbf7b33f786ccaa3de5521002a92fa builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
kernel/signal.c: In function 'verify_clear_sigcookie':
>> kernel/signal.c:2469:1: error: expected ';' before '}' token
}
^
vim +2469 kernel/signal.c
2463 return 1;
2464
2465 }
2466
2467 user_cookie = 0;
2468 return put_user(user_cookie, sig_cookie_ptr)
> 2469 }
2470
2471 EXPORT_SYMBOL(verify_clear_sigcookie);
2472 EXPORT_SYMBOL(set_sigcookie);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 03/08/2016 02:03 PM, One Thousand Gnomes wrote:
>> static int ia32_restore_sigcontext(struct pt_regs *regs,
>> - struct sigcontext_32 __user *sc)
>> + struct sigcontext_32 __user *sc,
>> + void __user **user_cookie)
>> {
>> 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 = compat_ptr(tmp + fpu__getsize(1));
>> + else
>> + *user_cookie = NULL;
>
> user_cookie is is __user, so shouldn't just be poking at it without
> get/put_user ? It might fault if someone has engineered a bad stack frame.
>
> Alan
>
I guess I got a little carried away with the __user annotations. I will remove the __user annotation from this function
since what we're dereferencing isn't actually a __user pointer, its some stack memory sitting in the caller's stack frame.
see patch 2/3:
void __user *user_cookie;
vv-- Passed in here
if (ia32_restore_sigcontext(regs, &frame->sc, &user_cookie))
goto badframe;
On 03/08/2016 01:58 PM, Andy Lutomirski wrote:
> On Tue, Mar 8, 2016 at 12:47 PM, Scott Bauer <[email protected]> wrote:
>> 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.
>>
>
> Potentially silly question: it's been a while since I read the SROP
> paper, but would the technique be effectively mitigated if sigreturn
> were to zero out the whole signal frame before returning to user mode?
>
I don't know if I fully understand your question, but I'll respond anyway.
SROP is possible because the kernel doesn't know whether or not the
incoming sigreturn syscall is in response from a legitimate signal that
the kernel had previously delivered and the program handled. So essentially
these patches are an attempt to give the kernel a way to verify whether or
not the the incoming sigreturn is a valid response or a exploit trying to
hijack control of the user program.
So no, zeroing out the frame wouldn't do much because if I understand your
question correctly once we call sigreturn the kernel is going to hand off
control to wherever the sigframe tells it to so I don't think zeroing would
do much.
The reason why I zero out the cookie is so if there is a stack leak bug or
something along those lines an attacker couldnt leak the cookie and try and
derive what the per-process kernel secret is.
Hope that clarifies!
On Tue, Mar 8, 2016 at 1:49 PM, Scotty Bauer <[email protected]> wrote:
>
>
> On 03/08/2016 01:58 PM, Andy Lutomirski wrote:
>> On Tue, Mar 8, 2016 at 12:47 PM, Scott Bauer <[email protected]> wrote:
>>> 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.
>>>
>>
>> Potentially silly question: it's been a while since I read the SROP
>> paper, but would the technique be effectively mitigated if sigreturn
>> were to zero out the whole signal frame before returning to user mode?
>>
>
> I don't know if I fully understand your question, but I'll respond anyway.
>
> SROP is possible because the kernel doesn't know whether or not the
> incoming sigreturn syscall is in response from a legitimate signal that
> the kernel had previously delivered and the program handled. So essentially
> these patches are an attempt to give the kernel a way to verify whether or
> not the the incoming sigreturn is a valid response or a exploit trying to
> hijack control of the user program.
>
I got that part, but I thought that the interesting SROP bit was using
sigreturn to return back to a frame where you could just repeat the
sigreturn a bunch of times to compute things and do other evil. I'm
wondering whether zeroing the whole frame would make SROP much less
interesting to an attacker.
--Andy
On 03/08/2016 02:57 PM, Andy Lutomirski wrote:
> On Tue, Mar 8, 2016 at 1:49 PM, Scotty Bauer <[email protected]> wrote:
>>
>>
>> On 03/08/2016 01:58 PM, Andy Lutomirski wrote:
>>> On Tue, Mar 8, 2016 at 12:47 PM, Scott Bauer <[email protected]> wrote:
>>>> 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.
>>>>
>>>
>>> Potentially silly question: it's been a while since I read the SROP
>>> paper, but would the technique be effectively mitigated if sigreturn
>>> were to zero out the whole signal frame before returning to user mode?
>>>
>>
>> I don't know if I fully understand your question, but I'll respond anyway.
>>
>> SROP is possible because the kernel doesn't know whether or not the
>> incoming sigreturn syscall is in response from a legitimate signal that
>> the kernel had previously delivered and the program handled. So essentially
>> these patches are an attempt to give the kernel a way to verify whether or
>> not the the incoming sigreturn is a valid response or a exploit trying to
>> hijack control of the user program.
>>
>
> I got that part, but I thought that the interesting SROP bit was using
> sigreturn to return back to a frame where you could just repeat the
> sigreturn a bunch of times to compute things and do other evil. I'm
> wondering whether zeroing the whole frame would make SROP much less
> interesting to an attacker.
>
> --Andy
>
Ah, I see now. I believe that would work for subsequent sigreturns but not
the first.
The paper did talk about actually ROP'ing using sigreturns but I never really
liked that idea. I think they missed the obvious reason an attacker would use
SROP. The reason an attacker would use is it gives you an extremely easy
way to get values into registers.. you just set them to what you want them to be
then sigret. Previously an attacker would have to find gadgets and ROP around until
the registers are in a good enough state to do what ever they want.
I mostly designed the patches with that in mind instead of actually ROPing with sigret.
I can certainly add zeroing the stack frame, but not sure if there would be a performance
regression in doing so or if it's really relevant if we keep the cookie.
* Scott Bauer <[email protected]> wrote:
> 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.
> /*
> + * Canary value for signal frames placed on user stack.
> + * This helps mitigate "Signal Return oriented program"
> + * exploits in userland.
> + */
> + unsigned long sig_cookie;
Could you please add a high level description in Documentation
that explains the attack and the way how this mitigation code
prevents that kind of attack?
Also, the first changelogs should contain more high level
description as well. For example, what does the 'verification'
of the signal cookie mean, and how does it prevent an SROP
attempt?
All of these patches seem to assume that people reading this code
know what SROP is and how we defend against it - that is not so.
Thanks,
Ingo
On 03/08/2016 02:57 PM, Andy Lutomirski wrote:
> On Tue, Mar 8, 2016 at 1:49 PM, Scotty Bauer <[email protected]> wrote:
>>
>>
>> On 03/08/2016 01:58 PM, Andy Lutomirski wrote:
>>> On Tue, Mar 8, 2016 at 12:47 PM, Scott Bauer <[email protected]> wrote:
>>>> 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.
>>>>
>>>
>>> Potentially silly question: it's been a while since I read the SROP
>>> paper, but would the technique be effectively mitigated if sigreturn
>>> were to zero out the whole signal frame before returning to user mode?
>>>
>>
>> I don't know if I fully understand your question, but I'll respond anyway.
>>
>> SROP is possible because the kernel doesn't know whether or not the
>> incoming sigreturn syscall is in response from a legitimate signal that
>> the kernel had previously delivered and the program handled. So essentially
>> these patches are an attempt to give the kernel a way to verify whether or
>> not the the incoming sigreturn is a valid response or a exploit trying to
>> hijack control of the user program.
>>
>
> I got that part, but I thought that the interesting SROP bit was using
> sigreturn to return back to a frame where you could just repeat the
> sigreturn a bunch of times to compute things and do other evil. I'm
> wondering whether zeroing the whole frame would make SROP much less
> interesting to an attacker.
>
> --Andy
>
I've been thinking about this a little bit more. I don't think zeroing the frame
is a proper mitigation. If an attacker has the ability to write a lot of data to
the stack they could simply create a new fake signal frame above the current frame.
In this scenario the kernel would zero the current frame then return somewhere attacker
controlled, where the attackers payload would then use the next signal frame above
the zero'd frame.
So while this zeroing would solve a stricter case where an attacker has to keep reusing
the same frame over and over, perhaps to avoid overwriting a stack cookie, It doesn't solve
every case.
Thanks for the good ideas.
On 03/09/2016 01:32 AM, Ingo Molnar wrote:
>
> * Scott Bauer <[email protected]> wrote:
>
>> 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.
>
>> /*
>> + * Canary value for signal frames placed on user stack.
>> + * This helps mitigate "Signal Return oriented program"
>> + * exploits in userland.
>> + */
>> + unsigned long sig_cookie;
>
> Could you please add a high level description in Documentation
> that explains the attack and the way how this mitigation code
> prevents that kind of attack?
>
> Also, the first changelogs should contain more high level
> description as well. For example, what does the 'verification'
> of the signal cookie mean, and how does it prevent an SROP
> attempt?
>
> All of these patches seem to assume that people reading this code
> know what SROP is and how we defend against it - that is not so.
>
> Thanks,
>
> Ingo
>
I'm going to submit v4 to fix some nits where I'll include the explanation
and a change log, I apologize for not doing that here. In the meantime if
you don't mind visiting a link I included a brief explanation on previous
versions of the patch set.
https://lkml.org/lkml/2016/2/6/166
Thanks
On Wed, 9 Mar 2016 15:07:07 -0700
Scotty Bauer <[email protected]> wrote:
> On 03/09/2016 01:32 AM, Ingo Molnar wrote:
> >
> > Could you please add a high level description in Documentation
> > that explains the attack and the way how this mitigation code
> > prevents that kind of attack?
> >
> > Also, the first changelogs should contain more high level
> > description as well. For example, what does the 'verification'
> > of the signal cookie mean, and how does it prevent an SROP
> > attempt?
> >
> > All of these patches seem to assume that people reading this code
> > know what SROP is and how we defend against it - that is not so.
>
> I'm going to submit v4 to fix some nits where I'll include the explanation
> and a change log, I apologize for not doing that here. In the meantime if
> you don't mind visiting a link I included a brief explanation on previous
> versions of the patch set.
>
> https://lkml.org/lkml/2016/2/6/166
The curious might also find background information in my article about this
patch set:
https://lwn.net/Articles/676803/
(The information still belongs with the patch posting, of course...)
jon
On Tue, Mar 8, 2016 at 1:00 PM, One Thousand Gnomes
<[email protected]> wrote:
> On Tue, 8 Mar 2016 13:47:55 -0700
> Scott Bauer <[email protected]> wrote:
>
>> This patch adds a sysctl argument to disable SROP protection.
>
> Shouldn't it be a sysctl to enable it irrevocably, otherwise if I have DAC
> capability I can turn off SROP and attack something to get to higher
> capability levels ?
>
> (The way almost all distros are set up its kind of academic but for a
> properly secured system it might matter).
Perhaps use proc_dointvec_minmax_sysadmin instead to tie changes
strictly to CAP_SYS_ADMIN?
-Kees
>
> Alan
--
Kees Cook
Chrome OS & Brillo Security
On Wed, Mar 9, 2016 at 10:36 PM, Kees Cook <[email protected]> wrote:
> On Tue, Mar 8, 2016 at 1:00 PM, One Thousand Gnomes
> <[email protected]> wrote:
>> On Tue, 8 Mar 2016 13:47:55 -0700
>> Scott Bauer <[email protected]> wrote:
>>
>>> This patch adds a sysctl argument to disable SROP protection.
>>
>> Shouldn't it be a sysctl to enable it irrevocably, otherwise if I have DAC
>> capability I can turn off SROP and attack something to get to higher
>> capability levels ?
>>
>> (The way almost all distros are set up its kind of academic but for a
>> properly secured system it might matter).
>
> Perhaps use proc_dointvec_minmax_sysadmin instead to tie changes
> strictly to CAP_SYS_ADMIN?
I don't see why this needs to be irrevocable. If you have
CAP_SYS_ADMIN or write access to /proc or whatever, you can do much
worse things than turning off a user-level mitigation. For example,
you can ptrace things. Also, you're already root, so what's the
point?
--Andy
* Jonathan Corbet <[email protected]> wrote:
> On Wed, 9 Mar 2016 15:07:07 -0700
> Scotty Bauer <[email protected]> wrote:
>
> > On 03/09/2016 01:32 AM, Ingo Molnar wrote:
> > >
> > > Could you please add a high level description in Documentation
> > > that explains the attack and the way how this mitigation code
> > > prevents that kind of attack?
> > >
> > > Also, the first changelogs should contain more high level
> > > description as well. For example, what does the 'verification'
> > > of the signal cookie mean, and how does it prevent an SROP
> > > attempt?
> > >
> > > All of these patches seem to assume that people reading this code
> > > know what SROP is and how we defend against it - that is not so.
> >
> > I'm going to submit v4 to fix some nits where I'll include the explanation
> > and a change log, I apologize for not doing that here. In the meantime if
> > you don't mind visiting a link I included a brief explanation on previous
> > versions of the patch set.
> >
> > https://lkml.org/lkml/2016/2/6/166
>
> The curious might also find background information in my article about this
> patch set:
>
> https://lwn.net/Articles/676803/
Scott, mind including a prominent link to the (excellent!) LWN.net article in the
changelog/documentation as well?
Thanks,
Ingo