pkg/symlink: avoid following out of scope

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

pkg/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,25 +66,36 @@ func FollowSymlinkInScope(link, root string) (string, error) {
 				}
 				return "", err
 			}
-			if stat.Mode()&os.ModeSymlink == os.ModeSymlink {
-				dest, err := os.Readlink(prev)
-				if err != nil {
-					return "", err
-				}
 
-				if path.IsAbs(dest) {
-					prev = filepath.Join(root, dest)
-				} else {
-					prev, _ = filepath.Abs(prev)
+			// let's break if we're not dealing with a symlink
+			if stat.Mode()&os.ModeSymlink != os.ModeSymlink {
+				break
+			}
 
-					if prev = filepath.Join(filepath.Dir(prev), dest); len(prev) < len(root) {
-						prev = filepath.Join(root, filepath.Base(dest))
-					}
-				}
+			// process the symlink
+			dest, err := os.Readlink(prev)
+			if err != nil {
+				return "", err
+			}
+
+			if path.IsAbs(dest) {
+				prev = filepath.Join(root, dest)
 			} else {
-				break
+				prev, _ = filepath.Abs(prev)
+
+				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))
+				}
 			}
 		}
 	}
+	if prev == "/" {
+		prev = root
+	}
 	return prev, nil
 }

pkg/symlink/fs_test.go

@@ -98,25 +98,151 @@ func TestFollowSymLinkRelativeLink(t *testing.T) {
 }
 
 func TestFollowSymLinkRelativeLinkScope(t *testing.T) {
-	link := "testdata/fs/a/f"
-
-	rewrite, err := FollowSymlinkInScope(link, "testdata")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	if expected := abs(t, "testdata/test"); expected != rewrite {
-		t.Fatalf("Expected %s got %s", expected, rewrite)
-	}
-
-	link = "testdata/fs/b/h"
-
-	rewrite, err = FollowSymlinkInScope(link, "testdata")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	if expected := abs(t, "testdata/root"); expected != rewrite {
-		t.Fatalf("Expected %s got %s", expected, rewrite)
+	// 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")
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if expected := abs(t, "testdata/test"); expected != rewrite {
+			t.Fatalf("Expected %s got %s", expected, rewrite)
+		}
+	}
+
+	// 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/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)
+		}
+
+		if expected := abs(t, "testdata/root"); expected != rewrite {
+			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)
+		}
 	}
 }

pkg/symlink/testdata/fs/j/k

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