linux-kernel - Re: [PATCH 0/3] vfs: plug some holes involving LAST_BIND symlinks and file bind mounts (try #5)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <m1my2cdhi3.fsf@fess.ebiederm.org>
Date:	Mon, 23 Nov 2009 14:05:24 -0800
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Jeff Layton <jlayton@...hat.com>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	pavel@....cz, miklos@...redi.hu, viro@...IV.linux.org.uk
Subject: Re: [PATCH 0/3] vfs: plug some holes involving LAST_BIND symlinks and file bind mounts (try #5)

Jeff Layton <jlayton@...hat.com> writes:

> There are a few situations where a lookup can end up returning a dentry
> without revalidating it, and without checking whether the calling
> process has permissions to access it. Two situations identified so far
> are:
>
> 1) LAST_BIND symlinks (such as those under /proc/<pid>)
>
> 2) file bind mounts
>
> This patchset is intended to fix this by forcing revalidation of the
> returned dentries at appropriate locations.
>
> In the case of LAST_BIND symlinks it also adds a check to verify that
> the target of the symlink is accessible by the current process by
> walking mounts and dentries back up to the root and checking permission
> on each inode.
>
> This set fixes the reproducers I have (including the reproducer that
> Pavel provided for the permissions bypass). It's still pretty rough
> though and I expect that it'll need revision. At this point, I'm mainly
> looking to get these questions answered:
>
> 1) what should we do if these dentries are found to be invalid? Is it ok
> to d_invalidate them? Or is that likely to break something (particularly
> in the case of file bind mounts)?

The normal sequence in do_revalidate should be safe.  In practice what we
should see is d_drop().  If we access the dentries via another path today
we already go through d_revalidate.  It is only the reference count on
the dentry that keeps them alive and working.  The cases I have looked
at for distributed filesystems have to call d_drop themselves so I don't
know if it would add anything if the vfs called d_revalidate.  Especially
since FS_REVAL_DOT doesn't have that logic.

> 2) I'm using FS_REVAL_DOT as an indicator of whether to force a
> d_revalidate. I think that it's appropriate to key off of that flag, but
> we may want to rename it (maybe FS_FORCE_D_REVAL ?).

Perhaps FS_ALWAYS_REVAL.    I don't think it makes much of
a difference either way.  I expect a rename should come after we fix
nfsv4 so there is a chance at pushing the fixes back to stable.

> 3) is check_path_accessible racy? It seems to work, but something
> doesn't seem quite right with this approach. Is this defeatable somehow?
> Could a rename of one of the intermediate path components cause
> problems?

check_path_accessible seems pretty horrible.   If a process is running
inside of a subdirectory it doesn't have permissions to access, say
a chroot, /proc/self/fd/XXX becomes completely unusable.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/