/libseccomp

RFE: expose improved kernel logging features through libseccomp #92

Closed
wants to merge 7 commits into
from

Conversation

Reviewers

@pcmoore pcmoore

Assignees
No one assigned
Labels
enhancement pending/revision priority/medium
Projects
None yet
Milestone
No milestone
3 participants
@tyhicks @coveralls @pcmoore
@tyhicks
Contributor

tyhicks commented Aug 25, 2017 •
Edited 1 time

This pull request aims to provide expose the new seccomp logging functionality that's on its way into the upcoming 4.14 merge window (acked by Kees and in linux-next).

The main goal is to allow programs to use the new SECCOMP_FILTER_FLAG_LOG flag and SECCOMP_RET_LOG action to allow application developers more control over what seccomp actions are logged. To meet that goal, additional functionality had to be added to libseccomp to ensure that the new flag and action are only used when the currently running kernel supports them. This PR allows libseccomp to query the kernel to see if a given flag or action is supported. This is also important for the new SECCOMP_RET_KILL_PROCESS action that is also on its way into the upcoming 4.14 merge window.

A new libseccomp global attribute is created in order to allow applications, including the libseccomp test suite, to create filters containing a new action while running under an old kernel that doesn't support the new action. The attribute is global, rather than filter-specific, so that it can be set before seccomp_init(3) is called. While this functionality or something similar is necessary, it is a slight abuse of the existing seccomp_attr_set(3)/seccomp_attr_get(3) API resulting. That design excels in developer friendliness but lacks a bit in libseccomp code cleanliness while another discussed design is the exact opposite. The decision was made between Paul and myself to go with the former and that's what's present in this PR.

All the tests (C and Python) pass under "old" kernels as well as under a linux-next kernel containing the logging changes (linux-next tag next-20170824).
@tyhicks
Contributor

tyhicks commented Aug 25, 2017

I'll also mention that the method of filter flag validation used in 0fe4375 was added as a kernel selftest to ensure that the method of validation doesn't break in the future.
@tyhicks
Contributor

tyhicks commented Aug 25, 2017

It is best to review/merge #91 first so that the basic Python test changes in this PR are not skipped.
@coveralls

coveralls commented Aug 25, 2017

Coverage Status

Coverage decreased (-0.5%) to 88.276% when pulling f9cb04b on tyhicks:improved-logging into 9b01871 on seccomp:master.

@pcmoore pcmoore changed the title from Expose improved kernel logging features through libseccomp to RFE: expose improved kernel logging features through libseccomp Sep 9, 2017

@pcmoore pcmoore added enhancement pending/review priority/medium labels Sep 9, 2017

@pcmoore pcmoore self-requested a review Sep 12, 2017

@pcmoore

pcmoore requested changes Sep 13, 2017
View changes

A few minor things scattered across the commits, but I think my main concern is the API bits for detecting support; as we discussed over email I would prefer to see an "API level" interface as opposed to a piecemeal check/set API.

doc/man/man3/seccomp_action_valid.3
-kernel that's currently running.
+kernel that's currently running. This function is affected by the
+.BR SCMP_GLBATR_CTL_KCHECKACTS
+attribute which will disable the kernel support check for newer actions.
@pcmoore

pcmoore Sep 13, 2017

Contributor

This goes back to our email discussion about checking/setting a "API level" instead of checking/setting specific "things". I'm not saying "no" to this approach yet, but I would like to see an "API level" approach before making a call on this.

doc/man/man3/seccomp_action_valid.3
+.B #include <seccomp.h>
+.sp
+.BI "int seccomp_action_valid(uint32_t " action ");"
+.sp
@pcmoore

pcmoore Sep 13, 2017

Contributor

As we talked about over email, I'm not overly excited about public APIs like this; I would much prefer a "API level" check instead of having to check for specific things (e.g. actions, flags, etc.).

@tyhicks

tyhicks Sep 13, 2017

Contributor

Ok, lets see if we can find some time to chat about this at LSS over the next couple days.

