From ff51b0d77d3ef2a66ae2bed6ed8a6e20c41fb1f1 Mon Sep 17 00:00:00 2001 From: Nick Adcock Date: Thu, 7 Mar 2019 14:28:42 +0000 Subject: [PATCH] harden config.Path() to disallow directory traversal Signed-off-by: Nick Adcock --- cli-plugins/manager/manager.go | 25 +++++++++++--- cli-plugins/manager/manager_test.go | 14 +++++--- cli/config/config.go | 11 +++++-- cli/config/config_test.go | 51 +++++++++++++++++++++++++---- 4 files changed, 82 insertions(+), 19 deletions(-) diff --git a/cli-plugins/manager/manager.go b/cli-plugins/manager/manager.go index 21d8ed0f19..78ff64bb43 100644 --- a/cli-plugins/manager/manager.go +++ b/cli-plugins/manager/manager.go @@ -35,15 +35,20 @@ func IsNotFound(err error) bool { return ok } -func getPluginDirs(dockerCli command.Cli) []string { +func getPluginDirs(dockerCli command.Cli) ([]string, error) { var pluginDirs []string if cfg := dockerCli.ConfigFile(); cfg != nil { pluginDirs = append(pluginDirs, cfg.CLIPluginsExtraDirs...) } - pluginDirs = append(pluginDirs, config.Path("cli-plugins")) + pluginDir, err := config.Path("cli-plugins") + if err != nil { + return nil, err + } + + pluginDirs = append(pluginDirs, pluginDir) pluginDirs = append(pluginDirs, defaultSystemPluginDirs...) - return pluginDirs + return pluginDirs, nil } func addPluginCandidatesFromDir(res map[string][]string, d string) error { @@ -96,7 +101,12 @@ func listPluginCandidates(dirs []string) (map[string][]string, error) { // ListPlugins produces a list of the plugins available on the system func ListPlugins(dockerCli command.Cli, rootcmd *cobra.Command) ([]Plugin, error) { - candidates, err := listPluginCandidates(getPluginDirs(dockerCli)) + pluginDirs, err := getPluginDirs(dockerCli) + if err != nil { + return nil, err + } + + candidates, err := listPluginCandidates(pluginDirs) if err != nil { return nil, err } @@ -132,7 +142,12 @@ func PluginRunCommand(dockerCli command.Cli, name string, rootcmd *cobra.Command return nil, errPluginNotFound(name) } exename := addExeSuffix(NamePrefix + name) - for _, d := range getPluginDirs(dockerCli) { + pluginDirs, err := getPluginDirs(dockerCli) + if err != nil { + return nil, err + } + + for _, d := range pluginDirs { path := filepath.Join(d, exename) // We stat here rather than letting the exec tell us diff --git a/cli-plugins/manager/manager_test.go b/cli-plugins/manager/manager_test.go index 14176e57ab..d727fdac8a 100644 --- a/cli-plugins/manager/manager_test.go +++ b/cli-plugins/manager/manager_test.go @@ -92,10 +92,14 @@ func TestErrPluginNotFound(t *testing.T) { func TestGetPluginDirs(t *testing.T) { cli := test.NewFakeCli(nil) - expected := []string{config.Path("cli-plugins")} - expected = append(expected, defaultSystemPluginDirs...) + pluginDir, err := config.Path("cli-plugins") + assert.NilError(t, err) + expected := append([]string{pluginDir}, defaultSystemPluginDirs...) - assert.Equal(t, strings.Join(expected, ":"), strings.Join(getPluginDirs(cli), ":")) + var pluginDirs []string + pluginDirs, err = getPluginDirs(cli) + assert.Equal(t, strings.Join(expected, ":"), strings.Join(pluginDirs, ":")) + assert.NilError(t, err) extras := []string{ "foo", "bar", "baz", @@ -104,5 +108,7 @@ func TestGetPluginDirs(t *testing.T) { cli.SetConfigFile(&configfile.ConfigFile{ CLIPluginsExtraDirs: extras, }) - assert.DeepEqual(t, expected, getPluginDirs(cli)) + pluginDirs, err = getPluginDirs(cli) + assert.DeepEqual(t, expected, pluginDirs) + assert.NilError(t, err) } diff --git a/cli/config/config.go b/cli/config/config.go index 2a6c5f0ea4..f5e33f2b3e 100644 --- a/cli/config/config.go +++ b/cli/config/config.go @@ -5,6 +5,7 @@ import ( "io" "os" "path/filepath" + "strings" "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/cli/config/credentials" @@ -43,12 +44,16 @@ func ContextStoreDir() string { // SetDir sets the directory the configuration file is stored in func SetDir(dir string) { - configDir = dir + configDir = filepath.Clean(dir) } // Path returns the path to a file relative to the config dir -func Path(p ...string) string { - return filepath.Join(append([]string{Dir()}, p...)...) +func Path(p ...string) (string, error) { + path := filepath.Join(append([]string{Dir()}, p...)...) + if !strings.HasPrefix(path, Dir()+string(filepath.Separator)) { + return "", errors.Errorf("path %q is outside of root config directory %q", path, Dir()) + } + return path, nil } // LegacyLoadFromReader is a convenience function that creates a ConfigFile object from diff --git a/cli/config/config_test.go b/cli/config/config_test.go index 85288291b4..f41c09d38e 100644 --- a/cli/config/config_test.go +++ b/cli/config/config_test.go @@ -2,6 +2,7 @@ package config import ( "bytes" + "fmt" "io" "io/ioutil" "os" @@ -552,13 +553,49 @@ func TestLoadDefaultConfigFile(t *testing.T) { func TestConfigPath(t *testing.T) { oldDir := Dir() - SetDir("dummy1") - f1 := Path("a", "b") - assert.Equal(t, f1, filepath.Join("dummy1", "a", "b")) - - SetDir("dummy2") - f2 := Path("c", "d") - assert.Equal(t, f2, filepath.Join("dummy2", "c", "d")) + for _, tc := range []struct { + name string + dir string + path []string + expected string + expectedErr string + }{ + { + name: "valid_path", + dir: "dummy", + path: []string{"a", "b"}, + expected: filepath.Join("dummy", "a", "b"), + }, + { + name: "valid_path_absolute_dir", + dir: "/dummy", + path: []string{"a", "b"}, + expected: filepath.Join("/dummy", "a", "b"), + }, + { + name: "invalid_relative_path", + dir: "dummy", + path: []string{"e", "..", "..", "f"}, + expectedErr: fmt.Sprintf("is outside of root config directory %q", "dummy"), + }, + { + name: "invalid_absolute_path", + dir: "dummy", + path: []string{"/a", "..", ".."}, + expectedErr: fmt.Sprintf("is outside of root config directory %q", "dummy"), + }, + } { + t.Run(tc.name, func(t *testing.T) { + SetDir(tc.dir) + f, err := Path(tc.path...) + assert.Equal(t, f, tc.expected) + if tc.expectedErr == "" { + assert.NilError(t, err) + } else { + assert.ErrorContains(t, err, tc.expectedErr) + } + }) + } SetDir(oldDir) }