All of lore.kernel.org
 help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: Linux Containers <containers@lists.linux-foundation.org>
Cc: <linux-security-module@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Andy Lutomirski <luto@amacapital.net>,
	David Howells <dhowells@redhat.com>
Subject: [PATCH 1/4] Fix cap_capable to only allow owners in the parent user namespace to have caps.
Date: Fri, 14 Dec 2012 14:03:05 -0800	[thread overview]
Message-ID: <87obhwxpeu.fsf@xmission.com> (raw)
In-Reply-To: <87txroxpgq.fsf@xmission.com> (Eric W. Biederman's message of "Fri, 14 Dec 2012 14:01:57 -0800")


Andy Lutomirski pointed out that the current behavior of allowing the
owner of a user namespace to have all caps when that owner is not in a
parent user namespace is wrong.  Add a test to ensure the owner of a user
namespace is in the parent of the user namespace to fix this bug.

Thankfully this bug did not apply to the initial user namespace, keeping
the mischief that can be caused by this bug quite small.

This is bug was introduced in v3.5 by commit 783291e6900
"Simplify the user_namespace by making userns->creator a kuid."
But did not matter until the permisions required to create
a user namespace were relaxed allowing a user namespace to be created
inside of a user namespace.

The bug made it possible for the owner of a user namespace to be
present in a child user namespace.  Since the owner of a user nameapce
is granted all capabilities it became possible for users in a
grandchild user namespace to have all privilges over their parent user
namspace.

Reorder the checks in cap_capable.  This should make the common case
faster and make it clear that nothing magic happens in the initial
user namespace.  The reordering is safe because cred->user_ns
can only be in targ_ns or targ_ns->parent but not both.

Add a comment a the top of the loop to make the logic of
the code clear.

Add a distinct variable ns that changes as we walk up
the user namespace hierarchy to make it clear which variable
is changing.

Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 security/commoncap.c |   25 +++++++++++++++++--------
 1 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 6dbae46..7ee08c7 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -76,24 +76,33 @@ int cap_netlink_send(struct sock *sk, struct sk_buff *skb)
 int cap_capable(const struct cred *cred, struct user_namespace *targ_ns,
 		int cap, int audit)
 {
-	for (;;) {
-		/* The owner of the user namespace has all caps. */
-		if (targ_ns != &init_user_ns && uid_eq(targ_ns->owner, cred->euid))
-			return 0;
+	struct user_namespace *ns = targ_ns;
 
+	/* See if cred has the capability in the target user namespace
+	 * by examining the target user namespace and all of the target
+	 * user namespace's parents.
+	 */
+	for (;;) {
 		/* Do we have the necessary capabilities? */
-		if (targ_ns == cred->user_ns)
+		if (ns == cred->user_ns)
 			return cap_raised(cred->cap_effective, cap) ? 0 : -EPERM;
 
 		/* Have we tried all of the parent namespaces? */
-		if (targ_ns == &init_user_ns)
+		if (ns == &init_user_ns)
 			return -EPERM;
 
+		/* 
+		 * The owner of the user namespace in the parent of the
+		 * user namespace has all caps.
+		 */
+		if ((ns->parent == cred->user_ns) && uid_eq(ns->owner, cred->euid))
+			return 0;
+
 		/*
-		 *If you have a capability in a parent user ns, then you have
+		 * If you have a capability in a parent user ns, then you have
 		 * it over all children user namespaces as well.
 		 */
-		targ_ns = targ_ns->parent;
+		ns = ns->parent;
 	}
 
 	/* We never get here */
-- 
1.7.5.4


  reply	other threads:[~2012-12-14 22:03 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-12-14 22:01 [PATCH 0/4] user namespace fixes Eric W. Biederman
2012-12-14 22:01 ` Eric W. Biederman
2012-12-14 22:03 ` Eric W. Biederman [this message]
2012-12-14 22:03 ` [PATCH 2/4] userns: Require CAP_SYS_ADMIN for most uses of setns Eric W. Biederman
     [not found]   ` <87hanoxpdh.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
2012-12-14 23:35     ` Serge E. Hallyn
2012-12-14 23:35       ` Serge E. Hallyn
2012-12-17 19:03     ` Andy Lutomirski
2012-12-17 19:03   ` Andy Lutomirski
2012-12-14 22:04 ` [PATCH 3/4] userns: Add a more complete capability subset test to commit_creds Eric W. Biederman
2012-12-15  0:03   ` Serge E. Hallyn
2012-12-15  0:11     ` Eric W. Biederman
     [not found]       ` <87r4msrx6t.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
2012-12-15  0:47         ` Serge E. Hallyn
2012-12-15  0:47           ` Serge E. Hallyn
     [not found]           ` <20121215004735.GA14295-7LNsyQBKDXoIagZqoN9o3w@public.gmane.org>
2012-12-15  0:48             ` Eric W. Biederman
2012-12-15  0:48               ` Eric W. Biederman
2012-12-15  2:06               ` Serge E. Hallyn
2012-12-17 19:08               ` Andy Lutomirski
     [not found]               ` <87lid0rvh9.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
2012-12-15  2:06                 ` Serge E. Hallyn
2012-12-17 19:08                 ` Andy Lutomirski
     [not found]     ` <20121215000338.GC13659-7LNsyQBKDXoIagZqoN9o3w@public.gmane.org>
2012-12-15  0:11       ` Eric W. Biederman
     [not found]   ` <87bodwxpcg.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
2012-12-15  0:03     ` Serge E. Hallyn
     [not found] ` <87txroxpgq.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
2012-12-14 22:03   ` [PATCH 1/4] Fix cap_capable to only allow owners in the parent user namespace to have caps Eric W. Biederman
2012-12-14 22:03   ` [PATCH 2/4] userns: Require CAP_SYS_ADMIN for most uses of setns Eric W. Biederman
2012-12-14 22:04   ` [PATCH 3/4] userns: Add a more complete capability subset test to commit_creds Eric W. Biederman
2012-12-14 22:05   ` [PATCH 4/4] userns: Fix typo in description of the limitation of userns_install Eric W. Biederman
2012-12-14 22:05     ` Eric W. Biederman
     [not found]     ` <876244xpbj.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
2012-12-14 23:36       ` Serge E. Hallyn
2012-12-17 19:08       ` Andy Lutomirski
2012-12-17 19:08         ` Andy Lutomirski
2012-12-14 23:36     ` Serge E. Hallyn
2012-12-17 19:03   ` [PATCH 0/4] user namespace fixes Andy Lutomirski
2012-12-17 19:03     ` Andy Lutomirski
     [not found]     ` <CALCETrX2Fa-DuM+wkgsij7oiJXzCD8W6Phkv-MjgCDg_Ma6CTg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-12-17 21:01       ` Eric W. Biederman
2012-12-17 21:01     ` Eric W. Biederman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87obhwxpeu.fsf@xmission.com \
    --to=ebiederm@xmission.com \
    --cc=containers@lists.linux-foundation.org \
    --cc=dhowells@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=serge@hallyn.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.