diff --git a/daemon/archive_unix.go b/daemon/archive_unix.go index 08074a3fc642a..ab8cc83e3e52c 100644 --- a/daemon/archive_unix.go +++ b/daemon/archive_unix.go @@ -15,6 +15,7 @@ import ( "github.com/docker/docker/pkg/ioutils" volumemounts "github.com/docker/docker/volume/mounts" "github.com/moby/go-archive" + "github.com/moby/go-archive/compression" "github.com/pkg/errors" ) @@ -101,6 +102,15 @@ func (daemon *Daemon) containerExtractToDir(container *container.Container, path container.Lock() defer container.Unlock() + // Decompress the archive before switching into the container's + // filesystem to avoid executing decompression binaries (xz, unpigz) + // that may have been placed inside the container image. + decompressed, err := compression.DecompressStream(content) + if err != nil { + return err + } + defer decompressed.Close() + cfs, err := daemon.openContainerFS(container) if err != nil { return err @@ -143,7 +153,7 @@ func (daemon *Daemon) containerExtractToDir(container *container.Container, path } } - return archive.Untar(content, absPath, options) + return archive.UntarUncompressed(decompressed, absPath, options) }) if err != nil { return err diff --git a/integration/container/copy_test.go b/integration/container/copy_test.go index b2acd9bec7e89..fd3dde48b5ca2 100644 --- a/integration/container/copy_test.go +++ b/integration/container/copy_test.go @@ -19,6 +19,9 @@ import ( "github.com/docker/docker/pkg/jsonmessage" "github.com/docker/docker/testutil/fakecontext" "github.com/moby/go-archive" + buildtypes "github.com/moby/moby/api/types/build" + "github.com/moby/moby/v2/integration/internal/build" + "github.com/moby/moby/v2/internal/testutil/daemon" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" "gotest.tools/v3/skip" @@ -218,7 +221,7 @@ func makeTestImage(ctx context.Context, t *testing.T) (imageID string) { defer resp.Body.Close() err = jsonmessage.DisplayJSONMessagesStream(resp.Body, io.Discard, 0, false, func(msg jsonmessage.JSONMessage) { - var r build.Result + var r buildtypes.Result assert.NilError(t, json.Unmarshal(*msg.Aux, &r)) imageID = r.ID }) @@ -293,7 +296,7 @@ func TestCopyFromContainer(t *testing.T) { var imageID string err = jsonmessage.DisplayJSONMessagesStream(resp.Body, io.Discard, 0, false, func(msg jsonmessage.JSONMessage) { - var r build.Result + var r buildtypes.Result assert.NilError(t, json.Unmarshal(*msg.Aux, &r)) imageID = r.ID }) @@ -363,3 +366,76 @@ func TestCopyFromContainer(t *testing.T) { }) } } + +// TestCopyToContainerXZBinaryNotExecutedOnDaemon tests that when +// uploading an xz-compressed archive to a container via the +// PUT /containers/{id}/archive API, the daemon does NOT execute the xz +// binary found inside the container's filesystem. +// This is a regression test for +// https://github.com/moby/moby/security/advisories/GHSA-x86f-5xw2-fm2r +func TestCopyToContainerXZBinaryNotExecutedOnDaemon(t *testing.T) { + skip.If(t, testEnv.DaemonInfo.OSType == "windows") + skip.If(t, testEnv.IsRemoteDaemon, "cannot start daemon on remote test run") + ctx := setupTest(t) + + const tokenEnv = "MOBY_EXPLOIT_TEST_TOKEN" + const tokenVal = "host-boundary-crossed" + + d := daemon.New(t) + defer d.Cleanup(t) + d.SetEnvVar(tokenEnv, tokenVal) + d.StartWithBusybox(ctx, t, "--iptables=false", "--ip6tables=false") + defer d.Stop(t) + + apiClient := d.NewClientT(t) + + dir := t.TempDir() + buildCtx := fakecontext.New(t, dir, + // The fake xz writes the daemon's secret env var to a marker file. + // A process inside the container would not have this env var. + fakecontext.WithFile("fake-xz", "#!/bin/sh\necho $"+tokenEnv+" > /xz-was-executed\n"), + fakecontext.WithDockerfile(`FROM busybox +COPY fake-xz /usr/bin/xz +RUN chmod +x /usr/bin/xz`), + ) + defer buildCtx.Close() + + imageID := build.Do(ctx, t, apiClient, buildCtx, client.ImageBuildOptions{}) + + cID := container.Run(ctx, t, apiClient, container.WithImage(imageID)) + defer container.Remove(ctx, t, apiClient, cID, client.ContainerRemoveOptions{Force: true}) + + // Craft a payload that starts with xz magic bytes so the daemon's + // DecompressStream identifies it as xz-compressed. The payload is + // deliberately invalid xz data; we only care whether the daemon + // attempts to run the container's /usr/bin/xz to decompress it. + xzMagic := []byte{0xFD, 0x37, 0x7A, 0x58, 0x5A, 0x00} + payload := append(xzMagic, []byte("not-real-xz-data")...) + + // CopyToContainer is expected to fail because the payload is not a + // valid xz archive. We don't care about the error; we care about + // whether the container's xz binary was invoked. + _, _ = apiClient.CopyToContainer(ctx, cID, client.CopyToContainerOptions{ + DestinationPath: "/tmp", + Content: bytes.NewReader(payload), + }) + + t.Run("binary not executed", func(t *testing.T) { + // If the container's /usr/bin/xz was executed, the marker file + // will exist, and this assertion will fail. + res, err := container.Exec(ctx, apiClient, cID, []string{"test", "-f", "/xz-was-executed"}) + assert.NilError(t, err) + assert.Check(t, is.Equal(res.ExitCode, 1), + "container's xz binary was executed by the daemon during archive extraction; marker file /xz-was-executed was created") + }) + + t.Run("runs container", func(t *testing.T) { + // If the binary ran, check that it ran in the daemon's process + // context by looking for the daemon's secret env var in the + // marker file. A container process would not have this env var. + res, err := container.Exec(ctx, apiClient, cID, []string{"cat", "/xz-was-executed"}) + assert.NilError(t, err) + assert.Check(t, !strings.Contains(res.Stdout(), tokenVal), + "container's xz binary was executed in the daemon's process context: marker file contains the daemon's secret env var") + }) +}