| Message ID | 20220922151728.1557914-11-brauner@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | None | expand |
On Thu, Sep 22, 2022 at 11:18 AM Christian Brauner <brauner@kernel.org> wrote: > > The current way of setting and getting posix acls through the generic > xattr interface is error prone and type unsafe. The vfs needs to > interpret and fixup posix acls before storing or reporting it to > userspace. Various hacks exist to make this work. The code is hard to > understand and difficult to maintain in it's current form. Instead of > making this work by hacking posix acls through xattr handlers we are > building a dedicated posix acl api around the get and set inode > operations. This removes a lot of hackiness and makes the codepaths > easier to maintain. A lot of background can be found in [1]. > > So far posix acls were passed as a void blob to the security and > integrity modules. Some of them like evm then proceed to interpret the > void pointer and convert it into the kernel internal struct posix acl > representation to perform their integrity checking magic. This is > obviously pretty problematic as that requires knowledge that only the > vfs is guaranteed to have and has lead to various bugs. Add a proper > security hook for setting posix acls and pass down the posix acls in > their appropriate vfs format instead of hacking it through a void > pointer stored in the uapi format. > > I spent considerate time in the security module infrastructure and > audited all codepaths. SELinux has no restrictions based on the posix > acl values passed through it. The capability hook doesn't need to be > called either because it only has restrictions on security.* xattrs. So > this all becomes a very simple hook for SELinux. > > Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1] > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > --- > security/selinux/hooks.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 79573504783b..bbc0ce3bde35 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3239,6 +3239,13 @@ static int selinux_inode_setxattr(struct user_namespace *mnt_userns, > &ad); > } > > +static int selinux_inode_set_acl(struct user_namespace *mnt_userns, > + struct dentry *dentry, const char *acl_name, > + struct posix_acl *kacl) > +{ > + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); > +} > + > static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name, > const void *value, size_t size, > int flags) > @@ -7063,6 +7070,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(inode_getxattr, selinux_inode_getxattr), > LSM_HOOK_INIT(inode_listxattr, selinux_inode_listxattr), > LSM_HOOK_INIT(inode_removexattr, selinux_inode_removexattr), > + LSM_HOOK_INIT(inode_set_acl, selinux_inode_set_acl), See my other reply about needing to see the full patchset in order to properly review the changes, but one thing immediately jumped out at me when looking at this: why is the LSM hook "security_inode_set_acl()" when we are passing a dentry instead of an inode? We don't have a lot of them, but there are `security_dentry_*()` LSM hooks in the existing kernel code.
On Thu, Sep 22, 2022 at 01:16:57PM -0400, Paul Moore wrote: > properly review the changes, but one thing immediately jumped out at > me when looking at this: why is the LSM hook > "security_inode_set_acl()" when we are passing a dentry instead of an > inode? We don't have a lot of them, but there are > `security_dentry_*()` LSM hooks in the existing kernel code. I'm no LSM expert, but isn't the inode vs dentry for if it is related to an inode operation or dentry operation, not about that the first argument is?
On Fri, Sep 23, 2022 at 08:47:07AM +0200, Christoph Hellwig wrote: > On Thu, Sep 22, 2022 at 01:16:57PM -0400, Paul Moore wrote: > > properly review the changes, but one thing immediately jumped out at > > me when looking at this: why is the LSM hook > > "security_inode_set_acl()" when we are passing a dentry instead of an > > inode? We don't have a lot of them, but there are > > `security_dentry_*()` LSM hooks in the existing kernel code. > > I'm no LSM expert, but isn't the inode vs dentry for if it is > related to an inode operation or dentry operation, not about that > the first argument is? Indeed. For example, void security_inode_post_setxattr(struct dentry *dentry, const char *name, const void *value, size_t size, int flags) { if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) return; call_void_hook(inode_post_setxattr, dentry, name, value, size, flags); evm_inode_post_setxattr(dentry, name, value, size); } int security_inode_getxattr(struct dentry *dentry, const char *name) { if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) return 0; return call_int_hook(inode_getxattr, 0, dentry, name); } int security_inode_listxattr(struct dentry *dentry) { if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) return 0; return call_int_hook(inode_listxattr, 0, dentry); }
On Fri, Sep 23, 2022 at 3:57 AM Christian Brauner <brauner@kernel.org> wrote: > On Fri, Sep 23, 2022 at 08:47:07AM +0200, Christoph Hellwig wrote: > > On Thu, Sep 22, 2022 at 01:16:57PM -0400, Paul Moore wrote: > > > properly review the changes, but one thing immediately jumped out at > > > me when looking at this: why is the LSM hook > > > "security_inode_set_acl()" when we are passing a dentry instead of an > > > inode? We don't have a lot of them, but there are > > > `security_dentry_*()` LSM hooks in the existing kernel code. > > > > I'm no LSM expert, but isn't the inode vs dentry for if it is > > related to an inode operation or dentry operation, not about that > > the first argument is? > > Indeed. For example ... If the goal is for this LSM hook to operate on an inode and not a dentry, let's pass it an inode instead. This should help prevent misuse and I suspect the individual implementations will be quicker for it anyway (it should be the case for SELinux, and while I'm not a Smack expert it looks to be true for Smack as well). There is the potential for some additional work in the case where the inode needs to be revalidated (I believe this is only a SELinux issue at the moment) as well as if an audit event needs to be generated, but these should both happen infrequently enough that I don't believe they are a real concern.
On Fri, Sep 23, 2022 at 10:26:35AM -0400, Paul Moore wrote: > On Fri, Sep 23, 2022 at 3:57 AM Christian Brauner <brauner@kernel.org> wrote: > > On Fri, Sep 23, 2022 at 08:47:07AM +0200, Christoph Hellwig wrote: > > > On Thu, Sep 22, 2022 at 01:16:57PM -0400, Paul Moore wrote: > > > > properly review the changes, but one thing immediately jumped out at > > > > me when looking at this: why is the LSM hook > > > > "security_inode_set_acl()" when we are passing a dentry instead of an > > > > inode? We don't have a lot of them, but there are > > > > `security_dentry_*()` LSM hooks in the existing kernel code. > > > > > > I'm no LSM expert, but isn't the inode vs dentry for if it is > > > related to an inode operation or dentry operation, not about that > > > the first argument is? > > > > Indeed. For example ... > > If the goal is for this LSM hook to operate on an inode and not a > dentry, let's pass it an inode instead. This should help prevent I would be ok with that but EVM requires a dentry being passed and as evm is called from security_inode_set_acl() exactly like it is from security_inode_setxattr() and similar the hook has to take a dentry. And I want to minimize - ideally get rid of at some point - separate calls to security_*() and evm_*() or ima_() in the vfs. So the evm hook should please stay in there.
On Fri, Sep 23, 2022 at 10:35 AM Christian Brauner <brauner@kernel.org> wrote: > On Fri, Sep 23, 2022 at 10:26:35AM -0400, Paul Moore wrote: > > On Fri, Sep 23, 2022 at 3:57 AM Christian Brauner <brauner@kernel.org> wrote: > > > On Fri, Sep 23, 2022 at 08:47:07AM +0200, Christoph Hellwig wrote: > > > > On Thu, Sep 22, 2022 at 01:16:57PM -0400, Paul Moore wrote: > > > > > properly review the changes, but one thing immediately jumped out at > > > > > me when looking at this: why is the LSM hook > > > > > "security_inode_set_acl()" when we are passing a dentry instead of an > > > > > inode? We don't have a lot of them, but there are > > > > > `security_dentry_*()` LSM hooks in the existing kernel code. > > > > > > > > I'm no LSM expert, but isn't the inode vs dentry for if it is > > > > related to an inode operation or dentry operation, not about that > > > > the first argument is? > > > > > > Indeed. For example ... > > > > If the goal is for this LSM hook to operate on an inode and not a > > dentry, let's pass it an inode instead. This should help prevent > > I would be ok with that but EVM requires a dentry being passed and as > evm is called from security_inode_set_acl() exactly like it is from > security_inode_setxattr() and similar the hook has to take a dentry. If a dentry is truly needed by EVM (a quick look indicates that it may just be for the VFS getxattr API, but I haven't traced the full code path), then I'm having a hard time reconciling that this isn't a dentry operation. Yes, I get that the ACLs belong to the inode and not the dentry, but then why do we need the dentry? It seems like the interfaces are broken slightly, or at least a little odd ... <shrug> > And I want to minimize - ideally get rid of at some point - separate > calls to security_*() and evm_*() or ima_() in the vfs. So the evm hook > should please stay in there. For the record, I want to get rid of the IMA and EVM specific hooks in the kernel. They were a necessity back when there could only be one LSM active at a given time, but with that no longer the case I see little reason why IMA/EVM/etc. remain separate; it makes the code worse and complicates a lot of things both at the LSM layer as well as the rest of the kernel. I've mentioned this to a few people, including Mimi, and it came up during at talk at LPC this year.
On Fri, Sep 23, 2022 at 01:35:08PM -0400, Paul Moore wrote: > On Fri, Sep 23, 2022 at 10:35 AM Christian Brauner <brauner@kernel.org> wrote: > > On Fri, Sep 23, 2022 at 10:26:35AM -0400, Paul Moore wrote: > > > On Fri, Sep 23, 2022 at 3:57 AM Christian Brauner <brauner@kernel.org> wrote: > > > > On Fri, Sep 23, 2022 at 08:47:07AM +0200, Christoph Hellwig wrote: > > > > > On Thu, Sep 22, 2022 at 01:16:57PM -0400, Paul Moore wrote: > > > > > > properly review the changes, but one thing immediately jumped out at > > > > > > me when looking at this: why is the LSM hook > > > > > > "security_inode_set_acl()" when we are passing a dentry instead of an > > > > > > inode? We don't have a lot of them, but there are > > > > > > `security_dentry_*()` LSM hooks in the existing kernel code. > > > > > > > > > > I'm no LSM expert, but isn't the inode vs dentry for if it is > > > > > related to an inode operation or dentry operation, not about that > > > > > the first argument is? > > > > > > > > Indeed. For example ... > > > > > > If the goal is for this LSM hook to operate on an inode and not a > > > dentry, let's pass it an inode instead. This should help prevent > > > > I would be ok with that but EVM requires a dentry being passed and as > > evm is called from security_inode_set_acl() exactly like it is from > > security_inode_setxattr() and similar the hook has to take a dentry. > > If a dentry is truly needed by EVM (a quick look indicates that it may > just be for the VFS getxattr API, but I haven't traced the full code > path), then I'm having a hard time reconciling that this isn't a > dentry operation. Yes, I get that the ACLs belong to the inode and > not the dentry, but then why do we need the dentry? It seems like the > interfaces are broken slightly, or at least a little odd ... <shrug> There's multiple reasons for the generic xattr api to take a dentry. For example, there are quite a few filesystems that require dentry access during (specific or all) xattr operations. So ideally, we'd just want to pass the dentry I'd say. But we can't do that because of security modules. Some security modules call security_d_instantiate() which in turn calls __vfs_{g,s}et_xattr() in the hook implementation. That's at least true of SELinux and Smack iirc. They want dentry and inode but security_d_instantiate() is called in e.g., d_instantiate and d_add() before the inode is attached to the dentry: selinux_d_instantiate() -> inode_doinit_with_dentry() -> inode_doinit_use_xattr() -> __vfs_getxattr() smack_d_instantiate() -> __vfs_getxattr() -> __vfs_setxattr() So that mandates both dentry and inode in vfs xattr helpers. I don't think we can and want to solve this in this patchset. For now we can stick with the naming as set by precedent and then in the future the security modules can decide whether they want to do a rename patchset for most of the xattr hooks at some point. > > > And I want to minimize - ideally get rid of at some point - separate > > calls to security_*() and evm_*() or ima_() in the vfs. So the evm hook > > should please stay in there. > > For the record, I want to get rid of the IMA and EVM specific hooks in > the kernel. They were a necessity back when there could only be one > LSM active at a given time, but with that no longer the case I see > little reason why IMA/EVM/etc. remain separate; it makes the code > worse and complicates a lot of things both at the LSM layer as well as > the rest of the kernel. I've mentioned this to a few people, > including Mimi, and it came up during at talk at LPC this year. Sounds good.
On Mon, Sep 26, 2022 at 5:05 AM Christian Brauner <brauner@kernel.org> wrote: > On Fri, Sep 23, 2022 at 01:35:08PM -0400, Paul Moore wrote: > > On Fri, Sep 23, 2022 at 10:35 AM Christian Brauner <brauner@kernel.org> wrote: > > > On Fri, Sep 23, 2022 at 10:26:35AM -0400, Paul Moore wrote: > > > > On Fri, Sep 23, 2022 at 3:57 AM Christian Brauner <brauner@kernel.org> wrote: > > > > > On Fri, Sep 23, 2022 at 08:47:07AM +0200, Christoph Hellwig wrote: > > > > > > On Thu, Sep 22, 2022 at 01:16:57PM -0400, Paul Moore wrote: > > > > > > > properly review the changes, but one thing immediately jumped out at > > > > > > > me when looking at this: why is the LSM hook > > > > > > > "security_inode_set_acl()" when we are passing a dentry instead of an > > > > > > > inode? We don't have a lot of them, but there are > > > > > > > `security_dentry_*()` LSM hooks in the existing kernel code. > > > > > > > > > > > > I'm no LSM expert, but isn't the inode vs dentry for if it is > > > > > > related to an inode operation or dentry operation, not about that > > > > > > the first argument is? > > > > > > > > > > Indeed. For example ... > > > > > > > > If the goal is for this LSM hook to operate on an inode and not a > > > > dentry, let's pass it an inode instead. This should help prevent > > > > > > I would be ok with that but EVM requires a dentry being passed and as > > > evm is called from security_inode_set_acl() exactly like it is from > > > security_inode_setxattr() and similar the hook has to take a dentry. > > > > If a dentry is truly needed by EVM (a quick look indicates that it may > > just be for the VFS getxattr API, but I haven't traced the full code > > path), then I'm having a hard time reconciling that this isn't a > > dentry operation. Yes, I get that the ACLs belong to the inode and > > not the dentry, but then why do we need the dentry? It seems like the > > interfaces are broken slightly, or at least a little odd ... <shrug> > > There's multiple reasons for the generic xattr api to take a dentry. For > example, there are quite a few filesystems that require dentry access > during (specific or all) xattr operations. So ideally, we'd just want to > pass the dentry I'd say ... Independent of the naming issue discussed above, I want to make it clear that in general I believe that the dentry is more useful to the LSMs if for no other reason that it is pretty trivial to go from a dentry to an inode, whereas the opposite is not true. Of course there are cases where the dentry is not always available, or the connectivity between the dentry and the inode is not yet present. In those cases we would obviously need to pass the inode, or both the inode and the dentry, which gets me to my next point and my motivation behind bringing all this up once we started down the rathole ... My main concern is making sure that the LSM hooks are declared in such a way that they are fairly resistant to misuse; not that I expect any malice, but I do believe accidental misuse of the hooks is legitimate concern. With respect to the various VFS related hooks, one of the things that has always caught my attention in this respect has been hooks which pass both a dentry and a inode for the same file. Of course there are cases where this will always be necessary, but I hate to see bad interfaces continued forward simply because "that's the way we've always done it". In this particular patch the hook prototype is reasonably well defined as far as I'm concerned, with no worries of both a dentry and an inode, so that's good. I still don't really like the name disconnect between the function name and the parameter list (inode vs dentry), but que sera sera; my appetite for argument here is rapidly waning.
On Fri, Sep 23, 2022 at 01:35:08PM -0400, Paul Moore wrote: > If a dentry is truly needed by EVM (a quick look indicates that it may > just be for the VFS getxattr API, but I haven't traced the full code > path), then I'm having a hard time reconciling that this isn't a > dentry operation. Yes, I get that the ACLs belong to the inode and > not the dentry, but then why do we need the dentry? It seems like the > interfaces are broken slightly, or at least a little odd ... <shrug> The dentry_operations are bit misnamed and should probably have been called dcache_operations, that is they are all about managing the dcache state and closely related operations. ACLs aren't like that.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 79573504783b..bbc0ce3bde35 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3239,6 +3239,13 @@ static int selinux_inode_setxattr(struct user_namespace *mnt_userns, &ad); } +static int selinux_inode_set_acl(struct user_namespace *mnt_userns, + struct dentry *dentry, const char *acl_name, + struct posix_acl *kacl) +{ + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); +} + static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name, const void *value, size_t size, int flags) @@ -7063,6 +7070,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(inode_getxattr, selinux_inode_getxattr), LSM_HOOK_INIT(inode_listxattr, selinux_inode_listxattr), LSM_HOOK_INIT(inode_removexattr, selinux_inode_removexattr), + LSM_HOOK_INIT(inode_set_acl, selinux_inode_set_acl), LSM_HOOK_INIT(inode_getsecurity, selinux_inode_getsecurity), LSM_HOOK_INIT(inode_setsecurity, selinux_inode_setsecurity), LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity),
The current way of setting and getting posix acls through the generic xattr interface is error prone and type unsafe. The vfs needs to interpret and fixup posix acls before storing or reporting it to userspace. Various hacks exist to make this work. The code is hard to understand and difficult to maintain in it's current form. Instead of making this work by hacking posix acls through xattr handlers we are building a dedicated posix acl api around the get and set inode operations. This removes a lot of hackiness and makes the codepaths easier to maintain. A lot of background can be found in [1]. So far posix acls were passed as a void blob to the security and integrity modules. Some of them like evm then proceed to interpret the void pointer and convert it into the kernel internal struct posix acl representation to perform their integrity checking magic. This is obviously pretty problematic as that requires knowledge that only the vfs is guaranteed to have and has lead to various bugs. Add a proper security hook for setting posix acls and pass down the posix acls in their appropriate vfs format instead of hacking it through a void pointer stored in the uapi format. I spent considerate time in the security module infrastructure and audited all codepaths. SELinux has no restrictions based on the posix acl values passed through it. The capability hook doesn't need to be called either because it only has restrictions on security.* xattrs. So this all becomes a very simple hook for SELinux. Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1] Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> --- security/selinux/hooks.c | 8 ++++++++ 1 file changed, 8 insertions(+)