Skip to contentSkip to conversation

mnt: Prevent pivot_root from creating a loop in the mount tree

PATCH In review Eric W. Biederman 2 participants 5 messages Oct 8, 2014 – Oct 14, 2014 1 message not in the archive VFS

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): CALCETrVRqe+KSWHT8bLsx_zo4mYcJE+3H4=0pnhu+g_ZGH1hjQ@mail.gmail.com

#1 lore raw

PATCH 1/1 mnt: Prevent pivot_root from creating a loop in the mount tree

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.

---
 fs/namespace.c | 3 +++
 1 file changed, 3 insertions(+)
fs/namespace.c +3 -0
index b3bdda8b5a01..7b776285832e 100644
@@ -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);
Signature · 1 line
1.9.1
#2 lore raw
Quoted 4 lines · Eric W. Biederman

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

Quoted 33 lines

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.

---
 fs/namespace.c | 3 +++
 1 file changed, 3 insertions(+)
fs/namespace.c +3 -1
index b3bdda8b5a01..7b776285832e 100644
@@ -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

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
#3 lore raw
Quoted 9 lines · Andy Lutomirski
Quoted 4 lines · Eric W. Biederman

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

Quoted 35 lines

--Andy

Quoted 33 lines

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.

---
 fs/namespace.c | 3 +++
 1 file changed, 3 insertions(+)
fs/namespace.c +3 -1
index b3bdda8b5a01..7b776285832e 100644
@@ -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

Reviewed-by
#4 lore raw
Quoted 12 lines · Eric W. Biederman
Quoted 9 lines · Andy Lutomirski
Quoted 4 lines · Eric W. Biederman

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?

Quoted 7 lines

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:

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

Also, this needs Cc: [email protected], 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

Quoted 36 lines
Quoted 36 lines

--Andy

Quoted 33 lines

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.

---
 fs/namespace.c | 3 +++
 1 file changed, 3 insertions(+)
fs/namespace.c +3 -1
index b3bdda8b5a01..7b776285832e 100644
@@ -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

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
#5 lore raw
Quoted 22 lines · Andy Lutomirski
Quoted 12 lines · Eric W. Biederman
Quoted 9 lines · Andy Lutomirski
Quoted 4 lines · Eric W. Biederman

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?

Quoted 7 lines

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

Quoted 5 lines

FWIW, the patch is:

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.

Quoted 1 line

Also, this needs Cc: [email protected], I believe.

Agreed.

Quoted 6 lines

(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

Quoted 35 lines
Quoted 35 lines
Quoted 35 lines

--Andy

Quoted 33 lines

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.

---
 fs/namespace.c | 3 +++
 1 file changed, 3 insertions(+)
fs/namespace.c +3 -1
index b3bdda8b5a01..7b776285832e 100644
@@ -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