From d91f77cb004f0ad4001f0d2b38b64e8e158f65f7 Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Thu, 1 Aug 2024 01:01:00 -0700
Subject: [PATCH] libct: fix wrt ambient vs inheritable capabilities

Fixing a long standing bug in github.com/syndtr/gocapability package
(ignoring errors when setting ambient caps, see [1]) revealed that
it's not possible to raise those ambient capabilities for which
inheritable capabilities are not raised. In other words, "the Ambient
vector cannot contain values not raised in the Inh vector" ([2]).

The example spec in libct/specconv has a few ambient capabilities set
but no inheritable ones. As a result, when capability package with fix
from [1] is used, we get an error trying to start a container ("unable
to apply caps: permission denied").

The only decent way to fix this is to ignore raised ambient capabilities
for which inheritable capabilities are not raised (essentially mimicking
the old behavior). Let's also add a warning about ignored capabilities.

Fix the example spec (remove the ambient caps).

This is in preparation to switch to github.com/kolyshkin/capability.

[1]: https://github.com/kolyshkin/capability/pull/3
[2]: https://pkg.go.dev/kernel.org/pub/linux/libs/security/libcap/cap#IAB.SetVector
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 libcontainer/capabilities/capabilities.go | 33 ++++++++++++++++++-----
 libcontainer/specconv/example.go          |  5 ----
 tests/integration/capabilities.bats       |  8 ++++++
 3 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/libcontainer/capabilities/capabilities.go b/libcontainer/capabilities/capabilities.go
index 7928c340848..1111aac35fb 100644
--- a/libcontainer/capabilities/capabilities.go
+++ b/libcontainer/capabilities/capabilities.go
@@ -3,6 +3,7 @@
 package capabilities
 
 import (
+	"slices"
 	"sort"
 	"strings"
 	"sync"
@@ -57,27 +58,47 @@ func New(capConfig *configs.Capabilities) (*Caps, error) {
 
 	cm := capMap()
 	unknownCaps := make(map[string]struct{})
+	ignoredCaps := make(map[string]struct{})
 	// capSlice converts the slice of capability names in caps, to their numeric
 	// equivalent, and returns them as a slice. Unknown or unavailable capabilities
 	// are not returned, but appended to unknownCaps.
-	capSlice := func(caps []string) []capability.Cap {
+	//
+	// If mustHave argument is not nil, caps that are not present in mustHave
+	// are appended to ignoredCaps instead of the resulting slice.
+	capSlice := func(caps []string, mustHave []capability.Cap) []capability.Cap {
 		out := make([]capability.Cap, 0, len(caps))
 		for _, c := range caps {
 			if v, ok := cm[c]; !ok {
 				unknownCaps[c] = struct{}{}
 			} else {
+				if mustHave != nil && !slices.Contains(mustHave, v) {
+					ignoredCaps[c] = struct{}{}
+					continue
+				}
 				out = append(out, v)
 			}
 		}
 		return out
 	}
+	inheritable := capSlice(capConfig.Inheritable, nil)
+	// Ambient vector can not contain values not raised in the Inheritable vector,
+	// and errors setting ambient capabilities were previously ignored due to a bug
+	// (see https://github.com/kolyshkin/capability/pull/3), so to maintain backward
+	// compatibility we have to ignore those Ambient caps that are not also raised
+	// in Inheritable (and issue a warning).
+	ambient := capSlice(capConfig.Ambient, inheritable)
+	if len(ignoredCaps) > 0 {
+		logrus.Warn("unable to set Ambient capabilities which are not set in Inheritable; ignoring following Ambient capabilities: ", mapKeys(ignoredCaps))
+	}
+
 	c.caps = map[capability.CapType][]capability.Cap{
-		capability.BOUNDING:    capSlice(capConfig.Bounding),
-		capability.EFFECTIVE:   capSlice(capConfig.Effective),
-		capability.INHERITABLE: capSlice(capConfig.Inheritable),
-		capability.PERMITTED:   capSlice(capConfig.Permitted),
-		capability.AMBIENT:     capSlice(capConfig.Ambient),
+		capability.BOUNDING:    capSlice(capConfig.Bounding, nil),
+		capability.EFFECTIVE:   capSlice(capConfig.Effective, nil),
+		capability.INHERITABLE: inheritable,
+		capability.PERMITTED:   capSlice(capConfig.Permitted, nil),
+		capability.AMBIENT:     ambient,
 	}
+
 	if c.pid, err = capability.NewPid2(0); err != nil {
 		return nil, err
 	}
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",
diff --git a/tests/integration/capabilities.bats b/tests/integration/capabilities.bats
index aa9a3726268..ded015d381c 100644
--- a/tests/integration/capabilities.bats
+++ b/tests/integration/capabilities.bats
@@ -54,6 +54,14 @@ function teardown() {
 	[[ "${output}" == *"NoNewPrivs:	1"* ]]
 }
 
+@test "runc run [ambient caps not set in inheritable result in a warning]" {
+	update_config '	  .process.capabilities.inheritable = ["CAP_KILL"]
+			| .process.capabilities.ambient = ["CAP_KILL", "CAP_AUDIT_WRITE"]'
+	runc run test_amb
+	[ "$status" -eq 0 ]
+	[[ "$output" == **"ignoring following Ambient capabilities: [CAP_AUDIT_WRITE]"* ]]
+}
+
 @test "runc exec --cap" {
         update_config '	  .process.args = ["/bin/sh"]
 			| .process.capabilities = {}'