Skip to content

Commit c41bcd7

Browse files
committed
proc: switch to much safer resolver for non-openat2 systems
Previously we would just use openat(2) to open subpaths with the "safe" procfs resolver. For some users (namely, those that couldn't get a non-overmounted handle and didn't have openat2), this was actually just as unsafe as operating on /proc directly. This problem has been solved in libpathrs (by creating a fairly minimal resolver just for procfs paths, that removes the need for procfs-based verification and also adds some additional protections regular filesystem resolvers don't need), but due to the needs of that project the resolver is a bit more complicated. For filepath-securejoin (which only supports O_PATH|O_NOFOLLOW for resolution, followed by reopening) we can use an even simpler lookup implementation. Signed-off-by: Aleksa Sarai <[email protected]>
1 parent 19e3c52 commit c41bcd7

File tree

4 files changed

+262
-61
lines changed

4 files changed

+262
-61
lines changed

CHANGELOG.md

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,41 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
66

77
## [Unreleased] ##
88

9+
### Added ###
10+
- Previously, the hardened procfs implementation (used internally within
11+
`Reopen` and `Open(at)InRoot`) only protected against overmount attacks on
12+
systems with `openat2(2)` (Linux 5.6) or systems with `fsopen(2)` or
13+
`open_tree(2)` (Linux 4.18) and programs with privileges to use them (with
14+
some caveats about locked mounts that probably affect very few users). For
15+
other users, an attacker with the ability to create malicious mounts (on most
16+
systems, a sysadmin) could trick you into operating on files you didn't
17+
expect. This attack only really makes sense in the context of container
18+
runtime implementations.
19+
20+
This was considered a reasonable trade-off, as the long-term intention was to
21+
get all users to just switch to [libpathrs][] if they wanted to use the safe
22+
`procfs` API (which had more extensive protections, and is what these new
23+
protections in `filepath-securejoin` are based on).
24+
25+
The procfs API will now be more protected against attackers on systems
26+
lacking the aforementioned protections. However, the most comprehensive of
27+
these protections effectively rely on [`statx(STATX_MNT_ID)`][statx.2] (Linux
28+
5.8). On older kernel versions, there is no effective protection (there is
29+
some minimal protection against non-`procfs` filesystem components but a
30+
sufficiently clever attacker can work around those). In addition,
31+
`STATX_MNT_ID` is vulnerable to mount ID reuse attacks by sufficiently
32+
motivated and privileged attackers -- this problem is mitigated with
33+
`STATX_MNT_ID_UNIQUE` (Linux 6.8) but that raises the minimum kernel version
34+
for more protection.
35+
36+
The fact that these protections are quite limited despite needing a fair bit
37+
of extra code to handle was one of the primary reasons we did not initially
38+
implement this in `filepath-securejoin` ([libpathrs][] supports all of this,
39+
of course).
40+
41+
[libpathrs]: https://github.com/openSUSE/libpathrs
42+
[statx.2]: https://www.man7.org/linux/man-pages/man2/statx.2.html
43+
944
## [0.4.1] - 2025-01-28 ##
1045

1146
### Fixed ###

procfs_linux.go

Lines changed: 8 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
//go:build linux
22

3-
// Copyright (C) 2024 SUSE LLC. All rights reserved.
3+
// Copyright (C) 2024-2025 SUSE LLC. All rights reserved.
44
// Use of this source code is governed by a BSD-style
55
// license that can be found in the LICENSE file.
66

@@ -256,58 +256,13 @@ func procThreadSelf(procRoot *os.File, subpath string) (_ *os.File, _ procThread
256256
}
257257
}
258258

