Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

validate host paths on the kubelet for backsteps #47290

Merged
merged 1 commit into from Jun 17, 2017
Merged

validate host paths on the kubelet for backsteps #47290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 16 additions & 3 deletions pkg/api/validation/validation.go
View file
Open in desktop
Expand Up @@ -20,8 +20,8 @@ import (
"encoding/json"
"fmt"
"net"
"os"
"path"
"path/filepath"
"reflect"
"regexp"
"strconv"
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function should be uppercase, as being called in other package.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is, there is a similar method in the volume package that is getting called

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 '..'"))
Expand Down
27 changes: 27 additions & 0 deletions pkg/api/validation/validation_test.go
View file
Open in desktop
Expand Up @@ -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 {
Expand Down Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions pkg/kubelet/BUILD
View file
Open in desktop
Expand Up @@ -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",
Expand Down
10 changes: 10 additions & 0 deletions pkg/kubelet/kubelet_pods.go
View file
Open in desktop
Expand Up @@ -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"
)

Expand Down Expand Up @@ -138,6 +139,15 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h
return nil, err
}
if mount.SubPath != "" {
Copy link
Member

@liggitt liggitt Jun 12, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhorwit2 did you ever track down whether we need to be concerned with traversals in mount.MountPath?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel like that's a concern. That's the path within the container where the volume is mounted. Escaping out of that directory only yields you directories in the container you can already access; not extra info on the host.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conceptually, that's what I would expect, but I don't know enough about the implementation to know if that is actually the case. would like an ack on that from @kubernetes/sig-storage-pr-reviews

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This question is also outstanding, correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhorwit2 is right, MountPath is relative to the inside of the container so it does not matter. mount.SubPath should be checked.

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
Expand Down
179 changes: 120 additions & 59 deletions pkg/kubelet/kubelet_pods_test.go
View file
Open in desktop
Expand Up @@ -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"}},
},
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/path3",
Name: "disk",
ReadOnly: true,
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/path4",
Name: "disk4",
ReadOnly: false,
expectErr: false,
},
"invalid absolute SubPath": {
podVolumes: kubecontainer.VolumeMap{
"disk": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/disk"}},
},
{
MountPath: "/mnt/path5",
Name: "disk5",
ReadOnly: false,
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) {
Expand Down
1 change: 1 addition & 0 deletions pkg/volume/host_path/BUILD
View file
Open in desktop
Expand Up @@ -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",
Expand Down
6 changes: 6 additions & 0 deletions pkg/volume/host_path/host_path.go
View file
Open in desktop
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
}

Expand Down
25 changes: 25 additions & 0 deletions pkg/volume/host_path/host_path_test.go
View file
Open in desktop
Expand Up @@ -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))
Expand Down