Re: [PATCH] mnt: Prevent pivot_root from creating a loop in the mount tree

[ebiederm@xmission.com (Eric W. Biederman)]

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
>>>>