Re: [PATCH v2 3/4] seccomp: Create an action to log before allowing

[Tyler Hicks <tyhicks@canonical.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6460 bytes --]

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
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]