[PATCH] userns,pidns: Verify the userns for new pid namespaces

[PATCH] userns,pidns: Verify the userns for new pid namespaces

[Eric W. Biederman]

It is pointless and confusing to allow a pid namespace hierarchy and
the user namespace hierarchy to get out of sync.  The owner of a child
pid namespace should be the owner of the parent pid namespace or
a descendant of the owner of the parent pid namespace.

Otherwise it is possible to construct scenarios where it is legal to
do something in a parent pid namespace but in a child pid namespace.

It requires use of setns into a pid namespace (but not into a user
namespace) to create such a scenario.

Add the function in_userns to help in making this determination.

Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---

While review a patch from Kiril Tkhai I realized we were missing this
sanity check....

 include/linux/user_namespace.h |  8 +++++++-
 kernel/pid_namespace.c         |  4 ++++
 kernel/user_namespace.c        | 18 ++++++++++++------
 3 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 32354b4b4b2b..497ed50004db 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -112,8 +112,9 @@ extern ssize_t proc_projid_map_write(struct file *, const char __user *, size_t,
 extern ssize_t proc_setgroups_write(struct file *, const char __user *, size_t, loff_t *);
 extern int proc_setgroups_show(struct seq_file *m, void *v);
 extern bool userns_may_setgroups(const struct user_namespace *ns);
+extern bool in_userns(const struct user_namespace *ancestor,
+		       const struct user_namespace *child);
 extern bool current_in_userns(const struct user_namespace *target_ns);
-
 struct ns_common *ns_get_owner(struct ns_common *ns);
 #else
 
@@ -144,6 +145,11 @@ static inline bool userns_may_setgroups(const struct user_namespace *ns)
 	return true;
 }
 
+static inline bool in_userns(const struct user_namespace *target_ns)
+{
+	return true;
+}
+
 static inline bool current_in_userns(const struct user_namespace *target_ns)
 {
 	return true;
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index de461aa0bf9a..749147f5a613 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -101,6 +101,10 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
 	int i;
 	int err;
 
+	err = -EINVAL;
+	if (!in_userns(parent_pid_ns->user_ns, user_ns))
+		goto out;
+
 	err = -ENOSPC;
 	if (level > MAX_PID_NS_LEVEL)
 		goto out;
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 2f735cbe05e8..7d8658fbabc8 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -986,19 +986,25 @@ bool userns_may_setgroups(const struct user_namespace *ns)
 }
 
 /*
- * Returns true if @ns is the same namespace as or a descendant of
- * @target_ns.
+ * Returns true if @child is the same namespace or a descendant of
+ * @ancestor.
  */
