Skip to content
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.

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

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

Merged
merged 2 commits into from Jun 4, 2019

Conversation

@cpuguy83
Copy link
Collaborator

@cpuguy83 cpuguy83 commented May 30, 2019

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

@cpuguy83 cpuguy83 commented May 30, 2019

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

@cpuguy83 cpuguy83 commented May 30, 2019

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

@cpuguy83 cpuguy83 force-pushed the cpuguy83:root_dir_on_copy branch May 30, 2019
@cpuguy83 cpuguy83 force-pushed the cpuguy83:root_dir_on_copy branch May 30, 2019
@codecov
Copy link

@codecov codecov bot commented May 30, 2019

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

@cpuguy83 cpuguy83 commented May 30, 2019

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 cpuguy83 force-pushed the cpuguy83:root_dir_on_copy branch May 30, 2019
@cpuguy83 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 cpuguy83 force-pushed the cpuguy83:root_dir_on_copy branch May 30, 2019
@cpuguy83
Copy link
Collaborator Author

@cpuguy83 cpuguy83 commented May 30, 2019

Ah missing a close, of course.

@cyphar
Copy link
Contributor

@cyphar cyphar commented May 30, 2019

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

@cpuguy83 cpuguy83 commented May 30, 2019

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

@cpuguy83 cpuguy83 force-pushed the cpuguy83:root_dir_on_copy branch May 30, 2019
@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented May 30, 2019

/cc @kolyshkin PTAL

}

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

This comment has been minimized.

@panpan0000

panpan0000 May 31, 2019

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

This comment has been minimized.

@cyphar

cyphar May 31, 2019
Contributor

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

This comment has been minimized.

@cpuguy83

cpuguy83 May 31, 2019
Author Collaborator

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

@cpuguy83 cpuguy83 commented May 31, 2019

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

@relyt0925 relyt0925 commented Jun 3, 2019

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

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jun 3, 2019

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
Copy link
Contributor

@cyphar cyphar commented Jun 3, 2019

@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
Copy link
Contributor

@justincormack justincormack commented Jun 3, 2019

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

cpuguy83 added 2 commits May 30, 2019
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 cpuguy83 force-pushed the cpuguy83:root_dir_on_copy branch to 3029e76 Jun 3, 2019
@cpuguy83
Copy link
Collaborator Author

@cpuguy83 cpuguy83 commented Jun 3, 2019

Added unit tests.

@cpuguy83
Copy link
Collaborator Author

@cpuguy83 cpuguy83 commented Jun 3, 2019

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

@panpan0000 panpan0000 commented Jun 4, 2019

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

@cpuguy83 cpuguy83 commented Jun 4, 2019

@panpan0000 all fixed

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jun 4, 2019

@tonistiigi LGTY?

@cyphar
Copy link
Contributor

@cyphar cyphar commented Jun 4, 2019

From my quick testing, this LGTM.

@AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Jun 4, 2019

merging

@AkihiroSuda AkihiroSuda merged commit 364f9bc into moby:master Jun 4, 2019
6 of 7 checks passed
6 of 7 checks passed
@GordonTheTurtle
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 25442 has failed
Details
@GordonTheTurtle
dco-signed All commits are signed
@GordonTheTurtle
experimental Jenkins build Docker-PRs-experimental 45514 has succeeded
Details
@GordonTheTurtle
janky Jenkins build Docker-PRs 54374 has succeeded
Details
@GordonTheTurtle
powerpc Jenkins build Docker-PRs-powerpc 14560 has succeeded
Details
@GordonTheTurtle
windowsRS5-process Jenkins build Docker-PRs-WoW-RS5-Process 2641 has succeeded
Details
@GordonTheTurtle
z Jenkins build Docker-PRs-s390x 14477 has succeeded
Details
@cpuguy83 cpuguy83 deleted the cpuguy83:root_dir_on_copy branch Jun 4, 2019
@thaJeztah thaJeztah added this to the 20.03.0 milestone Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants