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:4.281 / U:0.692 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site