Message-ID: <e651b9df-bf39-5dd3-1146-c9efbfd1c29d@canonical.com>
Date:   Fri, 4 Aug 2017 17:54:04 -0500
From:   Tyler Hicks <tyhicks@...onical.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Andy Lutomirski <luto@...capital.net>,
        Will Drewry <wad@...omium.org>,
        Paul Moore <paul@...l-moore.com>,
        Eric Paris <eparis@...hat.com>,
        John Crispin <john@...ozen.org>, linux-audit@...hat.com,
        LKML <linux-kernel@...r.kernel.org>,
        Linux API <linux-api@...r.kernel.org>
Subject: Re: [PATCH v5 3/6] seccomp: Filter flag to log all actions except
 SECCOMP_RET_ALLOW

On 08/03/2017 11:51 AM, Kees Cook wrote:
> On Fri, Jul 28, 2017 at 1:55 PM, Tyler Hicks <tyhicks@...onical.com> wrote:
>> Add a new filter flag, SECCOMP_FILTER_FLAG_LOG, that enables logging for
>> all actions except for SECCOMP_RET_ALLOW for the given filter.
>>
>> SECCOMP_RET_KILL actions are always logged, when "kill" is in the
>> actions_logged sysctl, and SECCOMP_RET_ALLOW actions are never logged,
>> regardless of this flag.
>>
>> This flag can be used to create noisy filters that result in all
>> non-allowed actions to be logged. A process may have one noisy filter,
>> which is loaded with this flag, as well as a quiet filter that's not
>> loaded with this flag. This allows for the actions in a set of filters
>> to be selectively conveyed to the admin.
>>
>> Since a system could have a large number of allocated seccomp_filter
>> structs, struct packing was taken in consideration. On 64 bit x86, the
>> new log member takes up one byte of an existing four byte hole in the
>> struct. On 32 bit x86, the new log member creates a new four byte hole
>> (unavoidable) and consumes one of those bytes.
> 
> Ah, good note about packing. I'll adjust my other patch to move into
> the hole in 64-bit.
> 
> FWIW, I would have asked to extract the filter-assignment logic into a
> separate patch (since it's a distinct logical piece), but since I've
> already stolen it for the kill-process series, that's all done now. ;)
> I think it's a good sign that both that and this need similar
> functionality, so thank you for implementing that!
> 
>> Unfortunately, the tests added for SECCOMP_FILTER_FLAG_LOG are not
>> capable of inspecting the audit log to verify that the actions taken in
>> the filter were logged.
> 
> Strictly speaking, the test could probably do some horrible stuff to
> access the kernel log, but yes, I'm fine with this just getting listed
> in the test TODO.
> 
>>
>> Signed-off-by: Tyler Hicks <tyhicks@...onical.com>
>> ---
>>
>> * Changes since v4:
>>   - This is a new patch that allows the application to have a say in what gets
>>     logged
>>
>>  include/linux/seccomp.h                       |  3 +-
>>  include/uapi/linux/seccomp.h                  |  1 +
>>  kernel/seccomp.c                              | 78 ++++++++++++++++-----------
>>  tools/testing/selftests/seccomp/seccomp_bpf.c | 66 +++++++++++++++++++++++
>>  4 files changed, 116 insertions(+), 32 deletions(-)
>>
>> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
>> index ecc296c..c8bef43 100644
>> --- a/include/linux/seccomp.h
>> +++ b/include/linux/seccomp.h
>> @@ -3,7 +3,8 @@
>>
>>  #include <uapi/linux/seccomp.h>
>>
>> -#define SECCOMP_FILTER_FLAG_MASK       (SECCOMP_FILTER_FLAG_TSYNC)
>> +#define SECCOMP_FILTER_FLAG_MASK       (SECCOMP_FILTER_FLAG_TSYNC | \
>> +                                        SECCOMP_FILTER_FLAG_LOG)
>>
>>  #ifdef CONFIG_SECCOMP
>>
>> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
>> index 0f238a4..82c823c 100644
>> --- a/include/uapi/linux/seccomp.h
>> +++ b/include/uapi/linux/seccomp.h
>> @@ -16,6 +16,7 @@
>>
>>  /* Valid flags for SECCOMP_SET_MODE_FILTER */
>>  #define SECCOMP_FILTER_FLAG_TSYNC      1
>> +#define SECCOMP_FILTER_FLAG_LOG                2
> 
> I can sort out the flag collision with ..._KILL_PROCESS.
> 
>>
>>  /*
>>   * All BPF programs must return a 32-bit value.
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index 87257f2..1c4c496 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -44,6 +44,7 @@
>>   *         get/put helpers should be used when accessing an instance
>>   *         outside of a lifetime-guarded section.  In general, this
>>   *         is only needed for handling filters shared across tasks.
>> + * @log: true if all actions except for SECCOMP_RET_ALLOW should be logged
>>   * @prev: points to a previously installed, or inherited, filter
>>   * @prog: the BPF program to evaluate
>>   *
>> @@ -59,6 +60,7 @@
>>   */
>>  struct seccomp_filter {
>>         refcount_t usage;
>> +       bool log;
>>         struct seccomp_filter *prev;
>>         struct bpf_prog *prog;
>>  };
>> @@ -172,11 +174,14 @@ static int seccomp_check_filter(struct sock_filter *filter, unsigned int flen)
>>
>>  /**
>>   * seccomp_run_filters - evaluates all seccomp filters against @sd
>> + * @filter: upon return, points to the matched filter but may be NULL in some
>> + *          unexpected situations
>>   * @sd: optional seccomp data to be passed to filters
>>   *
>>   * Returns valid seccomp BPF response codes.
>>   */
>> -static u32 seccomp_run_filters(const struct seccomp_data *sd)
>> +static u32 seccomp_run_filters(struct seccomp_filter **filter,
>> +                              const struct seccomp_data *sd)
>>  {
>>         struct seccomp_data sd_local;
>>         u32 ret = SECCOMP_RET_ALLOW;
>> @@ -184,6 +189,8 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd)
>>         struct seccomp_filter *f =
>>                         lockless_dereference(current->seccomp.filter);
>>
>> +       *filter = f;
>> +
>>         /* Ensure unexpected behavior doesn't result in failing open. */
>>         if (unlikely(WARN_ON(f == NULL)))
>>                 return SECCOMP_RET_KILL;
>> @@ -200,8 +207,10 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd)
>>         for (; f; f = f->prev) {
>>                 u32 cur_ret = BPF_PROG_RUN(f->prog, sd);
>>
>> -               if ((cur_ret & SECCOMP_RET_ACTION) < (ret & SECCOMP_RET_ACTION))
>> +               if ((cur_ret & SECCOMP_RET_ACTION) < (ret & SECCOMP_RET_ACTION)) {
>>                         ret = cur_ret;
>> +                       *filter = f;
>> +               }
>>         }
>>         return ret;
>>  }
>> @@ -446,6 +455,9 @@ static long seccomp_attach_filter(unsigned int flags,
>>                         return ret;
>>         }
>>
>> +       if (flags & SECCOMP_FILTER_FLAG_LOG)
>> +               filter->log = true;
>> +
>>         /*
>>          * If there is an existing filter, make it the prev and don't drop its
>>          * task reference.
>> @@ -526,36 +538,39 @@ static void seccomp_send_sigsys(int syscall, int reason)
>>  static u32 seccomp_actions_logged = SECCOMP_LOG_KILL  | SECCOMP_LOG_TRAP  |
>>                                     SECCOMP_LOG_ERRNO | SECCOMP_LOG_TRACE;
>>
>> -static inline void seccomp_log(unsigned long syscall, long signr, u32 action)
>> +static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
>> +                              bool requested)
>>  {
>> -       bool log;
>> +       if (requested) {
>> +               bool log;
>> +
>> +               switch (action) {
>> +               case SECCOMP_RET_TRAP:
>> +                       log = seccomp_actions_logged & SECCOMP_LOG_TRAP;
>> +                       break;
>> +               case SECCOMP_RET_ERRNO:
>> +                       log = seccomp_actions_logged & SECCOMP_LOG_ERRNO;
>> +                       break;
>> +               case SECCOMP_RET_TRACE:
>> +                       log = seccomp_actions_logged & SECCOMP_LOG_TRACE;
>> +                       break;
>> +               case SECCOMP_RET_ALLOW:
>> +                       log = false;
>> +                       break;
>> +               case SECCOMP_RET_KILL:
>> +               default:
>> +                       log = seccomp_actions_logged & SECCOMP_LOG_KILL;
>> +               }
>>
>> -       switch (action) {
>> -       case SECCOMP_RET_TRAP:
>> -               log = seccomp_actions_logged & SECCOMP_LOG_TRAP;
>> -               break;
>> -       case SECCOMP_RET_ERRNO:
>> -               log = seccomp_actions_logged & SECCOMP_LOG_ERRNO;
>> -               break;
>> -       case SECCOMP_RET_TRACE:
>> -               log = seccomp_actions_logged & SECCOMP_LOG_TRACE;
>> -               break;
>> -       case SECCOMP_RET_ALLOW:
>> -               log = false;
>> -               break;
>> -       case SECCOMP_RET_KILL:
>> -       default:
>> -               log = seccomp_actions_logged & SECCOMP_LOG_KILL;
>> +               /*
>> +                * Force an audit message to be emitted when the action is
>> +                * allowed to be logged by the admin.
>> +                */
>> +               if (log)
>> +                       return __audit_seccomp(syscall, signr, action);
>>         }
>>
>>         /*
>> -        * Force an audit message to be emitted when the action is allowed to
>> -        * be logged by the admin.
>> -        */
>> -       if (log)
>> -               return __audit_seccomp(syscall, signr, action);
>> -
>> -       /*
>>          * Let the audit subsystem decide if the action should be audited based
>>          * on whether the current task itself is being audited.
>>          */
>> @@ -587,7 +602,7 @@ static void __secure_computing_strict(int this_syscall)
>>  #ifdef SECCOMP_DEBUG
>>         dump_stack();
>>  #endif
>> -       seccomp_log(this_syscall, SIGKILL, SECCOMP_RET_KILL);
>> +       seccomp_log(this_syscall, SIGKILL, SECCOMP_RET_KILL, true);
> 
> I had to read these patches a few times to convince myself that the
> logic was correct for RET_KILL logging, etc. As it stands, I'd kind of
> like to rearrange the checks in seccomp_log() so the action stays at
> the top-level, so it's slightly easier to scan for the logic of how an
> action is logged. For example:
> 
>        bool log = false;
> 
>        switch (action) {
>        case SECCOMP_RET_ALLOW:
>                break;
>        case SECCOMP_RET_TRAP:
>                log = requested && seccomp_actions_logged & SECCOMP_LOG_TRAP;
>                break;
>        case SECCOMP_RET_ERRNO:
>                log = requested && seccomp_actions_logged & SECCOMP_LOG_ERRNO;
>                break;
>        case SECCOMP_RET_TRACE:
>                log = requested && seccomp_actions_logged & SECCOMP_LOG_TRACE;
>                break;       case SECCOMP_RET_KILL:
>        default:
>                log = seccomp_actions_logged & SECCOMP_LOG_KILL;
> 
> i.e. ALLOW and KILL are clear about how they're special. ALLOW is
> never logged, and KILL is only logged if admin wants it (which is the
> default sysctl value).

I will make these changes.

> 
> 
> 
>>         do_exit(SIGKILL);
>>  }
>>
>> @@ -613,6 +628,7 @@ void secure_computing_strict(int this_syscall)
>>  static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>>                             const bool recheck_after_trace)
>>  {
>> +       struct seccomp_filter *filter;
> 
> Out of paranoia I set this to NULL by default in the extracted
> filter-assignment patch.

Good idea. I went back and forth on if I should do that. :)

