Should unprivileged linkat(..., AT_EMPTY_PATH) work on O_TMPFILE files?

Should unprivileged linkat(..., AT_EMPTY_PATH) work on O_TMPFILE files?

[Andy Lutomirski]

The change:

commit f4e0c30c191f87851c4a53454abb55ee276f4a7e
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Tue Jun 11 08:34:36 2013 +0400

    allow the temp files created by open() to be linked to

    O_TMPFILE | O_CREAT => linkat() with AT_SYMLINK_FOLLOW and /proc/self/fd/<n>
    as oldpath (i.e. flink()) will create a link
    O_TMPFILE | O_CREAT | O_EXCL => ENOENT on attempt to link those guys

    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

makes it possible to hardlink an O_TMPFILE file using procfs.  Should
linkat(fd, "", newdirfd, newpath, AT_EMPTY_PATH) also work?

AFAICS it currently requires CAP_DAC_READ_SEARCH, but I'm a bit
confused as to why linkat(..., AT_EMPTY_PATH) should have a stricter
check than linkat(AT_FDCWD, "/proc/self/fd/n", ...,
AT_SYMLINK_FOLLOW),  (The relevant change is
11a7b371b64ef39fc5fb1b6f2218eef7c4d035e3.)

FWIW, this program works on Linux 3.9, which makes me doubt that the
security restriction on linkat is doing any good:

#include <stdio.h>
#include <err.h>
#include <fcntl.h>
#include <unistd.h>

int main(int argc, char **argv)
{
  char buf[128];

  if (argc != 3)
    errx(1, "Usage: flink FD PATH");

  sprintf(buf, "/proc/self/fd/%d", atoi(argv[1]));
  if (linkat(AT_FDCWD, buf, AT_FDCWD, argv[2], AT_SYMLINK_FOLLOW) != 0)
    err(1, "linkat");
  return 0;
}


Removing the check from the AT_EMPTY_PATH case would simplify code
that wants to write a file, fsync it, and then flink it in.


--Andy

P.S. For even more fun, I'd *love* a linkat flag that would allow the
destination to be overwritten, but that's a different can of worms.

[PATCH] fs: Allow unprivileged linkat(..., AT_EMPTY_PATH) aka flink

[Andy Lutomirski]

Every now and then someone proposes a new flink syscall, and this spawns
a long discussion of whether it would be a security problem.  I think
that this is missing the point: flink is *already* allowed without
privilege as long as /proc is mounted -- it's called AT_SYMLINK_FOLLOW.

Now that O_TMPFILE is here, the ability to create a file with O_TMPFILE,
write it, and link it in is very convenient.  The only problem is that
it requires that /proc be mounted so that you can do:

linkat(AT_FDCWD, "/proc/self/fd/<tmpfd>", dfd, path, AT_SYMLINK_NOFOLLOW)

This sucks -- it's much nicer to do:

linkat(tmpfd, "", dfd, path, AT_EMPTY_PATH)

Let's allow it.

