Skip to content

Commit e29b5c2

Browse files
committed
pathrs: add "hallucination" helpers for SecureJoin magic
In order to maintain compatibility with previous releases of runc (which permitted dangling symlinks as path components by permitting non-existent path components to be treated like real directories) we have to first do SecureJoin to construct a target path that is compatible with the old behaviour but has all dangling symlinks (or other invalid paths like ".." components after non-existent directories) removed. This is effectively a more generic verison of commit 3f92552 ("rootfs: re-allow dangling symlinks in mount targets") and will let us remove the need for open-coding SecureJoin workarounds. Signed-off-by: Aleksa Sarai <[email protected]>
1 parent 42e14bb commit e29b5c2

File tree

4 files changed

+40
-25
lines changed

4 files changed

+40
-25
lines changed

internal/pathrs/mkdirall_pathrslite.go

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ package pathrs
2121
import (
2222
"fmt"
2323
"os"
24-
"path/filepath"
2524

2625
"github.com/cyphar/filepath-securejoin/pathrs-lite"
2726
"github.com/sirupsen/logrus"
@@ -50,18 +49,9 @@ import (
5049
// needed for a lot of runc callers and fixing this would require reworking a
5150
// lot of path logic).
5251
func MkdirAllInRoot(root, unsafePath string, mode os.FileMode) (*os.File, error) {
53-
// If the path is already "within" the root, get the path relative to the
54-
// root and use that as the unsafe path. This is necessary because a lot of
55-
// MkdirAllInRoot callers have already done SecureJoin, and refactoring
56-
// all of them to stop using these SecureJoin'd paths would require a fair
57-
// amount of work.
58-
// TODO(cyphar): Do the refactor to libpathrs once it's ready.
59-
if IsLexicallyInRoot(root, unsafePath) {
60-
subPath, err := filepath.Rel(root, unsafePath)
61-
if err != nil {
62-
return nil, err
63-
}
64-
unsafePath = subPath
52+
unsafePath, err := hallucinateUnsafePath(root, unsafePath)
53+
if err != nil {
54+
return nil, fmt.Errorf("failed to construct hallucinated target path: %w", err)
6555
}
6656

6757
// Check for any silly mode bits.

internal/pathrs/path.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import (
2222
"os"
2323
"path/filepath"
2424
"strings"
25+
26+
securejoin "github.com/cyphar/filepath-securejoin"
2527
)
2628

2729
// IsLexicallyInRoot is shorthand for strings.HasPrefix(path+"/", root+"/"),
@@ -82,3 +84,33 @@ func LexicallyStripRoot(root, path string) string {
8284
}
8385
return LexicallyCleanPath("/" + path)
8486
}
87+
88+
// hallucinateUnsafePath creates a new unsafePath which has all symlinks
89+
// (including dangling symlinks) fully resolved and any non-existent components
90+
// treated as though they are real. This is effectively just a wrapper around
91+
// [securejoin.SecureJoin] that strips the root. This path *IS NOT* safe to use
92+
// as-is, you *MUST* operate on the returned path with pathrs-lite.
93+
//
94+
// The reason for this methods is that in previous runc versions, we would
95+
// tolerate nonsense paths with dangling symlinks as path components.
96+
// pathrs-lite does not support this, so instead we have to emulate this
97+
// behaviour by doing SecureJoin *purely to get a semi-reasonable path to use*
98+
// and then we use pathrs-lite to operate on the path safely.
99+
//
100+
// It would be quite difficult to emulate this in a race-free way in
101+
// pathrs-lite, so instead we use [securejoin.SecureJoin] to simply produce a
102+
// new candidate path for operations like [MkdirAllInRoot] so they can then
103+
// operate on the new unsafePath as if it was what the user requested.
104+
//
105+
// If unsafePath is already lexically inside root, it is stripped before
106+
// re-resolving it (this is done to ensure compatibility with legacy callers
107+
// within runc that call SecureJoin before calling into pathrs).
108+
func hallucinateUnsafePath(root, unsafePath string) (string, error) {
109+
unsafePath = LexicallyStripRoot(root, unsafePath)
110+
weirdPath, err := securejoin.SecureJoin(root, unsafePath)
111+
if err != nil {
112+
return "", err
113+
}
114+
unsafePath = LexicallyStripRoot(root, weirdPath)
115+
return unsafePath, nil
116+
}

internal/pathrs/root_pathrslite.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,11 @@ func OpenInRoot(root, subpath string, flags int) (*os.File, error) {
5050
// include it in the passed flags. The fileMode argument uses unix.* mode bits,
5151
// *not* os.FileMode.
5252
func CreateInRoot(root, subpath string, flags int, fileMode uint32) (*os.File, error) {
53+
subpath, err := hallucinateUnsafePath(root, subpath)
54+
if err != nil {
55+
return nil, fmt.Errorf("failed to construct hallucinated target path: %w", err)
56+
}
57+
5358
dir, filename := filepath.Split(subpath)
5459
if filepath.Join("/", filename) == "/" {
5560
return nil, fmt.Errorf("create in root subpath %q has bad trailing component %q", subpath, filename)

libcontainer/rootfs_linux.go

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -546,18 +546,6 @@ func (m *mountEntry) createOpenMountpoint(rootfs string) (Err error) {
546546
}
547547
dstIsFile = !fi.IsDir()
548548
}
549-
550-
// In previous runc versions, we would tolerate nonsense paths with
551-
// dangling symlinks as path components. pathrs-lite does not support
552-
// this, so instead we have to emulate this behaviour by doing
553-
// SecureJoin *purely to get a semi-reasonable path to use* and then we
554-
// use pathrs-lite to operate on the path safely.
555-
newUnsafePath, err := securejoin.SecureJoin(rootfs, unsafePath)
556-
if err != nil {
557-
return err
558-
}
559-
unsafePath = pathrs.LexicallyStripRoot(rootfs, newUnsafePath)
560-
561549
if dstIsFile {
562550
dstFile, err = pathrs.CreateInRoot(rootfs, unsafePath, unix.O_CREAT|unix.O_EXCL|unix.O_NOFOLLOW, 0o644)
563551
} else {

0 commit comments

Comments
 (0)