lkml.org 
[lkml]   [2017]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 3/4] seccomp: Create an action to log before allowing
    From
    Date
    On 02/07/2017 06:33 PM, Kees Cook wrote:
    > On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
    >> Add a new action, SECCOMP_RET_LOG, that logs a syscall before allowing
    >> the syscall. At the implementation level, this action is identical to
    >> the existing SECCOMP_RET_ALLOW action. However, it can be very useful when
    >> initially developing a seccomp filter for an application. The developer
    >> can set the default action to be SECCOMP_RET_LOG, maybe mark any
    >> obviously needed syscalls with SECCOMP_RET_ALLOW, and then put the
    >> application through its paces. A list of syscalls that triggered the
    >> default action (SECCOMP_RET_LOG) can be easily gleaned from the logs and
    >> that list can be used to build the syscall whitelist. Finally, the
    >> developer can change the default action to the desired value.
    >>
    >> This provides a more friendly experience than seeing the application get
    >> killed, then updating the filter and rebuilding the app, seeing the
    >> application get killed due to a different syscall, then updating the
    >> filter and rebuilding the app, etc.
    >>
    >> The functionality is similar to what's supported by the various LSMs.
    >> SELinux has permissive mode, AppArmor has complain mode, SMACK has
    >> bring-up mode, etc.
    >>
    >> SECCOMP_RET_LOG is given a lower value than SECCOMP_RET_ALLOW so that
    >> "allow" can be written to the max_action_to_log sysctl in order to get a
    >> list of logged actions without the, potentially larger, set of allowed
    >> actions.
    >>
    >> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
    >> ---
    >> Documentation/prctl/seccomp_filter.txt | 6 ++++++
    >> include/uapi/linux/seccomp.h | 1 +
    >> kernel/seccomp.c | 4 ++++
    >> 3 files changed, 11 insertions(+)
    >>
    >> diff --git a/Documentation/prctl/seccomp_filter.txt b/Documentation/prctl/seccomp_filter.txt
    >> index 1e469ef..ba55a91 100644
    >> --- a/Documentation/prctl/seccomp_filter.txt
    >> +++ b/Documentation/prctl/seccomp_filter.txt
    >> @@ -138,6 +138,12 @@ SECCOMP_RET_TRACE:
    >> allow use of ptrace, even of other sandboxed processes, without
    >> extreme care; ptracers can use this mechanism to escape.)
    >>
    >> +SECCOMP_RET_LOG:
    >> + Results in the system call being executed after it is logged. This
    >> + should be used by application developers to learn which syscalls their
    >> + application needs without having to iterate through multiple test and
    >> + development cycles to build the list.
    >> +
    >> SECCOMP_RET_ALLOW:
    >> Results in the system call being executed.
    >>
    >> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
    >> index 0f238a4..67f72cd 100644
    >> --- a/include/uapi/linux/seccomp.h
    >> +++ b/include/uapi/linux/seccomp.h
    >> @@ -29,6 +29,7 @@
    >> #define SECCOMP_RET_TRAP 0x00030000U /* disallow and force a SIGSYS */
    >> #define SECCOMP_RET_ERRNO 0x00050000U /* returns an errno */
    >> #define SECCOMP_RET_TRACE 0x7ff00000U /* pass to a tracer or disallow */
    >> +#define SECCOMP_RET_LOG 0x7ffe0000U /* allow after logging */
    >
    > This adds to UAPI, so it'd be good to think for a moment about how
    > this would work on older kernels: right now, if someone tried to use
    > this RET_LOG on an old kernel, it'll get treated like RET_KILL. Is
    > this sane?

    It is not sane for userspace code to blindly attempt to use a new
    feature on an old kernel. One of the main motivations of the
    actions_avail sysctl is to allow userspace to be smart about what the
    current kernel supports.

    I'll be adding logic (requested by Paul) to libseccomp that checks this
    sysctl when SECOMP_RET_LOG is attempted to be used. Programs that don't
    use libseccomp will have to do something similar.

    >
    > I'm also trying to figure out if there is some other solution to this,
    > but they all involve tests against an otherwise RET_ALLOW case, which
    > I want to avoid. :)
    >
    > So, I think, for now, this looks good, but I'd prefer this be
    > 0x7ffc0000U, just to make sure we have not painted ourselves into a
    > numerical corner if we for some reason ever need to put something
    > between RET_ALLOW and RET_LOG.

    That makes sense. I'll do that in v3.
    >
    >> #define SECCOMP_RET_ALLOW 0x7fff0000U /* allow */
    >>
    >> /* Masks for the return value sections. */
    >> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
    >> index 548fb89..8627481 100644
    >> --- a/kernel/seccomp.c
    >> +++ b/kernel/seccomp.c
    >> @@ -650,6 +650,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
    >>
    >> return 0;
    >>
    >> + case SECCOMP_RET_LOG:
    >
    > Given my protective feelings about the RET_ALLOW case, can you make
    > this a fully separate case statement? I'd rather have RET_ALLOW be
    > distinctly separate.

    Sure! It actually has to be two different cases now that we're doing the
    hot path approach for SECCOMP_RET_ALLOW.

    Tyler

    >
    >> case SECCOMP_RET_ALLOW:
    >> seccomp_log(this_syscall, 0, action);
    >> return 0;
    >> @@ -934,6 +935,7 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
    >> #define SECCOMP_RET_TRAP_NAME "trap"
    >> #define SECCOMP_RET_ERRNO_NAME "errno"
    >> #define SECCOMP_RET_TRACE_NAME "trace"
    >> +#define SECCOMP_RET_LOG_NAME "log"
    >> #define SECCOMP_RET_ALLOW_NAME "allow"
    >>
    >> /* Largest strlen() of all action names */
    >> @@ -943,6 +945,7 @@ static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME " "
    >> SECCOMP_RET_TRAP_NAME " "
    >> SECCOMP_RET_ERRNO_NAME " "
    >> SECCOMP_RET_TRACE_NAME " "
    >> + SECCOMP_RET_LOG_NAME " "
    >> SECCOMP_RET_ALLOW_NAME;
    >>
    >> struct seccomp_action_name {
    >> @@ -955,6 +958,7 @@ static struct seccomp_action_name seccomp_action_names[] = {
    >> { SECCOMP_RET_TRAP, SECCOMP_RET_TRAP_NAME },
    >> { SECCOMP_RET_ERRNO, SECCOMP_RET_ERRNO_NAME },
    >> { SECCOMP_RET_TRACE, SECCOMP_RET_TRACE_NAME },
    >> + { SECCOMP_RET_LOG, SECCOMP_RET_LOG_NAME },
    >> { SECCOMP_RET_ALLOW, SECCOMP_RET_ALLOW_NAME },
    >> { }
    >> };
    >> --
    >> 2.7.4
    >>
    >
    > -Kees
    >


    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2017-02-11 01:19    [W:7.733 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site