Merge pull request #1616 from mheon/seccomp_fix_breakage

Fix breaking change in Seccomp profile behavior

Author: Mrunal Patel

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")
+	}
+}

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)
+			}
 		}
 	}