[release-1.51] Backport CVE-2024-9676 fix #2146
Conversation
fix the detection for the maximum userns size from an image. If the maximum ID used in an image is X, we need to use a user namespace with size X+1 to include UID=X. Closes: containers#2104 Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
the alpine image defines a "nogroup": $ podman run --rm alpine grep nogroup /etc/group nogroup:x:65533: ignore it as we are already doing for the "nobody" user. Signed-off-by: Giuseppe Scrivano <[email protected]>
|
Too old securejoin to have the new API, reverting to the old one |
mheon
force-pushed
the
backport_2024_9676_release151
branch
from
872d5b6 to
67270e0
Compare
userns.go
Outdated
| func secureOpen(containerMount, file string) (*os.File, error) { | ||
| finalPath, err := securejoin.SecureJoin(containerMount, file) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return os.Open(finalPath) | ||
| } |
There was a problem hiding this comment.
@mheon, would it be possible to use a similar code to the one from:
Just to make things consistent.
There was a problem hiding this comment.
FYI, this is not sufficient if there is a risk of a racing process changing the filesystem from underneath you. You should instead do something like (untested):
func somewhatSafeOpen(root, unsafePath string, mode int) (_ *os.File, Err error) {
if mode&(os.O_CREAT|os.O_TRUNC) != 0 {
return nil, fmt.Errorf("O_CREAT|O_TRUNC cannot be used safely")
}
path, err := securejoin.SecureJoin(root, unsafePath)
if err != nil {
return nil, err
}
handle, err := os.OpenFile(path, mode)
if err != nil {
return nil, err
}
defer func() {
if Err != nil {
_ = handle.Close()
}
}()
// At this point, we aren't sure that the handle we grabbed is the same
// path we expected. So double-check it using procfs.
actualPath, err := os.Readlink(fmt.Sprintf("/proc/self/fd/%d", handle.Fd()))
if err != nil {
return nil, fmt.Errorf("failed to check handle path for %q: %w", path, err)
}
if path != actualPath {
return nil, fmt.Errorf("container breakout detected: tried to open %q but instead opened %q", path, actualPath)
}
return handle, nil
}We need to read /etc/passwd and /etc/group in the container to get an idea of how many UIDs and GIDs we need to allocate for a user namespace when `--userns=auto` is specified. We were forming paths for these using filepath.Join, which is not safe for paths within a container, resulting in this CVE allowing crafted symlinks in the container to access paths on the host instead. Cherry-pick conflict fixed for v1.51 branch, and converted to use the old securejoin API (securejoin.SecureJoin and then os.Open) as this branch is too old to have the new API. Addresses CVE-2024-9676 Signed-off-by: Matt Heon <[email protected]>
mheon
force-pushed
the
backport_2024_9676_release151
branch
from
67270e0 to
0dc4fc9
Compare
Signed-off-by: Matt Heon <[email protected]>
|
/approve |
|
@kwilczynski: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
LGTM |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kwilczynski, mheon, TomSweeneyRedHat The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This backports #2134 to release-1.51 (with some bugfix patches from Giuseppe to make it apply cleanly). release-1.51 is used by Podman 4.9.x, which we are still supported on F39 for a couple more months.