| Message ID | 1502904620-20075-3-git-send-email-zohar@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 8/16/2017 10:30 AM, Mimi Zohar wrote: > IMA measures a file, verifies a file's integrity, and caches the > results. On filesystems with MS_I_VERSION enabled, IMA can detect > file changes and cause them to be re-measured and verified. On > filesystems without MS_I_VERSION enabled, files are measured and > verified just once. > > This patch logs filesystems mounted without MS_I_VERSION. > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > --- > include/linux/ima.h | 5 +++++ > security/integrity/ima/ima_main.c | 44 +++++++++++++++++++++++++++++++++++++++ > security/security.c | 1 + > 3 files changed, 50 insertions(+) > > diff --git a/include/linux/ima.h b/include/linux/ima.h > index 0e4647e0eb60..4475cb01149c 100644 > --- a/include/linux/ima.h > +++ b/include/linux/ima.h > @@ -23,6 +23,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id); > extern int ima_post_read_file(struct file *file, void *buf, loff_t size, > enum kernel_read_file_id id); > extern void ima_post_path_mknod(struct dentry *dentry); > +extern void ima_sb_post_new_mount(const struct vfsmount *newmnt, > + const struct path *path); > > #ifdef CONFIG_IMA_KEXEC > extern void ima_add_kexec_buffer(struct kimage *image); > @@ -65,6 +67,9 @@ static inline void ima_post_path_mknod(struct dentry *dentry) > return; > } > > +static inline void ima_sb_post_new_mount(const struct vfsmount *newmnt, > + const struct path *path) > +{ } > #endif /* CONFIG_IMA */ > > #ifndef CONFIG_IMA_KEXEC > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index b00186914df8..a0a685189001 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -354,6 +354,50 @@ void ima_post_path_mknod(struct dentry *dentry) > } > > /** > + * ima_sb_post_new_mount - check filesystem mounted flags > + * > + * Indicate that filesystem isn't mounted with i_version enabled. > + */ > +void ima_sb_post_new_mount(const struct vfsmount *newmnt, > + const struct path *path) > +{ > + struct super_block *sb; > + unsigned long pseudo_fs[] = {CGROUP_SUPER_MAGIC, CGROUP2_SUPER_MAGIC, > + SYSFS_MAGIC, DEVPTS_SUPER_MAGIC, PSTOREFS_MAGIC, EFIVARFS_MAGIC, > + DEBUGFS_MAGIC, TMPFS_MAGIC}; > + char *pathbuf = NULL; > + char filename[NAME_MAX]; > + const char *pathname; > + bool found = 0; > + int i; > + > + sb = newmnt ? newmnt->mnt_sb : path->mnt->mnt_sb; > + > + if ((sb->s_flags & MS_I_VERSION) || (sb->s_flags & MS_RDONLY) || > + (sb->s_flags & MS_KERNMOUNT)) > + return; > + > + for (i = 0; i < ARRAY_SIZE(pseudo_fs); i++) { > + if (pseudo_fs[i] != sb->s_magic) > + continue; > + > + found = 1; > + break; > + } > + if (found) > + return; > + > + pathname = ima_d_path(path, &pathbuf, filename); > + if (!pathname) > + return; > + > + if (newmnt) > + pr_warn("ima: %s mounted without i_version enabled\n", > + pathname); > + __putname(pathbuf); > +} > + > +/** > * ima_read_file - pre-measure/appraise hook decision based on policy > * @file: pointer to the file to be measured/appraised/audit > * @read_id: caller identifier > diff --git a/security/security.c b/security/security.c > index 592153e8d2b6..79111141b383 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -402,6 +402,7 @@ void security_sb_post_new_mount(const struct vfsmount *newmnt, > const struct path *path) > { > call_void_hook(sb_post_new_mount, newmnt, path); > + ima_sb_post_new_mount(newmnt, path); Have you thought about converting the IMA code into a security module that gets loaded after all the others? That would make this (and several other similar instances) bit of special case code go away. > } > > int security_sb_umount(struct vfsmount *mnt, int flags)
On Wed, 2017-08-16 at 12:24 -0700, Casey Schaufler wrote: > On 8/16/2017 10:30 AM, Mimi Zohar wrote: > > diff --git a/security/security.c b/security/security.c > > index 592153e8d2b6..79111141b383 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -402,6 +402,7 @@ void security_sb_post_new_mount(const struct vfsmount *newmnt, > > const struct path *path) > > { > > call_void_hook(sb_post_new_mount, newmnt, path); > > + ima_sb_post_new_mount(newmnt, path); > > Have you thought about converting the IMA code into > a security module that gets loaded after all the others? > That would make this (and several other similar instances) > bit of special case code go away. Yes, each time I need to add another IMA call, where there isn't an existing LSM hook, a decision needs to be made as to whether it needs to be a generic LSM hook or not. Assuming we made IMA an LSM module, what would we do with these other calls? Would they need to be converted to LSM hooks? (Are all LSMs visited, even if an earlier LSM fails? Or does the first LSM failure, stop the LSM traversal?) Unlike LSMs which are sharing the i_sec, IMA doesn't have an entry in the inode, but does an rbtree lookup to access the associated data. Having an i_sec would simplify a lot of the code, but making this sort of change would be a major undertaking. In this context, I'm not sure what you mean by "loaded". IMA needs to be enabled from the very beginning to capture all measurements and verify the integrity of files, without any gaps. At some point this would include other LSM policies. IMA certainly cannot be loaded late like kernel modules. Similarly, we would need to think about EVM. Mimi
On Wed, 16 Aug 2017, Mimi Zohar wrote: > In this context, I'm not sure what you mean by "loaded". IMA needs to > be enabled from the very beginning to capture all measurements and > verify the integrity of files, without any gaps. At some point this > would include other LSM policies. I think it's better to keep IMA orthogonal to LSM for this reason. The original motivation to implement IMA as a separate API was because LSM was at the time considered specific to access control mechanisms, although that is not the case now.
On Wed, 2017-08-16 at 13:30 -0400, Mimi Zohar wrote: > IMA measures a file, verifies a file's integrity, and caches the > results. On filesystems with MS_I_VERSION enabled, IMA can detect > file changes and cause them to be re-measured and verified. On > filesystems without MS_I_VERSION enabled, files are measured and > verified just once. > > This patch logs filesystems mounted without MS_I_VERSION. > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > --- > include/linux/ima.h | 5 +++++ > security/integrity/ima/ima_main.c | 44 +++++++++++++++++++++++++++++++++++++++ > security/security.c | 1 + > 3 files changed, 50 insertions(+) > Sorry for the late review. I just started dusting off my i_version rework, and noticed that IMA still has unaddressed problems here. > diff --git a/include/linux/ima.h b/include/linux/ima.h > index 0e4647e0eb60..4475cb01149c 100644 > --- a/include/linux/ima.h > +++ b/include/linux/ima.h > @@ -23,6 +23,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id); > extern int ima_post_read_file(struct file *file, void *buf, loff_t size, > enum kernel_read_file_id id); > extern void ima_post_path_mknod(struct dentry *dentry); > +extern void ima_sb_post_new_mount(const struct vfsmount *newmnt, > + const struct path *path); > > #ifdef CONFIG_IMA_KEXEC > extern void ima_add_kexec_buffer(struct kimage *image); > @@ -65,6 +67,9 @@ static inline void ima_post_path_mknod(struct dentry *dentry) > return; > } > > +static inline void ima_sb_post_new_mount(const struct vfsmount *newmnt, > + const struct path *path) > +{ } > #endif /* CONFIG_IMA */ > > #ifndef CONFIG_IMA_KEXEC > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index b00186914df8..a0a685189001 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -354,6 +354,50 @@ void ima_post_path_mknod(struct dentry *dentry) > } > > /** > + * ima_sb_post_new_mount - check filesystem mounted flags > + * > + * Indicate that filesystem isn't mounted with i_version enabled. > + */ > +void ima_sb_post_new_mount(const struct vfsmount *newmnt, > + const struct path *path) > +{ > + struct super_block *sb; > + unsigned long pseudo_fs[] = {CGROUP_SUPER_MAGIC, CGROUP2_SUPER_MAGIC, > + SYSFS_MAGIC, DEVPTS_SUPER_MAGIC, PSTOREFS_MAGIC, EFIVARFS_MAGIC, > + DEBUGFS_MAGIC, TMPFS_MAGIC}; Barf. What about procfs? This looks like something that will very subject to bitrot. > + char *pathbuf = NULL; > + char filename[NAME_MAX]; > + const char *pathname; > + bool found = 0; > + int i; > + > + sb = newmnt ? newmnt->mnt_sb : path->mnt->mnt_sb; > + > + if ((sb->s_flags & MS_I_VERSION) || (sb->s_flags & MS_RDONLY) || > + (sb->s_flags & MS_KERNMOUNT)) > + return; > + > + for (i = 0; i < ARRAY_SIZE(pseudo_fs); i++) { > + if (pseudo_fs[i] != sb->s_magic) > + continue; > + > + found = 1; > + break; > + } > + if (found) > + return; > + > + pathname = ima_d_path(path, &pathbuf, filename); > + if (!pathname) > + return; > + > + if (newmnt) > + pr_warn("ima: %s mounted without i_version enabled\n", > + pathname); > + __putname(pathbuf); > +} > + > +/** > * ima_read_file - pre-measure/appraise hook decision based on policy > * @file: pointer to the file to be measured/appraised/audit > * @read_id: caller identifier > diff --git a/security/security.c b/security/security.c > index 592153e8d2b6..79111141b383 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -402,6 +402,7 @@ void security_sb_post_new_mount(const struct vfsmount *newmnt, > const struct path *path) > { > call_void_hook(sb_post_new_mount, newmnt, path); > + ima_sb_post_new_mount(newmnt, path); > } > > int security_sb_umount(struct vfsmount *mnt, int flags) Personally, I'm not a huge fan of this scheme. It seems quite invasive, and doesn't really seem to address the stated problem well. The warning itself seems ok, but I don't really see what's wrong with performing remeasurement when the mtime changes on filesystems that don't have SB_I_VERSION set. Surely that's better than limiting it to an initial measurement? Maybe I just don't understand what you're really trying to achieve here.
Hi Jeff, [The IMA/EVM and the TPM mailing lists have been combined as a single linux-integrity mailing list.] On Thu, 2017-12-07 at 07:26 -0500, Jeff Layton wrote: > Sorry for the late review. I just started dusting off my i_version > rework, and noticed that IMA still has unaddressed problems here. <snip> > Personally, I'm not a huge fan of this scheme. It seems quite invasive, > and doesn't really seem to address the stated problem well. A cleaned up version of this patch set was meant to follow the introduction of a new integrity_read method, but that patch set was rejected. At this point, I have no intentions of upstreaming a cleaned up version this patch set either. > The warning itself seems ok, but I don't really see what's wrong with > performing remeasurement when the mtime changes on filesystems that > don't have SB_I_VERSION set. Surely that's better than limiting it to an > initial measurement? > > Maybe I just don't understand what you're really trying to achieve here. Based on discussions with Sascha Hauer, he convinced me the i_version test is basically just a performance improvement and posted a patch that checks the filesystem for i_version support, before relying on it - https://www.spinics.net/lists/linux-integrity/msg00033.html. Mimi
On Thu, 2017-12-07 at 09:35 -0500, Mimi Zohar wrote: > Hi Jeff, > > [The IMA/EVM and the TPM mailing lists have been combined as a single > linux-integrity mailing list.] > > On Thu, 2017-12-07 at 07:26 -0500, Jeff Layton wrote: > > Sorry for the late review. I just started dusting off my i_version > > rework, and noticed that IMA still has unaddressed problems here. > > <snip> > > > Personally, I'm not a huge fan of this scheme. It seems quite invasive, > > and doesn't really seem to address the stated problem well. > > A cleaned up version of this patch set was meant to follow the > introduction of a new integrity_read method, but that patch set was > rejected. At this point, I have no intentions of upstreaming a > cleaned up version this patch set either. > > > The warning itself seems ok, but I don't really see what's wrong with > > performing remeasurement when the mtime changes on filesystems that > > don't have SB_I_VERSION set. Surely that's better than limiting it to an > > initial measurement? > > > > Maybe I just don't understand what you're really trying to achieve here. > > Based on discussions with Sascha Hauer, he convinced me the i_version > test is basically just a performance improvement and posted a patch > that checks the filesystem for i_version support, before relying on it > - https://www.spinics.net/lists/linux-integrity/msg00033.html. > > Mimi > Thanks for the link. That patch looks good to me. Any idea when and if it will be merged? Thanks,
On Thu, 2017-12-07 at 09:50 -0500, Jeff Layton wrote: > On Thu, 2017-12-07 at 09:35 -0500, Mimi Zohar wrote: > > Hi Jeff, > > > > [The IMA/EVM and the TPM mailing lists have been combined as a single > > linux-integrity mailing list.] > > > > On Thu, 2017-12-07 at 07:26 -0500, Jeff Layton wrote: > > > Sorry for the late review. I just started dusting off my i_version > > > rework, and noticed that IMA still has unaddressed problems here. > > > > <snip> > > > > > Personally, I'm not a huge fan of this scheme. It seems quite invasive, > > > and doesn't really seem to address the stated problem well. > > > > A cleaned up version of this patch set was meant to follow the > > introduction of a new integrity_read method, but that patch set was > > rejected. At this point, I have no intentions of upstreaming a > > cleaned up version this patch set either. > > > > > The warning itself seems ok, but I don't really see what's wrong with > > > performing remeasurement when the mtime changes on filesystems that > > > don't have SB_I_VERSION set. Surely that's better than limiting it to an > > > initial measurement? > > > > > > Maybe I just don't understand what you're really trying to achieve here. > > > > Based on discussions with Sascha Hauer, he convinced me the i_version > > test is basically just a performance improvement and posted a patch > > that checks the filesystem for i_version support, before relying on it > > - https://www.spinics.net/lists/linux-integrity/msg00033.html. > > > > Mimi > > > > Thanks for the link. That patch looks good to me. Any idea when and if > it will be merged? Is that an Ack? Barring any testing issues, I'll upstream it with yours in the next open window. Mimi
On Thu, 2017-12-07 at 10:08 -0500, Mimi Zohar wrote: > On Thu, 2017-12-07 at 09:50 -0500, Jeff Layton wrote: > > On Thu, 2017-12-07 at 09:35 -0500, Mimi Zohar wrote: > > > Hi Jeff, > > > > > > [The IMA/EVM and the TPM mailing lists have been combined as a single > > > linux-integrity mailing list.] > > > > > > On Thu, 2017-12-07 at 07:26 -0500, Jeff Layton wrote: > > > > Sorry for the late review. I just started dusting off my i_version > > > > rework, and noticed that IMA still has unaddressed problems here. > > > > > > <snip> > > > > > > > Personally, I'm not a huge fan of this scheme. It seems quite invasive, > > > > and doesn't really seem to address the stated problem well. > > > > > > A cleaned up version of this patch set was meant to follow the > > > introduction of a new integrity_read method, but that patch set was > > > rejected. At this point, I have no intentions of upstreaming a > > > cleaned up version this patch set either. > > > > > > > The warning itself seems ok, but I don't really see what's wrong with > > > > performing remeasurement when the mtime changes on filesystems that > > > > don't have SB_I_VERSION set. Surely that's better than limiting it to an > > > > initial measurement? > > > > > > > > Maybe I just don't understand what you're really trying to achieve here. > > > > > > Based on discussions with Sascha Hauer, he convinced me the i_version > > > test is basically just a performance improvement and posted a patch > > > that checks the filesystem for i_version support, before relying on it > > > - https://www.spinics.net/lists/linux-integrity/msg00033.html. > > > > > > Mimi > > > > > > > Thanks for the link. That patch looks good to me. Any idea when and if > > it will be merged? > > Is that an Ack? Barring any testing issues, I'll upstream it with > yours in the next open window. > > Mimi > Sure, you can add: Reviewed-by: Jeff Layton <jlayton@redhat.com>
On Thu, 2017-12-07 at 10:09 -0500, Jeff Layton wrote: > On Thu, 2017-12-07 at 10:08 -0500, Mimi Zohar wrote: > > On Thu, 2017-12-07 at 09:50 -0500, Jeff Layton wrote: > > > On Thu, 2017-12-07 at 09:35 -0500, Mimi Zohar wrote: > > > > Hi Jeff, > > > > > > > > [The IMA/EVM and the TPM mailing lists have been combined as a single > > > > linux-integrity mailing list.] > > > > > > > > On Thu, 2017-12-07 at 07:26 -0500, Jeff Layton wrote: > > > > > Sorry for the late review. I just started dusting off my i_version > > > > > rework, and noticed that IMA still has unaddressed problems here. > > > > > > > > <snip> > > > > > > > > > Personally, I'm not a huge fan of this scheme. It seems quite invasive, > > > > > and doesn't really seem to address the stated problem well. > > > > > > > > A cleaned up version of this patch set was meant to follow the > > > > introduction of a new integrity_read method, but that patch set was > > > > rejected. At this point, I have no intentions of upstreaming a > > > > cleaned up version this patch set either. > > > > > > > > > The warning itself seems ok, but I don't really see what's wrong with > > > > > performing remeasurement when the mtime changes on filesystems that > > > > > don't have SB_I_VERSION set. Surely that's better than limiting it to an > > > > > initial measurement? > > > > > > > > > > Maybe I just don't understand what you're really trying to achieve here. > > > > > > > > Based on discussions with Sascha Hauer, he convinced me the i_version > > > > test is basically just a performance improvement and posted a patch > > > > that checks the filesystem for i_version support, before relying on it > > > > - https://www.spinics.net/lists/linux-integrity/msg00033.html. > > > > > > > > Mimi > > > > > > > > > > Thanks for the link. That patch looks good to me. Any idea when and if > > > it will be merged? > > > > Is that an Ack? Barring any testing issues, I'll upstream it with > > yours in the next open window. > > > > Mimi > > > > Sure, you can add: > > Reviewed-by: Jeff Layton <jlayton@redhat.com> BTW, could you get this into linux-next sometime soon? I have a series of patches to overhaul i_version handling that I want to go in soon and there could be merge conflicts. Thanks,
diff --git a/include/linux/ima.h b/include/linux/ima.h index 0e4647e0eb60..4475cb01149c 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -23,6 +23,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id); extern int ima_post_read_file(struct file *file, void *buf, loff_t size, enum kernel_read_file_id id); extern void ima_post_path_mknod(struct dentry *dentry); +extern void ima_sb_post_new_mount(const struct vfsmount *newmnt, + const struct path *path); #ifdef CONFIG_IMA_KEXEC extern void ima_add_kexec_buffer(struct kimage *image); @@ -65,6 +67,9 @@ static inline void ima_post_path_mknod(struct dentry *dentry) return; } +static inline void ima_sb_post_new_mount(const struct vfsmount *newmnt, + const struct path *path) +{ } #endif /* CONFIG_IMA */ #ifndef CONFIG_IMA_KEXEC diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index b00186914df8..a0a685189001 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -354,6 +354,50 @@ void ima_post_path_mknod(struct dentry *dentry) } /** + * ima_sb_post_new_mount - check filesystem mounted flags + * + * Indicate that filesystem isn't mounted with i_version enabled. + */ +void ima_sb_post_new_mount(const struct vfsmount *newmnt, + const struct path *path) +{ + struct super_block *sb; + unsigned long pseudo_fs[] = {CGROUP_SUPER_MAGIC, CGROUP2_SUPER_MAGIC, + SYSFS_MAGIC, DEVPTS_SUPER_MAGIC, PSTOREFS_MAGIC, EFIVARFS_MAGIC, + DEBUGFS_MAGIC, TMPFS_MAGIC}; + char *pathbuf = NULL; + char filename[NAME_MAX]; + const char *pathname; + bool found = 0; + int i; + + sb = newmnt ? newmnt->mnt_sb : path->mnt->mnt_sb; + + if ((sb->s_flags & MS_I_VERSION) || (sb->s_flags & MS_RDONLY) || + (sb->s_flags & MS_KERNMOUNT)) + return; + + for (i = 0; i < ARRAY_SIZE(pseudo_fs); i++) { + if (pseudo_fs[i] != sb->s_magic) + continue; + + found = 1; + break; + } + if (found) + return; + + pathname = ima_d_path(path, &pathbuf, filename); + if (!pathname) + return; + + if (newmnt) + pr_warn("ima: %s mounted without i_version enabled\n", + pathname); + __putname(pathbuf); +} + +/** * ima_read_file - pre-measure/appraise hook decision based on policy * @file: pointer to the file to be measured/appraised/audit * @read_id: caller identifier diff --git a/security/security.c b/security/security.c index 592153e8d2b6..79111141b383 100644 --- a/security/security.c +++ b/security/security.c @@ -402,6 +402,7 @@ void security_sb_post_new_mount(const struct vfsmount *newmnt, const struct path *path) { call_void_hook(sb_post_new_mount, newmnt, path); + ima_sb_post_new_mount(newmnt, path); } int security_sb_umount(struct vfsmount *mnt, int flags)
IMA measures a file, verifies a file's integrity, and caches the results. On filesystems with MS_I_VERSION enabled, IMA can detect file changes and cause them to be re-measured and verified. On filesystems without MS_I_VERSION enabled, files are measured and verified just once. This patch logs filesystems mounted without MS_I_VERSION. Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> --- include/linux/ima.h | 5 +++++ security/integrity/ima/ima_main.c | 44 +++++++++++++++++++++++++++++++++++++++ security/security.c | 1 + 3 files changed, 50 insertions(+)