Received-SPF: pass (google.com: domain of linux-kernel+bounces-141345-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240411001012.12513-1-torvalds@linux-foundation.org>
 <CAHk-=wiaoij30cnx=jfvg=Br3YTxhQjp4VWRc6=xYE2=+EVRPg@mail.gmail.com>
 <20240411-alben-kocht-219170e9dc99@brauner> <CAHk-=wjrPDx=f5OSnQVbbJ4id6SZk-exB1VW9Uz3R7rKFvTQeQ@mail.gmail.com>
In-Reply-To: <CAHk-=wjrPDx=f5OSnQVbbJ4id6SZk-exB1VW9Uz3R7rKFvTQeQ@mail.gmail.com>
From: Charles Mirabile <cmirabil@redhat.com>
Date: Thu, 11 Apr 2024 12:44:46 -0400
Message-ID: <CABe3_aGbsPHY9Z5B9WyVWakeWFtief4DpBrDxUiD00qk1irMrg@mail.gmail.com>
Subject: Re: [PATCH] vfs: relax linkat() AT_EMPTY_PATH - aka flink() - requirements
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Christian Brauner <brauner@kernel.org>, Alexander Viro <viro@zeniv.linux.org.uk>, Jan Kara <jack@suse.cz>, 
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Andrew Lutomirski <luto@kernel.org>, Peter Anvin <hpa@zytor.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 11, 2024 at 12:15=E2=80=AFPM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, 11 Apr 2024 at 02:05, Christian Brauner <brauner@kernel.org> wrot=
e:
> >
> > I had a similar discussion a while back someone requested that we relax
> > permissions so linkat can be used in containers.
>
> Hmm.
>
> Ok, that's different - it just wants root to be able to do it, but
> "root" being just in the container itself.
>
> I don't think that's all that useful - I think one of the issues with
> linkat(AT_EMPTY_PATH) is exactly that "it's only useful for root",
> which means that it's effectively useless. Inside a container or out.
>
> Because very few loads run as root-only (and fewer still run with any
> capability bits that aren't just "root or nothing").
>
> Before I did all this, I did a Debian code search for linkat with
> AT_EMPTY_PATH, and it's almost non-existent. And I think it's exactly
> because of this "when it's only useful for root, it's hardly useful at
> all" issue.
>
> (Of course, my Debian code search may have been broken).
>
> So I suspect your special case is actually largely useless, and what
> the container user actually wanted was what my patch does, but they
> didn't think that was possible, so they asked to just extend the
> "root" notion.
>
Yes, that is absolutely the case. When Christian poked holes in my
initial submission, I started working on a v2 but haven't had a chance
to send it because I wanted to make sure my arguments etc were
airtight because I am well aware of just how long and storied the
history of flink is. In the v2 that I didn't post yet, I extended the
ability to link *any* file from only true root to also "root" within a
container following the potentially suspect approach that christian
suggested (I see where you are coming from with the unobvious approach
to avoiding toctou though I obviously support this patch being
merged), but I added a followup patch that checks for whether the file
in question is an `__O_TMPFILE` file which is trivial once we are
jumping through the hoops of looking up the struct file. If it is a
tmpfile (i.e. linkcount =3D 0) I think that all the concerns raised
about processes that inherit a fd being able to create links to the
file inappropriately are moot. Here is an excerpt from the cover
letter I was planning to send that explains my reasoning.

 -  the ability to create an unnamed file, adjust its contents
and attributes (i.e. uid, gid, mode, xattrs, etc) and then only give it a n=
ame
once they are ready is exactly the ideal use-case for a hypothetical `flink=
`
system call. The ability to use `AT_EMPTY_PATH` with `linkat` essentially t=
urns
it into `flink`, but these restrictions hamper it from actually being used,=
 as
most code is not privileged. By checking whether the file to be linked is a
temporary (i.e. unnamed) file we can allow this useful case without allowin=
g
the more risky cases that could have security implications.

 - Although it might appear that the security posture is affected, it is no=
t.
Callers who open an extant file on disk in read only mode for sharing with
potentially untrustworthy code can trust that this change will not affect t=
hem
because it only applies to temporary files. Callers who open a temporary fi=
le
need to do so with write access if they want to actually put any data in it=
,
so they will already have to reopen the file (e.g. by linking it to a path
and opening that path, or opening the magic symlink in proc) if they want t=
o
share it in read-only mode with untrustworthy code. As such, that new file
description will no longer be marked as a temporary file and thus these cha=
nges
do not apply. The final case I can think of is the unlikely possibility of
intentionally sharing read write access to data stored in a temporary file =
with
untrustworthy code, but where that code being able to make a link would sti=
ll
be deleterious. In such a bizarre case, the `O_EXCL` can and should be used=
 to
fully prevent the temporary file from ever having a name, and this change d=
oes
not alter its efficacy.

> I've added Charles to the Cc.
>
> But yes, with my patch, it would now be trivial to make that
>
>         capable(CAP_DAC_READ_SEARCH)
>
> test also be
>
>         ns_capable(f.file->f_cred->user_ns, CAP_DAC_READ_SEARCH)
>
> instead. I suspect not very many would care any more, but it does seem
> conceptually sensible.
>
> As to your patch - I don't like your nd->root  games in that patch at
> all. That looks odd.
>
> Yes, it makes lookup ignore the dfd (so you avoid the TOCTOU issue),
> but it also makes lookup ignore "/". Which happens to be ok with an
> empty path, but still...
>
> So it feels to me like that patch of yours mis-uses something that is
> just meant for vfs_path_lookup().
>
> It may happen to work, but it smells really odd to me.
>
>              Linus
>