cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

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 <[email protected]>

Fixes 8db6c34f1dbc ("Introduce v3 namespaced file capabilities")

Signed-off-by Eddie Horng <[email protected]>

---
 security/commoncap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

security/commoncap.c +0 -0

index 1ce701fcb3f3..147f6131842a 100644

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 <[email protected]>

Fixes 8db6c34f1dbc ("Introduce v3 namespaced file capabilities")

Signed-off-by Eddie Horng <[email protected]>

---
 security/commoncap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

security/commoncap.c +1 -1

index 1ce701fcb3f3..147f6131842a 100644
@@ -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;

Signature · 2 lines

-- 
2.12.5

Quoted 36 lines · Eddie.Horng

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

Quoted 2 lines

Suggested-by Amir Goldstein <[email protected]>

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.

Quoted 37 lines · Serge E. Hallyn

Quoted 36 lines · Eddie.Horng

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

Quoted 5 lines

Quoted 2 lines

Suggested-by Amir Goldstein <[email protected]>

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.

Quoted 6 lines

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.

Quoted 47 lines · Amir Goldstein

Quoted 37 lines · Serge E. Hallyn

Quoted 36 lines · Eddie.Horng

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

Quoted 5 lines

Quoted 2 lines

Suggested-by Amir Goldstein <[email protected]>

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

Quoted 8 lines

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.

Quoted 8 lines

Quoted 6 lines

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 <[email protected]>

Quoted 3 lines

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.

Quoted 10 lines

Quoted 1 line

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 <[email protected]>

Quoted 9 lines

Quoted 3 lines

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.

Quoted 11 lines · Serge E. Hallyn

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.

Quoted 1 line

-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".

Quoted 12 lines · Eddie.Horng

Quoted 11 lines · Serge E. Hallyn

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.

Quoted 7 lines

Quoted 1 line

-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,