Follow symlink for --device argument. #20684

Merged
merged 1 commit into from Mar 1, 2016

Conversation

Projects
None yet
6 participants
@yongtang
Member

yongtang commented Feb 25, 2016

This pull request fixes issue #13840 where a --device argument fails when the listed device is actually a symlink to a device. It checks to see if the path is a symlink and, if it is, resolves the symlink and continue the operation with the resolved path.

The tests are done in this way. First a symlink is created to link to /dev/zero. Then this symlink is used to map devices in the container. Inside the container a buffer is created and the md5 is calculated:

dd if=/dev/symzero bs=4K count=8 | md5sum

The expected md5 should be bb7df04e1b0a2570657527a7e108ae23.

Fixes: #13840

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 25, 2016

Member

Should we have tests for this?

Member

thaJeztah commented Feb 25, 2016

Should we have tests for this?

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Feb 25, 2016

Member

@thaJeztah Sure let me update the pull request.

Member

yongtang commented Feb 25, 2016

@thaJeztah Sure let me update the pull request.

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Feb 26, 2016

Member

@thaJeztah Just added the tests. Please let me know if you see any other issues.

Member

yongtang commented Feb 26, 2016

@thaJeztah Just added the tests. Please let me know if you see any other issues.

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion Feb 29, 2016

Contributor

Code looks good. It is certainly lucky that it was decided that --device and --volume should be separate. Otherwise, this PR would lead an absolutely devilish security problem:

Any volume that got mounted twice by the same image could contain a symlink to an otherwise forbidden device, because the first time the container was launched the symlink would be created, and the second time the container was launched, the symlink would be followed.

It might be worth adding a note in the docs that you can no longer safely use --device in an untrusted directory which might contain malicious symlinks...

Contributor

timthelion commented Feb 29, 2016

Code looks good. It is certainly lucky that it was decided that --device and --volume should be separate. Otherwise, this PR would lead an absolutely devilish security problem:

Any volume that got mounted twice by the same image could contain a symlink to an otherwise forbidden device, because the first time the container was launched the symlink would be created, and the second time the container was launched, the symlink would be followed.

It might be worth adding a note in the docs that you can no longer safely use --device in an untrusted directory which might contain malicious symlinks...

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 29, 2016

Member

It's still in design review, so we could consider making this behavior optional, e.g. Add an extra option (--device /foobar:follow-symlinks).

Member

thaJeztah commented Feb 29, 2016

It's still in design review, so we could consider making this behavior optional, e.g. Add an extra option (--device /foobar:follow-symlinks).

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion Feb 29, 2016

Contributor

I don't think that there are many cases where the security flaw could happen, but the following one comes to mind, which I know exists in the wild:

$ docker run --volume=/dev/snd:/dev/snd --device=/dev/snd image-tag /path/to/executable

The reason why people do this is in order to get the /dev/snd/by-path/ to show up in the container. The exploit would be to have the container create a symlink in /dev/snd which would point to some otherwise forbidden device, say /dev/sda1. The next time the container was run (or any container which was passed the --device=/dev/snd argument, for that matter) that new container would have access to /dev/sda1. Even though the user who issued --volume=/dev/snd never had the intention of giving containers access to /dev/sda1...

Perhaps there really should be a :follow-symlinks option, because we know that this can be exploited in reality, even though it is really the fault of the people doing --volume=/dev/snd:/dev/snd rather than --volume=/dev/snd:/dev/snd:r. And not Docker's fault at all...

Contributor

timthelion commented Feb 29, 2016

I don't think that there are many cases where the security flaw could happen, but the following one comes to mind, which I know exists in the wild:

$ docker run --volume=/dev/snd:/dev/snd --device=/dev/snd image-tag /path/to/executable

The reason why people do this is in order to get the /dev/snd/by-path/ to show up in the container. The exploit would be to have the container create a symlink in /dev/snd which would point to some otherwise forbidden device, say /dev/sda1. The next time the container was run (or any container which was passed the --device=/dev/snd argument, for that matter) that new container would have access to /dev/sda1. Even though the user who issued --volume=/dev/snd never had the intention of giving containers access to /dev/sda1...

Perhaps there really should be a :follow-symlinks option, because we know that this can be exploited in reality, even though it is really the fault of the people doing --volume=/dev/snd:/dev/snd rather than --volume=/dev/snd:/dev/snd:r. And not Docker's fault at all...

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Feb 29, 2016

Contributor

LGTM

Contributor

calavera commented Feb 29, 2016

LGTM

+ c.Assert(err, checker.IsNil)
+
+ // Create a temporary file, then a symbolic link to the temporary file
+ tmpFile := filepath.Join(tmpDir, "zero")

This comment has been minimized.

@cpuguy83

cpuguy83 Feb 29, 2016

Contributor

