All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mnt: Prevent pivot_root from creating a loop in the mount tree
       [not found] <CALCETrVRqe+KSWHT8bLsx_zo4mYcJE+3H4=0pnhu+g_ZGH1hjQ@mail.gmail.com>
@ 2014-10-08 17:42 ` Eric W. Biederman
  2014-10-14 16:41   ` Andy Lutomirski
  0 siblings, 1 reply; 5+ messages in thread
From: Eric W. Biederman @ 2014-10-08 17:42 UTC (permalink / raw)
  To: Andy Lutomirski; +Cc: security, Al Viro, Serge Hallyn, linux-fsdevel


Andy Lutomirski recently demonstrated that when chroot is used to set
the root path below the path for the new ``root'' passed to pivot_root
the pivot_root system call succeeds and leaks mounts.

In examining the code I see that starting with a new root that is
below the current root in the mount tree will result in a loop in the
mount tree after the mounts are detached and then reattached to one
another.  Resulting in all kinds of ugliness including a leak of that
mounts involved in the leak of the mount loop.

Prevent this problem by ensuring that the new mount is reachable from
the current root of the mount tree.

Reported-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/namespace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/namespace.c b/fs/namespace.c
index b3bdda8b5a01..7b776285832e 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2830,6 +2830,9 @@ SYSCALL_DEFINE2(pivot_root, const char __user *, new_root,
 	/* make sure we can reach put_old from new_root */
 	if (!is_path_reachable(old_mnt, old.dentry, &new))
 		goto out4;
+	/* make certain new is below the root */
+	if (!is_path_reachable(new_mnt, new.dentry, &root))
+		goto out4;
 	root_mp->m_count++; /* pin it so it won't go away */
 	lock_mount_hash();
 	detach_mnt(new_mnt, &parent_path);
-- 
1.9.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] mnt: Prevent pivot_root from creating a loop in the mount tree
  2014-10-08 17:42 ` [PATCH] mnt: Prevent pivot_root from creating a loop in the mount tree Eric W. Biederman
@ 2014-10-14 16:41   ` Andy Lutomirski
  2014-10-14 19:57     ` Eric W. Biederman
  0 siblings, 1 reply; 5+ messages in thread
From: Andy Lutomirski @ 2014-10-14 16:41 UTC (permalink / raw)
  To: Eric W. Biederman; +Cc: security, Al Viro, Serge Hallyn, Linux FS Devel

On Wed, Oct 8, 2014 at 10:42 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Andy Lutomirski recently demonstrated that when chroot is used to set
> the root path below the path for the new ``root'' passed to pivot_root
> the pivot_root system call succeeds and leaks mounts.

As part of my security fix what-happened-to-it quest: what happened to this fix?

--Andy

>
> In examining the code I see that starting with a new root that is
> below the current root in the mount tree will result in a loop in the
> mount tree after the mounts are detached and then reattached to one
> another.  Resulting in all kinds of ugliness including a leak of that
> mounts involved in the leak of the mount loop.
>
> Prevent this problem by ensuring that the new mount is reachable from
> the current root of the mount tree.
>
> Reported-by: Andy Lutomirski <luto@amacapital.net>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  fs/namespace.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index b3bdda8b5a01..7b776285832e 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2830,6 +2830,9 @@ SYSCALL_DEFINE2(pivot_root, const char __user *, new_root,
>         /* make sure we can reach put_old from new_root */
>         if (!is_path_reachable(old_mnt, old.dentry, &new))
>                 goto out4;
> +       /* make certain new is below the root */
> +       if (!is_path_reachable(new_mnt, new.dentry, &root))
> +               goto out4;
>         root_mp->m_count++; /* pin it so it won't go away */
>         lock_mount_hash();
>         detach_mnt(new_mnt, &parent_path);
> --
> 1.9.1
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] mnt: Prevent pivot_root from creating a loop in the mount tree
  2014-10-14 16:41   ` Andy Lutomirski
@ 2014-10-14 19:57     ` Eric W. Biederman
  2014-10-14 20:23       ` Andy Lutomirski
  0 siblings, 1 reply; 5+ messages in thread
From: Eric W. Biederman @ 2014-10-14 19:57 UTC (permalink / raw)
  To: Andy Lutomirski; +Cc: security, Al Viro, Serge Hallyn, Linux FS Devel

Andy Lutomirski <luto@amacapital.net> writes:

> On Wed, Oct 8, 2014 at 10:42 AM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>>
>> Andy Lutomirski recently demonstrated that when chroot is used to set
>> the root path below the path for the new ``root'' passed to pivot_root
>> the pivot_root system call succeeds and leaks mounts.
>
> As part of my security fix what-happened-to-it quest: what happened to
> this fix?