Tyler

> 
>>         u32 filter_ret, action;
>>         int data;
>>
>> @@ -622,7 +638,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>>          */
>>         rmb();
>>
>> -       filter_ret = seccomp_run_filters(sd);
>> +       filter_ret = seccomp_run_filters(&filter, sd);
>>         data = filter_ret & SECCOMP_RET_DATA;
>>         action = filter_ret & SECCOMP_RET_ACTION;
>>
>> @@ -690,7 +706,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>>
>>         case SECCOMP_RET_KILL:
>>         default:
>> -               seccomp_log(this_syscall, SIGSYS, action);
>> +               seccomp_log(this_syscall, SIGSYS, action, true);
>>                 /* Dump core only if this is the last remaining thread. */
>>                 if (get_nr_threads(current) == 1) {
>>                         siginfo_t info;
>> @@ -707,7 +723,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>>         unreachable();
>>
>>  skip:
>> -       seccomp_log(this_syscall, 0, action);
>> +       seccomp_log(this_syscall, 0, action, filter ? filter->log : false);
> 
> Yes, thanks for the double-check on the filter being non-NULL! :)
> 
>>         return -1;
>>  }
>>  #else
>> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
>> index 73f5ea6..eeb4f7a 100644
>> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
>> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
>> @@ -1687,6 +1687,10 @@ TEST_F_SIGNAL(TRACE_syscall, kill_after_ptrace, SIGSYS)
>>  #define SECCOMP_FILTER_FLAG_TSYNC 1
>>  #endif
>>
>> +#ifndef SECCOMP_FILTER_FLAG_LOG
>> +#define SECCOMP_FILTER_FLAG_LOG 2
>> +#endif
>> +
>>  #ifndef seccomp
>>  int seccomp(unsigned int op, unsigned int flags, void *args)
>>  {
>> @@ -2421,6 +2425,67 @@ TEST(syscall_restart)
>>                 _metadata->passed = 0;
>>  }
>>
>> +TEST_SIGNAL(filter_flag_log, SIGSYS)
>> +{
>> +       struct sock_filter allow_filter[] = {
>> +               BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
>> +       };
>> +       struct sock_filter kill_filter[] = {
>> +               BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
>> +                       offsetof(struct seccomp_data, nr)),
>> +               BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getpid, 0, 1),
>> +               BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL),
>> +               BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
>> +       };
>> +       struct sock_fprog allow_prog = {
>> +               .len = (unsigned short)ARRAY_SIZE(allow_filter),
>> +               .filter = allow_filter,
>> +       };
>> +       struct sock_fprog kill_prog = {
>> +               .len = (unsigned short)ARRAY_SIZE(kill_filter),
>> +               .filter = kill_filter,
>> +       };
>> +       long ret;
>> +       pid_t parent = getppid();
>> +
>> +       ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
>> +       ASSERT_EQ(0, ret);
>> +
>> +       /* Verify that the FILTER_FLAG_LOG flag isn't accepted in strict mode */
>> +       ret = seccomp(SECCOMP_SET_MODE_STRICT, SECCOMP_FILTER_FLAG_LOG,
>> +                     &allow_prog);
>> +       ASSERT_NE(ENOSYS, errno) {
>> +               TH_LOG("Kernel does not support seccomp syscall!");
>> +       }
>> +       EXPECT_NE(0, ret) {
>> +               TH_LOG("Kernel accepted FILTER_FLAG_LOG flag in strict mode!");
>> +       }
>> +       EXPECT_EQ(EINVAL, errno) {
>> +               TH_LOG("Kernel returned unexpected errno for FILTER_FLAG_LOG flag in strict mode!");
>> +       }
>> +
>> +       /* Verify that a simple, permissive filter can be added with no flags */
>> +       ret = seccomp(SECCOMP_SET_MODE_FILTER, 0, &allow_prog);
>> +       EXPECT_EQ(0, ret);
>> +
>> +       /* See if the same filter can be added with the FILTER_FLAG_LOG flag */
>> +       ret = seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_LOG,
>> +                     &allow_prog);
>> +       ASSERT_NE(EINVAL, errno) {
>> +               TH_LOG("Kernel does not support the FILTER_FLAG_LOG flag!");
>> +       }
>> +       EXPECT_EQ(0, ret);
>> +
>> +       /* Ensure that the kill filter works with the FILTER_FLAG_LOG flag */
>> +       ret = seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_LOG,
>> +                     &kill_prog);
>> +       EXPECT_EQ(0, ret);
>> +
>> +       EXPECT_EQ(parent, syscall(__NR_getppid));
>> +       /* getpid() should never return. */
>> +       EXPECT_EQ(0, syscall(__NR_getpid));
>> +}
>> +
>>  /*
>>   * TODO:
>>   * - add microbenchmarks
>> @@ -2429,6 +2494,7 @@ TEST(syscall_restart)
>>   * - endianness checking when appropriate
>>   * - 64-bit arg prodding
>>   * - arch value testing (x86 modes especially)
>> + * - verify that FILTER_FLAG_LOG filters generate log messages
>>   * - ...
>>   */
>>
>> --
>> 2.7.4
>>
> 
> Test looks good, thanks!
> 
> -Kees
> 




Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)

Powered by blists - more mailing lists