a seccomp fix regarding ugetrlimit/prlimit64 by poettering · Pull Request #6952 · systemd/systemd

poettering · 2017-09-30T12:38:02Z

No description provided.

tixxdz

Patch 1 lgtm, small fixes for patch 2.

tixxdz · 2017-09-30T13:22:49Z

Did you let these inside @privileged set on purpose ? they should be removed from that set or at least have a way to import the @chown into @privileged and let the syscalls opencoded in one place. However, for backward compatibility, capabilities should cover this so removing them from @privileged should be ok.

P.S. will post patches to add logic to auto import a set into another one soon for the abstracted sandbox model.

you are right, @privileged should just import this group. will fix.

tixxdz · 2017-09-30T13:23:11Z

Maybe prefix this set with fs-sync or file-system-sync ?

dunno if that's necessary... This is in the context of syscalls after all, and in syscalls the name "sync" is clear enough i think.

poettering · 2017-10-02T07:19:12Z

I force pushed a new version now, that ports @privileged to use the new @chown as well as the already existing @reboot and @swap to make it a bit shorter, inspired by the suggestion @tixxdz made.

Please have another look!

tixxdz · 2017-10-02T08:44:32Z

lgtm, thank you!

keszybz · 2017-10-02T13:10:07Z

× xenial-amd64:

test_boot (__main__.NspawnTest) ... FAIL
test_service (__main__.NspawnTest) ... Job for systemd-nspawn@c1.service failed because the control process exited with error code.
See "systemctl  status systemd-nspawn@c1.service" and "journalctl  -xe" for details.
ERROR

xenial-i386 fails in a similar way.

The underlying reason seems to be trouble with starting a container:

TEST RUN: Basic systemd setup
W: can't run QEMU, skipping
Could not find a suitable QEMU binary
+ env --unset=UNIFIED_CGROUP_HIERARCHY systemd-nspawn --register=no --kill-signal=SIGKILL --directory=/var/tmp/systemd-test.syP2Dt/nspawn-root /lib/systemd/systemd
Spawning container nspawn-root on /var/tmp/systemd-test.syP2Dt/nspawn-root.
Press ^] three times within 1s to kill container.
E: nspawn failed with exit code 1

Hmm, other PRs don't seem to fail in this way. The failure could be related to changes in this PR. I'll restart CI.

poettering · 2017-10-02T15:36:05Z

CI failed again, with a similar issue. Not grokking it. The only effective difference in the seccomp lists is that ugetrlimit is added to nspawn's default whitelist. Which is a syscall that doesn't exist on x86-64. most likely there's something wrong with the handling of that missing syscall in libseccomp or so. I tried to figure out why that doesn#t fail here though... nothing really changed with handling of that syscall in libseccomp recently afaics (but i only had a cursory look).

i force pushed a new version now that conditions out them item, if libseccomp doesn't explicitly define a pseudo syscall nr for ugetrlimit. maybe that fixes it. otherwise i have no real idea.. i am puzzled. anyone?

poettering · 2017-10-02T16:18:34Z

@tixxdz btw, could you dissmiss your negative review, so that github is happy?

topimiettinen

Commit message: igetrlimit should be ugetrlimit.

keszybz · 2017-10-03T06:16:40Z

Bisect:

#6968 — at least i386 passes, amd64 is still building
#6969 — fails with a merge conflict
#6970 — fails with a merge conflict
this  — bad

keszybz · 2017-10-04T12:14:43Z

So...

seccomp: include prlimit64 and ugetrlimit in @default 78aa3a4

only amd64 fails with "upstream FAIL timed out"

seccomp: add three more seccomp groups 4698fd7

fails with a "merge conflict"

seccomp: port @PRIVILEGED to use @reboot + @swap 0d7aca7

fails with a "merge conflict"

this is a test for the CI – do not merge yet 48c77a7

fails on "boot-and-services" and "upstream", but only on amd64 and i386

It seems something is off, but we can't bisect because the CI is unreliable.

tixxdz · 2017-10-04T12:58:29Z

@keszybz

So...
seccomp: include prlimit64 and ugetrlimit in @default 78aa3a4
only amd64 fails with "upstream FAIL timed out"

so we need to check if it is defined before:
#ifdef __NR_ugetrlimit

seems only for i386 and arm.

tixxdz · 2017-10-04T12:59:53Z

Hmm, or maybe it is not related since we should be able to handle that from our seccomp logic... ?!

poettering · 2017-10-04T17:05:32Z

