CVE-2019-11251: Remove symlink support from kubectl cp by soltysh · Pull Request #82143 · kubernetes/kubernetes

soltysh · 2019-08-29T19:41:57Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Based on the recent discussion in SIG-CLI we've decided to drop support when copying data from pods.

Special notes for your reviewer:
/assign @liggitt @tallclair
Only the second commit matters, the first is coming from #82087

Fixes #87773

Does this PR introduce a user-facing change?:

`kubectl cp` no longer supports copying symbolic links from containers; to support this use case, see `kubectl exec --help` for examples using `tar` directly

soltysh · 2019-08-29T19:42:19Z

/sig cli
/priority important-longterm

liggitt · 2019-08-29T23:12:24Z

Some packages in hack/.staticcheck_failures are passing staticcheck. Please remove them.

  pkg/kubectl/cmd/cp

Executing tests from //pkg/kubectl/cmd/cp:go_default_test
-----------------------------------------------------------------------------
--- FAIL: TestTarUntar (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference

liggitt · 2019-08-29T23:19:00Z

rather than holding a pointer to the command, consider storing the name of the parent command here (e.g. o.ExecParentCmdName = cmd.Parent().Name()

then, make the hint about using exec condition on ExecParentCmdName being non-empty. that way, CopyOptions stays reusable

liggitt · 2019-08-29T23:20:23Z

this should refer users to help describing how to use exec for symlink support... it is not immediately obvious how you would do that (even just see cp command help for details)

tallclair · 2019-08-29T23:18:53Z

nit: this is verbose for an error that might be printed a bunch of times. Maybe simplify it to something like:

Suggested change

fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is a symlink, skipping (consider using %s exec for symlink support)\n", destFileName, o.cmd.Parent().Name())

fmt.Fprintf(o.IOStreams.ErrOut, "skipping symlink: %q -> %q\n", destFileName, linkTarget)

I'd tend to agree. the pointer to exec can be printed just once

odinuge · 2019-08-30T19:08:19Z

This continue makes no difference, so I think it should be omitted.

Another alternative (my personal favorite) would be to remove the else and keep the continue to make the code path even cleaner and easier to read.

Fair point, I prefer the no else approach too.

odinuge · 2019-08-30T19:23:42Z

I think o.cmd can be nil in some of the tests, as the o.Complete func that sets cmd isn't executed in all of them. The applies to at least TestTarUntar.

liggitt · 2019-08-31T15:55:43Z

/milestone v1.16
/area security

soltysh · 2019-09-02T14:29:52Z

@liggitt @tallclair @odinuge updated, sample output:

$ kubectl cp hello-1-b5fwp:/tmp/test ./
tar: Removing leading `/' from member names
warning: file "link" is a symlink, skipping (consider using "kubectl exec -n "" "hello-1-b5fwp" -- tar cf - "/tmp/test" | tar xf -")
warning: skipping symlink: "link2" -> "file2"

soltysh · 2019-09-02T19:00:53Z

/retest

soltysh · 2019-09-02T20:09:57Z

/retest

liggitt · 2019-09-03T02:56:54Z

nit: does this copy to /tmp/bar like the description says?

liggitt · 2019-09-03T02:57:08Z

nit: does this copy to /tmp/bar like the description says?

liggitt · 2019-09-03T02:57:58Z

&& len(o.ExecParentCmdName) > 0

soltysh · 2019-09-03T08:44:59Z

@liggitt nits addressed, comments answered, ptal

liggitt · 2019-09-03T12:14:17Z

/lgtm
/approve
/retest

k8s-ci-robot · 2019-09-03T12:14:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, 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:

~~hack/OWNERS~~ [liggitt,soltysh]
~~pkg/kubectl/OWNERS~~ [liggitt,soltysh]

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

odinuge

/lgtm

odinuge · 2019-09-03T15:36:07Z

/retest

tallclair · 2019-09-03T16:00:50Z

-		evaledPath, err := filepath.EvalSymlinks(baseName)
+		if mode&os.ModeSymlink != 0 {
+			if !symlinkWarningPrinted && len(o.ExecParentCmdName) > 0 {
+				fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is a symlink, skipping (consider using \"%s exec -n %q %q -- tar cf - %q | tar xf -\")\n", destFileName, o.ExecParentCmdName, src.PodNamespace, src.PodName, src.File)


nit: I would make this error message more consistent with the other one, so they can be visually scanned together

Suggested change

fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is a symlink, skipping (consider using \"%s exec -n %q %q -- tar cf - %q | tar xf -\")\n", destFileName, o.ExecParentCmdName, src.PodNamespace, src.PodName, src.File)

fmt.Fprintf(o.IOStreams.ErrOut, "warning: skipping symlink: %q -> %q (consider using \"%s exec -n %q %q -- tar cf - %q | tar xf -\")\n", destFileName, o.ExecParentCmdName, src.PodNamespace, src.PodName, src.File)

fejta-bot · 2019-09-03T20:29:42Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 29, 2019

k8s-ci-robot assigned liggitt and tallclair Aug 29, 2019

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 29, 2019

k8s-ci-robot requested review from dims and lavalamp August 29, 2019 19:42

liggitt reviewed Aug 29, 2019

View reviewed changes

Comment thread pkg/kubectl/cmd/cp/cp.go Outdated

tallclair reviewed Aug 29, 2019

View reviewed changes

liggitt mentioned this pull request Aug 30, 2019

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

Closed

odinuge reviewed Aug 30, 2019

View reviewed changes

liggitt mentioned this pull request Aug 31, 2019

[HOLD] Don't kubectl cp symlinks by default #82038

Closed

k8s-ci-robot added the area/security label Aug 31, 2019

k8s-ci-robot added this to the v1.16 milestone Aug 31, 2019

soltysh mentioned this pull request Sep 2, 2019

Allow kubectl cp to an existing directory #81782

Closed

soltysh force-pushed the cp_no_links branch from 73b8ea8 to 2a615ef Compare September 2, 2019 14:29

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 2, 2019

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

soltysh force-pushed the cp_no_links branch from 2a615ef to 258de3e Compare September 2, 2019 15:56

liggitt reviewed Sep 3, 2019

View reviewed changes

Remove symlink support from kubectl cp

786acdb

soltysh force-pushed the cp_no_links branch from 258de3e to 786acdb Compare September 3, 2019 08:44

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 3, 2019

odinuge approved these changes Sep 3, 2019

View reviewed changes

k8s-ci-robot assigned odinuge Sep 3, 2019

tallclair reviewed Sep 3, 2019

View reviewed changes

k8s-ci-robot merged commit 35e0f17 into kubernetes:master Sep 3, 2019

soltysh deleted the cp_no_links branch September 4, 2019 11:36

This was referenced Sep 4, 2019

Automated cherry pick of #82143: Remove symlink support from kubectl cp #82343

Closed

Automated cherry pick of #82143: Remove symlink support from kubectl cp #82344

Closed

Automated cherry pick of #82143: Remove symlink support from kubectl cp #82345

Closed

soltysh mentioned this pull request Oct 1, 2019

Update symlink warning #83350

Merged

tallclair mentioned this pull request Dec 5, 2019

kubectl cp is still escape when copy file from pod #80283

Closed

liggitt changed the title ~~Remove symlink support from kubectl cp~~ CVE-2019-11240: Remove symlink support from kubectl cp Feb 3, 2020

liggitt changed the title ~~CVE-2019-11240: Remove symlink support from kubectl cp~~ CVE-2019-11251: Remove symlink support from kubectl cp Feb 3, 2020

liggitt mentioned this pull request Feb 3, 2020

CVE-2019-11251: kubectl cp symlink vulnerability #87773

Closed

	fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is a symlink, skipping (consider using %s exec for symlink support)\n", destFileName, o.cmd.Parent().Name())
	fmt.Fprintf(o.IOStreams.ErrOut, "skipping symlink: %q -> %q\n", destFileName, linkTarget)

Conversation

soltysh commented Aug 29, 2019 • edited by liggitt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soltysh commented Aug 29, 2019

Uh oh!

liggitt commented Aug 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Aug 31, 2019

Uh oh!

soltysh commented Sep 2, 2019

Uh oh!

soltysh commented Sep 2, 2019

Uh oh!

soltysh commented Sep 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

soltysh commented Sep 3, 2019

Uh oh!

liggitt commented Sep 3, 2019

Uh oh!

k8s-ci-robot commented Sep 3, 2019

Uh oh!

odinuge left a comment

Choose a reason for hiding this comment

Uh oh!

odinuge commented Sep 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fejta-bot commented Sep 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

soltysh commented Aug 29, 2019 •

edited by liggitt

Loading