From: 吴异 <wangzhibei1999@gmail.com>
To: Trond Myklebust <trondmy@hammerspace.com>
Cc: "bfields@fieldses.org" <bfields@fieldses.org>,
"security@kernel.org" <security@kernel.org>,
"w@1wt.eu" <w@1wt.eu>, "greg@kroah.com" <greg@kroah.com>,
"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
"chuck.lever@oracle.com" <chuck.lever@oracle.com>
Subject: Re: nfsd vurlerability submit
Date: Tue, 19 Jan 2021 00:29:28 +0800 [thread overview]
Message-ID: <CAHxDmpQVxOPmA6o535yEC34fNrA2Of=_W-f49L6gDvxVC3FH6w@mail.gmail.com> (raw)
In-Reply-To: <8296b696a7fa5591ad3fbb05bfcf6bdf6175cc38.camel@hammerspace.com>
hello,
I want to consult you on what is the original intention of designing
subtree_check and whether it is to solve the 'I want to export a
subtree of a filesystem' problem.
As far as I know, when opening subtree_check, the folder's file
handle does not contain the inode information of its parent directory
and
'while (tdentry != exp->ex_path.dentry && !IS_ROOT(tdentry))' in
nfsd_acceptable can work well to Intercept handles beyond the export
point.
This seems to delete code as follows in nfsfh.c could solve the 'I
want to export a subtree of a filesystem' problem and ensure safety:
if (exp->ex_flags & NFSEXP_NOSUBTREECHECK)
return 1;
Or replace by follow:
if (exp->ex_path.dentry == exp->vfs_mount->mnt_root)
return 1;
When I was reading the nfsd code, I was confused about whether the
designer used the file system as a security boundary or an export
point.Since exporting a complete file system is the safest, why not
directly prohibit unsafe practices, but add code like subtree_check to
try to verify the file handle.
I may not understand your design ideas.
Yours sincerely,
Trond Myklebust <trondmy@hammerspace.com> 于2021年1月13日周三 上午12:53写道:
>
> On Tue, 2021-01-12 at 10:32 -0500, J. Bruce Fields wrote:
> > On Tue, Jan 12, 2021 at 10:48:00PM +0800, 吴异 wrote:
> > > Telling users how to configure the exported file system in the most
> > > secure
> > > way does
> > > mitigate the problem to some extent, but this does not seem to
> > > address the
> > > security risks posed by no_ subtree_ check in the code. In my
> > > opinion,when
> > > the generated filehandle does not contain the inode information of
> > > the
> > > parent directory,the nfsd_acceptable function can also recursively
> > > determine whether the request file exceeds the export path
> > > dentry.Enabling
> > > subtree_check to add parent directory information only brings some
> > > troubles.
> >
> > Filesystems don't necessarily provide us with an efficient way to
> > find
> > parent directories from any given file. (And note a single file may
> > have multiple parent directories.)
> >
> > (I do wonder if we could do better in the directory case, though. We
> > already reconnect directories all the way back up to the root.)
> >
> > > I have a bold idea, why not directly remove the file handle
> > > modification in
> > > subtree_check, and then normalize the judgment of whether dentry
> > > exceeds
> > > the export point directory in nfsd_acceptable (line 38 to 54 in
> > > /fs/nfsd/nfsfh.c) .
> > >
> > > As far as I understand it, the reason why subtree_check is not
> > > turned on by
> > > default is that it will cause problems when reading and writing
> > > files,
> > > rather than it wastes more time when nfsd_acceptable.
> > >
> > > In short,I think it's open to question whether the security of the
> > > system
> > > depends on the user's complete correct configuration(the system
> > > does not
> > > prohibit the export of a subdirectory).
> >
> > > Enabling subtree_check to add parent directoryinformation only
> > > brings
> > > some troubles.
> > >
> > > In short,I think it's open to question whether the security of the
> > > system depends on the user's complete correct configuration(the
> > > system
> > > does not prohibit the export of a subdirectory).
> >
> > I'd love to replace the export interface by one that prohibited
> > subdirectory exports (or at least made it more obvious where they're
> > being used.)
> >
> > But given the interface we already have, that would be a disruptive
> > and
> > time-consuming change.
> >
> > Another approach is to add more entropy to filehandles so they're
> > harder
> > to guess; see e.g.:
> >
> > https://www.fsl.cs.stonybrook.edu/docs/nfscrack-tr/index.html
> >
> > In the end none of these change the fact that a filehandle has an
> > infinite lifetime, so once it's leaked, there's nothing you can do.
> > The
> > authors suggest NFSv4 volatile filehandles as a solution to that
> > problem, but I don't think they've thought through the obstacles to
> > making volatile filehandles work.
> >
> > --b.
>
> The point is that there is no good solution to the 'I want to export a
> subtree of a filesystem' problem, and so it is plainly wrong to try to
> make a default of those solutions, which break the one sane case of
> exporting the whole filesystem.
>
> Just a reminder that we kicked out subtree_check not only because a
> trivial rename of a file breaks the client's ability to perform I/O by
> invalidating the filehandle. In addition, that option causes filehandle
> aliasing (i.e. multiple filehandles pointing to the same file) which is
> a major PITA for clients to try to manage for more or less the same
> reason that it is a major PITA to try to manage these files using
> paths.
>
> The discussion on volatile filehandles in RFC5661 does try to address
> some of the above issues, but ends up concluding that you need to
> introduce POSIX-incompatible restrictions, such as trying to ban
> renames and deletions of open files in order to make it work.
>
> None of these compromises are necessary if you export a whole
> filesystem (or a hierarchy of whole filesystems). That's the sane case.
> That's the one that people should default to using.
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>
next prev parent reply other threads:[~2021-01-18 16:31 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CAHxDmpTKJfnhGY9CVupyVYhNCTDVKBB6KRwh-E6u_XEPJq4WJQ@mail.gmail.com>
[not found] ` <20210105165633.GC14893@fieldses.org>
[not found] ` <X/hEB8awvGyMKi6x@kroah.com>
[not found] ` <20210108152017.GA4183@fieldses.org>
[not found] ` <CAHxDmpSp1LHzKD5uqbfi+jcnb+nFaAZbc5++E0oOvLsYvyYDpw@mail.gmail.com>
[not found] ` <20210108164433.GB8699@fieldses.org>
[not found] ` <CAHxDmpSjwrcr_fqLJa5=Zo=xmbt2Eo9dcy6TQuoU8+F3yVVNhw@mail.gmail.com>
[not found] ` <20210110201740.GA8789@fieldses.org>
[not found] ` <20210110202815.GB8789@fieldses.org>
[not found] ` <CAHxDmpR8S7NR8OU2nWJmWBdFU9a7wDuDnxviQ2E9RDOeW9fExg@mail.gmail.com>
2021-01-11 19:25 ` nfsd vurlerability submit J. Bruce Fields
2021-01-11 21:01 ` [PATCH] nfsd4: readdirplus shouldn't return parent of export J. Bruce Fields
2021-01-12 13:31 ` Chuck Lever
2021-01-12 13:50 ` Bruce Fields
[not found] ` <20210108152607.GA950@1wt.eu>
[not found] ` <20210108153237.GB4183@fieldses.org>
[not found] ` <20210108154230.GB950@1wt.eu>
[not found] ` <20210111193655.GC2600@fieldses.org>
[not found] ` <CAHxDmpR1zG25ADfK2jat4VKGbAOCg6YM_0WA+a_jQE82hbnMjA@mail.gmail.com>
[not found] ` <CAHxDmpRfmVukMR_yF4coioiuzrsp72zBraHWZ8gaMydUuLwKFg@mail.gmail.com>
2021-01-12 15:32 ` nfsd vurlerability submit J. Bruce Fields
2021-01-12 16:53 ` Trond Myklebust
2021-01-12 17:20 ` Patrick Goetz
2021-01-12 18:03 ` bfields
2021-01-13 8:12 ` Christoph Hellwig
2021-01-13 14:34 ` Trond Myklebust
2021-01-13 14:40 ` hch
2021-01-13 15:16 ` Trond Myklebust
2021-01-13 15:30 ` hch
2021-01-13 15:45 ` Frank Filz
2021-01-21 20:01 ` Patrick Goetz
2021-01-21 22:04 ` bfields
2021-01-21 23:19 ` Patrick Goetz
2021-01-22 1:30 ` bfields
2021-01-22 13:20 ` Patrick Goetz
2021-01-22 14:48 ` Tom Talpey
[not found] ` <CAHxDmpTEBJ1jd_fr3GJ4k7KgzaBpe1LwKgyZn0AJ0D1ESK12fQ@mail.gmail.com>
2021-01-12 17:47 ` Trond Myklebust
[not found] ` <CAHxDmpTyrG74hOkzmDK834t+JiQduWHVWxCf_7nrDVa++EK2mA@mail.gmail.com>
2021-01-13 14:25 ` Trond Myklebust
2021-01-14 18:07 ` bfields
2021-01-14 18:29 ` Linus Torvalds
2021-01-14 18:35 ` Chuck Lever
2021-01-14 18:37 ` Linus Torvalds
2021-01-18 16:29 ` 吴异 [this message]
2021-01-18 22:55 ` bfields
2021-01-19 2:48 ` 吴异
2021-01-19 3:46 ` bfields
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='CAHxDmpQVxOPmA6o535yEC34fNrA2Of=_W-f49L6gDvxVC3FH6w@mail.gmail.com' \
--to=wangzhibei1999@gmail.com \
--cc=bfields@fieldses.org \
--cc=chuck.lever@oracle.com \
--cc=greg@kroah.com \
--cc=linux-nfs@vger.kernel.org \
--cc=security@kernel.org \
--cc=trondmy@hammerspace.com \
--cc=w@1wt.eu \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).