From 3e3f96034f1d454835c27f0ef230f0d5af4adc16 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 1 Aug 2024 17:50:57 -0700 Subject: [PATCH 1/3] runc exec --cap: do not add capabilities to ambient Commit 98fe566c removed setting inheritable capabilities from runc exec --cap, but neglected to also remove ambient capabilities. An ambient capability could only be set if the same inheritable capability is set, so as a result of the above change ambient capabilities were not set (but due to a bug in gocapability package, those errors are never reported). Once we start using a library with the fix [1], that bug will become apparent. Alas, we do not have any tests for runc exec --cap, so add one. Yet, if some inheritable bits are already set from spec, let's set ambient to avoid a possible regression. Add a test case for that, too. [1]: https://github.com/kolyshkin/capability/pull/3 Fixes: 98fe566c ("runc: do not set inheritable capabilities") Co-authored-by: lifubang Signed-off-by: Kir Kolyshkin --- exec.go | 7 +++- tests/integration/capabilities.bats | 63 +++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/exec.go b/exec.go index ad8a369a5dd..1e47f6cfb6b 100644 --- a/exec.go +++ b/exec.go @@ -234,7 +234,12 @@ func getProcess(context *cli.Context, bundle string) (*specs.Process, error) { p.Capabilities.Bounding = append(p.Capabilities.Bounding, c) p.Capabilities.Effective = append(p.Capabilities.Effective, c) p.Capabilities.Permitted = append(p.Capabilities.Permitted, c) - p.Capabilities.Ambient = append(p.Capabilities.Ambient, c) + // Since ambient capabilities can't be set without inherritable, + // and runc exec --cap don't set inheritable, let's only set + // ambient if we already have some inheritable bits set from spec. + if p.Capabilities.Inheritable != nil { + p.Capabilities.Ambient = append(p.Capabilities.Ambient, c) + } } } // append the passed env variables diff --git a/tests/integration/capabilities.bats b/tests/integration/capabilities.bats index 968041223f7..5d1178b7c1a 100644 --- a/tests/integration/capabilities.bats +++ b/tests/integration/capabilities.bats @@ -53,3 +53,66 @@ function teardown() { [[ "${output}" == *"CapPrm: 0000000000200000"* ]] [[ "${output}" == *"NoNewPrivs: 1"* ]] } + +@test "runc exec --cap" { + update_config ' .process.args = ["/bin/sh"] + | .process.capabilities = {}' + runc run -d --console-socket "$CONSOLE_SOCKET" test_exec_cap + [ "$status" -eq 0 ] + + runc exec test_exec_cap cat /proc/self/status + [ "$status" -eq 0 ] + # Check no capabilities are set. + [[ "${output}" == *"CapInh: 0000000000000000"* ]] + [[ "${output}" == *"CapPrm: 0000000000000000"* ]] + [[ "${output}" == *"CapEff: 0000000000000000"* ]] + [[ "${output}" == *"CapBnd: 0000000000000000"* ]] + [[ "${output}" == *"CapAmb: 0000000000000000"* ]] + + runc exec --cap CAP_KILL --cap CAP_AUDIT_WRITE test_exec_cap cat /proc/self/status + [ "$status" -eq 0 ] + # Check capabilities are added into bounding/effective/permitted only, + # but not to inheritable or ambient. + # + # CAP_KILL is 5, the bit mask is 0x20 (1 << 5). + # CAP_AUDIT_WRITE is 26, the bit mask is 0x20000000 (1 << 26). + [[ "${output}" == *"CapInh: 0000000000000000"* ]] + [[ "${output}" == *"CapPrm: 0000000020000020"* ]] + [[ "${output}" == *"CapEff: 0000000020000020"* ]] + [[ "${output}" == *"CapBnd: 0000000020000020"* ]] + [[ "${output}" == *"CapAmb: 0000000000000000"* ]] +} + +@test "runc exec --cap [ambient is set from spec]" { + update_config ' .process.args = ["/bin/sh"] + | .process.capabilities.inheritable = ["CAP_CHOWN", "CAP_SYSLOG"] + | .process.capabilities.permitted = ["CAP_KILL", "CAP_CHOWN"] + | .process.capabilities.effective = ["CAP_KILL"] + | .process.capabilities.bounding = ["CAP_KILL", "CAP_CHOWN", "CAP_SYSLOG"] + | .process.capabilities.ambient = ["CAP_CHOWN"]' + runc run -d --console-socket "$CONSOLE_SOCKET" test_some_caps + [ "$status" -eq 0 ] + + runc exec test_some_caps cat /proc/self/status + [ "$status" -eq 0 ] + # Check that capabilities are as set in spec. + # + # CAP_CHOWN is 0, the bit mask is 0x1 (1 << 0) + # CAP_KILL is 5, the bit mask is 0x20 (1 << 5). + # CAP_SYSLOG is 34, the bit mask is 0x400000000 (1 << 34). + [[ "${output}" == *"CapInh: 0000000400000001"* ]] + [[ "${output}" == *"CapPrm: 0000000000000021"* ]] + [[ "${output}" == *"CapEff: 0000000000000021"* ]] + [[ "${output}" == *"CapBnd: 0000000400000021"* ]] + [[ "${output}" == *"CapAmb: 0000000000000001"* ]] + + # Check that if config.json has an inheritable capability set, + # runc exec --cap adds ambient capabilities. + runc exec --cap CAP_SYSLOG test_some_caps cat /proc/self/status + [ "$status" -eq 0 ] + [[ "${output}" == *"CapInh: 0000000400000001"* ]] + [[ "${output}" == *"CapPrm: 0000000400000021"* ]] + [[ "${output}" == *"CapEff: 0000000400000021"* ]] + [[ "${output}" == *"CapBnd: 0000000400000021"* ]] + [[ "${output}" == *"CapAmb: 0000000400000001"* ]] +} From 0de1953333143cdbc5df7cfc4b0c6bf91ee99709 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 6 Aug 2024 10:44:25 -0700 Subject: [PATCH 2/3] runc spec, libct/int: do not add ambient capabilities Commit 98fe566c removed inheritable capabilities from the example spec (used by runc spec) and from the libcontainer/integration test config, but neglected to also remove ambient capabilities. An ambient capability could only be set if the same inheritable capability is set, so as a result of the above change ambient capabilities were not set (but due to a bug in gocapability package, those errors are never reported). Once we start using a library with the fix [1], that bug will become apparent (both bats-based and libct/int tests will fail). [1]: https://github.com/kolyshkin/capability/pull/3 Fixes: 98fe566c ("runc: do not set inheritable capabilities") Signed-off-by: Kir Kolyshkin --- libcontainer/integration/template_test.go | 16 ---------------- libcontainer/specconv/example.go | 5 ----- 2 files changed, 21 deletions(-) diff --git a/libcontainer/integration/template_test.go b/libcontainer/integration/template_test.go index 63c46b28fae..473f601ed49 100644 --- a/libcontainer/integration/template_test.go +++ b/libcontainer/integration/template_test.go @@ -75,22 +75,6 @@ func newTemplateConfig(t *testing.T, p *tParam) *configs.Config { "CAP_KILL", "CAP_AUDIT_WRITE", }, - Ambient: []string{ - "CAP_CHOWN", - "CAP_DAC_OVERRIDE", - "CAP_FSETID", - "CAP_FOWNER", - "CAP_MKNOD", - "CAP_NET_RAW", - "CAP_SETGID", - "CAP_SETUID", - "CAP_SETFCAP", - "CAP_SETPCAP", - "CAP_NET_BIND_SERVICE", - "CAP_SYS_CHROOT", - "CAP_KILL", - "CAP_AUDIT_WRITE", - }, Effective: []string{ "CAP_CHOWN", "CAP_DAC_OVERRIDE", diff --git a/libcontainer/specconv/example.go b/libcontainer/specconv/example.go index 152d938a50a..1e9cfa2dbfe 100644 --- a/libcontainer/specconv/example.go +++ b/libcontainer/specconv/example.go @@ -41,11 +41,6 @@ func Example() *specs.Spec { "CAP_KILL", "CAP_NET_BIND_SERVICE", }, - Ambient: []string{ - "CAP_AUDIT_WRITE", - "CAP_KILL", - "CAP_NET_BIND_SERVICE", - }, Effective: []string{ "CAP_AUDIT_WRITE", "CAP_KILL", From 7a449109f3c854a6280d0d749d02116f4b64a9fd Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 1 Aug 2024 00:59:19 -0700 Subject: [PATCH 3/3] libct/README: simplify example, rm inheritable caps The example is too long since it lists too many capabilities. Simplify it, leaving only two capabilities. Also, remove ambient capabilities from the set. Inheritable capabilities were removed earlier by commit 98fe566c, but ambient capabilities can't be raised without inheritable ones. Fixes: 98fe566c Signed-off-by: Kir Kolyshkin --- libcontainer/README.md | 52 ------------------------------------------ 1 file changed, 52 deletions(-) diff --git a/libcontainer/README.md b/libcontainer/README.md index 50e9a1325b5..b1e7b0cd15c 100644 --- a/libcontainer/README.md +++ b/libcontainer/README.md @@ -59,66 +59,14 @@ config := &configs.Config{ Rootfs: "/your/path/to/rootfs", Capabilities: &configs.Capabilities{ Bounding: []string{ - "CAP_CHOWN", - "CAP_DAC_OVERRIDE", - "CAP_FSETID", - "CAP_FOWNER", - "CAP_MKNOD", - "CAP_NET_RAW", - "CAP_SETGID", - "CAP_SETUID", - "CAP_SETFCAP", - "CAP_SETPCAP", - "CAP_NET_BIND_SERVICE", - "CAP_SYS_CHROOT", "CAP_KILL", "CAP_AUDIT_WRITE", }, Effective: []string{ - "CAP_CHOWN", - "CAP_DAC_OVERRIDE", - "CAP_FSETID", - "CAP_FOWNER", - "CAP_MKNOD", - "CAP_NET_RAW", - "CAP_SETGID", - "CAP_SETUID", - "CAP_SETFCAP", - "CAP_SETPCAP", - "CAP_NET_BIND_SERVICE", - "CAP_SYS_CHROOT", "CAP_KILL", "CAP_AUDIT_WRITE", }, Permitted: []string{ - "CAP_CHOWN", - "CAP_DAC_OVERRIDE", - "CAP_FSETID", - "CAP_FOWNER", - "CAP_MKNOD", - "CAP_NET_RAW", - "CAP_SETGID", - "CAP_SETUID", - "CAP_SETFCAP", - "CAP_SETPCAP", - "CAP_NET_BIND_SERVICE", - "CAP_SYS_CHROOT", - "CAP_KILL", - "CAP_AUDIT_WRITE", - }, - Ambient: []string{ - "CAP_CHOWN", - "CAP_DAC_OVERRIDE", - "CAP_FSETID", - "CAP_FOWNER", - "CAP_MKNOD", - "CAP_NET_RAW", - "CAP_SETGID", - "CAP_SETUID", - "CAP_SETFCAP", - "CAP_SETPCAP", - "CAP_NET_BIND_SERVICE", - "CAP_SYS_CHROOT", "CAP_KILL", "CAP_AUDIT_WRITE", },