RFE: expose improved kernel logging features through libseccomp #92

tyhicks · 2017-08-25T04:38:02Z

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 · 2017-08-25T04:46:43Z

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 · 2017-08-25T04:48:45Z

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

coveralls · 2017-08-25T05:10:41Z

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

pcmoore

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.

pcmoore · 2017-09-12T22:59:51Z

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


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

pcmoore · 2017-09-12T23:03:03Z

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 */


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).

Right! Nice catch.

pcmoore · 2017-09-12T23:12:38Z

src/system.c

+
+ rc = syscall(_nr_seccomp, SECCOMP_GET_ACTION_AVAIL, 0, &action);
+ if (rc == -1) {
+ if (errno == EINVAL) {


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

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.

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

pcmoore · 2017-09-12T23:14:31Z

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.
+ */


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().

That's fine with me.

pcmoore · 2017-09-12T23:16:47Z

src/system.c

+int sys_chk_seccomp_action(uint32_t action)
+{
+ int rc;
+


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.

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?

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

pcmoore · 2017-09-13T16:52:14Z

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


See my previous comment about the golang bindings.

pcmoore · 2017-09-13T16:53:01Z

src/db.c

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


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

Yes, it does.

pcmoore · 2017-09-13T16:55:12Z

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;


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

pcmoore · 2017-09-13T17:04:03Z

doc/man/man3/seccomp_action_valid.3

+.B #include <seccomp.h>
+.sp
+.BI "int seccomp_action_valid(uint32_t " action ");"
+.sp


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.).

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.

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).

pcmoore · 2017-09-13T17:07:35Z

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.


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.

tyhicks · 2017-09-18T20:56:33Z

@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 · 2017-09-18T21:17:55Z

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

tyhicks · 2017-09-18T21:37:06Z

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 · 2017-09-18T22:57:15Z

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

coveralls · 2017-09-18T23:17:05Z

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

coveralls · 2017-09-19T03:07:24Z

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

coveralls · 2017-09-20T00:59:14Z

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

tyhicks · 2017-09-21T04:26:00Z

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

pcmoore · 2017-09-21T19:42:44Z

@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.

pcmoore · 2017-10-08T20:59:05Z

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 · 2017-10-08T21:00:10Z

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

tyhicks · 2017-10-09T04:35:48Z

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 · 2017-10-10T05:07:29Z

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 · 2017-10-11T04:14:15Z

@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 · 2017-10-11T23:18:24Z

@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 · 2017-10-18T06:50:40Z

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 · 2017-10-18T07:16:24Z

Coverage decreased (-0.1%) to 88.692% when pulling 24bb7b7 on tyhicks:improved-logging into a1c4094 on seccomp:master.

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 <[email protected]>

tyhicks · 2017-10-19T18:34:08Z

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 · 2017-10-19T18:59:46Z

Changes Unknown when pulling 49fcf6b on tyhicks:improved-logging into ** on seccomp:master**.

pcmoore · 2017-10-24T20:19:38Z

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)


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.

I agree. Nice catch!

pcmoore · 2017-10-24T20:20:47Z

src/api.c

- break;
+ case 3:
+ sys_set_seccomp_flag(SECCOMP_FILTER_FLAG_LOG, true);
+ /* fallthrough */


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

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

Huh, okay, sorry for the noise.

pcmoore · 2017-10-24T20:22:37Z

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);


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

Hmm... you're right.

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

pcmoore · 2017-10-24T21:16:51Z

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);


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

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

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

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 · 2017-10-24T21:29:07Z

@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.

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 <[email protected]>

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 <[email protected]>

Extend libseccomp to support SECCOMP_RET_LOG, which is intended to log the syscall before allowing it. Signed-off-by: Tyler Hicks <[email protected]>

The basics needed to handle tests that use the new SCMP_ACT_LOG action. Signed-off-by: Tyler Hicks <[email protected]>

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 <[email protected]>

Test SCMP_ACT_LOG as the default action which all syscalls trigger. Signed-off-by: Tyler Hicks <[email protected]>