So, my educated guess is that libseccomp (at least in its debian/ubuntu version) gets confused by references to geturlimit on archs that don't have it.

I am tempted to add an ugly hack for it, to work around this. But first, let's see if @pcmoore has an idea about that? Are you aware of any recent issues with handling of ugetrlimit() in libseccomp? specifically, if we reference it on x86-64 even though it doesn't exist there?

Normally, libseccomp should of course isolate us from issues like that...

poettering · 2017-10-04T17:16:12Z

so, i force pushed a new, rebased version now. It also contains a hackish commit that skips adding geturlimit on archs that aren't arm of i386. If that fixes the tests for this everything points to a libseccomp issue

poettering · 2017-10-04T19:05:02Z

bummer. that didn't work either... i have no clue

poettering · 2017-10-04T19:16:35Z

So I split this PR into two bits now: this PR shall only be about the getrlimit/ugetrlimit/prlimit64 issue.

And there's now #6999 for the rest. Given that the new additions are kind unrelated to the ugetrlimit thing, let's do this in two PRs.

And now I am curious whether we can make the two PRs pass CI...

pcmoore · 2017-10-04T20:50:53Z

Without digging through all of the systemd seccomp code, can someone explain the problem beyond "ugetrlimit is acting funny on some ABIs"? Bonus points if you can show me the libseccomp calls ;)

evverx · 2017-10-04T22:03:45Z

libseccomp has started supporting preadv2 and pwrite2 since v2.3.2 was realeased, so adding them to @basic-io while using v2.3.1 doesn't sound like a very good idea.

I think the following patch should "fix" the issue:

