Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: cannot add blacklist items to a whitelist #44

Closed
justincormack opened this issue Jul 11, 2016 · 16 comments
Closed

BUG: cannot add blacklist items to a whitelist #44

justincormack opened this issue Jul 11, 2016 · 16 comments
Assignees
Labels

Comments

@justincormack
Copy link
Contributor

For docker we ship a default seccomp profile that is a whitelist using libseccomp-golang see https://github.com/docker/docker/tree/master/profiles/seccomp

However it seems to be impossible to have a default action of ERRNO and then add an ERRNO rule to blacklist a particular pattern as we get a "requested action matches default action of filter" error message passed through from libseccomp.

We want to block some particular argument values of a particular syscall (see moby/moby#23893 ), so eg setsockopt(x, 0, 41, x) and setsockopt(x, 0, 96, x) and setsockopt(x, 41, 41, x) should be denied, while allowing any other values, but although this can easily be written with a socket filter directly, it does not seem to be possible to write with libseccomp.

I was wondering if perhaps it could accept rules with the same action as the default and construct the appropriate bpf.

This is also mentioned in this issue comment, although there is workaround in that case #27 (comment)

@pcmoore pcmoore changed the title Cannot add blacklist items to a whitelist BUG: default ERRNO(x) action blocks rules with ERRNO(y) Jul 11, 2016
@pcmoore pcmoore added the bug label Jul 11, 2016
@pcmoore pcmoore added this to the v2.4 milestone Jul 11, 2016
@pcmoore pcmoore self-assigned this Jul 11, 2016
@pcmoore pcmoore changed the title BUG: default ERRNO(x) action blocks rules with ERRNO(y) BUG: default SCMP_ACT_ERRNO(x) action blocks rules with SCMP_ACT_ERRNO(y) Jul 11, 2016
@pcmoore
Copy link
Member

pcmoore commented Jul 11, 2016

We don't want to allow rules with the same action as the default action (that doesn't make any sense), but we should take into account the errno value when making this decision. For example, we should allow a rule with ERRNO(y) when the default rule is ERRNO(x), but block the addition of a rule with ERRNO(x). Does that make sense?

We should also apply the same change to the TRACE(x) handling.

@justincormack
Copy link
Contributor Author

Hi, no that is not what my issue is, that is a different issue. My issue is that I cannot express the rules I want at all, I was just suggesting that perhaps one workaround would be to accept those rules and interpret them as a way of making the required bpf program, as the current format is not expressive enough for me to write the rules I want.

@justincormack justincormack changed the title BUG: default SCMP_ACT_ERRNO(x) action blocks rules with SCMP_ACT_ERRNO(y) Cannot add blacklist items to a whitelist Jul 11, 2016
@pcmoore pcmoore removed the bug label Jul 11, 2016
@pcmoore pcmoore removed this from the v2.4 milestone Jul 11, 2016
@pcmoore pcmoore changed the title Cannot add blacklist items to a whitelist BUG: cannot add blacklist items to a whitelist Jul 11, 2016
@pcmoore
Copy link
Member

pcmoore commented Jul 11, 2016

Okay, I guess I'm a bit confused by this section of your original problem report:

However it seems to be impossible to have a default action of ERRNO and then add an ERRNO rule to blacklist a particular pattern as we get a "requested action matches default action of filter" error message passed through from libseccomp.

Can you help clarify this for me?

@justincormack
Copy link
Contributor Author

Well I just thought one way of specifying a blacklist item with a whitelist might be to try to return the same return value as the default, ie errno, but obviously that doesnt work. What I want is:

default errno
if match1 then errno
if match2 then errno
otherwise allow

The actual bpf program wouldnt look quite like this, but the libseccomp format doesnt express jumps...

@pcmoore
Copy link
Member

pcmoore commented Jul 12, 2016

Okay, I think I understand what you are trying to do now and you can't do that, although to be honest you really shouldn't be doing that as there is a simpler approach. Taking your example above:

default errno
if match1 then errno
if match2 then errno
otherwise allow

NOTE: the "otherwise" is slightly ambiguous as it is unclear if it belongs to the second if-condition or the filter as a whole, I'm assuming the latter.

You define a default action of "errno", but in reality the default action is "allow" as your filter only triggers "errno" if "match1" or "match2" are true. I would suggest the following as functionally equivalent and libseccomp-legal alternative:

default allow
if match1 then errno
if match2 then errno

Does that make sense? Am I misunderstanding what you are trying to do?

@pcmoore
Copy link
Member

pcmoore commented Jul 12, 2016

Actually, it doesn't matter if the "otherwise" is for the second if-condition or the filter as a whole, the suggested filter should work just fine.

@justincormack
Copy link
Contributor Author

We can't have a default of allow as we are using a whitelist.

@pcmoore
Copy link
Member

pcmoore commented Jul 12, 2016

The example you provided isn't a whitelist.

@justincormack
Copy link
Contributor Author

The full profile is a whitelist, but I want to effectively blacklist a few specific patterns. If I was writing a filter program by hand I could do that with some conditions and jumps, but I cannot express that with libseccomp as far as I can see.