coveralls · 2017-10-24T23:21:19Z

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

tyhicks · 2017-10-24T23:56:34Z

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

pcmoore · 2017-11-01T18:44:09Z

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 changed the title ~~Expose improved kernel logging features through libseccomp~~ RFE: expose improved kernel logging features through libseccomp Sep 9, 2017

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

pcmoore self-requested a review September 12, 2017 22:55

pcmoore requested changes Sep 13, 2017

View reviewed changes

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

tyhicks force-pushed the improved-logging branch from f9cb04b to 1e33ce4 Compare September 18, 2017 20:48

tyhicks force-pushed the improved-logging branch from 1e33ce4 to 4b4b0c5 Compare September 18, 2017 22:53

tyhicks force-pushed the improved-logging branch from 4b4b0c5 to 4544c8b Compare September 19, 2017 02:45

tyhicks force-pushed the improved-logging branch from 4544c8b to b36a546 Compare September 19, 2017 23:11

tyhicks mentioned this pull request Sep 21, 2017

expose improved kernel logging features through libseccomp-golang seccomp/libseccomp-golang#29

Closed

This was referenced Oct 3, 2017

golang: Add support for SCMP_ACT_LOG mvo5/libseccomp-golang#1

Merged

snap-confine, snap-seccomp: utilize new seccomp logging features canonical/snapd#3998

Merged

tyhicks force-pushed the improved-logging branch from b36a546 to 24bb7b7 Compare October 18, 2017 06:48

tyhicks force-pushed the improved-logging branch from 24bb7b7 to 49fcf6b Compare October 19, 2017 18:32

pcmoore reviewed Oct 24, 2017

View reviewed changes

tyhicks added 6 commits October 24, 2017 22:53

all: add support for new log action

a1fbec2

Extend libseccomp to support SECCOMP_RET_LOG, which is intended to log the syscall before allowing it. Signed-off-by: Tyler Hicks <[email protected]>

tests: test suite infrastructure changes for SCMP_ACT_LOG

01bce1b

The basics needed to handle tests that use the new SCMP_ACT_LOG action. Signed-off-by: Tyler Hicks <[email protected]>

tests: add test for SCMP_ACT_LOG of all syscalls

91558fb

Test SCMP_ACT_LOG as the default action which all syscalls trigger. Signed-off-by: Tyler Hicks <[email protected]>

tyhicks force-pushed the improved-logging branch from 49fcf6b to 91558fb Compare October 24, 2017 22:55

pcmoore closed this Nov 1, 2017

This comment was marked as spam.

Sign in to view

RFE: expose improved kernel logging features through libseccomp #92

RFE: expose improved kernel logging features through libseccomp #92

Conversation

tyhicks commented Aug 25, 2017 • edited Loading

tyhicks commented Aug 25, 2017

tyhicks commented Aug 25, 2017

coveralls commented Aug 25, 2017

pcmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyhicks commented Sep 18, 2017

coveralls commented Sep 18, 2017

tyhicks commented Sep 18, 2017

tyhicks commented Sep 18, 2017

coveralls commented Sep 18, 2017

coveralls commented Sep 19, 2017

coveralls commented Sep 20, 2017

tyhicks commented Sep 21, 2017

pcmoore commented Sep 21, 2017

pcmoore commented Oct 8, 2017

pcmoore commented Oct 8, 2017

tyhicks commented Oct 9, 2017 • edited Loading

tyhicks commented Oct 10, 2017

tyhicks commented Oct 11, 2017

pcmoore commented Oct 11, 2017

tyhicks commented Oct 18, 2017

coveralls commented Oct 18, 2017

tyhicks commented Oct 19, 2017

coveralls commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcmoore commented Oct 24, 2017

coveralls commented Oct 24, 2017

tyhicks commented Oct 24, 2017

pcmoore commented Nov 1, 2017

This comment was marked as spam.

tyhicks commented Aug 25, 2017 •

edited

Loading

tyhicks commented Oct 9, 2017 •

edited

Loading