Skip to content

Strip leading slashes to prevent Beneath base-directory escape on non-Linux Unix#3

Open
evilgensec wants to merge 1 commit into
google:mainfrom
evilgensec:fix-beneath-leading-slash-traversal
Open

Strip leading slashes to prevent Beneath base-directory escape on non-Linux Unix#3
evilgensec wants to merge 1 commit into
google:mainfrom
evilgensec:fix-beneath-leading-slash-traversal

Conversation

@evilgensec

Copy link
Copy Markdown

Summary

On non-Linux Unix builds (unix && !linux: macOS, the BSDs, illumos, Solaris, AIX), a leading-slash argument to the Beneath family (OpenBeneath, CreateBeneath, OpenFileBeneath, ReadFileBeneath, WriteFileBeneath) escapes the base directory, allowing arbitrary file read and write outside it — defeating the documented guarantee that these APIs "do not permit path traversal" and that the file "must be somewhere underneath the base directory".

base := t.TempDir()
os.WriteFile(filepath.Join(base, "..", "SECRET.txt"), []byte("outside"), 0600)

f, _ := safeopen.OpenBeneath(base, "/../SECRET.txt")         // reads the file OUTSIDE base
safeopen.WriteFileBeneath(base, "/../pwned.txt", data, 0600) // writes OUTSIDE base
safeopen.OpenBeneath(base, "../SECRET.txt")                  // (control) correctly rejected

Root cause

unixRelativePathDoesntTraverse (safeopen_nix.go) does not strip a leading / before its traversal check. filepath.Clean("/../x") returns "/x", so the strings.HasPrefix(path, "../") check passes and the path is accepted. openFileBeneath then walks the raw path, skipping the empty leading segment and opening .. via unix.Openat (O_NOFOLLOW does not stop ..), climbing out of the base directory.

Linux is unaffected: canTraverseUnixRelPath (safeopen_linux.go) already does path = strings.TrimLeft(path, "/") and additionally uses openat2 RESOLVE_BENEATH. The At family is unaffected because it rejects any /.

Fix

Mirror the Linux implementation: strip leading slashes in unixRelativePathDoesntTraverse before the traversal check, so an absolute-looking input is treated as relative to the base directory. A leading-slash input that nets a traversal (/../x) is then rejected, while a leading-slash input that stays in-base (/subdir/x) still succeeds.

Test

Adds TestUnixBeneathLeadingSlashTraversal, covering both read and write for "/../x", "/./../x", and "/d/../../x", plus an in-base "/ok.txt" that must still succeed. The test fails before this change and passes after it; the existing suite is unchanged and still passes.

…se-directory escape

On non-Linux Unix builds (unix && !linux), a leading-slash argument to the
Beneath family (OpenBeneath/CreateBeneath/ReadFileBeneath/WriteFileBeneath)
escaped the base directory. unixRelativePathDoesntTraverse did not strip a
leading "/", so filepath.Clean("/../x") == "/x" -- the "../" prefix check then
passed, and openFileBeneath walked the raw path, opening ".." via unix.Openat
(O_NOFOLLOW does not stop ".."), climbing out of the base directory. This
allowed arbitrary file read and write outside the intended base, defeating the
"do not permit path traversal" / "must be somewhere underneath the base
directory" guarantee on macOS, the BSDs, illumos, Solaris and AIX. Linux was
unaffected because canTraverseUnixRelPath already strips the leading slash (and
uses openat2 RESOLVE_BENEATH); the At family is unaffected because it rejects
any "/".

Mirror the Linux behavior: strip leading slashes before the traversal check so
an absolute-looking input is treated as relative to the base directory. Add a
regression test (TestUnixBeneathLeadingSlashTraversal) covering read and write
for "/../x", "/./../x" and "/d/../../x", and an in-base "/ok.txt" that must
still succeed. The test fails before this change and passes after it.
@evilgensec

Copy link
Copy Markdown
Author

Checking in on this. Anything I can do to help get it reviewed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant