many: introduce seccomp denylist to block ioctl with TIOCLINUX to fix CVE-2023-1523 #12849
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
snap-seccomp has always implemented an allow-list approach to syscalls - such that the listed syscalls are allowed and any non-listed will get blocked. However, in the case where we want to disallow a syscall with particular arguments, it is only possible to block one instance of the sycall with a given argument. If a second similar rule is added, each rule effectively allows the other and so neither get disallowed as a result. So introduce the concept of explicitly denying system calls listed in the seccomp profile by prefixing them with a tilde (~). The seccomp action for these is then EACCES (since EPERM is the default for unmatched syscalls and seccomp doesn't allow to specify an action which is the same as the default). This then allows to specify to block various syscall argument combinations as expected, and so is used as the mechanism to fix CVE-2023-1523. Signed-off-by: Alex Murray <[email protected]>
Fixes CVE-2023-1523 Signed-off-by: Alex Murray <[email protected]>
Fixes CVE-2023-1523 Signed-off-by: Alex Murray <[email protected]>
Add a spread test which exercises the two tty injection PoCs for both CVE-2023-1523 and CVE-2019-7303 Signed-off-by: Alex Murray <[email protected]>
Signed-off-by: Alex Murray <[email protected]>
Signed-off-by: Alex Murray <[email protected]>
…stems and on ubuntu core
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 #12849 +/- ##
=======================================
Coverage 78.60% 78.61%
=======================================
Files 990 991 +1
Lines 122688 122768 +80
=======================================
+ Hits 96443 96508 +65
- Misses 20176 20187 +11
- Partials 6069 6073 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Thanks for all the fixups @mvo5 |
|
spread tests look good - thanks again @mvo5 for fixing the various deficiencies in my code (we really need a way to get spread runs on private github forks so that we can test the other arches / platforms more easily before brining these changes public...) |
valentindavid
approved these changes
May 26, 2023
mvo5
reviewed
May 26, 2023
| // fish out syscall | ||
| syscallName := tokens[0] | ||
| if strings.HasPrefix(syscallName, "~") { |
There was a problem hiding this comment.
After some playing and poking I think this is okay short term but I think we should replace it ASAP in a followup. The issue is that it seems like libseccomp will "optimize" rules away. As it is designed as an allow-list, the behavior of this:
~ioctl - TIOCSTI
~ioctl - TIOCLINUX
ioctl
is counter intuitive - it will generate an allow rule for "ioctl" and discard the "~ioctl" entirely:
echo -e '~ioctl - TIOCSTI\nioctl\n' > seccomp-profile && SNAP_SECCOMP_DEBUG=1 ./snap-seccomp compile seccomp-profile /tmp/xxx
#
# pseudo filter code start
#
# filter for arch x86_64 (3221225534)
if ($arch == 3221225534)
# filter for syscall "ioctl" (16) [priority: 65535]
if ($syscall == 16)
action ALLOW;
# default action
action ERRNO(1);
this only works as expected hen combined with another specific rule like:
~ioctl - TIOCSTI
~ioctl - TIOCLINUX
ioctl - !TIOCSTI
see
$ echo -e '~ioctl - TIOCSTI\n~ioctl - TIOCLINUX\nioctl - !TIOCSTI' > seccomp-profile && SNAP_SECCOMP_DEBUG=1 ./snap-seccomp compile seccomp-profile /tmp/xxx
#
# pseudo filter code start
#
# filter for arch x86_64 (3221225534)
if ($arch == 3221225534)
# filter for syscall "ioctl" (16) [priority: 65532]
if ($syscall == 16)
if ($a1.hi32 == 0)
if ($a1.lo32 == 21532)
action ERRNO(13);
if ($a1.lo32 == 21522)
action ERRNO(13);
else
action ALLOW;
else
action ALLOW;
# default action
action ERRNO(1);
There was a problem hiding this comment.
I think we probably want instead of "~" a new: ioctl - !TIOCSTI&&!TIOCLINUX
There was a problem hiding this comment.
Unfortunately there is no way to express this in the libseccomp API https://libseccomp.readthedocs.io/en/latest/man/man3/seccomp_rule_add.3/ that I am aware of - we can only do something like 'compare argument 1 is equal to this value' or 'compare argument 1 is greater than this value' - we can't do higher level operations like booleans etc - so I don't think that is possible with libseccomp. If we were to write the BPF filter by hand we could add this but that would be a significant departure from the current implementation.
|
So just to clarify - I think we want to tweak this a bit but it's also working correctly and protecting from TIOCLINUX so it's fine to go in but for long term maintainence I think we need something slightly different in the near future. |
mvo5
added a commit
to mvo5/snappy
that referenced
this pull request
Jul 10, 2023
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 of the way libseccomp is working. 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)
mvo5
added a commit
to mvo5/snappy
that referenced
this pull request
Jul 10, 2023
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 of the way libseccomp is working. 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)
mvo5
added a commit
to mvo5/snappy
that referenced
this pull request
Jul 10, 2023
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
mvo5
added a commit
to mvo5/snappy
that referenced
this pull request
Jul 10, 2023
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
mvo5
added a commit
to mvo5/snappy
that referenced
this pull request
Jul 10, 2023
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
mvo5
added a commit
to mvo5/snappy
that referenced
this pull request
Jul 10, 2023
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
mvo5
added a commit
to mvo5/snappy
that referenced
this pull request
Jul 10, 2023
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
mvo5
added a commit
to mvo5/snappy
that referenced
this pull request
Jul 28, 2023
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. The file format of the `snap.snap.app.bin` changes to `.bin2` and includes a proper header that would allow us to extend more easily in the future. The exiting tests for negative filtering got also updated so that the deny/allow is tested via different errno numbers to ensure that the expected filter denies the access. The `snap-seccomp` spread test now also runs on all ubuntu releases. This work will also allow us to remove the `global.bin` seccomp filter in a followup PR. [1] canonical#12849 (comment) [2] seccomp/libseccomp#44
mvo5
added a commit
to mvo5/snappy
that referenced
this pull request
Jul 31, 2023
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. The file format of the `snap.snap.app.bin` changes to `.bin2` and includes a proper header that would allow us to extend more easily in the future. The exiting tests for negative filtering got also updated so that the deny/allow is tested via different errno numbers to ensure that the expected filter denies the access. The `snap-seccomp` spread test now also runs on all ubuntu releases. This work will also allow us to remove the `global.bin` seccomp filter in a followup PR. [1] canonical#12849 (comment) [2] seccomp/libseccomp#44
mvo5
added a commit
to mvo5/snappy
that referenced
this pull request
Jul 31, 2023
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. The file format of the `snap.snap.app.bin` changes to `.bin2` and includes a proper header that would allow us to extend more easily in the future. The exiting tests for negative filtering got also updated so that the deny/allow is tested via different errno numbers to ensure that the expected filter denies the access. The `snap-seccomp` spread test now also runs on all ubuntu releases. This work will also allow us to remove the `global.bin` seccomp filter in a followup PR. [1] canonical#12849 (comment) [2] seccomp/libseccomp#44
mvo5
added a commit
to mvo5/snappy
that referenced
this pull request
Jul 31, 2023
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. The file format of the `snap.snap.app.bin` changes to `.bin2` and includes a proper header that would allow us to extend more easily in the future. The exiting tests for negative filtering got also updated so that the deny/allow is tested via different errno numbers to ensure that the expected filter denies the access. The `snap-seccomp` spread test now also runs on all ubuntu releases. This work will also allow us to remove the `global.bin` seccomp filter in a followup PR. [1] canonical#12849 (comment) [2] seccomp/libseccomp#44
jslarraz
pushed a commit
to jslarraz/snapd
that referenced
this pull request
Jan 2, 2024
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. The file format of the `snap.snap.app.bin` changes to `.bin2` and includes a proper header that would allow us to extend more easily in the future. The exiting tests for negative filtering got also updated so that the deny/allow is tested via different errno numbers to ensure that the expected filter denies the access. The `snap-seccomp` spread test now also runs on all ubuntu releases. This work will also allow us to remove the `global.bin` seccomp filter in a followup PR. [1] canonical#12849 (comment) [2] seccomp/libseccomp#44
ernestl
pushed a commit
that referenced
this pull request
Apr 10, 2024
…#13443) * snap-{seccomp,confine}: rework seccomp denylist 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. The file format of the `snap.snap.app.bin` changes to `.bin2` and includes a proper header that would allow us to extend more easily in the future. The exiting tests for negative filtering got also updated so that the deny/allow is tested via different errno numbers to ensure that the expected filter denies the access. The `snap-seccomp` spread test now also runs on all ubuntu releases. This work will also allow us to remove the `global.bin` seccomp filter in a followup PR. [1] #12849 (comment) [2] seccomp/libseccomp#44 * snap-confine: tweak sc_seccomp_file_header struct (thanks Philip!) * snap-confine: tweak struct init workaround in sc_apply_seccomp_profile_for_security_tag (thanks Philip) * snap-seccomp: remove outdated comment about big endian * snap-confine: rename sc_must_read_header_from_file->sc_must_read_and_validate_header_from_file * snap-seccomp: rework exportBPF() to not require a temp file Thanks to Valentin for the suggestion. Also reverts the change to the `install-store-laaaarge` tests because there is no need for space in /tmp anymore. * tests: improve messae in security-seccomp deny test * snap-confine: rename "struct stat buf" -> "struct stat stat_buf" * snap-confine: check that filer size if multiple of sock_filter Thanks to Valentin for the suggestion. Also adds a bunch of C unit tests to check that the code works correctly. Sadly C makes it hard to write this in a concise way so there is a bit of repetition. * snap-confine: extract must_read_and_validate_header_from_file_dies_with() helper * snap-confine: workaround bug in gcc from 14.04 The gcc (4.8.4) in 14.04 will not compile the following code: ``` struct sc_seccomp_file_header hdr = {0}; ``` and will error with: ``` snap-confine/seccomp-support.c: In function ‘sc_apply_seccomp_profile_for_security_tag’: snap-confine/seccomp-support.c:246:9: error: missing braces around initializer [-Werror=missing-braces] struct sc_seccomp_file_header hdr = {0}; ^ snap-confine/seccomp-support.c:246:9: error: (near initialization for ‘hdr.header’) [-Werror=missing-braces] ``` to workaround this a pragma is added. * snap-confine: check filters are not empty and keep read access to global.bin file * tests: add details field to security-profiles and snap-seccomp spread tests * snap-confine: move empty filter validation to sc_must_read_filter_from_file to avoid conflicts with classic snaps * snap-{seccomp,confine}: add tests for missing seccomp profile and explicit deny has precedence to to explicit allow * snap-confine: run make fmt * cmd/snap-confine: make fmt again with indent 2.2.13+ Signed-off-by: Maciej Borzecki <[email protected]> * snap-confine: Several code improvements * snap-confine: Fix format * snap-confine: Test fix and update deprecated SEEK_CUR * snap-confine: Fix test * snap-confine: Use inclusive language where possible * snap-confine: Make woke happy until we can remove cmd/snap-seccomp-blacklist --------- Signed-off-by: Maciej Borzecki <[email protected]> Co-authored-by: Michael Vogt <[email protected]> Co-authored-by: Maciej Borzecki <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update snap-seccomp to support explicitly denying certain syscall argument combinations via a new
~prefix - and use this to block the use ofioctlwithTIOCLINUX- to fix snapd CVE-2023-1523 (similar to flatpak CVE-2023-28100 GHSA-7qpw-3vjv-xrqp)