--- a/src/shared/seccomp-util.c
+++ b/src/shared/seccomp-util.c
@@ -337,10 +337,8 @@ const SyscallFilterSet syscall_filter_sets[_SYSCALL_FILTER_SET_MAX] = {
                 "lseek\0"
                 "pread64\0"
                 "preadv\0"
-                "preadv2\0"
                 "pwrite64\0"
                 "pwritev\0"
-                "pwritev2\0"
                 "read\0"
                 "readv\0"
                 "write\0"

evverx · 2017-10-04T22:54:41Z

The patch above has just enabled me to successfully run nspawn.

By the way, I'm not sure I understand why almost all the errors in setup_seccomp are ignored. Is there any point in the filters that are partly added?

evverx · 2017-10-05T01:24:44Z

If ignoring errors is really needed, seccomp_add_syscall_filter_set should skip everything instead of giving up when the first error occurs.

diff --git a/src/shared/seccomp-util.c b/src/shared/seccomp-util.c
index 5f3d7d9..48e0af9 100644
--- a/src/shared/seccomp-util.c
+++ b/src/shared/seccomp-util.c
@@ -852,15 +852,12 @@ static int seccomp_add_syscall_filter_set(
                 char **exclude) {

         const char *sys;
-        int r;

         assert(seccomp);
         assert(set);

         NULSTR_FOREACH(sys, set->value) {
-                r = seccomp_add_syscall_filter_item(seccomp, sys, action, exclude);
-                if (r < 0)
-                        return r;
+                (void) seccomp_add_syscall_filter_item(seccomp, sys, action, exclude);
         }

         return 0;

poettering · 2017-10-05T09:06:14Z

By the way, I'm not sure I understand why almost all the errors in setup_seccomp are ignored. Is there any point in the filters that are partly added?

iirc libseccomp doesn't define clear error codes in some cases where we really want to ignore errors, specifically if we add a syscall to a filter that doesn't actually exist on the arch of the filter. and because we can't detect this case we instead check for cases where we know the error as relevant and something to propagate. We should really rework this one day when libseccomp documents error codes, so that we only eat up errors we really want to ignore

Also, move prlimit64() out of @resources. prlimit64() may be used both for getting and setting resource limits, and is implicitly called by glibc at various places, on some archs, the same was as getrlimit(). SImilar, igetrlimit() is an arch-specific replacement for getrlimit(), and hence should be whitelisted at the same place as getrlimit() and prlimit64(). Also see: https://lists.freedesktop.org/archives/systemd-devel/2017-September/039543.html

When a libseccomp implementation doesn't know a syscall yet, that's no reason for us to fail completely. Instead, debug log, and proceed. This hopefully fixes the preadv2/pwritev2 issues pointed out here: systemd#6952 (comment)

Unfortunately libseccomp doesn't return (nor document) clean error codes, hence until then only check for specific error codes that we propagate, but ignore (but debug log) all others. Do this at one more place, we are already doing that at all others.

…rule_add_exact() System calls might exist on some archs but not on others, or might be multiplexed but not on others. Ignore such errors when putting together a filter at this location like we already do it on all others.

poettering · 2017-10-05T09:30:39Z

OK, another try, incorporating what @evverx: we need to ignore more errors...

@pcmoore so, as it turns out the getrlimit thing was misleading and a false alarm. Sorry for the noise. That said, is there any chance we can get clean, documented error codes from libseccomp functions? i.e. what is returned in which cases? that would be great, so that we can make libseccomp failing fatal...

tixxdz · 2017-10-05T10:37:10Z

+                return r;
+        if (r < 0)
+                log_debug_errno(r, "Failed to restrict system call architectures, skipping: %m");
+


I am bit concerned about ignoring errors here, if users tried to explicitly set it then and we ignore other architectures then those calls can be used to bypass the native architecture. Maybe turn these debug messages into warnings ? at least users can act on that ?

yeah, it sucks, but there's little we can do until libseccomp reports proper errors back we can match against. There are cases we validly want to ignore errors and cases where we shouldn't. right now we propagate only EPERM/EACCESS as we know under which conditions those are generated. We should invert this all of course, but we can't as long as we don't have any error codes to check for.

Specifically, we want to at least ignore issues regarding:

syscall doesn't exist on some arch

syscall cannot matched on some arch (because multiplexed, think socket/socketcall)

and there are some more, but as long as we can't detect these properly we don't really have any realistic option than to ignore most, but propagate only the ones we know aren't OK.

btw, do note that this PR doesn't really change much in this regard, in just adjusts one utlier to match what all the other invocations of seccomp_load() do here. Reworking the error handling of our seccomp_load() calls certainly should be revisited but probably later on, and then for all cases at once.

evverx · 2017-10-05T10:51:14Z

OK, another try, incorporating what @evverx: we need to ignore more errors...

I think the new commits should be pushed in #6999, because preadv2 and pwritev2 are being added to @basic-io there. I'm sorry for mixing up two PRs.

poettering · 2017-10-05T10:53:53Z

nice! the CI completed!

evverx · 2017-10-05T12:24:02Z

that would be great, so that we can make libseccomp failing fatal...

That would be great, because it is currently impossible to predict what systemd might and might not filter and why.

pcmoore · 2017-10-06T20:48:12Z

@pcmoore so, as it turns out the getrlimit thing was misleading and a false alarm. Sorry for the noise.

No worries. Glad you got everything working, even more glad it wasn't my fault ;)

That said, is there any chance we can get clean, documented error codes from libseccomp functions? i.e. what is returned in which cases? that would be great, so that we can make libseccomp failing fatal...

It's on the todo list (I'm pretty sure there is a GH issue somewhere...), I've just got a lot of things on my plate at the moment.

topimiettinen · 2017-10-07T14:45:06Z

I opened a new issue 97 for libseccomp as the problem didn't seem to match existing issues.

thelounge-zz · 2018-04-05T18:06:32Z

still an issue on Fedora 27 and systemd-234-10.git5f8984e.fc27.x86_64

SystemCallFilter=~@clock @cpu-emulation @debug @keyring @module @mount @obsolete @raw-io @reboot @swap acct modify_ldt add_key adjtimex clock_adjtime delete_module fanotify_init finit_module get_mempolicy init_module io_destroy io_getevents iopl ioperm io_setup io_submit io_cancel kcmp kexec_load keyctl lookup_dcookie mbind migrate_pages mount move_pages open_by_handle_at perf_event_open pivot_root process_vm_readv process_vm_writev ptrace remap_file_pages request_key set_mempolicy swapoff swapon umount2 uselib vmsplice

as soon as you add @resources httpd no longer starts while this was not a problem until Fedora 26

Stack trace of thread 6856:
#0  0x00007f1511c33b40 __getrlimit (libc.so.6)
#1  0x00007f1511f03fc8 __pthread_initialize_minimal (libpthread.so.0)
#2  0x00007f1511f03619 _init (libpthread.so.0)
Apr  5 20:00:50 testserver systemd[1]: httpd.service: Service hold-off time over, scheduling restart.
Apr  5 20:00:50 testserver systemd[1]: Stopped Apache Webserver.
Apr  5 20:00:50 testserver systemd[1]: httpd.service: Start request repeated too quickly.
Apr  5 20:00:50 testserver systemd[1]: Failed to start Apache Webserver.
Apr  5 20:00:50 testserver systemd[1]: httpd.service: Unit entered failed state.
Apr  5 20:00:50 testserver systemd[1]: httpd.service: Failed with result 'signal'.

poettering added the seccomp label Sep 30, 2017

tixxdz suggested changes Sep 30, 2017

View reviewed changes

topimiettinen approved these changes Sep 30, 2017

View reviewed changes

poettering force-pushed the seccomp-getrlimit branch from 53148d0 to 697b2cc Compare October 2, 2017 07:18

poettering force-pushed the seccomp-getrlimit branch from 697b2cc to 48c77a7 Compare October 2, 2017 15:32

tixxdz approved these changes Oct 2, 2017

View reviewed changes

topimiettinen reviewed Oct 2, 2017

View reviewed changes

poettering mentioned this pull request Oct 2, 2017

[WIP] Sandbox: seccomp: add more basic syscall groups #6963

Closed

poettering force-pushed the seccomp-getrlimit branch from 48c77a7 to 3f719ab Compare October 4, 2017 17:15

poettering mentioned this pull request Oct 4, 2017

add three new syscall groups, and port @privileged to make use of more existing ones #6999

Merged

poettering force-pushed the seccomp-getrlimit branch from 3f719ab to 2e09a8f Compare October 4, 2017 19:15

poettering changed the title ~~a seccomp fix, and a seccomp extension~~ a seccomp fix regarding ugetrlimit/prlimit64 Oct 4, 2017

poettering added 4 commits October 5, 2017 11:27

poettering force-pushed the seccomp-getrlimit branch from 2e09a8f to 448ac52 Compare October 5, 2017 09:28

tixxdz reviewed Oct 5, 2017

View reviewed changes

keszybz added this to the v235 milestone Oct 5, 2017

keszybz merged commit be03929 into systemd:master Oct 5, 2017

keszybz mentioned this pull request Oct 5, 2017

NEWS: some nitpicking and bike-shedding #7006

Merged

topimiettinen mentioned this pull request Oct 7, 2017

RFE: more precisely defined error return values seccomp/libseccomp#97

Closed

thelounge-zz mentioned this pull request Dec 14, 2017

Linux 4.14.4: Fatal: ink_sys_control.cc:59: failed assertion setrlimit(MAGIC_CAST(which), &rl) >= 0 apache/trafficserver#2908

Closed

Conversation

poettering commented Sep 30, 2017

Uh oh!

tixxdz left a comment

Choose a reason for hiding this comment

Uh oh!

tixxdz Sep 30, 2017

Choose a reason for hiding this comment

Uh oh!

poettering Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

tixxdz Sep 30, 2017

Choose a reason for hiding this comment

Uh oh!

poettering Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

poettering commented Oct 2, 2017

Uh oh!

tixxdz commented Oct 2, 2017

Uh oh!

keszybz commented Oct 2, 2017

Uh oh!

poettering commented Oct 2, 2017

Uh oh!

poettering commented Oct 2, 2017

Uh oh!

topimiettinen left a comment

Choose a reason for hiding this comment

Uh oh!

keszybz commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keszybz commented Oct 4, 2017

Uh oh!

tixxdz commented Oct 4, 2017

Uh oh!

tixxdz commented Oct 4, 2017

Uh oh!

poettering commented Oct 4, 2017

Uh oh!

poettering commented Oct 4, 2017

Uh oh!

poettering commented Oct 4, 2017

Uh oh!

poettering commented Oct 4, 2017

Uh oh!

pcmoore commented Oct 4, 2017

Uh oh!

evverx commented Oct 4, 2017

Uh oh!

evverx commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evverx commented Oct 5, 2017

Uh oh!

poettering commented Oct 5, 2017

Uh oh!

poettering commented Oct 5, 2017

Uh oh!

tixxdz Oct 5, 2017

Choose a reason for hiding this comment

Uh oh!

poettering Oct 5, 2017

Choose a reason for hiding this comment

Uh oh!

poettering Oct 5, 2017

Choose a reason for hiding this comment

Uh oh!

evverx commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poettering commented Oct 5, 2017

Uh oh!

evverx commented Oct 5, 2017

Uh oh!

pcmoore commented Oct 6, 2017

Uh oh!

topimiettinen commented Oct 7, 2017

keszybz commented Oct 3, 2017 •

edited

Loading

evverx commented Oct 4, 2017 •

edited

Loading

evverx commented Oct 5, 2017 •

edited

Loading

thelounge-zz commented Apr 5, 2018 •

edited by filbranden

Loading