Message-ID: <20200304201113.GA98782@google.com>
Date:   Wed, 4 Mar 2020 13:11:13 -0700
From:   Ross Zwisler <zwisler@...gle.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Ross Zwisler <zwisler@...omium.org>,
        Aleksa Sarai <cyphar@...har.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mattias Nissler <mnissler@...omium.org>,
        David Howells <dhowells@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        Raul Rangel <rrangel@...gle.com>,
        linux-fsdevel@...r.kernel.org,
        Benjamin Gordon <bmgordon@...gle.com>,
        Micah Morton <mortonm@...gle.com>,
        Dmitry Torokhov <dtor@...gle.com>,
        Jesse Barnes <jsbarnes@...gle.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND v6] Add a "nosymfollow" mount option.

On Wed, Mar 04, 2020 at 06:38:29PM +0000, Al Viro wrote:
> On Wed, Mar 04, 2020 at 10:34:46AM -0700, Ross Zwisler wrote:
> > From: Mattias Nissler <mnissler@...omium.org>
> > 
> > For mounts that have the new "nosymfollow" option, don't follow symlinks
> > when resolving paths. The new option is similar in spirit to the
> > existing "nodev", "noexec", and "nosuid" options, as well as to the
> > LOOKUP_NO_SYMLINKS resolve flag in the openat2(2) syscall. Various BSD
> > variants have been supporting the "nosymfollow" mount option for a long
> > time with equivalent implementations.
> > 
> > Note that symlinks may still be created on file systems mounted with
> > the "nosymfollow" option present. readlink() remains functional, so
> > user space code that is aware of symlinks can still choose to follow
> > them explicitly.
> > 
> > Setting the "nosymfollow" mount option helps prevent privileged
> > writers from modifying files unintentionally in case there is an
> > unexpected link along the accessed path. The "nosymfollow" option is
> > thus useful as a defensive measure for systems that need to deal with
> > untrusted file systems in privileged contexts.
> > 
> > More information on the history and motivation for this patch can be
> > found here:
> > 
> > https://sites.google.com/a/chromium.org/dev/chromium-os/chromiumos-design-docs/hardening-against-malicious-stateful-data#TOC-Restricting-symlink-traversal
> > 
> > Signed-off-by: Mattias Nissler <mnissler@...omium.org>
> > Signed-off-by: Ross Zwisler <zwisler@...gle.com>
> > ---
> > Resending v6 which was previously posted here [0].
> > 
> > Aleksa, if I've addressed all of your feedback, would you mind adding
> > your Reviewed-by?
> > 
> > Andrew, would you please consider merging this?
> 
> NAK.  It's not that I hated the patch, but I call hard moratorium on
> fs/namei.c features this cycle.
> 
> Reason: very massive rewrite of the entire area about to hit -next.
> Moreover, that rewrite is still in the "might be reordered/rebased/whatnot"
> stage.  The patches had been posted on fsdevel, along with the warning
> that it's going into -next shortly.
> 
> Folks, we are close enough to losing control of complexity in that
> code.  It needs to be sanitized, or we'll get into a state where the
> average amount of new bugs introduced by fixing an old one exceeds 1.
> 
> There had been several complexity injections into that thing over
> years (r/o bind-mounts, original RCU pathwalk merge, atomic_open,
> mount traps, openat2 to name some) and while some of that got eventually
> cleaned up, there's a lot of subtle stuff accumulated in the area.
> It can be sanitized and I am doing just that (62 commits in the local
> branch at the moment).  If that gets in the way of someone's patches -
> too fucking bad.  The stuff already in needs to be integrated properly;
> that gets priority over additional security hardening any day, especially
> since this cycle has already seen
> 	* user-triggerable oops in several years old hardening stuff
> (use-after-free, unlikely to be escalatable beyond null pointer
> dereference).  And I'm not blaming the patch authors - liveness analysis
> in do_last() as it is in mainline is a nightmare.
> 	* my own brown paperbag braino in attempt to fix that.
> Fortunately that one was easily caught by fuzzers and it was trivial to fix
> once found.  Again, liveness analysis (and data invariants) from hell...
> 	* gaps in LOOKUP_NO_XDEV (openat2 series, just merged).  Missed
> on review.  Reason: several places implementing mount crossing, with
> varying amount of divergence between them.  One got missed...
> 	* rather interesting corner cases of aushit vs. open vs. NFS.
> Fairly old ones, at that.  Still sorting that one out...
> 
> Anyway, the bottom line is: leave fs/namei.c (especially around the
> pathwalk-related code) alone for now.  Or work on top of the posted
> series, but expect it to change quite a bit under you.  Trying to
> dump that fun job on akpm is unlikely to work.  And if all of that
> comes as a surprise since you are not following fsdevel, consider
> doing so in the future, please.
> 
> PS:
> al@...zy:~/linux/trees/vfs$ git diff --stat v5.6-rc1..HEAD fs/namei.c
>  fs/namei.c | 1408 +++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------
>  1 file changed, 597 insertions(+), 811 deletions(-)
> al@...zy:~/linux/trees/vfs$ wc -l fs/namei.c
> 4723 fs/namei.c
> 
> The affected area is almost exclusively in core pathname resolution
> code.

Makes sense, thank you for the response.

Powered by blists - more mailing lists