diff --git a/daemon/containerfs_linux.go b/daemon/containerfs_linux.go index 62e77c80a6..5a7697d77e 100644 --- a/daemon/containerfs_linux.go +++ b/daemon/containerfs_linux.go @@ -143,7 +143,10 @@ func (daemon *Daemon) openContainerFS(ctr *container.Container) (_ *containerFSV // file descriptor pinning the resolved inode. Using // /proc/self/fd/ as the mount target prevents any // subsequent symlink swap from redirecting the mount. - targetFile, err := root.Open(relDest) + // O_PATH is required: the target is not necessarily a + // regular file or a directory, and opening a socket or a + // FIFO for I/O would fail or block. + targetFile, err := root.OpenFile(relDest, unix.O_PATH, 0) if err != nil { return fmt.Errorf("open mount target %q: %w", m.Destination, err) } @@ -314,7 +317,16 @@ func createIfNotExists(root *os.Root, unsafePath string, isDir bool) error { } } - f, err := root.OpenFile(unsafePath, os.O_CREATE|os.O_WRONLY, 0o755) + // O_EXCL leaves an existing destination alone whatever its type, and does + // so atomically. Checking for the destination and then opening it would + // let a container process substitute a FIFO in between, and opening a + // FIFO for writing blocks until a reader shows up -- wedging the thread + // while the container lock is held. Opening an existing socket would fail + // with ENXIO, and an existing directory with EISDIR. + f, err := root.OpenFile(unsafePath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o755) + if errors.Is(err, os.ErrExist) { + return nil + } if err != nil { return err } diff --git a/daemon/containerfs_linux_test.go b/daemon/containerfs_linux_test.go index 715c9bce0e..12dc98f703 100644 --- a/daemon/containerfs_linux_test.go +++ b/daemon/containerfs_linux_test.go @@ -1,10 +1,13 @@ package daemon import ( + "net" "os" "path/filepath" "testing" + "time" + "golang.org/x/sys/unix" "gotest.tools/v3/assert" ) @@ -42,4 +45,67 @@ func TestCreateIfNotExists(t *testing.T) { err = createIfNotExists(root, "file/to/create", false) assert.NilError(t, err, "Should not fail if already exists") }) + + // A mount destination which already exists is left alone whatever its + // type. Nested bind mounts make the destination of the inner mount + // resolve to the outer mount's contents, so it can be any kind of file. + t.Run("existing socket", func(t *testing.T) { + dir := t.TempDir() + l, err := net.Listen("unix", filepath.Join(dir, "sock")) + assert.NilError(t, err) + defer l.Close() + + root, err := os.OpenRoot(dir) + assert.NilError(t, err) + defer root.Close() + + err = createIfNotExists(root, "sock", false) + assert.NilError(t, err, "Should not fail if already exists as a socket") + }) + t.Run("existing fifo", func(t *testing.T) { + dir := t.TempDir() + assert.NilError(t, unix.Mkfifo(filepath.Join(dir, "fifo"), 0o600)) + + root, err := os.OpenRoot(dir) + assert.NilError(t, err) + defer root.Close() + + // Opening a FIFO blocks until the other end is opened, so guard + // against the test hanging for the whole -timeout if it regresses. + done := make(chan error, 1) + go func() { done <- createIfNotExists(root, "fifo", false) }() + select { + case err := <-done: + assert.NilError(t, err, "Should not fail if already exists as a FIFO") + case <-time.After(10 * time.Second): + t.Fatal("createIfNotExists blocked on an existing FIFO") + } + }) + t.Run("existing socket when a directory is expected", func(t *testing.T) { + dir := t.TempDir() + l, err := net.Listen("unix", filepath.Join(dir, "sock")) + assert.NilError(t, err) + defer l.Close() + + root, err := os.OpenRoot(dir) + assert.NilError(t, err) + defer root.Close() + + // Nothing can be bind-mounted onto it -- the kernel rejects a + // directory source with ENOTDIR -- so it is reported rather than + // left for mount(2) to fail on. + err = createIfNotExists(root, "sock", true) + assert.ErrorIs(t, err, os.ErrExist) + }) + t.Run("existing directory when a file is expected", func(t *testing.T) { + dir := t.TempDir() + assert.NilError(t, os.Mkdir(filepath.Join(dir, "dir"), 0o755)) + + root, err := os.OpenRoot(dir) + assert.NilError(t, err) + defer root.Close() + + err = createIfNotExists(root, "dir", false) + assert.NilError(t, err, "Should not fail if already exists as a directory") + }) } diff --git a/integration/container/copy_linux_test.go b/integration/container/copy_linux_test.go index d3d43944a9..463a6f92aa 100644 --- a/integration/container/copy_linux_test.go +++ b/integration/container/copy_linux_test.go @@ -2,6 +2,8 @@ package container import ( "bytes" + "io" + "net" "os" "path/filepath" "testing" @@ -64,3 +66,52 @@ RUN touch /root/nil && ln -s /root /sockets }) assert.NilError(t, err) } + +// TestCopyWithNestedBindMountedSocket is a regression test for a container +// with two overlapping bind mounts: one covering a directory, and one whose +// destination is a path inside that directory. +// +// openContainerFS replays the container's bind mounts parent-first, so by the +// time the inner mount is set up its destination resolves to the contents of +// the outer mount, and may be any kind of file. The security fix in +// GHSA-vp62-88p7-qqf5 started unconditionally opening that destination -- to +// create it, and again to pin its inode -- which fails with ENXIO when it is a +// socket, breaking docker cp in both directions for the whole container. +func TestCopyWithNestedBindMountedSocket(t *testing.T) { + skip.If(t, testEnv.DaemonInfo.OSType != "linux") + ctx := setupTest(t) + apiClient := testEnv.APIClient() + + // Use testutil.TempDir so the rootless daemon can access the bind-mount + // source: t.TempDir() creates a 0700 parent that the fake-root user + // cannot stat. + srcDir := testutil.TempDir(t) + l, err := net.Listen("unix", filepath.Join(srcDir, "sock")) + assert.NilError(t, err) + defer l.Close() + + cid := container.Create(ctx, t, apiClient, + container.WithMount(mounttypes.Mount{ + Type: mounttypes.TypeBind, + Source: srcDir, + Target: "/mountpoint", + }), + container.WithMount(mounttypes.Mount{ + Type: mounttypes.TypeBind, + Source: filepath.Join(srcDir, "sock"), + Target: "/mountpoint/sock", + }), + ) + + res, err := apiClient.CopyFromContainer(ctx, cid, client.CopyFromContainerOptions{SourcePath: "/mountpoint"}) + assert.NilError(t, err) + defer res.Content.Close() + _, err = io.Copy(io.Discard, res.Content) + assert.NilError(t, err) + + _, err = apiClient.CopyToContainer(ctx, cid, client.CopyToContainerOptions{ + DestinationPath: "/mountpoint/", + Content: bytes.NewReader(nil), + }) + assert.NilError(t, err) +}