Pass root to chroot to for chroot Tar/Untar (CVE-2018-15664) #39292

cpuguy83 · 2019-05-30T18:18:48Z

This is useful for preventing CVE-2018-15664 where a malicious container
process can take advantage of a race on symlink resolution/sanitization.

Before this change chrootarchive would chroot to the destination
directory which is attacker controlled. With this patch we always chroot
to the container's root which is not attacker controlled.

replaces #39252

cpuguy83 · 2019-05-30T18:21:14Z

fyi, I still need to test this (to see if it actually fixes the CVE), but wanted to get this out to there for others to review.

cpuguy83 · 2019-05-30T19:51:24Z

In my testing this does seem to fix expected part of this vulnerability (copy file to a container).

codecov · 2019-05-30T22:01:50Z

Codecov Report

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

@@            Coverage Diff            @@
##             master   #39292   +/-   ##
=========================================
  Coverage          ?   36.99%           
=========================================
  Files             ?      612           
  Lines             ?    45553           
  Branches          ?        0           
=========================================
  Hits              ?    16852           
  Misses            ?    26413           
  Partials          ?     2288

cpuguy83 · 2019-05-30T22:04:11Z

And now I have added a new command for docker-tar which chroots for tar operations.
The tests I ran seem to pass for it (some manual testing and some integration tests), however @cyphar's script (run_read.sh) seems to hang the API on copying the tar stream to the client, but only when using that new command.

cpuguy83 · 2019-05-30T22:31:34Z

Ah missing a close, of course.

cyphar · 2019-05-30T22:33:08Z

From a cursory look, this does look like it should fix the issue. I'll close my PR in favour of this one. Though, we still need quite a few more drastic changes to FollowSymlinkInScope and all of its users.

cpuguy83 · 2019-05-30T22:34:12Z

So, seeing some "SUCCESS" on the read script need to figure out why.

thaJeztah · 2019-05-30T23:19:20Z

/cc @kolyshkin PTAL

panpan0000 · 2019-05-31T09:20:24Z

