diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bcd65c..9a4841a 100644 --- a/CHANGELOG.md +++ b/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 #### diff --git a/join.go b/join.go index b19e95a..e6634d4 100644 --- a/join.go +++ b/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 diff --git a/join_test.go b/join_test.go index a4adfcf..ac7ddbf 100644 --- a/join_test.go +++ b/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) + }) } } diff --git a/join_windows_test.go b/join_windows_test.go new file mode 100644 index 0000000..af1a09b --- /dev/null +++ b/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 +// 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)) + }) + } +}