Merge pull request #27036 from sttts/sttts-secure-seccomp-path

Automatic merge from submit-queue

Filter seccomp profile path from malicious .. and /

Without this patch with `localhost/<some-releative-path>` as seccomp profile one can load any file on the host, e.g. `localhost/../../../../dev/mem` which is not healthy for the kubelet.

/cc @jfrazelle 

Unit tests depend on https://github.com/kubernetes/kubernetes/pull/26710.

pkg/api/helpers.go

@@ -416,6 +416,14 @@ const (
 	// TaintsAnnotationKey represents the key of taints data (json serialized)
 	// in the Annotations of a Node.
 	TaintsAnnotationKey string = "scheduler.alpha.kubernetes.io/taints"
+
+	// SeccompPodAnnotationKey represents the key of a seccomp profile applied
+	// to all containers of a pod.
+	SeccompPodAnnotationKey string = "seccomp.security.alpha.kubernetes.io/pod"
+
+	// SeccompContainerAnnotationKeyPrefix represents the key of a seccomp profile applied
+	// to one container of a pod.
+	SeccompContainerAnnotationKeyPrefix string = "container.seccomp.security.alpha.kubernetes.io/"
 )
 
 // GetAffinityFromPod gets the json serialized affinity data from Pod.Annotations

pkg/api/validation/validation.go

@@ -122,6 +122,8 @@ func ValidatePodSpecificAnnotations(annotations map[string]string, fldPath *fiel
 		}
 	}
 
+	allErrs = append(allErrs, ValidateSeccompPodAnnotations(annotations, fldPath)...)
+
 	return allErrs
 }
 
@@ -1846,6 +1848,33 @@ func ValidateTolerationsInPodAnnotations(annotations map[string]string, fldPath
 	return allErrs
 }
 
+func validateSeccompProfile(p string, fldPath *field.Path) field.ErrorList {
+	if p == "docker/default" {
+		return nil
+	}
+	if p == "unconfined" {
+		return nil
+	}
+	if strings.HasPrefix(p, "localhost/") {
+		return validateSubPath(strings.TrimPrefix(p, "localhost/"), fldPath)
+	}
+	return field.ErrorList{field.Invalid(fldPath, p, "must be a valid seccomp profile")}
+}
+
+func ValidateSeccompPodAnnotations(annotations map[string]string, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	if p, exists := annotations[api.SeccompPodAnnotationKey]; exists {
+		allErrs = append(allErrs, validateSeccompProfile(p, fldPath.Child(api.SeccompPodAnnotationKey))...)
+	}
+	for k, p := range annotations {
+		if strings.HasPrefix(k, api.SeccompContainerAnnotationKeyPrefix) {
+			allErrs = append(allErrs, validateSeccompProfile(p, fldPath.Child(k))...)
+		}
+	}
+
+	return allErrs
+}
+
 // ValidatePodSecurityContext test that the specified PodSecurityContext has valid data.
 func ValidatePodSecurityContext(securityContext *api.PodSecurityContext, spec *api.PodSpec, specPath, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}

pkg/api/validation/validation_test.go

@@ -2299,6 +2299,62 @@ func TestValidatePod(t *testing.T) {
 				DNSPolicy:     api.DNSClusterFirst,
 			},
 		},