On my part I am in the middle of a move right now and I don't have time
to push it to Linus.

But I will mention quickly that the fix below addresses your question of
how should pivot_root behave if chrooted because I continue to allow the
cases that actually work. (Not that I think anyone cares but this is
what we have).

> --Andy
>
>>
>> In examining the code I see that starting with a new root that is
>> below the current root in the mount tree will result in a loop in the
>> mount tree after the mounts are detached and then reattached to one
>> another.  Resulting in all kinds of ugliness including a leak of that
>> mounts involved in the leak of the mount loop.
>>
>> Prevent this problem by ensuring that the new mount is reachable from
>> the current root of the mount tree.
>>
>> Reported-by: Andy Lutomirski <luto@amacapital.net>
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>>  fs/namespace.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/namespace.c b/fs/namespace.c
>> index b3bdda8b5a01..7b776285832e 100644
>> --- a/fs/namespace.c
>> +++ b/fs/namespace.c
>> @@ -2830,6 +2830,9 @@ SYSCALL_DEFINE2(pivot_root, const char __user *, new_root,
>>         /* make sure we can reach put_old from new_root */
>>         if (!is_path_reachable(old_mnt, old.dentry, &new))
>>                 goto out4;
>> +       /* make certain new is below the root */
>> +       if (!is_path_reachable(new_mnt, new.dentry, &root))
>> +               goto out4;
>>         root_mp->m_count++; /* pin it so it won't go away */
>>         lock_mount_hash();
>>         detach_mnt(new_mnt, &parent_path);
>> --
>> 1.9.1
>>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] mnt: Prevent pivot_root from creating a loop in the mount tree
  2014-10-14 19:57     ` Eric W. Biederman
@ 2014-10-14 20:23       ` Andy Lutomirski
  2014-10-14 20:44         ` Eric W. Biederman
  0 siblings, 1 reply; 5+ messages in thread
From: Andy Lutomirski @ 2014-10-14 20:23 UTC (permalink / raw)
  To: Eric W. Biederman, Linus Torvalds
  Cc: security, Al Viro, Serge Hallyn, Linux FS Devel

On Tue, Oct 14, 2014 at 12:57 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Andy Lutomirski <luto@amacapital.net> writes:
>
>> On Wed, Oct 8, 2014 at 10:42 AM, Eric W. Biederman
>> <ebiederm@xmission.com> wrote:
>>>
>>> Andy Lutomirski recently demonstrated that when chroot is used to set
>>> the root path below the path for the new ``root'' passed to pivot_root
>>> the pivot_root system call succeeds and leaks mounts.
>>
>> As part of my security fix what-happened-to-it quest: what happened to
>> this fix?
>
> On my part I am in the middle of a move right now and I don't have time
> to push it to Linus.
>
> But I will mention quickly that the fix below addresses your question of
> how should pivot_root behave if chrooted because I continue to allow the
> cases that actually work. (Not that I think anyone cares but this is
> what we have).

FWIW, the patch is:

Reviewed-by: Andy Lutomirski <luto@amacapital.net>

Linus, Al, can one of you take this?  Eric, are you okay with that?

Also, this needs Cc: stable@vger.kernel.org, I believe.  (No point in
limiting it to 3.8 and newer -- this is a bug bug, not just a security
bug, and based on my extremely limited understanding of how Docker and
lxc work, this could be exploitable even without user namespaces.  Of
course, containers set up like that are giant security problems
regardless.)

--Andy

>
>> --Andy
>>
>>>
>>> In examining the code I see that starting with a new root that is
>>> below the current root in the mount tree will result in a loop in the
>>> mount tree after the mounts are detached and then reattached to one
>>> another.  Resulting in all kinds of ugliness including a leak of that
>>> mounts involved in the leak of the mount loop.
>>>
>>> Prevent this problem by ensuring that the new mount is reachable from
>>> the current root of the mount tree.
>>>
>>> Reported-by: Andy Lutomirski <luto@amacapital.net>
>>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>> ---
>>>  fs/namespace.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/fs/namespace.c b/fs/namespace.c
>>> index b3bdda8b5a01..7b776285832e 100644
>>> --- a/fs/namespace.c
>>> +++ b/fs/namespace.c
>>> @@ -2830,6 +2830,9 @@ SYSCALL_DEFINE2(pivot_root, const char __user *, new_root,
>>>         /* make sure we can reach put_old from new_root */
>>>         if (!is_path_reachable(old_mnt, old.dentry, &new))
>>>                 goto out4;
>>> +       /* make certain new is below the root */
>>> +       if (!is_path_reachable(new_mnt, new.dentry, &root))
>>> +               goto out4;
>>>         root_mp->m_count++; /* pin it so it won't go away */
>>>         lock_mount_hash();
>>>         detach_mnt(new_mnt, &parent_path);
>>> --
>>> 1.9.1
>>>



-- 
Andy Lutomirski
AMA Capital Management, LLC

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] mnt: Prevent pivot_root from creating a loop in the mount tree
  2014-10-14 20:23       ` Andy Lutomirski
