not allow backsteps in local volume plugin #47236

dixudx · 2017-06-09T10:33:47Z

Which issue this PR fixes : fixes #47207

Special notes for your reviewer:
cc @msau42 @ddysher
Just follow @liggitt commented.

Release note:

NONE

k8s-ci-robot · 2017-06-09T10:33:54Z

Hi @dixudx. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jhorwit2 · 2017-06-09T12:53:01Z

pkg/volume/util/util.go

+// 3. start with '..'
+// 4. contain filename larger than 255 characters
+// 5. be longer than 4096 characters
+func ValidateLocalPath(targetPath string) error {


This should also probably be used for the host volume plugin, so perhaps a different name?

Why is this check different from

https://github.com/kubernetes/kubernetes/blob/master/pkg/api/validation/validation.go#L947

It seems weird to me to have different validation here than every where else.

@jhorwit2 Yeah, you're right. Thanks for pointing out.

These two functions are nearly the same. But https://github.com/kubernetes/kubernetes/blob/master/pkg/api/validation/validation.go#L947 may not be suitable to be called in pkg/volume/local/local.go.

liggitt · 2017-06-09T13:21:07Z

pkg/api/validation/validation.go

@@ -51,6 +51,7 @@ import (
 	"k8s.io/kubernetes/pkg/capabilities"
 	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/security/apparmor"
+	"k8s.io/kubernetes/pkg/volume/util"


I don't really think the api package should be importing the volume package... I'd rather see the validation function stay in the validation package

liggitt · 2017-06-09T13:22:28Z

pkg/volume/util/util.go

+		if item == ".." {
+			return fmt.Errorf("invalid path: must not contain '..': %s", targetPath)
+		}
+		if len(item) > maxFileNameLength {


is maxFileNameLength the max of all platforms' max, or the min of all platforms' max?

Isn't this already covered on each node when the mounts are provisioned, which would be platform specific?

@liggitt Linux has a maximum filename length of 255 characters for most filesystems (including EXT4), and a maximum path of 4096 characters. On windows, the maximum length for a path is defined as 260 characters and a maximum total path length is 32,767 characters. Here maxFileNameLength is the min value. Also for maxPathLength.

Since there are differences between platforms, I don't think we should be enforcing this this way. Let's limit this to the backstep path segment prevention for now

Yeah, good advice. Function validateLocalNonReservedPath is quite similar with this function. Shall we abstract the core part to a common function?

jhorwit2 · 2017-06-09T13:27:30Z

Local volumes would still allow backsteps in the VolumeMount.SubPath, correct? Do we want to also address that here? If I'm not mistaken that's taken care of outside the local volume mounter just like with the host volume mounter.

php-coder · 2017-06-09T16:11:17Z

pkg/volume/util/util.go

+// 5. be longer than 4096 characters
+func ValidateLocalPath(targetPath string) error {
+	if targetPath == "" {
+		return fmt.Errorf("invalid path: must not be empty: %q", targetPath)


if it's empty then I don't see a reason of showing it in quotes

I don't think that we should add "invalid path:" prefix to each error message. We're using field.Invalid() and it also adds similar prefix to the error AFAIR.

@php-coder Actually function validateLocalNonReservedPath is exactly like what you said. But I think it is not quite suitable to be called by other functions.

php-coder · 2017-06-09T16:14:55Z

pkg/volume/util/util_test.go

+		}
+
+		if !tc.valid && err == nil {
+			t.Errorf("%v: unexpected success", tc.name)


"%v: unexpected success for the path %q" ?

php-coder · 2017-06-09T16:15:36Z

pkg/volume/util/util_test.go

+	for _, tc := range cases {
+		err := ValidateLocalPath(tc.path)
+		if tc.valid && err != nil {
+			t.Errorf("%v: unexpected failure: %v", tc.name, err)


I suggest to include path in the error msg, WDYT?

Yeah, will update it.

php-coder · 2017-06-09T16:19:08Z

pkg/volume/util/util.go

+		}
+	}
+	if strings.HasPrefix(items[0], "..") && len(items[0]) > 2 {
+		return fmt.Errorf("invalid path: must not start with '..': %s", targetPath)


What is reserved paths and why we don't allow them? How they're dangerous? Is it something Windows-specific?

msau42 · 2017-06-09T16:51:03Z

Do we want local and hostpath path validations to use the same function?

dixudx · 2017-06-11T05:58:21Z

@jhorwit2 Local volumes does not allow backsteps in the VolumeMount.SubPath, which has already been validated in ValidateVolumeMounts.

jhorwit2 · 2017-06-11T21:30:40Z

@dixudx that check assumes the apiserver & the node are the same OS. I opened a PR that hopes to address that issue. #47290

dixudx · 2017-06-17T05:00:59Z

@liggitt PTAL. Revised based on #47290.

liggitt · 2017-06-17T05:41:03Z

pkg/api/validation/validation.go

@@ -1134,6 +1134,8 @@ func validateLocalVolumeSource(ls *api.LocalVolumeSource, fldPath *field.Path) f
 	if ls.Path == "" {
 		allErrs = append(allErrs, field.Required(fldPath.Child("path"), ""))


return early

liggitt · 2017-06-17T05:43:37Z

pkg/volume/local/local.go

@@ -192,6 +193,11 @@ func (m *localVolumeMounter) SetUpAt(dir string, fsGroup *types.UnixGroupID) err
 		return err
 	}

+	err := validation.ValidatePathNoBacksteps(m.globalPath)


should this be validating m.globalPath or dir?

validating m.globalPath.

msau42 · 2017-06-19T17:28:53Z

@k8s-bot ok to test

msau42 · 2017-06-19T17:29:00Z

/lgtm

dixudx · 2017-06-20T14:35:09Z

@msau42 @liggitt need your approval. Thx.

supereagle · 2017-06-20T14:50:18Z

@dixudx Need to add release note to remove the label release-note-label-needed. Please refer to Write Release Notes if Needed.

liggitt · 2017-06-21T03:33:30Z

/lgtm

dixudx · 2017-06-21T07:47:39Z

/assign @thockin

Needs your approval @thockin . Thx.

dchen1107 · 2017-06-21T18:00:06Z

/approve

Thanks!

k8s-github-robot · 2017-06-21T18:13:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, dixudx, liggitt, msau42

Associated issue: 47207

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/api/OWNERS~~ [dchen1107]
~~pkg/volume/local/OWNERS~~ [dchen1107,msau42]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-06-21T20:30:06Z

Automatic merge from submit-queue (batch tested with PRs 34515, 47236, 46694, 47819, 47792)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 9, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 9, 2017

k8s-github-robot assigned screeley44 and jingxu97 Jun 9, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jun 9, 2017

dixudx force-pushed the not_allow_backsteps_in_local_volume branch from 6937333 to c339a06 Compare June 9, 2017 10:38

jhorwit2 reviewed Jun 9, 2017

View reviewed changes

liggitt reviewed Jun 9, 2017

View reviewed changes

php-coder reviewed Jun 9, 2017

View reviewed changes

dixudx force-pushed the not_allow_backsteps_in_local_volume branch 2 times, most recently from 03dd60c to 50b233d Compare June 11, 2017 06:05

dixudx closed this Jun 17, 2017

dixudx force-pushed the not_allow_backsteps_in_local_volume branch from 50b233d to 5262a37 Compare June 17, 2017 04:56

dixudx reopened this Jun 17, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 17, 2017

liggitt reviewed Jun 17, 2017

View reviewed changes

not allow backsteps in local volume plugin

aa23ed5

dixudx force-pushed the not_allow_backsteps_in_local_volume branch from e56d032 to aa23ed5 Compare June 17, 2017 06:37

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 19, 2017

k8s-ci-robot assigned msau42 Jun 19, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2017

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jun 19, 2017

k8s-github-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jun 20, 2017

k8s-ci-robot assigned liggitt Jun 21, 2017

k8s-ci-robot assigned thockin Jun 21, 2017

dchen1107 unassigned thockin and jingxu97 Jun 21, 2017

dchen1107 removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jun 21, 2017

dchen1107 added this to the v1.7 milestone Jun 21, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2017

k8s-github-robot merged commit 1184ce8 into kubernetes:master Jun 21, 2017

dixudx deleted the not_allow_backsteps_in_local_volume branch September 9, 2017 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

not allow backsteps in local volume plugin #47236

not allow backsteps in local volume plugin #47236

dixudx commented Jun 9, 2017 •

edited

k8s-ci-robot commented Jun 9, 2017

jhorwit2 Jun 9, 2017

jhorwit2 Jun 9, 2017

dixudx Jun 10, 2017

liggitt Jun 9, 2017

liggitt Jun 9, 2017

jhorwit2 Jun 9, 2017

dixudx Jun 10, 2017

liggitt Jun 10, 2017

dixudx Jun 10, 2017

jhorwit2 commented Jun 9, 2017

php-coder Jun 9, 2017

dixudx Jun 10, 2017

php-coder Jun 9, 2017

php-coder Jun 9, 2017

dixudx Jun 10, 2017

php-coder Jun 9, 2017

msau42 commented Jun 9, 2017

dixudx commented Jun 11, 2017

jhorwit2 commented Jun 11, 2017

dixudx commented Jun 17, 2017

liggitt Jun 17, 2017

liggitt Jun 17, 2017

dixudx Jun 17, 2017

msau42 commented Jun 19, 2017

msau42 commented Jun 19, 2017

dixudx commented Jun 20, 2017 •

edited

supereagle commented Jun 20, 2017

liggitt commented Jun 21, 2017

dixudx commented Jun 21, 2017

dchen1107 commented Jun 21, 2017

k8s-github-robot commented Jun 21, 2017

k8s-github-robot commented Jun 21, 2017

		@@ -1134,6 +1134,8 @@ func validateLocalVolumeSource(ls api.LocalVolumeSource, fldPath field.Path) f
		if ls.Path == "" {
		allErrs = append(allErrs, field.Required(fldPath.Child("path"), ""))

not allow backsteps in local volume plugin #47236

not allow backsteps in local volume plugin #47236

Conversation

dixudx commented Jun 9, 2017 • edited

k8s-ci-robot commented Jun 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhorwit2 commented Jun 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Jun 9, 2017

dixudx commented Jun 11, 2017

jhorwit2 commented Jun 11, 2017

dixudx commented Jun 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Jun 19, 2017

msau42 commented Jun 19, 2017

dixudx commented Jun 20, 2017 • edited

supereagle commented Jun 20, 2017

liggitt commented Jun 21, 2017

dixudx commented Jun 21, 2017

dchen1107 commented Jun 21, 2017

k8s-github-robot commented Jun 21, 2017

k8s-github-robot commented Jun 21, 2017

dixudx commented Jun 9, 2017 •

edited

dixudx commented Jun 20, 2017 •

edited