From bc750ad62e1e91382c72e623ced46099258aa887 Mon Sep 17 00:00:00 2001
From: Aleksa Sarai <cyphar@cyphar.com>
Date: Fri, 10 Jan 2025 00:43:02 +1100
Subject: [PATCH] join: return an error if root is unclean path
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If a user provides an unclean root path, we will implicitly clean it at
the end of SecureJoin (which may result in a path that doesn't exist or
has "escaped" the root). Such usage is fundamentally unsafe so we should
just return an error.

Reported-by: Erik Sjölund <erik.sjolund@gmail.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 CHANGELOG.md | 16 ++++++++++++++++
 join.go      | 20 +++++++++++++++++++-
 join_test.go |  7 +++++++
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index cb1252b..b437ee6 100644
--- a/CHANGELOG.md
+++ b/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 ###
diff --git a/join.go b/join.go
index e0ee3f2..b19e95a 100644
--- a/join.go
+++ b/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{}
diff --git a/join_test.go b/join_test.go
index ef081d6..a4adfcf 100644
--- a/join_test.go
+++ b/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)
+	}
+}