From 5cfb91d505d7345b51748688bd17753f46775a0d 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: warn on amb caps when inh not set

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 had 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 | 34 +++++++++++++++++++----
 tests/integration/capabilities.bats       |  8 ++++++
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/libcontainer/capabilities/capabilities.go b/libcontainer/capabilities/capabilities.go
index 7928c340848..8082b4f01fe 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,48 @@ 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))
+		clear(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/tests/integration/capabilities.bats b/tests/integration/capabilities.bats
index acb3a2739ef..629e764e177 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 = {}'