Skip to content

volume: evaluate symlinks before relabeling mount source #34792

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

Merged
merged 1 commit into from
Sep 27, 2017

Conversation

runcom
Copy link
Member

@runcom runcom commented Sep 8, 2017

Simple reproducer:

$ mkdir /var/foo
$ touch /var/foo/test
$ ln -s /var/foo /var/bar
$ docker run -ti -v /var/bar:/var/bar:Z fedora sh
sh-4.3# ls -lZ /var/bar/
ls: cannot open directory '/var/bar/': Permission denied

@cpuguy83 PTAL

Signed-off-by: Antonio Murdaca runcom@redhat.com

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Copy link

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @runcom!

I have a couple of suggestions that might be useful or not.

  1. Requesting we update the doc of that function
  2. Perhaps we could collaborate on a test for this change. This is my first time diving into Docker's source code, so I could use your chaperoning. However, here is an idea for a test, based off the repro you provided
package test

import (
	"io/ioutil"
	"os"
	"path/filepath"
	"testing"
)

func TestSymlinkResolutionBeforeRelabeling(t *testing.T) {
	// Initial setup:
	// Our target repro on the commandline is:
	// 1. $ mkdir /var/foo
	// 2. $ touch /var/foo/test
	// 3. $ ln -s /var/foo /var/bar
	// 4. $ docker run -ti -v /var/bar:/var/bar:Z fedora sh

	// 1.
	scratchDir, err := ioutil.TempDir("", "foo")
	if err != nil {
		t.Fatalf("1. mkdir err: %v", err)
	}
	defer os.RemoveAll(scratchDir)

	// 2.
	f, err := os.Create(filepath.Join(scratchDir, "test"))
	if err != nil {
		t.Fatalf("2. creating test: %v", err)
	}
	f.Close()

	// 3.
	scratchParentDir := filepath.Base(scratchDir)
	symlinkPath := filepath.Join(scratchParentDir, "bar")
	if err := os.Symlink(scratchDir, symlinkPath); err != nil {
		t.Fatalf("3. symlink err: %v", err)
	}
	defer os.RemoveAll(symlinkPath)

	// 4.
	mPt, err := moby.ParseMountRaw(symlinkPath, "fedora")
	if err != nil {
		t.Fatalf("4. mount: %v", err)
	}
	mountPath := symlinkPath + ":Z"
	mountedPath, err := mPt.Setup(mountPath, "fedora", nil)
	if err != nil {
		t.Fatalf("4. expected a successful mount")
	}

	// Perhaps in here, add in the sh step
	if g, w := mountedPath, scratchDir; g != w {
		t.Fatalf("4. resolved paths don't match: got = %q want = %q", g, w)
	}
}

Please help me fill in the blanks with the respective code steps, but otherwise I think the general structure of it reflects a 1-to-1 structure in Shell-to-Go of the repro you provided.

volume/volume.go Outdated
@@ -156,7 +157,13 @@ func (m *MountPoint) Setup(mountLabel string, rootIDs idtools.IDPair, checkFun f
return
}

err = label.Relabel(m.Source, mountLabel, label.IsShared(m.Mode))
sourcePath, err := filepath.EvalSymlinks(m.Source)

Choose a reason for hiding this comment

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

Could we perhaps append a comment in the doc for (m *MountPoint) Setup
to describe the new behavior with something like this, maybe?

// If the MountPoint's source is a symlink, it is first 
// evaluated to an absolute path before being relabeled.

@runcom runcom added the kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. label Sep 10, 2017
@runcom
Copy link
Member Author

runcom commented Sep 10, 2017

Docker CI doesn't test with selinux, does it @cpuguy83?

@cpuguy83
Copy link
Member

Not this CI, no.

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

Seems ok.
LGTM

@cpuguy83
Copy link
Member

Needs a rebase, though.

@runcom
Copy link
Member Author

runcom commented Sep 18, 2017

rebased

@runcom runcom force-pushed the fix-relabel-symlinks branch from 5e39020 to 3a46162 Compare September 18, 2017 12:49
@thaJeztah
Copy link
Member

Janky failed; possibly due to:

13:02:55 volume/volume.go:162:4:warning: ineffectual assignment to err (ineffassign)
13:02:55 volume/volume.go:171:4:warning: ineffectual assignment to err (ineffassign)

(although it only prints "warning" if I see correctly)

@thaJeztah
Copy link
Member

I guess it's confused by the named return variables (perhaps // nolint: ineffassign) ? @dnephin thoughts?

@dnephin
Copy link
Member

dnephin commented Sep 18, 2017

I'm pretty sure the linter is correct, the code is wrong (https://play.golang.org/p/bKhOuWeynv). The assignment on line 159 is masking the named return value.

A unit test for MountPoint.Setup() would be good.

@thaJeztah
Copy link
Member

doh, yes! I looked at that, but didn't test it, you're right

@runcom ^^

Simple reproducer:

```sh
$ mkdir /var/foo
$ touch /var/foo/test
$ ln -s /var/foo /var/bar
$ docker run -ti -v /var/bar:/var/bar:Z fedora sh
sh-4.3# ls -lZ /var/bar/
ls: cannot open directory '/var/bar/': Permission denied
```

Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@runcom runcom force-pushed the fix-relabel-symlinks branch from 3a46162 to e0b22c0 Compare September 19, 2017 08:54
@runcom
Copy link
Member Author

runcom commented Sep 19, 2017

Should be fixed now

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

@dnephin PTAL (if you have thoughts on that test; I see @odeke-em proposed a test above #34792 (review))

@dnephin
Copy link
Member

dnephin commented Sep 19, 2017

I'm not sure what you mean by thoughts on the test. The test should call MountMount.Setup() in a way that exercises the new code. If that's not trivial then the code needs to be refactored so that it is easy to unit test.

@thaJeztah
Copy link
Member

Let me merge this one for now; if this needs a refactor, we can do a refactor + integration test in a follow-up. I opened #34998 for tracking this, and assigned it to @runcom

@thaJeztah thaJeztah merged commit f60e7aa into moby:master Sep 27, 2017
@runcom runcom deleted the fix-relabel-symlinks branch June 23, 2020 09:30
kolyshkin added a commit to kolyshkin/moby that referenced this pull request Nov 23, 2021
Commit 317e94b introduced a local variable err, which masked
the err used as a named return value. As a result, any relabeling error
(happened in that defer) is not returned, and the caller gets "", nil,
moving on to performing a bind mount with empty m.Source argument
(which, apparently, results in mounting a current directory).

This can be reproduced by trying to add a bind mount which has no space
left (see RHBZ link below for details).

This issue was spotted upstream during the review, and fixed before the
merge (see [1]). In this repository, though, it was never fixed.

Fixes: https://bugzilla.redhat.com/1878621

[1]  moby#34792 (comment)

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security/selinux kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants