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

soltysh · 2019-04-18T19:40:56Z

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 · 2019-04-18T19:41:27Z

/sig cli
/priority important-longterm

tallclair · 2019-04-18T22:19:15Z

pkg/kubectl/cmd/cp/cp.go

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

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.

tallclair · 2019-04-18T22:57:52Z

pkg/kubectl/cmd/cp/cp.go

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

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

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?

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 · 2019-04-23T20:04:16Z

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

soltysh · 2019-04-24T11:19:23Z

/retest

tallclair

lgtm, just fix the comment.

tallclair · 2019-04-25T17:11:48Z

pkg/kubectl/cmd/cp/cp_test.go

Suggested change

file{ // Absolute file

file{ // Relative directory path with terminating /

soltysh · 2019-04-26T13:08:46Z

@tallclair updated, ptal

k8s-ci-robot · 2019-04-26T20:20:22Z

[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

Details

Needs approval from an approver in each of these files:

~~pkg/kubectl/OWNERS~~ [soltysh]

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

soltysh · 2019-04-26T20:21:09Z

Now with fixed e2e.

soltysh · 2019-04-30T11:45:39Z

Self-tagging based on previous approval.

…8-upstream-release-1.14 Automated cherry pick of #76788: Test kubectl cp escape

…8-upstream-release-1.13 Automated cherry pick of #76788: Test kubectl cp escape

…8-upstream-release-1.12 Automated cherry pick of #76788: Test kubectl cp escape

abursavich · 2019-06-24T22:42:49Z

pkg/kubectl/cmd/cp/cp.go

+		}
+		// 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)) {


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

kubectl cp potential directory traversal - CVE-2019-11246 kubectl cp potential directory traversal - CVE-2019-11246 kubernetes#75037 kubernetes#76788 See merge request !53

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 18, 2019

k8s-ci-robot assigned tallclair Apr 18, 2019

k8s-ci-robot requested review from brendandburns and lavalamp April 18, 2019 19:41

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Apr 18, 2019

tallclair reviewed Apr 18, 2019

View reviewed changes

soltysh force-pushed the tar-fixes branch 3 times, most recently from 9baef94 to a93a18c Compare April 23, 2019 20:02

tallclair reviewed Apr 25, 2019

View reviewed changes

pkg/kubectl/cmd/cp/cp_test.go Outdated

Copy link

Member

tallclair Apr 25, 2019

Choose a reason for hiding this comment

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

Suggested change

file{ // Absolute file

file{ // Relative directory path with terminating /

soltysh force-pushed the tar-fixes branch from a93a18c to 9c6a9b7 Compare April 26, 2019 13:08

tallclair and others added 2 commits April 26, 2019 22:19

Test kubectl cp escape

a867b39

Properly handle links in tar

7962231

soltysh force-pushed the tar-fixes branch from 9c6a9b7 to 7962231 Compare April 26, 2019 20:20

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2019

soltysh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 30, 2019

soltysh mentioned this pull request Apr 30, 2019

Automated cherry pick of #76788: Test kubectl cp escape #77260

Merged

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 30, 2019

This was referenced Apr 30, 2019

Automated cherry pick of #76788: Test kubectl cp escape #77261

Merged

Automated cherry pick of #76788: Test kubectl cp escape #77262

Merged

Automated cherry pick of #76788: Test kubectl cp escape #77263

Closed

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

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

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…

151efe1

…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…

267805c

…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…

ec31303

…8-upstream-release-1.12 Automated cherry pick of #76788: Test kubectl cp escape

liggitt added the area/security label May 16, 2019

abursavich reviewed Jun 24, 2019

View reviewed changes

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

cji mentioned this pull request Aug 8, 2019

Kubernetes 3rd Party Security Audit Findings #81146

Open

liggitt mentioned this pull request Aug 13, 2019

Preserve file mode when copying files out of a pod #73053

Closed

dvandra mentioned this pull request Sep 25, 2019

[python] Add CVE-2019-11246 fabric8-analytics/cvedb#5994

Closed

tatianab mentioned this pull request Nov 8, 2023

x/vulndb: potential Go vuln in github.com/kubernetes/kubernetes: CVE-2019-11246 golang/vulndb#2230

Closed

	file{ // Absolute file
	file{ // Relative directory path with terminating /

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

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

Uh oh!

Conversation

soltysh commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soltysh commented Apr 18, 2019

Uh oh!

tallclair Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

soltysh Apr 19, 2019

Choose a reason for hiding this comment

Uh oh!

tallclair Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

soltysh Apr 19, 2019

Choose a reason for hiding this comment

Uh oh!

tallclair Apr 19, 2019

Choose a reason for hiding this comment

Uh oh!

soltysh Apr 23, 2019

Choose a reason for hiding this comment

Uh oh!

soltysh commented Apr 23, 2019

Uh oh!

soltysh commented Apr 24, 2019

Uh oh!

tallclair left a comment

Choose a reason for hiding this comment

Uh oh!

tallclair Apr 25, 2019

Choose a reason for hiding this comment

Uh oh!

soltysh commented Apr 26, 2019

Uh oh!

k8s-ci-robot commented Apr 26, 2019

Uh oh!

soltysh commented Apr 26, 2019

Uh oh!

soltysh commented Apr 30, 2019

Uh oh!

abursavich Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

soltysh commented Apr 18, 2019 •

edited

Loading

abursavich Jun 24, 2019 •

edited

Loading