Can you explain this?
tmpFile is the same as symZero and then we're writing to it. Can we skip the tmpFile parts and instead use symZero?

@cpuguy83

cpuguy83 Feb 29, 2016

Contributor

Can you explain this?
tmpFile is the same as symZero and then we're writing to it. Can we skip the tmpFile parts and instead use symZero?

This comment has been minimized.

@yongtang

yongtang Mar 1, 2016

Member

@cpuguy83:
Sorry my bad. That was a bug/typo.
There are two tests:
In the first test is to create a symlink (tmpDir/zero) pointing to /dev/zero, then inside the container do a md5 calculation. This test is to show symlink to a device works.
In the second test, I was meant to create a temporary file, write some data into this temporary file, then create a symlink (tmpDir/file) pointing to the temporary file created. This test was meant to show that symlink to a file (not a device) will not work. (The name of the tmpFile should be something else, not filepath.Join(tmpDir, "zero"))
I will update the pull request shortly.

@yongtang

yongtang Mar 1, 2016

Member

@cpuguy83:
Sorry my bad. That was a bug/typo.
There are two tests:
In the first test is to create a symlink (tmpDir/zero) pointing to /dev/zero, then inside the container do a md5 calculation. This test is to show symlink to a device works.
In the second test, I was meant to create a temporary file, write some data into this temporary file, then create a symlink (tmpDir/file) pointing to the temporary file created. This test was meant to show that symlink to a file (not a device) will not work. (The name of the tmpFile should be something else, not filepath.Join(tmpDir, "zero"))
I will update the pull request shortly.

Follow symlink for --device argument.
Fixes: #13840

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Mar 1, 2016

Member

@cpuguy83 Just updated the pull request. Let me know if there are any other issues.

Member

yongtang commented Mar 1, 2016

@cpuguy83 Just updated the pull request. Let me know if there are any other issues.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 1, 2016

Contributor

Thanks, LGTM.

Contributor

cpuguy83 commented Mar 1, 2016

Thanks, LGTM.

cpuguy83 added a commit that referenced this pull request Mar 1, 2016

Merge pull request #20684 from yongtang/13840-follow-symlink
Follow symlink for --device argument.

@cpuguy83 cpuguy83 merged commit d883002 into moby:master Mar 1, 2016

8 checks passed

docker/dco-signed All commits signed
Details
documentation success 1 tests run, 1 skipped, 0 failed.
Details
experimental Jenkins build Docker-PRs-experimental 15634 has succeeded
Details
gccgo Jenkins build Docker-PRs-gccgo 2532 has succeeded
Details
janky Jenkins build Docker-PRs 24422 has succeeded
Details
userns Jenkins build Docker-PRs-userns 6773 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 22538 has succeeded
Details
windowsTP4 Jenkins build Docker-PRs-WoW-TP4 1837 has succeeded
Details

@thaJeztah thaJeztah added this to the 1.11.0 milestone Mar 6, 2016

yongtang added a commit to yongtang/docker that referenced this pull request Apr 23, 2016

Relative symlinks don't work with --device argument
This fix tries to address the issue raised in #22271 where
relative symlinks don't work with --device argument.

