Skip to content
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

Jump to bottom

seccomp: add support for flags #3390

Merged
merged 2 commits into from
Jul 29, 2022
Merged

seccomp: add support for flags #3390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c152e83
go.mod: update runtime-spec
alban Jul 19, 2022
58ea21d
seccomp: add support for flags
alban Feb 22, 2022
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
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ require (
github.com/godbus/dbus/v5 v5.1.0
github.com/moby/sys/mountinfo v0.6.2
github.com/mrunalp/fileutils v0.5.0
github.com/opencontainers/runtime-spec v1.0.3-0.20210326190908-1c3f411f0417
github.com/opencontainers/runtime-spec v1.0.3-0.20220718201635-a8106e99982b
github.com/opencontainers/selinux v1.10.1
github.com/seccomp/libseccomp-golang v0.10.0
github.com/sirupsen/logrus v1.8.1
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ github.com/moby/sys/mountinfo v0.6.2 h1:BzJjoreD5BMFNmD9Rus6gdd1pLuecOFPt8wC+Vyg
github.com/moby/sys/mountinfo v0.6.2/go.mod h1:IJb6JQeOklcdMU9F5xQ8ZALD+CUr5VlGpwtX+VE0rpI=
github.com/mrunalp/fileutils v0.5.0 h1:NKzVxiH7eSk+OQ4M+ZYW1K6h27RUV3MI6NUTsHhU6Z4=
github.com/mrunalp/fileutils v0.5.0/go.mod h1:M1WthSahJixYnrXQl/DFQuteStB1weuxD2QJNHXfbSQ=
github.com/opencontainers/runtime-spec v1.0.3-0.20210326190908-1c3f411f0417 h1:3snG66yBm59tKhhSPQrQ/0bCrv1LQbKt40LnUPiUxdc=
github.com/opencontainers/runtime-spec v1.0.3-0.20210326190908-1c3f411f0417/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0=
github.com/opencontainers/runtime-spec v1.0.3-0.20220718201635-a8106e99982b h1:udwtfS44rxYE/ViMLchHQBjfE60GZSB1arY7BFbyxLs=
github.com/opencontainers/runtime-spec v1.0.3-0.20220718201635-a8106e99982b/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0=
github.com/opencontainers/selinux v1.10.1 h1:09LIPVRP3uuZGQvgR+SgMSNBd1Eb3vlRbGqQpoHsF8w=
github.com/opencontainers/selinux v1.10.1/go.mod h1:2i0OySw99QjzBBQByd1Gr9gSjvuho1lHsJxIJ3gGbJI=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
Expand Down
13 changes: 7 additions & 6 deletions libcontainer/configs/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,13 @@ type IDMap struct {
// for syscalls. Additional architectures can be added by specifying them in
// Architectures.
type Seccomp struct {
DefaultAction Action `json:"default_action"`
Architectures []string `json:"architectures"`
Syscalls []*Syscall `json:"syscalls"`
DefaultErrnoRet *uint `json:"default_errno_ret"`
ListenerPath string `json:"listener_path,omitempty"`
ListenerMetadata string `json:"listener_metadata,omitempty"`
DefaultAction Action `json:"default_action"`
Architectures []string `json:"architectures"`
Flags []specs.LinuxSeccompFlag `json:"flags"`
Syscalls []*Syscall `json:"syscalls"`
DefaultErrnoRet *uint `json:"default_errno_ret"`
ListenerPath string `json:"listener_path,omitempty"`
ListenerMetadata string `json:"listener_metadata,omitempty"`
}

// Action is taken upon rule match in Seccomp
Expand Down
24 changes: 24 additions & 0 deletions libcontainer/seccomp/seccomp_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (

"github.com/opencontainers/runc/libcontainer/configs"
"github.com/opencontainers/runc/libcontainer/seccomp/patchbpf"
"github.com/opencontainers/runtime-spec/specs-go"
)

var (
Expand Down Expand Up @@ -86,6 +87,29 @@ func InitSeccomp(config *configs.Seccomp) (int, error) {
}
}

// Add extra flags
for _, flag := range config.Flags {
switch flag {
case "SECCOMP_FILTER_FLAG_TSYNC":
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: I think we should have them as consts in the runtime spec. I proposed that change in opencontainers/runtime-spec#1138

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that seccomp-golang sets this flag unconditionally, and we (runc/libcontainer/seccomp/patchbpf) do not set this at all (not sure if this was by design or just forgotten; @cyphar WDYT?)

From what I see, it seems that for runc it does not make sense to do so, as we execute runc init right after applying seccomp rules, and it does not matter much whether these rules were applied to all threads or not.

Copy link
Member

Choose a reason for hiding this comment

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

There was an issue with setting it in patchbpf, I can't remember the exact details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SECCOMP_FILTER_FLAG_TSYNC is not a flag on the bpf instructions but a flag for the seccomp() syscall, so I don't think patchbpf can do something with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

TSYNC is set by libseccomp-golang by default, and there's no way to remove it. So adding it here does not make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, OK, here we're ignoring it, which I guess is a good thing to do.

// libseccomp-golang always use filterAttrTsync when
// possible so all goroutines will receive the same
// rules, so there is nothing to do. It does not make
// sense to apply the seccomp filter on only one
// thread; other threads will be terminated after exec
// anyway.
case specs.LinuxSeccompFlagLog:
if err := filter.SetLogBit(true); err != nil {
return -1, fmt.Errorf("error adding log flag to seccomp filter: %w", err)
}
case specs.LinuxSeccompFlagSpecAllow:
if err := filter.SetSSB(true); err != nil {
return -1, fmt.Errorf("error adding SSB flag to seccomp filter: %w", err)
}
default:
return -1, fmt.Errorf("seccomp flags %q not yet supported by runc", flag)
}
}

// Unset no new privs bit
if err := filter.SetNoNewPrivsBit(false); err != nil {
return -1, fmt.Errorf("error setting no new privileges: %w", err)
Expand Down
18 changes: 13 additions & 5 deletions libcontainer/specconv/spec_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,14 +1016,22 @@ func SetupSeccomp(config *specs.LinuxSeccomp) (*configs.Seccomp, error) {
return nil, nil
}

// We don't currently support seccomp flags.
if len(config.Flags) != 0 {
return nil, errors.New("seccomp flags are not yet supported by runc")
}

newConfig := new(configs.Seccomp)
newConfig.Syscalls = []*configs.Syscall{}

// The list of flags defined in runtime-spec is a subset of the flags
// in the seccomp() syscall
for _, flag := range config.Flags {
switch flag {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add support for SECCOMP_FILTER_FLAG_SPEC_ALLOW as well as SECCOMP_FILTER_FLAG_TSYNC

Copy link
Contributor

Choose a reason for hiding this comment

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

SECCOMP_FILTER_FLAG_TSYNC is there already and it's on by default (in libseccomp-golang). How would it be supported? Do you mean "let's explicitly ignore it", as I see no other way.

case "SECCOMP_FILTER_FLAG_TSYNC":
// Tsync can be silently ignored
case specs.LinuxSeccompFlagLog, specs.LinuxSeccompFlagSpecAllow:
newConfig.Flags = append(newConfig.Flags, flag)
default:
return nil, fmt.Errorf("seccomp flag %q not yet supported by runc", flag)
}
}

if len(config.Architectures) > 0 {
newConfig.Architectures = []string{}
for _, arch := range config.Architectures {
Expand Down
32 changes: 32 additions & 0 deletions tests/integration/seccomp.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,38 @@ function teardown() {
[[ "$output" == *"Network is down"* ]]
}

@test "runc run [seccomp] (SECCOMP_FILTER_FLAG_*)" {
# Linux 4.14: SECCOMP_FILTER_FLAG_LOG
# Linux 4.17: SECCOMP_FILTER_FLAG_SPEC_ALLOW
requires_kernel 4.17
SECCOMP_FILTER_FLAGS=(
'' # no flag
'"SECCOMP_FILTER_FLAG_LOG"'
'"SECCOMP_FILTER_FLAG_SPEC_ALLOW"'
'"SECCOMP_FILTER_FLAG_TSYNC"'
'"SECCOMP_FILTER_FLAG_LOG","SECCOMP_FILTER_FLAG_SPEC_ALLOW"'
'"SECCOMP_FILTER_FLAG_LOG","SECCOMP_FILTER_FLAG_TSYNC"'
'"SECCOMP_FILTER_FLAG_SPEC_ALLOW","SECCOMP_FILTER_FLAG_TSYNC"'
'"SECCOMP_FILTER_FLAG_LOG","SECCOMP_FILTER_FLAG_SPEC_ALLOW","SECCOMP_FILTER_FLAG_TSYNC"'
)
for flags in "${SECCOMP_FILTER_FLAGS[@]}"; do
update_config ' .process.args = ["/bin/sh", "-c", "mkdir /dev/shm/foo"]
| .process.noNewPrivileges = false
| .linux.seccomp = {
"defaultAction":"SCMP_ACT_ALLOW",
"architectures":["SCMP_ARCH_X86","SCMP_ARCH_X32","SCMP_ARCH_X86_64","SCMP_ARCH_AARCH64","SCMP_ARCH_ARM"],
"flags":['"${flags}"'],
"syscalls":[{"names":["mkdir"], "action":"SCMP_ACT_ERRNO"}]
}'

# This test checks that the flags are accepted without errors but does
# not check they are effectively applied
runc run test_busybox
[ "$status" -ne 0 ]
[[ "$output" == *"mkdir:"*"/dev/shm/foo"*"Operation not permitted"* ]]
done
}

@test "runc run [seccomp] (SCMP_ACT_KILL)" {
update_config ' .process.args = ["/bin/sh", "-c", "mkdir /dev/shm/foo"]
| .process.noNewPrivileges = false
Expand Down
97 changes: 81 additions & 16 deletions vendor/github.com/opencontainers/runtime-spec/specs-go/config.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion vendor/modules.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ github.com/moby/sys/mountinfo
# github.com/mrunalp/fileutils v0.5.0
## explicit; go 1.13
github.com/mrunalp/fileutils
# github.com/opencontainers/runtime-spec v1.0.3-0.20210326190908-1c3f411f0417
# github.com/opencontainers/runtime-spec v1.0.3-0.20220718201635-a8106e99982b
## explicit
github.com/opencontainers/runtime-spec/specs-go
# github.com/opencontainers/selinux v1.10.1
Expand Down