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.

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

Resolve symlinks in cp #3214

Merged
merged 9 commits into from
May 30, 2019
Merged

Resolve symlinks in cp #3214

merged 9 commits into from
May 30, 2019

Conversation

mheon
Copy link
Member

@mheon mheon commented May 28, 2019

Fixes CVE-2018-15664 and #3211

Still needs manpage updates, bash completion updates, and a test

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M labels May 28, 2019
@vrothberg
Copy link
Member

vrothberg commented May 28, 2019 •
edited
Loading

That looks very painless 👍 Changes LGTM but I'd love to have some tests for #3211.

@QiWang19
Copy link
Contributor

LGTM

@baude
Copy link
Member

baude commented May 28, 2019

lgtm, test would be nth

@mheon
Copy link
Member Author

mheon commented May 28, 2019

Added a test

vrothberg
vrothberg reviewed May 28, 2019
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @mheon

@baude
Copy link
Member

baude commented May 29, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 29, 2019
@baude
Copy link
Member

baude commented May 29, 2019

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 29, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 29, 2019
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mheon
Copy link
Member Author

mheon commented May 29, 2019

Update: fix here is still vulnerable to races. I need to move copy code inside libpod to avoid them (so we can grab the container lock while copying)

@mheon
Copy link
Member Author

mheon commented May 29, 2019

Alright, I think this is "good enough" for a release. There's further work to be done, but that will require a much larger refactor.

@rhatdan
Copy link
Member

rhatdan commented May 29, 2019

/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels May 29, 2019
@mheon
Copy link
Member Author

mheon commented May 30, 2019

Hm. Rootless containers don't have CGroups, so we can't pause them.

As such, we can't really mitigate for them... Hm.

@mheon
Copy link
Member Author

mheon commented May 30, 2019

With any luck, tests will go green now.

mheon added 2 commits May 29, 2019 22:53
@mheon
Use securejoin to merge paths in podman cp
7b7d542 
Securejoin ensures that paths are resolved in the container, not
on the host.

Fixes containers#3211

Signed-off-by: Matthew Heon <matthew.heon@pm.me>
@mheon
Pause containers while copying into them
49dc185 
Should fix CVE-2018-15664 for Podman.

Signed-off-by: Matthew Heon <matthew.heon@pm.me>
mheon added 5 commits May 29, 2019 22:53
@mheon
Add --pause to podman cp manpage and bash completions
32fc6b9 
Signed-off-by: Matthew Heon <matthew.heon@pm.me>
@mheon
Add test to ensure symlinks are resolved in ctr scope
431e633 
Signed-off-by: Matthew Heon <matthew.heon@pm.me>
@mheon
Tolerate non-running containers in paused cp
79990b7 
Signed-off-by: Matthew Heon <matthew.heon@pm.me>
@mheon
Fix bug in e2e tests for podman cp
f456825 
Signed-off-by: Matthew Heon <matthew.heon@pm.me>
@mheon
We can't pause rootless containers during cp
48e35f7 
Rootless containers can't be paused (no CGroups, so no freezer).

We could try and emulate this with a SIGSTOP to all PIDs in the
container, but that's inherently racy, so let's avoid it for now.

Signed-off-by: Matthew Heon <matthew.heon@pm.me>
@rhatdan
Copy link
Member

rhatdan commented May 30, 2019

@mheon How about we fail and say you can not copy on a running container when in rootless mode?

@mheon
Error when trying to copy into a running rootless ctr
57d4093 
We can't pause them, so if that's requested, throw an error.

Signed-off-by: Matthew Heon <matthew.heon@pm.me>
@mheon
Copy link
Member Author

mheon commented May 30, 2019

Good idea - pushed another commit. We now error on trying to copy into running rootless container with --pause set (which is the default).

You can still do this with --pause=false

@mheon
Fix podman cp tests
5a07311 
Signed-off-by: Matthew Heon <matthew.heon@pm.me>
@mheon
Copy link
Member Author

mheon commented May 30, 2019

OK, tests mostly passing. Restarted one flake.
@baude @rhatdan Someone want to LGTM again?

@baude
Copy link
Member

baude commented May 30, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 30, 2019
@mheon
Copy link
Member Author

mheon commented May 30, 2019

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 30, 2019
@openshift-merge-robot openshift-merge-robot merged commit 2dcfd3d into containers:master May 30, 2019
@rh-atomic-bot rh-atomic-bot mentioned this pull request May 30, 2019
Merged
@sfowl
Copy link

sfowl commented May 31, 2019

CVE-2019-10152 has been assigned for this issue:

https://bugzilla.redhat.com/show_bug.cgi?id=1715667

@ret2libc
Copy link

What about the problems outlined in moby/moby#39252 (comment) ? It seems docker is not going to pause containers, for what I understand, because it is a breaking change (e.g. copying big files may stop the process for too long and breaking network connections).

Is podman affected by the same issue?

giuseppe
giuseppe reviewed May 31, 2019
View reviewed changes
cmd/podman/cp.go
} else if err == nil {
// Only add the defer if we actually paused
defer func() {
if err := ctr.Unpause(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

is it possible that two processes are cp'ing at the same time and one of them unpause the container while the other process is still resolving the symlinks?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. That's part of why I was looking to move cp into the container.

Copy link
Member Author

Choose a reason for hiding this comment

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

Into libpod, rather

@mheon
Copy link
Member Author

mheon commented May 31, 2019

@ret2libc Hm. We should be able to grab that - it's definitely a more elegant solution than this. We'll keep this in for now, but I'll avoid cutting a release with it until we verify if we can swap to the alternative fix - in which case we can revert this.

@rhatdan
Copy link
Member

rhatdan commented Jun 2, 2019

Yes I like the Moby Solution better, and we need to back port that ASAP as soon as it is merged.

@mheon mheon mentioned this pull request Sep 9, 2019
Merged
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@giuseppe giuseppe giuseppe left review comments

@vrothberg vrothberg vrothberg left review comments

@umohnani8 umohnani8 Awaiting requested review from umohnani8

Assignees

@rhatdan rhatdan

@baude baude

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@mheon @vrothberg @QiWang19 @baude @openshift-ci-robot @rhatdan @sfowl @ret2libc @giuseppe @openshift-merge-robot