From: Kees Cook <keescook@chromium.org>
To: Andy Lutomirski <luto@amacapital.net>
Cc: Tyler Hicks <tyhicks@canonical.com>,
Paul Moore <paul@paul-moore.com>, Eric Paris <eparis@redhat.com>,
Will Drewry <wad@chromium.org>,
linux-audit@redhat.com,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
John Crispin <john@phrozen.org>,
linux-api@ver.kernel.org
Subject: Re: [PATCH v3 0/4] Improved seccomp logging
Date: Wed, 22 Feb 2017 10:39:44 -0800 [thread overview]
Message-ID: <CAGXu5jJiK1CyM9M39F9nqpY1YJ90-St_gv6BYMzeUFejTDapPg@mail.gmail.com> (raw)
In-Reply-To: <CALCETrXzFDotNzqYmSoHwGoDUKidHVW+HKwJBRFFr+Z1Z=WbjQ@mail.gmail.com>
On Fri, Feb 17, 2017 at 9:00 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Thu, Feb 16, 2017 at 3:29 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Wed, Feb 15, 2017 at 7:24 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> If someone was going to do this, they could just as well set up a
>> tracer to use RET_TRAP. (And this is what things like minijail does
>> already, IIRC.) The reality of the situation is that this is way too
>> much overhead for the common case. We need a generalized logging
>> system that uses the existing logging mechanisms.
>
> True. And we can always add this part later if we want to.
>
> But let me propose a different, much more minor change to the patches:
>
> First, we currently have seccomp_run_filters running the whole stack
> and keeping (more or less) the lowest value. What if we changed it a
> bit so that return values of 0xff???????? were special. Specifically,
> a return value of 0xff?????? from a filter means "take some action
> right now but don't change the outcome of the filter stack". Then we
> define SECCOMP_RET_LOG as 0xff000000 and perhaps reserve a few bits to
> be a number reflected in the log entry. (e.g. SECCOMP_RET_LOG(x) =
> 0xff000000 | (x & 0xff)).
I'm not a fan of adding more logic to the filter-running loop, but
this idea is tempting.
> Now SECCOMP_RET_LOG or SECCOMP_RET_LOG(0) does approximately what it
> does in the current patch series if used in isolation, but you can
> install two filters, one of which logs and one of which kills, to get
> "log and kill".
>
> If we do this, we might want SECCOMP_RET_KILL to stop running filters
> so that filters farther up the stack don't log the syscall.
I don't want to change the semantics of filter execution for this, so
I'd prefer to avoid adding an early abort.
> What do you think? This should be a very small delta on top of the
> current patches.
What I don't like about this is that the logging and the action taken
are now totally separate. You could even end up having something
install multiple RET_LOGs, which aren't tied to what seccomp actually
decides to do in the end.
I'm not entirely opposed to the 0xff.... idea, but I don't think it
overlaps with logging very well. I would want seccomp logging to
reflect the actual action taken. Okay, I will now begin
stream-of-consciousness dumping to email...
I'm sure gmail will mangle whitespace, but here's sort of the 0xff
idea, well, actually 0x80....
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index 0f238a43ff1e..f61a0b783f6d 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -32,6 +32,7 @@
#define SECCOMP_RET_ALLOW 0x7fff0000U /* allow */
/* Masks for the return value sections. */
+#define SECCOMP_RET_OOB 0x80000000U
#define SECCOMP_RET_ACTION 0x7fff0000U
#define SECCOMP_RET_DATA 0x0000ffffU
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index f8f88ebcb3ba..4fac64fdfdc1 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -197,6 +197,9 @@ 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 (unlikely(cur_ret & SECCOMP_RET_OOB))
+ seccomp_oob_action(cur_ret);
+
if ((cur_ret & SECCOMP_RET_ACTION) < (ret & SECCOMP_RET_ACTION))
ret = cur_ret;
}
So, if SECCOMP_RET_OOB_LOG existed, it'd need to be installed as a
stand-alone filter, since it's still a BPF return value. That seems
... unfriendly ... as it would have to duplicate all the same logic as
another filter running along side it. e.g.:
filter 1 (actual actions):
- check arch
- check syscalls, jump to resolution
- ok: ret_allow
- unexpected: ret_allow
- weird: ret_errno
- bad: ret_kill
filter 2 ("oob" logging):
- check arch
- check syscalls, jump to resolution
- ok: ret_allow
- unexpected: ret_oob_log <- only difference
- weird: ret_errno
- bad: ret_kill
I mean, filter 2 could also just be:
- check arch
- check syscalls, jump to resolution
- unexpected: ret_oob_log
- everything else: ret_allow
But this kind of split logic seems needlessly complex and error-prone
on the filter developer's end. I don't think OOB logging should be
used here. Adding RET_LOG seems like the right approach to me.
The observations that it's per-process logging vs system-wide log
reporting is, I think, still problematic, but the case where a
developer is using RET_LOG is under non-production development and it
can be argued that the general case is developer==system owner, so
this is, again, sufficient.
So... I remain convinced that Tyler's series is the correct direction
to solve the bulk of the logging needs currently expressed by
developers and system owners.
-Kees
--
Kees Cook
Pixel Security
next prev parent reply other threads:[~2017-02-22 18:39 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-14 3:45 [PATCH v3 0/4] Improved seccomp logging Tyler Hicks
2017-02-14 3:45 ` [PATCH v3 1/4] seccomp: Add sysctl to display available actions Tyler Hicks
2017-02-16 1:00 ` Kees Cook
2017-02-16 3:14 ` Andy Lutomirski
2017-02-14 3:45 ` [PATCH v3 2/4] seccomp: Add sysctl to configure actions that should be logged Tyler Hicks
2017-02-14 3:45 ` [PATCH v3 3/4] seccomp: Create an action to log before allowing Tyler Hicks
2017-02-14 3:45 ` [PATCH v3 4/4] seccomp: Add tests for SECCOMP_RET_LOG Tyler Hicks
2017-02-16 3:24 ` [PATCH v3 0/4] Improved seccomp logging Andy Lutomirski
2017-02-16 23:29 ` Kees Cook
2017-02-17 17:00 ` Andy Lutomirski
2017-02-22 18:39 ` Kees Cook [this message]
[not found] ` <CAGXu5j+muyh2bwtMXDHuUHsDV9ZyEY-hMHrJjVuX2vC20MVSZw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-02-22 18:46 ` Kees Cook
[not found] ` <CAGXu5jLtLgYmDJDfGA2EtfB7Fqze-SP768ezq=fgWZ=X-ObW3w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-04-07 22:16 ` Tyler Hicks
[not found] ` <ac79529e-f6b6-690c-e597-5adeb75b0f25-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
2017-04-07 22:46 ` Kees Cook
[not found] ` <CAGXu5j+qUOpnDeF4TMH2AXXgHZB_WfHHfxe3TBSShmneisR-Lg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-04-07 23:46 ` Tyler Hicks
2017-04-11 3:59 ` Kees Cook
2017-04-27 22:17 ` Tyler Hicks
[not found] ` <0b1a2337-7006-e7cb-f519-dec389ede041-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
2017-04-27 23:42 ` Kees Cook
2017-05-02 2:41 ` Tyler Hicks
2017-05-02 16:14 ` Andy Lutomirski
2017-04-10 15:18 ` Steve Grubb
2017-04-10 15:57 ` Andy Lutomirski
[not found] ` <CALCETrXJKtnXmzRHs=7mEXN7FVAYjzxKb=jwrqwXQoXB0dHHPg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-04-10 19:22 ` Tyler Hicks
2017-04-11 4:03 ` Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAGXu5jJiK1CyM9M39F9nqpY1YJ90-St_gv6BYMzeUFejTDapPg@mail.gmail.com \
--to=keescook@chromium.org \
--cc=eparis@redhat.com \
--cc=john@phrozen.org \
--cc=linux-api@ver.kernel.org \
--cc=linux-audit@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@amacapital.net \
--cc=paul@paul-moore.com \
--cc=tyhicks@canonical.com \
--cc=wad@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).