+		{ // docker default seccomp profile
+			ObjectMeta: api.ObjectMeta{
+				Name:      "123",
+				Namespace: "ns",
+				Annotations: map[string]string{
+					api.SeccompPodAnnotationKey: "docker/default",
+				},
+			},
+			Spec: api.PodSpec{
+				Containers:    []api.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent"}},
+				RestartPolicy: api.RestartPolicyAlways,
+				DNSPolicy:     api.DNSClusterFirst,
+			},
+		},
+		{ // unconfined seccomp profile
+			ObjectMeta: api.ObjectMeta{
+				Name:      "123",
+				Namespace: "ns",
+				Annotations: map[string]string{
+					api.SeccompPodAnnotationKey: "unconfined",
+				},
+			},
+			Spec: api.PodSpec{
+				Containers:    []api.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent"}},
+				RestartPolicy: api.RestartPolicyAlways,
+				DNSPolicy:     api.DNSClusterFirst,
+			},
+		},
+		{ // localhost seccomp profile
+			ObjectMeta: api.ObjectMeta{
+				Name:      "123",
+				Namespace: "ns",
+				Annotations: map[string]string{
+					api.SeccompPodAnnotationKey: "localhost/foo",
+				},
+			},
+			Spec: api.PodSpec{
+				Containers:    []api.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent"}},
+				RestartPolicy: api.RestartPolicyAlways,
+				DNSPolicy:     api.DNSClusterFirst,
+			},
+		},
+		{ // localhost seccomp profile for a container
+			ObjectMeta: api.ObjectMeta{
+				Name:      "123",
+				Namespace: "ns",
+				Annotations: map[string]string{
+					api.SeccompContainerAnnotationKeyPrefix + "foo": "localhost/foo",
+				},
+			},
+			Spec: api.PodSpec{
+				Containers:    []api.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent"}},
+				RestartPolicy: api.RestartPolicyAlways,
+				DNSPolicy:     api.DNSClusterFirst,
+			},
+		},
 	}
 	for _, pod := range successCases {
 		if errs := ValidatePod(&pod); len(errs) != 0 {
@@ -2711,6 +2767,90 @@ func TestValidatePod(t *testing.T) {
 				DNSPolicy:     api.DNSClusterFirst,
 			},
 		},
+		"must be a valid pod seccomp profile": {
+			ObjectMeta: api.ObjectMeta{
+				Name:      "123",
+				Namespace: "ns",
+				Annotations: map[string]string{
+					api.SeccompPodAnnotationKey: "foo",
+				},
+			},
+			Spec: api.PodSpec{
+				Containers:    []api.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent"}},
+				RestartPolicy: api.RestartPolicyAlways,
+				DNSPolicy:     api.DNSClusterFirst,
+			},
+		},
+		"must be a valid container seccomp profile": {
+			ObjectMeta: api.ObjectMeta{
+				Name:      "123",
+				Namespace: "ns",
+				Annotations: map[string]string{
+					api.SeccompContainerAnnotationKeyPrefix + "foo": "foo",
+				},
+			},
+			Spec: api.PodSpec{
+				Containers:    []api.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent"}},
+				RestartPolicy: api.RestartPolicyAlways,
+				DNSPolicy:     api.DNSClusterFirst,
+			},
+		},
+		"must be a non-empty container name in seccomp annotation": {
+			ObjectMeta: api.ObjectMeta{
+				Name:      "123",
+				Namespace: "ns",
+				Annotations: map[string]string{
+					api.SeccompContainerAnnotationKeyPrefix: "foo",
+				},
+			},
+			Spec: api.PodSpec{
+				Containers:    []api.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent"}},
+				RestartPolicy: api.RestartPolicyAlways,
+				DNSPolicy:     api.DNSClusterFirst,
+			},
+		},
+		"must be a non-empty container profile in seccomp annotation": {
+			ObjectMeta: api.ObjectMeta{
+				Name:      "123",
+				Namespace: "ns",
+				Annotations: map[string]string{
+					api.SeccompContainerAnnotationKeyPrefix + "foo": "",
+				},
+			},
+			Spec: api.PodSpec{
+				Containers:    []api.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent"}},
+				RestartPolicy: api.RestartPolicyAlways,
+				DNSPolicy:     api.DNSClusterFirst,
+			},
+		},
+		"must be a relative path in a node-local seccomp profile annotation": {
+			ObjectMeta: api.ObjectMeta{
+				Name:      "123",
+				Namespace: "ns",
+				Annotations: map[string]string{
+					api.SeccompPodAnnotationKey: "localhost//foo",
+				},
+			},
+			Spec: api.PodSpec{
+				Containers:    []api.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent"}},
+				RestartPolicy: api.RestartPolicyAlways,
+				DNSPolicy:     api.DNSClusterFirst,
+			},
+		},
+		"must not start with '../'": {
+			ObjectMeta: api.ObjectMeta{
+				Name:      "123",
+				Namespace: "ns",
+				Annotations: map[string]string{
+					api.SeccompPodAnnotationKey: "localhost/../foo",
+				},
+			},
+			Spec: api.PodSpec{
+				Containers:    []api.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent"}},
+				RestartPolicy: api.RestartPolicyAlways,
+				DNSPolicy:     api.DNSClusterFirst,
+			},
+		},
 	}
 	for k, v := range errorCases {
 		if errs := ValidatePod(&v); len(errs) == 0 {

pkg/kubelet/dockertools/fixtures/seccomp/sub/subtest

@@ -0,0 +1,3 @@
+{
+    "abc": "def"
+}

pkg/kubelet/dockertools/fixtures/seccomp/test

@@ -0,0 +1,3 @@
+{
+    "foo": "bar"
+}

pkg/kubelet/dockertools/manager.go

@@ -991,10 +991,10 @@ func (dm *DockerManager) getSecurityOpt(pod *api.Pod, ctrName string) ([]string,
 		return nil, nil
 	}
 
-	profile, profileOK := pod.ObjectMeta.Annotations["container.seccomp.security.alpha.kubernetes.io/"+ctrName]
+	profile, profileOK := pod.ObjectMeta.Annotations[api.SeccompContainerAnnotationKeyPrefix+ctrName]
 	if !profileOK {
 		// try the pod profile
-		profile, profileOK = pod.ObjectMeta.Annotations["seccomp.security.alpha.kubernetes.io/pod"]
+		profile, profileOK = pod.ObjectMeta.Annotations[api.SeccompPodAnnotationKey]
 		if !profileOK {
 			// return early the default
 			return defaultSecurityOpt, nil
@@ -1015,9 +1015,11 @@ func (dm *DockerManager) getSecurityOpt(pod *api.Pod, ctrName string) ([]string,
 		return nil, fmt.Errorf("unknown seccomp profile option: %s", profile)
 	}
 
-	file, err := ioutil.ReadFile(filepath.Join(dm.seccompProfileRoot, strings.TrimPrefix(profile, "localhost/")))
+	name := strings.TrimPrefix(profile, "localhost/") // by pod annotation validation, name is a valid subpath
+	fname := filepath.Join(dm.seccompProfileRoot, filepath.FromSlash(name))
+	file, err := ioutil.ReadFile(fname)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("cannot load seccomp profile %q: %v", name, err)
 	}
 
 	b := bytes.NewBuffer(nil)
@@ -1976,7 +1978,7 @@ func (dm *DockerManager) SyncPod(pod *api.Pod, _ api.PodStatus, podStatus *kubec
 		podInfraContainerID, err, msg = dm.createPodInfraContainer(pod)
 		if err != nil {
 			startContainerResult.Fail(err, msg)
-			glog.Errorf("Failed to create pod infra container: %v; Skipping pod %q", err, format.Pod(pod))
+			glog.Errorf("Failed to create pod infra container: %v; Skipping pod %q: %s", err, format.Pod(pod), msg)
 			return
 		}
 

pkg/kubelet/dockertools/manager_test.go

@@ -21,8 +21,10 @@ import (
 	"io/ioutil"
 	"net/http"
 	"os"
+	"path"
 	"reflect"
 	"regexp"
+	goruntime "runtime"
 	"sort"
 	"strconv"
 	"strings"
@@ -1762,7 +1764,7 @@ func TestUnconfinedSeccompProfileWithDockerV110(t *testing.T) {
 			Name:      "foo4",
 			Namespace: "new",
 			Annotations: map[string]string{
-				"seccomp.security.alpha.kubernetes.io/pod": "unconfined",
+				api.SeccompPodAnnotationKey: "unconfined",
 			},
 		},
 		Spec: api.PodSpec{
@@ -1804,7 +1806,7 @@ func TestDefaultSeccompProfileWithDockerV110(t *testing.T) {
 			Name:      "foo1",
 			Namespace: "new",
 			Annotations: map[string]string{
-				"seccomp.security.alpha.kubernetes.io/pod": "docker/default",
+				api.SeccompPodAnnotationKey: "docker/default",
 			},
 		},
 		Spec: api.PodSpec{
@@ -1846,8 +1848,8 @@ func TestSeccompContainerAnnotationTrumpsPod(t *testing.T) {
 			Name:      "foo2",
 			Namespace: "new",
 			Annotations: map[string]string{
-				"seccomp.security.alpha.kubernetes.io/pod":            "unconfined",
-				"container.seccomp.security.alpha.kubernetes.io/bar2": "docker/default",
+				api.SeccompPodAnnotationKey:                      "unconfined",
+				api.SeccompContainerAnnotationKeyPrefix + "bar2": "docker/default",
 			},
 		},
 		Spec: api.PodSpec{
@@ -1881,6 +1883,80 @@ func TestSeccompContainerAnnotationTrumpsPod(t *testing.T) {
 	assert.NotContains(t, newContainer.HostConfig.SecurityOpt, "seccomp:unconfined", "Container annotation should trump the pod annotation for seccomp.")
 }
 
+func TestSeccompLocalhostProfileIsLoaded(t *testing.T) {
+	tests := []struct {
+		annotations    map[string]string
+		expectedSecOpt string
+		expectedError  string
+	}{
+		{
+			annotations: map[string]string{
+				api.SeccompPodAnnotationKey: "localhost/test",
+			},
+			expectedSecOpt: `seccomp={"foo":"bar"}`,
+		},
+		{
+			annotations: map[string]string{
+				api.SeccompPodAnnotationKey: "localhost/sub/subtest",
+			},
+			expectedSecOpt: `seccomp={"abc":"def"}`,
+		},
+		{
+			annotations: map[string]string{
+				api.SeccompPodAnnotationKey: "localhost/not-existing",
+			},
+			expectedError: "cannot load seccomp profile",
+		},
+	}
+
+	for _, test := range tests {
+		dm, fakeDocker := newTestDockerManagerWithVersion("1.10.1", "1.22")
+		_, filename, _, _ := goruntime.Caller(0)
+		dm.seccompProfileRoot = path.Join(path.Dir(filename), "fixtures", "seccomp")
+
+		pod := &api.Pod{
+			ObjectMeta: api.ObjectMeta{
+				UID:         "12345678",
+				Name:        "foo2",
+				Namespace:   "new",
+				Annotations: test.annotations,
+			},
+			Spec: api.PodSpec{
+				Containers: []api.Container{
+					{Name: "bar2"},
+				},
+			},
+		}
+
+		result := runSyncPod(t, dm, fakeDocker, pod, nil, test.expectedError != "")
+		if test.expectedError != "" {
+			assert.Contains(t, result.Error().Error(), test.expectedError)
+			continue
+		}
+
+		verifyCalls(t, fakeDocker, []string{
+			// Create pod infra container.
+			"create", "start", "inspect_container", "inspect_container",
+			// Create container.
+			"create", "start", "inspect_container",
+		})
+
+		fakeDocker.Lock()
+		if len(fakeDocker.Created) != 2 ||
+			!matchString(t, "/k8s_POD\\.[a-f0-9]+_foo2_new_", fakeDocker.Created[0]) ||
+			!matchString(t, "/k8s_bar2\\.[a-f0-9]+_foo2_new_", fakeDocker.Created[1]) {
+			t.Errorf("unexpected containers created %v", fakeDocker.Created)
+		}
+		fakeDocker.Unlock()
+
+		newContainer, err := fakeDocker.InspectContainer(fakeDocker.Created[1])
+		if err != nil {
+			t.Fatalf("unexpected error %v", err)
+		}
+		assert.Contains(t, newContainer.HostConfig.SecurityOpt, test.expectedSecOpt, "The compacted seccomp json profile should be loaded.")
+	}
+}
+
 func TestSecurityOptsAreNilWithDockerV19(t *testing.T) {
 	dm, fakeDocker := newTestDockerManagerWithVersion("1.9.1", "1.21")
 	pod := &api.Pod{

test/e2e/security_context.go

@@ -110,33 +110,33 @@ var _ = framework.KubeDescribe("Security Context [Feature:SecurityContext]", fun
 	It("should support seccomp alpha unconfined annotation on the container [Feature:Seccomp]", func() {
 		// TODO: port to SecurityContext as soon as seccomp is out of alpha
 		pod := scTestPod(false, false)
-		pod.Annotations["container.seccomp.security.alpha.kubernetes.io/test-container"] = "unconfined"
-		pod.Annotations["seccomp.security.alpha.kubernetes.io/pod"] = "docker/default"
+		pod.Annotations[api.SeccompContainerAnnotationKeyPrefix+"test-container"] = "unconfined"
+		pod.Annotations[api.SeccompPodAnnotationKey] = "docker/default"
 		pod.Spec.Containers[0].Command = []string{"grep", "ecc", "/proc/self/status"}
-		f.TestContainerOutput("pod.Spec.SecurityContext.Seccomp", pod, 0, []string{"0"}) // seccomp disabled
+		f.TestContainerOutput(api.SeccompPodAnnotationKey, pod, 0, []string{"0"}) // seccomp disabled
 	})
 
 	It("should support seccomp alpha unconfined annotation on the pod [Feature:Seccomp]", func() {
 		// TODO: port to SecurityContext as soon as seccomp is out of alpha
 		pod := scTestPod(false, false)
-		pod.Annotations["seccomp.security.alpha.kubernetes.io/pod"] = "unconfined"
+		pod.Annotations[api.SeccompPodAnnotationKey] = "unconfined"
 		pod.Spec.Containers[0].Command = []string{"grep", "ecc", "/proc/self/status"}
-		f.TestContainerOutput("pod.Spec.SecurityContext.Seccomp", pod, 0, []string{"0"}) // seccomp disabled
+		f.TestContainerOutput(api.SeccompPodAnnotationKey, pod, 0, []string{"0"}) // seccomp disabled
 	})
 
 	It("should support seccomp alpha docker/default annotation [Feature:Seccomp]", func() {
 		// TODO: port to SecurityContext as soon as seccomp is out of alpha
 		pod := scTestPod(false, false)
-		pod.Annotations["container.seccomp.security.alpha.kubernetes.io/test-container"] = "docker/default"
+		pod.Annotations[api.SeccompContainerAnnotationKeyPrefix+"test-container"] = "docker/default"
 		pod.Spec.Containers[0].Command = []string{"grep", "ecc", "/proc/self/status"}
-		f.TestContainerOutput("pod.Spec.SecurityContext.Seccomp", pod, 0, []string{"2"}) // seccomp filtered
+		f.TestContainerOutput(api.SeccompPodAnnotationKey, pod, 0, []string{"2"}) // seccomp filtered
 	})
 
 	It("should support seccomp default which is unconfined [Feature:Seccomp]", func() {
 		// TODO: port to SecurityContext as soon as seccomp is out of alpha
 		pod := scTestPod(false, false)
 		pod.Spec.Containers[0].Command = []string{"grep", "ecc", "/proc/self/status"}
-		f.TestContainerOutput("pod.Spec.SecurityContext.Seccomp", pod, 0, []string{"0"}) // seccomp disabled
+		f.TestContainerOutput(api.SeccompPodAnnotationKey, pod, 0, []string{"0"}) // seccomp disabled
 	})
 })