Do not glob value of symlink on cp #3834

jwhonce · 2019-08-16T17:35:51Z

Fixes #3829
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1741709

Signed-off-by: Jhon Honce [email protected]

TomSweeneyRedHat · 2019-08-16T18:24:56Z

cmd/podman/cp.go

+	fi, err := os.Lstat(srcPath)
+	switch errors.Cause(err).(type) {
+	case *os.PathError:
+		glob = append(glob, srcPath)


Maybe my headache today is getting the better of me, but why do you append the glob if lstat returns a PathError (which I think is it's main error?)? I'd thought you'd only continue on for nil, and hit default for the others?

@TomSweeneyRedHat I wanted the code after the switch to throw the expected errors etc. Trying to keep the patch as small as possible.

OK, I'm a bit more confused. What I thought should happen is line 218 and 219 should disappear. I thought if we couldn't resolve the file or symlink, it should die. That would make the patch even smaller.... Back to dig up some more tylenol.

I would remove the glob functionality. The functionality does not seem to be mentioned in the Podman documentation and it seems the functionality is not present in Docker (please correct me if I'm wrong).

If the functionality needs to be kept, to avoid surprises podman should only do globbing on exactly the path that the user provided on the command line. As soon as symlinks have been followed the path could look quite different. I think at this point in the code the variable srcPath could have some other value than what the user provided on the command line.

@eriksjolund I made the mistake of assuming '*' was expected as it was in the code. :)

For the container vibrant_brattain:

$ sudo docker cp vibrant_brattain:/etc/* /tmp Error: No such container:path: vibrant_brattain:/etc/*

So this is a feature we've introduced. I'll back it out and test.

@mheon Sound good?

mheon · 2019-08-16T20:04:23Z

The * character isn't strictly a glob - it's a wildcard to indicate the entire directory should be copied, AFAIK. It is only allowed to exist after a / and at the very end of the path. I think we can detect and special case that and avoid glob entirely.

…

On Fri, Aug 16, 2019, 14:50 Erik Sjölund ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In cmd/podman/cp.go <#3834 (comment)>: > @@ -210,10 +210,26 @@ func copyBetweenHostAndContainer(runtime *libpod.Runtime, src string, dest strin srcPath = cleanedPath } } - glob, err = filepath.Glob(srcPath) - if err != nil { - return errors.Wrapf(err, "invalid glob %q", srcPath) + + // Don't glob srcPath if it's the value of a symlink + // Fix for https://bugzilla.redhat.com/show_bug.cgi?id=1741709 + fi, err := os.Lstat(srcPath) + switch errors.Cause(err).(type) { + case *os.PathError: + glob = append(glob, srcPath) I would remove the glob functionality. The functionality does not seem to be mentioned in the Podman documentation and it seems the functionality is not present in Docker (please correct me if I'm wrong). If the functionality needs to be kept, to avoid surprises podman should only do globbing on exactly the path that the user provided on the command line. As soon as symlinks have been followed the path could look quite different. I think at this point in the code the variable srcPath could have some other value than what the user provided on the command line. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#3834>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB3AOCCUQUUPWJD4T6F2U4DQE3ZH5ANCNFSM4IMLMFIA> .

jwhonce · 2019-08-16T22:41:19Z

@mheon I'm working on another version of this patch that walks the directory if given path/*

Latest code matches docker cli behavior

mtrmac

I can’t see how this fixes the primary cause of the original problem:

Lines 200/206 have resolved the symlink in srcPath already, and we are now checking whether the target of the symlink ends with /*.
The copy function called on each of the entries in files will continue to resolve symlinks (in the host context).

So I would expect (without testing it, to be honest) that this does not change the behavior of the reproducers at all.

Still, getting rid of the Glob should make the actual fix much easier to implement, so this could be nice in that sense.

cmd/podman/cp.go

mtrmac · 2019-08-17T13:09:28Z

The * character isn't strictly a glob - it's a wildcard to indicate the entire directory should be copied, AFAIK. It is only allowed to exist after a / and at the very end of the path. I think we can detect and special case that and avoid glob entirely.

…

@mheon I'm working on another version of this patch that walks the directory if given path/*

Latest code matches docker cli behavior

Where’s the corresponding code/documentation? I can’t find any code handling /* specially, neither in the CLI client nor in the server; everything in there pretty soon does some variant of .Stat on the input. https://docs.docker.com/engine/reference/commandline/cp/ does not document any /* handling either.

mtrmac · 2019-08-17T13:11:13Z

… now I have finally read all comments and all of that is already known. I’m sorry.

edsantiago · 2019-08-19T13:16:06Z

The glob issue looks fixed but the cp is still happening in host-space:

$ podman run --name foo alpine sh -c 'ln -s /etc/machine-id file1'
$ podman cp foo:/\* .
$ ls
machine-id

edsantiago · 2019-08-19T15:01:45Z

Hey @jwhonce feel free to use this for testing. The second test ("will not recognize symlink pointing into host space" is currently failing, consistent with my comment above. Once that's fixed, it might still fail because I don't know exactly what error message to expect. All other tests pass right now and should continue passing. Feel free to git-add this and include it in your PR.

065-cp.bats.txt

rhatdan · 2019-08-19T15:49:49Z

@ed so you would expect the above command to create file1 as a symlink?

edsantiago · 2019-08-19T16:01:25Z

I would expect this to fail because /etc/machine-id does not exist on alpine. What happens instead is that the host's /etc/machine-id gets copied into the current directory. A perhaps-clearer example:

$ echo "This file exists only on the host" >/tmp/hostfile
$ podman run --name foo alpine sh -c 'ln -s /tmp/hostfile /tmp/link-in-container'
$ podman cp foo:/tmp/\* .
[ succeeds. Should fail.]
$ cat hostfile
This file exists only on the host

rhatdan · 2019-08-19T16:18:43Z

Yes I agree that is bad.

edsantiago · 2019-08-19T16:48:00Z

FWIW it's something in the glob expansion because directly specifying the file will (correctly) fail:

$ podman cp foo:/tmp/link-in-container .
Error: error evaluating symlinks "": lstat /home/esm/.local/share/containers/storage/overlay/90222cdf3b4b5b05b493d3e2a6f9ae986c3214d831b0937c270c657b1ff8049b/merged/tmp/hostfile: no such file or directory

...and, while I'm at it, so does specifying a completely-nonexistent file, but it fails with the same lstat error, which I find disconcerting:

$ podman cp foo:/sdfsdf .
Error: error evaluating symlinks "": lstat /home/esm/.local/share/containers/storage/overlay/90222cdf3b4b5b05b493d3e2a6f9ae986c3214d831b0937c270c657b1ff8049b/merged/sdfsdf: no such file or directory
[ I did not expect "error evaluating symlinks"; I was hoping for "foo:/sdfsdf: no such file or directory" ]

mheon · 2019-08-19T16:53:22Z

@edsantiago That should be fine - expected, I'd say. We unconditionally use a scoped symlink resolution function to find the target file in the container, even if no symlinks are present in the path. It's easier to maintain things that way.

edsantiago · 2019-08-19T16:55:24Z

@mheon thanks; it's just that, from an end-user perspective, that's a pretty difficult message to grok.

jwhonce · 2019-08-19T17:12:59Z

@edsantiago I can add the other "path" to the error message. I.e., the path from the user's POV vs. the containers. Just giving the input path makes debugging much harder because then the path in the error message is not what really failed on the file system.

rhatdan · 2019-08-19T17:32:04Z

What about refusing to copy symlinks, or in just copying the symlink, not the content.

edsantiago · 2019-08-19T17:58:20Z

I still think this is solvable with just a little figuring-out of glob expansion. I think that for the most part podman is doing the right thing, it's just that something is wonky with expanding '*'. I don't know how to fix it, but suspect that there's just too much indirection going on: I think * is somehow triggering nested processing before podman switches into container mount space. My reasoning:

$ echo "This file exists only on the host" >/tmp/hostfile
$ podman run --name foo alpine sh -c 'ln -s /tmp/hostfile /tmp/link-in-container'

$ podman cp foo:/sdfsdf .
Error: error evaluating symlinks "": lstat /home/esm/.local/share/containers/storage/overlay/90222cdf3b4b5b05b493d3e2a6f9ae986c3214d831b0937c270c657b1ff8049b/merged/sdfsdf: no such file or directory
! ^^^ GOOD! This shows that podman-cp, when given an explicit path, is correctly
!     checking in container-mountspace and realizing the symlink is invalid

$ podman cp foo:/tmp/\* .
! ^^^ this one not only succeeds, it creates the file >>hostfile<<  (not link-in-container)
!    ...implying that something has expanded the glob (* -> link-in-container)
!    **and then run readlink() on that result** (yielding "hostfile"). I believe this
!    is the crux of the incorrect behavior.

Hope this makes sense.

mheon · 2019-08-19T19:24:59Z

At present cp does not use the container mount ns. That needs to be fixed.

…

On Mon, Aug 19, 2019, 13:58 Ed Santiago ***@***.***> wrote: I still think this is solvable with just a little figuring-out of glob expansion. I think that for the most part podman is doing the right thing, it's just that something is wonky with expanding '*'. I don't know how to fix it, but suspect that there's just too much indirection going on: I think * is somehow triggering nested processing before podman switches into container mount space. My reasoning: $ echo "This file exists only on the host" >/tmp/hostfile $ podman run --name foo alpine sh -c 'ln -s /tmp/hostfile /tmp/link-in-container' $ podman cp foo:/sdfsdf . Error: error evaluating symlinks "": lstat /home/esm/.local/share/containers/storage/overlay/90222cdf3b4b5b05b493d3e2a6f9ae986c3214d831b0937c270c657b1ff8049b/merged/sdfsdf: no such file or directory ! ^^^ GOOD! This shows that podman-cp, when given an explicit path, is correctly ! checking in container-mountspace and realizing the symlink is invalid $ podman cp foo:/tmp/\* . ! ^^^ this one not only succeeds, it creates the file >>hostfile<< (not link-in-container) ! ...implying that something has expanded the glob (* -> link-in-container) ! **and then run readlink() on that result** (yielding "hostfile"). I believe this ! is the crux of the incorrect behavior. Hope this makes sense. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3834>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB3AOCCWMOKHL7H5FS3YQ7TQFLNMBANCNFSM4IMLMFIA> .

rhatdan · 2019-08-20T13:23:31Z

I think there is two situations here. 1 is if the container is running. then cp running in the same mount namespace as the container would be good.
The question would be what happens if you run podman cp when the container is not running. Should it mount up the containers mount namespace?

edsantiago · 2019-08-20T19:37:49Z

There's some more serious weirdness when cp'ing into the container: link resolution is taking a wrong turn somewhere, creating files and directories that IMHO it shouldn't.

$ podman run -d --name foo alpine sh -c 'ln -s nonesuch /tmp/\*;sleep 60'
<sha>
$ podman cp /etc/services foo:/tmp/\*                 <--- no trailing slash
$ podman exec foo ls -l /tmp
total 680
lrwxrwxrwx    1 root     root             8 Aug 20 19:25 * -> nonesuch
-rw-r--r--    1 root     root        692241 Apr  9 07:47 nonesuch

In the above, podman resolved the '*' symlink and created the file 'nonesuch'. This is weird; I was expecting podman to overwrite the '*' symlink and make it an actual file.

$ podman rm -f foo                      <--- previous container may still be running
$ !?run                                 <--- run the exact same container again
$ podman cp /etc/services foo:/tmp/\*/  <--- this time with trailing slash
$ podman exec foo ls -lR /tmp
/tmp:
total 4
lrwxrwxrwx    1 root     root             8 Aug 20 19:26 * -> nonesuch
drwxr-xr-x    2 root     root          4096 Aug 20 19:26 nonesuch

/tmp/nonesuch:
total 680
-rw-r--r--    1 root     root        692241 Apr  9 07:47 services

This one is weirder: podman cp read the '*symlink, resolved it asnonesuch`, treated it as a directory because of the trailing slash in the command-line cp, created the directory, and then created a file with the same name as the source file. This is information leakage from the host to the container, although I can't imagine any scenario in which it's serious.

I believe that the first case should transform '*' from a symlink into a file, and the second should fail with ENOTDIR.

eriksjolund · 2019-08-21T05:22:14Z

As I understand it Podman aims to be a drop-in replacement for Docker.
Therefore I think the wildcard functionality (if any) should be enabled by a
command-line flag. Otherwise the functionality will not match 100%.

Another thing: The handling of - looks strange.

filename := filepath.Base(destPath)
if filename == "-" && !isFromHostToCtr {

That would mean

somedir/-

is the same as

-

rhatdan · 2019-08-27T19:04:44Z

@jwhonce What is the state of this one? Is it ready for merge?

mtrmac · 2019-08-27T19:18:42Z

Still #3834 (review), and Ed’s testing confirms that.

mtrmac · 2019-08-27T19:19:25Z

cmd/podman/cp.go

-	if err != nil {
-		return err
+func copy(src, destPath, dest string, idMappingOpts storage.IDMappingOptions, chownOpts *idtools.IDPair, extract, isFromHostToCtr bool) (err error) {
+	// Don't symlink process /dev/stdin


This comment says “what”, but not “why”. I have no idea “why”; why does this matter, and/or hurt?

mtrmac · 2019-08-27T19:20:28Z

cmd/podman/cp.go

+func copy(src, destPath, dest string, idMappingOpts storage.IDMappingOptions, chownOpts *idtools.IDPair, extract, isFromHostToCtr bool) (err error) {
+	// Don't symlink process /dev/stdin
+	srcPath := os.Stdin.Name()
+	srcfi, _ := os.Stat(srcPath)


This is an extra system call on /dev/stdin we do even in cases when src is not /dev/stdin; it would be nice to avoid that. (If this code needs to exist at all.)

openshift-ci-robot · 2019-09-04T21:58:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jwhonce

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

openshift-ci-robot requested review from mheon and umohnani8 August 16, 2019 17:35

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S labels Aug 16, 2019

jwhonce requested review from QiWang19, edsantiago and mtrmac August 16, 2019 17:36

jwhonce self-assigned this Aug 16, 2019

jwhonce added the bug label Aug 16, 2019

TomSweeneyRedHat reviewed Aug 16, 2019

View reviewed changes

jwhonce force-pushed the issue/3829 branch from 5f7d2cb to 51ff025 Compare August 16, 2019 22:08

jwhonce force-pushed the issue/3829 branch from 51ff025 to 33405af Compare August 16, 2019 23:31

openshift-ci-robot added size/M and removed size/S labels Aug 16, 2019

mtrmac reviewed Aug 17, 2019

View reviewed changes

cmd/podman/cp.go Outdated Show resolved Hide resolved

jwhonce force-pushed the issue/3829 branch from 33405af to d87223d Compare August 19, 2019 01:23

jwhonce force-pushed the issue/3829 branch from d87223d to 405b37e Compare August 19, 2019 23:15

jwhonce changed the title ~~Do not glob value of symlink on cp~~ [WIP] Do not glob value of symlink on cp Aug 19, 2019

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 19, 2019

jwhonce force-pushed the issue/3829 branch from 405b37e to 3a2b3a5 Compare August 21, 2019 18:00

openshift-ci-robot added size/L and removed size/M labels Aug 21, 2019

jwhonce force-pushed the issue/3829 branch from 3a2b3a5 to 8e85574 Compare August 21, 2019 21:34

jwhonce changed the title ~~[WIP] Do not glob value of symlink on cp~~ Do not glob value of symlink on cp Aug 22, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 22, 2019

mheon added the Release Notes 1.5.2 label Aug 26, 2019

mtrmac reviewed Aug 27, 2019

View reviewed changes

jwhonce force-pushed the issue/3829 branch from 8e85574 to a847c93 Compare September 4, 2019 21:57

jwhonce closed this Sep 4, 2019

jwhonce force-pushed the issue/3829 branch from a847c93 to e74fcd7 Compare September 4, 2019 21:58

openshift-ci-robot added size/XS and removed size/L labels Sep 4, 2019

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 bot locked as resolved and limited conversation to collaborators Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not glob value of symlink on cp #3834

Do not glob value of symlink on cp #3834

jwhonce commented Aug 16, 2019

TomSweeneyRedHat Aug 16, 2019

jwhonce Aug 16, 2019

TomSweeneyRedHat Aug 16, 2019

eriksjolund Aug 16, 2019

jwhonce Aug 16, 2019

mheon commented Aug 16, 2019 via email

jwhonce commented Aug 16, 2019 •

edited

Loading

mtrmac left a comment

mtrmac commented Aug 17, 2019

mtrmac commented Aug 17, 2019

edsantiago commented Aug 19, 2019

edsantiago commented Aug 19, 2019

rhatdan commented Aug 19, 2019

edsantiago commented Aug 19, 2019

rhatdan commented Aug 19, 2019

edsantiago commented Aug 19, 2019

mheon commented Aug 19, 2019

edsantiago commented Aug 19, 2019

jwhonce commented Aug 19, 2019

rhatdan commented Aug 19, 2019

edsantiago commented Aug 19, 2019

mheon commented Aug 19, 2019 via email

rhatdan commented Aug 20, 2019

edsantiago commented Aug 20, 2019

eriksjolund commented Aug 21, 2019

rhatdan commented Aug 27, 2019

mtrmac commented Aug 27, 2019

mtrmac Aug 27, 2019

mtrmac Aug 27, 2019

openshift-ci-robot commented Sep 4, 2019

Do not glob value of symlink on cp #3834

Do not glob value of symlink on cp #3834

Conversation

jwhonce commented Aug 16, 2019

TomSweeneyRedHat Aug 16, 2019

Choose a reason for hiding this comment

jwhonce Aug 16, 2019

Choose a reason for hiding this comment

TomSweeneyRedHat Aug 16, 2019

Choose a reason for hiding this comment

eriksjolund Aug 16, 2019

Choose a reason for hiding this comment

jwhonce Aug 16, 2019

Choose a reason for hiding this comment

mheon commented Aug 16, 2019 via email

jwhonce commented Aug 16, 2019 • edited Loading

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac commented Aug 17, 2019

mtrmac commented Aug 17, 2019

edsantiago commented Aug 19, 2019

edsantiago commented Aug 19, 2019

rhatdan commented Aug 19, 2019

edsantiago commented Aug 19, 2019

rhatdan commented Aug 19, 2019

edsantiago commented Aug 19, 2019

mheon commented Aug 19, 2019

edsantiago commented Aug 19, 2019

jwhonce commented Aug 19, 2019

rhatdan commented Aug 19, 2019

edsantiago commented Aug 19, 2019

mheon commented Aug 19, 2019 via email

rhatdan commented Aug 20, 2019

edsantiago commented Aug 20, 2019

eriksjolund commented Aug 21, 2019

rhatdan commented Aug 27, 2019

mtrmac commented Aug 27, 2019

mtrmac Aug 27, 2019

Choose a reason for hiding this comment

mtrmac Aug 27, 2019

Choose a reason for hiding this comment

openshift-ci-robot commented Sep 4, 2019

jwhonce commented Aug 16, 2019 •

edited

Loading