Skip to content

Fix breaking change in Seccomp profile behavior #1616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mrunalp merged 1 commit into from
Oct 19, 2017
Merged

Fix breaking change in Seccomp profile behavior #1616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 96 additions & 0 deletions libcontainer/integration/seccomp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
}
47 changes: 39 additions & 8 deletions libcontainer/seccomp/seccomp_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
}
}

Expand Down Expand Up @@ -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 {
Copy link

@TomSweeneyRedHat TomSweeneyRedHat Oct 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, I might be missing a golang nuance, but could you just do "len(argCounts) > 1"?

Copy link
Contributor Author

@mheon mheon Oct 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

len(argCounts) should always be 6, as it's a 6-long array of uints, one for each argument possible in a Linux sycall. Each one holds the number of conditions that check that specific argument. If we had reduce in Go, I could make this a one-liner, but as things are needs to check each individual field in the array so it needs to iterate.

Copy link

@TomSweeneyRedHat TomSweeneyRedHat Oct 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see the 6 now at line 184. Missed that, was thinking this was dynamically allocated. Thanks for the 411.

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

Expand Down