From e9193ba6e612c13c064449ee4189b268d1a9c94c Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 16 Oct 2017 16:27:40 -0400 Subject: [PATCH] Fix breaking change in Seccomp profile behavior Multiple conditions were previously allowed to be placed upon the same syscall argument. Restore this behavior. Signed-off-by: Matthew Heon --- libcontainer/integration/seccomp_test.go | 96 ++++++++++++++++++++++++ libcontainer/seccomp/seccomp_linux.go | 47 ++++++++++-- 2 files changed, 135 insertions(+), 8 deletions(-) diff --git a/libcontainer/integration/seccomp_test.go b/libcontainer/integration/seccomp_test.go index 767a149d..3d80032e 100644 --- a/libcontainer/integration/seccomp_test.go +++ b/libcontainer/integration/seccomp_test.go @@ -321,3 +321,99 @@ func TestSeccompDenyWriteMultipleConditions(t *testing.T) { t.Fatalf("Expected output %s but got %s\n", expected, actual) } } + +func TestSeccompMultipleConditionSameArgDeniesStdout(t *testing.T) { + if testing.Short() { + return + } + + rootfs, err := newRootfs() + if err != nil { + t.Fatal(err) + } + defer remove(rootfs) + + // Prevent writing to both stdout and stderr + config := newTemplateConfig(rootfs) + config.Seccomp = &configs.Seccomp{ + DefaultAction: configs.Allow, + Syscalls: []*configs.Syscall{ + { + Name: "write", + Action: configs.Errno, + Args: []*configs.Arg{ + { + Index: 0, + Value: 1, + Op: configs.EqualTo, + }, + { + Index: 0, + Value: 2, + Op: configs.EqualTo, + }, + }, + }, + }, + } + + buffers, exitCode, err := runContainer(config, "", "ls", "/") + if err != nil { + t.Fatalf("%s: %s", buffers, err) + } + if exitCode != 0 { + t.Fatalf("exit code not 0. code %d buffers %s", exitCode, buffers) + } + // Verify that nothing was printed + if len(buffers.Stdout.String()) != 0 { + t.Fatalf("Something was written to stdout, write call succeeded!\n") + } +} + +func TestSeccompMultipleConditionSameArgDeniesStderr(t *testing.T) { + if testing.Short() { + return + } + + rootfs, err := newRootfs() + if err != nil { + t.Fatal(err) + } + defer remove(rootfs) + + // Prevent writing to both stdout and stderr + config := newTemplateConfig(rootfs) + config.Seccomp = &configs.Seccomp{ + DefaultAction: configs.Allow, + Syscalls: []*configs.Syscall{ + { + Name: "write", + Action: configs.Errno, + Args: []*configs.Arg{ + { + Index: 0, + Value: 1, + Op: configs.EqualTo, + }, + { + Index: 0, + Value: 2, + Op: configs.EqualTo, + }, + }, + }, + }, + } + + buffers, exitCode, err := runContainer(config, "", "ls", "/does_not_exist") + if err == nil { + t.Fatalf("Expecting error return, instead got 0") + } + if exitCode == 0 { + t.Fatalf("Busybox should fail with negative exit code, instead got %d!", exitCode) + } + // Verify nothing was printed + if len(buffers.Stderr.String()) != 0 { + t.Fatalf("Something was written to stderr, write call succeeded!\n") + } +} diff --git a/libcontainer/seccomp/seccomp_linux.go b/libcontainer/seccomp/seccomp_linux.go index 2523cbf9..d99f3fe6 100644 --- a/libcontainer/seccomp/seccomp_linux.go +++ b/libcontainer/seccomp/seccomp_linux.go @@ -22,6 +22,11 @@ var ( actErrno = libseccomp.ActErrno.SetReturnCode(int16(unix.EPERM)) ) +const ( + // Linux system calls can have at most 6 arguments + syscallMaxArguments int = 6 +) + // Filters given syscalls in a container, preventing them from being used // Started in the container init process, and carried over to all child processes // Setns calls, however, require a separate invocation, as they are not children @@ -45,11 +50,11 @@ func InitSeccomp(config *configs.Seccomp) error { for _, arch := range config.Architectures { scmpArch, err := libseccomp.GetArchFromString(arch) if err != nil { - return err + return fmt.Errorf("error validating Seccomp architecture: %s", err) } if err := filter.AddArch(scmpArch); err != nil { - return err + return fmt.Errorf("error adding architecture to seccomp filter: %s", err) } } @@ -170,29 +175,55 @@ func matchCall(filter *libseccomp.ScmpFilter, call *configs.Syscall) error { // Convert the call's action to the libseccomp equivalent callAct, err := getAction(call.Action) if err != nil { - return err + return fmt.Errorf("action in seccomp profile is invalid: %s", err) } // Unconditional match - just add the rule if len(call.Args) == 0 { if err = filter.AddRule(callNum, callAct); err != nil { - return err + return fmt.Errorf("error adding seccomp filter rule for syscall %s: %s", call.Name, err) } } else { - // Conditional match - convert the per-arg rules into library format + // If two or more arguments have the same condition, + // Revert to old behavior, adding each condition as a separate rule + argCounts := make([]uint, syscallMaxArguments) conditions := []libseccomp.ScmpCondition{} for _, cond := range call.Args { newCond, err := getCondition(cond) if err != nil { - return err + return fmt.Errorf("error creating seccomp syscall condition for syscall %s: %s", call.Name, err) } + argCounts[cond.Index] += 1 + conditions = append(conditions, newCond) } - if err = filter.AddRuleConditional(callNum, callAct, conditions); err != nil { - return err + hasMultipleArgs := false + for _, count := range argCounts { + if count > 1 { + hasMultipleArgs = true + break + } + } + + if hasMultipleArgs { + // Revert to old behavior + // Add each condition attached to a separate rule + for _, cond := range conditions { + condArr := []libseccomp.ScmpCondition{cond} + + if err = filter.AddRuleConditional(callNum, callAct, condArr); err != nil { + return fmt.Errorf("error adding seccomp rule for syscall %s: %s", call.Name, err) + } + } + } else { + // No conditions share same argument + // Use new, proper behavior + if err = filter.AddRuleConditional(callNum, callAct, conditions); err != nil { + return fmt.Errorf("error adding seccomp rule for syscall %s: %s", call.Name, err) + } } }