Merge pull request #47290 from jhorwit2/jah/hostpath-psp-backstep-check

Automatic merge from submit-queue (batch tested with PRs 47626, 47674, 47683, 47290, 47688)

validate host paths on the kubelet for backsteps

**What this PR does / why we need it**:

This PR adds validation on the kubelet to ensure the host path does not contain backsteps that could allow the volume to escape the PSP's allowed host paths. Currently, there is validation done at in API server; however, that does not account for mismatch of OS's on the kubelet vs api server. 

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #47107

**Special notes for your reviewer**:

cc @liggitt

**Release note**:


```release-note
Paths containing backsteps (for example, "../bar") are no longer allowed in hostPath volume paths, or in volumeMount subpaths
```

pkg/api/validation/validation.go

@@ -20,8 +20,8 @@ import (
 	"encoding/json"
 	"fmt"
 	"net"
-	"os"
 	"path"
+	"path/filepath"
 	"reflect"
 	"regexp"
 	"strconv"
@@ -627,7 +627,10 @@ func validateHostPathVolumeSource(hostPath *api.HostPathVolumeSource, fldPath *f
 	allErrs := field.ErrorList{}
 	if len(hostPath.Path) == 0 {
 		allErrs = append(allErrs, field.Required(fldPath.Child("path"), ""))
+		return allErrs
 	}
+
+	allErrs = append(allErrs, validatePathNoBacksteps(hostPath.Path, fldPath.Child("path"))...)
 	return allErrs
 }
 
@@ -958,8 +961,18 @@ func validateLocalDescendingPath(targetPath string, fldPath *field.Path) field.E
 		allErrs = append(allErrs, field.Invalid(fldPath, targetPath, "must be a relative path"))
 	}
 
