hardening config.Path() to disallow directory traversal by zappy-shu · Pull Request #1720 · docker/cli

zappy-shu · 2019-03-06T16:48:30Z

- What I did
Resolving issue raised in comment, removing the ability to get paths from above the config directory's root path

- How I did it
Checks whether the resolved path is underneath the root config directory using a strings.HasPrefix on the cleaned paths. Returns an error if it's not.

- How to verify it
Added unit test for attempting to traverse outside of the config directory

- Description for the changelog

hardening config.Path() to disallow directory traversal

codecov-io · 2019-03-06T17:06:21Z

Codecov Report

Merging #1720 into master will decrease coverage by 0.01%.
The diff coverage is 50%.

@@ Coverage Diff @@## master #1720 +/- ##
==========================================
- Coverage 56.16% 56.14% -0.02% 
==========================================
Files 306 306 Lines 21016 21028 +12 ==========================================
+ Hits 11803 11807 +4 - Misses 8359 8366 +7 - Partials 854 855 +1

thaJeztah

Left some notes / suggestions

thaJeztah · 2019-03-06T18:22:57Z

-	return filepath.Join(append([]string{Dir()}, p...)...)
+func Path(p ...string) (string, error) {
+	path := filepath.Join(append([]string{Dir()}, p...)...)
+	if !strings.HasPrefix(path, filepath.Clean(Dir())+string(filepath.Separator)) {


Wondering if the filepath.Clean() should actually be handled by SetDir() (i.e., the output of Dir() should be directly consumable)
Note that if we just want to check if it's outside of the base-dir, we could also simplify the check it self, and check if filepath.Join(p...) results in a path with .. as prefix; (go playground: https://play.golang.org/p/GtKy3dp_fqT)
package main import ( "fmt""path/filepath" ) funcmain() { p:= []string{"e", "..", "..", "f"} f:=filepath.Join(p...) fmt.Println(f) }

The problem with just filepath.Join(p...) is when p is an absolute path then the join result will start with a slash or drive: https://play.golang.org/p/Nm0ZY_C1cO5.
Sorry, I should have (and will) add a test for that.

Ah, good one; didn't think about that case 👍

thaJeztah · 2019-03-06T18:24:00Z


 	SetDir("dummy1")
-	f1 := Path("a", "b")
+	f1, err1 := Path("a", "b")


I see this was already there, but there's no need to create a new fX, errX for each test-case, you could re-use the same one.

thaJeztah · 2019-03-06T18:32:49Z


 	SetDir("dummy2")
-	f2 := Path("c", "d")
+	f2, err2 := Path("c", "d")


Looking at this test-case; I think this is exactly the same as the previous one (only different letters), so perhaps we should just remove this one;
funcTestConfigPath(t*testing.T) { f, err:=Path("a", "b") assert.Equal(t, f, filepath.Join(Dir(), "a", "b")) assert.NilError(t, err) f, err=Path("e", "..", "..", "f") assert.Equal(t, f, "") assert.ErrorContains(t, err, "is outside of root config directory '"+Dir()+"'") }
We could make this a simple subtest, but that may be a bit over-the-top if we won't be adding more test-cases;
funcTestConfigPath(t*testing.T) { for_, tc:=range []struct { namestringpath []stringexpectedstringexpectedErrstring }{ { name: "valid", path: []string{"a", "b"}, expected: filepath.Join(Dir(), "a", "b"), }, { name: "invalid", path: []string{"e", "..", "..", "f"}, expectedErr: "is outside of root config directory '"+Dir() +"'", }, } { t.Run(tc.name, func(t*testing.T) { f, err:=Path(tc.path...) assert.Equal(t, f, tc.expected) iftc.expectedErr=="" { assert.NilError(t, err) } else { assert.ErrorContains(t, err, tc.expectedErr) } }) } }

Good point. Thanks for the example. I've altered the tests to match this and added a few extra cases for testing absolute paths

thaJeztah · 2019-03-06T18:44:52Z

@@ -553,12 +553,19 @@ func TestConfigPath(t *testing.T) {
 	oldDir := Dir()

 	SetDir("dummy1")


Actually, looking at this, and wondering if we should do this as part of this test (perhaps it was done to increase coverage at the time, but I don't think we need to change the path here).

I've added tests that will also check functionality when the Dir() is set to an absolute path so I've kept this part

ijc

LGTM, left a nit and a question but still fine.

ijc · 2019-03-07T11:04:04Z

 	expected = append(expected, defaultSystemPluginDirs...)

-	assert.Equal(t, strings.Join(expected, ":"), strings.Join(getPluginDirs(cli), ":"))
+	pluginDirs1, err1 := getPluginDirs(cli)


nit: you can (and should) reuse pluginDir and err here and the same for the 2s below.

Yes, agreed; I see this PR still needs some squashing, so if you could make that change as part of that 👍

ijc · 2019-03-07T11:05:19Z

+ },
+ {
+ name: "invalid_absolute_path_windows",
+ path: []string{"C:/", "..", ".."},


Does this test DTRT when GOOS=linux (or !=windows generally)?

TIL DTRT - had to look that one up 😅

If we're testing for these situations, we also might need a test for
C:\ (backslash)
Embedded travering (for better name); []string{"a", "../../..", "b"} (although we might enter the area of testing Go stdlib itself)

The reason I ask is that path/filepath is (by definition) platform specific, so on Unix-y platforms it doesn't treat e.g .C:/ as special (I expect it thinks it is a directory in the root called C:).

Yeah, on second look this won't work as the build is on unix. Probably easiest to simply remove the "windows" tests for now

thaJeztah

Thanks! Left some comments inline, also:

There's three commits in this PR. Please squash some of those commits before merging (at least the second one is a touch-up commit).

The "clean config dir on set and add config path tests for root paths and" commit contains the change to add filepath.Clean() to SetDir(). It's a small change, so I'd be ok with squashing it with the other changes, but if you do want to keep it separate, it would be better to make that the first commit, with only that change;

func SetDir(dir string) {
- configDir = dir+ configDir = filepath.Clean(dir)
}

Also, small nit: try to make the commit message's first line fit within 50..72 characters so that it doesn't wrap (requires some creativity at times 😅)

thaJeztah · 2019-03-07T11:25:46Z

+ },
+ {
+ name: "invalid_absolute_path_windows",
+ path: []string{"C:/", "..", ".."},


TIL DTRT - had to look that one up 😅

thaJeztah · 2019-03-07T11:35:09Z

-	expected := []string{config.Path("cli-plugins")}
+	pluginDir, err := config.Path("cli-plugins")
+	assert.NilError(t, err)
+	expected := []string{pluginDir}


nit: could be combined to a single line
Suggested change
expected:= []string{pluginDir}
expected:=append([]string{pluginDir}, defaultSystemPluginDirs...)

thaJeztah · 2019-03-07T11:35:49Z

 	expected = append(expected, defaultSystemPluginDirs...)

-	assert.Equal(t, strings.Join(expected, ":"), strings.Join(getPluginDirs(cli), ":"))
+	pluginDirs1, err1 := getPluginDirs(cli)


Yes, agreed; I see this PR still needs some squashing, so if you could make that change as part of that 👍

thaJeztah · 2019-03-07T11:45:37Z

+ },
+ {
+ name: "invalid_absolute_path_windows",
+ path: []string{"C:/", "..", ".."},


If we're testing for these situations, we also might need a test for
C:\ (backslash)
Embedded travering (for better name); []string{"a", "../../..", "b"} (although we might enter the area of testing Go stdlib itself)

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

thaJeztah

LGTM

silvin-lubecki

LGTM 👍

GordonTheTurtle added the status/0-triage label Mar 6, 2019

thaJeztah requested changes Mar 6, 2019
View reviewed changes

thaJeztah added status/2-code-review area/plugins and removed status/0-triage labels Mar 7, 2019

ijc approved these changes Mar 7, 2019
View reviewed changes

thaJeztah requested changes Mar 7, 2019
View reviewed changes

zappy-shu force-pushed the harden-config-path branch 3 times, most recently from 43f091c to ddb1183Compare March 7, 2019 14:29

harden config.Path() to disallow directory traversal
ff51b0d
Signed-off-by: Nick Adcock <nick.adcock@docker.com>

zappy-shu force-pushed the harden-config-path branch from ddb1183 to ff51b0dCompare March 7, 2019 14:41

thaJeztah approved these changes Mar 7, 2019
View reviewed changes

silvin-lubecki approved these changes Mar 10, 2019
View reviewed changes

silvin-lubecki merged commit fdb0ef7 into docker:masterMar 10, 2019

GordonTheTurtle added this to the 19.03.0 milestone Mar 10, 2019

		@@ -553,12 +553,19 @@ func TestConfigPath(t *testing.T) {
		oldDir := Dir()

		SetDir("dummy1")

	expected:= []string{pluginDir}
	expected:=append([]string{pluginDir}, defaultSystemPluginDirs...)

Uh oh!

Conversation

zappy-shu commented Mar 6, 2019

Uh oh!

codecov-io commented Mar 6, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Mar 6, 2019•
edited
Loading