Skip to content

Commit

Permalink
merge #17 into cyphar/filepath-securejoin:main
Browse files Browse the repository at this point in the history 
Aleksa Sarai (5):
  test mocks: procfs: make unsafe fallback more realistic
  tests: lookup: actually swap root in root-swap tests
  mkdir: fix *os.File leak when reopening starting path
  open: make OpenInRoot errors match a simple openat2
  lookup: special-case non-partial lookups

LGTMs: cyphar
  • Loading branch information
@cyphar
cyphar committed Jul 23, 2024
2 parents a604eb6 + 1f4688a commit edab538
Show file tree
Hide file tree
Showing 9 changed files with 252 additions and 219 deletions.
140 changes: 70 additions & 70 deletions lookup_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,16 +40,18 @@ func (se symlinkStackEntry) Close() {

type symlinkStack []*symlinkStackEntry

func (s symlinkStack) IsEmpty() bool {
return len(s) == 0
func (s *symlinkStack) IsEmpty() bool {
return s == nil || len(*s) == 0
}

func (s *symlinkStack) Close() {
for _, link := range *s {
link.Close()
if s != nil {
for _, link := range *s {
link.Close()
}
// TODO: Switch to clear once we switch to Go 1.21.
*s = nil
}
// TODO: Switch to clear once we switch to Go 1.21.
*s = nil
}

var (
Expand All @@ -58,7 +60,7 @@ var (
)

func (s *symlinkStack) popPart(part string) error {
if s.IsEmpty() {
if s == nil || s.IsEmpty() {
// If there is nothing in the symlink stack, then the part was from the
// real path provided by the user, and this is a no-op.
return errEmptyStack
Expand Down Expand Up @@ -102,6 +104,9 @@ func (s *symlinkStack) PopPart(part string) error {
}

func (s *symlinkStack) push(dir *os.File, remainingPath, linkTarget string) error {
if s == nil {
return nil
}
// Split the link target and clean up any "" parts.
linkTargetParts := slices.DeleteFunc(
strings.Split(linkTarget, "/"),
Expand Down Expand Up @@ -145,7 +150,7 @@ func (s *symlinkStack) SwapLink(linkPart string, dir *os.File, remainingPath, li
}

func (s *symlinkStack) PopTopSymlink() (*os.File, string, bool) {
if s.IsEmpty() {
if s == nil || s.IsEmpty() {
return nil, "", false
}
tailEntry := (*s)[0]
Expand All @@ -157,7 +162,22 @@ func (s *symlinkStack) PopTopSymlink() (*os.File, string, bool) {
// within the provided root (a-la RESOLVE_IN_ROOT) and opens the final existing
// component of the requested path, returning a file handle to the final
// existing component and a string containing the remaining path components.
func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string, Err error) {
func partialLookupInRoot(root *os.File, unsafePath string) (*os.File, string, error) {
return lookupInRoot(root, unsafePath, true)
}

func completeLookupInRoot(root *os.File, unsafePath string) (*os.File, error) {
handle, remainingPath, err := lookupInRoot(root, unsafePath, false)
if remainingPath != "" && err == nil {
// should never happen
err = fmt.Errorf("[bug] non-empty remaining path when doing a non-partial lookup: %q", remainingPath)
}
// lookupInRoot(partial=false) will always close the handle if an error is
// returned, so no need to double-check here.
return handle, err
}

func lookupInRoot(root *os.File, unsafePath string, partial bool) (Handle *os.File, _ string, _ error) {
unsafePath = filepath.ToSlash(unsafePath) // noop

// This is very similar to SecureJoin, except that we operate on the
Expand All @@ -166,7 +186,7 @@ func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string

// Try to use openat2 if possible.
if hasOpenat2() {
return partialLookupOpenat2(root, unsafePath)
return lookupOpenat2(root, unsafePath, partial)
}

// Get the "actual" root path from /proc/self/fd. This is necessary if the
Expand All @@ -183,7 +203,8 @@ func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string
return nil, "", fmt.Errorf("clone root fd: %w", err)
}
defer func() {
if Err != nil {
// If a handle is not returned, close the internal handle.
if Handle == nil {
_ = currentDir.Close()
}
}()
Expand All @@ -200,8 +221,11 @@ func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string
// Note that the stack is ONLY used for book-keeping. All of the actual
// path walking logic is still based on currentPath/remainingPath and
// currentDir (as in SecureJoin).
var symlinkStack symlinkStack
defer symlinkStack.Close()
var symStack *symlinkStack
if partial {
symStack = new(symlinkStack)
defer symStack.Close()
}

var (
linksWalked int
Expand Down Expand Up @@ -233,7 +257,7 @@ func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string
// If we logically hit the root, just clone the root rather than
// opening the part and doing all of the other checks.
if nextPath == "/" {
if err := symlinkStack.PopPart(part); err != nil {
if err := symStack.PopPart(part); err != nil {
return nil, "", fmt.Errorf("walking into root with part %q failed: %w", part, err)
}
// Jump to root.
Expand All @@ -258,35 +282,6 @@ func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string
}

switch st.Mode() & os.ModeType {
case os.ModeDir:
// If we are dealing with a directory, simply walk into it.
_ = currentDir.Close()
currentDir = nextDir
currentPath = nextPath

// The part was real, so drop it from the symlink stack.
if err := symlinkStack.PopPart(part); err != nil {
return nil, "", fmt.Errorf("walking into directory %q failed: %w", part, err)
}

// If we are operating on a .., make sure we haven't escaped.
// We only have to check for ".." here because walking down
// into a regular component component cannot cause you to
// escape. This mirrors the logic in RESOLVE_IN_ROOT, except we
// have to check every ".." rather than only checking after a
// rename or mount on the system.
if part == ".." {
// Make sure the root hasn't moved.
if err := checkProcSelfFdPath(logicalRootPath, root); err != nil {
return nil, "", fmt.Errorf("root path moved during lookup: %w", err)
}
// Make sure the path is what we expect.
fullPath := logicalRootPath + nextPath
if err := checkProcSelfFdPath(fullPath, currentDir); err != nil {
return nil, "", fmt.Errorf("walking into %q had unexpected result: %w", part, err)
}
}

case os.ModeSymlink:
// readlinkat implies AT_EMPTY_PATH.
linkDest, err := readlinkatFile(nextDir, "")
Expand All @@ -298,11 +293,11 @@ func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string

linksWalked++
if linksWalked > maxSymlinkLimit {
return nil, "", &os.PathError{Op: "partialLookupInRoot", Path: logicalRootPath + "/" + unsafePath, Err: unix.ELOOP}
return nil, "", &os.PathError{Op: "securejoin.lookupInRoot", Path: logicalRootPath + "/" + unsafePath, Err: unix.ELOOP}
}

// Swap out the symlink's component for the link entry itself.
if err := symlinkStack.SwapLink(part, currentDir, oldRemainingPath, linkDest); err != nil {
if err := symStack.SwapLink(part, currentDir, oldRemainingPath, linkDest); err != nil {
return nil, "", fmt.Errorf("walking into symlink %q failed: push symlink: %w", part, err)
}

Expand All @@ -319,48 +314,53 @@ func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string
currentDir = rootClone
currentPath = "/"
}

default:
// For any other file type, we can't walk further and so we've
// hit the end of the lookup. The handling is very similar to
// ENOENT from openat(2), except that we return a handle to the
// component we just walked into (and we drop the component
// from the symlink stack).
// If we are dealing with a directory, simply walk into it.
_ = currentDir.Close()
currentDir = nextDir
currentPath = nextPath

// The part existed, so drop it from the symlink stack.
if err := symlinkStack.PopPart(part); err != nil {
return nil, "", fmt.Errorf("walking into non-directory %q failed: %w", part, err)
// The part was real, so drop it from the symlink stack.
if err := symStack.PopPart(part); err != nil {
return nil, "", fmt.Errorf("walking into directory %q failed: %w", part, err)
}

// If there are any remaining components in the symlink stack,
// we are still within a symlink resolution and thus we hit a
// dangling symlink. So pretend that the first symlink in the
// stack we hit was an ENOENT (to match openat2).
if oldDir, remainingPath, ok := symlinkStack.PopTopSymlink(); ok {
_ = nextDir.Close()
return oldDir, remainingPath, nil
// If we are operating on a .., make sure we haven't escaped.
// We only have to check for ".." here because walking down
// into a regular component component cannot cause you to
// escape. This mirrors the logic in RESOLVE_IN_ROOT, except we
// have to check every ".." rather than only checking after a
// rename or mount on the system.
if part == ".." {
// Make sure the root hasn't moved.
if err := checkProcSelfFdPath(logicalRootPath, root); err != nil {
return nil, "", fmt.Errorf("root path moved during lookup: %w", err)
}
// Make sure the path is what we expect.
fullPath := logicalRootPath + nextPath
if err := checkProcSelfFdPath(fullPath, currentDir); err != nil {
return nil, "", fmt.Errorf("walking into %q had unexpected result: %w", part, err)
}
}

// The current component exists, so return it.
return nextDir, remainingPath, nil
}

case errors.Is(err, os.ErrNotExist):
default:
if !partial {
return nil, "", err
}
// If there are any remaining components in the symlink stack, we
// are still within a symlink resolution and thus we hit a dangling
// symlink. So pretend that the first symlink in the stack we hit
// was an ENOENT (to match openat2).
if oldDir, remainingPath, ok := symlinkStack.PopTopSymlink(); ok {
if oldDir, remainingPath, ok := symStack.PopTopSymlink(); ok {
_ = currentDir.Close()
return oldDir, remainingPath, nil
return oldDir, remainingPath, err
}
// We have hit a final component that doesn't exist, so we have our
// partial open result. Note that we have to use the OLD remaining
// path, since the lookup failed.
return currentDir, oldRemainingPath, nil

default:
return nil, "", err
return currentDir, oldRemainingPath, err
}
}
// All of the components existed!
Expand Down
Loading

0 comments on commit edab538

Please sign in to comment.