@ 2014-10-14 20:44         ` Eric W. Biederman
  0 siblings, 0 replies; 5+ messages in thread
From: Eric W. Biederman @ 2014-10-14 20:44 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Linus Torvalds, security, Al Viro, Serge Hallyn, Linux FS Devel

Andy Lutomirski <luto@amacapital.net> writes:

> On Tue, Oct 14, 2014 at 12:57 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> Andy Lutomirski <luto@amacapital.net> writes:
>>
>>> On Wed, Oct 8, 2014 at 10:42 AM, Eric W. Biederman
>>> <ebiederm@xmission.com> wrote:
>>>>
>>>> Andy Lutomirski recently demonstrated that when chroot is used to set
>>>> the root path below the path for the new ``root'' passed to pivot_root
>>>> the pivot_root system call succeeds and leaks mounts.
>>>
>>> As part of my security fix what-happened-to-it quest: what happened to
>>> this fix?
>>
>> On my part I am in the middle of a move right now and I don't have time
>> to push it to Linus.
>>
>> But I will mention quickly that the fix below addresses your question of
>> how should pivot_root behave if chrooted because I continue to allow the
>> cases that actually work. (Not that I think anyone cares but this is
>> what we have).
>
> FWIW, the patch is:
>
> Reviewed-by: Andy Lutomirski <luto@amacapital.net>
>
> Linus, Al, can one of you take this?  Eric, are you okay with that?

I have no objections.  The patch is tested and works from my side, I
just don't have time to retransmit right now.

> Also, this needs Cc: stable@vger.kernel.org, I believe.
Agreed.

>  (No point in
> limiting it to 3.8 and newer -- this is a bug bug, not just a security
> bug, and based on my extremely limited understanding of how Docker and
> lxc work, this could be exploitable even without user namespaces.  Of
> course, containers set up like that are giant security problems
> regardless.)

Andy you might want to add your Reviewed-by and the Cc: stable and stuff
this patch in a git tree so Linus or whomever can grab it.

My regrets for not having more time to push this.

Eric


>>> --Andy
>>>
>>>>
>>>> In examining the code I see that starting with a new root that is
>>>> below the current root in the mount tree will result in a loop in the
>>>> mount tree after the mounts are detached and then reattached to one
>>>> another.  Resulting in all kinds of ugliness including a leak of that
>>>> mounts involved in the leak of the mount loop.
>>>>
>>>> Prevent this problem by ensuring that the new mount is reachable from
>>>> the current root of the mount tree.
>>>>
>>>> Reported-by: Andy Lutomirski <luto@amacapital.net>
>>>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>>> ---
>>>>  fs/namespace.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/fs/namespace.c b/fs/namespace.c
>>>> index b3bdda8b5a01..7b776285832e 100644
>>>> --- a/fs/namespace.c
>>>> +++ b/fs/namespace.c
>>>> @@ -2830,6 +2830,9 @@ SYSCALL_DEFINE2(pivot_root, const char __user *, new_root,
>>>>         /* make sure we can reach put_old from new_root */
>>>>         if (!is_path_reachable(old_mnt, old.dentry, &new))
>>>>                 goto out4;
>>>> +       /* make certain new is below the root */
>>>> +       if (!is_path_reachable(new_mnt, new.dentry, &root))
>>>> +               goto out4;
>>>>         root_mp->m_count++; /* pin it so it won't go away */
>>>>         lock_mount_hash();
>>>>         detach_mnt(new_mnt, &parent_path);
>>>> --
>>>> 1.9.1
>>>>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2014-10-14 20:44 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CALCETrVRqe+KSWHT8bLsx_zo4mYcJE+3H4=0pnhu+g_ZGH1hjQ@mail.gmail.com>
2014-10-08 17:42 ` [PATCH] mnt: Prevent pivot_root from creating a loop in the mount tree Eric W. Biederman
2014-10-14 16:41   ` Andy Lutomirski
2014-10-14 19:57     ` Eric W. Biederman
2014-10-14 20:23       ` Andy Lutomirski
2014-10-14 20:44         ` Eric W. Biederman

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.