pkg/chrootarchive/archive.go

 }

 // UntarUncompressed reads a stream of bytes from `archive`, parses it as a tar archive,
 // and unpacks it into the directory at `dest`.
 // The archive must be an uncompressed stream.
 func UntarUncompressed(tarArchive io.Reader, dest string, options *archive.TarOptions) error {
-	return untarHandler(tarArchive, dest, options, false)
+	return untarHandler(tarArchive, dest, options, false, dest)


the 5th parameter is the same with 2nd parameter , both dest. is it by purpose ?
it will make the chroot folder always as "/" ?

Same with Untar -- is there a reason to have the old functions around given that they're broken?

I just didn't want to make a bunch of changes to code that isn't affected by this.
In reality there are only a couple of places this is used, I think it's safe to go ahead just change the method signatures rather than introduce a new one.

cpuguy83 · 2019-05-31T21:12:56Z

Ok in my local testing this seems to be working well. Curious what others are seeing.
All tests are green (except windows rs1 b/c CI seems to be hitting the same failure on every PR).

@cyphar I'll push an additional commit which consolidates the Untar stuff to require a root to chroot to.

relyt0925 · 2019-06-03T12:38:43Z

Sorry if this isn’t the right place but will this fix be backported to 18.06?

thaJeztah · 2019-06-03T13:12:51Z

will this fix be backported to 18.06?

Docker 18.06 reached EOL, so no, no backport to that version. It will be backported to versions that are still actively maintained (Docker 18.09 and the upcoming 19.03, as well as Docker Enterprise versions (17.06 EE, 18.03 EE, 18.09 EE))

cyphar · 2019-06-03T13:33:10Z

@justincormack I will run a test tomorrow morning, but I believe @cpuguy83 mentioned that he was running the symlink attack scripts I published to verify that the patch worked.

justincormack · 2019-06-03T15:50:22Z

@cyphar yes I believe he did but just wanted you to check too in case they were not exactly the same tests.

This is useful for preventing CVE-2018-15664 where a malicious container process can take advantage of a race on symlink resolution/sanitization. Before this change chrootarchive would chroot to the destination directory which is attacker controlled. With this patch we always chroot to the container's root which is not attacker controlled. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Previously only unpack operations were supported with chroot. This adds chroot support for packing operations. This prevents potential breakouts when copying data from a container. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 · 2019-06-03T16:48:12Z

Added unit tests.

cpuguy83 · 2019-06-03T16:49:56Z

I believe we are good to go here.
There's some extra hardening we could probably do, like setting uid/gid maps so that docker-tar and docker-untar are not running as root, but that's just extra that we can do separately.

panpan0000 · 2019-06-04T11:22:12Z

I believe we are good to go here.
There's some extra hardening we could probably do, like setting uid/gid maps so that docker-tar and docker-untar are not running as root, but that's just extra that we can do separately.

Hi, @cpuguy83, thank you to provide this fix.
How about when you mentioned seeing some "SUCCESS" on the read script need to figure out why. ?

cpuguy83 · 2019-06-04T12:47:19Z

@panpan0000 all fixed

thaJeztah · 2019-06-04T12:54:31Z

@tonistiigi LGTY?

cyphar · 2019-06-04T13:00:40Z

From my quick testing, this LGTM.

AkihiroSuda · 2019-06-04T16:05:02Z

merging

GordonTheTurtle added the status/0-triage label May 30, 2019

cpuguy83 force-pushed the root_dir_on_copy branch from dace5ed to 6cb4c08 Compare May 30, 2019 20:01

cpuguy83 added status/2-code-review and removed status/0-triage labels May 30, 2019

cpuguy83 force-pushed the root_dir_on_copy branch from 6cb4c08 to d630c68 Compare May 30, 2019 21:50

cpuguy83 force-pushed the root_dir_on_copy branch from be1219c to 8978894 Compare May 30, 2019 22:06

cpuguy83 mentioned this pull request May 30, 2019

daemon: archive: pause containers before doing filesystem operations #39252

Closed

cpuguy83 changed the title ~~Pass root to chroot to for chroot Untar~~ Pass root to chroot to for chroot Tar/Untar May 30, 2019

cpuguy83 force-pushed the root_dir_on_copy branch from 8978894 to 4e524ee Compare May 30, 2019 22:09

cpuguy83 force-pushed the root_dir_on_copy branch from 4e524ee to 2d377c6 Compare May 30, 2019 23:12

ZYecho mentioned this pull request May 31, 2019

feature: add pouch cp cmd AliyunContainerService/pouch#2879

Merged

panpan0000 reviewed May 31, 2019

View reviewed changes

cpuguy83 force-pushed the root_dir_on_copy branch 2 times, most recently from c23a5dc to 497dcff Compare May 31, 2019 17:50

cpuguy83 added the rebuild/windowsRS1 label May 31, 2019

GordonTheTurtle removed the rebuild/windowsRS1 label May 31, 2019

cpuguy83 added the rebuild/windowsRS1 label May 31, 2019

GordonTheTurtle removed the rebuild/windowsRS1 label May 31, 2019

cpuguy83 requested review from kolyshkin, tonistiigi and justincormack May 31, 2019 21:38

thaJeztah changed the title ~~Pass root to chroot to for chroot Tar/Untar~~ Pass root to chroot to for chroot Tar/Untar (CVE-2018-15664) Jun 3, 2019

cpuguy83 added 2 commits June 3, 2019 09:45

Add chroot for tar packing operations

3029e76

Previously only unpack operations were supported with chroot. This adds chroot support for packing operations. This prevents potential breakouts when copying data from a container. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 force-pushed the root_dir_on_copy branch from 497dcff to 3029e76 Compare June 3, 2019 16:46

This was referenced Jun 3, 2019

[18.09 backport] Pass root to chroot to for chroot Tar/Untar (CVE-2018-15664) docker-archive/engine#253

Merged

[19.03 backport] Pass root to chroot to for chroot Tar/Untar (CVE-2018-15664) docker-archive/engine#254

Merged

AkihiroSuda merged commit 364f9bc into moby:master Jun 4, 2019

cpuguy83 deleted the root_dir_on_copy branch June 4, 2019 16:19

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Jun 5, 2019

tiborvass mentioned this pull request Jun 10, 2019

Regression due to CVE mitigation from 39292 #39348

Closed

cpuguy83 mentioned this pull request Jun 11, 2019

Fix chroot Tar when copying from container root #39351

Closed

tiborvass mentioned this pull request Jun 18, 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

novas0x2a mentioned this pull request Jun 28, 2019

build failure on !linux !windows platforms #39437

Closed

cpuguy83 mentioned this pull request Jul 4, 2019

Add realChroot for non linux/windows #39462

Merged

This was referenced Jul 4, 2019

[19.03 backport] Add realChroot for non linux/windows docker-archive/engine#294

Merged

[18.09 backport] Add realChroot for non linux/windows docker-archive/engine#295

Merged

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

Pass root to chroot to for chroot Tar/Untar (CVE-2018-15664) #39292

Pass root to chroot to for chroot Tar/Untar (CVE-2018-15664) #39292

cpuguy83 commented May 30, 2019 •

edited by thaJeztah

Loading

cpuguy83 commented May 30, 2019

cpuguy83 commented May 30, 2019

codecov bot commented May 30, 2019 •

edited

Loading

cpuguy83 commented May 30, 2019

cpuguy83 commented May 30, 2019

cyphar commented May 30, 2019 •

edited

Loading

cpuguy83 commented May 30, 2019

thaJeztah commented May 30, 2019

panpan0000 May 31, 2019

cyphar May 31, 2019

cpuguy83 May 31, 2019

cpuguy83 commented May 31, 2019

relyt0925 commented Jun 3, 2019

thaJeztah commented Jun 3, 2019

cyphar commented Jun 3, 2019

justincormack commented Jun 3, 2019

cpuguy83 commented Jun 3, 2019

cpuguy83 commented Jun 3, 2019

panpan0000 commented Jun 4, 2019

cpuguy83 commented Jun 4, 2019

thaJeztah commented Jun 4, 2019

cyphar commented Jun 4, 2019

AkihiroSuda commented Jun 4, 2019

Pass root to chroot to for chroot Tar/Untar (CVE-2018-15664) #39292

Pass root to chroot to for chroot Tar/Untar (CVE-2018-15664) #39292

Conversation

cpuguy83 commented May 30, 2019 • edited by thaJeztah Loading

cpuguy83 commented May 30, 2019

cpuguy83 commented May 30, 2019

codecov bot commented May 30, 2019 • edited Loading

Codecov Report

cpuguy83 commented May 30, 2019

cpuguy83 commented May 30, 2019

cyphar commented May 30, 2019 • edited Loading

cpuguy83 commented May 30, 2019

thaJeztah commented May 30, 2019

panpan0000 May 31, 2019

Choose a reason for hiding this comment

cyphar May 31, 2019

Choose a reason for hiding this comment

cpuguy83 May 31, 2019

Choose a reason for hiding this comment

cpuguy83 commented May 31, 2019

relyt0925 commented Jun 3, 2019

thaJeztah commented Jun 3, 2019

cyphar commented Jun 3, 2019

justincormack commented Jun 3, 2019

cpuguy83 commented Jun 3, 2019

cpuguy83 commented Jun 3, 2019

panpan0000 commented Jun 4, 2019

cpuguy83 commented Jun 4, 2019

thaJeztah commented Jun 4, 2019

cyphar commented Jun 4, 2019

AkihiroSuda commented Jun 4, 2019

cpuguy83 commented May 30, 2019 •

edited by thaJeztah

Loading

codecov bot commented May 30, 2019 •

edited

Loading

cyphar commented May 30, 2019 •

edited

Loading