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

Ensure docker cp cannot traverse outside container rootfs #5720

Merged
merged 3 commits into from
May 14, 2014

Conversation

cyphar
Copy link
Contributor

@cyphar cyphar commented May 10, 2014

This patch fixes the bug (#5656) that allowed cp to copy files outside of the containers rootfs, by passing a relative path (such as ../../../../../../../../etc/shadow). This is fixed by first converting the path to an absolute path (relative to /) and then appending it to the container's rootfs before continuing.

Docker-DCO-1.1-Signed-off-by: Aleksa Sarai cyphar@cyphar.com (github: cyphar)

EDIT: I want to point out that while the issue (#5656) is closed, the vulnerability still works. The problem is that the docker socket can, even when running in multi-tenant situations, be used to read files as root since paths aren't sanitised properly. This problem has yet to be fixed.

EDIT 2: This patch also now includes fixes to general path generation in the daemon subsystem, in order to try and prevent future exploits in the same vein as this one.

Fixes #5656

/cc @shykes @creack @vieux @crosbymichael

@cyphar
Copy link
Contributor Author

cyphar commented May 10, 2014

Although, there should be a single container method that will get the full path of a resource in the container's rootfs. And everything that creates their own path from a resource should use that method instead...

EDIT: Added.

@LK4D4
Copy link
Contributor

LK4D4 commented May 10, 2014

I see that you pretty familiar with docker cp now :) Can you please write a tests for your changes?

@cyphar
Copy link
Contributor Author

cyphar commented May 10, 2014

The archive package needs to be reworked. It will only decompress and compress from files located at strings, rather than taking an interface (such as io.Reader and io.Writer), which would massively improve testing. But yeah, I've added the tests.

@LK4D4
Copy link
Contributor

LK4D4 commented May 10, 2014

integration is deprecated :) Can you please move your tests to integration-cli?
Also join to #docker-dev to discuss archive package.

@cyphar
Copy link
Contributor Author

cyphar commented May 11, 2014

Rebased after moving the tests, and adding myself to AUTHORS.

/ping @shykes @creack @vieux @crosbymichael

@vieux
Copy link
Contributor

vieux commented May 12, 2014

LGTM

@vieux
Copy link
Contributor

vieux commented May 12, 2014

ping @unclejack

@crosbymichael
Copy link
Contributor

@cyphar this needs rebased. I moved the symlink func to a separate pkg in pkg/symlink

This patch fixes the bug that allowed cp to copy files outside of
the containers rootfs, by passing a relative path (such as
../../../../../../../../etc/shadow). This is fixed by first converting
the path to an absolute path (relative to /) and then appending it
to the container's rootfs before continuing.

Docker-DCO-1.1-Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> (github: cyphar)
This patch adds integration tests for the copying of resources
from a container, to ensure that regressions in the security of
resource copying can be easily discovered.

Docker-DCO-1.1-Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> (github: cyphar)
This patch is a preventative patch, it fixes possible future
vulnerabilities regarding unsantised paths. Due to several recent
vulnerabilities, wherein the docker daemon could be fooled into
accessing data from the host (rather than a container), this patch
was created to try and mitigate future possible vulnerabilities in
the same vein.

Docker-DCO-1.1-Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> (github: cyphar)
@cyphar
Copy link
Contributor Author

cyphar commented May 14, 2014

@crosbymichael rebase'd.

Bumping, since this vulnerability is very serious.
/ping @shykes @creack @crosbymichael @unclejack

@unclejack
Copy link
Contributor

@cyphar Just to make it clear, it's a bug, not a vulnerability per se. If you can run a privileged container, you've got root on the host and that lets you do this and far more.

@unclejack
Copy link
Contributor

LGTM

1 similar comment
@crosbymichael
Copy link
Contributor

LGTM

crosbymichael added a commit that referenced this pull request May 14, 2014
Ensure `docker cp` cannot traverse outside container rootfs
@crosbymichael crosbymichael merged commit 4af465f into moby:master May 14, 2014
@cyphar cyphar deleted the 5656-cp-absolute-paths branch May 15, 2014 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access to docker daemon gives unrestricted read access to host filesystem
5 participants