]> Cypherpunks repositories - gostls13.git/commitdiff
os: avoid escape from Root via paths ending in ../
authorDamien Neil <dneil@google.com>
Wed, 16 Apr 2025 18:01:19 +0000 (11:01 -0700)
committerCherry Mui <cherryyz@google.com>
Tue, 6 May 2025 18:33:18 +0000 (11:33 -0700)
The doInRoot function operates on a path split into components.
The final path component retained any trailing path separator
characters, to permit operations in a Root to retain the
trailing-separator behavior of non-Root operations. However,
doInRoot failed to take trailing separators into account
when checking for .. path components.

This could permit opening the parent directory of the Root
with a path ending in "../".

Change the split path to never include path separators in
components, and handle trailing separators independently
of the split path.

Thanks to Dan Sebastian Thrane of SDU eScience Center for
reporting this issue.

Fixes #73555
Fixes CVE-2025-22873

Change-Id: I9a33a145c22f5eb1dd4e4cafae5fcc61a8d4f0d4
Reviewed-on: https://go-internal-review.googlesource.com/c/go/+/2160
Reviewed-by: Neal Patel <nealpatel@google.com>
Reviewed-by: Roland Shoemaker <bracewell@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/670036
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
src/os/root.go
src/os/root_js.go
src/os/root_openat.go
src/os/root_test.go