IMO, this is really only needed for the libseccomp test suite (it could be handy for linking applications but definitely isn't necessary) so I hate to over-engineer it. On the other hand, I'm really not happy with the code that resulted from the global API approach as it feels pretty hackish to me.

@tyhicks

tyhicks Sep 15, 2017

Contributor

For anyone else following along, Paul and I were able to chat about this at LSS. He's planning to take a crack at the "API level" approach in the coming week or so and I'm going to rebase my SECCOMP_RET_LOG and SECCOMP_FILTER_FLAG_LOG changes on top of his patch(es).

include/seccomp.h.in
@@ -65,6 +65,7 @@ enum scmp_filter_attr {
SCMP_FLTATR_CTL_TSYNC = 4, /**< sync threads on filter load */
SCMP_FLTATR_API_TSKIP = 5, /**< allow rules with a -1 syscall */
SCMP_GLBATR_CTL_KCHECKACTS = 6, /**< ignore kernel support checks */
@pcmoore

pcmoore Sep 13, 2017

Contributor

I'm not sure if GitHub is messing with the commit ordering, but the SCMP_GLBATR_CTL_KCHECKACTS stuff shouldn't be here yet ...

@pcmoore

pcmoore Sep 13, 2017

Contributor

Ohh, GitHub is being sneaky/clever/stupid here ... I made the comment above in another commit and it is appearing here (presumably because this is the commit that introduces the code).

src/db.c
+ /* supported */
+ rc = 0;
+ col->attr.log_enable = (value ? 1 : 0);
+ } else if (rc == 0) {
@pcmoore

pcmoore Sep 13, 2017

Contributor

Another make check-syntax question, does it allow these curly braces?

@tyhicks

tyhicks Sep 13, 2017

Contributor

Yes, it does.

src/python/libseccomp.pxd
@@ -72,6 +72,7 @@ cdef extern from "seccomp.h":
cdef enum:
SCMP_ACT_KILL
SCMP_ACT_TRAP
+ SCMP_ACT_LOG
@pcmoore

pcmoore Sep 13, 2017

Contributor

Have you looked at the golang bindings?

@tyhicks

tyhicks Sep 13, 2017

Contributor

Yes and I will be submitting a PR against libseccomp-golang once the dust settles on this PR.

src/python/libseccomp.pxd
@@ -58,6 +58,7 @@ cdef extern from "seccomp.h":
SCMP_FLTATR_CTL_TSYNC
SCMP_FLTATR_API_TSKIP
SCMP_GLBATR_CTL_KCHECKACTS
+ SCMP_FLTATR_CTL_LOG
@pcmoore

pcmoore Sep 13, 2017

Contributor

See my previous comment about the golang bindings.

src/python/seccomp.pyx
@@ -30,6 +30,7 @@ by application developers.
Filter action values:
KILL - kill the process
+ LOG - allow the syscall to be execute after the action has been logged
@pcmoore

pcmoore Sep 13, 2017

Contributor

Take your pick, either "... syscall to be executed after ..." or "... syscall to execute after ..."

src/system.c
@@ -174,7 +174,7 @@ int sys_filter_load(const struct db_filter_col *col)
if (sys_chk_seccomp_syscall() == 1) {
int flgs = 0;
if (col->attr.tsync_enable)
- flgs = SECCOMP_FILTER_FLAG_TSYNC;
+ flgs |= SECCOMP_FILTER_FLAG_TSYNC;
@pcmoore

pcmoore Sep 13, 2017

Contributor

I would just merge this patch into the other patch, there really is no reason for this to be a standalone commit.

src/system.c
+ * otherwise.
+ */
+static int _sys_chk_seccomp_flag(int flag)
+{
@pcmoore

pcmoore Sep 13, 2017

Contributor

So why didn't you just add this to the existing sys_chk_seccomp_flag() function? I don't understand why we need/want this function.

@tyhicks

tyhicks Sep 13, 2017 •
Edited 1 time

Contributor

The code changed over time and the two functions were originally a bit more complicated than they ended up being. I agree that it now makes sense to merge them into the same function.

src/system.c
+ * If the action is supported one is returned, zero if unsupported, negative
+ * values on error. A return code of -EOPNOTSUPP indicates that the kernel is
+ * not new enough to support action queries.
+ */
@pcmoore

pcmoore Sep 13, 2017

Contributor

I'd rather just stick to a return value of 1 (supported) or 0 (not supported). I'm not convinced we care about the distinction between 0 and <0. See sys_chk_seccomp_syscall().

@tyhicks

tyhicks Sep 13, 2017

Contributor

That's fine with me.

src/system.c
+int sys_chk_seccomp_action(uint32_t action)
+{
+ int rc;
+
@pcmoore

pcmoore Sep 13, 2017

Contributor

I realize you aren't using it to check the existing actions, but for the sake of completeness it would be nice if this function was also valid for the existing actions.

@tyhicks

tyhicks Sep 13, 2017

Contributor

I agree and I believe that it is valid for the existing actions. What was it that makes you think it may not be valid for the existing actions?

@pcmoore

pcmoore Oct 24, 2017

Contributor

Looking at this again .... I have absolutely no idea, sorry about that.

src/system.c
+
+ rc = syscall(_nr_seccomp, SECCOMP_GET_ACTION_AVAIL, 0, &action);
+ if (rc == -1) {
+ if (errno == EINVAL) {
@pcmoore

pcmoore Sep 13, 2017

Contributor

Just curious, did you run make check-syntax? I'm not sure if it is going to flag these curly braces ...

@tyhicks

tyhicks Sep 13, 2017

Contributor

I didn't run it prior to submitting the PR (my bad!). However, I did just run it and the curly braces weren't flagged.

@tyhicks

tyhicks Sep 13, 2017

Contributor

In fact, the entire PR passed make check-syntax. Lucky me.

src/system.c
+ return 0;
+supported:
+ return 1;
+}
@pcmoore

pcmoore Sep 13, 2017

Contributor

The "supported" and "unsupported" jump labels seems a bit much, just do the returns directly in the code. We use the jump labels in sys_chk_seccomp_syscall() because we do more there than just return.

@tyhicks

tyhicks Sep 13, 2017

Contributor

Heh, agreed. This is another case of the flag checking functions becoming more simplified as I fine-tuned the patches prior to the PR. I should have fine-tuned a little bit more. :)

@pcmoore

pcmoore Oct 24, 2017

Contributor

See my comments in the later patches, but it seems like we need the ability to force the flag value, e.g. sys_set_seccomp_flag_kernel(...).

src/system.h
@@ -59,6 +59,7 @@ struct db_filter_col;
#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 0x7ffc0000U /* allow after logging */
@pcmoore

pcmoore Sep 13, 2017

Contributor

We need to worry about the case where the system passes the HAVE_LINUX_SECCOMP_H check but does not have the SECCOMP_RET_LOG macro defined (which will be most systems at the start).

@tyhicks

tyhicks Sep 13, 2017

Contributor

Right! Nice catch.

@pcmoore pcmoore added pending/revision and removed pending/review labels Sep 13, 2017

@tyhicks
Contributor

tyhicks commented Sep 18, 2017

@pcmoore I've made all of the changes there were requested, with the exception of the API level check that you planned to take a stab at, and pushed the new commits. The changes include:

  • src/python/seccomp.pyx: Fix documentation typo
  • src/system.c: Merge _sys_chk_seccomp_flag() into sys_chk_seccomp_flag()
    • Got rid of overly complicated unsupported and supported jump labels
  • src/system.c: Only return 0 (unsupported) or 1 (supported) from sys_chk_seccomp_flag()
  • src/system.h: Ensure SECCOMP_RET_LOG is defined even when the system header is available
@coveralls

coveralls commented Sep 18, 2017

Coverage Status

Coverage decreased (-0.5%) to 88.266% when pulling 1e33ce4 on tyhicks:improved-logging into 9e61fd7 on seccomp:master.
@tyhicks
Contributor

tyhicks commented Sep 18, 2017

I just noticed that I missed something. You asked for no negative values (only 0 for unsupported and 1 for supported) to be returned from sys_chk_seccomp_action() yet I only made that change for sys_check_seccomp_flag(). I assume that you want both functions to act similar to I'll retain the new behavior for sys_check_seccomp_flag() and push some new patches after I've updated sys_chk_seccomp_action().
@tyhicks
Contributor

tyhicks commented Sep 18, 2017

Alright, I've fixed up the return codes for sys_chk_seccomp_action().
@coveralls

coveralls commented Sep 18, 2017

Coverage Status

Coverage decreased (-0.4%) to 88.421% when pulling 4b4b0c5 on tyhicks:improved-logging into 9e61fd7 on seccomp:master.
@coveralls

coveralls commented Sep 19, 2017

Coverage Status

Coverage decreased (-0.4%) to 88.421% when pulling 4544c8b on tyhicks:improved-logging into 9e61fd7 on seccomp:master.
@coveralls

coveralls commented Sep 20, 2017

Coverage Status

Coverage decreased (-0.4%) to 88.421% when pulling b36a546 on tyhicks:improved-logging into 9e61fd7 on seccomp:master.

@tyhicks tyhicks referenced this pull request in seccomp/libseccomp-golang Sep 21, 2017

Open

expose improved kernel logging features through libseccomp-golang #29

@tyhicks
Contributor

tyhicks commented Sep 21, 2017

I've opened seccomp/libseccomp-golang#29 for the corresponding changes needed in libseccomp-golang.
@pcmoore
Contributor

pcmoore commented Sep 21, 2017

@tyhicks my apologies on the late replies, I've been trying to get caught up from last week and OSS/LSS.

I've made all of the changes there were requested, with the exception of the API level check that you planned to take a stab at ...

I put together a fairly crude patch to demonstrate what I was talking about, you can find it in the working-api_level branch. It's incomplete, untested, etc. but at least it compiles ;)

Unfortunately I haven't had a chance yet to look at the changes you've made.

You asked for no negative values (only 0 for unsupported and 1 for supported) to be returned from sys_chk_seccomp_action() yet I only made that change for sys_check_seccomp_flag(). I assume that you want both functions to act similar to I'll retain the new behavior for sys_check_seccomp_flag() ...

I need to go back and look at that code. I realized when I was doing the API level stuff that the negative values may be useful, e.g. specifying a flag/action value that doesn't exist. I apologize for the confusion.

This was referenced Oct 3, 2017

Merged
Open
@pcmoore
Contributor

pcmoore commented Oct 8, 2017

FYI, I finished the API level patch and merged it to master, can you rebase your PR on top of that so we can re-review it?
@pcmoore
Contributor

pcmoore commented Oct 8, 2017

Ooops, mean to include the commit ID for the API level commit, here it is: e89d182
@tyhicks
Contributor

tyhicks commented Oct 9, 2017 •
Edited 1 time

FYI, I finished the API level patch and merged it to master, can you rebase your PR on top of that so we can re-review it?

Thanks! Sure, I'll rebase, retest, and update this PR.

While rebasing, I was surprised by this:

+# NOTE: this is a NULL test since we don't support the seccomp_version() API
+#       via the libseccomp python bindings

What's the reasoning for that? The Python tests (and Python client code) will need the seccomp_api_get() and seccomp_api_set() API to be exposed.
@tyhicks
Contributor

tyhicks commented Oct 10, 2017

The Python tests (and Python client code) will need the seccomp_api_get() and seccomp_api_set() API to be exposed.

I've proposed a patch (2bd18be) that exposes the API level through the Python bindings in #98
@tyhicks
Contributor

tyhicks commented Oct 11, 2017

@pcmoore I've (locally) rebased the changes in this PR on top of the changes proposed in #98 in order to use the Python bindings work that I did in that PR. I guess I'll wait until you're able to review #98 before updating this PR but let me know if you want me to do something different.
@pcmoore
Contributor

pcmoore commented Oct 11, 2017

@tyhicks Thanks! I'm swamped this week working towards a Friday, October 13th deadline, but I'll revisit this early next week. I'm sorry for the delay, I appreciate your work and your patience.
@tyhicks
Contributor

tyhicks commented Oct 18, 2017

I've rebased onto the current state of #98, since we seem to be finalizing that PR, and I've updated this PR in the case that you are able to carve out enough time to review the rest of both PRs at once. I hope this doesn't complicate things...
@coveralls

coveralls commented Oct 18, 2017

Coverage Status

Coverage decreased (-0.1%) to 88.692% when pulling 24bb7b7 on tyhicks:improved-logging into a1c4094 on seccomp:master.
@tyhicks
system: runtime check if a filter flag is supported by the kernel 
As new filter flags are added to the kernel, libseccomp needs a way to
verify that a filter flag is not only valid but also supported by the
current kernel at runtime. A good way of doing that is by attempting to
enter filter mode, with the flag bit(s) in question set, and a NULL
pointer for the args parameter of seccomp(2). EFAULT indicates that the
flag is valid and EINVAL indicates that the flag is invalid. This patch
errs on the side of caution and treats any errno, besides EFAULT, as
indicating that the flag is invalid.

This check should be safe to use for the existing
SECCOMP_FILTER_FLAG_TSYNC flag so this patch enables the check for that
flag.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
5e4906f
@tyhicks
Contributor

tyhicks commented Oct 19, 2017

I did a quick rebase and push now that the API level fixups PR is merged. The commit hashes changed from what was in the PR to what was merged so a rebase of this PR was necessary.
@coveralls

coveralls commented Oct 19, 2017

Coverage Status

Changes Unknown when pulling 49fcf6b on tyhicks:improved-logging into ** on seccomp:master**.
src/api.c
@@ -102,6 +102,10 @@ static unsigned int _seccomp_api_update(void)
sys_chk_seccomp_flag(SECCOMP_FILTER_FLAG_TSYNC) == 1)
level = 2;
+ /* level 3 */
+ if (sys_chk_seccomp_flag(SECCOMP_FILTER_FLAG_LOG) == 1)
@pcmoore

pcmoore Oct 24, 2017

Contributor

The API level is a "everything and below" indicator, meaning that if we say we are operating at level 3 it means we support everything at level 3 and below. For that reason I think this check should be something like the following:

if ((level == 2) && (sys_chk_seccomp_flag(SECCOMP_FILTER_FLAG_LOG) == 1))

Granted, I think in most cases if FLAG_LOG is supported then everything else will be supported as well, but I would rather be safe.

@tyhicks

tyhicks Oct 24, 2017

Contributor

I agree. Nice catch!

src/api.c
- break;
+ case 3:
+ sys_set_seccomp_flag(SECCOMP_FILTER_FLAG_LOG, true);
+ /* fallthrough */
@pcmoore

pcmoore Oct 24, 2017

Contributor

Nitpicky, but since you're probably going to respin for the above comment, please indent this comment to the same level as the code :)

@tyhicks

tyhicks Oct 24, 2017

Contributor

This is actually what astyle requires. I originally had it indented like what you're asking for but astyle flagged it as incorrect.

@pcmoore

pcmoore Oct 24, 2017

Contributor

Huh, okay, sorry for the noise.

src/api.c
- break;
+ case 3:
+ sys_set_seccomp_flag(SECCOMP_FILTER_FLAG_LOG, true);
+ /* fallthrough */
case 2:
sys_set_seccomp_syscall(true);
sys_set_seccomp_flag(SECCOMP_FILTER_FLAG_TSYNC, true);
@pcmoore

pcmoore Oct 24, 2017

Contributor

It seems as though we should force off the FLAG_LOG functionality in the case of API levels 1 and 2, yes?

@tyhicks

tyhicks Oct 24, 2017

Contributor

Hmm... you're right.

That means that the odd indentation of "fallthrough" doesn't matter because we can't fall through any longer.

src/system.h
@@ -114,6 +117,8 @@ typedef struct sock_filter bpf_instr_raw;
int sys_chk_seccomp_syscall(void);
void sys_set_seccomp_syscall(bool enable);
+int sys_chk_seccomp_action(uint32_t action);
@pcmoore

pcmoore Oct 24, 2017

Contributor

I'm also wondering if we need a sys_set_seccomp_action(...) function.

@tyhicks

tyhicks Oct 24, 2017

Contributor

We do need one. It gets added in the all: add support for new log action patch.

@pcmoore

pcmoore Oct 24, 2017

Contributor

Ah ha, found it later in the patchset. It probably should be included here, but not the end of the world.

@tyhicks

tyhicks Oct 24, 2017

Contributor

It sounds like you're not requiring the patchset surgery needed to move sys_set_seccomp_action() up a patch in the series so I'm going to opt for leaving it where it is at rather than risk making a mistake since we seem to be close to merging this. Let me know if that's a problem.

@pcmoore
Contributor

pcmoore commented Oct 24, 2017

@tyhicks I just finished going through the latest patchset (comments inline), sort out those last few issues, or help me understand why I'm wrong, and I think we are all set.

tyhicks added some commits Oct 18, 2017

@tyhicks
all: add support for new log filter flag 
Extend libseccomp to support SECCOMP_FILTER_FLAG_LOG, which is intended
to cause log events for all actions taken by a filter except for
SCMP_ACT_ALLOW actions. This is done via a new filter attribute called
SCMP_FLTATR_CTL_LOG that is off by default.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
3ecc145
@tyhicks
system: runtime check if an action is supported by the kernel 
As new actions are added to the kernel, libseccomp needs a way to
verify that an action is not only valid but also supported by the
current kernel at runtime. The only way to do this is by using the
SECCOMP_GET_ACTION_AVAIL operation which was added to seccomp(2) in
kernel version 4.14.

This check is not enabled for existing actions supported by libseccomp
since those actions were present in kernels at the inception of
libseccomp.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
f9caff0
@tyhicks
all: add support for new log action 
Extend libseccomp to support SECCOMP_RET_LOG, which is intended to
log the syscall before allowing it.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
a1fbec2
@tyhicks
tests: test suite infrastructure changes for SCMP_ACT_LOG 
The basics needed to handle tests that use the new SCMP_ACT_LOG
action.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
01bce1b
@tyhicks
tests: add SCMP_ACT_LOG test to 06-sim-actions 
Extend the 06-sim-actions set of tests to include tests for
SCMP_ACT_LOG. The CTL_KCHECKACTS global attribute must be set to prevent
test errors when running under an old kernel that doesn't support
SECCOMP_RET_LOG.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
9ae3954
@tyhicks
tests: add test for SCMP_ACT_LOG of all syscalls 
Test SCMP_ACT_LOG as the default action which all syscalls trigger.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>

Some checks were not successful

1 failing and 1 successful checks
coverage/coveralls — Coverage decreased (-0.2%) to 88.731%
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
91558fb
@coveralls

coveralls commented Oct 24, 2017

Coverage Status

Coverage decreased (-0.2%) to 88.731% when pulling 91558fb on tyhicks:improved-logging into 4f16fe2 on seccomp:master.
@tyhicks
Contributor

tyhicks commented Oct 24, 2017

@pcmoore Thanks! I think your feedback is addressed in the latest push.
@pcmoore
Contributor

pcmoore commented Nov 1, 2017

Everything should now be merged (commit IDs below), thanks again @tyhicks!

0cc7203 tests: add test for SCMP_ACT_LOG of all syscalls
649d4fa tests: add SCMP_ACT_LOG test to 06-sim-actions
8f37ea8 tests: test suite infrastructure changes for SCMP_ACT_LOG
3b22b15 all: add support for new log action
b61042b system: runtime check if an action is supported by the kernel
d0e1195 all: add support for new log filter flag
8a8576c system: runtime check if a filter flag is supported by the kernel

@pcmoore pcmoore closed this Nov 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment