From 7632776b35ceeed1a86be76b5d9d4e9e4997598a Mon Sep 17 00:00:00 2001 From: Philipp Schmied Date: Thu, 7 Feb 2019 09:17:35 +0100 Subject: [PATCH] Prevent overwriting irregular files (cp, save, export commands) Signed-off-by: Philipp Schmied --- cli/command/container/cp.go | 9 +++++++ cli/command/container/cp_test.go | 9 +++++++ cli/command/container/export.go | 4 ++++ cli/command/container/export_test.go | 16 +++++++++++++ cli/command/image/save.go | 14 +---------- cli/command/image/save_test.go | 7 +++++- cli/command/utils.go | 35 ++++++++++++++++++++++++++++ 7 files changed, 80 insertions(+), 14 deletions(-) diff --git a/cli/command/container/cp.go b/cli/command/container/cp.go index ffb9a211c2..20809bc362 100644 --- a/cli/command/container/cp.go +++ b/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 diff --git a/cli/command/container/cp_test.go b/cli/command/container/cp_test.go index 67cdaf15a9..f08b7ee520 100644 --- a/cli/command/container/cp_test.go +++ b/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) +} diff --git a/cli/command/container/export.go b/cli/command/container/export.go index f0f67373d7..ee77cdb55d 100644 --- a/cli/command/container/export.go +++ b/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) diff --git a/cli/command/container/export_test.go b/cli/command/container/export_test.go index b961ec7630..340d55e10e 100644 --- a/cli/command/container/export_test.go +++ b/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) +} diff --git a/cli/command/image/save.go b/cli/command/image/save.go index ef23ca1bb1..218a9a7974 100644 --- a/cli/command/image/save.go +++ b/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 -} diff --git a/cli/command/image/save_test.go b/cli/command/image/save_test.go index d051e8cb5c..cc960ba0e0 100644 --- a/cli/command/image/save_test.go +++ b/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 { diff --git a/cli/command/utils.go b/cli/command/utils.go index 13954c0101..2dcc8a2df2 100644 --- a/cli/command/utils.go +++ b/cli/command/utils.go @@ -11,6 +11,7 @@ import ( "github.com/docker/docker/api/types/filters" "github.com/docker/docker/pkg/system" + "github.com/pkg/errors" "github.com/spf13/pflag" ) @@ -125,3 +126,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 +}