From: Aleksa Sarai <email@example.com>
To: Amir Goldstein <firstname.lastname@example.org>
Cc: "Christian Brauner" <email@example.com>,
"Christian Göttsche" <firstname.lastname@example.org>,
"SElinux list" <email@example.com>,
"Miklos Szeredi" <firstname.lastname@example.org>,
"Linux API" <email@example.com>,
"Alexander Viro" <firstname.lastname@example.org>,
Subject: Re: [RFC PATCH] f*xattr: allow O_PATH descriptors
Date: Wed, 22 Jun 2022 12:57:15 +1000 [thread overview]
Message-ID: <20220622025715.upflevvao3ttaekj@senku> (raw)
[-- Attachment #1: Type: text/plain, Size: 7215 bytes --]
On 2022-06-20, Amir Goldstein <email@example.com> wrote:
> > > > The goal would be that the semantics of fooat(<fd>, AT_EMPTY_PATH) and
> > > > foo(/proc/self/fd/<fd>) should always be identical, and the current
> > > > semantics of /proc/self/fd/<fd> are too leaky so we shouldn't always
> > > > assume that keeping them makes sense (the most obvious example is being
> > > > able to do tricks to open /proc/$pid/exe as O_RDWR).
> > >
> > > Please make a note that I have applications relying on current magic symlink
> > > semantics w.r.t setxattr() and other metadata operations, and the libselinux
> > > commit linked from the patch commit message proves that magic symlink
> > > semantics are used in the wild, so it is not likely that those semantics could
> > > be changed, unless userspace breakage could be justified by fixing a serious
> > > security issue (i.e. open /proc/$pid/exe as O_RDWR).
> > Agreed. We also use magiclinks for similar TOCTOU-protection purposes in
> > runc (as does lxc) as well as in libpathrs so I'm aware we need to be
> > careful about changing existing behaviours. I would prefer to have the
> > default be as restrictive as possible, but naturally back-compat is
> > more important.
> > > > I suspect that the long-term solution would be to have more upgrade
> > > > masks so that userspace can opt-in to not allowing any kind of
> > > > (metadata) write access through a particular file descriptor. You're
> > > > quite right that we have several metadata write AT_EMPTY_PATH APIs, and
> > > > so we can't retroactively block /everything/ but we should try to come
> > > > up with less leaky rules by default if it won't break userspace.
> > >
> > > Ok, let me try to say this in my own words using an example to see that
> > > we are all on the same page:
> > >
> > > - lsetxattr(PATH_TO_FILE,..) has inherent TOCTOU races
> > > - fsetxattr(fd,...) is not applicable for symbolic links
> > While I agree with Christian's concerns about making O_PATH descriptors
> > more leaky, if userspace already relies on this through /proc/self/fd/$x
> > then there's not much we can do about it other than having an opt-out
> > available in openat2(2). Having the option to disable this stuff to
> > avoid making O_PATH descriptors less safe as a mechanism for passing
> > around "capability-less" file handles should make most people happy
> > (with the note that ideally we would not be *adding* capabilities to
> > O_PATH we don't need to).
> > > - setxattr("/proc/self/fd/<fd>",...) is the current API to avoid TOCTOU races
> > > when setting xattr on symbolic links
> > > - setxattrat(o_path_fd, "", ..., AT_EMPTY_PATH) is proposed as a the
> > > "new API" for setting xattr on symlinks (and special files)
> > If this is a usecase we need to support then we may as well just re-use
> > fsetxattr() since it's basically an *at(2) syscall already (and I don't
> > see why we'd want to split up the capabilities between two similar
> > *at(2)-like syscalls). Though this does come with the above caveats that
> > we need to have the opt-outs available if we're going to enshrine this
> > as intentional part of the ABI.
> Christian preferred that new functionality be added with a new API
> and I agree that this is nicer and more explicit.
Fair enough -- I misread the man page, setxattrat(2) makes more sense.
> The bigger question IMO is, whether fsomething() should stay identical
> to somethingat(,,,AT_EMPTY_PATH). I don't think that it should.
> To me, open(path,O_PATH)+somethingat(,,,AT_EMPTY_PATH) is identical
> to something(path) - it just breaks the path resolution and operation to two
> distinguished steps.
> fsomething() was traditionally used for "really" open fds, so if we don't need
> to, we better not relax it further by allowing O_PATH, but that's just one
Yeah, you're right -- it would be better to not muddle the two (even
though they are conceptually very similar).
> > > - The new API is going to be more strict than the old magic symlink API
> > > - *If* it turns out to not break user applications, old API can also become
> > > more strict to align with new API (unlikely the case for setxattr())
> > > - This will allow sandboxed containers to opt-out of the "old API", by
> > > restricting access to /proc/self/fd and to implement more fine grained
> > > control over which metadata operations are allowed on an O_PATH fd
> > >
> > > Did I understand the plan correctly?
> > Yup, except I don't think we need setxattrat(2).
> > > Do you agree with me that the plan to keep AT_EMPTY_PATH and magic
> > > symlink semantics may not be realistic?
> > To clarify -- my view is that if any current /proc/self/fd/$n semantic
> > needs to be maintained then I would prefer that the proc-less method of
> > doing it (such as through AT_EMPTY_PATH et al) would have the same
> > capability and semantics. There are some cases where the current
> > /proc/self/fd/$n semantics need to be fixed (such as the /proc/$pid/exe
> > example) and in that case the proc-less semantics also need to be made
> > safe.
> > While I would like us to restrict O_PATH as much as possible, if
> > userspace already depends on certain behaviour then we may not be able
> > to do much about it. Having an opt-out would be very important since
> > enshrining these leaky behaviours (which seem to have been overlooked)
> > means we need to consider how userspace can opt out of them.
> > Unfortunately, it should be noted that due to the "magical" nature of
> > nd_jump_link(), I'm not sure how happy Al Viro will be with the kinds of
> > restrictions necessary. Even my current (quite limited) upgrade-mask
> > patchset has to do a fair bit of work to unify the semantics of
> > magic-links and openat(O_EMPTYPATH) -- expanding this to all *at(2)
> > syscalls might be quite painful. (There are also several handfuls of
> > semantic questions which need to be answered about magic-link modes and
> > whether for other *at(2) operations we may need even more complicated
> > rules or even a re-thinking of my current approach.)
> The question remains, regarding the $SUBJECT patch,
> is it fair to block it and deprive libselinux of a non buggy API
> until such time that all the details around masking O_PATH fds
> will be made clear and the new API implemented?
> There is no guarantee this will ever happen, so it does not seem
> reasonable to me.
> To be a reasonable reaction to the currently broken API is
> to either accept the patch as is or request that setxattrat()
> will be added to provide the new functionality.
Since the current functionality cannot be retroactively disabled as it
is being used already through /proc/self/fd/$n, adding
*xattrat(AT_EMPTY_PATH) doesn't really change what is currently possible
I would say we should add *xattrat(2) and then we can add an upgrade
mask blocking it (and other operations) later.
Senior Software Engineer (Containers)
SUSE Linux GmbH
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
next prev parent reply other threads:[~2022-06-22 3:06 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-07 15:31 [RFC PATCH] f*xattr: allow O_PATH descriptors Christian Göttsche
2022-06-08 5:13 ` Amir Goldstein
2022-06-08 11:27 ` Christian Brauner
2022-06-08 12:28 ` Amir Goldstein
2022-06-08 12:48 ` Christian Brauner
2022-06-08 15:12 ` Amir Goldstein
2022-06-09 8:56 ` Christian Brauner
2022-06-18 3:18 ` Aleksa Sarai
2022-06-18 9:11 ` Amir Goldstein
2022-06-18 11:19 ` Christian Göttsche
2022-06-18 15:30 ` Amir Goldstein
2022-06-20 6:07 ` Aleksa Sarai
2022-06-20 7:45 ` Amir Goldstein
2022-06-22 2:57 ` Aleksa Sarai [this message]
2022-08-19 18:05 ` Christian Göttsche
2022-08-19 20:27 ` Amir Goldstein
2022-06-08 16:53 ` Andreas Dilger
2022-06-09 4:35 ` Amir Goldstein
2022-06-09 9:14 ` Christian Göttsche
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:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* 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).