If this turns out to be excessively scary, it we could instead require
that the inode in question be I_LINKABLE, but this seems pointless given
the /proc situation

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 fs/namei.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 8b61d10..89a612e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3671,15 +3671,11 @@ SYSCALL_DEFINE5(linkat, int, olddfd, const char __user *, oldname,
 	if ((flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
 		return -EINVAL;
 	/*
-	 * To use null names we require CAP_DAC_READ_SEARCH
-	 * This ensures that not everyone will be able to create
-	 * handlink using the passed filedescriptor.
+	 * Using empty names is equivalent to using AT_SYMLINK_FOLLOW
+	 * on /proc/self/fd/<fd>.
 	 */
-	if (flags & AT_EMPTY_PATH) {
-		if (!capable(CAP_DAC_READ_SEARCH))
-			return -ENOENT;
+	if (flags & AT_EMPTY_PATH)
 		how = LOOKUP_EMPTY;
-	}
 
 	if (flags & AT_SYMLINK_FOLLOW)
 		how |= LOOKUP_FOLLOW;
-- 
1.8.3.1

Re: Should unprivileged linkat(..., AT_EMPTY_PATH) work on O_TMPFILE files?

[Aneesh Kumar K.V]

Andy Lutomirski <luto@amacapital.net> writes:

> The change:
>
> commit f4e0c30c191f87851c4a53454abb55ee276f4a7e
> Author: Al Viro <viro@zeniv.linux.org.uk>
> Date:   Tue Jun 11 08:34:36 2013 +0400
>
>     allow the temp files created by open() to be linked to
>
>     O_TMPFILE | O_CREAT => linkat() with AT_SYMLINK_FOLLOW and /proc/self/fd/<n>
>     as oldpath (i.e. flink()) will create a link
>     O_TMPFILE | O_CREAT | O_EXCL => ENOENT on attempt to link those guys
>
>     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>
> makes it possible to hardlink an O_TMPFILE file using procfs.  Should
> linkat(fd, "", newdirfd, newpath, AT_EMPTY_PATH) also work?
>
> AFAICS it currently requires CAP_DAC_READ_SEARCH, but I'm a bit
> confused as to why linkat(..., AT_EMPTY_PATH) should have a stricter
> check than linkat(AT_FDCWD, "/proc/self/fd/n", ...,
> AT_SYMLINK_FOLLOW),  (The relevant change is
> 11a7b371b64ef39fc5fb1b6f2218eef7c4d035e3.)
>
> FWIW, this program works on Linux 3.9, which makes me doubt that the
> security restriction on linkat is doing any good:
>
> #include <stdio.h>
> #include <err.h>
> #include <fcntl.h>
> #include <unistd.h>
>
> int main(int argc, char **argv)
> {
>   char buf[128];
>
>   if (argc != 3)
>     errx(1, "Usage: flink FD PATH");
>
>   sprintf(buf, "/proc/self/fd/%d", atoi(argv[1]));
>   if (linkat(AT_FDCWD, buf, AT_FDCWD, argv[2], AT_SYMLINK_FOLLOW) != 0)
>     err(1, "linkat");
>   return 0;
> }
>
>
> Removing the check from the AT_EMPTY_PATH case would simplify code
> that wants to write a file, fsync it, and then flink it in.

I understand that this got merged upstream. But in case of the above
test we would walk the path pointed by /proc/self/fd/<x> right ?

ie,

20 -> /home/no-access/k

will the above test work ? Now if i pass the '20' to another application
I can affectively create a hardlink to that outside no-access and if k
happens to have 'r' for others, then everybody will be able to read
right ?. I understand that limitting the read access based on directory
permission is not a good idea. But aren't we expected to keep that ?

For O_TMPFILE we don't have a path name hence the above may not be a
real issue ? Can you help me understand what i am missing ?

-aneesh

Re: Should unprivileged linkat(..., AT_EMPTY_PATH) work on O_TMPFILE files?

[Andy Lutomirski]

On Sun, Aug 11, 2013 at 9:45 AM, Aneesh Kumar K.V
<aneesh.kumar@linux.vnet.ibm.com> wrote:
> Andy Lutomirski <luto@amacapital.net> writes:
>
>> The change:
>>
>> commit f4e0c30c191f87851c4a53454abb55ee276f4a7e
>> Author: Al Viro <viro@zeniv.linux.org.uk>
>> Date:   Tue Jun 11 08:34:36 2013 +0400
>>
>>     allow the temp files created by open() to be linked to
>>
>>     O_TMPFILE | O_CREAT => linkat() with AT_SYMLINK_FOLLOW and /proc/self/fd/<n>
>>     as oldpath (i.e. flink()) will create a link
>>     O_TMPFILE | O_CREAT | O_EXCL => ENOENT on attempt to link those guys
>>
>>     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>>
>> makes it possible to hardlink an O_TMPFILE file using procfs.  Should
>> linkat(fd, "", newdirfd, newpath, AT_EMPTY_PATH) also work?
>>
>> AFAICS it currently requires CAP_DAC_READ_SEARCH, but I'm a bit
>> confused as to why linkat(..., AT_EMPTY_PATH) should have a stricter
>> check than linkat(AT_FDCWD, "/proc/self/fd/n", ...,
>> AT_SYMLINK_FOLLOW),  (The relevant change is
>> 11a7b371b64ef39fc5fb1b6f2218eef7c4d035e3.)
>>
>> FWIW, this program works on Linux 3.9, which makes me doubt that the
>> security restriction on linkat is doing any good:
>>
>> #include <stdio.h>
>> #include <err.h>
>> #include <fcntl.h>
>> #include <unistd.h>
>>
>> int main(int argc, char **argv)
>> {
>>   char buf[128];
>>
>>   if (argc != 3)
>>     errx(1, "Usage: flink FD PATH");
>>
>>   sprintf(buf, "/proc/self/fd/%d", atoi(argv[1]));
>>   if (linkat(AT_FDCWD, buf, AT_FDCWD, argv[2], AT_SYMLINK_FOLLOW) != 0)
>>     err(1, "linkat");
>>   return 0;
>> }
>>
>>
>> Removing the check from the AT_EMPTY_PATH case would simplify code
>> that wants to write a file, fsync it, and then flink it in.
>
> I understand that this got merged upstream. But in case of the above
> test we would walk the path pointed by /proc/self/fd/<x> right ?
>
> ie,
>
> 20 -> /home/no-access/k
>
> will the above test work ? Now if i pass the '20' to another application
> I can affectively create a hardlink to that outside no-access and if k
> happens to have 'r' for others, then everybody will be able to read
> right ?. I understand that limitting the read access based on directory
> permission is not a good idea. But aren't we expected to keep that ?

The symlinks in /proc/self/fd are rather magical and don't actually
walk the path.  Give it a try :)

--Andy

Re: Should unprivileged linkat(..., AT_EMPTY_PATH) work on O_TMPFILE files?

[Aneesh Kumar K.V]

Andy Lutomirski <luto@amacapital.net> writes:

> On Sun, Aug 11, 2013 at 9:45 AM, Aneesh Kumar K.V
> <aneesh.kumar@linux.vnet.ibm.com> wrote:
>> Andy Lutomirski <luto@amacapital.net> writes:
>>
>>> The change:
>>>
>>> commit f4e0c30c191f87851c4a53454abb55ee276f4a7e
>>> Author: Al Viro <viro@zeniv.linux.org.uk>
>>> Date:   Tue Jun 11 08:34:36 2013 +0400
>>>
>>>     allow the temp files created by open() to be linked to
>>>
>>>     O_TMPFILE | O_CREAT => linkat() with AT_SYMLINK_FOLLOW and /proc/self/fd/<n>
>>>     as oldpath (i.e. flink()) will create a link
>>>     O_TMPFILE | O_CREAT | O_EXCL => ENOENT on attempt to link those guys
>>>
>>>     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>>>
>>> makes it possible to hardlink an O_TMPFILE file using procfs.  Should
>>> linkat(fd, "", newdirfd, newpath, AT_EMPTY_PATH) also work?
>>>
>>> AFAICS it currently requires CAP_DAC_READ_SEARCH, but I'm a bit
>>> confused as to why linkat(..., AT_EMPTY_PATH) should have a stricter
>>> check than linkat(AT_FDCWD, "/proc/self/fd/n", ...,
>>> AT_SYMLINK_FOLLOW),  (The relevant change is
>>> 11a7b371b64ef39fc5fb1b6f2218eef7c4d035e3.)
>>>
>>> FWIW, this program works on Linux 3.9, which makes me doubt that the
>>> security restriction on linkat is doing any good:
>>>
>>> #include <stdio.h>
>>> #include <err.h>
>>> #include <fcntl.h>
>>> #include <unistd.h>
>>>
>>> int main(int argc, char **argv)
>>> {
>>>   char buf[128];
>>>
>>>   if (argc != 3)
>>>     errx(1, "Usage: flink FD PATH");
>>>
>>>   sprintf(buf, "/proc/self/fd/%d", atoi(argv[1]));
>>>   if (linkat(AT_FDCWD, buf, AT_FDCWD, argv[2], AT_SYMLINK_FOLLOW) != 0)
>>>     err(1, "linkat");
>>>   return 0;
>>> }
>>>
>>>
>>> Removing the check from the AT_EMPTY_PATH case would simplify code
>>> that wants to write a file, fsync it, and then flink it in.
>>
>> I understand that this got merged upstream. But in case of the above
>> test we would walk the path pointed by /proc/self/fd/<x> right ?
>>
>> ie,
>>
>> 20 -> /home/no-access/k
>>
>> will the above test work ? Now if i pass the '20' to another application
>> I can affectively create a hardlink to that outside no-access and if k
>> happens to have 'r' for others, then everybody will be able to read
>> right ?. I understand that limitting the read access based on directory
>> permission is not a good idea. But aren't we expected to keep that ?
>
> The symlinks in /proc/self/fd are rather magical and don't actually
> walk the path.  Give it a try :)
>

How about fd passed from one application to another(say from a1 to
a2). a2 won't have read permission on /proc/a1/fd/ and also don't know
the value of file descriptor he should use right ? Will the /proc/self/fd
method work in such case ? IIUC with AT_EMPTY_PATH a2 can create the
link in the above case right ? So if /proc/self/fd doesn't work should
we allow that ?

-aneesh

Re: Should unprivileged linkat(..., AT_EMPTY_PATH) work on O_TMPFILE files?

[Aneesh Kumar K.V]

"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:

> Andy Lutomirski <luto@amacapital.net> writes:
>
>> On Sun, Aug 11, 2013 at 9:45 AM, Aneesh Kumar K.V
>> <aneesh.kumar@linux.vnet.ibm.com> wrote:
>>> Andy Lutomirski <luto@amacapital.net> writes:
>>>
>>>> The change:
>>>>
>>>> commit f4e0c30c191f87851c4a53454abb55ee276f4a7e
>>>> Author: Al Viro <viro@zeniv.linux.org.uk>
>>>> Date:   Tue Jun 11 08:34:36 2013 +0400
>>>>
>>>>     allow the temp files created by open() to be linked to
>>>>
>>>>     O_TMPFILE | O_CREAT => linkat() with AT_SYMLINK_FOLLOW and /proc/self/fd/<n>
>>>>     as oldpath (i.e. flink()) will create a link
>>>>     O_TMPFILE | O_CREAT | O_EXCL => ENOENT on attempt to link those guys
>>>>
>>>>     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>>>>
>>>> makes it possible to hardlink an O_TMPFILE file using procfs.  Should
>>>> linkat(fd, "", newdirfd, newpath, AT_EMPTY_PATH) also work?
>>>>
>>>> AFAICS it currently requires CAP_DAC_READ_SEARCH, but I'm a bit
>>>> confused as to why linkat(..., AT_EMPTY_PATH) should have a stricter
>>>> check than linkat(AT_FDCWD, "/proc/self/fd/n", ...,
>>>> AT_SYMLINK_FOLLOW),  (The relevant change is
>>>> 11a7b371b64ef39fc5fb1b6f2218eef7c4d035e3.)
>>>>
>>>> FWIW, this program works on Linux 3.9, which makes me doubt that the
>>>> security restriction on linkat is doing any good:
>>>>
>>>> #include <stdio.h>
>>>> #include <err.h>
>>>> #include <fcntl.h>
>>>> #include <unistd.h>
>>>>
>>>> int main(int argc, char **argv)
>>>> {
>>>>   char buf[128];
>>>>
>>>>   if (argc != 3)
>>>>     errx(1, "Usage: flink FD PATH");
>>>>
>>>>   sprintf(buf, "/proc/self/fd/%d", atoi(argv[1]));
>>>>   if (linkat(AT_FDCWD, buf, AT_FDCWD, argv[2], AT_SYMLINK_FOLLOW) != 0)
>>>>     err(1, "linkat");
>>>>   return 0;
>>>> }
>>>>
>>>>
>>>> Removing the check from the AT_EMPTY_PATH case would simplify code
>>>> that wants to write a file, fsync it, and then flink it in.
>>>
>>> I understand that this got merged upstream. But in case of the above
>>> test we would walk the path pointed by /proc/self/fd/<x> right ?
>>>
>>> ie,
>>>
>>> 20 -> /home/no-access/k
>>>
>>> will the above test work ? Now if i pass the '20' to another application
>>> I can affectively create a hardlink to that outside no-access and if k
>>> happens to have 'r' for others, then everybody will be able to read
>>> right ?. I understand that limitting the read access based on directory
>>> permission is not a good idea. But aren't we expected to keep that ?
>>
>> The symlinks in /proc/self/fd are rather magical and don't actually
>> walk the path.  Give it a try :)
>>
>
> How about fd passed from one application to another(say from a1 to
> a2). a2 won't have read permission on /proc/a1/fd/ and also don't know
> the value of file descriptor he should use right ? Will the /proc/self/fd
> method work in such case ? IIUC with AT_EMPTY_PATH a2 can create the
> link in the above case right ? So if /proc/self/fd doesn't work should
> we allow that ?

