From ab8480893ee1402a959d59b2684c2a49c25a23bd Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 1 Sep 2022 16:05:04 -0700 Subject: [PATCH 1/4] types/features: fix docstrings Fix a few copy-paste errors. Fixes: 520702dac Signed-off-by: Kir Kolyshkin --- types/features/features.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/types/features/features.go b/types/features/features.go index c6269ca6306..b9371daaa00 100644 --- a/types/features/features.go +++ b/types/features/features.go @@ -53,11 +53,11 @@ type Seccomp struct { // Nil value means "unknown", not "no support for any action". Actions []string `json:"actions,omitempty"` - // Operators is the list of the recognized actions, e.g., "SCMP_CMP_NE". + // Operators is the list of the recognized operators, e.g., "SCMP_CMP_NE". // Nil value means "unknown", not "no support for any operator". Operators []string `json:"operators,omitempty"` - // Operators is the list of the recognized archs, e.g., "SCMP_ARCH_X86_64". + // Archs is the list of the recognized archs, e.g., "SCMP_ARCH_X86_64". // Nil value means "unknown", not "no support for any arch". Archs []string `json:"archs,omitempty"` } From 076745a40fb6b8d84d2a31f038e987b1fdbc4b8c Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 2 Sep 2022 12:20:54 -0700 Subject: [PATCH 2/4] runc features: add seccomp filter flags Amend runc features to print seccomp flags. Two set of flags are added: * known flags are those that this version of runc is aware of; * supported flags are those that can be set; normally, this is the same set as known flags, but due to older version of kernel and/or libseccomp, some known flags might be unsupported. This commit also consolidates three different switch statements dealing with flags into one, in func setFlag. A note is added to this function telling what else to look for when adding new flags. Unfortunately, it also adds a list of known flags, that should be kept in sync with the switch statement. Signed-off-by: Kir Kolyshkin --- features.go | 10 ++- libcontainer/seccomp/config.go | 37 ++++++++ libcontainer/seccomp/patchbpf/enosys_linux.go | 1 + libcontainer/seccomp/seccomp_linux.go | 84 ++++++++++++++----- libcontainer/seccomp/seccomp_unsupported.go | 6 ++ libcontainer/specconv/spec_linux.go | 10 +-- types/features/features.go | 10 +++ 7 files changed, 127 insertions(+), 31 deletions(-) diff --git a/features.go b/features.go index c9cd15cd09d..c86adc0a266 100644 --- a/features.go +++ b/features.go @@ -59,10 +59,12 @@ var featuresCommand = cli.Command{ if seccomp.Enabled { feat.Linux.Seccomp = &features.Seccomp{ - Enabled: &tru, - Actions: seccomp.KnownActions(), - Operators: seccomp.KnownOperators(), - Archs: seccomp.KnownArchs(), + Enabled: &tru, + Actions: seccomp.KnownActions(), + Operators: seccomp.KnownOperators(), + Archs: seccomp.KnownArchs(), + KnownFlags: seccomp.KnownFlags(), + SupportedFlags: seccomp.SupportedFlags(), } major, minor, patch := seccomp.Version() feat.Annotations[features.AnnotationLibseccompVersion] = fmt.Sprintf("%d.%d.%d", major, minor, patch) diff --git a/libcontainer/seccomp/config.go b/libcontainer/seccomp/config.go index 2b15576ac90..3ca03ed8a30 100644 --- a/libcontainer/seccomp/config.go +++ b/libcontainer/seccomp/config.go @@ -5,8 +5,13 @@ import ( "sort" "github.com/opencontainers/runc/libcontainer/configs" + "github.com/opencontainers/runtime-spec/specs-go" ) +// flagTsync is recognized but ignored by runc, and it is not defined +// in the runtime-spec. +const flagTsync = "SECCOMP_FILTER_FLAG_TSYNC" + var operators = map[string]configs.Operator{ "SCMP_CMP_NE": configs.NotEqualTo, "SCMP_CMP_LT": configs.LessThan, @@ -111,3 +116,35 @@ func ConvertStringToArch(in string) (string, error) { } return "", fmt.Errorf("string %s is not a valid arch for seccomp", in) } + +// List of flags known to this version of runc. +var flags = []string{ + flagTsync, + string(specs.LinuxSeccompFlagSpecAllow), + string(specs.LinuxSeccompFlagLog), +} + +// KnownFlags returns the list of the known filter flags. +// Used by `runc features`. +func KnownFlags() []string { + return flags +} + +// SupportedFlags returns the list of the supported filter flags. +// This list may be a subset of one returned by KnownFlags due to +// some flags not supported by the current kernel and/or libseccomp. +// Used by `runc features`. +func SupportedFlags() []string { + if !Enabled { + return nil + } + + var res []string + for _, flag := range flags { + if FlagSupported(specs.LinuxSeccompFlag(flag)) == nil { + res = append(res, flag) + } + } + + return res +} diff --git a/libcontainer/seccomp/patchbpf/enosys_linux.go b/libcontainer/seccomp/patchbpf/enosys_linux.go index 1427f261d93..7fc9fd662c3 100644 --- a/libcontainer/seccomp/patchbpf/enosys_linux.go +++ b/libcontainer/seccomp/patchbpf/enosys_linux.go @@ -643,6 +643,7 @@ func filterFlags(config *configs.Seccomp, filter *libseccomp.ScmpFilter) (flags flags |= uint(C.C_FILTER_FLAG_SPEC_ALLOW) } } + // XXX: add newly supported filter flags above this line. for _, call := range config.Syscalls { if call.Action == configs.Notify { diff --git a/libcontainer/seccomp/seccomp_linux.go b/libcontainer/seccomp/seccomp_linux.go index 0cdb2f561fe..ffa79cdf105 100644 --- a/libcontainer/seccomp/seccomp_linux.go +++ b/libcontainer/seccomp/seccomp_linux.go @@ -87,27 +87,10 @@ func InitSeccomp(config *configs.Seccomp) (int, error) { } } - // Add extra flags + // Add extra flags. for _, flag := range config.Flags { - switch flag { - case "SECCOMP_FILTER_FLAG_TSYNC": - // 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) - } - // NOTE when adding more flags, make sure to also modify filterFlags in patchbpf. - default: - return -1, fmt.Errorf("seccomp flags %q not yet supported by runc", flag) + if err := setFlag(filter, flag); err != nil { + return -1, err } } @@ -149,6 +132,67 @@ func InitSeccomp(config *configs.Seccomp) (int, error) { return seccompFd, nil } +type unknownFlagError struct { + flag specs.LinuxSeccompFlag +} + +func (e *unknownFlagError) Error() string { + return "seccomp flag " + string(e.flag) + " is not known to runc" +} + +func setFlag(filter *libseccomp.ScmpFilter, flag specs.LinuxSeccompFlag) error { + switch flag { + case flagTsync: + // 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. + return nil + case specs.LinuxSeccompFlagLog: + if err := filter.SetLogBit(true); err != nil { + return fmt.Errorf("error adding log flag to seccomp filter: %w", err) + } + return nil + case specs.LinuxSeccompFlagSpecAllow: + if err := filter.SetSSB(true); err != nil { + return fmt.Errorf("error adding SSB flag to seccomp filter: %w", err) + } + return nil + } + // NOTE when adding more flags above, do not forget to also: + // - add new flags to `flags` slice in config.go; + // - add new flags to tests/integration/seccomp.bats flags test; + // - modify func filterFlags in patchbpf/ accordingly. + + return &unknownFlagError{flag: flag} +} + +// FlagSupported checks if the flag is known to runc and supported by +// currently used libseccomp and kernel (i.e. it can be set). +func FlagSupported(flag specs.LinuxSeccompFlag) error { + filter := &libseccomp.ScmpFilter{} + err := setFlag(filter, flag) + + // For flags we don't know, setFlag returns unknownFlagError. + var uf *unknownFlagError + if errors.As(err, &uf) { + return err + } + // For flags that are known to runc and libseccomp-golang but can not + // be applied because either libseccomp or the kernel is too old, + // seccomp.VersionError is returned. + var verErr *libseccomp.VersionError + if errors.As(err, &verErr) { + // Not supported by libseccomp or the kernel. + return err + } + + // All other flags are known and supported. + return nil +} + // Convert Libcontainer Action to Libseccomp ScmpAction func getAction(act configs.Action, errnoRet *uint) (libseccomp.ScmpAction, error) { switch act { diff --git a/libcontainer/seccomp/seccomp_unsupported.go b/libcontainer/seccomp/seccomp_unsupported.go index be2b324e057..885529dc7d0 100644 --- a/libcontainer/seccomp/seccomp_unsupported.go +++ b/libcontainer/seccomp/seccomp_unsupported.go @@ -7,6 +7,7 @@ import ( "errors" "github.com/opencontainers/runc/libcontainer/configs" + "github.com/opencontainers/runtime-spec/specs-go" ) var ErrSeccompNotEnabled = errors.New("seccomp: config provided but seccomp not supported") @@ -19,6 +20,11 @@ func InitSeccomp(config *configs.Seccomp) (int, error) { return -1, nil } +// FlagSupported tells if a provided seccomp flag is supported. +func FlagSupported(_ specs.LinuxSeccompFlag) error { + return ErrSeccompNotEnabled +} + // Version returns major, minor, and micro. func Version() (uint, uint, uint) { return 0, 0, 0 diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index 4b32f286e44..b4a001320b8 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/libcontainer/specconv/spec_linux.go @@ -1026,14 +1026,10 @@ func SetupSeccomp(config *specs.LinuxSeccomp) (*configs.Seccomp, error) { // 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 { - 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 err := seccomp.FlagSupported(flag); err != nil { + return nil, err } + newConfig.Flags = append(newConfig.Flags, flag) } if len(config.Architectures) > 0 { diff --git a/types/features/features.go b/types/features/features.go index b9371daaa00..4ea629eeaf4 100644 --- a/types/features/features.go +++ b/types/features/features.go @@ -60,6 +60,16 @@ type Seccomp struct { // Archs is the list of the recognized archs, e.g., "SCMP_ARCH_X86_64". // Nil value means "unknown", not "no support for any arch". Archs []string `json:"archs,omitempty"` + + // KnownFlags is the list of the recognized filter flags, e.g., "SECCOMP_FILTER_FLAG_LOG". + // Nil value means "unknown", not "no flags are recognized". + KnownFlags []string `json:"knownFlags,omitempty"` + + // SupportedFlags is the list of the supported filter flags, e.g., "SECCOMP_FILTER_FLAG_LOG". + // This list may be a subset of KnownFlags due to some flags + // not supported by the current kernel and/or libseccomp. + // Nil value means "unknown", not "no flags are supported". + SupportedFlags []string `json:"supportedFlags,omitempty"` } // Apparmor represents the "apparmor" field. From ac04154f0b136ad3f8636acb8ed29431b7bddd83 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 30 Aug 2022 12:05:02 -0700 Subject: [PATCH 3/4] seccomp: set SPEC_ALLOW by default If no seccomps flags are set in OCI runtime spec (not even the empty set), set SPEC_ALLOW as the default (if it's supported). Otherwise, use the flags as they are set (that includes no flags for empty seccomp.Flags array). This mimics the crun behavior, and makes runc seccomp performance on par with crun. Signed-off-by: Kir Kolyshkin --- libcontainer/specconv/spec_linux.go | 20 +++++++++++++++----- tests/integration/seccomp.bats | 2 +- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index b4a001320b8..0d53b20275f 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/libcontainer/specconv/spec_linux.go @@ -1024,12 +1024,22 @@ func SetupSeccomp(config *specs.LinuxSeccomp) (*configs.Seccomp, error) { 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 { - if err := seccomp.FlagSupported(flag); err != nil { - return nil, err + // in the seccomp() syscall. + if config.Flags == nil { + // No flags are set explicitly (not even the empty set); + // set the default of specs.LinuxSeccompFlagSpecAllow, + // if it is supported by the libseccomp and the kernel. + if err := seccomp.FlagSupported(specs.LinuxSeccompFlagSpecAllow); err == nil { + newConfig.Flags = []specs.LinuxSeccompFlag{specs.LinuxSeccompFlagSpecAllow} + } + } else { + // Fail early if some flags are unknown or unsupported. + for _, flag := range config.Flags { + if err := seccomp.FlagSupported(flag); err != nil { + return nil, err + } + newConfig.Flags = append(newConfig.Flags, flag) } - newConfig.Flags = append(newConfig.Flags, flag) } if len(config.Architectures) > 0 { diff --git a/tests/integration/seccomp.bats b/tests/integration/seccomp.bats index 2babf69047d..b0fdf85c345 100644 --- a/tests/integration/seccomp.bats +++ b/tests/integration/seccomp.bats @@ -80,7 +80,7 @@ function teardown() { }' declare -A FLAGS=( - ['REMOVE']=0 # No setting, use built-in default. + ['REMOVE']=4 # No setting, use built-in default. ['EMPTY']=0 # Empty set of flags. ['"SECCOMP_FILTER_FLAG_LOG"']=2 ['"SECCOMP_FILTER_FLAG_SPEC_ALLOW"']=4 From 19a9d9fc9e3530f483a7f121ae28f3f5764d8522 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 27 Sep 2022 18:33:32 -0700 Subject: [PATCH 4/4] tests/int: use runc features in seccomp flags test This test (initially added by commit 58ea21daefea8e3447d and later amended in commit 26dc55ef1a56ea0279) currently has two major deficiencies: 1. All possible flag combinations, and their respective numeric values, have to be explicitly listed. Currently we support 3 flags, so there is only 2^3 - 1 = 7 combinations, but adding more flags will become increasingly difficult (for example, 5 flags will result in 31 combinations). 2. The test requires kernel 4.17 (for SECCOMP_FILTER_FLAG_SPEC_ALLOW), and not doing any tests when running on an older kernel. This, too, will make it more difficult to add extra flags in the future. Both issues can be solved by using runc features which now prints all known and supported runc flags. We still have to hardcode the numeric values of all flags, but most of the other work is coded now. In particular: * The test only uses supported flags, meaning it can be used with older kernels, removing the limitation (2) above. * The test calculates the powerset (all possible combinations) of flags and their numeric values. This makes it easier to add more flags, removing the limitation (1) above. * The test will fail (in flags_value) if any new flags will be added to runc but the test itself is not amended. Signed-off-by: Kir Kolyshkin --- libcontainer/seccomp/seccomp_linux.go | 2 +- tests/integration/seccomp.bats | 70 +++++++++++++++++++++------ 2 files changed, 55 insertions(+), 17 deletions(-) diff --git a/libcontainer/seccomp/seccomp_linux.go b/libcontainer/seccomp/seccomp_linux.go index ffa79cdf105..fed02bcedc4 100644 --- a/libcontainer/seccomp/seccomp_linux.go +++ b/libcontainer/seccomp/seccomp_linux.go @@ -163,7 +163,7 @@ func setFlag(filter *libseccomp.ScmpFilter, flag specs.LinuxSeccompFlag) error { } // NOTE when adding more flags above, do not forget to also: // - add new flags to `flags` slice in config.go; - // - add new flags to tests/integration/seccomp.bats flags test; + // - add new flag values to flags_value() in tests/integration/seccomp.bats; // - modify func filterFlags in patchbpf/ accordingly. return &unknownFlagError{flag: flag} diff --git a/tests/integration/seccomp.bats b/tests/integration/seccomp.bats index b0fdf85c345..897c7ca8357 100644 --- a/tests/integration/seccomp.bats +++ b/tests/integration/seccomp.bats @@ -66,11 +66,32 @@ 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 +# Prints the numeric value of provided seccomp flags combination. +# The parameter is flags string, as supplied in OCI spec, for example +# '"SECCOMP_FILTER_FLAG_TSYNC","SECCOMP_FILTER_FLAG_LOG"'. +function flags_value() { + # Numeric values of seccomp flags. + declare -A values=( + ['"SECCOMP_FILTER_FLAG_TSYNC"']=0 # Supported but ignored by runc, thus 0. + ['"SECCOMP_FILTER_FLAG_LOG"']=2 + ['"SECCOMP_FILTER_FLAG_SPEC_ALLOW"']=4 + # XXX: add new values above this line. + ) + # Split the flags. + IFS=',' read -ra flags <<<"$1" + + local flag v sum=0 + for flag in "${flags[@]}"; do + # This will produce "values[$flag]: unbound variable" + # error for a new flag yet unknown to the test. + v=${values[$flag]} + ((sum += v)) || true + done + + echo $sum +} +@test "runc run [seccomp] (SECCOMP_FILTER_FLAG_*)" { update_config ' .process.args = ["/bin/sh", "-c", "mkdir /dev/shm/foo"] | .process.noNewPrivileges = false | .linux.seccomp = { @@ -79,18 +100,35 @@ function teardown() { "syscalls":[{"names":["mkdir", "mkdirat"], "action":"SCMP_ACT_ERRNO"}] }' - declare -A FLAGS=( - ['REMOVE']=4 # No setting, use built-in default. - ['EMPTY']=0 # Empty set of flags. - ['"SECCOMP_FILTER_FLAG_LOG"']=2 - ['"SECCOMP_FILTER_FLAG_SPEC_ALLOW"']=4 - ['"SECCOMP_FILTER_FLAG_TSYNC"']=0 # tsync flag is ignored. - ['"SECCOMP_FILTER_FLAG_LOG","SECCOMP_FILTER_FLAG_SPEC_ALLOW"']=6 - ['"SECCOMP_FILTER_FLAG_LOG","SECCOMP_FILTER_FLAG_TSYNC"']=2 - ['"SECCOMP_FILTER_FLAG_SPEC_ALLOW","SECCOMP_FILTER_FLAG_TSYNC"']=4 - ['"SECCOMP_FILTER_FLAG_LOG","SECCOMP_FILTER_FLAG_SPEC_ALLOW","SECCOMP_FILTER_FLAG_TSYNC"']=6 + # Get the list of flags supported by runc/seccomp/kernel, + # or "null" if no flags are supported or runc is too old. + mapfile -t flags < <(__runc features | jq -c '.linux.seccomp.supportedFlags' | + tr -d '[]\n' | tr ',' '\n') + + # This is a set of all possible flag combinations to test. + declare -A TEST_CASES=( + ['EMPTY']=0 # Special value: empty set of flags. + ['REMOVE']=0 # Special value: no flags set. ) - for key in "${!FLAGS[@]}"; do + + # If supported, runc should set SPEC_ALLOW if no flags are set. + if [[ " ${flags[*]} " == *' "SECCOMP_FILTER_FLAG_SPEC_ALLOW" '* ]]; then + TEST_CASES['REMOVE']=$(flags_value '"SECCOMP_FILTER_FLAG_SPEC_ALLOW"') + fi + + # Add all possible combinations of seccomp flags + # and their expected numeric values to TEST_CASES. + if [ "${flags[0]}" != "null" ]; then + # Use shell {a,}{b,}{c,} to generate the powerset. + for fc in $(eval echo "$(printf "{'%s,',}" "${flags[@]}")"); do + # Remove the last comma. + fc="${fc/%,/}" + TEST_CASES[$fc]=$(flags_value "$fc") + done + fi + + # Finally, run the tests. + for key in "${!TEST_CASES[@]}"; do case "$key" in 'REMOVE') update_config ' del(.linux.seccomp.flags)' @@ -108,7 +146,7 @@ function teardown() { [[ "$output" == *"mkdir:"*"/dev/shm/foo"*"Operation not permitted"* ]] # Check the numeric flags value, as printed in the debug log, is as expected. - exp="\"seccomp filter flags: ${FLAGS[$key]}\"" + exp="\"seccomp filter flags: ${TEST_CASES[$key]}\"" echo "flags $key, expecting $exp" [[ "$output" == *"$exp"* ]] done