Previously, the symlinks in --device was implemneted (#20684)
with `os.Readlink()` which does not resolve if the linked
target is a relative path. In this fix, `filepath.EvalSymlinks()`
has been used which will reolve correctly with relative
paths.

An additional test case has been added to the existing
`TestRunDeviceSymlink` to cover changes in this fix.

This fix is related to #13840 and #20684, #22271.
This fix fixes #22271.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang added a commit to yongtang/docker that referenced this pull request Apr 23, 2016

Relative symlinks don't work with --device argument
This fix tries to address the issue raised in #22271 where
relative symlinks don't work with --device argument.

Previously, the symlinks in --device was implemneted (#20684)
with `os.Readlink()` which does not resolve if the linked
target is a relative path. In this fix, `filepath.EvalSymlinks()`
has been used which will reolve correctly with relative
paths.

An additional test case has been added to the existing
`TestRunDeviceSymlink` to cover changes in this fix.

This fix is related to #13840 and #20684, #22271.
This fix fixes #22271.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang added a commit to yongtang/docker that referenced this pull request Apr 23, 2016

Relative symlinks don't work with --device argument
This fix tries to address the issue raised in #22271 where
relative symlinks don't work with --device argument.

Previously, the symlinks in --device was implemneted (#20684)
with `os.Readlink()` which does not resolve if the linked
target is a relative path. In this fix, `filepath.EvalSymlinks()`
has been used which will reolve correctly with relative
paths.

An additional test case has been added to the existing
`TestRunDeviceSymlink` to cover changes in this fix.

This fix is related to #13840 and #20684, #22271.
This fix fixes #22271.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang added a commit to yongtang/docker that referenced this pull request Apr 25, 2016

Relative symlinks don't work with --device argument
This fix tries to address the issue raised in #22271 where
relative symlinks don't work with --device argument.

Previously, the symlinks in --device was implemneted (#20684)
with `os.Readlink()` which does not resolve if the linked
target is a relative path. In this fix, `filepath.EvalSymlinks()`
has been used which will reolve correctly with relative
paths.

An additional test case has been added to the existing
`TestRunDeviceSymlink` to cover changes in this fix.

This fix is related to #13840 and #20684, #22271.
This fix fixes #22271.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang added a commit to yongtang/docker that referenced this pull request Apr 25, 2016

Relative symlinks don't work with --device argument
This fix tries to address the issue raised in #22271 where
relative symlinks don't work with --device argument.

Previously, the symlinks in --device was implemneted (#20684)
with `os.Readlink()` which does not resolve if the linked
target is a relative path. In this fix, `filepath.EvalSymlinks()`
has been used which will reolve correctly with relative
paths.

An additional test case has been added to the existing
`TestRunDeviceSymlink` to cover changes in this fix.

This fix is related to #13840 and #20684, #22271.
This fix fixes #22271.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang added a commit to yongtang/docker that referenced this pull request Apr 25, 2016

Relative symlinks don't work with --device argument
This fix tries to address the issue raised in #22271 where
relative symlinks don't work with --device argument.

Previously, the symlinks in --device was implemneted (#20684)
with `os.Readlink()` which does not resolve if the linked
target is a relative path. In this fix, `filepath.EvalSymlinks()`
has been used which will reolve correctly with relative
paths.

An additional test case has been added to the existing
`TestRunDeviceSymlink` to cover changes in this fix.

This fix is related to #13840 and #20684, #22271.
This fix fixes #22271.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

mlaventure added a commit to mlaventure/docker that referenced this pull request Apr 25, 2016

Relative symlinks don't work with --device argument
This fix tries to address the issue raised in #22271 where
relative symlinks don't work with --device argument.

Previously, the symlinks in --device was implemneted (#20684)
with `os.Readlink()` which does not resolve if the linked
target is a relative path. In this fix, `filepath.EvalSymlinks()`
has been used which will reolve correctly with relative
paths.

An additional test case has been added to the existing
`TestRunDeviceSymlink` to cover changes in this fix.

This fix is related to #13840 and #20684, #22271.
This fix fixes #22271.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
(cherry picked from commit 632b314)

mlaventure added a commit to mlaventure/docker that referenced this pull request Apr 25, 2016

Relative symlinks don't work with --device argument
This fix tries to address the issue raised in #22271 where
relative symlinks don't work with --device argument.

Previously, the symlinks in --device was implemneted (#20684)
with `os.Readlink()` which does not resolve if the linked
target is a relative path. In this fix, `filepath.EvalSymlinks()`
has been used which will reolve correctly with relative
paths.

An additional test case has been added to the existing
`TestRunDeviceSymlink` to cover changes in this fix.

This fix is related to #13840 and #20684, #22271.
This fix fixes #22271.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
(cherry picked from commit 632b314)

aanm added a commit to aanm/docker that referenced this pull request Apr 27, 2016

Relative symlinks don't work with --device argument
This fix tries to address the issue raised in #22271 where
relative symlinks don't work with --device argument.

Previously, the symlinks in --device was implemneted (#20684)
with `os.Readlink()` which does not resolve if the linked
target is a relative path. In this fix, `filepath.EvalSymlinks()`
has been used which will reolve correctly with relative
paths.

An additional test case has been added to the existing
`TestRunDeviceSymlink` to cover changes in this fix.

This fix is related to #13840 and #20684, #22271.
This fix fixes #22271.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017

Relative symlinks don't work with --device argument
This fix tries to address the issue raised in #22271 where
relative symlinks don't work with --device argument.

Previously, the symlinks in --device was implemneted (#20684)
with `os.Readlink()` which does not resolve if the linked
target is a relative path. In this fix, `filepath.EvalSymlinks()`
has been used which will reolve correctly with relative
paths.

An additional test case has been added to the existing
`TestRunDeviceSymlink` to cover changes in this fix.

backport from: moby#22272

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yuanhong Peng <pengyuanhong@huawei.com>

liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017

Merge branch 'fix-relative-symlinks' into 'huawei-1.11.2'
Relative symlinks don't work with --device argument

This fix tries to address the issue raised in #22271 where
relative symlinks don't work with --device argument.

Previously, the symlinks in --device was implemneted (#20684)
with `os.Readlink()` which does not resolve if the linked
target is a relative path. In this fix, `filepath.EvalSymlinks()`
has been used which will reolve correctly with relative
paths.

An additional test case has been added to the existing
`TestRunDeviceSymlink` to cover changes in this fix.

backport from: moby#22272

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yuanhong Peng <pengyuanhong@huawei.com>


See merge request docker/docker!348
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment