merge #43 into cyphar/filepath-securejoin:main

Aleksa Sarai (1):
  join: return an error if root is unclean path

LGTMs: cyphar

Author: cyphar

CHANGELOG.md

@@ -6,6 +6,22 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased] ##
 
+### Breaking ####
+- `SecureJoin(VFS)` will now return an error if the provided `root` is not a
+  `filepath.Clean`'d path.
+
+  While it is ultimately the responsibility of the caller to ensure the root is
+  a safe path to use, passing a path like `/symlink/..` as a root would result
+  in the `SecureJoin`'d path being placed in `/` even though `/symlink/..`
+  might be a different directory, and so we should more strongly discourage
+  such usage.
+
+  All major users of `securejoin.SecureJoin` already ensure that the paths they
+  provide are safe (and this is ultimately a question of user error), but
+  removing this foot-gun is probably a good idea. Of course, this is
+  necessarily a breaking API change (though we expect no real users to be
+  affected by it).
+
 ## [0.3.6] - 2024-12-17 ##
 
 ### Compatibility ###

join.go

@@ -1,5 +1,5 @@
 // Copyright (C) 2014-2015 Docker Inc & Go Authors. All rights reserved.
-// 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.
 
@@ -24,6 +24,10 @@ 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")
+
 // 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
 // path (when evaluated). Any symbolic links in the path are evaluated with the
@@ -46,7 +50,21 @@ func IsNotExist(err error) bool {
 // provided via direct input or when evaluating symlinks. Therefore:
 //
 // "C:\Temp" + "D:\path\to\file.txt" results in "C:\Temp\path\to\file.txt"
+//
+// If the provided root is not [filepath.Clean] then an error will be returned,
+// as such root paths are bordering on somewhat unsafe and using such paths is
+// not best practice. We also strongly suggest that any root path is first
+// fully resolved using [filepath.EvalSymlinks] or otherwise constructed to
+// 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
+	}
+
 	// Use the os.* VFS implementation if none was specified.
 	if vfs == nil {
 		vfs = osVFS{}

join_test.go

@@ -390,3 +390,10 @@ 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)
+	}
+}