index 49d09fe97b06c102676dfcdf658a2add7520d628..fb2bb8350a716d563d42e3a1105bc4a757f67300 100644 (file)
@@ -254,20 +254,20 @@ func (r *Root) logStat(name string) {
 //
 // "." components are removed, except in the last component.
 //
-// Path separators following the last component are preserved.
-func splitPathInRoot(s string, prefix, suffix []string) (_ []string, err error) {
+// Path separators following the last component are returned in suffixSep.
+func splitPathInRoot(s string, prefix, suffix []string) (_ []string, suffixSep string, err error) {
        if len(s) == 0 {
-               return nil, errors.New("empty path")
+               return nil, "", errors.New("empty path")
        }
        if IsPathSeparator(s[0]) {
-               return nil, errPathEscapes
+               return nil, "", errPathEscapes
        }
 
        if runtime.GOOS == "windows" {
                // Windows cleans paths before opening them.
                s, err = rootCleanPath(s, prefix, suffix)
                if err != nil {
-                       return nil, err
+                       return nil, "", err
                }
                prefix = nil
                suffix = nil
@@ -283,13 +283,14 @@ func splitPathInRoot(s string, prefix, suffix []string) (_ []string, err error)
                }
                parts = append(parts, s[i:j])
                // Advance to the next component, or end of the path.
+               partEnd := j
                for j < len(s) && IsPathSeparator(s[j]) {
                        j++
                }
                if j == len(s) {
                        // If this is the last path component,
                        // preserve any trailing path separators.
-                       parts[len(parts)-1] = s[i:]
+                       suffixSep = s[partEnd:]
                        break
                }
                if parts[len(parts)-1] == "." {
@@ -303,7 +304,7 @@ func splitPathInRoot(s string, prefix, suffix []string) (_ []string, err error)
                parts = parts[:len(parts)-1]
        }
        parts = append(parts, suffix...)
-       return parts, nil
+       return parts, suffixSep, nil
 }
 
 // FS returns a file system (an fs.FS) for the tree of files in the root.
index 70aa5f9ccd0cd01479c20615866a52c26075ee3d..56a37dafe1626e5125c69f4d8c7bf9310bd8ffbe 100644 (file)
@@ -33,7 +33,7 @@ func checkPathEscapesInternal(r *Root, name string, lstat bool) error {
        if r.root.closed.Load() {
                return ErrClosed
        }
-       parts, err := splitPathInRoot(name, nil, nil)
+       parts, suffixSep, err := splitPathInRoot(name, nil, nil)
        if err != nil {
                return err
        }
@@ -61,11 +61,15 @@ func checkPathEscapesInternal(r *Root, name string, lstat bool) error {
                        continue
                }
 
-               if lstat && i == len(parts)-1 {
-                       break
+               part := parts[i]
+               if i == len(parts)-1 {
+                       if lstat {
+                               break
+                       }
+                       part += suffixSep
                }
 
-               next := joinPath(base, parts[i])
+               next := joinPath(base, part)
                fi, err := Lstat(next)
                if err != nil {
                        if IsNotExist(err) {
@@ -82,10 +86,19 @@ func checkPathEscapesInternal(r *Root, name string, lstat bool) error {
                        if symlinks > rootMaxSymlinks {
                                return errors.New("too many symlinks")
                        }
-                       newparts, err := splitPathInRoot(link, parts[:i], parts[i+1:])
+                       newparts, newSuffixSep, err := splitPathInRoot(link, parts[:i], parts[i+1:])
                        if err != nil {
                                return err
                        }
+                       if i == len(parts) {
+                               // suffixSep contains any trailing path separator characters
+                               // in the link target.
+                               // If we are replacing the remainder of the path, retain these.
+                               // If we're replacing some intermediate component of the path,
+                               // ignore them, since intermediate components must always be
+                               // directories.
+                               suffixSep = newSuffixSep
+                       }
                        parts = newparts
                        continue
                }
index f67794cd720f6d5eabcc61176a6e3096c62d152f..db2ae2295f810b2a60a445099812d52956bb97fc 100644 (file)
@@ -178,7 +178,7 @@ func doInRoot[T any](r *Root, name string, f func(parent sysfdType, name string)
        }
        defer r.root.decref()
 
-       parts, err := splitPathInRoot(name, nil, nil)
+       parts, suffixSep, err := splitPathInRoot(name, nil, nil)
        if err != nil {
                return ret, err
        }
@@ -242,7 +242,9 @@ func doInRoot[T any](r *Root, name string, f func(parent sysfdType, name string)
                        // Call f to decide what to do with it.
                        // If f returns errSymlink, this element is a symlink
                        // which should be followed.
-                       ret, err = f(dirfd, parts[i])
+                       // suffixSep contains any trailing separator characters
+                       // which we rejoin to the final part at this time.
+                       ret, err = f(dirfd, parts[i]+suffixSep)
                        if _, ok := err.(errSymlink); !ok {
                                return ret, err
                        }
@@ -264,10 +266,19 @@ func doInRoot[T any](r *Root, name string, f func(parent sysfdType, name string)
                        if symlinks > rootMaxSymlinks {
                                return ret, syscall.ELOOP
                        }
-                       newparts, err := splitPathInRoot(string(e), parts[:i], parts[i+1:])
+                       newparts, newSuffixSep, err := splitPathInRoot(string(e), parts[:i], parts[i+1:])
                        if err != nil {
                                return ret, err
                        }
+                       if i == len(parts)-1 {
+                               // suffixSep contains any trailing path separator characters
+                               // in the link target.
+                               // If we are replacing the remainder of the path, retain these.
+                               // If we're replacing some intermediate component of the path,
+                               // ignore them, since intermediate components must always be
+                               // directories.
+                               suffixSep = newSuffixSep
+                       }
                        if len(newparts) < i || !slices.Equal(parts[:i], newparts[:i]) {
                                // Some component in the path which we have already traversed
                                // has changed. We need to restart parsing from the root.
index 63c921b66bec799930f739d6fa8be48ff01c40ad..530d5abda632cf9c6e5115fb9630ea63e6e51282 100644 (file)
@@ -187,6 +187,30 @@ var rootTestCases = []rootTest{{
        open:    "link",
        target:  "target",
        ltarget: "link",
+}, {
+       name: "symlink dotdot slash",
+       fs: []string{
+               "link => ../",
+       },
+       open:      "link",
+       ltarget:   "link",
+       wantError: true,
+}, {
+       name: "symlink ending in slash",
+       fs: []string{
+               "dir/",
+               "link => dir/",
+       },
+       open:   "link/target",
+       target: "dir/target",
+}, {
+       name: "symlink dotdot dotdot slash",
+       fs: []string{
+               "dir/link => ../../",
+       },
+       open:      "dir/link",
+       ltarget:   "dir/link",
+       wantError: true,
 }, {
        name: "symlink chain",
        fs: []string{
@@ -214,6 +238,16 @@ var rootTestCases = []rootTest{{
        },
        open:   "a/../a/b/../../a/b/../b/target",
        target: "a/b/target",
+}, {
+       name:      "path with dotdot slash",
+       fs:        []string{},
+       open:      "../",
+       wantError: true,
+}, {
+       name:      "path with dotdot dotdot slash",
+       fs:        []string{},
+       open:      "a/../../",
+       wantError: true,
 }, {
        name: "dotdot no symlink",
        fs: []string{
@@ -744,6 +778,21 @@ func testRootMoveFrom(t *testing.T, rename bool) {
                                if !rename && runtime.GOOS == "js" {
                                        wantError = true
                                }
+
+                               // Windows allows creating a hard link to a file symlink,
+                               // but not to a directory symlink.
+                               //
+                               // This uses os.Stat to check the link target, because this
+                               // is easier than figuring out whether the link itself is a
+                               // directory link. The link was created with os.Symlink,
+                               // which creates directory links when the target is a directory,
+                               // so this is good enough for a test.
+                               if !rename && runtime.GOOS == "windows" {
+                                       st, err := os.Stat(filepath.Join(root.Name(), test.ltarget))
+                                       if err == nil && st.IsDir() {
+                                               wantError = true
+                                       }
+                               }
                        }
 
                        const dstPath = "destination"
@@ -1019,6 +1068,20 @@ var rootConsistencyTestCases = []rootConsistencyTest{{
                "b/target",
        },
        open: "a/../target",
+}, {
+       name: "symlink to dir ends in slash",
+       fs: []string{
+               "dir/",
+               "link => dir",
+       },
+       open: "link",
+}, {
+       name: "symlink to file ends in slash",
+       fs: []string{
+               "file",
+               "link => file/",
+       },
+       open: "link",
 }, {
        name: "long file name",
        open: strings.Repeat("a", 500),