[PATCH] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

[PATCH] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

[Amir Goldstein]

[adding Eric because change went through his tree and probably fix will as well]

On Wed, Jun 27, 2018 at 9:59 AM, Eddie.Horng <eddie.horng@mediatek.com> wrote:
>
> The code in cap_inode_getsecurity(), introduced by commit 8db6c34f1dbc
> ("Introduce v3 namespaced file capabilities"), should use
> d_find_any_alias()
> instead of d_find_alias() do handle unhashed dentry correctly. This is
> needed,
> for example, if execveat() is called with an open but unlinked overlayfs
> file, because overlayfs unhashes dentry on unlink.
>

commit message lines should be wrapped at 74 chars I think.
Please run ./scripts/checkpatch.pl on your patch.

> Below reproducer and setup can reproduce the case.
>   const char* exec="echo";
>   const char *newargv[] = { "echo", "hello", NULL};
>   const char *newenviron[] = { NULL };
>   int fd, err;
>
>   fd = open(exec, O_PATH);
>   unlink(exec);
>   err = syscall(322/*SYS_execveat*/, fd, "", newargv, newenviron,
> AT_EMPTY_PATH);
>   if(err<0)
>     fprintf(stderr, "execveat: %s\n", strerror(errno));
>
> gcc compile into ~/test/a.out
> mount -t overlay -orw,lowerdir=/mnt/l,upperdir=/mnt/u,workdir=/mnt/w
> none /mnt/m
> cd /mnt/m
> cp /bin/echo .
> ~/test/a.out
>

Your next challenge, should you choose to accept it, is to write
an LTP test for your reproducer.
Starting points:
- use testcases/kernel/syscalls/inotify/inotify08.c as template
  of test that uses new lib and sets up an overlayfs mount
  (throw away all inotify stuff)
- use testcases/kernel/syscalls/execve/execve_child.c as
  template to clone syscalls/execveat/execveat_child.c
- place your new test at syscalls/execveat/execveat01.c

I'll be happy to help you with up-streaming the test.

