lkml.org 
[lkml]   [2020]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@chromium.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@chromium.org>
> > Signed-off-by: Ross Zwisler <zwisler@google.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@dizzy:~/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@dizzy:~/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.

\
 
 \ /
  Last update: 2020-03-04 21:12    [W:0.068 / U:1.492 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site