From 40f146841364bb62bb2437455a7365cdec8844ac Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Mon, 17 Sep 2018 21:38:30 +1000 Subject: [PATCH] keyring: handle ENOSYS with keyctl(KEYCTL_JOIN_SESSION_KEYRING) While all modern kernels (and I do mean _all_ of them -- this syscall was added in 2.6.10 before git had begun development!) have support for this syscall, LXC has a default seccomp profile that returns ENOSYS for this syscall. For most syscalls this would be a deal-breaker, and our use of session keyrings is security-based there are a few mitigating factors that make this change not-completely-insane: * We already have a flag that disables the use of session keyrings (for older kernels that had system-wide keyring limits and so on). So disabling it is not a new idea. * While the primary justification of using session keys *is* security-based, it's more of a security-by-obscurity protection. The main defense keyrings have is VFS credentials -- which is something that users already have better security tools for (setuid(2) and user namespaces). * Given the security justification you might argue that we shouldn't silently ignore this. However, the only way for the kernel to return -ENOSYS is either being ridiculously old (at which point we wouldn't work anyway) or that there is a seccomp profile in place blocking it. Given that the seccomp profile (if malicious) could very easily just return 0 or a silly return code (or something even more clever with seccomp-bpf) and trick us without this patch, there isn't much of a significant change in how much seccomp can trick us with or without this patch. Given all of that over-analysis, I'm pretty convinced there isn't a security problem in this very specific case and it will help out the ChromeOS folks by allowing Docker to run inside their LXC container setup. I'd be happy to be proven wrong. Ref: https://bugs.chromium.org/p/chromium/issues/detail?id=860565 Signed-off-by: Aleksa Sarai --- libcontainer/keys/keyctl.go | 4 +++- libcontainer/setns_init_linux.go | 11 +++++++++-- libcontainer/standard_init_linux.go | 26 +++++++++++++++++++------- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/libcontainer/keys/keyctl.go b/libcontainer/keys/keyctl.go index ce8b4e6b0..1522cabb7 100644 --- a/libcontainer/keys/keyctl.go +++ b/libcontainer/keys/keyctl.go @@ -7,6 +7,8 @@ import ( "strconv" "strings" + "github.com/pkg/errors" + "golang.org/x/sys/unix" ) @@ -15,7 +17,7 @@ type KeySerial uint32 func JoinSessionKeyring(name string) (KeySerial, error) { sessKeyId, err := unix.KeyctlJoinSessionKeyring(name) if err != nil { - return 0, fmt.Errorf("could not create session key: %v", err) + return 0, errors.Wrap(err, "create session key") } return KeySerial(sessKeyId), nil } diff --git a/libcontainer/setns_init_linux.go b/libcontainer/setns_init_linux.go index 096c601e7..946241f2b 100644 --- a/libcontainer/setns_init_linux.go +++ b/libcontainer/setns_init_linux.go @@ -11,6 +11,7 @@ import ( "github.com/opencontainers/runc/libcontainer/seccomp" "github.com/opencontainers/runc/libcontainer/system" "github.com/opencontainers/selinux/go-selinux/label" + "github.com/pkg/errors" "golang.org/x/sys/unix" ) @@ -29,9 +30,15 @@ func (l *linuxSetnsInit) getSessionRingName() string { func (l *linuxSetnsInit) Init() error { if !l.config.Config.NoNewKeyring { - // do not inherit the parent's session keyring + // Do not inherit the parent's session keyring. if _, err := keys.JoinSessionKeyring(l.getSessionRingName()); err != nil { - return err + // Same justification as in standart_init_linux.go as to why we + // don't bail on ENOSYS. + // + // TODO(cyphar): And we should have logging here too. + if errors.Cause(err) != unix.ENOSYS { + return errors.Wrap(err, "join session keyring") + } } } if l.config.CreateConsole { diff --git a/libcontainer/standard_init_linux.go b/libcontainer/standard_init_linux.go index 0b6530d68..f20fda7ad 100644 --- a/libcontainer/standard_init_linux.go +++ b/libcontainer/standard_init_linux.go @@ -48,13 +48,25 @@ func (l *linuxStandardInit) Init() error { ringname, keepperms, newperms := l.getSessionRingParams() // Do not inherit the parent's session keyring. - sessKeyId, err := keys.JoinSessionKeyring(ringname) - if err != nil { - return errors.Wrap(err, "join session keyring") - } - // Make session keyring searcheable. - if err := keys.ModKeyringPerm(sessKeyId, keepperms, newperms); err != nil { - return errors.Wrap(err, "mod keyring permissions") + if sessKeyId, err := keys.JoinSessionKeyring(ringname); err != nil { + // If keyrings aren't supported then it is likely we are on an + // older kernel (or inside an LXC container). While we could bail, + // the security feature we are using here is best-effort (it only + // really provides marignal protection since VFS credentials are + // the only significant protection of keyrings). + // + // TODO(cyphar): Log this so people know what's going on, once we + // have proper logging in 'runc init'. + if errors.Cause(err) != unix.ENOSYS { + return errors.Wrap(err, "join session keyring") + } + } else { + // Make session keyring searcheable. If we've gotten this far we + // bail on any error -- we don't want to have a keyring with bad + // permissions. + if err := keys.ModKeyringPerm(sessKeyId, keepperms, newperms); err != nil { + return errors.Wrap(err, "mod keyring permissions") + } } }