From c584cb0475fb6d1f5a5285f62706cba7bbb168ef Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 10 Jul 2023 13:59:33 +0200 Subject: [PATCH] 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] https://github.com/snapcore/snapd/pull/12849#discussion_r1206855700 [2] https://github.com/seccomp/libseccomp/issues/44 --- cmd/snap-confine/seccomp-support.c | 120 ++++++++++++--- cmd/snap-confine/snap-confine.apparmor.in | 2 +- cmd/snap-seccomp/export_test.go | 13 ++ cmd/snap-seccomp/main.go | 160 +++++++++++++++++--- cmd/snap-seccomp/main_test.go | 128 +++++++++++++--- interfaces/seccomp/backend.go | 13 +- interfaces/seccomp/backend_test.go | 31 ++-- interfaces/seccomp/template.go | 9 +- tests/main/install-store-laaaarge/task.yaml | 9 +- tests/main/preseed/task.yaml | 2 +- tests/main/security-profiles/task.yaml | 4 +- tests/main/security-seccomp/task.yaml | 12 +- tests/main/snap-seccomp/task.yaml | 59 ++++---- 13 files changed, 437 insertions(+), 125 deletions(-) diff --git a/cmd/snap-confine/seccomp-support.c b/cmd/snap-confine/seccomp-support.c index 7432d79cb15..2259ba4d918 100644 --- a/cmd/snap-confine/seccomp-support.c +++ b/cmd/snap-confine/seccomp-support.c @@ -17,11 +17,13 @@ #include "config.h" #include "seccomp-support.h" +#include #include #include #include #include #include +#include #include #include #include @@ -45,6 +47,31 @@ static const char *filter_profile_dir = "/var/lib/snapd/seccomp/bpf/"; typedef struct sock_filter bpf_instr; +// keep in sync with snap-seccomp/main.go +// +// header of a seccomp.bin2 filter file, in network byte order (big-endian) +// +// Note that padding is important +// alternatively "__attribute__((__packed__))" could be used +struct sc_seccomp_file_header { + // header: "SC" + char header[2]; + // version: 0x1 + char version; + // flags + char unrestricted; + // unused + char padding[4]; + + // size of allow filter in byte + uint32_t len_allow_filter; + // size of deny filter in byte + uint32_t len_deny_filter; + // reserved for future use + char reserved2[112]; +}; +static_assert(sizeof(struct sc_seccomp_file_header) == 128,"unexpected struct size"); + static void validate_path_has_strict_perms(const char *path) { struct stat stat_buf; @@ -109,12 +136,73 @@ static void validate_bpfpath_is_safe(const char *path) } } +static void sc_cleanup_sock_fprog(struct sock_fprog *prog) +{ + free(prog->filter); + prog->filter = NULL; +} + +static void sc_must_read_filter_from_file(FILE *file, uint32_t len_bytes, char *what, struct sock_fprog *prog) +{ + prog->len = len_bytes / sizeof(struct sock_filter); + prog->filter = (struct sock_filter *)malloc(len_bytes); + if (prog->filter == NULL) { + die("cannot allocate %u bytes of memory for %s seccomp filter ", len_bytes, what); + } + size_t num_read = fread(prog->filter, 1, len_bytes, file); + if (ferror(file)) { + die("cannot read %s filter", what); + } + if (num_read != len_bytes) { + die("short read for filter %s %zu != %i", what, num_read, len_bytes); + } +} + +static FILE* sc_must_read_header_from_file(const char *profile_path, struct sc_seccomp_file_header *hdr) +{ + FILE *file = fopen(profile_path, "rb"); + if (file == NULL) { + die("cannot open seccomp filter %s", profile_path); + } + size_t num_read = fread(hdr, 1, sizeof(struct sc_seccomp_file_header), file); + if (ferror(file) != 0) { + die("cannot read seccomp profile %s", profile_path); + } + if (num_read < sizeof(struct sc_seccomp_file_header)) { + die("short read on seccomp header: %zu", num_read); + } + if (hdr->header[0] != 'S' || hdr->header[1] != 'C') { + die("unexpected seccomp header: %x%x", hdr->header[0], hdr->header[1]); + } + if (hdr->version != 1) { + die("unexpected seccomp file version: %x", hdr->version); + } + if (hdr->len_allow_filter > MAX_BPF_SIZE) { + die("allow filter size too big %u", hdr->len_allow_filter); + } + if (hdr->len_deny_filter > MAX_BPF_SIZE) { + die("deny filter size too big %u", hdr->len_deny_filter); + } + struct stat buf; + if (fstat(fileno(file), &buf) != 0) { + die("cannot fstat the seccomp file"); + } + off_t expected_size = sizeof(struct sc_seccomp_file_header)+hdr->len_allow_filter+hdr->len_deny_filter; + if (buf.st_size != expected_size) { + die("unexpected filesize %ju != %ju", buf.st_size, expected_size); + } + + return file; +} + 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 SC_CLEANUP(sc_cleanup_sock_fprog) prog_allow = { 0 }; + struct sock_fprog SC_CLEANUP(sc_cleanup_sock_fprog) prog_deny = { 0 }; + sc_must_snprintf(profile_path, sizeof(profile_path), "%s/%s.bin2", filter_profile_dir, security_tag); // Wait some time for the security profile to show up. When @@ -152,24 +240,22 @@ bool sc_apply_seccomp_profile_for_security_tag(const char *security_tag) // set on the system. validate_bpfpath_is_safe(profile_path); - /* The extra space has dual purpose. First of all, it is required to detect - * feof() while still being able to correctly read MAX_BPF_SIZE bytes of - * seccomp profile. In addition, because we treat the profile as a - * quasi-string and use sc_streq(), to compare it. The extra space is used - * as a way to ensure the result is a terminated string (though in practice - * it can contain embedded NULs any earlier position). Note that - * sc_read_seccomp_filter knows about the extra space and ensures that the - * buffer is never empty. */ - char bpf[MAX_BPF_SIZE + 1] = { 0 }; - size_t num_read = sc_read_seccomp_filter(profile_path, bpf, sizeof bpf); - if (sc_streq(bpf, "@unrestricted\n")) { + // TODO: strange init to workaround bug in gcc on 14.04 that claims: + // "error: missing braces around initializer" + struct sc_seccomp_file_header hdr = {{0},0}; + FILE *file SC_CLEANUP(sc_cleanup_file) = sc_must_read_header_from_file(profile_path, &hdr); + if (hdr.unrestricted == 0x1) { return false; } - struct sock_fprog prog = { - .len = num_read / sizeof(struct sock_filter), - .filter = (struct sock_filter *)bpf, - }; - sc_apply_seccomp_filter(&prog); + + // populate allow + sc_must_read_filter_from_file(file, hdr.len_allow_filter, "allow", &prog_allow); + sc_must_read_filter_from_file(file, hdr.len_deny_filter, "deny", &prog_deny); + + // apply both filters + sc_apply_seccomp_filter(&prog_deny); + sc_apply_seccomp_filter(&prog_allow); + return true; } diff --git a/cmd/snap-confine/snap-confine.apparmor.in b/cmd/snap-confine/snap-confine.apparmor.in index 73d14c8c781..99937b47060 100644 --- a/cmd/snap-confine/snap-confine.apparmor.in +++ b/cmd/snap-confine/snap-confine.apparmor.in @@ -149,7 +149,7 @@ # change_profile unsafe /** -> **, # reading seccomp filters - /{tmp/snap.rootfs_*/,}var/lib/snapd/seccomp/bpf/*.bin r, + /{tmp/snap.rootfs_*/,}var/lib/snapd/seccomp/bpf/*.bin2 r, # adding a missing bpf mount mount fstype=bpf options=(rw) bpf -> /sys/fs/bpf/, diff --git a/cmd/snap-seccomp/export_test.go b/cmd/snap-seccomp/export_test.go index 4545b5ad3e5..74d88055287 100644 --- a/cmd/snap-seccomp/export_test.go +++ b/cmd/snap-seccomp/export_test.go @@ -19,11 +19,18 @@ package main +import ( + "os" + + "github.com/snapcore/snapd/testutil" +) + var ( Compile = compile SeccompResolver = seccompResolver VersionInfo = versionInfo GoSeccompFeatures = goSeccompFeatures + ExportBPF = exportBPF ) func MockArchDpkgArchitecture(f func() string) (restore func()) { @@ -65,3 +72,9 @@ func MockSeccompSyscalls(syscalls []string) (resture func()) { seccompSyscalls = old } } + +func MockOsCreateTemp(f func(dir, pattern string) (*os.File, error)) (restore func()) { + restore = testutil.Backup(&osCreateTemp) + osCreateTemp = f + return restore +} diff --git a/cmd/snap-seccomp/main.go b/cmd/snap-seccomp/main.go index 4ff3799b0a1..65b61c73b04 100644 --- a/cmd/snap-seccomp/main.go +++ b/cmd/snap-seccomp/main.go @@ -210,7 +210,9 @@ import "C" import ( "bufio" "bytes" + "encoding/binary" "fmt" + "io" "io/ioutil" "os" "strconv" @@ -583,11 +585,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 +607,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) @@ -691,8 +695,11 @@ func parseLine(line string, secFilter *seccomp.ScmpFilter) error { if err = secFilter.AddRuleConditionalExact(secSyscall, action, conds); err != nil { err = secFilter.AddRuleConditional(secSyscall, action, conds) } + if err != nil { + return fmt.Errorf("cannot add rule for line %q: %v", line, err) + } - return err + return nil } // used to mock in tests @@ -789,18 +796,120 @@ func complainAction() seccomp.ScmpAction { return seccomp.ActAllow } +var osCreateTemp = os.CreateTemp + +func exportBPF(filter *seccomp.ScmpFilter) (io.ReadCloser, int64, error) { + errPrefixFmt := "cannot export bpf filter: %w" + + filterFile, err := osCreateTemp("", "filter-file") + if err != nil { + return nil, 0, fmt.Errorf(errPrefixFmt, err) + } + if err := os.Remove(filterFile.Name()); err != nil { + return nil, 0, fmt.Errorf(errPrefixFmt, err) + } + // XXX: would be nice if ExportBPF would support an io.Writer + // but it requires a os.File + if err := filter.ExportBPF(filterFile); err != nil { + return nil, 0, fmt.Errorf(errPrefixFmt, err) + } + if _, err := filterFile.Seek(0, 0); err != nil { + return nil, 0, fmt.Errorf(errPrefixFmt, err) + } + stat, err := filterFile.Stat() + if err != nil { + return nil, 0, fmt.Errorf(errPrefixFmt, err) + } + + return filterFile, stat.Size(), nil +} + +// keep in sync with seccomp-support.c +type scSeccompFileHeader struct { + header [2]byte + version byte + // flags + unrestricted byte + // unused + padding [4]byte + // location of allow/deny, all offsets/len in bytes + lenAllowFilter uint32 + lenDenyFilter uint32 + // reserved for future use + reserved2 [112]byte +} + +func writeUnrestrictedFilter(out string) error { + hdr := scSeccompFileHeader{ + header: [2]byte{'S', 'C'}, + version: 0x1, + // tell snap-confine + unrestricted: 0x1, + } + fout, err := osutil.NewAtomicFile(out, 0644, 0, osutil.NoChown, osutil.NoChown) + if err != nil { + return err + } + defer fout.Cancel() + + if err := binary.Write(fout, arch.Endian(), hdr); err != nil { + return err + } + return fout.Commit() +} + +func writeSeccompFilter(outFile string, filterAllow, filterDeny *seccomp.ScmpFilter) error { + allowBpf, allowSize, err := exportBPF(filterAllow) + if err != nil { + return err + } + defer allowBpf.Close() + + denyBpf, denySize, err := exportBPF(filterDeny) + if err != nil { + return err + } + defer denyBpf.Close() + + fout, err := osutil.NewAtomicFile(outFile, 0644, 0, osutil.NoChown, osutil.NoChown) + if err != nil { + return err + } + defer fout.Cancel() + + hdr := scSeccompFileHeader{ + header: [2]byte{'S', 'C'}, + version: 0x1, + } + hdr.lenAllowFilter = uint32(allowSize) + hdr.lenDenyFilter = uint32(denySize) + // silly go does not give an easy way to use native endian so just + // write big endian and use ntohl() in the C code + if err := binary.Write(fout, arch.Endian(), hdr); err != nil { + return err + } + if _, err := io.Copy(fout, allowBpf); err != nil { + return err + } + if _, err := io.Copy(fout, denyBpf); err != nil { + return err + } + + return fout.Commit() +} + 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) + return writeUnrestrictedFilter(out) case complain: var complainAct seccomp.ScmpAction = complainAction() - secFilter, err = seccomp.NewFilter(complainAct) + secFilterAllow, err = seccomp.NewFilter(complainAct) if err != nil { if complainAct != seccomp.ActAllow { // ActLog is only supported in newer versions @@ -808,9 +917,16 @@ func compile(content []byte, out string) error { // libseccomp-golang. Attempt to fall back to // ActAllow before erroring out. complainAct = seccomp.ActAllow - secFilter, err = seccomp.NewFilter(complainAct) + secFilterAllow, err = seccomp.NewFilter(complainAct) } } + if err != nil { + return fmt.Errorf("cannot create allow seccomp filter: %s", err) + } + secFilterDeny, err = seccomp.NewFilter(complainAct) + if err != nil { + return fmt.Errorf("cannot create deny seccomp filter: %s", err) + } // Set unrestricted to 'true' to fallback to the pre-ActLog // behavior of simply setting the allow filter without adding @@ -819,19 +935,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 +964,14 @@ 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 { + if err := writeSeccompFilter(out, secFilterAllow, secFilterDeny); 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..4cd7fc91224 100644 --- a/cmd/snap-seccomp/main_test.go +++ b/cmd/snap-seccomp/main_test.go @@ -37,7 +37,6 @@ import ( main "github.com/snapcore/snapd/cmd/snap-seccomp" "github.com/snapcore/snapd/osutil" "github.com/snapcore/snapd/release" - "github.com/snapcore/snapd/testutil" ) // Hook up check.v1 into the "go test" runner @@ -53,6 +52,7 @@ var _ = Suite(&snapSeccompSuite{}) const ( Deny = iota + DenyExplicit Allow ) @@ -71,31 +71,57 @@ var seccompBpfLoaderContent = []byte(` #define MAX_BPF_SIZE 32 * 1024 +// keep in sync with: +// cmd/snap-confine/seccomp-support.c +// main.go:scSeccompFileHeader +struct sc_seccomp_file_header { + char header[2]; + char version; + char unrestricted; + char padding[4]; + uint32_t len_allow_filter; + uint32_t len_deny_filter; + char reserved2[112]; +}; + int sc_apply_seccomp_bpf(const char* profile_path) { - unsigned char bpf[MAX_BPF_SIZE + 1]; // account for EOF + struct sc_seccomp_file_header hdr = {{0}, 0}; + 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"); if (fp == NULL) { fprintf(stderr, "cannot read %s\n", profile_path); return -1; } - - // set 'size' to 1; to get bytes transferred - size_t num_read = fread(bpf, 1, sizeof(bpf), fp); - + fread(&hdr, 1, sizeof(struct sc_seccomp_file_header), fp); + if (ferror(fp) != 0) { + perror("fread() header"); + return -1; + } + fread(bpf_allow, 1, hdr.len_allow_filter, fp); if (ferror(fp) != 0) { perror("fread()"); return -1; - } else if (feof(fp) == 0) { - fprintf(stderr, "file too big\n"); + } + fread(bpf_deny, 1, hdr.len_deny_filter, fp); + if (ferror(fp) != 0) { + perror("fread()"); return -1; } + fclose(fp); - struct sock_fprog prog = { - .len = num_read / sizeof(struct sock_filter), - .filter = (struct sock_filter*)bpf, + struct sock_fprog prog_allow = { + .len = hdr.len_allow_filter / sizeof(struct sock_filter), + .filter = (struct sock_filter*)bpf_allow, + }; + + struct sock_fprog prog_deny = { + .len = hdr.len_deny_filter / sizeof(struct sock_filter), + .filter = (struct sock_filter*)bpf_deny, }; // Set NNP to allow loading seccomp policy into the kernel without @@ -105,10 +131,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; } @@ -153,9 +184,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; } @@ -342,13 +376,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) } @@ -363,7 +407,10 @@ func (s *snapSeccompSuite) TestUnrestricted(c *C) { err := main.Compile([]byte(inp), outPath) c.Assert(err, IsNil) - c.Check(outPath, testutil.FileEquals, inp) + expected := [128]byte{'S', 'C', 0x1, 0x1} + fileContent, err := ioutil.ReadFile(outPath) + c.Assert(err, IsNil) + c.Check(fileContent, DeepEquals, expected[:]) } // TestCompile iterates over a range of textual seccomp whitelist rules and @@ -395,8 +442,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: // >=, <=, !, <, >, | @@ -451,12 +499,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}, @@ -473,6 +521,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}, @@ -856,3 +911,32 @@ func (s *snapSeccompSuite) TestCompatArchWorks(c *C) { } } } + +func (s *snapSeccompSuite) TestExportBpfErrors(c *C) { + // invalid filter + _, _, err := main.ExportBPF(&seccomp.ScmpFilter{}) + c.Check(err, ErrorMatches, "cannot export bpf filter: filter is invalid or uninitialized") + + // error from temp + restore := main.MockOsCreateTemp(func(dir, pattern string) (*os.File, error) { + return nil, fmt.Errorf("boom") + }) + defer restore() + _, _, err = main.ExportBPF(&seccomp.ScmpFilter{}) + c.Check(err, ErrorMatches, "cannot export bpf filter: boom") + + // unwritable file + tmpdir := c.MkDir() + defer os.Chmod(tmpdir, 0755) + restore = main.MockOsCreateTemp(func(dir, pattern string) (*os.File, error) { + f, err := os.Create(filepath.Join(tmpdir, "unwritable")) + c.Assert(err, IsNil) + err = os.Chmod(tmpdir, 0100) + c.Assert(err, IsNil) + return f, nil + }) + defer restore() + + _, _, err = main.ExportBPF(&seccomp.ScmpFilter{}) + c.Check(err, ErrorMatches, "cannot export bpf filter: remove /.*/unwritable: permission denied") +} diff --git a/interfaces/seccomp/backend.go b/interfaces/seccomp/backend.go index e44e07d50a7..9deae3b6e95 100644 --- a/interfaces/seccomp/backend.go +++ b/interfaces/seccomp/backend.go @@ -80,6 +80,16 @@ type Backend struct { versionInfo seccomp.VersionInfo } +// TODO: now that snap-seccomp has full support for deny-listing this +// should be replaced with something like: +// +// ~ioctl - 4294967295|TIOCSTI +// ~ioctl - 4294967295|TIOCLINUX +// +// in the default template. This requires that MaskedEq learns +// to deal with two arguments (see also https://github.com/snapcore/snapd/compare/master...mvo5:rework-seccomp-denylist-incoperate-global.bin?expand=1) +// +// globalProfileLE is generated via cmd/snap-seccomp-blacklist var globalProfileLE = []byte{ 0x20, 0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x15, 0x00, 0x00, 0x04, 0x3e, 0x00, 0x00, 0xc0, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x35, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x40, @@ -93,6 +103,7 @@ var globalProfileLE = []byte{ 0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0x7f, } +// globalProfileBE is generated via cmd/snap-seccomp-blacklist var globalProfileBE = []byte{ 0x00, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0x00, 0x15, 0x00, 0x08, 0x80, 0x00, 0x00, 0x16, 0x00, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x15, 0x00, 0x06, 0x00, 0x00, 0x00, 0x36, @@ -149,7 +160,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")+".bin2") } func parallelCompile(compiler Compiler, profiles []string) error { diff --git a/interfaces/seccomp/backend_test.go b/interfaces/seccomp/backend_test.go index 72bdc039bc7..9baa20c59a1 100644 --- a/interfaces/seccomp/backend_test.go +++ b/interfaces/seccomp/backend_test.go @@ -133,7 +133,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 + ".bin2"}, }) } @@ -146,7 +146,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 + ".bin2"}, }) } @@ -178,7 +178,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 + ".bin2"}, }) raw, err := ioutil.ReadFile(profile + ".src") c.Assert(err, IsNil) @@ -219,7 +219,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 + ".bin2"}) s.snapSeccomp.ForgetCalls() s.RemoveSnap(c, snapInfo) @@ -236,7 +236,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 + ".bin2"}) s.snapSeccomp.ForgetCalls() s.RemoveSnap(c, snapInfo) @@ -647,7 +647,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 + ".bin2"}, }) // unchanged snap-seccomp version will not trigger a rebuild @@ -673,7 +673,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 + ".bin2"}, // initialization with new version {"snap-seccomp", "version-info"}, }) @@ -686,11 +686,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 + ".bin2"}, // 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 + ".bin2"}, }) } @@ -914,7 +914,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+".bin2"), testutil.FileEquals, "done "+p+".bin2") } } @@ -940,10 +940,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.bin2", "profile-009.bin2"}, } 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].bin2") // make sure all compiled profiles were removed d, err := os.Open(dirs.SnapSeccompDir) @@ -957,16 +957,17 @@ 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) + err = ioutil.WriteFile(filepath.Join(dirs.SnapSeccompDir, "profile-001.bin2"), nil, 0755) c.Assert(err, IsNil) - // make profiles directory non-accessible err = os.Chmod(dirs.SnapSeccompDir, 0000) 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, "remove .*/profile-001.bin2: permission denied") } 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/install-store-laaaarge/task.yaml b/tests/main/install-store-laaaarge/task.yaml index 273987c875b..9fb9a0d2888 100644 --- a/tests/main/install-store-laaaarge/task.yaml +++ b/tests/main/install-store-laaaarge/task.yaml @@ -2,7 +2,8 @@ summary: snap install a large snap from the store (bigger than tmpfs) prepare: | tests.systemd stop-unit snapd.service snapd.socket - mount -t tmpfs -o rw,nosuid,nodev,size=4 none /tmp + # tempfs needs to be big enough to hold the seccomp tmp file + mount -t tmpfs -o rw,nosuid,nodev,size=256k none /tmp systemctl start snapd.{socket,service} restore: | @@ -12,6 +13,6 @@ restore: | systemctl start snapd.{socket,service} execute: | - # test-snapd-sh is about 8k, tmpfs is 4k :-) - snap install test-snapd-sh - snap remove test-snapd-sh + # test-snapd-go-webserver is about 1MB, tmpfs is 256k :-) + snap install test-snapd-go-webserver + snap remove test-snapd-go-webserver diff --git a/tests/main/preseed/task.yaml b/tests/main/preseed/task.yaml index 6f8bc2d7dd0..a14d387074a 100644 --- a/tests/main/preseed/task.yaml +++ b/tests/main/preseed/task.yaml @@ -120,7 +120,7 @@ execute: | # the list of expected profiles isn't exhaustive, we're just checking some critical ones for prof in snap.lxd.lxd snap.lxd.hook.install snap.lxd.hook.configure snap.lxd.daemon; do test -f "$AA_PROFILES/$prof" - test -f "$SECCOMP_PROFILES/$prof.bin" + test -f "$SECCOMP_PROFILES/$prof.bin2" done echo "Checking that mount units have been created and enabled on the target image" diff --git a/tests/main/security-profiles/task.yaml b/tests/main/security-profiles/task.yaml index e2eb49936ff..164c22750be 100644 --- a/tests/main/security-profiles/task.yaml +++ b/tests/main/security-profiles/task.yaml @@ -17,7 +17,7 @@ execute: | for profile in snap.test-snapd-tools.block snap.test-snapd-tools.cat snap.test-snapd-tools.echo snap.test-snapd-tools.fail snap.test-snapd-tools.success do MATCH "^${profile} \\(enforce\\)$" <<<"$loaded_profiles" - [ -f "$seccomp_profile_directory/${profile}.bin" ] + [ -f "$seccomp_profile_directory/${profile}.bin2" ] done echo "Security profiles are generated and loaded for hooks" @@ -25,4 +25,4 @@ execute: | loaded_profiles=$(cat /sys/kernel/security/apparmor/profiles) echo "$loaded_profiles" | MATCH '^snap.basic-hooks.hook.configure \(enforce\)$' - [ -f "$seccomp_profile_directory/snap.basic-hooks.hook.configure.bin" ] + [ -f "$seccomp_profile_directory/snap.basic-hooks.hook.configure.bin2" ] diff --git a/tests/main/security-seccomp/task.yaml b/tests/main/security-seccomp/task.yaml index 85df4699005..afb28e59dd3 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.bin2 AAP: /var/lib/snapd/apparmor/profiles/snap.test-snapd-setpriority.test-snapd-setpriority prepare: | @@ -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\)' diff --git a/tests/main/snap-seccomp/task.yaml b/tests/main/snap-seccomp/task.yaml index b81a663bd91..dd6c475545c 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 @@ -16,26 +14,30 @@ execute: | snap install test-snapd-sh test-snapd-sh.sh -c 'echo hello' | MATCH hello - # FIXME: use dirs.sh in 2.27+ - echo "Ensure snap-seccomp is statically linked" - if ldd /usr/lib/snapd/snap-seccomp | MATCH libseccomp ; then - echo "found dynamically linked libseccomp, we need a staticly linked one" - exit 1 + # The 16.04 build is static as it goes into the snapd snap but 14.04 + # is dynamically linked. + if ! os.query is-trusty; then + echo "Ensure snap-seccomp is statically linked" + # FIXME: use dirs.sh in 2.27+ + if ldd /usr/lib/snapd/snap-seccomp | MATCH libseccomp ; then + echo "found dynamically linked libseccomp, we need a staticly linked one" + exit 1 + fi fi # from the old test_complain echo "Test that the @complain keyword works" - rm -f "${PROFILE}.bin" + rm -f "${PROFILE}.bin2" 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 - echo "$output" | MATCH "cannot apply seccomp profile: Invalid argument" + echo "$output" | MATCH "unexpected seccomp header: .*" - echo "Add huge snapd.test-snapd-sh.bin to ensure size limit works" - dd if=/dev/zero of="${PROFILE}.bin" count=50 bs=1M + echo "Add huge snapd.test-snapd-sh filters to ensure size limit works" + dd if=/dev/zero of="${PROFILE}.bin2" 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 - echo "$output" | MATCH "cannot fit .* to memory buffer" + # TODO: adjust the test so that the header is valid and the profile big + #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 -f "${PROFILE}.bin2" 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}.bin2" echo "" > "${PROFILE}.src" - $SNAP_SECCOMP compile "${PROFILE}.src" "${PROFILE}.bin" + $SNAP_SECCOMP compile "${PROFILE}.src" "${PROFILE}.bin2" 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 -f "${PROFILE}.bin2" cat >"${PROFILE}.src" <