Fix docker cp when container source path is / by tiborvass · Pull Request #39357 · moby/moby

tiborvass · 2019-06-12T19:01:08Z

Another attempt at fixing #39348
Fixes #39348
Previous attempt at #39351

Before 7a7357d, archive.TarResourceRebase was being used to copy files
and folders from the container. That function splits the source path
into a dirname + basename pair to support copying a file:
if you wanted to tar dir/file it would tar from dir the file file
(as part of the IncludedFiles option).

However, that path splitting logic was kept for folders as well, which
resulted in weird inputs to archive.TarWithOptions:
if you wanted to tar dir1/dir2 it would tar from dir1 the directory
dir2 (as part of IncludedFiles option).

Although it was weird, it worked fine until we started chrooting into
the container rootfs when doing a docker cp with container source set
to / (cf 3029e76 (#39292)).

The fix is to only do the path splitting logic if the source is a file.

Unfortunately, 7a7357d added support for LCOW by duplicating some of
this subtle logic. Ideally we would need to do more refactoring of the
archive codebase to properly encapsulate these behaviors behind well-
documented APIs.

This fix does not do that. Instead, it fixes the issue inline.

Signed-off-by: Tibor Vass tibor@docker.com

I added a couple of more tests than the actual issue needs, just to make sure there are no other regressions compared to before the cve fix (3029e76).

Huge thanks to @cpuguy83 ❤️who worked tirelessly with me to understand the code and make this PR.

CID=$(docker create alpine) docker cp $CID:/ out Signed-off-by: Brian Goff <cpuguy83@gmail.com> Signed-off-by: Tibor Vass <tibor@docker.com>

codecov · 2019-06-12T22:17:59Z

Codecov Report

❗ No coverage uploaded for pull request base (master@4dc6b21). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #39357   +/-   ##
=========================================
  Coverage          ?   36.97%           
=========================================
  Files             ?      612           
  Lines             ?    45645           
  Branches          ?        0           
=========================================
  Hits              ?    16877           
  Misses            ?    26480           
  Partials          ?     2288

cpuguy83 · 2019-06-12T22:37:51Z

15:29:30 --- FAIL: TestCopyFromContainerRoot (1.83s)
15:29:30     copy_test.go:146: assertion failed: expression is false: found["/foo"]: /foo file not found in archive
15:29:30     copy_test.go:147: assertion failed: expression is false: found["/bar/baz"]: /bar/baz file not found in archive

tiborvass · 2019-06-13T06:28:52Z

Ping @cpuguy83 @thaJeztah @tonistiigi @justincormack

Signed-off-by: Tibor Vass <tibor@docker.com>

Before 7a7357d, archive.TarResourceRebase was being used to copy files and folders from the container. That function splits the source path into a dirname + basename pair to support copying a file: if you wanted to tar `dir/file` it would tar from `dir` the file `file` (as part of the IncludedFiles option). However, that path splitting logic was kept for folders as well, which resulted in weird inputs to archive.TarWithOptions: if you wanted to tar `dir1/dir2` it would tar from `dir1` the directory `dir2` (as part of IncludedFiles option). Although it was weird, it worked fine until we started chrooting into the container rootfs when doing a `docker cp` with container source set to `/` (cf 3029e76). The fix is to only do the path splitting logic if the source is a file. Unfortunately, 7a7357d added support for LCOW by duplicating some of this subtle logic. Ideally we would need to do more refactoring of the archive codebase to properly encapsulate these behaviors behind well- documented APIs. This fix does not do that. Instead, it fixes the issue inline. Signed-off-by: Tibor Vass <tibor@docker.com>

pkg/archive/archive_unix.go

daemon/archive.go

pkg/archive/archive_unix.go

cpuguy83

LGTM

tonistiigi · 2019-06-14T01:03:11Z

daemon/archive.go

+	sourceDir := resolvedPath
+	sourceBase := "."
+
+	if stat.Mode&os.ModeDir == 0 { // not dir


I think this should be

if symlink { opts = rebase }

and always archivePath(resolvedPath).

Would be more understandable what is actually happening and wouldn't need the hack for removing the slash in the walker. But functionally they seem equivalent.

Agreed. Can be done in a follow up.

tiborvass · 2019-06-14T01:36:33Z

@thaJeztah @kolyshkin updated with small requested change.

Previously, getWalkRoot("/", "foo") would return "//foo" Now it returns "/foo" Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass · 2019-06-14T06:42:44Z

Failure is TestAPISwarmLeaderElection which is unrelated

thaJeztah

LGTM

thaJeztah · 2019-06-14T08:55:22Z

Looks like s390x passed, but is hanging on the cleanup; https://jenkins.dockerproject.org/job/Docker-PRs-s390x/14591/console

06:06:03 OK: 1421 passed, 81 skipped
06:06:03 PASS
06:06:03 ---> Making bundle: .integration-daemon-stop (in bundles/test-integration)
06:06:16 Clearing AppArmor profiles cache:.
06:06:16 All profile caches have been cleared, but no profiles have been unloaded.
06:06:16 Unloading profiles will leave already running processes permanently
06:06:16 unconfined, which can lead to unexpected situations.
06:06:16 
06:06:16 To set a process to complain mode, use the command line tool
06:06:16 'aa-complain'. To really tear down all profiles, run the init script
06:06:16 with the 'teardown' option."
06:06:16 Removing test suite binaries

…directory traversal. moby/moby#39357

GordonTheTurtle added the status/0-triage label Jun 12, 2019

tiborvass force-pushed the cp-slash-fix branch 2 times, most recently from 33c5db2 to cac6afc Compare June 12, 2019 20:44

Add test for copying entire container rootfs

6db9f1c

CID=$(docker create alpine) docker cp $CID:/ out Signed-off-by: Brian Goff <cpuguy83@gmail.com> Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the cp-slash-fix branch from cac6afc to 6b9bfcc Compare June 12, 2019 22:17

tiborvass force-pushed the cp-slash-fix branch 3 times, most recently from 087d4c3 to d718ac0 Compare June 13, 2019 06:22

tiborvass marked this pull request as ready for review June 13, 2019 06:28

Tibor Vass added 2 commits June 13, 2019 06:31

add more tests

02f1eb8

Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the cp-slash-fix branch from d718ac0 to 9e0876a Compare June 13, 2019 06:31

tiborvass added rebuild/windowsRS5-process status/2-code-review and removed status/0-triage labels Jun 13, 2019

thaJeztah added process/cherry-pick and removed rebuild/windowsRS5-process labels Jun 13, 2019

tiborvass commented Jun 13, 2019

View reviewed changes

pkg/archive/archive_unix.go Outdated Show resolved Hide resolved

thaJeztah reviewed Jun 13, 2019

View reviewed changes

daemon/archive.go Show resolved Hide resolved

thaJeztah requested changes Jun 13, 2019

View reviewed changes

pkg/archive/archive_unix.go Outdated Show resolved Hide resolved

pkg/archive/archive_unix.go Outdated Show resolved Hide resolved

cpuguy83 approved these changes Jun 13, 2019

View reviewed changes

tonistiigi approved these changes Jun 14, 2019

View reviewed changes

tonistiigi reviewed Jun 14, 2019

View reviewed changes

tiborvass force-pushed the cp-slash-fix branch from 9e0876a to 3d3bdd9 Compare June 14, 2019 01:35

tiborvass mentioned this pull request Jun 14, 2019

[19.03] Add chroot back to Tar/Untar without the previously introduced regression docker-archive/engine#280

Merged

pkg/archive: keep walkRoot clean if source is /

7410f1a

Previously, getWalkRoot("/", "foo") would return "//foo" Now it returns "/foo" Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the cp-slash-fix branch from 3d3bdd9 to 7410f1a Compare June 14, 2019 03:58

thaJeztah added status/4-merge and removed status/2-code-review labels Jun 14, 2019

thaJeztah approved these changes Jun 14, 2019

View reviewed changes

thaJeztah merged commit fb5fe24 into moby:master Jun 14, 2019

thaJeztah mentioned this pull request Jun 18, 2019

[18.09 backport] Fix docker cp when container source path is / docker-archive/engine#286

Merged

kaosagnt added a commit to kaosagnt/boot2docker-xfs-ng that referenced this pull request Jun 24, 2019

Docker 18.09.7-rc1. Fix: CVE-2018-15664 symlink-exchange attack with …

9df94d4

…directory traversal. moby/moby#39357

tiborvass deleted the cp-slash-fix branch July 17, 2019 00:34

thaJeztah mentioned this pull request Aug 13, 2019

[19.03 backport] Integration: change signatures to fix golint warnings docker-archive/engine#281

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Aug 13, 2019

thaJeztah added this to the 20.03.0 milestone Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix docker cp when container source path is /#39357

Fix docker cp when container source path is /#39357
thaJeztah merged 4 commits intomoby:masterfrom
tiborvass:cp-slash-fix

tiborvass commented Jun 12, 2019 •

edited by thaJeztah

Loading

Uh oh!

codecov bot commented Jun 12, 2019 •

edited

Loading

Uh oh!

cpuguy83 commented Jun 12, 2019

Uh oh!

tiborvass commented Jun 13, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpuguy83 left a comment

Uh oh!

tonistiigi Jun 14, 2019

Uh oh!

tiborvass Jun 14, 2019

Uh oh!

tiborvass commented Jun 14, 2019

Uh oh!

tiborvass commented Jun 14, 2019

Uh oh!

thaJeztah left a comment

Uh oh!

thaJeztah commented Jun 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

tiborvass commented Jun 12, 2019 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cpuguy83 commented Jun 12, 2019

Uh oh!

tiborvass commented Jun 13, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

tonistiigi Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

tiborvass Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Jun 14, 2019

Uh oh!

tiborvass commented Jun 14, 2019

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jun 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tiborvass commented Jun 12, 2019 •

edited by thaJeztah

Loading

codecov bot commented Jun 12, 2019 •

edited

Loading