From 2e03bd94d311781d5a47c7e95049b42da5651b33 Mon Sep 17 00:00:00 2001 From: Jonas 'Sortie' Termansen Date: Sat, 14 May 2016 01:14:26 +0200 Subject: [PATCH] Add protection against sigreturn oriented programming (SROP). This change hardens against invalid calls to sigreturn, which is a very useful gadget when compromising a process. The system call now verifies it is a real return from a signal and aborts the process otherwise. This should render such attacks impossible in threads that are not servicing a signal, and infeasible in threads that are handling signals they are yet to return from. The kernel now keeps track for each thread how many signals are being handled but haven't returned yet. Each thread now has a random signal value. It is re-randomized when the thread handles a signal and the current signal counter is zero. This is xorred with the context address and used as canary on the stack during signal dispatch, protecting the saved context on the stack. This works mostly like the regular stack protector. The kernel now keeps track of the stack pointer for a single handled signal per thread. It doesn't seem worth it to keep track of multiple handled signals, as more than one is rare. Note that each delivered signal will not necessarily result in a sigreturn because it is valid for a thread to longjmp(3) out of a signal handler to a valid jmp_buf. The sigreturn system call will abort if either: - It was not called from the kernel sigreturn page. - The thread is not currently processing a signal. - The thread is processing a single signal, and the stack pointer did not have the expected value. - It fails to read the context on the stack. - The canary is wrong. --- kernel/include/sortix/kernel/thread.h | 4 + kernel/process.cpp | 2 +- kernel/signal.cpp | 115 +++++++++++++++++++++++--- kernel/thread.cpp | 4 + 4 files changed, 111 insertions(+), 14 deletions(-) diff --git a/kernel/include/sortix/kernel/thread.h b/kernel/include/sortix/kernel/thread.h index 5db491cb..8cfcf4af 100644 --- a/kernel/include/sortix/kernel/thread.h +++ b/kernel/include/sortix/kernel/thread.h @@ -76,9 +76,13 @@ public: stack_t signal_stack; addr_t kernelstackpos; size_t kernelstacksize; + size_t signal_count; + uintptr_t signal_single_frame; + uintptr_t signal_canary; bool kernelstackmalloced; bool pledged_destruction; bool force_no_signals; + bool signal_single; Clock execute_clock; Clock system_clock; diff --git a/kernel/process.cpp b/kernel/process.cpp index 1f229dca..dc7f074d 100644 --- a/kernel/process.cpp +++ b/kernel/process.cpp @@ -1689,7 +1689,7 @@ void sys_scram(int event, const void* user_info) (intmax_t) process->pid, event); } - // TODO: Allow debugging this event. + // TODO: Allow debugging this event (and see signal.cpp sigreturn). kthread_exit(); } diff --git a/kernel/signal.cpp b/kernel/signal.cpp index b7e5182d..43f8d0e9 100644 --- a/kernel/signal.cpp +++ b/kernel/signal.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -508,6 +509,7 @@ struct stack_frame siginfo_t* siginfo_param; ucontext_t* ucontext_param; void* cookie_param; + uintptr_t canary; ucontext_t ucontext; siginfo_t siginfo; }; @@ -516,6 +518,7 @@ struct stack_frame { uintptr_t misalignment[1]; uintptr_t sigreturn; + uintptr_t canary; ucontext_t ucontext; siginfo_t siginfo; }; @@ -728,6 +731,12 @@ retry_another_signal: #error "You need to format the stack frame" #endif + // Store a canary so it can later be verified this is a real signal being + // returned from. + if ( signal_count == 0 ) + arc4random_buf(&signal_canary, sizeof(signal_canary)); + stack_frame.canary = signal_canary ^ (uintptr_t) stack; + // Format the siginfo into the stack frame. stack_frame.siginfo.si_signo = signum; #if defined(__i386__) || defined(__x86_64__) @@ -783,6 +792,17 @@ retry_another_signal: } } + // Know for sure when there's no signal still being handled. This is an + // over-approximation as programs may do absolutely awful things such as + // longjmp(3)ing out of signal handlers, so each delivered signal does not + // nessesarily mean a sigreturn. This will work correctly in reasonable + // programs though and will harden those programs. Remember the stack frame + // as well for verification if there is no recursive signal handling. + if ( signal_count != SIZE_MAX ) + signal_count++; + if ( (signal_single = signal_count == 1) ) + signal_single_frame = (uintptr_t) stack; + // Run the signal handler by returning to user-space. return; } @@ -792,31 +812,100 @@ void Thread::HandleSigreturn(struct interrupt_context* intctx) assert(Interrupt::IsEnabled()); assert(this == CurrentThread()); - ScopedLock lock(&process->signal_lock); - struct stack_frame stack_frame; - const struct stack_frame* user_stack_frame; + struct stack_frame* user_stack_frame; #if defined(__i386__) - user_stack_frame = (const struct stack_frame*) + user_stack_frame = (struct stack_frame*) (intctx->esp - offsetof(struct stack_frame, sigreturn) - 4); + bool wrong_ip = intctx->eip != (uintptr_t) process->sigreturn + 2; #elif defined(__x86_64__) - user_stack_frame = (const struct stack_frame*) + user_stack_frame = (struct stack_frame*) (intctx->rsp - offsetof(struct stack_frame, sigreturn) - 8); + bool wrong_ip = intctx->rip != (uintptr_t) process->sigreturn + 2; #else #error "You need to locate the stack we passed the signal handler" #endif - if ( CopyFromUser(&stack_frame, user_stack_frame, sizeof(stack_frame)) ) + // Protect against sigreturn oriented programming (SROP) as described in + // "Framing Signals - A Return to Portable Shellcode" (Bosman and Bos 2014). + + // If the we're not called from the kernel sigreturn page, it is not a valid + // sigreturn. + if ( wrong_ip ) { - memcpy(&signal_mask, &stack_frame.ucontext.uc_sigmask, sizeof(signal_mask)); - memcpy(&signal_stack, &stack_frame.ucontext.uc_stack, sizeof(signal_stack)); - signal_stack.ss_flags &= __SS_SUPPORTED_FLAGS; - struct thread_registers resume_regs; - Scheduler::SaveInterruptedContext(intctx, &resume_regs); - DecodeMachineContext(&stack_frame.ucontext.uc_mcontext, &resume_regs); - Scheduler::LoadInterruptedContext(intctx, &resume_regs); + process->ExitThroughSignal(SIGABRT); + Log::PrintF("%s[%ji]: sigreturn smashing detected: Bypassed kernel sigreturn page\n", + process->program_image_path, + (intmax_t) process->pid); + // TODO: Allow debugging this event (see scram(2)). + kthread_exit(); } + // If no signals are being serviced by this thread at the moment, it is not + // a valid sigreturn. + if ( signal_count == 0 ) + { + process->ExitThroughSignal(SIGABRT); + Log::PrintF("%s[%ji]: sigreturn smashing detected: Thread wasn't servicing a signal\n", + process->program_image_path, + (intmax_t) process->pid); + // TODO: Allow debugging this event (see scram(2)). + kthread_exit(); + } + + // If a single signal is being serviced by this thread at the moment, the + // stack pointer must be what we expect it to be. If there's multiple, we + // don't know which one is the correct. (We could keep track of them and + // ensure it's one of them, but that's not really worth it. The list of such + // delivered signals could grow without bound because it's valid to longjmp + // out of a signal handler) + if ( signal_single && (uintptr_t) user_stack_frame != signal_single_frame ) + { + process->ExitThroughSignal(SIGABRT); + Log::PrintF("%s[%ji]: sigreturn smashing detected: Stack pointer was wrong\n", + process->program_image_path, + (intmax_t) process->pid); + // TODO: Allow debugging this event (see scram(2)). + kthread_exit(); + } + + // If we couldn't read the frame, the sigreturn is certainly bad. + if ( !CopyFromUser(&stack_frame, user_stack_frame, sizeof(stack_frame)) ) + { + process->ExitThroughSignal(SIGABRT); + Log::PrintF("%s[%ji]: sigreturn smashing detected: Couldn't read stack frame: %m\n", + process->program_image_path, + (intmax_t) process->pid); + // TODO: Allow debugging this event (see scram(2)). + kthread_exit(); + } + ZeroUser(user_stack_frame, sizeof(*user_stack_frame)); + + // If the random canary isn't correct, the sigreturn is certianly bad. + if ( stack_frame.canary != (signal_canary ^ (uintptr_t) user_stack_frame) ) + { + process->ExitThroughSignal(SIGABRT); + Log::PrintF("%s[%ji]: sigreturn smashing detected: Verification value was incorrect\n", + process->program_image_path, + (intmax_t) process->pid); + // TODO: Allow debugging this event (see scram(2)). + kthread_exit(); + } + + ScopedLock lock(&process->signal_lock); + + memcpy(&signal_mask, &stack_frame.ucontext.uc_sigmask, sizeof(signal_mask)); + memcpy(&signal_stack, &stack_frame.ucontext.uc_stack, sizeof(signal_stack)); + signal_stack.ss_flags &= __SS_SUPPORTED_FLAGS; + struct thread_registers resume_regs; + Scheduler::SaveInterruptedContext(intctx, &resume_regs); + DecodeMachineContext(&stack_frame.ucontext.uc_mcontext, &resume_regs); + Scheduler::LoadInterruptedContext(intctx, &resume_regs); + + if ( signal_count != SIZE_MAX ) + signal_count--; + signal_single = false; + UpdatePendingSignals(this); intctx->signal_pending = asm_signal_is_pending; diff --git a/kernel/thread.cpp b/kernel/thread.cpp index d8bbc23b..7a216c2e 100644 --- a/kernel/thread.cpp +++ b/kernel/thread.cpp @@ -89,9 +89,13 @@ Thread::Thread() memset(®isters, 0, sizeof(registers)); kernelstackpos = 0; kernelstacksize = 0; + signal_count = 0; + signal_single_frame = 0; + signal_canary = 0; kernelstackmalloced = false; pledged_destruction = false; force_no_signals = false; + signal_single = false; sigemptyset(&signal_pending); sigemptyset(&signal_mask); memset(&signal_stack, 0, sizeof(signal_stack));