-	// TODO: this assumes the OS of apiserver & nodes are the same
-	parts := strings.Split(targetPath, string(os.PathSeparator))
+	allErrs = append(allErrs, validatePathNoBacksteps(targetPath, fldPath)...)
+
+	return allErrs
+}
+
+// validatePathNoBacksteps makes sure the targetPath does not have any `..` path elements when split
+//
+// This assumes the OS of the apiserver and the nodes are the same. The same check should be done
+// on the node to ensure there are no backsteps.
+func validatePathNoBacksteps(targetPath string, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	parts := strings.Split(filepath.ToSlash(targetPath), "/")
 	for _, item := range parts {
 		if item == ".." {
 			allErrs = append(allErrs, field.Invalid(fldPath, targetPath, "must not contain '..'"))

pkg/api/validation/validation_test.go

@@ -268,6 +268,19 @@ func TestValidatePersistentVolumes(t *testing.T) {
 					StorageClassName: "test-storage-class",
 				}),
 		},
+		"bad-hostpath-volume-backsteps": {
+			isExpectedFailure: true,
+			volume: testVolume("foo", "", api.PersistentVolumeSpec{
+				Capacity: api.ResourceList{
+					api.ResourceName(api.ResourceStorage): resource.MustParse("10G"),
+				},
+				AccessModes: []api.PersistentVolumeAccessMode{api.ReadWriteOnce},
+				PersistentVolumeSource: api.PersistentVolumeSource{
+					HostPath: &api.HostPathVolumeSource{Path: "/foo/.."},
+				},
+				StorageClassName: "backstep-hostpath",
+			}),
+		},
 	}
 
 	for name, scenario := range scenarios {
@@ -1102,6 +1115,20 @@ func TestValidateVolumes(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "invalid HostPath backsteps",
+			vol: api.Volume{
+				Name: "hostpath",
+				VolumeSource: api.VolumeSource{
+					HostPath: &api.HostPathVolumeSource{
+						Path: "/mnt/path/..",
+					},
+				},
+			},
+			errtype:   field.ErrorTypeInvalid,
+			errfield:  "path",
+			errdetail: "must not contain '..'",
+		},
 		// GcePersistentDisk
 		{
 			name: "valid GcePersistentDisk",

pkg/kubelet/BUILD

@@ -111,6 +111,7 @@ go_library(
         "//pkg/volume/util:go_default_library",
         "//pkg/volume/util/types:go_default_library",
         "//pkg/volume/util/volumehelper:go_default_library",
+        "//pkg/volume/validation:go_default_library",
         "//plugin/pkg/scheduler/algorithm:go_default_library",
         "//plugin/pkg/scheduler/algorithm/predicates:go_default_library",
         "//third_party/forked/golang/expansion:go_default_library",

pkg/kubelet/kubelet_pods.go

@@ -61,6 +61,7 @@ import (
 	"k8s.io/kubernetes/pkg/util"
 	"k8s.io/kubernetes/pkg/volume"
 	"k8s.io/kubernetes/pkg/volume/util/volumehelper"
+	volumevalidation "k8s.io/kubernetes/pkg/volume/validation"
 	"k8s.io/kubernetes/third_party/forked/golang/expansion"
 )
 
@@ -138,6 +139,15 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h
 			return nil, err
 		}
 		if mount.SubPath != "" {
+			if filepath.IsAbs(mount.SubPath) {
+				return nil, fmt.Errorf("error SubPath `%s` must not be an absolute path", mount.SubPath)
+			}
+
+			err = volumevalidation.ValidatePathNoBacksteps(mount.SubPath)
+			if err != nil {
+				return nil, fmt.Errorf("unable to provision SubPath `%s`: %v", mount.SubPath, err)
+			}
+
 			fileinfo, err := os.Lstat(hostPath)
 			if err != nil {
 				return nil, err

pkg/kubelet/kubelet_pods_test.go

@@ -42,76 +42,137 @@ import (
 )
 
 func TestMakeMounts(t *testing.T) {
-	container := v1.Container{
-		VolumeMounts: []v1.VolumeMount{
-			{
-				MountPath: "/etc/hosts",
-				Name:      "disk",
-				ReadOnly:  false,
+	testCases := map[string]struct {
+		container      v1.Container
+		podVolumes     kubecontainer.VolumeMap
+		expectErr      bool
+		expectedErrMsg string
+		expectedMounts []kubecontainer.Mount
+	}{
+		"valid mounts": {
+			podVolumes: kubecontainer.VolumeMap{
+				"disk":  kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/disk"}},
+				"disk4": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/host"}},
+				"disk5": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/var/lib/kubelet/podID/volumes/empty/disk5"}},
 			},
-			{
-				MountPath: "/mnt/path3",
-				Name:      "disk",
-				ReadOnly:  true,
+			container: v1.Container{
+				VolumeMounts: []v1.VolumeMount{
+					{
+						MountPath: "/etc/hosts",
+						Name:      "disk",
+						ReadOnly:  false,
+					},
+					{
+						MountPath: "/mnt/path3",
+						Name:      "disk",
+						ReadOnly:  true,
+					},
+					{
+						MountPath: "/mnt/path4",
+						Name:      "disk4",
+						ReadOnly:  false,
+					},
+					{
+						MountPath: "/mnt/path5",
+						Name:      "disk5",
+						ReadOnly:  false,
+					},
+				},
 			},
-			{
-				MountPath: "/mnt/path4",
-				Name:      "disk4",
-				ReadOnly:  false,
+			expectedMounts: []kubecontainer.Mount{
+				{
+					Name:           "disk",
+					ContainerPath:  "/etc/hosts",
+					HostPath:       "/mnt/disk",
+					ReadOnly:       false,
+					SELinuxRelabel: false,
+				},
+				{
+					Name:           "disk",
+					ContainerPath:  "/mnt/path3",
+					HostPath:       "/mnt/disk",
+					ReadOnly:       true,
+					SELinuxRelabel: false,
+				},
+				{
+					Name:           "disk4",
+					ContainerPath:  "/mnt/path4",
+					HostPath:       "/mnt/host",
+					ReadOnly:       false,
+					SELinuxRelabel: false,
+				},
+				{
+					Name:           "disk5",
+					ContainerPath:  "/mnt/path5",
+					HostPath:       "/var/lib/kubelet/podID/volumes/empty/disk5",
+					ReadOnly:       false,
+					SELinuxRelabel: false,
+				},
 			},
-			{
-				MountPath: "/mnt/path5",
-				Name:      "disk5",
-				ReadOnly:  false,
+			expectErr: false,
+		},
+		"invalid absolute SubPath": {
+			podVolumes: kubecontainer.VolumeMap{
+				"disk": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/disk"}},
 			},
+			container: v1.Container{
+				VolumeMounts: []v1.VolumeMount{
+					{
+						MountPath: "/mnt/path3",
+						SubPath:   "/must/not/be/absolute",
+						Name:      "disk",
+						ReadOnly:  true,
+					},
+				},
+			},
+			expectErr:      true,
+			expectedErrMsg: "error SubPath `/must/not/be/absolute` must not be an absolute path",
+		},
+		"invalid SubPath with backsteps": {
+			podVolumes: kubecontainer.VolumeMap{
+				"disk": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/disk"}},
+			},
+			container: v1.Container{
+				VolumeMounts: []v1.VolumeMount{
+					{
+						MountPath: "/mnt/path3",
+						SubPath:   "no/backsteps/../allowed",
+						Name:      "disk",
+						ReadOnly:  true,
+					},
+				},
+			},
+			expectErr:      true,
+			expectedErrMsg: "unable to provision SubPath `no/backsteps/../allowed`: must not contain '..'",
 		},
 	}
 
-	podVolumes := kubecontainer.VolumeMap{
-		"disk":  kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/disk"}},
-		"disk4": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/host"}},
-		"disk5": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/var/lib/kubelet/podID/volumes/empty/disk5"}},
-	}
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			pod := v1.Pod{
+				Spec: v1.PodSpec{
+					HostNetwork: true,
+				},
+			}
 
-	pod := v1.Pod{
-		Spec: v1.PodSpec{
-			HostNetwork: true,
-		},
-	}
+			mounts, err := makeMounts(&pod, "/pod", &tc.container, "fakepodname", "", "", tc.podVolumes)
 
-	mounts, _ := makeMounts(&pod, "/pod", &container, "fakepodname", "", "", podVolumes)
+			// validate only the error if we expect an error
+			if tc.expectErr {
+				if err == nil || err.Error() != tc.expectedErrMsg {
+					t.Fatalf("expected error message `%s` but got `%v`", tc.expectedErrMsg, err)
+				}
+				return
+			}
 
-	expectedMounts := []kubecontainer.Mount{
-		{
-			Name:           "disk",
-			ContainerPath:  "/etc/hosts",
-			HostPath:       "/mnt/disk",
-			ReadOnly:       false,
-			SELinuxRelabel: false,
-		},
-		{
-			Name:           "disk",
-			ContainerPath:  "/mnt/path3",
-			HostPath:       "/mnt/disk",
-			ReadOnly:       true,
-			SELinuxRelabel: false,
-		},
-		{
-			Name:           "disk4",
-			ContainerPath:  "/mnt/path4",
-			HostPath:       "/mnt/host",
-			ReadOnly:       false,
-			SELinuxRelabel: false,
-		},
-		{
-			Name:           "disk5",
-			ContainerPath:  "/mnt/path5",
-			HostPath:       "/var/lib/kubelet/podID/volumes/empty/disk5",
-			ReadOnly:       false,
-			SELinuxRelabel: false,
-		},
+			// otherwise validate the mounts
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			assert.Equal(t, tc.expectedMounts, mounts, "mounts of container %+v", tc.container)
+		})
 	}
-	assert.Equal(t, expectedMounts, mounts, "mounts of container %+v", container)
 }
 
 func TestHostsFileContent(t *testing.T) {

pkg/volume/host_path/BUILD

@@ -19,6 +19,7 @@ go_library(
         "//pkg/api/v1:go_default_library",
         "//pkg/volume:go_default_library",
         "//pkg/volume/util/volumehelper:go_default_library",
+        "//pkg/volume/validation:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/uuid:go_default_library",

pkg/volume/host_path/host_path.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/kubernetes/pkg/api/v1"
 	"k8s.io/kubernetes/pkg/volume"
 	"k8s.io/kubernetes/pkg/volume/util/volumehelper"
+	"k8s.io/kubernetes/pkg/volume/validation"
 )
 
 // This is the primary entrypoint for volume plugins.
@@ -103,6 +104,7 @@ func (plugin *hostPathPlugin) NewMounter(spec *volume.Spec, pod *v1.Pod, _ volum
 	if err != nil {
 		return nil, err
 	}
+
 	return &hostPathMounter{
 		hostPath: &hostPath{path: hostPathVolumeSource.Path},
 		readOnly: readOnly,
@@ -205,6 +207,10 @@ func (b *hostPathMounter) CanMount() error {
 
 // SetUp does nothing.
 func (b *hostPathMounter) SetUp(fsGroup *types.UnixGroupID) error {
+	err := validation.ValidatePathNoBacksteps(b.GetPath())
+	if err != nil {
+		return fmt.Errorf("invalid HostPath `%s`: %v", b.GetPath(), err)
+	}
 	return nil
 }
 

pkg/volume/host_path/host_path_test.go

@@ -182,6 +182,31 @@ func TestProvisioner(t *testing.T) {
 	os.RemoveAll(pv.Spec.HostPath.Path)
 }
 
+func TestInvalidHostPath(t *testing.T) {
+	plugMgr := volume.VolumePluginMgr{}
+	plugMgr.InitPlugins(ProbeVolumePlugins(volume.VolumeConfig{}), volumetest.NewFakeVolumeHost("fake", nil, nil))
+
+	plug, err := plugMgr.FindPluginByName(hostPathPluginName)
+	if err != nil {
+		t.Fatalf("Unable to find plugin %s by name: %v", hostPathPluginName, err)
+	}
+	spec := &v1.Volume{
+		Name:         "vol1",
+		VolumeSource: v1.VolumeSource{HostPath: &v1.HostPathVolumeSource{Path: "/no/backsteps/allowed/.."}},
+	}
+	pod := &v1.Pod{ObjectMeta: metav1.ObjectMeta{UID: types.UID("poduid")}}
+	mounter, err := plug.NewMounter(volume.NewSpecFromVolume(spec), pod, volume.VolumeOptions{})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = mounter.SetUp(nil)
+	expectedMsg := "invalid HostPath `/no/backsteps/allowed/..`: must not contain '..'"
+	if err.Error() != expectedMsg {
+		t.Fatalf("expected error `%s` but got `%s`", expectedMsg, err)
+	}
+}
+
 func TestPlugin(t *testing.T) {
 	plugMgr := volume.VolumePluginMgr{}
 	plugMgr.InitPlugins(ProbeVolumePlugins(volume.VolumeConfig{}), volumetest.NewFakeVolumeHost("fake", nil, nil))

pkg/volume/validation/pv_validation.go

@@ -17,6 +17,10 @@ limitations under the License.
 package validation
 
 import (
+	"errors"
+	"path/filepath"
+	"strings"
+
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/kubernetes/pkg/api"
 )
@@ -53,3 +57,15 @@ func checkMountOption(pv *api.PersistentVolume) field.ErrorList {
 	}
 	return allErrs
 }
+
+// ValidatePathNoBacksteps will make sure the targetPath does not have any element which is ".."
+func ValidatePathNoBacksteps(targetPath string) error {
+	parts := strings.Split(filepath.ToSlash(targetPath), "/")
+	for _, item := range parts {
+		if item == ".." {
+			return errors.New("must not contain '..'")
+		}
+	}
+
+	return nil
+}

pkg/volume/validation/pv_validation_test.go

@@ -84,3 +84,30 @@ func testVolumeWithMountOption(name string, namespace string, mountOptions strin
 		Spec:       spec,
 	}
 }
+
+func TestValidatePathNoBacksteps(t *testing.T) {
+	testCases := map[string]struct {
+		path        string
+		expectedErr bool
+	}{
+		"valid path": {
+			path: "/foo/bar",
+		},
+		"invalid path": {
+			path:        "/foo/bar/..",
+			expectedErr: true,
+		},
+	}
+
+	for name, tc := range testCases {
+		err := ValidatePathNoBacksteps(tc.path)
+
+		if err == nil && tc.expectedErr {
+			t.Fatalf("expected test `%s` to return an error but it didnt", name)
+		}
+
+		if err != nil && !tc.expectedErr {
+			t.Fatalf("expected test `%s` to return no error but got `%v`", name, err)
+		}
+	}
+}