rootfs: umount all procfs and sysfs with --no-pivot by giuseppe · Pull Request #1962 · opencontainers/runc

giuseppe · 2019-01-11T21:18:07Z

When creating a new user namespace, the kernel doesn't allow to mount
a new procfs or sysfs file system if there is not already one instance
fully visible in the current mount namespace.

When using --no-pivot we were effectively inhibiting this protection
from the kernel, as /proc and /sys from the host are still present in
the container mount namespace.

A container without full access to /proc could then create a new user
namespace, and from there able to mount a fully visible /proc, bypassing
the limitations in the container.

A simple reproducer for this issue is:

unshare -mrfp sh -c "mount -t proc none /proc && echo c > /proc/sysrq-trigger"

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

AkihiroSuda

LGTM (IANAM)

mrunalp · 2019-01-12T01:30:52Z

LGTM

AkihiroSuda · 2019-01-12T02:01:13Z

+			return err
+		}
+		if err := unix.Unmount(p, unix.MNT_DETACH); err != nil {
+			if err.(syscall.Errno) != unix.EINVAL {


Potentially we can also get EPERM here?

I guess, but I never saw that error message, when trying to umount /proc or /sys I got only EINVAL. I can amend the patch if you'd like

I went forward and amended the change in the updated version

When creating a new user namespace, the kernel doesn't allow to mount a new procfs or sysfs file system if there is not already one instance fully visible in the current mount namespace. When using --no-pivot we were effectively inhibiting this protection from the kernel, as /proc and /sys from the host are still present in the container mount namespace. A container without full access to /proc could then create a new user namespace, and from there able to mount a fully visible /proc, bypassing the limitations in the container. A simple reproducer for this issue is: unshare -mrfp sh -c "mount -t proc none /proc && echo c > /proc/sysrq-trigger" Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

mrunalp · 2019-01-14T15:34:37Z

LGTM

cyphar · 2019-01-15T04:15:13Z

LGTM.

@mrunalp

rootfs: umount all procfs and sysfs with --no-pivot LGTMs: @mrunalp @cyphar Closes #1962

Changes: opencontainers/runc@96ec217...12f6a99 Including critical security fix for `runc run --no-pivot` (`DOCKER_RAMDISK=1`): opencontainers/runc#1962 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

Including critical security fix for `runc run --no-pivot` (unlikely to affect BuildKit): opencontainers/runc#1962 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

Changes: opencontainers/runc@96ec217...12f6a99 Including critical security fix for `runc run --no-pivot` (`DOCKER_RAMDISK=1`): opencontainers/runc#1962 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

Changes: opencontainers/runc@96ec217...12f6a99 Including critical security fix for `runc run --no-pivot` (`DOCKER_RAMDISK=1`): opencontainers/runc#1962 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp> (cherry picked from commit 3aec9e7)

Changes: opencontainers/runc@96ec217...12f6a99 Including critical security fix for `runc run --no-pivot` (`DOCKER_RAMDISK=1`): opencontainers/runc#1962 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp> (cherry picked from commit 1ee33f4)

Changes: opencontainers/runc@96ec217...12f6a99 Including critical security fix for `runc run --no-pivot` (`DOCKER_RAMDISK=1`): opencontainers/runc#1962 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp> (cherry picked from commit 3aec9e7) Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

Changes: opencontainers/runc@69663f0...12f6a99 Including critical security fix for `runc run --no-pivot` (`DOCKER_RAMDISK=1`): opencontainers/runc#1962 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

Changes: opencontainers/runc@96ec217...12f6a99 Including critical security fix for `runc run --no-pivot` (`DOCKER_RAMDISK=1`): opencontainers/runc#1962 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

kolyshkin · 2020-03-13T01:41:47Z

+		return err
+	}
+
+	absRootfs, err := filepath.Abs(rootfs)


AFAICS this is not needed since rootfs is already validated by (*ConfigValidator).rootfs()

kolyshkin · 2020-03-13T01:43:13Z

+	}
+
+	for _, info := range mountinfos {
+		p, err := filepath.Abs(info.Mountpoint)


not needed either as paths returned from the kernel are absolute and clean (unless I'm missing something).

I have addressed this in #2255 (a separate commit to keep things simple), PTAL @giuseppe

AkihiroSuda approved these changes Jan 12, 2019

View reviewed changes

AkihiroSuda reviewed Jan 12, 2019

View reviewed changes

giuseppe force-pushed the no-pivot-umount-proc-sys branch from 5d8596f to c18aa18 Compare January 12, 2019 21:19

cyphar reviewed Jan 14, 2019

View reviewed changes

Comment thread libcontainer/rootfs_linux.go Outdated

giuseppe force-pushed the no-pivot-umount-proc-sys branch from c18aa18 to 28a697c Compare January 14, 2019 08:53

AkihiroSuda approved these changes Jan 15, 2019

View reviewed changes

cyphar merged commit 28a697c into opencontainers:master Jan 15, 2019

cyphar added a commit that referenced this pull request Jan 15, 2019

merge branch 'pr-1962'

12f6a99

rootfs: umount all procfs and sysfs with --no-pivot LGTMs: @mrunalp @cyphar Closes #1962

AkihiroSuda mentioned this pull request Jan 15, 2019

bump up runc moby/moby#38573

Closed

AkihiroSuda added a commit to AkihiroSuda/buildkit_poc that referenced this pull request Jan 15, 2019

bump up runc

b425827

Including critical security fix for `runc run --no-pivot` (unlikely to affect BuildKit): opencontainers/runc#1962 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda mentioned this pull request Jan 15, 2019

bump up runc moby/buildkit#782

Merged

AkihiroSuda mentioned this pull request Jan 15, 2019

bump up runc containerd/containerd#2931

Merged

AkihiroSuda mentioned this pull request Jan 15, 2019

[release/1.2] bump up runc containerd/containerd#2932

Merged

AkihiroSuda mentioned this pull request Jan 15, 2019

[release/1.1] bump up runc containerd/containerd#2933

Merged

AkihiroSuda mentioned this pull request Jan 15, 2019

[18.09] backport #38573 "bump up runc" docker-archive/engine#145

Closed

AkihiroSuda mentioned this pull request Jan 15, 2019

[18.06] backport #38573 "bump up runc" docker-archive/engine#146

Closed

tstromberg added a commit to tstromberg/minikube that referenced this pull request Jan 16, 2019

Pull in opencontainers/runc#1962 by updating to runc HEAD

0219c67

tstromberg mentioned this pull request Jan 16, 2019

Update to opencontainers/runc HEAD as of 2019-01-15 kubernetes/minikube#3535

Merged

giuseppe mentioned this pull request Jul 25, 2019

no-pivot option? containers/crun#56

Closed

kolyshkin reviewed Mar 13, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rootfs: umount all procfs and sysfs with --no-pivot#1962

rootfs: umount all procfs and sysfs with --no-pivot#1962
cyphar merged 1 commit into
opencontainers:masterfrom
giuseppe:no-pivot-umount-proc-sys

giuseppe commented Jan 11, 2019

Uh oh!

AkihiroSuda left a comment

Uh oh!

mrunalp commented Jan 12, 2019 •

edited by caniszczyk

Loading

Uh oh!

AkihiroSuda Jan 12, 2019

Uh oh!

giuseppe Jan 12, 2019

Uh oh!

giuseppe Jan 12, 2019

Uh oh!

Uh oh!

mrunalp commented Jan 14, 2019 •

edited by caniszczyk

Loading

Uh oh!

cyphar commented Jan 15, 2019 •

edited by caniszczyk

Loading

Uh oh!

kolyshkin Mar 13, 2020

Uh oh!

kolyshkin Mar 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

giuseppe commented Jan 11, 2019

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

mrunalp commented Jan 12, 2019 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda Jan 12, 2019

Choose a reason for hiding this comment

Uh oh!

giuseppe Jan 12, 2019

Choose a reason for hiding this comment

Uh oh!

giuseppe Jan 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrunalp commented Jan 14, 2019 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyphar commented Jan 15, 2019 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kolyshkin Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

kolyshkin Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mrunalp commented Jan 12, 2019 •

edited by caniszczyk

Loading

mrunalp commented Jan 14, 2019 •

edited by caniszczyk

Loading

cyphar commented Jan 15, 2019 •

edited by caniszczyk

Loading