Received-SPF: pass (google.com: domain of linux-kernel+bounces-141404-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
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>
 <CABe3_aGbsPHY9Z5B9WyVWakeWFtief4DpBrDxUiD00qk1irMrg@mail.gmail.com>
In-Reply-To: <CABe3_aGbsPHY9Z5B9WyVWakeWFtief4DpBrDxUiD00qk1irMrg@mail.gmail.com>
From: Charles Mirabile <cmirabil@redhat.com>
Date: Thu, 11 Apr 2024 13:29:58 -0400
Message-ID: <CABe3_aGGf7kb97gE4FdGmT79Kh5OhbB_2Hqt898WZ+4XGg6j4Q@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

Here is an updated version of linus's patch that makes the check
namespace relative
---
 fs/namei.c            | 17 ++++++++++++-----
 include/linux/namei.h |  1 +
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 4e0de939fea1..2bcc10976ba7 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2419,6 +2419,14 @@ static const char *path_init(struct nameidata
*nd, unsigned flags)
         if (!f.file)
             return ERR_PTR(-EBADF);

+        if (flags & LOOKUP_DFD_MATCH_CREDS) {
+            if (f.file->f_cred !=3D current_cred() &&
+                !ns_capable(f.file->f_cred->user_ns, CAP_DAC_READ_SEARCH))=
 {
+                fdput(f);
+                return ERR_PTR(-ENOENT);
+            }
+        }
+
         dentry =3D f.file->f_path.dentry;

         if (*s && unlikely(!d_can_lookup(dentry))) {
@@ -4640,14 +4648,13 @@ int do_linkat(int olddfd, struct filename
*old, int newdfd,
         goto out_putnames;
     }
     /*
-     * To use null names we require CAP_DAC_READ_SEARCH
+     * To use null names we require CAP_DAC_READ_SEARCH or
+     * that the open-time creds of the dfd matches current.
      * This ensures that not everyone will be able to create
      * handlink using the passed filedescriptor.
      */
-    if (flags & AT_EMPTY_PATH && !capable(CAP_DAC_READ_SEARCH)) {
-        error =3D -ENOENT;
-        goto out_putnames;
-    }
+    if (flags & AT_EMPTY_PATH)
+        how |=3D LOOKUP_DFD_MATCH_CREDS;

     if (flags & AT_SYMLINK_FOLLOW)
         how |=3D LOOKUP_FOLLOW;
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 74e0cc14ebf8..678ffe4acf99 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -44,6 +44,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT};
 #define LOOKUP_BENEATH        0x080000 /* No escaping from starting point.=
 */
 #define LOOKUP_IN_ROOT        0x100000 /* Treat dirfd as fs root. */
 #define LOOKUP_CACHED        0x200000 /* Only do cached lookup */
+#define LOOKUP_DFD_MATCH_CREDS    0x400000 /* Require that dfd creds
match current */
 /* LOOKUP_* flags which do scope-related checks based on the dirfd. */
 #define LOOKUP_IS_SCOPED (LOOKUP_BENEATH | LOOKUP_IN_ROOT)

--=20
2.44.0

On Thu, Apr 11, 2024 at 12:44=E2=80=AFPM Charles Mirabile <cmirabil@redhat.=
com> wrote:
>
> 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> wr=
ote:
> > >
> > > I had a similar discussion a while back someone requested that we rel=
ax
> > > 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=
 name
> once they are ready is exactly the ideal use-case for a hypothetical `fli=
nk`
> system call. The ability to use `AT_EMPTY_PATH` with `linkat` essentially=
 turns
> it into `flink`, but these restrictions hamper it from actually being use=
d, 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 allow=
ing
> the more risky cases that could have security implications.
>
>  - Although it might appear that the security posture is affected, it is =
not.
> Callers who open an extant file on disk in read only mode for sharing wit=
h
> potentially untrustworthy code can trust that this change will not affect=
 them
> because it only applies to temporary files. Callers who open a temporary =
file
> 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 pat=
h
> and opening that path, or opening the magic symlink in proc) if they want=
 to
> share it in read-only mode with untrustworthy code. As such, that new fil=
e
> description will no longer be marked as a temporary file and thus these c=
hanges
> do not apply. The final case I can think of is the unlikely possibility o=
f
> intentionally sharing read write access to data stored in a temporary fil=
e with
> untrustworthy code, but where that code being able to make a link would s=
till
> be deleterious. In such a bizarre case, the `O_EXCL` can and should be us=
ed to
> fully prevent the temporary file from ever having a name, and this change=
 does
> 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
> >