From d07e46090186d04365229e85a31cb959260e7f76 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 10 Jul 2023 13:59:33 +0200 Subject: [PATCH 01/10] 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. [1] https://github.com/snapcore/snapd/pull/12849#discussion_r1206855700 [2] https://github.com/seccomp/libseccomp/issues/44 --- cmd/snap-confine/seccomp-support.c | 42 ++++++++++--- cmd/snap-confine/seccomp-support.h | 4 ++ cmd/snap-seccomp/main.go | 94 +++++++++++++++++++++--------- cmd/snap-seccomp/main_test.go | 67 ++++++++++++++++----- interfaces/seccomp/template.go | 9 +-- tests/main/snap-seccomp/task.yaml | 44 +++++++------- 6 files changed, 179 insertions(+), 81 deletions(-) diff --git a/cmd/snap-confine/seccomp-support.c b/cmd/snap-confine/seccomp-support.c index 7432d79cb15..9c12c142f33 100644 --- a/cmd/snap-confine/seccomp-support.c +++ b/cmd/snap-confine/seccomp-support.c @@ -109,13 +109,38 @@ static void validate_bpfpath_is_safe(const char *path) } } -bool sc_apply_seccomp_profile_for_security_tag(const char *security_tag) -{ +bool sc_apply_seccomp_profile_for_security_tag(const char *security_tag) { debug("loading bpf program for security tag %s", security_tag); char profile_path[PATH_MAX] = { 0 }; - 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", + filter_profile_dir, security_tag); + if (!sc_load_seccomp_profile_path(profile_path, &prog_allow)) { + return false; + } + + struct sock_fprog prog_deny = { 0 }; + sc_must_snprintf(profile_path, sizeof(profile_path), "%s/%s.bin.deny", filter_profile_dir, security_tag); + if (!sc_load_seccomp_profile_path(profile_path, &prog_deny)) { + return false; + } + + sc_apply_seccomp_filter(&prog_deny); + sc_apply_seccomp_filter(&prog_allow); + + // XXX: this is not ideal, allocation happens in + // sc_load_seccomp_profile but free here :( + free(prog_allow.filter); + free(prog_deny.filter); + return true; +} + +bool sc_load_seccomp_profile_path(const char *profile_path, struct sock_fprog *prog) +{ + debug("loading bpf program from file %s", profile_path); // Wait some time for the security profile to show up. When // the system boots snapd will created security profiles, but @@ -165,11 +190,12 @@ bool sc_apply_seccomp_profile_for_security_tag(const char *security_tag) if (sc_streq(bpf, "@unrestricted\n")) { return false; } - struct sock_fprog prog = { - .len = num_read / sizeof(struct sock_filter), - .filter = (struct sock_filter *)bpf, - }; - sc_apply_seccomp_filter(&prog); + prog->len = num_read / sizeof(struct sock_filter); + prog->filter = (struct sock_filter *)malloc(num_read); + if (prog->filter == NULL) { + die("cannot allocate %li bytes of memory for seccomp filter ", num_read); + } + memcpy(prog->filter, bpf, num_read); return true; } diff --git a/cmd/snap-confine/seccomp-support.h b/cmd/snap-confine/seccomp-support.h index 9ed9b0aaaab..b387913e471 100644 --- a/cmd/snap-confine/seccomp-support.h +++ b/cmd/snap-confine/seccomp-support.h @@ -19,6 +19,10 @@ #include +#include + +bool sc_load_seccomp_profile_path(const char *profile_path, struct sock_fprog *prog); + /** * sc_apply_seccomp_profile_for_security_tag applies a seccomp profile to the * current process. The filter is loaded from a pre-compiled bpf bytecode diff --git a/cmd/snap-seccomp/main.go b/cmd/snap-seccomp/main.go index 4ff3799b0a1..b0df5941ddb 100644 --- a/cmd/snap-seccomp/main.go +++ b/cmd/snap-seccomp/main.go @@ -583,11 +583,12 @@ var ( errnoOnImplicitDenial int16 = C.EPERM ) -func parseLine(line string, secFilter *seccomp.ScmpFilter) error { +func parseLine(line string, secFilterAllow, secFilterDeny *seccomp.ScmpFilter) error { // ignore comments and empty lines if strings.HasPrefix(line, "#") || line == "" { return nil } + secFilter := secFilterAllow // regular line tokens := strings.Fields(line) @@ -604,6 +605,7 @@ func parseLine(line string, secFilter *seccomp.ScmpFilter) error { if strings.HasPrefix(syscallName, "~") { action = seccomp.ActErrno.SetReturnCode(errnoOnExplicitDenial) syscallName = syscallName[1:] + secFilter = secFilterDeny } secSyscall, err := seccomp.GetSyscallFromName(syscallName) @@ -789,27 +791,44 @@ func complainAction() seccomp.ScmpAction { return seccomp.ActAllow } +func newComplainFilter(complainAct seccomp.ScmpAction) (*seccomp.ScmpFilter, error) { + secFilter, err := seccomp.NewFilter(complainAct) + if err != nil { + if complainAct != seccomp.ActAllow { + // ActLog is only supported in newer versions + // of the kernel, libseccomp, and + // libseccomp-golang. Attempt to fall back to + // ActAllow before erroring out. + complainAct = seccomp.ActAllow + secFilter, err = seccomp.NewFilter(complainAct) + } + } + return secFilter, err +} + func compile(content []byte, out string) error { var err error - var secFilter *seccomp.ScmpFilter + var secFilterAllow, secFilterDeny *seccomp.ScmpFilter unrestricted, complain := preprocess(content) switch { case unrestricted: - return osutil.AtomicWrite(out, bytes.NewBufferString("@unrestricted\n"), 0644, 0) + for _, ext := range []string{".allow", ".deny"} { + if err := osutil.AtomicWrite(out+ext, bytes.NewBufferString("@unrestricted\n"), 0644, 0); err != nil { + return err + } + } + return nil case complain: var complainAct seccomp.ScmpAction = complainAction() - secFilter, err = seccomp.NewFilter(complainAct) + secFilterAllow, err = newComplainFilter(complainAct) if err != nil { - if complainAct != seccomp.ActAllow { - // ActLog is only supported in newer versions - // of the kernel, libseccomp, and - // libseccomp-golang. Attempt to fall back to - // ActAllow before erroring out. - complainAct = seccomp.ActAllow - secFilter, err = seccomp.NewFilter(complainAct) - } + return fmt.Errorf("cannot create seccomp filter: %s", err) + } + secFilterDeny, err = newComplainFilter(complainAct) + if err != nil { + return fmt.Errorf("cannot create seccomp filter: %s", err) } // Set unrestricted to 'true' to fallback to the pre-ActLog @@ -819,19 +838,26 @@ func compile(content []byte, out string) error { unrestricted = true } default: - secFilter, err = seccomp.NewFilter(seccomp.ActErrno.SetReturnCode(errnoOnImplicitDenial)) + secFilterAllow, err = seccomp.NewFilter(seccomp.ActErrno.SetReturnCode(errnoOnImplicitDenial)) + if err != nil { + return fmt.Errorf("cannot create seccomp filter: %s", err) + } + secFilterDeny, err = seccomp.NewFilter(seccomp.ActAllow) + if err != nil { + return fmt.Errorf("cannot create seccomp filter: %s", err) + } } - if err != nil { - return fmt.Errorf("cannot create seccomp filter: %s", err) + if err := addSecondaryArches(secFilterAllow); err != nil { + return err } - if err := addSecondaryArches(secFilter); err != nil { + if err := addSecondaryArches(secFilterDeny); err != nil { return err } if !unrestricted { scanner := bufio.NewScanner(bytes.NewBuffer(content)) for scanner.Scan() { - if err := parseLine(scanner.Text(), secFilter); err != nil { + if err := parseLine(scanner.Text(), secFilterAllow, secFilterDeny); err != nil { return fmt.Errorf("cannot parse line: %s", err) } } @@ -841,21 +867,33 @@ func compile(content []byte, out string) error { } if osutil.GetenvBool("SNAP_SECCOMP_DEBUG") { - secFilter.ExportPFC(os.Stdout) + secFilterAllow.ExportPFC(os.Stdout) + secFilterDeny.ExportPFC(os.Stdout) } // write atomically - fout, err := osutil.NewAtomicFile(out, 0644, 0, osutil.NoChown, osutil.NoChown) - if err != nil { - return err - } - // Cancel once Committed is a NOP - defer fout.Cancel() - - if err := secFilter.ExportBPF(fout.File); err != nil { - return err + for _, ext := range []string{".allow", ".deny"} { + fout, err := osutil.NewAtomicFile(out+ext, 0644, 0, osutil.NoChown, osutil.NoChown) + if err != nil { + return err + } + // Cancel once Committed is a NOP + defer fout.Cancel() + + switch ext { + case ".allow": + err = secFilterAllow.ExportBPF(fout.File) + case ".deny": + err = secFilterDeny.ExportBPF(fout.File) + } + if err != nil { + return err + } + if err := fout.Commit(); err != nil { + return err + } } - return fout.Commit() + return nil } // caches for uid and gid lookups diff --git a/cmd/snap-seccomp/main_test.go b/cmd/snap-seccomp/main_test.go index 38db6fa16e8..b61e7a67769 100644 --- a/cmd/snap-seccomp/main_test.go +++ b/cmd/snap-seccomp/main_test.go @@ -71,18 +71,45 @@ var seccompBpfLoaderContent = []byte(` #define MAX_BPF_SIZE 32 * 1024 -int sc_apply_seccomp_bpf(const char* profile_path) +int sc_apply_seccomp_bpf(const char* profile_path_allow, const char* profile_path_deny) { - unsigned char bpf[MAX_BPF_SIZE + 1]; // account for EOF + unsigned char bpf_allow[MAX_BPF_SIZE + 1]; // account for EOF + unsigned char bpf_deny[MAX_BPF_SIZE + 1]; // account for EOF FILE* fp; - fp = fopen(profile_path, "rb"); + + // allow + fp = fopen(profile_path_allow, "rb"); + if (fp == NULL) { + fprintf(stderr, "cannot read %s\n", profile_path_allow); + return -1; + } + + // set 'size' to 1; to get bytes transferred + size_t num_read = fread(bpf_allow, 1, sizeof(bpf_allow), fp); + + if (ferror(fp) != 0) { + perror("fread()"); + return -1; + } else if (feof(fp) == 0) { + fprintf(stderr, "file too big\n"); + return -1; + } + fclose(fp); + + struct sock_fprog prog_allow = { + .len = num_read / sizeof(struct sock_filter), + .filter = (struct sock_filter*)bpf_allow, + }; + + // deny + fp = fopen(profile_path_deny, "rb"); if (fp == NULL) { - fprintf(stderr, "cannot read %s\n", profile_path); + fprintf(stderr, "cannot read %s\n", profile_path_deny); return -1; } // set 'size' to 1; to get bytes transferred - size_t num_read = fread(bpf, 1, sizeof(bpf), fp); + num_read = fread(bpf_deny, 1, sizeof(bpf_deny), fp); if (ferror(fp) != 0) { perror("fread()"); @@ -93,9 +120,9 @@ int sc_apply_seccomp_bpf(const char* profile_path) } fclose(fp); - struct sock_fprog prog = { + struct sock_fprog prog_deny = { .len = num_read / sizeof(struct sock_filter), - .filter = (struct sock_filter*)bpf, + .filter = (struct sock_filter*)bpf_deny, }; // Set NNP to allow loading seccomp policy into the kernel without @@ -105,10 +132,15 @@ int sc_apply_seccomp_bpf(const char* profile_path) return -1; } - if (prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog)) { - perror("prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, ...) failed"); + if (prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog_deny)) { + perror("prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, ...) deny failed"); return -1; } + if (prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog_allow)) { + perror("prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, ...) allow failed"); + return -1; + } + return 0; } @@ -116,15 +148,16 @@ int main(int argc, char* argv[]) { int rc = 0; if (argc < 2) { - fprintf(stderr, "Usage: %s [prog ...]\n", argv[0]); + fprintf(stderr, "Usage: %s [prog ...]\n", argv[0]); return 1; } - rc = sc_apply_seccomp_bpf(argv[1]); + // allow, deny + rc = sc_apply_seccomp_bpf(argv[1], argv[2]); if (rc != 0) return -rc; - execv(argv[2], (char* const*)&argv[2]); + execv(argv[3], (char* const*)&argv[3]); perror("execv failed"); return 1; } @@ -333,7 +366,7 @@ mprotect } } - cmd := exec.Command(s.seccompBpfLoader, bpfPath, syscallRunner, syscallRunnerArgs[0], syscallRunnerArgs[1], syscallRunnerArgs[2], syscallRunnerArgs[3], syscallRunnerArgs[4], syscallRunnerArgs[5], syscallRunnerArgs[6]) + cmd := exec.Command(s.seccompBpfLoader, bpfPath+".allow", bpfPath+".deny", syscallRunner, syscallRunnerArgs[0], syscallRunnerArgs[1], syscallRunnerArgs[2], syscallRunnerArgs[3], syscallRunnerArgs[4], syscallRunnerArgs[5], syscallRunnerArgs[6]) cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr @@ -363,7 +396,8 @@ func (s *snapSeccompSuite) TestUnrestricted(c *C) { err := main.Compile([]byte(inp), outPath) c.Assert(err, IsNil) - c.Check(outPath, testutil.FileEquals, inp) + c.Check(outPath+".allow", testutil.FileEquals, inp) + c.Check(outPath+".deny", testutil.FileEquals, inp) } // TestCompile iterates over a range of textual seccomp whitelist rules and @@ -395,8 +429,9 @@ func (s *snapSeccompSuite) TestCompile(c *C) { {"read", "read", Allow}, {"read\nwrite\nexecve\n", "write", Allow}, - // trivial denial - {"read", "ioctl", Deny}, + // trivial denial (uses write in allow-list to ensure any + // errors printing is visible) + {"write", "ioctl", Deny}, // test argument filtering syntax, we currently support: // >=, <=, !, <, >, | diff --git a/interfaces/seccomp/template.go b/interfaces/seccomp/template.go index e92bafc3bde..3006e791bd4 100644 --- a/interfaces/seccomp/template.go +++ b/interfaces/seccomp/template.go @@ -205,14 +205,7 @@ inotify_rm_watch # TODO: this should be scaled back even more ~ioctl - TIOCSTI ~ioctl - TIOCLINUX -# restrict argument otherwise will match all uses of ioctl() and allow the rules -# that were disallowed above -# TODO: Fix the need to keep TIOCLINUX here - the issue is a unrestricted -# allow for "ioctl" here makes libseccomp "optimize" the deny rules -# above away and the generated bpf becomes just "allow ioctl". -# We should fix this by creating a way to make "AND" rules, so -# this becomes "ioctl - !TIOCSTI&&!TIOCLINUX" and remove the "~" again. -ioctl - !TIOCSTI +ioctl io_cancel io_destroy diff --git a/tests/main/snap-seccomp/task.yaml b/tests/main/snap-seccomp/task.yaml index b81a663bd91..a9ca07b59c4 100644 --- a/tests/main/snap-seccomp/task.yaml +++ b/tests/main/snap-seccomp/task.yaml @@ -1,8 +1,6 @@ summary: Ensure that the snap-seccomp bpf handling works -# FIXME: once $(snap debug confinment) can be used (in 2.27+) remove -# the systems line -systems: [ubuntu-16*, ubuntu-18*] +systems: [ubuntu-*] # Start early as it takes a long time. priority: 100 @@ -25,7 +23,7 @@ execute: | # from the old test_complain echo "Test that the @complain keyword works" - rm -f "${PROFILE}.bin" + rm -f "${PROFILE}.bin"* cat >"${PROFILE}.src" <"${PROFILE}.src" <"${PROFILE}.src" <"${PROFILE}.src" <&1 ); then - echo "test-snapd-sh.sh should fail with invalid seccomp profile" - exit 1 - fi + for ext in allow deny; do + dd if=/dev/urandom of="${PROFILE}.bin.$ext" count=1 bs=1024 + if output=$(test-snapd-sh.sh -c 'echo hello' 2>&1 ); then + echo "test-snapd-sh.sh should fail with invalid seccomp profile" + exit 1 + fi + done echo "$output" | MATCH "cannot apply seccomp profile: Invalid argument" echo "Add huge snapd.test-snapd-sh.bin to ensure size limit works" - dd if=/dev/zero of="${PROFILE}.bin" count=50 bs=1M - if output=$(test-snapd-sh.sh -c 'echo hello' 2>&1 ); then - echo "test-snapd-sh.sh should fail with big seccomp profile" - exit 1 - fi + for ext in allow deny; do + dd if=/dev/zero of="${PROFILE}.bin.$ext" count=50 bs=1M + if output=$(test-snapd-sh.sh -c 'echo hello' 2>&1 ); then + echo "test-snapd-sh.sh should fail with big seccomp profile" + exit 1 + fi + done echo "$output" | MATCH "cannot fit .* to memory buffer" echo "Ensure the code cannot not run with a missing .bin profile" - rm -f "${PROFILE}.bin" + rm -f "${PROFILE}.bin"* if test-snapd-sh.sh -c 'echo hello'; then echo "filtering broken: program should have failed to run" exit 1 fi echo "Ensure the code cannot not run with an empty seccomp profile" - rm -f "${PROFILE}.bin" + rm -f "${PROFILE}.bin"* echo "" > "${PROFILE}.src" $SNAP_SECCOMP compile "${PROFILE}.src" "${PROFILE}.bin" if test-snapd-sh.sh -c 'echo hello'; then @@ -121,7 +123,7 @@ execute: | fi echo "Ensure snap-confine waits for security profiles to appear" - rm -f "${PROFILE}.bin" + rm -f "${PROFILE}.bin"* cat >"${PROFILE}.src" < Date: Fri, 14 Jul 2023 12:18:46 +0200 Subject: [PATCH 02/10] snap-confine: create/use sc_cleanup_seccomp_profile (thanks to Alex Murray) --- cmd/snap-confine/seccomp-support.c | 31 +++++++++++++++--------------- cmd/snap-confine/seccomp-support.h | 18 +++++++++++++++-- 2 files changed, 32 insertions(+), 17 deletions(-) diff --git a/cmd/snap-confine/seccomp-support.c b/cmd/snap-confine/seccomp-support.c index 9c12c142f33..140abc9536f 100644 --- a/cmd/snap-confine/seccomp-support.c +++ b/cmd/snap-confine/seccomp-support.c @@ -114,28 +114,29 @@ bool sc_apply_seccomp_profile_for_security_tag(const char *security_tag) { char profile_path[PATH_MAX] = { 0 }; - struct sock_fprog prog_allow = { 0 }; + struct sock_fprog SC_CLEANUP(sc_cleanup_seccomp_profile) prog_allow = { 0 }; sc_must_snprintf(profile_path, sizeof(profile_path), "%s/%s.bin.allow", filter_profile_dir, security_tag); - if (!sc_load_seccomp_profile_path(profile_path, &prog_allow)) { - return false; - } + if (!sc_load_seccomp_profile_path(profile_path, &prog_allow)) { + return false; + } - struct sock_fprog prog_deny = { 0 }; + struct sock_fprog SC_CLEANUP(sc_cleanup_seccomp_profile) prog_deny = { 0 }; sc_must_snprintf(profile_path, sizeof(profile_path), "%s/%s.bin.deny", filter_profile_dir, security_tag); - if (!sc_load_seccomp_profile_path(profile_path, &prog_deny)) { - return false; - } + if (!sc_load_seccomp_profile_path(profile_path, &prog_deny)) { + return false; + } - sc_apply_seccomp_filter(&prog_deny); - sc_apply_seccomp_filter(&prog_allow); + sc_apply_seccomp_filter(&prog_deny); + sc_apply_seccomp_filter(&prog_allow); + + return true; +} - // XXX: this is not ideal, allocation happens in - // sc_load_seccomp_profile but free here :( - free(prog_allow.filter); - free(prog_deny.filter); - return true; +void sc_cleanup_seccomp_profile(struct sock_fprog *prog) { + free(prog->filter); + prog->filter = NULL; } bool sc_load_seccomp_profile_path(const char *profile_path, struct sock_fprog *prog) diff --git a/cmd/snap-confine/seccomp-support.h b/cmd/snap-confine/seccomp-support.h index b387913e471..f225a91c983 100644 --- a/cmd/snap-confine/seccomp-support.h +++ b/cmd/snap-confine/seccomp-support.h @@ -21,8 +21,6 @@ #include -bool sc_load_seccomp_profile_path(const char *profile_path, struct sock_fprog *prog); - /** * sc_apply_seccomp_profile_for_security_tag applies a seccomp profile to the * current process. The filter is loaded from a pre-compiled bpf bytecode @@ -49,6 +47,22 @@ bool sc_load_seccomp_profile_path(const char *profile_path, struct sock_fprog *p **/ bool sc_apply_seccomp_profile_for_security_tag(const char *security_tag); +/** sc_apply_global_seccomp_profile applies the global seccomp profile + **/ void sc_apply_global_seccomp_profile(void); +/** + * sc_load_seccomp_profile_path loads the seccomp profile from the given + * profile_path into the given sock_fprog. The sock_fprog needs to be + * freed with sc_cleanup_seccomp_profile() later. + * + * The return value indicates if the loading was successful. + **/ +bool sc_load_seccomp_profile_path(const char *profile_path, struct sock_fprog *prog); + +/** + * sc_cleanup_seccomp_profile frees the dynamic memory from sock_fprog. + **/ +void sc_cleanup_seccomp_profile(struct sock_fprog *prog); + #endif From 95b086a8f462ca0a2881e3f7c0c7d09a5587be63 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Fri, 14 Jul 2023 17:36:15 +0200 Subject: [PATCH 03/10] snap-{confine,seccomp}: use directory for seccomp bpf profiles 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). --- cmd/snap-confine/seccomp-support.c | 4 +- cmd/snap-seccomp/main.go | 96 +++++++++++++++++++++--------- cmd/snap-seccomp/main_test.go | 8 ++- interfaces/seccomp/backend.go | 4 +- interfaces/seccomp/backend_test.go | 33 +++++----- 5 files changed, 93 insertions(+), 52 deletions(-) diff --git a/cmd/snap-confine/seccomp-support.c b/cmd/snap-confine/seccomp-support.c index 140abc9536f..9d05126fb08 100644 --- a/cmd/snap-confine/seccomp-support.c +++ b/cmd/snap-confine/seccomp-support.c @@ -115,14 +115,14 @@ bool sc_apply_seccomp_profile_for_security_tag(const char *security_tag) { char profile_path[PATH_MAX] = { 0 }; struct sock_fprog SC_CLEANUP(sc_cleanup_seccomp_profile) prog_allow = { 0 }; - sc_must_snprintf(profile_path, sizeof(profile_path), "%s/%s.bin.allow", + sc_must_snprintf(profile_path, sizeof(profile_path), "%s/%s/filter.allow", filter_profile_dir, security_tag); if (!sc_load_seccomp_profile_path(profile_path, &prog_allow)) { return false; } struct sock_fprog SC_CLEANUP(sc_cleanup_seccomp_profile) prog_deny = { 0 }; - sc_must_snprintf(profile_path, sizeof(profile_path), "%s/%s.bin.deny", + sc_must_snprintf(profile_path, sizeof(profile_path), "%s/%s/filter.deny", filter_profile_dir, security_tag); if (!sc_load_seccomp_profile_path(profile_path, &prog_deny)) { return false; diff --git a/cmd/snap-seccomp/main.go b/cmd/snap-seccomp/main.go index b0df5941ddb..ca55ba5bb34 100644 --- a/cmd/snap-seccomp/main.go +++ b/cmd/snap-seccomp/main.go @@ -213,10 +213,13 @@ import ( "fmt" "io/ioutil" "os" + "path/filepath" "strconv" "strings" "syscall" + "golang.org/x/sys/unix" + "github.com/seccomp/libseccomp-golang" "github.com/snapcore/snapd/arch" @@ -791,6 +794,67 @@ func complainAction() seccomp.ScmpAction { return seccomp.ActAllow } +// XXX: find a more elegant way for this +func atomicWriteInDir(targetDir string, f func(stageDir string) error) error { + stageDir := targetDir + ".new" + for _, d := range []string{targetDir, stageDir} { + if err := os.MkdirAll(d, 0755); err != nil { + return err + } + } + defer os.RemoveAll(stageDir) + + if err := f(stageDir); err != nil { + return err + } + + if err := unix.Renameat2(0, stageDir, 0, targetDir, unix.RENAME_EXCHANGE); err != nil { + return err + } + + return nil +} + +func writeUnrestrictedFilter(targetDir string) error { + return atomicWriteInDir(targetDir, func(stageDir string) error { + for _, ext := range []string{"filter.allow", "filter.deny"} { + if err := osutil.AtomicWrite(filepath.Join(stageDir, ext), bytes.NewBufferString("@unrestricted\n"), 0644, 0); err != nil { + return err + } + } + return nil + }) +} + +func writeSeccompFilterFiles(targetDir string, filterAllow, filterDeny *seccomp.ScmpFilter) error { + return atomicWriteInDir(targetDir, func(stageDir string) error { + pathAllow := filepath.Join(stageDir, "filter.allow") + pathDeny := filepath.Join(stageDir, "filter.deny") + + for _, d := range []struct { + outFile string + outFilter *seccomp.ScmpFilter + }{ + {pathAllow, filterAllow}, + {pathDeny, filterDeny}, + } { + fout, err := os.Create(d.outFile) + if err != nil { + return err + } + defer fout.Close() + + if err := d.outFilter.ExportBPF(fout); err != nil { + return err + } + if err := fout.Sync(); err != nil { + return err + } + } + return nil + }) +} + func newComplainFilter(complainAct seccomp.ScmpAction) (*seccomp.ScmpFilter, error) { secFilter, err := seccomp.NewFilter(complainAct) if err != nil { @@ -813,12 +877,7 @@ func compile(content []byte, out string) error { unrestricted, complain := preprocess(content) switch { case unrestricted: - for _, ext := range []string{".allow", ".deny"} { - if err := osutil.AtomicWrite(out+ext, bytes.NewBufferString("@unrestricted\n"), 0644, 0); err != nil { - return err - } - } - return nil + return writeUnrestrictedFilter(out) case complain: var complainAct seccomp.ScmpAction = complainAction() @@ -871,27 +930,8 @@ func compile(content []byte, out string) error { secFilterDeny.ExportPFC(os.Stdout) } - // write atomically - for _, ext := range []string{".allow", ".deny"} { - fout, err := osutil.NewAtomicFile(out+ext, 0644, 0, osutil.NoChown, osutil.NoChown) - if err != nil { - return err - } - // Cancel once Committed is a NOP - defer fout.Cancel() - - switch ext { - case ".allow": - err = secFilterAllow.ExportBPF(fout.File) - case ".deny": - err = secFilterDeny.ExportBPF(fout.File) - } - if err != nil { - return err - } - if err := fout.Commit(); err != nil { - return err - } + if err := writeSeccompFilterFiles(out, secFilterAllow, secFilterDeny); err != nil { + return err } return nil } @@ -949,7 +989,7 @@ func main() { switch cmd { case "compile": if len(os.Args) < 4 { - fmt.Println("compile needs an input and output file") + fmt.Println("compile needs an input file and output dir") os.Exit(1) } content, err = ioutil.ReadFile(os.Args[2]) diff --git a/cmd/snap-seccomp/main_test.go b/cmd/snap-seccomp/main_test.go index b61e7a67769..576d01710e2 100644 --- a/cmd/snap-seccomp/main_test.go +++ b/cmd/snap-seccomp/main_test.go @@ -366,7 +366,9 @@ mprotect } } - cmd := exec.Command(s.seccompBpfLoader, bpfPath+".allow", bpfPath+".deny", syscallRunner, syscallRunnerArgs[0], syscallRunnerArgs[1], syscallRunnerArgs[2], syscallRunnerArgs[3], syscallRunnerArgs[4], syscallRunnerArgs[5], syscallRunnerArgs[6]) + pathAllow := filepath.Join(bpfPath, "filter.allow") + pathDeny := filepath.Join(bpfPath, "filter.deny") + cmd := exec.Command(s.seccompBpfLoader, pathAllow, pathDeny, syscallRunner, syscallRunnerArgs[0], syscallRunnerArgs[1], syscallRunnerArgs[2], syscallRunnerArgs[3], syscallRunnerArgs[4], syscallRunnerArgs[5], syscallRunnerArgs[6]) cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr @@ -396,8 +398,8 @@ func (s *snapSeccompSuite) TestUnrestricted(c *C) { err := main.Compile([]byte(inp), outPath) c.Assert(err, IsNil) - c.Check(outPath+".allow", testutil.FileEquals, inp) - c.Check(outPath+".deny", testutil.FileEquals, inp) + c.Check(filepath.Join(outPath, "filter.allow"), testutil.FileEquals, inp) + c.Check(filepath.Join(outPath, "filter.deny"), testutil.FileEquals, inp) } // TestCompile iterates over a range of textual seccomp whitelist rules and diff --git a/interfaces/seccomp/backend.go b/interfaces/seccomp/backend.go index 9149ec1f649..23f448bdc0b 100644 --- a/interfaces/seccomp/backend.go +++ b/interfaces/seccomp/backend.go @@ -159,7 +159,7 @@ func bpfSrcPath(srcName string) string { } func bpfBinPath(srcName string) string { - return filepath.Join(dirs.SnapSeccompDir, strings.TrimSuffix(srcName, ".src")+".bin") + return filepath.Join(dirs.SnapSeccompDir, strings.TrimSuffix(srcName, ".src")) } func parallelCompile(compiler Compiler, profiles []string) error { @@ -195,7 +195,7 @@ func parallelCompile(compiler Compiler, profiles []string) error { // remove the old profile first so that we are // not loading it accidentally should the // compilation fail - if err := os.Remove(out); err != nil && !os.IsNotExist(err) { + if err := os.RemoveAll(out); err != nil && !os.IsNotExist(err) { res <- err continue } diff --git a/interfaces/seccomp/backend_test.go b/interfaces/seccomp/backend_test.go index a28b56b8cc4..546c7429434 100644 --- a/interfaces/seccomp/backend_test.go +++ b/interfaces/seccomp/backend_test.go @@ -131,7 +131,7 @@ func (s *backendSuite) TestInstallingSnapWritesProfiles(c *C) { c.Check(err, IsNil) // and got compiled c.Check(s.snapSeccomp.Calls(), DeepEquals, [][]string{ - {"snap-seccomp", "compile", profile + ".src", profile + ".bin"}, + {"snap-seccomp", "compile", profile + ".src", profile}, }) } @@ -144,7 +144,7 @@ func (s *backendSuite) TestInstallingSnapWritesHookProfiles(c *C) { c.Check(err, IsNil) // and got compiled c.Check(s.snapSeccomp.Calls(), DeepEquals, [][]string{ - {"snap-seccomp", "compile", profile + ".src", profile + ".bin"}, + {"snap-seccomp", "compile", profile + ".src", profile}, }) } @@ -176,7 +176,7 @@ fi`) // ensure the snap-seccomp from the core snap was used instead c.Check(snapSeccompOnCore.Calls(), DeepEquals, [][]string{ {"snap-seccomp", "version-info"}, // from Initialize() - {"snap-seccomp", "compile", profile + ".src", profile + ".bin"}, + {"snap-seccomp", "compile", profile + ".src", profile}, }) raw, err := ioutil.ReadFile(profile + ".src") c.Assert(err, IsNil) @@ -217,7 +217,7 @@ func (s *backendSuite) TestUpdatingSnapToOneWithMoreApps(c *C) { // file called "snap.sambda.nmbd" was created c.Check(err, IsNil) // and got compiled - c.Check(s.snapSeccomp.Calls(), testutil.DeepContains, []string{"snap-seccomp", "compile", profile + ".src", profile + ".bin"}) + c.Check(s.snapSeccomp.Calls(), testutil.DeepContains, []string{"snap-seccomp", "compile", profile + ".src", profile}) s.snapSeccomp.ForgetCalls() s.RemoveSnap(c, snapInfo) @@ -234,7 +234,7 @@ func (s *backendSuite) TestUpdatingSnapToOneWithHooks(c *C) { // Verify that profile "snap.samba.hook.configure" was created. c.Check(err, IsNil) // and got compiled - c.Check(s.snapSeccomp.Calls(), testutil.DeepContains, []string{"snap-seccomp", "compile", profile + ".src", profile + ".bin"}) + c.Check(s.snapSeccomp.Calls(), testutil.DeepContains, []string{"snap-seccomp", "compile", profile + ".src", profile}) s.snapSeccomp.ForgetCalls() s.RemoveSnap(c, snapInfo) @@ -645,7 +645,7 @@ func (s *backendSuite) TestRebuildsWithVersionInfoWhenNeeded(c *C) { c.Check(profile+".src", testutil.FileEquals, s.profileHeader+"\ndefault\n") c.Check(s.snapSeccomp.Calls(), DeepEquals, [][]string{ - {"snap-seccomp", "compile", profile + ".src", profile + ".bin"}, + {"snap-seccomp", "compile", profile + ".src", profile}, }) // unchanged snap-seccomp version will not trigger a rebuild @@ -671,7 +671,7 @@ fi`) c.Check(s.snapSeccomp.Calls(), HasLen, 2) c.Check(s.snapSeccomp.Calls(), DeepEquals, [][]string{ // compilation from first Setup() - {"snap-seccomp", "compile", profile + ".src", profile + ".bin"}, + {"snap-seccomp", "compile", profile + ".src", profile}, // initialization with new version {"snap-seccomp", "version-info"}, }) @@ -684,11 +684,11 @@ fi`) c.Check(s.snapSeccomp.Calls(), HasLen, 3) c.Check(s.snapSeccomp.Calls(), DeepEquals, [][]string{ // compilation from first Setup() - {"snap-seccomp", "compile", profile + ".src", profile + ".bin"}, + {"snap-seccomp", "compile", profile + ".src", profile}, // initialization with new version {"snap-seccomp", "version-info"}, // compilation of profiles with new compiler version - {"snap-seccomp", "compile", profile + ".src", profile + ".bin"}, + {"snap-seccomp", "compile", profile + ".src", profile}, }) } @@ -912,7 +912,7 @@ func (s *backendSuite) TestParallelCompileHappy(c *C) { c.Assert(m.profiles, DeepEquals, profiles) for _, p := range profiles { - c.Check(filepath.Join(dirs.SnapSeccompDir, p+".bin"), testutil.FileEquals, "done "+p+".bin") + c.Check(filepath.Join(dirs.SnapSeccompDir, p), testutil.FileEquals, "done "+p) } } @@ -938,10 +938,10 @@ func (s *backendSuite) TestParallelCompileError(c *C) { } m := mockedSyncedFailingCompiler{ // pretend compilation of those 2 fails - whichFail: []string{"profile-005.bin", "profile-009.bin"}, + whichFail: []string{"profile-005", "profile-009"}, } err = seccomp.ParallelCompile(&m, profiles) - c.Assert(err, ErrorMatches, "cannot compile .*/bpf/profile-00[59]: failed profile-00[59].bin") + c.Assert(err, ErrorMatches, "cannot compile .*/bpf/profile-00[59]: failed profile-00[59]") // make sure all compiled profiles were removed d, err := os.Open(dirs.SnapSeccompDir) @@ -955,16 +955,15 @@ func (s *backendSuite) TestParallelCompileError(c *C) { func (s *backendSuite) TestParallelCompileRemovesFirst(c *C) { err := os.MkdirAll(dirs.SnapSeccompDir, 0755) c.Assert(err, IsNil) - err = ioutil.WriteFile(filepath.Join(dirs.SnapSeccompDir, "profile-001.bin"), nil, 0755) - c.Assert(err, IsNil) - // make profiles directory non-accessible - err = os.Chmod(dirs.SnapSeccompDir, 0000) + err = os.MkdirAll(filepath.Join(dirs.SnapSeccompDir, "profile-001"), 0700) c.Assert(err, IsNil) + err = os.Chmod(dirs.SnapSeccompDir, 0500) + c.Assert(err, IsNil) defer os.Chmod(dirs.SnapSeccompDir, 0755) m := mockedSyncedCompiler{} err = seccomp.ParallelCompile(&m, []string{"profile-001"}) - c.Assert(err, ErrorMatches, "remove .*/profile-001.bin: permission denied") + c.Assert(err, ErrorMatches, "unlinkat .*/profile-001: permission denied") } From 79ae34846441d7734dd4cfc1415b06a032d79f5e Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Fri, 14 Jul 2023 17:48:06 +0200 Subject: [PATCH 04/10] tests: update security-seccomp test --- tests/main/security-seccomp/task.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/main/security-seccomp/task.yaml b/tests/main/security-seccomp/task.yaml index 62899373958..9b380b29ae5 100644 --- a/tests/main/security-seccomp/task.yaml +++ b/tests/main/security-seccomp/task.yaml @@ -25,7 +25,7 @@ details: | environment: SRC: /var/lib/snapd/seccomp/bpf/snap.test-snapd-setpriority.test-snapd-setpriority.src - BIN: /var/lib/snapd/seccomp/bpf/snap.test-snapd-setpriority.test-snapd-setpriority.bin + BIN: /var/lib/snapd/seccomp/bpf/snap.test-snapd-setpriority.test-snapd-setpriority AAP: /var/lib/snapd/apparmor/profiles/snap.test-snapd-setpriority.test-snapd-setpriority prepare: | From 8293acce992e1ddc77c9d66edb0ff5208468c80f Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 17 Jul 2023 09:27:29 +0200 Subject: [PATCH 05/10] tests: update snap-seccomp filter test for latest code change --- tests/main/snap-seccomp/task.yaml | 42 +++++++++++++++---------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/tests/main/snap-seccomp/task.yaml b/tests/main/snap-seccomp/task.yaml index a9ca07b59c4..cd8799b113d 100644 --- a/tests/main/snap-seccomp/task.yaml +++ b/tests/main/snap-seccomp/task.yaml @@ -23,17 +23,17 @@ execute: | # from the old test_complain echo "Test that the @complain keyword works" - rm -f "${PROFILE}.bin"* + rm -rf "${PROFILE}" cat >"${PROFILE}.src" <"${PROFILE}.src" <"${PROFILE}.src" <"${PROFILE}.src" <&1 ); then echo "test-snapd-sh.sh should fail with invalid seccomp profile" exit 1 @@ -95,9 +95,9 @@ execute: | done echo "$output" | MATCH "cannot apply seccomp profile: Invalid argument" - echo "Add huge snapd.test-snapd-sh.bin to ensure size limit works" - for ext in allow deny; do - dd if=/dev/zero of="${PROFILE}.bin.$ext" count=50 bs=1M + echo "Add huge snapd.test-snapd-sh filters to ensure size limit works" + for f in filter.allow filter.deny; do + dd if=/dev/zero of="${PROFILE}/$f" count=50 bs=1M if output=$(test-snapd-sh.sh -c 'echo hello' 2>&1 ); then echo "test-snapd-sh.sh should fail with big seccomp profile" exit 1 @@ -106,28 +106,28 @@ execute: | echo "$output" | MATCH "cannot fit .* to memory buffer" - echo "Ensure the code cannot not run with a missing .bin profile" - rm -f "${PROFILE}.bin"* + echo "Ensure the code cannot not run with a missing filter profile" + rm -rf "${PROFILE}" if test-snapd-sh.sh -c 'echo hello'; then echo "filtering broken: program should have failed to run" exit 1 fi echo "Ensure the code cannot not run with an empty seccomp profile" - rm -f "${PROFILE}.bin"* + rm -rf "${PROFILE}" echo "" > "${PROFILE}.src" - $SNAP_SECCOMP compile "${PROFILE}.src" "${PROFILE}.bin" + $SNAP_SECCOMP compile "${PROFILE}.src" "${PROFILE}" if test-snapd-sh.sh -c 'echo hello'; then echo "filtering broken: program should have failed to run" exit 1 fi echo "Ensure snap-confine waits for security profiles to appear" - rm -f "${PROFILE}.bin"* + rm -rf "${PROFILE}" cat >"${PROFILE}.src" < Date: Mon, 17 Jul 2023 12:31:40 +0200 Subject: [PATCH 06/10] snap-seccomp: fix tests for explicit/implicit denials 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. --- cmd/snap-seccomp/main_test.go | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/cmd/snap-seccomp/main_test.go b/cmd/snap-seccomp/main_test.go index 576d01710e2..028efe60367 100644 --- a/cmd/snap-seccomp/main_test.go +++ b/cmd/snap-seccomp/main_test.go @@ -53,6 +53,7 @@ var _ = Suite(&snapSeccompSuite{}) const ( Deny = iota + DenyExplicit Allow ) @@ -186,9 +187,12 @@ int main(int argc, char** argv) syscall_ret = syscall(l[0], l[1], l[2], l[3], l[4], l[5], l[6]); // 911 is our mocked errno for implicit denials via unlisted syscalls and // 999 is explicit denial - if (syscall_ret < 0 && (errno == 911 || errno == 999)) { + if (syscall_ret < 0 && errno == 911) { ret = 10; } + if (syscall_ret < 0 && errno == 999) { + ret = 20; + } syscall(SYS_exit, ret, 0, 0, 0, 0, 0); return 0; } @@ -377,13 +381,23 @@ mprotect // else is unexpected (segv, strtoll failure, ...) exitCode, e := osutil.ExitCode(err) c.Assert(e, IsNil) - c.Assert(exitCode == 0 || exitCode == 10, Equals, true, Commentf("unexpected exit code: %v for %v - test setup broken", exitCode, seccompWhitelist)) + c.Assert(exitCode == 0 || exitCode == 10 || exitCode == 20, Equals, true, Commentf("unexpected exit code: %v for %v - test setup broken", exitCode, seccompWhitelist)) switch expected { case Allow: if err != nil { c.Fatalf("unexpected error for %q (failed to run %q)", seccompWhitelist, err) } case Deny: + if exitCode != 10 { + c.Fatalf("unexpected exit code for %q %q (%v != %v)", seccompWhitelist, bpfInput, exitCode, 10) + } + if err == nil { + c.Fatalf("unexpected success for %q %q (ran but should have failed)", seccompWhitelist, bpfInput) + } + case DenyExplicit: + if exitCode != 20 { + c.Fatalf("unexpected exit code for %q %q (%v != %v)", seccompWhitelist, bpfInput, exitCode, 20) + } if err == nil { c.Fatalf("unexpected success for %q %q (ran but should have failed)", seccompWhitelist, bpfInput) } @@ -488,12 +502,12 @@ func (s *snapSeccompSuite) TestCompile(c *C) { {"ioctl - TIOCSTI", "ioctl;native;-,TIOCSTI", Allow}, {"ioctl - TIOCSTI", "ioctl;native;-,99", Deny}, {"ioctl - !TIOCSTI", "ioctl;native;-,TIOCSTI", Deny}, - {"~ioctl - TIOCSTI", "ioctl;native;-,TIOCSTI", Deny}, + {"ioctl\n~ioctl - TIOCSTI", "ioctl;native;-,TIOCSTI", DenyExplicit}, // also check we can deny multiple uses of ioctl but still allow // others - {"~ioctl - TIOCSTI\n~ioctl - TIOCLINUX\nioctl - !TIOCSTI", "ioctl;native;-,TIOCSTI", Deny}, - {"~ioctl - TIOCSTI\n~ioctl - TIOCLINUX\nioctl - !TIOCSTI", "ioctl;native;-,TIOCLINUX", Deny}, - {"~ioctl - TIOCSTI\n~ioctl - TIOCLINUX\nioctl - !TIOCSTI", "ioctl;native;-,TIOCGWINSZ", Allow}, + {"ioctl\n~ioctl - TIOCSTI\n~ioctl - TIOCLINUX\nioctl - !TIOCSTI", "ioctl;native;-,TIOCSTI", DenyExplicit}, + {"ioctl\n~ioctl - TIOCSTI\n~ioctl - TIOCLINUX\nioctl - !TIOCSTI", "ioctl;native;-,TIOCLINUX", DenyExplicit}, + {"ioctl\n~ioctl - TIOCSTI\n~ioctl - TIOCLINUX\nioctl - !TIOCSTI", "ioctl;native;-,TIOCGWINSZ", Allow}, // test_bad_seccomp_filter_args_clone {"setns - CLONE_NEWNET", "setns;native;-,99", Deny}, @@ -510,6 +524,13 @@ func (s *snapSeccompSuite) TestCompile(c *C) { // test_bad_seccomp_filter_args_prio {"setpriority PRIO_PROCESS 0 >=0", "setpriority;native;PRIO_PROCESS,0,19", Allow}, {"setpriority PRIO_PROCESS 0 >=0", "setpriority;native;99", Deny}, + // negative filtering + {"setpriority\n~setpriority PRIO_PROCESS 0 >=0", "setpriority;native;PRIO_PROCESS,0,10", DenyExplicit}, + // mix negative/positiv filtering + // allow setprioty >= 5 but explicitly deny >=10 + {"setpriority PRIO_PROCESS 0 >=5\n~setpriority PRIO_PROCESS 0 >=10", "setpriority;native;PRIO_PROCESS,0,2", Deny}, + {"setpriority PRIO_PROCESS 0 >=5\n~setpriority PRIO_PROCESS 0 >=10", "setpriority;native;PRIO_PROCESS,0,5", Allow}, + {"setpriority PRIO_PROCESS 0 >=5\n~setpriority PRIO_PROCESS 0 >=10", "setpriority;native;PRIO_PROCESS,0,10", DenyExplicit}, // test_bad_seccomp_filter_args_quotactl {"quotactl Q_GETQUOTA", "quotactl;native;Q_GETQUOTA", Allow}, From d5471b46dcfd2d6d9320ed03a89825e0e482c22b Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 17 Jul 2023 13:07:02 +0200 Subject: [PATCH 07/10] tests: update security-seccomp test --- tests/main/security-seccomp/task.yaml | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/main/security-seccomp/task.yaml b/tests/main/security-seccomp/task.yaml index 9b380b29ae5..3c6ec637613 100644 --- a/tests/main/security-seccomp/task.yaml +++ b/tests/main/security-seccomp/task.yaml @@ -93,10 +93,14 @@ execute: | echo "and check that negative nice fails" test-snapd-setpriority -10 | MATCH 'Operation not permitted \(EPERM\)' + # TODO: filtering on setpriority is a bit confusing as it is not part + # of the "negative args" filter added in ec7c9f27c97 so the fact that + # negative args are denied is a bit magic echo "Explicitly deny arg filtered setpriority rule" - sed 's/^\(setpriority.*\)/~\1/g' "$SRC".orig > "$SRC" + cp -a "$SRC".orig "$SRC" + echo '~setpriority PRIO_PROCESS 0 >10' >> "$SRC" snapd.tool exec snap-seccomp compile "$SRC" "$BIN" - echo "and check that positive nice fails with explicit denial" - test-snapd-setpriority 10 | MATCH 'Insufficient privileges \(EACCES\)' + echo "and check that explicit requested fails with explicit denial" + test-snapd-setpriority 11 | MATCH 'Insufficient privileges \(EACCES\)' echo "and check that negative nice still fails with implicit denial" test-snapd-setpriority -10 | MATCH 'Operation not permitted \(EPERM\)' From 5752680694d4088d444e6aea75da2575019ba69e Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 17 Jul 2023 18:14:43 +0200 Subject: [PATCH 08/10] tests: fix snap-seccomp test --- tests/main/snap-seccomp/task.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/main/snap-seccomp/task.yaml b/tests/main/snap-seccomp/task.yaml index cd8799b113d..f8554ea2f39 100644 --- a/tests/main/snap-seccomp/task.yaml +++ b/tests/main/snap-seccomp/task.yaml @@ -86,7 +86,7 @@ execute: | fi echo "Break snapd.test-snapd-sh filters to ensure (kernel) validation works" - for ext in filter.allow filter.deny; do + for f in filter.allow filter.deny; do dd if=/dev/urandom of="${PROFILE}/$f" count=1 bs=1024 if output=$(test-snapd-sh.sh -c 'echo hello' 2>&1 ); then echo "test-snapd-sh.sh should fail with invalid seccomp profile" From 7ba40d6ad29c6d955b6f3c2df10b6ada1b07d21b Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 17 Jul 2023 21:26:58 +0200 Subject: [PATCH 09/10] seccomp: fix remove of binary profiles --- interfaces/seccomp/backend.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/interfaces/seccomp/backend.go b/interfaces/seccomp/backend.go index 23f448bdc0b..075a8917f70 100644 --- a/interfaces/seccomp/backend.go +++ b/interfaces/seccomp/backend.go @@ -280,7 +280,7 @@ func (b *Backend) Setup(snapInfo *snap.Info, opts interfaces.ConfinementOptions, return fmt.Errorf("cannot synchronize security files for snap %q: %s", snapName, err) } for _, c := range removed { - err := os.Remove(bpfBinPath(c)) + err := os.RemoveAll(bpfBinPath(c)) if err != nil && !os.IsNotExist(err) { return err } From 5d5e7046f3cd0554060af7bca65afb62a52f4109 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Tue, 18 Jul 2023 12:23:03 +0200 Subject: [PATCH 10/10] osutil: make EnsureDirStateGlobs() deal with subdirs --- interfaces/seccomp/backend.go | 5 ++++- osutil/syncdir.go | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/interfaces/seccomp/backend.go b/interfaces/seccomp/backend.go index 075a8917f70..9cd696a5be2 100644 --- a/interfaces/seccomp/backend.go +++ b/interfaces/seccomp/backend.go @@ -291,10 +291,13 @@ func (b *Backend) Setup(snapInfo *snap.Info, opts interfaces.ConfinementOptions, // Remove removes seccomp profiles of a given snap. func (b *Backend) Remove(snapName string) error { + if err := os.RemoveAll(bpfBinPath(snapName)); err != nil { + return fmt.Errorf("cannot remove bpf security files for snap %q: %s", snapName, err) + } glob := interfaces.SecurityTagGlob(snapName) _, _, err := osutil.EnsureDirState(dirs.SnapSeccompDir, glob, nil) if err != nil { - return fmt.Errorf("cannot synchronize security files for snap %q: %s", snapName, err) + return fmt.Errorf("cannot remove bpf security files for snap %q: %s", snapName, err) } return nil } diff --git a/osutil/syncdir.go b/osutil/syncdir.go index ddf3b4be344..845e16e614a 100644 --- a/osutil/syncdir.go +++ b/osutil/syncdir.go @@ -161,7 +161,8 @@ func EnsureDirStateGlobs(dir string, globs []string, content map[string]FileStat if content[baseName] != nil { continue } - err := os.Remove(path) + // XXX: option? + err := os.RemoveAll(path) if err != nil { if firstErr == nil { firstErr = err