8000 Fix chroot Tar when copying from container root by cpuguy83 · Pull Request #39351 · moby/moby · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix chroot Tar when copying from container root #39351

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix chroot Tar when copying from container root #39351

wants to merge 1 commit into from

Conversation

cpuguy83
Copy link
Member
@cpuguy83 cpuguy83 commented Jun 11, 2019 •
edited by thaJeztah
Loading

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)"

tiborvass
tiborvass reviewed Jun 11, 2019
View reviewed changes
@thaJeztah
Copy link
Member
thaJeztah commented Jun 11, 2019 •
edited
Loading

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 thaJeztah mentioned this pull request Jun 11, 2019
Closed
@thaJeztah
Copy link
Member
thaJeztah commented Jun 11, 2019 •
edited
Loading

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

thaJeztah
thaJeztah reviewed Jun 11, 2019
View reviewed changes
@cpuguy83 cpuguy83 force-pushed the 39348_investigate_chroot_pack branch 4 times, most recently from 438d8db to df4fc6d Compare June 11, 2019 17:50
@cpuguy83
Copy link
Member Author

Updated and this is all green.

@tiborvass
Copy link
Contributor

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.

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

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.

@cpuguy83
Fix chroot Tar when copying from container root
55d38d7 
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 <cpuguy83@gmail.com>
@cpuguy83 cpuguy83 force-pushed the 39348_investigate_chroot_pack branch from 47a9c89 to 55d38d7 Compare June 11, 2019 21:35
@codecov
Copy link
codecov bot commented Jun 11, 2019 8000

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
Copy link
Member Author

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
tiborvass requested changes Jun 12, 2019
View reviewed changes
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
Copy link
Contributor
@tiborvass tiborvass Jun 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

And CI is running vfs as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@cpuguy83
Copy link
Member Author

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.

@cpuguy83 cpuguy83 closed this Jun 12, 2019
@cpuguy83 cpuguy83 deleted the 39348_investigate_chroot_pack branch June 12, 2019 18:32
@tiborvass tiborvass mentioned this pull request Jun 12, 2019
Merged
@thaJeztah thaJeztah mentioned this pull request Jun 18, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiborvass tiborvass tiborvass requested changes

@thaJeztah thaJeztah thaJeztah left review comments

@seemethere seemethere seemethere approved these changes

Assignees
No one assigned
Labels
status/2-code-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Regression due to CVE mitigation from 39292
5 participants
@cpuguy83 @thaJeztah @tiborvass @seemethere @GordonTheTurtle
0