snap-{seccomp,confine}: rework seccomp denylist #12971
Conversation
When a denylist was introduced in PR#12849 we reached the limits of the API of libseccomp and some things are now hard to reason about [1]. This is mostly because what we try to do does not match the libseccomp API very well and a slightly different approach is probably more aligned with it's design (see also this libseccomp issue [2] that is related to our issue). So this commit changes the approach and instead of trying to use a single filter the work is split into two filters: 1. explicit allow list 2. explicit deny list and then load both filters into the kernel. The way the kernel works is that it selects the most restrictive action. So in the case of PR#12849: ``` ~ioctl - TIOCSTI ~ioctl - TIOCLINUX ioctl ``` For `ioctl(TIOCLINUX)` the first allow filter would pass `ioctl` but the second deny filter would correctly deny the TIOCLINUX. [1] canonical#12849 (comment) [2] seccomp/libseccomp#44
There was a problem hiding this comment.
This looks really good - thanks @mvo5 - my personal preference would be to name these as "snap.foo.allow.src" rather than "snap.foo.src.allow" but I can see that the way you have done it keeps the code churn to a minimum - but that is just a stylistic thing.
cmd/snap-confine/seccomp-support.c
Outdated
| sc_must_snprintf(profile_path, sizeof(profile_path), "%s/%s.bin", | ||
|
|
||
| struct sock_fprog prog_allow = { 0 }; | ||
| sc_must_snprintf(profile_path, sizeof(profile_path), "%s/%s.bin.allow", |
There was a problem hiding this comment.
would it be better to name these ".allow.bin" and ".deny.bin"? (that way we can still have the .src and .bin suffixes)
There was a problem hiding this comment.
While working on this I noticed that having two files means we are less atomic than before. So I changed the code to use a directory for the binary profiles (with renameat2(RENAME_EXCHANGE) and to called the filters inside now filter.{allow,deny} (but happy to change things as needed).
|
Some of the tests need adjusting as we are now denying with a different return code - I wonder if some of the code I wrote in #12849 could be removed now - in particular the changes to have implicit and explicit denial doesn't seem relevant anymore now that we have a specific seccomp denial filter. We can probably go back to just the previous "implicit" deny code. |
The generated seccomp bpf profile consists of two files now. This
makes it impossible to write them atomically. So instead move
them into a new place:
```
/var/lib/snapd/seccomp/bpf/$snapname/filter.{allow,deny}
```
that can be written in a tmpdir and then is replaced via
renameat2(RENAME_EXCHANGE).
The existing tests for negative filtering did not really test what we wanted them to test because the seccomp filtering in the kernel seems to be clever enough to see that e.g. "setpriority" was not part of the allow list so it never reached the explicit deny list. This commit fixes this and updates the tests to actually check for implicit/explicit denials.
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #12971 +/- ##
==========================================
+ Coverage 78.61% 78.65% +0.03%
==========================================
Files 999 999
Lines 123816 123930 +114
==========================================
+ Hits 97343 97475 +132
+ Misses 20333 20306 -27
- Partials 6140 6149 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Closing for now in favor of #13014 but keeping in case we decide to go back to the two files approach. |
[DRAFT FOR NOW BECAUSE CODE NEEDS CLEANUP AND I WANT TO SEE THE SPREAD OUTPUT]
[but concept should be sound]
When a denylist was introduced in PR#12849 we reached the limits of the API of libseccomp and some things are now hard to reason about [1]. This is mostly because what we try to do does not match the libseccomp API very well and a slightly different approach is probably more aligned with it's design (see also this libseccomp issue [2] that is related to our issue).
So this commit changes the approach and instead of trying to use a single filter the work is split into two filters:
and then load both filters into the kernel. The way the kernel works is that it selects the most restrictive action.
So in the case of PR#12849:
For
ioctl(TIOCLINUX)the first allow filter would passioctlbut the second deny filter would correctly deny the TIOCLINUX.[1] #12849 (comment)
[2] seccomp/libseccomp#44