> Expected result:
> hello
> Actually result:
> execveat: Invalid argument
> dmesg:
> Invalid argument reading file caps for /dev/fd/3
>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities")
> Signed-off-by: Eddie Horng <eddie.horng@mediatek.com>
> ---
>  security/commoncap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 1ce701fcb3f3..147f6131842a 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -388,7 +388,7 @@ int cap_inode_getsecurity(struct inode *inode, const
> char *name, void **buffer,
>         if (strcmp(name, "capability") != 0)
>                 return -EOPNOTSUPP;
>
> -       dentry = d_find_alias(inode);
> +       dentry = d_find_any_alias(inode);
>         if (!dentry)
>                 return -EINVAL;
>

You may add:
Acked-by: Amir Goldstein <amir73il@gmail.com>

Thanks,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

[Serge E. Hallyn]

Quoting Eddie.Horng (eddie.horng at mediatek.com):
> 
> The code in cap_inode_getsecurity(), introduced by commit 8db6c34f1dbc
> ("Introduce v3 namespaced file capabilities"), should use
> d_find_any_alias()
> instead of d_find_alias() do handle unhashed dentry correctly. This is
> needed,
> for example, if execveat() is called with an open but unlinked overlayfs
> file, because overlayfs unhashes dentry on unlink.
> 
> Below reproducer and setup can reproduce the case.
>   const char* exec="echo";
>   const char *newargv[] = { "echo", "hello", NULL};
>   const char *newenviron[] = { NULL };
>   int fd, err;
> 
>   fd = open(exec, O_PATH);
>   unlink(exec);
>   err = syscall(322/*SYS_execveat*/, fd, "", newargv, newenviron,
> AT_EMPTY_PATH);
>   if(err<0)
>     fprintf(stderr, "execveat: %s\n", strerror(errno));
> 
> gcc compile into ~/test/a.out
> mount -t overlay -orw,lowerdir=/mnt/l,upperdir=/mnt/u,workdir=/mnt/w
> none /mnt/m
> cd /mnt/m
> cp /bin/echo .
> ~/test/a.out
> 
> Expected result:
> hello
> Actually result:
> execveat: Invalid argument
> dmesg:
> Invalid argument reading file caps for /dev/fd/3
> 
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities")

Did 8db6c34f1dbc actually introduce a regression?

Note this does seem to potentially introduce an attack where a
user fetches an open fd to any file with filecaps, waits for a
CVE publication, then after the admin has updated the package
causing the file to be deleted, then does execveat to run the
deleted package with privs.

> Signed-off-by: Eddie Horng <eddie.horng@mediatek.com>
> ---
>  security/commoncap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 1ce701fcb3f3..147f6131842a 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -388,7 +388,7 @@ int cap_inode_getsecurity(struct inode *inode, const
> char *name, void **buffer,
>         if (strcmp(name, "capability") != 0)
>                 return -EOPNOTSUPP;
>  
> -       dentry = d_find_alias(inode);
> +       dentry = d_find_any_alias(inode);
>         if (!dentry)
>                 return -EINVAL;
>  
> -- 
> 2.12.5
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

[Amir Goldstein]

On Thu, Jun 28, 2018 at 6:01 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> Quoting Eddie.Horng (eddie.horng at mediatek.com):
>>
>> The code in cap_inode_getsecurity(), introduced by commit 8db6c34f1dbc
>> ("Introduce v3 namespaced file capabilities"), should use
>> d_find_any_alias()
>> instead of d_find_alias() do handle unhashed dentry correctly. This is
>> needed,
>> for example, if execveat() is called with an open but unlinked overlayfs
>> file, because overlayfs unhashes dentry on unlink.
>>
>> Below reproducer and setup can reproduce the case.
>>   const char* exec="echo";
>>   const char *newargv[] = { "echo", "hello", NULL};
>>   const char *newenviron[] = { NULL };
>>   int fd, err;
>>
>>   fd = open(exec, O_PATH);
>>   unlink(exec);
>>   err = syscall(322/*SYS_execveat*/, fd, "", newargv, newenviron,
>> AT_EMPTY_PATH);
>>   if(err<0)
>>     fprintf(stderr, "execveat: %s\n", strerror(errno));
>>
>> gcc compile into ~/test/a.out
>> mount -t overlay -orw,lowerdir=/mnt/l,upperdir=/mnt/u,workdir=/mnt/w
>> none /mnt/m
>> cd /mnt/m
>> cp /bin/echo .
>> ~/test/a.out
>>
>> Expected result:
>> hello
>> Actually result:
>> execveat: Invalid argument
>> dmesg:
>> Invalid argument reading file caps for /dev/fd/3
>>
>> Suggested-by: Amir Goldstein <amir73il@gmail.com>
>> Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities")
>
> Did 8db6c34f1dbc actually introduce a regression?
>

Yes, it did. The regression was reported to the overlayfs list:
https://marc.info/?l=linux-unionfs&m=152940239421174&w=2

I directed Eddie toward this simple reproducer, but problem
was observed in real life environment.

I realize now that the information about the original regression
is missing from the commit message.
Eddie you may add a link to the mailing list thread in your
commit message, but please do mention the nature of the
regression you reported is a real life application.

> Note this does seem to potentially introduce an attack where a
> user fetches an open fd to any file with filecaps, waits for a
> CVE publication, then after the admin has updated the package
> causing the file to be deleted, then does execveat to run the
> deleted package with privs.
>

Without arguing what the expected behavior should be (I believe
execveat is meant to prevent to exact opposite attack), the change
in this patch does NOT change behavior for ext4 and probably
other local file systems. It *only* changes behavior for overlayfs
and possibly other networking file systems that unhash the dentry
on unlink.

So the attack vector you are referring to exists in current code
as well for local file systems.

Thanks,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

[Serge E. Hallyn]

Quoting Amir Goldstein (amir73il at gmail.com):
> On Thu, Jun 28, 2018 at 6:01 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> > Quoting Eddie.Horng (eddie.horng at mediatek.com):
> >>
> >> The code in cap_inode_getsecurity(), introduced by commit 8db6c34f1dbc
> >> ("Introduce v3 namespaced file capabilities"), should use
> >> d_find_any_alias()
> >> instead of d_find_alias() do handle unhashed dentry correctly. This is
> >> needed,
> >> for example, if execveat() is called with an open but unlinked overlayfs
> >> file, because overlayfs unhashes dentry on unlink.
> >>
> >> Below reproducer and setup can reproduce the case.
> >>   const char* exec="echo";
> >>   const char *newargv[] = { "echo", "hello", NULL};
> >>   const char *newenviron[] = { NULL };
> >>   int fd, err;
> >>
> >>   fd = open(exec, O_PATH);
> >>   unlink(exec);
> >>   err = syscall(322/*SYS_execveat*/, fd, "", newargv, newenviron,
> >> AT_EMPTY_PATH);
> >>   if(err<0)
> >>     fprintf(stderr, "execveat: %s\n", strerror(errno));
> >>
> >> gcc compile into ~/test/a.out
> >> mount -t overlay -orw,lowerdir=/mnt/l,upperdir=/mnt/u,workdir=/mnt/w
> >> none /mnt/m
> >> cd /mnt/m
> >> cp /bin/echo .
> >> ~/test/a.out
> >>
> >> Expected result:
> >> hello
> >> Actually result:
> >> execveat: Invalid argument
> >> dmesg:
> >> Invalid argument reading file caps for /dev/fd/3
> >>
> >> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> >> Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities")
> >
> > Did 8db6c34f1dbc actually introduce a regression?
> >
> 
> Yes, it did. The regression was reported to the overlayfs list:
> https://marc.info/?l=linux-unionfs&m=152940239421174&w=2
> 
> I directed Eddie toward this simple reproducer, but problem
> was observed in real life environment.
> 
> I realize now that the information about the original regression
> is missing from the commit message.
> Eddie you may add a link to the mailing list thread in your
> commit message, but please do mention the nature of the
> regression you reported is a real life application.

Ah, thanks.  I misunderstood, and thought that it was just a case
of namespaced file capabilities not working.

> > Note this does seem to potentially introduce an attack where a
> > user fetches an open fd to any file with filecaps, waits for a
> > CVE publication, then after the admin has updated the package
> > causing the file to be deleted, then does execveat to run the
> > deleted package with privs.
> >
> 
> Without arguing what the expected behavior should be (I believe

Yes, I avoided mentioning that in my first email because I kept
waffling.  It seems like requiring a package manager that cares
to clear the fscaps (maybe just chowning to clear all suid/etc)
is the right thing.  On the other hand demanding extra work from
pre-existing users for a new features is wrong.

Acked-by: Serge Hallyn <serge@hallyn.com>

> execveat is meant to prevent to exact opposite attack), the change
> in this patch does NOT change behavior for ext4 and probably
> other local file systems. It *only* changes behavior for overlayfs

Hm, I'll have to take your word for it - following the code in
vfs_unlink() seems to suggest it's immediately unhashed, and the
ext4 orphan list only holds the inode without any hashed dentries.
But I'm probably mis-reading.

> and possibly other networking file systems that unhash the dentry
> on unlink.
> 
> So the attack vector you are referring to exists in current code
> as well for local file systems.
> 
> Thanks,
> Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

[Amir Goldstein]

On Thu, Jun 28, 2018 at 8:26 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> Quoting Amir Goldstein (amir73il at gmail.com):
>> On Thu, Jun 28, 2018 at 6:01 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
>> > Quoting Eddie.Horng (eddie.horng at mediatek.com):
>> >>
>> >> The code in cap_inode_getsecurity(), introduced by commit 8db6c34f1dbc
>> >> ("Introduce v3 namespaced file capabilities"), should use
>> >> d_find_any_alias()
>> >> instead of d_find_alias() do handle unhashed dentry correctly. This is
>> >> needed,
>> >> for example, if execveat() is called with an open but unlinked overlayfs
>> >> file, because overlayfs unhashes dentry on unlink.
>> >>
>> >> Below reproducer and setup can reproduce the case.
>> >>   const char* exec="echo";
>> >>   const char *newargv[] = { "echo", "hello", NULL};
>> >>   const char *newenviron[] = { NULL };
>> >>   int fd, err;
>> >>
>> >>   fd = open(exec, O_PATH);
>> >>   unlink(exec);
>> >>   err = syscall(322/*SYS_execveat*/, fd, "", newargv, newenviron,
>> >> AT_EMPTY_PATH);
>> >>   if(err<0)
>> >>     fprintf(stderr, "execveat: %s\n", strerror(errno));
>> >>
>> >> gcc compile into ~/test/a.out
>> >> mount -t overlay -orw,lowerdir=/mnt/l,upperdir=/mnt/u,workdir=/mnt/w
>> >> none /mnt/m
>> >> cd /mnt/m
>> >> cp /bin/echo .
>> >> ~/test/a.out
>> >>
>> >> Expected result:
>> >> hello
>> >> Actually result:
>> >> execveat: Invalid argument
>> >> dmesg:
>> >> Invalid argument reading file caps for /dev/fd/3
>> >>
>> >> Suggested-by: Amir Goldstein <amir73il@gmail.com>
>> >> Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities")
>> >
>> > Did 8db6c34f1dbc actually introduce a regression?
>> >
>>
>> Yes, it did. The regression was reported to the overlayfs list:
>> https://marc.info/?l=linux-unionfs&m=152940239421174&w=2
>>
>> I directed Eddie toward this simple reproducer, but problem
>> was observed in real life environment.
>>
>> I realize now that the information about the original regression
>> is missing from the commit message.
>> Eddie you may add a link to the mailing list thread in your
>> commit message, but please do mention the nature of the
>> regression you reported is a real life application.
>
> Ah, thanks.  I misunderstood, and thought that it was just a case
> of namespaced file capabilities not working.
>
>> > Note this does seem to potentially introduce an attack where a
>> > user fetches an open fd to any file with filecaps, waits for a
>> > CVE publication, then after the admin has updated the package
>> > causing the file to be deleted, then does execveat to run the
>> > deleted package with privs.
>> >
>>
>> Without arguing what the expected behavior should be (I believe
>
> Yes, I avoided mentioning that in my first email because I kept
> waffling.  It seems like requiring a package manager that cares
> to clear the fscaps (maybe just chowning to clear all suid/etc)
> is the right thing.  On the other hand demanding extra work from
> pre-existing users for a new features is wrong.
>
> Acked-by: Serge Hallyn <serge@hallyn.com>
>
>> execveat is meant to prevent to exact opposite attack), the change
>> in this patch does NOT change behavior for ext4 and probably
>> other local file systems. It *only* changes behavior for overlayfs
>
> Hm, I'll have to take your word for it - following the code in
> vfs_unlink() seems to suggest it's immediately unhashed, and the
> ext4 orphan list only holds the inode without any hashed dentries.
> But I'm probably mis-reading.
>

Hmm, don't take my word for it, but this is what Eddie reported.
Reproducer works with overlayfs and not with ext4.
I see that d_delete() prefers to keep the dentry hashed but turn it
into a negative dentry if "we are the only user",
which is the case in this reproducer.
But I don't expect that d_find_alias() will find a negative dentry!,
so I can't explain why ext4 passes the reproducer.
Overlayfs OTOH, does explicit d_drop() in file system code,
so it is positive that d_find_any_alias() is needed for overlayfs
as the reproducer shows.

Thanks,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

[Serge E. Hallyn]

Quoting Amir Goldstein (amir73il at gmail.com):
...
> >> Without arguing what the expected behavior should be (I believe
> >
> > Yes, I avoided mentioning that in my first email because I kept
> > waffling.  It seems like requiring a package manager that cares
> > to clear the fscaps (maybe just chowning to clear all suid/etc)
> > is the right thing.  On the other hand demanding extra work from
> > pre-existing users for a new features is wrong.
> >
> > Acked-by: Serge Hallyn <serge@hallyn.com>
> >
> >> execveat is meant to prevent to exact opposite attack), the change
> >> in this patch does NOT change behavior for ext4 and probably
> >> other local file systems. It *only* changes behavior for overlayfs
> >
> > Hm, I'll have to take your word for it - following the code in
> > vfs_unlink() seems to suggest it's immediately unhashed, and the
> > ext4 orphan list only holds the inode without any hashed dentries.
> > But I'm probably mis-reading.
> >
> 
> Hmm, don't take my word for it, but this is what Eddie reported.
> Reproducer works with overlayfs and not with ext4.
> I see that d_delete() prefers to keep the dentry hashed but turn it
> into a negative dentry if "we are the only user",
> which is the case in this reproducer.
> But I don't expect that d_find_alias() will find a negative dentry!,
> so I can't explain why ext4 passes the reproducer.
> Overlayfs OTOH, does explicit d_drop() in file system code,
> so it is positive that d_find_any_alias() is needed for overlayfs
> as the reproducer shows.

Ah - I just tried his reproducer, and in fact got:

0 ? serge at sl ~/test $ getcap execveat
execveat = cap_sys_admin+ep
0 ? serge at sl ~/test $ ./execveat
execveat: Bad file descriptor

on ext4, with 4.15.0-22-generic #24~16.04.1-Ubuntu

Without the filecap, it works.

-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

[Serge E. Hallyn]

On Fri, Jun 29, 2018 at 09:57:11AM +0800, Eddie.Horng wrote:
> On Thu, 2018-06-28 at 13:28 -0500, Serge E. Hallyn wrote:
> > Ah - I just tried his reproducer, and in fact got:
> > 
> > 0 ? serge at sl ~/test $ getcap execveat
> > execveat = cap_sys_admin+ep
> > 0 ? serge at sl ~/test $ ./execveat
> > execveat: Bad file descriptor
> > 
> > on ext4, with 4.15.0-22-generic #24~16.04.1-Ubuntu
> > 
> > Without the filecap, it works.
> > 
> > -serge
> 
> The simple reproducer expected /bin/echo exists in the same 
> dir of execveat executable and does not check the return fd 
> of open("echo", ...). I'm not sure if you run into this case,
> but I tried to run execveat without echo exists, got same result:
> "execveat: Bad file descriptor".

Hah!  Yes, i was in too much of a hurry;  I ran it once with
./echo existing and no caps, that worked;  then i set the caps
on execveat instead of ./echo, and echo had gotten deleted by the
previous test causing the failure like you said.

So, the same thing does happen with setuid anyway, so while that
seems worth addressing one day,

Acked-by: Serge Hallyn <serge@hallyn.com>

thanks,
-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

[Amir Goldstein]

On Fri, Jun 29, 2018 at 5:53 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
> On Fri, Jun 29, 2018 at 09:57:11AM +0800, Eddie.Horng wrote:
>> On Thu, 2018-06-28 at 13:28 -0500, Serge E. Hallyn wrote:
>> > Ah - I just tried his reproducer, and in fact got:
>> >
>> > 0 ? serge at sl ~/test $ getcap execveat
>> > execveat = cap_sys_admin+ep
>> > 0 ? serge at sl ~/test $ ./execveat
>> > execveat: Bad file descriptor
>> >
>> > on ext4, with 4.15.0-22-generic #24~16.04.1-Ubuntu
>> >
>> > Without the filecap, it works.
>> >
>> > -serge
>>
>> The simple reproducer expected /bin/echo exists in the same
>> dir of execveat executable and does not check the return fd
>> of open("echo", ...). I'm not sure if you run into this case,
>> but I tried to run execveat without echo exists, got same result:
>> "execveat: Bad file descriptor".
>
> Hah!  Yes, i was in too much of a hurry;  I ran it once with
> ./echo existing and no caps, that worked;  then i set the caps
> on execveat instead of ./echo, and echo had gotten deleted by the
> previous test causing the failure like you said.
>
> So, the same thing does happen with setuid anyway, so while that
> seems worth addressing one day,
>

Serge,

I misunderstood the bottom line.

Can the problem be reproduced on local fs with/without caps?
and if so, what is missing from reproducer script for that.

Thanks,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html