harden config.Path() to disallow directory traversal

Signed-off-by: Nick Adcock <nick.adcock@docker.com>

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

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")}
+	pluginDir, err := config.Path("cli-plugins")
-	expected = append(expected, defaultSystemPluginDirs...)
+	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)
 }

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 {
+func Path(p ...string) (string, error) {
-	return filepath.Join(append([]string{Dir()}, p...)...)
+	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

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")
+	for _, tc := range []struct {
-	f1 := Path("a", "b")
+		name        string
-	assert.Equal(t, f1, filepath.Join("dummy1", "a", "b"))
+		dir         string
-
+		path        []string
-	SetDir("dummy2")
+		expected    string
-	f2 := Path("c", "d")
+		expectedErr string
-	assert.Equal(t, f2, filepath.Join("dummy2", "c", "d"))
+	}{
+		{
+			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)
 }