Fix chroot Tar when copying from container root by cpuguy83 · Pull Request #39351 · moby/moby

cpuguy83 · 2019-06-11T03:01:38Z

When the requested container path is the root, the path that is
requested to be tared up is the parent directory of the container
rootfs.
Really this isn't any different than other cases, however in this case
we end up chrooting to the container rootfs and the requsted tar path
goes out of scope.

Fixes #39348 "Regression due to CVE mitigation from 39292"
relates to #39292 "Pass root to chroot to for chroot Tar/Untar (CVE-2018-15664)"

integration/container/copy_test.go

thaJeztah · 2019-06-11T07:59:54Z

Looks like this test is a bit flaky recently;

https://jenkins.dockerproject.org/job/Docker-PRs-WoW-RS5-Process/2722/console

03:45:02 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
03:45:02 
03:45:02 assertion failed: 
03:45:02 Command:  d:\CI\CI-f4d92b94d9\binary\docker.exe run -i --name test-inter-restart --restart=always busybox sh
03:45:02 ExitCode: 0
03:45:02 Stdout:   
03:45:02 Stderr:   
03:45:02 
03:45:02 Failures:
03:45:02 ExitCode was 0 expected 11

opened #39352

thaJeztah · 2019-06-11T08:01:13Z

Linting failure;

03:03:19 integration/container/copy_test.go:93:5:warning: ineffectual assignment to err (ineffassign)
03:03:20 Build step 'Execute shell' marked build as failure

integration/container/copy_test.go

cpuguy83 · 2019-06-11T20:16:02Z

Updated and this is all green.

tiborvass · 2019-06-11T21:15:27Z

I was hoping to not add more special cases (this part of the code is FULL of that) but if this is fine for others I don't want to block it. Would be great to have the test run on Windows too.

tiborvass · 2019-06-11T21:34:56Z

On second thought, this corner case is basically never even a thing on Windows because of how base images are on Windows. So I'm fine with skipping the test on windows, as long as other windows tests don't fail because of this PR.

When the requested container path is the root, the path that is requested to be tared up is the parent directory of the container rootfs. Really this isn't any different than other cases, however in this case we end up chrooting to the container rootfs and the requsted tar path goes out of scope. Signed-off-by: Brian Goff <[email protected]>

codecov · 2019-06-11T21:35:21Z

Codecov Report

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

@@            Coverage Diff            @@
##             master   #39351   +/-   ##
=========================================
  Coverage          ?   36.97%           
=========================================
  Files             ?      612           
  Lines             ?    45643           
  Branches          ?        0           
=========================================
  Hits              ?    16875           
  Misses            ?    26482           
  Partials          ?     2286

cpuguy83 · 2019-06-11T21:39:23Z

Pushed an update thinking maybe it would be simple to support Windows but after discussing with Tibor on Slack, yeah agreed it's probably not so simple, and we have other tests which are covered on Windows... so pushed again to add the skip back in for the new test.

tiborvass · 2019-06-12T01:25:21Z

daemon/archive.go

+	// handle a case where the source path that was passed in is actually the parent of `root`
+	// this would happen when the requested archive path *is* the root.
+	if filepath.Dir(root) == src {
+		root = src


this is not good because it only works for overlay2. For vfs, since the layout is different (no merged directory), src will be equal to /var/lib/docker/vfs/dir while root will be /var/lib/docker/vfs/dir/$ID. ~~You don't want to tar all of /var/lib/docker/vfs/dir~~. The chroot will happen one level above what's desired.

I debugged root and src when running -e DOCKER_GRAPHDRIVER=vfs

This is the chroot path, not changing the source path.
I'm running my new test against vfs and it seems to be ok.

And CI is running vfs as well.

I edited the first comment of this thread, I incorrectly said it would tar all the vfs dirs, when in fact the problem is that the chroot would not happen at the correct directory.

@cpuguy83 sorry i didn't see your comments here (got github cached). I wasn't expecting the tests to fail under vfs.

cpuguy83 · 2019-06-12T18:32:43Z

This opens us back up to the same attack for docker cp from, albeit with a more limited scope.
But in any case it's not the right fix so closing.

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

cpuguy83 added status/2-code-review and removed status/0-triage labels Jun 11, 2019

tiborvass reviewed Jun 11, 2019

View reviewed changes

integration/container/copy_test.go Outdated Show resolved Hide resolved

thaJeztah added the process/cherry-pick label Jun 11, 2019

thaJeztah mentioned this pull request Jun 11, 2019

Flaky test: TestRunInteractiveWithRestartPolicy #39352

Closed

thaJeztah reviewed Jun 11, 2019

View reviewed changes

integration/container/copy_test.go Outdated Show resolved Hide resolved

cpuguy83 force-pushed the 39348_investigate_chroot_pack branch 4 times, most recently from 438d8db to df4fc6d Compare June 11, 2019 17:50

seemethere approved these changes Jun 11, 2019

View reviewed changes

cpuguy83 force-pushed the 39348_investigate_chroot_pack branch from df4fc6d to 47a9c89 Compare June 11, 2019 21:26

cpuguy83 force-pushed the 39348_investigate_chroot_pack branch from 47a9c89 to 55d38d7 Compare June 11, 2019 21:35

tiborvass requested changes Jun 12, 2019

View reviewed changes

cpuguy83 added the rebuild/powerpc label Jun 12, 2019

GordonTheTurtle removed the rebuild/powerpc label Jun 12, 2019

cpuguy83 closed this Jun 12, 2019

cpuguy83 deleted the 39348_investigate_chroot_pack branch June 12, 2019 18:32

tiborvass mentioned this pull request Jun 12, 2019

Fix docker cp when container source path is / #39357

Merged

thaJeztah mentioned this pull request Jun 18, 2019

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

Merged

thaJeztah removed the process/cherry-pick label Jul 13, 2019

Conversation

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

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

cpuguy83 commented Jun 11, 2019

Uh oh!

tiborvass commented Jun 11, 2019

Uh oh!

tiborvass commented Jun 11, 2019

Uh oh!

codecov bot commented Jun 11, 2019

Codecov Report

Uh oh!

cpuguy83 commented Jun 11, 2019

Uh oh!

tiborvass Jun 12, 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!

tiborvass Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

tiborvass Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

tiborvass Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Jun 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cpuguy83 commented Jun 11, 2019 •

edited by thaJeztah

Loading

thaJeztah commented Jun 11, 2019 •

edited

Loading

thaJeztah commented Jun 11, 2019 •

edited

Loading

tiborvass Jun 12, 2019 •

edited

Loading