join: loosen cleanliness requirements for SecureJoin root

It turns out that some users do provide unclean paths like "foo/bar/"
and as a result the new behaviour in commit bc750ad ("join: return
an error if root is unclean path") was far too aggressive and lead to
regressions.

The more gentle solution is to only error out if the path contains a
".." component (which is the only component type we are really worried
about here because it's the only one that can turn a safe
root-joined-path into an unsafe one due to how symlinks are resolved on
Linux).

Fixes: bc750ad ("join: return an error if root is unclean path")
Signed-off-by: Aleksa Sarai <[email protected]>

Author: cyphar

CHANGELOG.md

@@ -6,6 +6,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased] ##
 
+### Fixed ###
+- The restrictions added for `root` paths passed to `SecureJoin` in 0.4.0 was
+  found to be too strict and caused some regressions when folks tried to
+  update, so this restriction has been relaxed to only return an error if the
+  path contains a `..` component. We still recommend users use `filepath.Clean`
+  (and even `filepath.EvalSymlinks`) on the `root` path they are using, but at
+  least you will no longer be punished for "trivial" unclean paths.
+
 ## [0.4.0] - 2025-01-13 ##
 
 ### Breaking ####

join.go

@@ -24,9 +24,30 @@ func IsNotExist(err error) bool {
 	return errors.Is(err, os.ErrNotExist) || errors.Is(err, syscall.ENOTDIR) || errors.Is(err, syscall.ENOENT)
 }
 
-// errUncleanRoot is returned if the user provides SecureJoinVFS with a path
-// that is not filepath.Clean'd.
-var errUncleanRoot = errors.New("root path provided to SecureJoin was not filepath.Clean")
+// errUnsafeRoot is returned if the user provides SecureJoinVFS with a path
+// that contains ".." components.
+var errUnsafeRoot = errors.New("root path provided to SecureJoin contains '..' components")
+
+// stripVolume just gets rid of the Windows volume included in a path. Based on
+// some godbolt tests, the Go compiler is smart enough to make this a no-op on
+// Linux.
+func stripVolume(path string) string {
+	return path[len(filepath.VolumeName(path)):]
+}
+
+// hasDotDot checks if the path contains ".." components in a platform-agnostic
+// way.
+func hasDotDot(path string) bool {
+	// If we are on Windows, strip any volume letters. It turns out that
+	// C:..\foo may (or may not) be a valid pathname and we need to handle that
+	// leading "..".
+	path = stripVolume(path)
+	// Look for "/../" in the path, but we need to handle leading and trailing
+	// ".."s by adding separators. Doing this with filepath.Separator is ugly
+	// so just convert to Unix-style "/" first.
+	path = filepath.ToSlash(path)
+	return strings.Contains("/"+path+"/", "/../")
+}
 
 // SecureJoinVFS joins the two given path components (similar to [filepath.Join]) except
 // that the returned path is guaranteed to be scoped inside the provided root
