Fix copying of symlinks in containers by jlhawn · Pull Request #14885 · moby/moby

jlhawn · 2015-07-23T00:54:38Z

Copying a symlink should do just that: copy the symlink NOT copy the target of the symlink. Also, the resulting file from the copy should have the name of the symlink NOT the name of the target file.

This patch includes a fix to StatPath, ArchivePath, and ExtractToDir methods on containers. These operations should resolve symlinks that are in the path but if the resource itself is a symlink then it should not be resolved. This patch puts this logic into a common function resolvePath which resolves symlinks of the path's dir in scope of the container rootfs but does not resolve the final element of the path. Now archive, extract, and stat operations will return symlinks if the path is indeed a symlink.

Notably, destination paths should always be fully resolved, i.e., copying to a symlink should affect the symlink target, not the symlink itself.

jlhawn · 2015-07-23T00:55:03Z

ping @tonistiigi @thaJeztah

jlhawn · 2015-07-23T00:58:32Z

see discussion in #13171 (comment)

icecrime · 2015-07-23T01:08:17Z

Can you please let us know if this is strictly required for 1.8.0, and if we're only fixing usability issues or potential security holes?

jlhawn · 2015-07-23T02:22:49Z

@icecrime So far it's just a usability/behavior issue. We haven't yet determined if there's a real security issue with this.

thaJeztah · 2015-07-23T07:56:18Z