@pcmoore
Copy link
Member

pcmoore commented Jul 12, 2016

I'm still somewhat at a loss. If you have a whitelist and you want to exclude specific patterns you leave them out of the rules you explicitly add, it's that simple.

Are you basically looking for a runtime assert(3) API for the libseccomp filter?

Also, don't forget that you can install multiple filters for any given process; each filter will be executed with the most restrictive action being the winner. For example, if you had a whitelist where you wanted to allow "match2" and "match4" but you wanted to explicitly disallow "match1" and "match3" you could create two filters like the following:

# whitelist
default errno
if match2 then allow
if match4 then allow

# blacklist
default allow
if match1 then errno
if match3 then errno

Using the two filters above, you still have an effective blacklist as the most restrictive default is the "errno" from the whitelist with "match2" and "match4" being allowed as they are only mentioned in the whitelist and "match1" and "match3" being denied with "errno" as they are explicitly mentioned in the blacklist. My guess is this best fits what you are trying to do.

@justincormack
Copy link
Contributor Author

I cant "leave out" the rules from a rule that otherwise accepts every value, when they are of the form register1=a, register2=b and there are several of them. I could have stacked profiles, but that adds a lot of complexity.

I am trying to drop the values of setsockopt that cause an issue for a CVE, see moby/moby#23893

@pcmoore
Copy link
Member

pcmoore commented Jul 12, 2016

I'm not sure what you mean by "profiles", but the way to do what I think you are asking is with the two installed filters as described above.

@justincormack
Copy link
Contributor Author

yes, sorry, profiles=filters.

It is perfectly possible to do this natively with bpf with a single filter though, because it is fine to return with any return value anywhere in the filter. I know libseccomp is a simplification, but I don't see that it would be impossible to do the right code generation for this type of use case. Having two stacked filters makes our configuration much more complicated.

@pcmoore
Copy link
Member

pcmoore commented Jul 12, 2016

Yes, I'm well versed on what is possible with raw BPF, I've spent plenty of time looking at it the past few years. As you stated, providing all the functionality of raw BPF is not a goal of libseccomp (that would be silly); libseccomp is designed to make it easier for developers to use the seccomp mode 2 functionality by providing an ABI agnostic programmatic interface that doesn't suck too badly.

What you are asking for is currently handled via two filters in libseccomp, that is the recommended solution. We will likely make the library more expressive in the future, but the functionality you are asking for isn't likely to be introduced soon enough to satisfy your immediate need. As I stated before, I'm not too familiar with the Docker/libseccomp integration, but it seems to me that it tying a syscall filtering profile to a single libseccomp filter is not the best solution, especially given this idea of blocking specific syscall invocations in order to block CVEs/exploits. However, you know Docker/libseccomp better than I do.

Regardless, I don't think there is much we can do on the libseccomp end of things right now, beyond what I've mentioned above. I'll leave this issue open a bit longer in case you have something you would like to add.

@justincormack
Copy link
Contributor Author

It is not an immediate need, we don't really need that PR for other reasons. But that type of thing may well come up again in future. Multiple filters is possible we have an issue moby/moby#22109 but adds a large amount of complexity for configuration that we want to avoid if at all possible. I would rather work on improving expressivity myself, but if that is not in the roadmap that is fine.

@pcmoore
Copy link
Member

pcmoore commented Jul 12, 2016

@justincormack Patches are always welcome :)

mvo5 added a commit to mvo5/snappy that referenced this issue 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] snapcore#12849 (comment)
[2] seccomp/libseccomp#44
mvo5 added a commit to mvo5/snappy that referenced this issue 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] snapcore#12849 (comment)
[2] seccomp/libseccomp#44
mvo5 added a commit to mvo5/snappy that referenced this issue 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] snapcore#12849 (comment)
[2] seccomp/libseccomp#44
mvo5 added a commit to mvo5/snappy that referenced this issue 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] snapcore#12849 (comment)
[2] seccomp/libseccomp#44
mvo5 added a commit to mvo5/snappy that referenced this issue 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] snapcore#12849 (comment)
[2] seccomp/libseccomp#44
mvo5 added a commit to mvo5/snappy that referenced this issue 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] snapcore#12849 (comment)
[2] seccomp/libseccomp#44
mvo5 added a commit to mvo5/snappy that referenced this issue 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] snapcore#12849 (comment)
[2] seccomp/libseccomp#44
mvo5 added a commit to mvo5/snappy that referenced this issue 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] snapcore#12849 (comment)
[2] seccomp/libseccomp#44
mvo5 added a commit to mvo5/snappy that referenced this issue 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] snapcore#12849 (comment)
[2] seccomp/libseccomp#44
jslarraz pushed a commit to jslarraz/snapd that referenced this issue 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] snapcore#12849 (comment)
[2] seccomp/libseccomp#44
ernestl pushed a commit to snapcore/snapd that referenced this issue 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 <maciej.borzecki@canonical.com>

* 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 <maciej.borzecki@canonical.com>
Co-authored-by: Michael Vogt <mvo@ubuntu.com>
Co-authored-by: Maciej Borzecki <maciej.borzecki@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants