On Sat, Jun 18, 2016 at 3:21 AM, Andy Lutomirski <[email protected]> wrote: > On Jun 18, 2016 12:02 AM, "Kees Cook" <[email protected]> wrote: >> >> On Fri, Jun 17, 2016 at 11:55 AM, Andy Lutomirski <[email protected]> >> wrote: >> > On Fri, Jun 17, 2016 at 12:15 AM, James Morris <[email protected]> wrote: >> >> On Tue, 14 Jun 2016, Kees Cook wrote: >> >> >> >>> Hi, >> >>> >> >>> Please pull these seccomp changes for next. These have been tested by >> >>> myself and Andy, and close a long-standing issue with seccomp where >> >>> tracers >> >>> could change the syscall out from under seccomp. >> >> >> >> Pulled to security -next. >> > >> > As a heads up: I think this doesn't quite close the hole on x86. Consider: >> > >> > 64-bit task arranges to be traced by a 32-bit task (or presumably a >> > 64-bit task that calls ptrace via int80). >> > >> > Tracer does PTRACE_SYSCALL. >> > >> > Tracee does a normal syscall. >> > >> > Tracer writes tracee's orig_ax, thus invoking this thing in >> > arch/x86/kernel/ptrace.c: >> > >> > if (syscall_get_nr(child, regs) >= 0) >> > child->thread.status |= TS_COMPAT; >> > >> > Tracer resumes and gets confused. >> > >> > I think the right fix is to just delete: >> > >> > if (syscall_get_nr(child, regs) >= 0) >> > child->thread.status |= TS_COMPAT; >> > >> > from ptrace.c. The comment above it is garbage, too. >> >> I'm perfectly happy to see it removed. I can't make sense of the comment. :) >> >> That said, the only confusion I see is pretty minor. The arch is saved >> before the tracer could force TS_COMPAT, so nothing confused is handed >> to seccomp (the first time). And the syscall will continue to be >> looked up on sys_call_table not ia32_sys_call_table. > > Hmm, right, but... > >> >> The only thing I see is if the tracer has also added a >> SECCOMP_RET_TRACE filter, after which the recheck will reload all the >> seccomp info, including the arch. And at this point, a sensible filter >> will reject a non-matching architecture. >> > > Yes for some filters, but others might have different logic for > different arches, at which point there's a minor bypass. > >> Maybe I'm missing something more? >> > > You can also use this to do a 64-bit syscall with in_compat_syscall() > returning true, which could cause issues for audit and maybe some > ioctl handlers irrespective of this patch series. I'll see about > getting it fixed in x86/urgent.
Hi Kees and James- On further consideration: (a) that TS_COMPAT thing is highly, highly buggy -- much buggier and more confusing than I thought, and not in the way I thought. (b) it doesn't interfere with seccomp at all, so fixing it is not at all a prerequisite for these changes. -- Andy Lutomirski AMA Capital Management, LLC