-bool current_in_userns(const struct user_namespace *target_ns)
+bool in_userns(const struct user_namespace *ancestor,
+	       const struct user_namespace *child)
 {
-	struct user_namespace *ns;
-	for (ns = current_user_ns(); ns; ns = ns->parent) {
-		if (ns == target_ns)
+	const struct user_namespace *ns;
+	for (ns = child; ns; ns = ns->parent) {
+		if (ns == ancestor)
 			return true;
 	}
 	return false;
 }
 
+bool current_in_userns(const struct user_namespace *target_ns)
+{
+	return in_userns(target_ns, current_user_ns());
+}
+
 static inline struct user_namespace *to_user_ns(struct ns_common *ns)
 {
 	return container_of(ns, struct user_namespace, ns);
-- 
2.10.1

Re: [PATCH] userns,pidns: Verify the userns for new pid namespaces

[Eric W. Biederman]

"Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:

> Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
>> 
>> It is pointless and confusing to allow a pid namespace hierarchy and
>> the user namespace hierarchy to get out of sync.  The owner of a child
>> pid namespace should be the owner of the parent pid namespace or
>> a descendant of the owner of the parent pid namespace.
>> 
>> Otherwise it is possible to construct scenarios where it is legal to
>> do something in a parent pid namespace but in a child pid namespace.
>
> Hi,
>
> did you mean 'but not in a child...' above?

Actually I believe I meant:

>> Otherwise it is possible to construct scenarios where it is not legal
>> to do something in a parent pid namespace but it is legal a child pid
>> namespace.

I definitely need to fix that wording thank you.

Eric

Re: [PATCH] userns,pidns: Verify the userns for new pid namespaces

[Eric W. Biederman]

ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman) writes:

> "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:
>
>> Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
>>> 
>>> It is pointless and confusing to allow a pid namespace hierarchy and
>>> the user namespace hierarchy to get out of sync.  The owner of a child
>>> pid namespace should be the owner of the parent pid namespace or
>>> a descendant of the owner of the parent pid namespace.
>>> 
>>> Otherwise it is possible to construct scenarios where it is legal to
>>> do something in a parent pid namespace but in a child pid namespace.
>>
>> Hi,
>>
>> did you mean 'but not in a child...' above?
>
> Actually I believe I meant:
>
>>> Otherwise it is possible to construct scenarios where it is not legal
>>> to do something in a parent pid namespace but it is legal a child pid
>>> namespace.
>
> I definitely need to fix that wording thank you.

Looking at some more I mean:

Otherwise it is possible to construct scenarios where a process has a
capability in a over a parent pid namespace but does not have the
capability over a child pid namespace.  Which confusingly makes
permission checks non-transitive.


Eric

Re: [PATCH] userns,pidns: Verify the userns for new pid namespaces

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
> 
> It is pointless and confusing to allow a pid namespace hierarchy and
> the user namespace hierarchy to get out of sync.  The owner of a child
> pid namespace should be the owner of the parent pid namespace or
> a descendant of the owner of the parent pid namespace.
> 
> Otherwise it is possible to construct scenarios where it is legal to
> do something in a parent pid namespace but in a child pid namespace.

Hi,

did you mean 'but not in a child...' above?

> It requires use of setns into a pid namespace (but not into a user
> namespace) to create such a scenario.
> 
> Add the function in_userns to help in making this determination.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> ---
> 
> While review a patch from Kiril Tkhai I realized we were missing this
> sanity check....
> 
>  include/linux/user_namespace.h |  8 +++++++-
>  kernel/pid_namespace.c         |  4 ++++
>  kernel/user_namespace.c        | 18 ++++++++++++------
>  3 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index 32354b4b4b2b..497ed50004db 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -112,8 +112,9 @@ extern ssize_t proc_projid_map_write(struct file *, const char __user *, size_t,
>  extern ssize_t proc_setgroups_write(struct file *, const char __user *, size_t, loff_t *);
>  extern int proc_setgroups_show(struct seq_file *m, void *v);
>  extern bool userns_may_setgroups(const struct user_namespace *ns);
> +extern bool in_userns(const struct user_namespace *ancestor,
> +		       const struct user_namespace *child);
>  extern bool current_in_userns(const struct user_namespace *target_ns);
> -
>  struct ns_common *ns_get_owner(struct ns_common *ns);
>  #else
>  
> @@ -144,6 +145,11 @@ static inline bool userns_may_setgroups(const struct user_namespace *ns)
>  	return true;
>  }
>  
> +static inline bool in_userns(const struct user_namespace *target_ns)
> +{
> +	return true;
> +}
> +
>  static inline bool current_in_userns(const struct user_namespace *target_ns)
>  {
>  	return true;
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index de461aa0bf9a..749147f5a613 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -101,6 +101,10 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
>  	int i;
>  	int err;
>  
> +	err = -EINVAL;
> +	if (!in_userns(parent_pid_ns->user_ns, user_ns))
> +		goto out;
> +
>  	err = -ENOSPC;
>  	if (level > MAX_PID_NS_LEVEL)
>  		goto out;
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 2f735cbe05e8..7d8658fbabc8 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -986,19 +986,25 @@ bool userns_may_setgroups(const struct user_namespace *ns)
>  }
>  
>  /*
> - * Returns true if @ns is the same namespace as or a descendant of
> - * @target_ns.
> + * Returns true if @child is the same namespace or a descendant of
> + * @ancestor.
>   */
> -bool current_in_userns(const struct user_namespace *target_ns)
> +bool in_userns(const struct user_namespace *ancestor,
> +	       const struct user_namespace *child)
>  {
> -	struct user_namespace *ns;
> -	for (ns = current_user_ns(); ns; ns = ns->parent) {
> -		if (ns == target_ns)
> +	const struct user_namespace *ns;
> +	for (ns = child; ns; ns = ns->parent) {
> +		if (ns == ancestor)
>  			return true;
>  	}
>  	return false;
>  }
>  
> +bool current_in_userns(const struct user_namespace *target_ns)
> +{
> +	return in_userns(target_ns, current_user_ns());
> +}
> +
>  static inline struct user_namespace *to_user_ns(struct ns_common *ns)
>  {
>  	return container_of(ns, struct user_namespace, ns);
> -- 
> 2.10.1

Re: [PATCH] userns,pidns: Verify the userns for new pid namespaces

[Kirill Tkhai]

On 29.04.2017 22:25, Eric W. Biederman wrote:
> 
> It is pointless and confusing to allow a pid namespace hierarchy and
> the user namespace hierarchy to get out of sync.  The owner of a child
> pid namespace should be the owner of the parent pid namespace or
> a descendant of the owner of the parent pid namespace.
> 
> Otherwise it is possible to construct scenarios where it is legal to
> do something in a parent pid namespace but in a child pid namespace.
> 
> It requires use of setns into a pid namespace (but not into a user
> namespace) to create such a scenario.
> 
> Add the function in_userns to help in making this determination.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> ---
> 
> While review a patch from Kiril Tkhai I realized we were missing this
> sanity check....
> 
>  include/linux/user_namespace.h |  8 +++++++-
>  kernel/pid_namespace.c         |  4 ++++
>  kernel/user_namespace.c        | 18 ++++++++++++------
>  3 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index 32354b4b4b2b..497ed50004db 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -112,8 +112,9 @@ extern ssize_t proc_projid_map_write(struct file *, const char __user *, size_t,
>  extern ssize_t proc_setgroups_write(struct file *, const char __user *, size_t, loff_t *);
>  extern int proc_setgroups_show(struct seq_file *m, void *v);
>  extern bool userns_may_setgroups(const struct user_namespace *ns);
> +extern bool in_userns(const struct user_namespace *ancestor,
> +		       const struct user_namespace *child);
>  extern bool current_in_userns(const struct user_namespace *target_ns);
> -
>  struct ns_common *ns_get_owner(struct ns_common *ns);
>  #else
>  
> @@ -144,6 +145,11 @@ static inline bool userns_may_setgroups(const struct user_namespace *ns)
>  	return true;
>  }
>  
> +static inline bool in_userns(const struct user_namespace *target_ns)
> +{
> +	return true;
> +}
> +
>  static inline bool current_in_userns(const struct user_namespace *target_ns)
>  {
>  	return true;
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index de461aa0bf9a..749147f5a613 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -101,6 +101,10 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
>  	int i;
>  	int err;
>  
> +	err = -EINVAL;
> +	if (!in_userns(parent_pid_ns->user_ns, user_ns))
> +		goto out;
> +
>  	err = -ENOSPC;
>  	if (level > MAX_PID_NS_LEVEL)
>  		goto out;
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 2f735cbe05e8..7d8658fbabc8 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -986,19 +986,25 @@ bool userns_may_setgroups(const struct user_namespace *ns)
>  }
>  
>  /*
> - * Returns true if @ns is the same namespace as or a descendant of
> - * @target_ns.
> + * Returns true if @child is the same namespace or a descendant of
> + * @ancestor.
>   */
> -bool current_in_userns(const struct user_namespace *target_ns)
> +bool in_userns(const struct user_namespace *ancestor,
> +	       const struct user_namespace *child)
>  {
> -	struct user_namespace *ns;
> -	for (ns = current_user_ns(); ns; ns = ns->parent) {
> -		if (ns == target_ns)
> +	const struct user_namespace *ns;
> +	for (ns = child; ns; ns = ns->parent) {
> +		if (ns == ancestor)
>  			return true;
>  	}
>  	return false;
>  }

We have user_namespace::level, so it's possible to stop iterations earlier
and save some cpu cycles:

	for (ns = child; ns->level >= ancestor->level; ns = ns->parent)
		;
	return (ns == ancestor);

>  
> +bool current_in_userns(const struct user_namespace *target_ns)
> +{
> +	return in_userns(target_ns, current_user_ns());
> +}
> +
>  static inline struct user_namespace *to_user_ns(struct ns_common *ns)
>  {
>  	return container_of(ns, struct user_namespace, ns);
> 

Re: [PATCH] userns,pidns: Verify the userns for new pid namespaces

[Kirill Tkhai]

On 02.05.2017 13:03, Kirill Tkhai wrote:
> 
> 
> On 29.04.2017 22:25, Eric W. Biederman wrote:
>>
>> It is pointless and confusing to allow a pid namespace hierarchy and
>> the user namespace hierarchy to get out of sync.  The owner of a child
>> pid namespace should be the owner of the parent pid namespace or
>> a descendant of the owner of the parent pid namespace.
>>
>> Otherwise it is possible to construct scenarios where it is legal to
>> do something in a parent pid namespace but in a child pid namespace.
>>
>> It requires use of setns into a pid namespace (but not into a user
>> namespace) to create such a scenario.
>>
>> Add the function in_userns to help in making this determination.
>>
>> Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
>> ---
>>
>> While review a patch from Kiril Tkhai I realized we were missing this
>> sanity check....
>>
>>  include/linux/user_namespace.h |  8 +++++++-
>>  kernel/pid_namespace.c         |  4 ++++
>>  kernel/user_namespace.c        | 18 ++++++++++++------
>>  3 files changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
>> index 32354b4b4b2b..497ed50004db 100644
>> --- a/include/linux/user_namespace.h
>> +++ b/include/linux/user_namespace.h
>> @@ -112,8 +112,9 @@ extern ssize_t proc_projid_map_write(struct file *, const char __user *, size_t,
>>  extern ssize_t proc_setgroups_write(struct file *, const char __user *, size_t, loff_t *);
>>  extern int proc_setgroups_show(struct seq_file *m, void *v);
>>  extern bool userns_may_setgroups(const struct user_namespace *ns);
>> +extern bool in_userns(const struct user_namespace *ancestor,
>> +		       const struct user_namespace *child);
>>  extern bool current_in_userns(const struct user_namespace *target_ns);
>> -
>>  struct ns_common *ns_get_owner(struct ns_common *ns);
>>  #else
>>  
>> @@ -144,6 +145,11 @@ static inline bool userns_may_setgroups(const struct user_namespace *ns)
>>  	return true;
>>  }
>>  
>> +static inline bool in_userns(const struct user_namespace *target_ns)
>> +{
>> +	return true;
>> +}
>> +
>>  static inline bool current_in_userns(const struct user_namespace *target_ns)
>>  {
>>  	return true;
>> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
>> index de461aa0bf9a..749147f5a613 100644
>> --- a/kernel/pid_namespace.c
>> +++ b/kernel/pid_namespace.c
>> @@ -101,6 +101,10 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
>>  	int i;
>>  	int err;
>>  
>> +	err = -EINVAL;
>> +	if (!in_userns(parent_pid_ns->user_ns, user_ns))
>> +		goto out;
>> +
>>  	err = -ENOSPC;
>>  	if (level > MAX_PID_NS_LEVEL)
>>  		goto out;
>> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
>> index 2f735cbe05e8..7d8658fbabc8 100644
>> --- a/kernel/user_namespace.c
>> +++ b/kernel/user_namespace.c
>> @@ -986,19 +986,25 @@ bool userns_may_setgroups(const struct user_namespace *ns)
>>  }
>>  
>>  /*
>> - * Returns true if @ns is the same namespace as or a descendant of
>> - * @target_ns.
>> + * Returns true if @child is the same namespace or a descendant of
>> + * @ancestor.
>>   */
>> -bool current_in_userns(const struct user_namespace *target_ns)
>> +bool in_userns(const struct user_namespace *ancestor,
>> +	       const struct user_namespace *child)
>>  {
>> -	struct user_namespace *ns;
>> -	for (ns = current_user_ns(); ns; ns = ns->parent) {
>> -		if (ns == target_ns)
>> +	const struct user_namespace *ns;
>> +	for (ns = child; ns; ns = ns->parent) {
>> +		if (ns == ancestor)
>>  			return true;
>>  	}
>>  	return false;
>>  }
> 
> We have user_namespace::level, so it's possible to stop iterations earlier
> and save some cpu cycles:
> 
> 	for (ns = child; ns->level >= ancestor->level; ns = ns->parent)

Just ">" here.

> 		;
> 	return (ns == ancestor);
> 
>>  
>> +bool current_in_userns(const struct user_namespace *target_ns)
>> +{
>> +	return in_userns(target_ns, current_user_ns());
>> +}
>> +
>>  static inline struct user_namespace *to_user_ns(struct ns_common *ns)
>>  {
>>  	return container_of(ns, struct user_namespace, ns);
>>

Re: [PATCH] userns,pidns: Verify the userns for new pid namespaces

[Eric W. Biederman]

Kirill Tkhai <ktkhai-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org> writes:
>>> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
>>> index 2f735cbe05e8..7d8658fbabc8 100644
>>> --- a/kernel/user_namespace.c
>>> +++ b/kernel/user_namespace.c
>>> @@ -986,19 +986,25 @@ bool userns_may_setgroups(const struct user_namespace *ns)
>>>  }
>>>  
>>>  /*
>>> - * Returns true if @ns is the same namespace as or a descendant of
>>> - * @target_ns.
>>> + * Returns true if @child is the same namespace or a descendant of
>>> + * @ancestor.
>>>   */
>>> -bool current_in_userns(const struct user_namespace *target_ns)
>>> +bool in_userns(const struct user_namespace *ancestor,
>>> +	       const struct user_namespace *child)
>>>  {
>>> -	struct user_namespace *ns;
>>> -	for (ns = current_user_ns(); ns; ns = ns->parent) {
>>> -		if (ns == target_ns)
>>> +	const struct user_namespace *ns;
>>> +	for (ns = child; ns; ns = ns->parent) {
>>> +		if (ns == ancestor)
>>>  			return true;
>>>  	}
>>>  	return false;
>>>  }
>> 
>> We have user_namespace::level, so it's possible to stop iterations earlier
>> and save some cpu cycles:
>> 
>> 	for (ns = child; ns->level >= ancestor->level; ns = ns->parent)
>
> Just ">" here.
>
>> 		;
>> 	return (ns == ancestor);

Good observation.  Thank you.

Eric