Skip to content

Additional user namespace checks #1649

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.

Sign up for GitHub

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

Closed
fntlnz wants to merge 1 commit into from
+23 −0
Closed

Additional user namespace checks #1649

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions libcontainer/configs/validate/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ package validate

import (
"fmt"
"io/ioutil"
"os"
"path/filepath"
"strings"
"syscall"

"github.com/opencontainers/runc/libcontainer/configs"
"github.com/opencontainers/runc/libcontainer/intelrdt"
Expand Down Expand Up @@ -108,6 +110,27 @@ func (v *ConfigValidator) usernamespace(config *configs.Config) error {
if _, err := os.Stat("/proc/self/ns/user"); os.IsNotExist(err) {
return fmt.Errorf("USER namespaces aren't enabled in the kernel")
}

root, err := os.Stat("/")
if err != nil {
return fmt.Errorf("Cannot stat root to check if the process is inside a chroot")
}

// Verify that the current process is not inside a chroot,
// the kernel does not allow creating a new user namespace in a chroot.
// for information, see: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/user_namespace.c
if root.Sys().(*syscall.Stat_t).Ino != 2 {
Copy link
Copy Markdown
Member

@cyphar cyphar Nov 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

From memory, this check will break if you're trying to nest containers or have any other form of weird rootfs mount structure set up.

Also, I just checked and on my system and / appears to have an inode number of 256 with btrfs:

% stat /
  File: /
  Size: 218             Blocks: 0          IO Block: 4096   directory
Device: 2dh/45d Inode: 256         Links: 1
Access: (0755/drwxr-xr-x)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2017-11-12 07:05:28.393935000 +1100
Modify: 2017-04-27 22:57:13.841679200 +1000
Change: 2017-04-27 22:57:13.841679200 +1000
 Birth: -

And I believe that XFS has a root inode of 128.

Copy link
Copy Markdown
Member

@cyphar cyphar Nov 13, 2017

Choose a reason for hiding this comment

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

Oh, also this trick won't be able to detect if you have mounted a filesystem onto /jail and then done chroot /jail -- because then the inode number will still be the same.

There are some ways to detect chroot "sort of", using a combination of /proc/$pid/root and /proc/1/root, or /proc/1/mountinfo. But unfortunately they're all very iffy and don't handle all of the corner cases. It would be ideal if the kernel exposed current_chrooted to userspace but I imagine that's not likely to happen. Another possibility is to propose that we should be able to introspect the root of /proc/$pid/ns/mnt -- something that Eric is more likely to consider (but passing paths around is generally not a good idea especially since the kernel likes to changes paths to / if it feels that something fishy is going on).

Copy link
Copy Markdown
Contributor Author

@fntlnz fntlnz Nov 13, 2017

Choose a reason for hiding this comment

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

@cyphar Tried the pivot_root approach however doesn't seem to be very reliable. Thanks for taking the time to look at this

return fmt.Errorf("USER namespaces can't be created inside a chroot")
}

// On debian kernels there is specific patch to add a sysctl entry to enable user namespaces since
// the kernel is compiled without user namespaces by default.
// See: kernel.unprivileged_userns_clone
unc, err := ioutil.ReadFile("/proc/sys/kernel/unprivileged_userns_clone")
if err == nil && unc[0] == '0' && os.Getuid() != 0 {
Copy link
Copy Markdown
Member

@cyphar cyphar Nov 13, 2017

Choose a reason for hiding this comment

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

I'm not a huge fan of distro-specific code (especially since Red Hat have a similar patch but it has different semantics and they also have some other patches as well). However, this is also not correct if the system has /usr/bin/newuidmap and /usr/bin/newgidmap set up -- see #1529.

Copy link
Copy Markdown
Contributor Author

@fntlnz fntlnz Nov 13, 2017

Choose a reason for hiding this comment

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

Agree

return fmt.Errorf("Unprivileged USER namespaces are disabled in this debian kernel, to enable, toggle the sysctl option kernel.unprivileged_userns_clone to 1")
}

} else {
if config.UidMappings != nil || config.GidMappings != nil {
return fmt.Errorf("User namespace mappings specified, but USER namespace isn't enabled in the config")
Expand Down