259-
// Grab the handle.
260-
var (
261-
handle *os.File
262-
err error
263-
)
264-
if hasOpenat2() {
265-
// We prefer being able to use RESOLVE_NO_XDEV if we can, to be
266-
// absolutely sure we are operating on a clean /proc handle that
267-
// doesn't have any cheeky overmounts that could trick us (including
268-
// symlink mounts on top of /proc/thread-self). RESOLVE_BENEATH isn't
269-
// strictly needed, but just use it since we have it.
270-
//
271-
// NOTE: /proc/self is technically a magic-link (the contents of the
272-
// symlink are generated dynamically), but it doesn't use
273-
// nd_jump_link() so RESOLVE_NO_MAGICLINKS allows it.
274-
//
275-
// NOTE: We MUST NOT use RESOLVE_IN_ROOT here, as openat2File uses
276-
// procSelfFdReadlink to clean up the returned f.Name() if we use
277-
// RESOLVE_IN_ROOT (which would lead to an infinite recursion).
278-
handle, err = openat2File(procRoot, threadSelf+subpath, &unix.OpenHow{
279-
Flags: unix.O_PATH | unix.O_NOFOLLOW | unix.O_CLOEXEC,
280-
Resolve: unix.RESOLVE_BENEATH | unix.RESOLVE_NO_XDEV | unix.RESOLVE_NO_MAGICLINKS,
281-
})
282-
if err != nil {
283-
// TODO: Once we bump the minimum Go version to 1.20, we can use
284-
// multiple %w verbs for this wrapping. For now we need to use a
285-
// compatibility shim for older Go versions.
286-
// err = fmt.Errorf("%w: %w", errUnsafeProcfs, err)
287-
return nil, nil, wrapBaseError(err, errUnsafeProcfs)
288-
}
289-
} else {
290-
handle, err = openatFile(procRoot, threadSelf+subpath, unix.O_PATH|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
291-
if err != nil {
292-
// TODO: Once we bump the minimum Go version to 1.20, we can use
293-
// multiple %w verbs for this wrapping. For now we need to use a
294-
// compatibility shim for older Go versions.
295-
// err = fmt.Errorf("%w: %w", errUnsafeProcfs, err)
296-
return nil, nil, wrapBaseError(err, errUnsafeProcfs)
297-
}
298-
defer func() {
299-
if Err != nil {
300-
_ = handle.Close()
301-
}
302-
}()
303-
// We can't detect bind-mounts of different parts of procfs on top of
304-
// /proc (a-la RESOLVE_NO_XDEV), but we can at least be sure that we
305-
// aren't on the wrong filesystem here.
306-
if statfs, err := fstatfs(handle); err != nil {
307-
return nil, nil, err
308-
} else if statfs.Type != procSuperMagic {
309-
return nil, nil, fmt.Errorf("%w: incorrect /proc/self/fd filesystem type 0x%x", errUnsafeProcfs, statfs.Type)
310-
}
259+
handle, err := procfsLookupInRoot(procRoot, threadSelf+subpath)
260+
if err != nil {
261+
// TODO: Once we bump the minimum Go version to 1.20, we can use
262+
// multiple %w verbs for this wrapping. For now we need to use a
263+
// compatibility shim for older Go versions.
264+
// err = fmt.Errorf("%w: %w", errUnsafeProcfs, err)
265+
return nil, nil, wrapBaseError(err, errUnsafeProcfs)
311266
}
312267
return handle, runtime.UnlockOSThread, nil
313268
}

procfs_linux_test.go

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -129,14 +129,10 @@ func testProcOvermountSubdir(t *testing.T, procRootFn procRootFunc, expectOvermo
129129
require.NoError(t, err)
130130
defer procRoot.Close() //nolint:errcheck // test code
131131

132-
// We expect to always detect tmpfs overmounts if we have a /proc with
133-
// overmounts.
134-
detectFdinfo := expectOvermounts
135-
testProcThreadSelf(t, procRoot, "fdinfo", detectFdinfo)
136-
// We only expect to detect procfs bind-mounts if there are /proc
137-
// overmounts and we have openat2.
138-
detectAttrCurrent := expectOvermounts && hasOpenat2()
139-
testProcThreadSelf(t, procRoot, "attr/current", detectAttrCurrent)
132+
// For both tmpfs and procfs overmounts, we should catch them (with or
133+
// without openat2, thanks to procfsLookupInRoot).
134+
testProcThreadSelf(t, procRoot, "fdinfo", expectOvermounts)
135+
testProcThreadSelf(t, procRoot, "attr/current", expectOvermounts)
140136

141137
// For magic-links we expect to detect overmounts if there are any.
142138
symlinkOvermountErr := errUnsafeProcfs

procfs_lookup_linux.go

Lines changed: 215 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,215 @@
1+
//go:build linux
2+
3+
// Copyright (C) 2024-2025 SUSE LLC. All rights reserved.
4+
// Use of this source code is governed by a BSD-style
5+
// license that can be found in the LICENSE file.
6+
7+
// This code is adapted to be a minimal version of the libpathrs proc resolver
8+
// <https://github.com/opensuse/libpathrs/blob/v0.1.3/src/resolvers/procfs.rs>.
9+
// As we only need O_PATH|O_NOFOLLOW support, this is not too much to port.
10+
11+
package securejoin
12+
13+
import (
14+
"fmt"
15+
"os"
16+
"path"
17+
"path/filepath"
18+
"strings"
19+
20+
"golang.org/x/sys/unix"
21+
)
22+
23+
// procfsLookupInRoot is a stripped down version of completeLookupInRoot,
24+
// entirely designed to support the very small set of features necessary to
25+
// make procfs handling work. Unlike completeLookupInRoot, we always have
26+
// O_PATH|O_NOFOLLOW behaviour for trailing symlinks.
27+
//
28+
// The main restrictions are:
29+
//
30+
// - ".." is not supported (as it requires either os.Root-style replays,
31+
// which is more bug-prone; or procfs verification, which is not possible
32+
// due to re-entrancy issues).
33+
// - Absolute symlinks for the same reason (and all absolute symlinks in
34+
// procfs are magic-links, which we want to skip anyway).
35+
// - If statx is supported (checkSymlinkOvermount), any mount-point crossings
36+
// (which is the main attack of concern against /proc).
37+
// - Partial lookups are not supported, so the symlink stack is not needed.
38+
// - Trailing slash special handling is not necessary in most cases (if we
39+
// operating on procfs, it's usually with programmer-controlled strings
40+
// that will then be re-opened), so we skip it since whatever re-opens it
41+
// can deal with it. It's a creature comfort anyway.
42+
//
43+
// If the system supports openat2(), this is implemented using equivalent flags
44+
// (RESOLVE_BENEATH | RESOLVE_NO_XDEV | RESOLVE_NO_MAGICLINKS).
45+
func procfsLookupInRoot(procRoot *os.File, unsafePath string) (Handle *os.File, _ error) {
46+
unsafePath = filepath.ToSlash(unsafePath) // noop
47+
48+
// Make sure that an empty unsafe path still returns something sane, even
49+
// with openat2 (which doesn't have AT_EMPTY_PATH semantics yet).
50+
if unsafePath == "" {
51+
unsafePath = "."
52+
}
53+
54+
// This is already checked by getProcRoot, but make sure here since the
55+
// core security of this lookup is based on this assumption.
56+
if err := verifyProcRoot(procRoot); err != nil {
57+
return nil, err
58+
}
59+
60+
if hasOpenat2() {
61+
// We prefer being able to use RESOLVE_NO_XDEV if we can, to be
62+
// absolutely sure we are operating on a clean /proc handle that
63+
// doesn't have any cheeky overmounts that could trick us (including
64+
// symlink mounts on top of /proc/thread-self). RESOLVE_BENEATH isn't
65+
// strictly needed, but just use it since we have it.
66+
//
67+
// NOTE: /proc/self is technically a magic-link (the contents of the
68+
// symlink are generated dynamically), but it doesn't use
69+
// nd_jump_link() so RESOLVE_NO_MAGICLINKS allows it.
70+
//
71+
// NOTE: We MUST NOT use RESOLVE_IN_ROOT here, as openat2File uses
72+
// procSelfFdReadlink to clean up the returned f.Name() if we use
73+
// RESOLVE_IN_ROOT (which would lead to an infinite recursion).
74+
//
75+
// TODO: It would be nice to have RESOLVE_NO_DOTDOT, purely for
76+
// self-consistency with the backup O_PATH resolver.
77+
handle, err := openat2File(procRoot, unsafePath, &unix.OpenHow{
78+
Flags: unix.O_PATH | unix.O_NOFOLLOW | unix.O_CLOEXEC,
79+
Resolve: unix.RESOLVE_BENEATH | unix.RESOLVE_NO_XDEV | unix.RESOLVE_NO_MAGICLINKS,
80+
})
81+
if err != nil {
82+
// TODO: Once we bump the minimum Go version to 1.20, we can use
83+
// multiple %w verbs for this wrapping. For now we need to use a
84+
// compatibility shim for older Go versions.
85+
// err = fmt.Errorf("%w: %w", errUnsafeProcfs, err)
86+
return nil, wrapBaseError(err, errUnsafeProcfs)
87+
}
88+
return handle, nil
89+
}
90+
91+
// To mirror openat2(RESOLVE_BENEATH), we need to return an error if the
92+
// path is absolute.
93+
if path.IsAbs(unsafePath) {
94+
return nil, fmt.Errorf("%w: cannot resolve absolute paths in procfs resolver", errPossibleBreakout)
95+
}
96+
97+
currentDir, err := dupFile(procRoot)
98+
if err != nil {
99+
return nil, fmt.Errorf("clone root fd: %w", err)
100+
}
101+
defer func() {
102+
// If a handle is not returned, close the internal handle.
103+
if Handle == nil {
104+
_ = currentDir.Close()
105+
}
106+
}()
107+
108+
var (
109+
linksWalked int
110+
currentPath string
111+
remainingPath = unsafePath
112+
)
113+
for remainingPath != "" {
114+
// Get the next path component.
115+
var part string
116+
if i := strings.IndexByte(remainingPath, '/'); i == -1 {
117+
part, remainingPath = remainingPath, ""
118+
} else {
119+
part, remainingPath = remainingPath[:i], remainingPath[i+1:]
120+
}
121+
if part == "" {
122+
// no-op component, but treat it the same as "."
123+
part = "."
124+
}
125+
if part == ".." {
126+
// not permitted
127+
return nil, fmt.Errorf("%w: cannot walk into '..' in procfs resolver", errPossibleBreakout)
128+
}
129+
130+
// Apply the component lexically to the path we are building.
131+
// currentPath does not contain any symlinks, and we are lexically
132+
// dealing with a single component, so it's okay to do a filepath.Clean
133+
// here. (Not to mention that ".." isn't allowed.)
134+
nextPath := path.Join("/", currentPath, part)
135+
// If we logically hit the root, just clone the root rather than
136+
// opening the part and doing all of the other checks.
137+
if nextPath == "/" {
138+
// Jump to root.
139+
rootClone, err := dupFile(procRoot)
140+
if err != nil {
141+
return nil, fmt.Errorf("clone root fd: %w", err)
142+
}
143+
_ = currentDir.Close()
144+
currentDir = rootClone
145+
currentPath = nextPath
146+
continue
147+
}
148+
149+
// Try to open the next component.
150+
nextDir, err := openatFile(currentDir, part, unix.O_PATH|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
151+
if err != nil {
152+
return nil, err
153+
}
154+
155+
// Make sure we are still on procfs and haven't crossed mounts.
156+
if err := verifyProcHandle(nextDir); err != nil {
157+
_ = nextDir.Close()
158+
return nil, fmt.Errorf("check %q component is on procfs: %w", part, err)
159+
}
160+
if err := checkSubpathOvermount(procRoot, nextDir, ""); err != nil {
161+
_ = nextDir.Close()
162+
return nil, fmt.Errorf("check %q component is not overmounted: %w", part, err)
163+
}
164+
165+
// We are emulating O_PATH|O_NOFOLLOW, so we only need to traverse into
166+
// trailing symlinks if we are not the final component. Otherwise we
167+
// can just return the currentDir.
168+
if remainingPath != "" {
169+
st, err := nextDir.Stat()
170+
if err != nil {
171+
_ = nextDir.Close()
172+
return nil, fmt.Errorf("stat component %q: %w", part, err)
173+
}
174+
175+
if st.Mode()&os.ModeType == os.ModeSymlink {
176+
// readlinkat implies AT_EMPTY_PATH since Linux 2.6.39. See
177+
// Linux commit 65cfc6722361 ("readlinkat(), fchownat() and
178+
// fstatat() with empty relative pathnames").
179+
linkDest, err := readlinkatFile(nextDir, "")
180+
// We don't need the handle anymore.
181+
_ = nextDir.Close()
182+
if err != nil {
183+
return nil, err
184+
}
185+
186+
linksWalked++
187+
if linksWalked > maxSymlinkLimit {
188+
return nil, &os.PathError{Op: "securejoin.procfsLookupInRoot", Path: "/proc/" + unsafePath, Err: unix.ELOOP}
189+
}
190+
191+
// Update our logical remaining path.
192+
remainingPath = linkDest + "/" + remainingPath
193+
// Absolute symlinks are probably magiclinks, we reject them.
194+
if path.IsAbs(linkDest) {
195+
return nil, fmt.Errorf("%w: cannot jump to / in procfs resolver -- possible magiclink", errPossibleBreakout)
196+
}
197+
continue
198+
}
199+
}
200+
201+
// Walk into the next component.
202+
_ = currentDir.Close()
203+
currentDir = nextDir
204+
currentPath = nextPath
205+
}
206+
207+
// One final sanity-check.
208+
if err := verifyProcHandle(currentDir); err != nil {
209+
return nil, fmt.Errorf("check final handle is on procfs: %w", err)
210+
}
211+
if err := checkSubpathOvermount(procRoot, currentDir, ""); err != nil {
212+
return nil, fmt.Errorf("check final handle is not overmounted: %w", err)
213+
}
214+
return currentDir, nil
215+
}

0 commit comments

Comments
 (0)