6:01 p.m.
On 02/07/2017 06:33 PM, Kees Cook wrote:
On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks
<tyhicks(a)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(a)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