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.

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

CVE-2019-11246: Clean links handling in cp's tar code #76788

Merged
merged 2 commits into from Apr 30, 2019

Conversation

@soltysh
Copy link
Contributor

commented Apr 18, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR cleans up the tar code used in kubectl cp.

Special notes for your reviewer:
/assign @tallclair

Does this PR introduce a user-facing change?:

Clean links handling in cp's tar code
@soltysh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

/sig cli
/priority important-longterm

//if no file was copied
errInfo := fmt.Sprintf("error: %s no such file or directory", prefix)
return errors.New(errInfo)
}

This comment has been minimized.

Copy link
@tallclair

tallclair Apr 18, 2019

Member

how come you removed this? (I'm not sure I disagree, just curious)

This comment has been minimized.

Copy link
@soltysh

soltysh Apr 19, 2019

Author Contributor

The entire code was introduced in #46762 to deal with copying a file into a dir, which is now much simpler. I've wanted to clean this so with this significant changes it was a good time to do it.

// We need to ensure that the destination file is always within boundries
// of the destination directory. This prevents any kind of path traversal
// from within tar archive.
dir, file := filepath.Split(destFileName)

This comment has been minimized.

Copy link
@tallclair

tallclair Apr 18, 2019

Member

What happens here if the destination file is a directory? Can this get confused if the path ends in a /?

This comment has been minimized.

Copy link
@soltysh

soltysh Apr 19, 2019

Author Contributor

Based on https://golang.org/pkg/path/filepath/#Split it should be solid.

This comment has been minimized.

Copy link
@tallclair

tallclair Apr 19, 2019

Member

Looking at the implementation, it seems like the destination file would be included in the directory if the path end sin a slash, and the file would be "" I think.

I think it's fine, but might be worth adding a test case?

This comment has been minimized.

Copy link
@soltysh

soltysh Apr 23, 2019

Author Contributor

I've added directory test, but that's actually handled a few lines earlier in

destFileName := path.Join(destDir, header.Name[len(prefix):])

destFileName will get cleaned (path.Join calls Clean internally) even if it ends with /.

@soltysh soltysh force-pushed the soltysh:tar-fixes branch 3 times, most recently from 9baef94 to a93a18c Apr 19, 2019

@soltysh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@tallclair added that one last test case and also fixed the verify failure, ptal

@soltysh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

/retest

@tallclair
Copy link
Member

left a comment

lgtm, just fix the comment.

})

files = append(files,
file{ // Absolute file

This comment has been minimized.

Copy link
@tallclair

tallclair Apr 25, 2019

Member
Suggested change
file{ // Absolute file
file{ // Relative directory path with terminating /

@soltysh soltysh force-pushed the soltysh:tar-fixes branch from a93a18c to 9c6a9b7 Apr 26, 2019

@soltysh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@tallclair updated, ptal

tallclair and others added some commits Apr 13, 2019

@soltysh soltysh force-pushed the soltysh:tar-fixes branch from 9c6a9b7 to 7962231 Apr 26, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@soltysh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Now with fixed e2e.

@soltysh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Self-tagging based on previous approval.

@soltysh soltysh added the lgtm label Apr 30, 2019

@k8s-ci-robot k8s-ci-robot merged commit b95fca0 into kubernetes:master Apr 30, 2019

20 checks passed

cla/linuxfoundation soltysh authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@soltysh soltysh deleted the soltysh:tar-fixes branch May 2, 2019

k8s-ci-robot added a commit that referenced this pull request May 2, 2019

Merge pull request #77260 from soltysh/automated-cherry-pick-of-#7678…
…8-upstream-release-1.14

Automated cherry pick of #76788: Test kubectl cp escape

k8s-ci-robot added a commit that referenced this pull request May 3, 2019

Merge pull request #77261 from soltysh/automated-cherry-pick-of-#7678…
…8-upstream-release-1.13

Automated cherry pick of #76788: Test kubectl cp escape

k8s-ci-robot added a commit that referenced this pull request May 8, 2019

Merge pull request #77262 from soltysh/automated-cherry-pick-of-#7678…
…8-upstream-release-1.12

Automated cherry pick of #76788: Test kubectl cp escape
}
// For scrutiny we verify both the actual destination as well as we follow
// all the links that might lead outside of the destination directory.
if !isDestRelative(destDir, destFileName) || !isDestRelative(destDir, filepath.Join(evaledPath, file)) {

This comment has been minimized.

Copy link
@abursavich

abursavich Jun 24, 2019

Contributor

I haven't had enough time to go through everything to understand the intention here, but I think the symlink checking is probably more aggressive than it should be. Picking up this change breaks existing scripts that copy files to temp directories on Mac.

Please be aware that Mac's default readlink doesn't support canonicalizing a path, so it's not as easy to portably fix on the user's side as you might think (see: stackoverflow).

$ FILE=path/to/file
$ TMPD=$(mktemp -d)
$ kubectl cp --no-preserve "$NAMESPACE/$POD:$FILE" "$TMPD/$FILE" -c $CONTAINER
warning: link "/var/folders/h7/ydd9wql57gdf6883cj9779sm0zsnn7/T/tmp.3TVUBz8Mqe/path/to/file" is pointing to "" which is outside target destination, skipping

$ echo $TMPD
/var/folders/h7/ydd9wql57gdf6883cj9779sm0zsnn7/T/tmp.3TVUBz8Mqe
$ readlink -f $TMPD  # using GNU readlink
/private/var/folders/h7/ydd9wql57gdf6883cj9779sm0zsnn7/T/tmp.3TVUBz8Mqe

@liggitt liggitt changed the title Clean links handling in cp's tar code CVE-2019-11246: Clean links handling in cp's tar code Jul 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.