pkg/symlink: avoid following out of scope

Docker-DCO-1.1-Signed-off-by: Cristian Staretu <cristian.staretu@gmail.com> (github: unclejack)

symlink/fs.go

@@ -12,6 +12,12 @@ const maxLoopCounter = 100
 
 // FollowSymlink will follow an existing link and scope it to the root
 // path provided.
+// The role of this function is to return an absolute path in the root
+// or normalize to the root if the symlink leads to a path which is
+// outside of the root.
+// Errors encountered while attempting to follow the symlink in path
+// will be reported.
+// Normalizations to the root don't constitute errors.
 func FollowSymlinkInScope(link, root string) (string, error) {
 	root, err := filepath.Abs(root)
 	if err != nil {
@@ -60,7 +66,13 @@ func FollowSymlinkInScope(link, root string) (string, error) {
 				}
 				return "", err
 			}
-			if stat.Mode()&os.ModeSymlink == os.ModeSymlink {
+
+			// let's break if we're not dealing with a symlink
+			if stat.Mode()&os.ModeSymlink != os.ModeSymlink {
+				break
+			}
+
+			// process the symlink
 			dest, err := os.Readlink(prev)
 			if err != nil {
 				return "", err
@@ -71,14 +83,19 @@ func FollowSymlinkInScope(link, root string) (string, error) {
 			} else {
 				prev, _ = filepath.Abs(prev)
 
-					if prev = filepath.Join(filepath.Dir(prev), dest); len(prev) < len(root) {
+				dir := filepath.Dir(prev)
+				prev = filepath.Join(dir, dest)
+				if dir == root && !strings.HasPrefix(prev, root) {
+					prev = root
+				}
+				if len(prev) < len(root) || (len(prev) == len(root) && prev != root) {
 					prev = filepath.Join(root, filepath.Base(dest))
 				}
 			}
-			} else {
-				break
 		}
 	}
+	if prev == "/" {
+		prev = root
 	}
 	return prev, nil
 }

symlink/fs_test.go

@@ -46,6 +46,7 @@ func TestFollowSymLinkUnderLinkedDir(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	defer os.RemoveAll(dir)
 
 	os.Mkdir(filepath.Join(dir, "realdir"), 0700)
 	os.Symlink("realdir", filepath.Join(dir, "linkdir"))
@@ -97,6 +98,10 @@ func TestFollowSymLinkRelativeLink(t *testing.T) {
 }
 
 func TestFollowSymLinkRelativeLinkScope(t *testing.T) {
+	// avoid letting symlink f lead us out of the "testdata" scope
+	// we don't normalize because symlink f is in scope and there is no
+	// information leak
+	{
 		link := "testdata/fs/a/f"
 
 		rewrite, err := FollowSymlinkInScope(link, "testdata")
@@ -107,10 +112,31 @@ func TestFollowSymLinkRelativeLinkScope(t *testing.T) {
 		if expected := abs(t, "testdata/test"); expected != rewrite {
 			t.Fatalf("Expected %s got %s", expected, rewrite)
 		}
+	}
 
-	link = "testdata/fs/b/h"
+	// avoid letting symlink f lead us out of the "testdata/fs" scope
+	// we don't normalize because symlink f is in scope and there is no
+	// information leak
+	{
+		link := "testdata/fs/a/f"
 
-	rewrite, err = FollowSymlinkInScope(link, "testdata")
+		rewrite, err := FollowSymlinkInScope(link, "testdata/fs")
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if expected := abs(t, "testdata/fs/test"); expected != rewrite {
+			t.Fatalf("Expected %s got %s", expected, rewrite)
+		}
+	}
+
+	// avoid letting symlink g (pointed at by symlink h) take out of scope
+	// TODO: we should probably normalize to scope here because ../[....]/root
+	// is out of scope and we leak information
+	{
+		link := "testdata/fs/b/h"
+
+		rewrite, err := FollowSymlinkInScope(link, "testdata")
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -119,3 +145,104 @@ func TestFollowSymLinkRelativeLinkScope(t *testing.T) {
 			t.Fatalf("Expected %s got %s", expected, rewrite)
 		}
 	}
+
+	// avoid letting allowing symlink e lead us to ../b
+	// normalize to the "testdata/fs/a"
+	{
+		link := "testdata/fs/a/e"
+
+		rewrite, err := FollowSymlinkInScope(link, "testdata/fs/a")
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if expected := abs(t, "testdata/fs/a"); expected != rewrite {
+			t.Fatalf("Expected %s got %s", expected, rewrite)
+		}
+	}
+
+	// avoid letting symlink -> ../directory/file escape from scope
+	// normalize to "testdata/fs/j"
+	{
+		link := "testdata/fs/j/k"
+
+		rewrite, err := FollowSymlinkInScope(link, "testdata/fs/j")
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if expected := abs(t, "testdata/fs/j"); expected != rewrite {
+			t.Fatalf("Expected %s got %s", expected, rewrite)
+		}
+	}
+
+	// make sure we don't allow escaping to /
+	// normalize to dir
+	{
+		dir, err := ioutil.TempDir("", "docker-fs-test")
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer os.RemoveAll(dir)
+
+		linkFile := filepath.Join(dir, "foo")
+		os.Mkdir(filepath.Join(dir, ""), 0700)
+		os.Symlink("/", linkFile)
+
+		rewrite, err := FollowSymlinkInScope(linkFile, dir)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if rewrite != dir {
+			t.Fatalf("Expected %s got %s", dir, rewrite)
+		}
+	}
+
+	// make sure we don't allow escaping to /
+	// normalize to dir
+	{
+		dir, err := ioutil.TempDir("", "docker-fs-test")
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer os.RemoveAll(dir)
+
+		linkFile := filepath.Join(dir, "foo")
+		os.Mkdir(filepath.Join(dir, ""), 0700)
+		os.Symlink("/../../", linkFile)
+
+		rewrite, err := FollowSymlinkInScope(linkFile, dir)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if rewrite != dir {
+			t.Fatalf("Expected %s got %s", dir, rewrite)
+		}
+	}
+
+	// make sure we stay in scope without leaking information
+	// this also checks for escaping to /
+	// normalize to dir
+	{
+		dir, err := ioutil.TempDir("", "docker-fs-test")
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer os.RemoveAll(dir)
+
+		linkFile := filepath.Join(dir, "foo")
+		os.Mkdir(filepath.Join(dir, ""), 0700)
+		os.Symlink("../../", linkFile)
+
+		rewrite, err := FollowSymlinkInScope(linkFile, dir)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if rewrite != dir {
+			t.Fatalf("Expected %s got %s", dir, rewrite)
+		}
+	}
+}

symlink/testdata/fs/j/k

@@ -0,0 +1 @@
+../i/a