Hmm I guess a2 will be able to use /proc/a2/fd/<received_fd> to create a
link ?

-aneesh

Re: Should unprivileged linkat(..., AT_EMPTY_PATH) work on O_TMPFILE files?

[Andy Lutomirski]

On Sun, Aug 11, 2013 at 9:25 PM, Aneesh Kumar K.V
<aneesh.kumar@linux.vnet.ibm.com> wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
>
>> Andy Lutomirski <luto@amacapital.net> writes:
>>
>>> On Sun, Aug 11, 2013 at 9:45 AM, Aneesh Kumar K.V
>>> <aneesh.kumar@linux.vnet.ibm.com> wrote:
>>>> Andy Lutomirski <luto@amacapital.net> writes:
>>>>
>>>>> The change:
>>>>>
>>>>> commit f4e0c30c191f87851c4a53454abb55ee276f4a7e
>>>>> Author: Al Viro <viro@zeniv.linux.org.uk>
>>>>> Date:   Tue Jun 11 08:34:36 2013 +0400
>>>>>
>>>>>     allow the temp files created by open() to be linked to
>>>>>
>>>>>     O_TMPFILE | O_CREAT => linkat() with AT_SYMLINK_FOLLOW and /proc/self/fd/<n>
>>>>>     as oldpath (i.e. flink()) will create a link
>>>>>     O_TMPFILE | O_CREAT | O_EXCL => ENOENT on attempt to link those guys
>>>>>
>>>>>     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>>>>>
>>>>> makes it possible to hardlink an O_TMPFILE file using procfs.  Should
>>>>> linkat(fd, "", newdirfd, newpath, AT_EMPTY_PATH) also work?
>>>>>
>>>>> AFAICS it currently requires CAP_DAC_READ_SEARCH, but I'm a bit
>>>>> confused as to why linkat(..., AT_EMPTY_PATH) should have a stricter
>>>>> check than linkat(AT_FDCWD, "/proc/self/fd/n", ...,
>>>>> AT_SYMLINK_FOLLOW),  (The relevant change is
>>>>> 11a7b371b64ef39fc5fb1b6f2218eef7c4d035e3.)
>>>>>
>>>>> FWIW, this program works on Linux 3.9, which makes me doubt that the
>>>>> security restriction on linkat is doing any good:
>>>>>
>>>>> #include <stdio.h>
>>>>> #include <err.h>
>>>>> #include <fcntl.h>
>>>>> #include <unistd.h>
>>>>>
>>>>> int main(int argc, char **argv)
>>>>> {
>>>>>   char buf[128];
>>>>>
>>>>>   if (argc != 3)
>>>>>     errx(1, "Usage: flink FD PATH");
>>>>>
>>>>>   sprintf(buf, "/proc/self/fd/%d", atoi(argv[1]));
>>>>>   if (linkat(AT_FDCWD, buf, AT_FDCWD, argv[2], AT_SYMLINK_FOLLOW) != 0)
>>>>>     err(1, "linkat");
>>>>>   return 0;
>>>>> }
>>>>>
>>>>>
>>>>> Removing the check from the AT_EMPTY_PATH case would simplify code
>>>>> that wants to write a file, fsync it, and then flink it in.
>>>>
>>>> I understand that this got merged upstream. But in case of the above
>>>> test we would walk the path pointed by /proc/self/fd/<x> right ?
>>>>
>>>> ie,
>>>>
>>>> 20 -> /home/no-access/k
>>>>
>>>> will the above test work ? Now if i pass the '20' to another application
>>>> I can affectively create a hardlink to that outside no-access and if k
>>>> happens to have 'r' for others, then everybody will be able to read
>>>> right ?. I understand that limitting the read access based on directory
>>>> permission is not a good idea. But aren't we expected to keep that ?
>>>
>>> The symlinks in /proc/self/fd are rather magical and don't actually
>>> walk the path.  Give it a try :)
>>>
>>
>> How about fd passed from one application to another(say from a1 to
>> a2). a2 won't have read permission on /proc/a1/fd/ and also don't know
>> the value of file descriptor he should use right ? Will the /proc/self/fd
>> method work in such case ? IIUC with AT_EMPTY_PATH a2 can create the
>> link in the above case right ? So if /proc/self/fd doesn't work should
>> we allow that ?
>
> Hmm I guess a2 will be able to use /proc/a2/fd/<received_fd> to create a
> link ?

Yes.

-- 
Andy Lutomirski
AMA Capital Management, LLC