Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

internal: proc: do not join the process user namespace #92

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

cyphar
Copy link
Contributor

@cyphar cyphar commented Jan 11, 2022

The only reason we joined the process user namespace was to map a
handful of fields into the same usernamepsace as that process. This
procedure can be implemented entirely in Go without having to run code
inside the container.

In addition, since psgo is used inside "podman top", we were actually
executing the nsenter binary from the container without all of the
container's security profiles applied. At the very least this would
allow a container process to return bad data to psgo (possibly confusing
management scripts using psgo) and at the very worst it would allow the
container process to escalate privileges by getting podman to execute
code without all of the container security profiles applied.

See containers/podman#10941.
Signed-off-by: Aleksa Sarai [email protected]

@cyphar
Copy link
Contributor Author

cyphar commented Jan 11, 2022

I'm not quite sure how to test whether this works -- the sample binary doesn't seem to do anything with huser/hgroup/hgroups even on HEAD.

@rhatdan
Copy link
Member

rhatdan commented Jan 11, 2022

If you vendor this into Podman you should see

$ podman run -d alpine top
$ podman top -l huser hgroup user group
HUSER       HGROUP      USER        GROUP
3267        3267        root        root

@cyphar
Copy link
Contributor Author

cyphar commented Jan 11, 2022

Yup, with this patch applied you get the right results and you can run it on containers that don't have nsenter installed:

% ./bin/podman run --userns=auto -it -u root registry.opensuse.org/opensuse/tumbleweed:latest bash
% ./bin/podman top -l huser hgroup user group
HUSER       HGROUP      USER        GROUP
200001024   200001024   root        root

@rhatdan
Copy link
Member

rhatdan commented Jan 11, 2022

LGTM
@vrothberg PTAL

@rhatdan
Copy link
Member

rhatdan commented Jan 11, 2022

@giuseppe PTAL

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

go.mod Outdated Show resolved Hide resolved
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minimal nits. Excellent work, @cyphar, thanks a lot for tackling that!

internal/proc/status.go Outdated Show resolved Hide resolved
The only reason we joined the process user namespace was to map a
handful of fields into the same usernamepsace as that process. This
procedure can be implemented entirely in Go without having to run code
inside the container.

In addition, since psgo is used inside "podman top", we were actually
executing the nsenter binary *from the container* without all of the
container's security profiles applied. At the very least this would
allow a container process to return bad data to psgo (possibly confusing
management scripts using psgo) and at the very worst it would allow the
container process to escalate privileges by getting podman to execute
code without all of the container security profiles applied.

Signed-off-by: Aleksa Sarai <[email protected]>
@cyphar cyphar force-pushed the proc-do-not-join-ns branch from d67bb38 to d9467da Compare January 12, 2022 09:08
@cyphar
Copy link
Contributor Author

cyphar commented Jan 12, 2022

PTAL @vrothberg, I've switch to containers/storage/pkg/idtools and removed init().

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @cyphar.

@giuseppe @rhatdan PTAL

We need to cut a new c/storage before cutting a new c/psgo.

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@giuseppe
Copy link
Member

We need to cut a new c/storage before cutting a new c/psgo.

c/storage is currently broken (by me!), waiting for valyala/gozstd#41

we can temporarily revert these patches:

  • 812c8bd316aa1788980c38e345a01354dc615ab4
  • 5bb6d8e65ed440670082af4807d70088a93b945b
  • a3abf19ed46fcbed400d6909b18eb01f09751b65
  • 7ac0e7bfff3a88ce711b7744db62c1646131468b

and I'll add them back once valyala/gozstd is fixed

@giuseppe giuseppe merged commit 003d03f into containers:main Jan 12, 2022
@vrothberg
Copy link
Member

@giuseppe, thanks! I am OK to wait; we still have until late next week.

@cyphar cyphar deleted the proc-do-not-join-ns branch January 12, 2022 12:20
@cyphar
Copy link
Contributor Author

cyphar commented Jan 12, 2022

I noticed when vendoring storage there appears to have been another breakage with podman -- podman seems to default to a 1024-long user namespace which causes issues with the newest version of containers/storage (you get an error about unmappable users -- this is fixed by bumping up the size).

@nickthetait
Copy link

Have releases of psgo been made that include this fix? What version numbers?

lsm5 added a commit to lsm5/podman that referenced this pull request Apr 13, 2022
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.

5 participants