@@ -58,11 +79,12 @@ var errUncleanRoot = errors.New("root path provided to SecureJoin was not filepa
 // avoid containing symlink components. Of course, the root also *must not* be
 // attacker-controlled.
 func SecureJoinVFS(root, unsafePath string, vfs VFS) (string, error) {
-	// The root path needs to be clean, otherwise when we join the subpath we
-	// will end up with a weird path. We could work around this but users
-	// should not be giving us unclean paths in the first place.
-	if filepath.Clean(root) != root {
-		return "", errUncleanRoot
+	// The root path must not contain ".." components, otherwise when we join
+	// the subpath we will end up with a weird path. We could work around this
+	// in other ways but users shouldn't be giving us non-lexical root paths in
+	// the first place.
+	if hasDotDot(root) {
+		return "", errUnsafeRoot
 	}
 
 	// Use the os.* VFS implementation if none was specified.
@@ -77,9 +99,10 @@ func SecureJoinVFS(root, unsafePath string, vfs VFS) (string, error) {
 		linksWalked   int
 	)
 	for remainingPath != "" {
-		if v := filepath.VolumeName(remainingPath); v != "" {
-			remainingPath = remainingPath[len(v):]
-		}
+		// On Windows, if we managed to end up at a path referencing a volume,
+		// drop the volume to make sure we don't end up with broken paths or
+		// escaping the root volume.
+		remainingPath = stripVolume(remainingPath)
 
 		// Get the next path component.
 		var part string

join_test.go

@@ -1,4 +1,4 @@
-// Copyright (C) 2017-2024 SUSE LLC. All rights reserved.
+// Copyright (C) 2017-2025 SUSE LLC. All rights reserved.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
@@ -12,6 +12,8 @@ import (
 	"runtime"
 	"syscall"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 // TODO: These tests won't work on plan9 because it doesn't have symlinks, and
@@ -392,8 +394,59 @@ func TestSecureJoinVFSErrors(t *testing.T) {
 }
 
 func TestUncleanRoot(t *testing.T) {
-	safePath, err := SecureJoin("/foo/..", "bar/baz")
-	if !errors.Is(err, errUncleanRoot) {
-		t.Errorf("SecureJoin with non-clean path should return errUncleanRoot, instead got (%q, %v)", safePath, err)
+	root := t.TempDir()
+
+	for _, test := range []struct {
+		testName, root string
+		expectedErr    error
+	}{
+		{"trailing-dotdot", "foo/..", errUnsafeRoot},
+		{"leading-dotdot", "../foo", errUnsafeRoot},
+		{"middle-dotdot", "../foo", errUnsafeRoot},
+		{"many-dotdot", "foo/../foo/../a", errUnsafeRoot},
+		{"trailing-slash", root + "/foo/bar/", nil},
+		{"trailing-slashes", root + "/foo/bar///", nil},
+		{"many-slashes", root + "/foo///bar////baz", nil},
+		{"plain-dot", root + "/foo/./bar", nil},
+		{"many-dot", root + "/foo/./bar/./.", nil},
+		{"unclean-safe", root + "/foo///./bar/.///.///", nil},
+		{"unclean-unsafe", root + "/foo///./bar/..///.///", errUnsafeRoot},
+	} {
+		test := test // copy iterator
+		t.Run(test.testName, func(t *testing.T) {
+			_, err := SecureJoin(test.root, "foo/bar/baz")
+			if test.expectedErr != nil {
+				assert.ErrorIsf(t, err, test.expectedErr, "SecureJoin with unsafe root %q", test.root)
+			} else {
+				assert.NoErrorf(t, err, "SecureJoin with safe but unclean root %q", test.root)
+			}
+		})
+	}
+}
+
+func TestHasDotDot(t *testing.T) {
+	for _, test := range []struct {
+		testName, path string
+		expected       bool
+	}{
+		{"plain-dotdot", "..", true},
+		{"trailing-dotdot", "foo/bar/baz/..", true},
+		{"leading-dotdot", "../foo/bar/baz", true},
+		{"middle-dotdot", "foo/bar/../baz", true},
+		{"dotdot-in-name1", "foo/..bar/baz", false},
+		{"dotdot-in-name2", "foo/bar../baz", false},
+		{"dotdot-in-name3", "foo/b..r/baz", false},
+		{"dotdot-in-name4", "..foo/bar/baz", false},
+		{"dotdot-in-name5", "foo/bar/baz..", false},
+		{"dot1", "./foo/bar/baz", false},
+		{"dot2", "foo/bar/baz/.", false},
+		{"dot3", "foo/././bar/baz", false},
+		{"unclean", "foo//.//bar/baz////", false},
+	} {
+		test := test // copy iterator
+		t.Run(test.testName, func(t *testing.T) {
+			got := hasDotDot(test.path)
+			assert.Equalf(t, test.expected, got, "unexpected result for hasDotDot(%q)", test.path)
+		})
 	}
 }

join_windows_test.go

@@ -0,0 +1,43 @@
+// Copyright (C) 2017-2025 SUSE LLC. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package securejoin
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// Windows has very specific behaviour relating to volumes, and we can only
+// test it on Windows machines because filepath.* behaviour depends on GOOS.
+//
+// See <https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats>
+// for more information about the various path formats we need to make sure are
+// correctly handled.
+func TestHasDotDot_WindowsVolumes(t *testing.T) {
+	for _, test := range []struct {
+		testName, path string
+		expected       bool
+	}{
+		{"plain-dotdot", `C:..`, true},            // apparently legal
+		{"relative-dotdot", `C:..\foo\bar`, true}, // apparently legal
+		{"trailing-dotdot", `D:\foo\bar\..`, true},
+		{"leading-dotdot", `F:\..\foo\bar`, true},
+		{"middle-dotdot", `F:\foo\..\bar`, true},
+		{"drive-like-path", `\foo\C:..\bar`, false}, // C:.. is a filename here
+		{"unc-dotdot", `\\gondor\share\call\for\aid\..\help`, true},
+		{"dos-dotpath-dotdot1", `\\.\C:\..\foo\bar`, true},
+		{"dos-dotpath-dotdot2", `\\.\C:\foo\..\bar`, true},
+		{"dos-questionpath-dotdot1", `\\?\C:\..\foo\bar`, true},
+		{"dos-questionpath-dotdot2", `\\?\C:\foo\..\bar`, true},
+	} {
+		test := test // copy iterator
+		t.Run(test.testName, func(t *testing.T) {
+			got := hasDotDot(test.path)
+			assert.Equalf(t, test.expected, got, "unexpected result for hasDotDot(`%s`) (VolumeName: %q)", test.path, filepath.VolumeName(test.path))
+		})
+	}
+}