From 48b3fb84abdc0fee12299eed262da287d48110de Mon Sep 17 00:00:00 2001 From: Josh Horwitz Date: Sat, 10 Jun 2017 09:48:42 -0400 Subject: [PATCH] do not allow backsteps in host volume plugin Fixes #47107 --- pkg/api/validation/validation.go | 19 ++- pkg/api/validation/validation_test.go | 27 +++ pkg/kubelet/BUILD | 1 + pkg/kubelet/kubelet_pods.go | 10 ++ pkg/kubelet/kubelet_pods_test.go | 179 +++++++++++++------- pkg/volume/host_path/BUILD | 1 + pkg/volume/host_path/host_path.go | 6 + pkg/volume/host_path/host_path_test.go | 25 +++ pkg/volume/validation/pv_validation.go | 16 ++ pkg/volume/validation/pv_validation_test.go | 27 +++ 10 files changed, 249 insertions(+), 62 deletions(-) diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 78e055dde9..28a317bcdf 100644 --- a/pkg/api/validation/validation.go +++ b/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 '..'")) diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index b56c059ea4..838ce41a30 100644 --- a/pkg/api/validation/validation_test.go +++ b/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", diff --git a/pkg/kubelet/BUILD b/pkg/kubelet/BUILD index 5c63667e3d..f02fb7a225 100644 --- a/pkg/kubelet/BUILD +++ b/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", diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index e4c1aa5738..b1d286fde5 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/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 diff --git a/pkg/kubelet/kubelet_pods_test.go b/pkg/kubelet/kubelet_pods_test.go index e8e31aa925..ef3cb60cb3 100644 --- a/pkg/kubelet/kubelet_pods_test.go +++ b/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) { diff --git a/pkg/volume/host_path/BUILD b/pkg/volume/host_path/BUILD index 0d0776dd56..1827afffcd 100644 --- a/pkg/volume/host_path/BUILD +++ b/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", diff --git a/pkg/volume/host_path/host_path.go b/pkg/volume/host_path/host_path.go index 35e88c1753..eccf80fec6 100644 --- a/pkg/volume/host_path/host_path.go +++ b/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 } diff --git a/pkg/volume/host_path/host_path_test.go b/pkg/volume/host_path/host_path_test.go index 88c7c7d25e..e8f0d3bfed 100644 --- a/pkg/volume/host_path/host_path_test.go +++ b/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)) diff --git a/pkg/volume/validation/pv_validation.go b/pkg/volume/validation/pv_validation.go index 65d20dcbc9..45db2f5e52 100644 --- a/pkg/volume/validation/pv_validation.go +++ b/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 +} diff --git a/pkg/volume/validation/pv_validation_test.go b/pkg/volume/validation/pv_validation_test.go index ad2d70fef5..d0a8f1c3ee 100644 --- a/pkg/volume/validation/pv_validation_test.go +++ b/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) + } + } +}