The security fix in GHSA-vp62-88p7-qqf5 switched openContainerFS to
os.Root for mount-destination operations, but stopped walking the
destination through in-container symlinks.
os.Root refuses to follow absolute symlinks, so any container whose
image had an absolute symlink along the mount target's path (e.g. the
common /var/run -> /run in ubuntu/alpine/busybox) broke `docker cp`.
Walk m.Destination through ctr.GetResourcePath first which follows
symlinks to get a path relative to BaseFS, then keep using os.Root for
the actual MkdirAll/OpenFile/Open calls.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
| ... | ... |
@@ -100,10 +100,16 @@ func (daemon *Daemon) openContainerFS(ctr *container.Container) (_ *containerFSV |
| 100 | 100 |
|
| 101 | 101 |
// TODO(vvoland): Refactor this after security release. |
| 102 | 102 |
for _, m := range mounts {
|
| 103 |
- // Destination is an absolute path within container |
|
| 104 |
- // filesystem. For the os.Root to work, we need to convert it |
|
| 105 |
- // to a path relative to root fs / |
|
| 106 |
- relDest, err := filepath.Rel("/", m.Destination)
|
|
| 103 |
+ // Walk m.Destination through the container's symlinks before |
|
| 104 |
+ // passing it to os.Root, which refuses absolute symlinks |
|
| 105 |
+ // (e.g. the common /var/run -> /run). The resolution itself |
|
| 106 |
+ // is lexical; subsequent os.Root operations still enforce |
|
| 107 |
+ // the GHSA-vp62-88p7-qqf5 / GHSA-rg2x-37c3-w2rh protections. |
|
| 108 |
+ resolved, err := ctr.GetResourcePath(m.Destination) |
|
| 109 |
+ if err != nil {
|
|
| 110 |
+ return fmt.Errorf("resolve mount destination %q: %w", m.Destination, err)
|
|
| 111 |
+ } |
|
| 112 |
+ relDest, err := filepath.Rel(ctr.BaseFS, resolved) |
|
| 107 | 113 |
if err != nil {
|
| 108 | 114 |
return fmt.Errorf("make destination relative: %w", err)
|
| 109 | 115 |
} |
| 110 | 116 |
new file mode 100644 |
| ... | ... |
@@ -0,0 +1,66 @@ |
| 0 |
+package container |
|
| 1 |
+ |
|
| 2 |
+import ( |
|
| 3 |
+ "bytes" |
|
| 4 |
+ "os" |
|
| 5 |
+ "path/filepath" |
|
| 6 |
+ "testing" |
|
| 7 |
+ |
|
| 8 |
+ mounttypes "github.com/moby/moby/api/types/mount" |
|
| 9 |
+ "github.com/moby/moby/client" |
|
| 10 |
+ "github.com/moby/moby/v2/integration/internal/build" |
|
| 11 |
+ "github.com/moby/moby/v2/integration/internal/container" |
|
| 12 |
+ "github.com/moby/moby/v2/internal/testutil" |
|
| 13 |
+ "github.com/moby/moby/v2/internal/testutil/fakecontext" |
|
| 14 |
+ "gotest.tools/v3/assert" |
|
| 15 |
+ "gotest.tools/v3/skip" |
|
| 16 |
+) |
|
| 17 |
+ |
|
| 18 |
+// TestCopyWithAbsoluteSymlinkedMountTarget was introduced as a regression test |
|
| 19 |
+// for https://github.com/moby/moby/issues/52653. |
|
| 20 |
+// |
|
| 21 |
+// The security fix in GHSA-vp62-88p7-qqf5 switched openContainerFS to use |
|
| 22 |
+// os.Root for the mount-destination operations. |
|
| 23 |
+// os.Root refuses to follow absolute symlinks, but distro images commonly ship |
|
| 24 |
+// /var/run as an absolute symlink to /run. |
|
| 25 |
+// As a result, any container with a bind mount whose target traversed such a |
|
| 26 |
+// symlink (e.g. -v /host/sock:/var/run/docker.sock) made `docker cp` fail. |
|
| 27 |
+func TestCopyWithAbsoluteSymlinkedMountTarget(t *testing.T) {
|
|
| 28 |
+ skip.If(t, testEnv.DaemonInfo.OSType != "linux") |
|
| 29 |
+ ctx := setupTest(t) |
|
| 30 |
+ apiClient := testEnv.APIClient() |
|
| 31 |
+ |
|
| 32 |
+ // Build an image with an absolute in-container symlink along the mount |
|
| 33 |
+ // target path. |
|
| 34 |
+ // Stock distro images expose this shape via /var/run -> /run, but we set |
|
| 35 |
+ // up our own /sockets -> /root pair so the test does not depend on any |
|
| 36 |
+ // particular base image's layout. |
|
| 37 |
+ buildCtx := fakecontext.New(t, "", |
|
| 38 |
+ fakecontext.WithDockerfile(`FROM busybox |
|
| 39 |
+RUN touch /root/nil && ln -s /root /sockets |
|
| 40 |
+`), |
|
| 41 |
+ ) |
|
| 42 |
+ defer buildCtx.Close() |
|
| 43 |
+ imgID := build.Do(ctx, t, apiClient, buildCtx, client.ImageBuildOptions{})
|
|
| 44 |
+ |
|
| 45 |
+ // Use testutil.TempDir so the rootless daemon can access the bind-mount |
|
| 46 |
+ // source: t.TempDir() creates a 0700 parent that the fake-root user |
|
| 47 |
+ // cannot stat. |
|
| 48 |
+ srcDir := testutil.TempDir(t) |
|
| 49 |
+ assert.NilError(t, os.WriteFile(filepath.Join(srcDir, "sock"), nil, 0o644)) |
|
| 50 |
+ |
|
| 51 |
+ cid := container.Create(ctx, t, apiClient, |
|
| 52 |
+ container.WithImage(imgID), |
|
| 53 |
+ container.WithMount(mounttypes.Mount{
|
|
| 54 |
+ Type: mounttypes.TypeBind, |
|
| 55 |
+ Source: filepath.Join(srcDir, "sock"), |
|
| 56 |
+ Target: "/sockets/docker.sock", |
|
| 57 |
+ }), |
|
| 58 |
+ ) |
|
| 59 |
+ |
|
| 60 |
+ _, err := apiClient.CopyToContainer(ctx, cid, client.CopyToContainerOptions{
|
|
| 61 |
+ DestinationPath: "/sockets/", |
|
| 62 |
+ Content: bytes.NewReader(nil), |
|
| 63 |
+ }) |
|
| 64 |
+ assert.NilError(t, err) |
|
| 65 |
+} |