From mboxrd@z Thu Jan 1 00:00:00 1970 From: serge@hallyn.com (Serge E. Hallyn) Date: Thu, 28 Jun 2018 12:26:48 -0500 Subject: [PATCH] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias() In-Reply-To: References: <1530082758.30361.7.camel@mtkswgap22> <20180628150152.GA10398@mail.hallyn.com> Message-ID: <20180628172648.GA12196@mail.hallyn.com> To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org Quoting Amir Goldstein (amir73il at gmail.com): > On Thu, Jun 28, 2018 at 6:01 PM, Serge E. Hallyn 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 > >> 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 > 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