Because none of us is a security expert, I'd prefer to have the security team to have a look,
both at this change and the issue it should resolve (reported here #13171 (comment) and discussed on IRC, starting here: https://botbot.me/freenode/docker-dev/2015-07-22/?msg=45272608&page=6).
Better safe than sorry, now that we are still able to check before the 1.8 release.

ping @ewindisch @diogomonica @NathanMcCauley

(just my 0.02, apologies if I crossed my boundaries here)

tonistiigi · 2015-07-23T12:04:59Z

This seems to only address the Stat-header issue and not rebasing. If I run docker cp id:/foo/ bar where foo is a symlink I don't get local folder bar but a folder that has the name of the symlink target.

This breaks symlink targets without slash. cp -a seems to use same behavior for symlinks-to-dirs as regular directories. I think for local paths this was already different before but for remotes it changed with this PR.

Any comment on the other points in #13171 (comment) ?

tonistiigi · 2015-07-23T12:05:31Z

daemon/archive.go

I think we should do this clean step always. There's a breakout with path /.. for example.

ah, yeah. I'll remove this condition then.

icecrime · 2015-07-23T16:18:32Z

Ok! Adding this to the milestone, we don't have to block 1.8.0-rc1 on it though.

jlhawn · 2015-07-23T17:18:08Z

@tonistiigi

This seems to only address the Stat-header issue and not rebasing. If I run docker cp id:/foo/ bar where foo is a symlink I don't get local folder bar but a folder that has the name of the symlink target.

I'll add a test case for this and make sure it's fixed.

jlhawn · 2015-07-24T21:06:28Z

pkg/archive/archive.go

Rationale for removing this: It is never used, we have logic that uses it below, but I replaced it with the rebaseNames map.

jlhawn · 2015-07-24T22:44:32Z

~~A couple of windows test failures as expected... wish I could run those locally :-(.~~

thaJeztah · 2015-07-24T22:51:49Z

@jlhawn I'm sure @ahmetalpbalkan is able to give you access to an azure machine 😄

jlhawn · 2015-07-24T22:57:47Z

@thaJeztah eh, no biggie... I have a windows 8.1 VM locally that I could use, I just haven't bothered setting up the docker dev environment on it. The test failure was also pretty minor: I was comparing a local windows backslash-delimited path with a constant unix-style forward-slash-delimited path.

jlhawn · 2015-07-25T00:04:22Z

@tonistiigi This should now fix the behavior you saw on the source being a symlink to a directory.

As for your other question from #13171 (comment):

How can we protect this from running containers updating a filesystem/volumes when the request is taking place? I mean we check for breakouts in the beginning of GET/PUT but if a container is using the filesystem it could just flip a symlink in the right time and then we would have full read/write to host.

I think the only thing we could do is require that the container be stopped or paused? (can that be considered outside the scope of this PR?)

tonistiigi · 2015-07-28T15:40:52Z

Is docker cp with symlinks supposed to follow the semantics of cp -a? Docs are bit contradictory saying it's cp -a but also that symlink is always/only followed if it's followed by a slash.

#setup (GNU coreutils 8.21):
mkdir foo
touch foo/bar
ln -s foo baz
mkdir abc
touch abc/def

cp -a abc baz # creates foo/abc/def
docker cp t0:abc baz # error: cannot copy directory
docker cp abc t0:baz # error: cannot copy directory

cp -a abc/def baz # creates foo/def
docker cp t0:abc/def baz # creates baz
docker cp abc/def t0:baz # creates baz

cp -a baz/. abc # creates abc/bar
docker cp t0:baz/. abc # creates abc/bar
docker cp baz/. t0:abc # creates abc/(baz -> foo) + WARN

The resolvePath() logic itself seems safe but I'm bit concerned about the readability of the actual place where the breakout protection happens. There is bunch of logic in TarResource for testing symlink/ paths but if I understand it correctly this is only used in client side and would be unsafe otherwise. Maybe we could add a comment in the daemon code that calls TarResource that the path sent there can be symlink or dir/ but should never be symlink/ or symlink/. and it's the calling code's responsibility to check that.

Having two separate code paths that do tar rebasing probably also isn't ideal maintenance-wise and I can't help to point out that by not having the first-level directory we could have probably avoided the need to do rebasing in both ends(plus the need to validate downloaded tar that we should probably also do).

tonistiigi · 2015-07-28T15:42:37Z

daemon/archive.go

This does not seem right. If the path is /myvolume/symlinktoroot/ it would be matched to the volume and writing would go to the readonly rootfs.

jlhawn · 2015-07-28T17:35:58Z

Is docker cp with symlinks supposed to follow the semantics of cp -a? Docs are bit contradictory saying it's cp -a but also that symlink is always/only followed if it's followed by a slash.

This should only apply to the source argument. From your example (thanks!) it looks like we've also erroneously applied it to the destination argument 😦. I'll fold this into this PR also (and add a test case for it based on your example).

jlhawn · 2015-07-29T00:05:34Z

Okay, all updated again. Currently the ~~Windows~~ CI is on fire, so I guess we'll have to wait on that.

tonistiigi · 2015-07-29T17:46:58Z

daemon/archive.go

What's the definition of Name? If its always basename of Path then why return as a separate field at all?

Yes, it's the basename, while Path is the absolute path.

I'll go ahead ad remove the Path field. Your right, it's unnecessary given that the user has to provide a path in the first place.

Well, Path is cleaned and made absolute so it may actually have some value. Name, on the other hand, anyone can easily get from Path (if the definition remains as it is currently).

tonistiigi · 2015-07-29T21:33:49Z

LGTM

icecrime · 2015-07-29T23:56:53Z

LGTM

calavera · 2015-07-30T17:16:13Z

integration-cli/docker_cli_cp_test.go

ioutil.WriteFile?

calavera · 2015-07-30T19:08:47Z

LGTM, but can we squash all those commits? all of the are related to the same thing and it would be easier to keep track of the for the release.

jlhawn · 2015-07-30T19:10:00Z

@calavera okay, I'll squash them together

[pkg/archive] Update archive/copy path handling - Remove unused TarOptions.Name field. - Add new TarOptions.RebaseNames field. - Update some of the logic around path dir/base splitting. - Update some of the logic behind archive entry name rebasing. [api/types] Add LinkTarget field to PathStat [daemon] Fix stat, archive, extract of symlinks These operations *should* resolve symlinks that are in the path but if the resource itself is a symlink then it *should not* be resolved. This patch puts this logic into a common function `resolvePath` which resolves symlinks of the path's dir in scope of the container rootfs but does not resolve the final element of the path. Now archive, extract, and stat operations will return symlinks if the path is indeed a symlink. [api/client] Update cp path hanling [docs/reference/api] Update description of stat Add the linkTarget field to the header of the archive endpoint. Remove path field. [integration-cli] Fix/Add cp symlink test cases Copying a symlink should do just that: copy the symlink NOT copy the target of the symlink. Also, the resulting file from the copy should have the name of the symlink NOT the name of the target file. Copying to a symlink should copy to the symlink target and not modify the symlink itself. Docker-DCO-1.1-Signed-off-by: Josh Hawn <[email protected]> (github: jlhawn)

calavera · 2015-07-30T20:42:27Z

💚 Merging!

Fix copying of symlinks in containers

jlhawn · 2015-07-30T20:43:42Z

🎉

calavera · 2015-07-30T20:56:52Z

cherry picked into #15091

thaJeztah · 2015-07-30T21:09:26Z

Great work guys!

GordonTheTurtle added the status/0-triage label Jul 23, 2015

tonistiigi reviewed Jul 23, 2015
View reviewed changes

icecrime added this to the 1.8.0 milestone Jul 23, 2015

icecrime added status/2-code-review and removed status/0-triage labels Jul 23, 2015

jlhawn force-pushed the fix_cp_symlink branch from 5332b72 to 472d5de Compare July 24, 2015 21:02

jlhawn reviewed Jul 24, 2015
View reviewed changes

jlhawn force-pushed the fix_cp_symlink branch from 472d5de to 33f3329 Compare July 24, 2015 21:15

jlhawn force-pushed the fix_cp_symlink branch from 33f3329 to 82a0d97 Compare July 24, 2015 22:54

jlhawn force-pushed the fix_cp_symlink branch 2 times, most recently from 465df6a to afb7388 Compare July 25, 2015 00:01

jlhawn force-pushed the fix_cp_symlink branch 4 times, most recently from 5c268ea to ec8e53d Compare July 27, 2015 22:33

tonistiigi reviewed Jul 28, 2015
View reviewed changes

jlhawn force-pushed the fix_cp_symlink branch 3 times, most recently from 266dadd to 6c25a12 Compare July 29, 2015 00:04

jlhawn force-pushed the fix_cp_symlink branch 2 times, most recently from 69a564e to 9417a29 Compare July 29, 2015 02:11

tonistiigi reviewed Jul 29, 2015
View reviewed changes

jlhawn force-pushed the fix_cp_symlink branch from 9417a29 to 3451350 Compare July 29, 2015 20:18

jlhawn force-pushed the fix_cp_symlink branch from 3451350 to 0b62820 Compare July 30, 2015 17:03

calavera reviewed Jul 30, 2015
View reviewed changes

jlhawn force-pushed the fix_cp_symlink branch 2 times, most recently from f199312 to 05478d9 Compare July 30, 2015 18:06

jlhawn force-pushed the fix_cp_symlink branch from 05478d9 to 75f6929 Compare July 30, 2015 19:14

calavera added a commit that referenced this pull request Jul 30, 2015

Merge pull request #14885 from jlhawn/fix_cp_symlink

030f61d

Fix copying of symlinks in containers

calavera merged commit 030f61d into moby:master Jul 30, 2015

jlhawn deleted the fix_cp_symlink branch July 31, 2015 18:02

aespinosa mentioned this pull request Oct 9, 2015

revert following symlinks in docker cp #16896

Closed

thaJeztah mentioned this pull request Jan 21, 2024

integration-cli: TestPutContainerArchiveErrSymlinkInVolumeToReadOnlyRootfs: use current API #47151

Merged

Comments

Conversation

jlhawn commented Jul 23, 2015

Uh oh!

jlhawn commented Jul 23, 2015

Uh oh!

jlhawn commented Jul 23, 2015

Uh oh!

icecrime commented Jul 23, 2015

Uh oh!

jlhawn commented Jul 23, 2015

Uh oh!

thaJeztah commented Jul 23, 2015

Uh oh!

tonistiigi commented Jul 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icecrime commented Jul 23, 2015

Uh oh!

jlhawn commented Jul 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlhawn commented Jul 24, 2015

Uh oh!

thaJeztah commented Jul 24, 2015

Uh oh!

jlhawn commented Jul 24, 2015

Uh oh!

jlhawn commented Jul 25, 2015

Uh oh!

tonistiigi commented Jul 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlhawn commented Jul 28, 2015

Uh oh!

jlhawn commented Jul 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Jul 29, 2015

Uh oh!

icecrime commented Jul 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calavera commented Jul 30, 2015

Uh oh!

jlhawn commented Jul 30, 2015

Uh oh!

calavera commented Jul 30, 2015

Uh oh!

jlhawn commented Jul 30, 2015

Uh oh!

calavera commented Jul 30, 2015

Uh oh!

thaJeztah commented Jul 30, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants