Merge pull request #1515 from sw-pschmied/1514-prevent-replacing-irregular-files

Prevent overwriting irregular files (cp, save, export commands)

cli/command/container/cp.go

@@ -128,6 +128,10 @@ func copyFromContainer(ctx context.Context, dockerCli command.Cli, copyConfig cp
 		}
 	}
 
+	if err := command.ValidateOutputPath(dstPath); err != nil {
+		return err
+	}
+
 	client := dockerCli.Client()
 	// if client requests to follow symbol link, then must decide target file to be copied
 	var rebaseName string
@@ -209,6 +213,11 @@ func copyToContainer(ctx context.Context, dockerCli command.Cli, copyConfig cpCo
 		dstStat, err = client.ContainerStatPath(ctx, copyConfig.container, linkTarget)
 	}
 
+	// Validate the destination path
+	if err := command.ValidateOutputPathFileMode(dstStat.Mode); err != nil {
+		return errors.Wrapf(err, `destination "%s:%s" must be a directory or a regular file`, copyConfig.container, dstPath)
+	}
+
 	// Ignore any error and assume that the parent directory of the destination
 	// path exists, in which case the copy may still succeed. If there is any
 	// type of conflict (e.g., non-directory overwriting an existing directory

cli/command/container/cp_test.go

@@ -190,3 +190,12 @@ func TestSplitCpArg(t *testing.T) {
 		})
 	}
 }
+
+func TestRunCopyFromContainerToFilesystemIrregularDestination(t *testing.T) {
+	options := copyOptions{source: "container:/dev/null", destination: "/dev/random"}
+	cli := test.NewFakeCli(nil)
+	err := runCopy(cli, options)
+	assert.Assert(t, err != nil)
+	expected := `"/dev/random" must be a directory or a regular file`
+	assert.ErrorContains(t, err, expected)
+}

cli/command/container/export.go

@@ -41,6 +41,10 @@ func runExport(dockerCli command.Cli, opts exportOptions) error {
 		return errors.New("cowardly refusing to save to a terminal. Use the -o flag or redirect")
 	}
 
+	if err := command.ValidateOutputPath(opts.output); err != nil {
+		return errors.Wrap(err, "failed to export container")
+	}
+
 	clnt := dockerCli.Client()
 
 	responseBody, err := clnt.ContainerExport(context.Background(), opts.container)

cli/command/container/export_test.go

@@ -31,3 +31,19 @@ func TestContainerExportOutputToFile(t *testing.T) {
 
 	assert.Assert(t, fs.Equal(dir.Path(), expected))
 }
+
+func TestContainerExportOutputToIrregularFile(t *testing.T) {
+	cli := test.NewFakeCli(&fakeClient{
+		containerExportFunc: func(container string) (io.ReadCloser, error) {
+			return ioutil.NopCloser(strings.NewReader("foo")), nil
+		},
+	})
+	cmd := NewExportCommand(cli)
+	cmd.SetOutput(ioutil.Discard)
+	cmd.SetArgs([]string{"-o", "/dev/random", "container"})
+
+	err := cmd.Execute()
+	assert.Assert(t, err != nil)
+	expected := `"/dev/random" must be a directory or a regular file`
+	assert.ErrorContains(t, err, expected)
+}

cli/command/image/save.go

@@ -3,8 +3,6 @@ package image
 import (
 	"context"
 	"io"
-	"os"
-	"path/filepath"
 
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
@@ -44,7 +42,7 @@ func RunSave(dockerCli command.Cli, opts saveOptions) error {
 		return errors.New("cowardly refusing to save to a terminal. Use the -o flag or redirect")
 	}
 
-	if err := validateOutputPath(opts.output); err != nil {
+	if err := command.ValidateOutputPath(opts.output); err != nil {
 		return errors.Wrap(err, "failed to save image")
 	}
 
@@ -61,13 +59,3 @@ func RunSave(dockerCli command.Cli, opts saveOptions) error {
 
 	return command.CopyToFile(opts.output, responseBody)
 }
-
-func validateOutputPath(path string) error {
-	dir := filepath.Dir(path)
-	if dir != "" && dir != "." {
-		if _, err := os.Stat(dir); os.IsNotExist(err) {
-			return errors.Errorf("unable to validate output path: directory %q does not exist", dir)
-		}
-	}
-	return nil
-}

cli/command/image/save_test.go

@@ -44,7 +44,12 @@ func TestNewSaveCommandErrors(t *testing.T) {
 		{
 			name:          "output directory does not exist",
 			args:          []string{"-o", "fakedir/out.tar", "arg1"},
-			expectedError: "failed to save image: unable to validate output path: directory \"fakedir\" does not exist",
+			expectedError: "failed to save image: invalid output path: directory \"fakedir\" does not exist",
+		},
+		{
+			name:          "output file is irregular",
+			args:          []string{"-o", "/dev/null", "arg1"},
+			expectedError: "failed to save image: invalid output path: \"/dev/null\" must be a directory or a regular file",
 		},
 	}
 	for _, tc := range testCases {

cli/command/utils.go

@@ -12,6 +12,7 @@ import (
 	"github.com/docker/cli/cli/streams"
 	"github.com/docker/docker/api/types/filters"
 	"github.com/docker/docker/pkg/system"
+	"github.com/pkg/errors"
 	"github.com/spf13/pflag"
 )
 
@@ -126,3 +127,37 @@ func AddPlatformFlag(flags *pflag.FlagSet, target *string) {
 	flags.SetAnnotation("platform", "version", []string{"1.32"})
 	flags.SetAnnotation("platform", "experimental", nil)
 }
+
+// ValidateOutputPath validates the output paths of the `export` and `save` commands.
+func ValidateOutputPath(path string) error {
+	dir := filepath.Dir(path)
+	if dir != "" && dir != "." {
+		if _, err := os.Stat(dir); os.IsNotExist(err) {
+			return errors.Errorf("invalid output path: directory %q does not exist", dir)
+		}
+	}
+	// check whether `path` points to a regular file
+	// (if the path exists and doesn't point to a directory)
+	if fileInfo, err := os.Stat(path); !os.IsNotExist(err) {
+		if fileInfo.Mode().IsDir() || fileInfo.Mode().IsRegular() {
+			return nil
+		}
+
+		if err := ValidateOutputPathFileMode(fileInfo.Mode()); err != nil {
+			return errors.Wrapf(err, fmt.Sprintf("invalid output path: %q must be a directory or a regular file", path))
+		}
+	}
+	return nil
+}
+
+// ValidateOutputPathFileMode validates the output paths of the `cp` command and serves as a
+// helper to `ValidateOutputPath`
+func ValidateOutputPathFileMode(fileMode os.FileMode) error {
+	switch {
+	case fileMode&os.ModeDevice != 0:
+		return errors.New("got a device")
+	case fileMode&os.ModeIrregular != 0:
+		return errors.New("got an irregular file")
+	}
+	return nil
+}