| Message ID | alpine.LRH.2.21.1802101718330.4382@namei.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 9, 2018 at 10:26 PM, James Morris <jmorris@namei.org> wrote: > These patches ensure that IMA works correctly on FUSE filesystems, so that > cached integrity data is not used. FUSE filesystems can change this data > at any time without notifying the kernel and we now verify it for each > use. > > This work is late in the kernel cycle, but they have had good review, > testing, and acks. They only impact FUSE and IMA. This seems entirely insane. You simply cannot use IMA on a fuse filesystem, because the data can change dynamically any time. But that doesn't mean that you can't cache the measurements - it means that the measurements are pointless. Those are two completely different things. This patch seems to disable caching, but still _use_ the measurement. Which seems *worse* than what we do now, in that it wastes time and effort on re-creating those pointless measurements because it disables the caching of them. So honestly, the only sane thing seems to be to disable IMA on fuse, not to force it to do even _more_ pointless work. What am I missing? Linus -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2018-02-10 at 12:44 -0800, Linus Torvalds wrote: > On Fri, Feb 9, 2018 at 10:26 PM, James Morris <jmorris@namei.org> wrote: > > These patches ensure that IMA works correctly on FUSE filesystems, so that > > cached integrity data is not used. FUSE filesystems can change this data > > at any time without notifying the kernel and we now verify it for each > > use. > > > > This work is late in the kernel cycle, but they have had good review, > > testing, and acks. They only impact FUSE and IMA. > > This seems entirely insane. > > You simply cannot use IMA on a fuse filesystem, because the data can > change dynamically any time. > > But that doesn't mean that you can't cache the measurements - it means > that the measurements are pointless. Those are two completely > different things. > > This patch seems to disable caching, but still _use_ the measurement. > > Which seems *worse* than what we do now, in that it wastes time and > effort on re-creating those pointless measurements because it disables > the caching of them. > > So honestly, the only sane thing seems to be to disable IMA on fuse, > not to force it to do even _more_ pointless work. > > What am I missing? No, you're right. The file could change at any time, making the measurement(s) and by extension signature verification meaningless. Custom policy rules could be defined to disable measurement, appraisal, and audit for files on fuse. However, I don't think we want to automatically disable measurement, even meaningless measurements. Some indication needs to be included for remote attestation, security analytics, or forensics. For systems with policies that require file signatures even on fuse, the safest thing would seem to be to fail the signature verification. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 10, 2018 at 8:41 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: >> >> What am I missing? > > No, you're right. The file could change at any time, making the > measurement(s) and by extension signature verification meaningless. > Custom policy rules could be defined to disable measurement, > appraisal, and audit for files on fuse. However, I don't think we > want to automatically disable measurement, even meaningless > measurements. Some indication needs to be included for remote > attestation, security analytics, or forensics. For systems with > policies that require file signatures even on fuse, the safest thing > would seem to be to fail the signature verification. Failing seems like a sane model, although I also suspect it would just break a lot of cases that currently work fine because *in*practice* fuse works fine as a normal filesystem (think fuse "exfat" module etc). So yes, the failing behavior is sane, but I agree with you that it should be something that requires a specific policy ("fail on untrusted filesystems like fuse"). But regardless, disabling caching just seems broken in all situations and never right, so I really don't want to pull that tree unless somebody can point out where it makes sense. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2018-02-10 at 20:50 -0800, Linus Torvalds wrote: > On Sat, Feb 10, 2018 at 8:41 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > >> > >> What am I missing? > > > > No, you're right. The file could change at any time, making the > > measurement(s) and by extension signature verification meaningless. > > Custom policy rules could be defined to disable measurement, > > appraisal, and audit for files on fuse. However, I don't think we > > want to automatically disable measurement, even meaningless > > measurements. Some indication needs to be included for remote > > attestation, security analytics, or forensics. For systems with > > policies that require file signatures even on fuse, the safest thing > > would seem to be to fail the signature verification. > > Failing seems like a sane model, although I also suspect it would just > break a lot of cases that currently work fine because *in*practice* > fuse works fine as a normal filesystem (think fuse "exfat" module > etc). > > So yes, the failing behavior is sane, but I agree with you that it > should be something that requires a specific policy ("fail on > untrusted filesystems like fuse"). Could we differentiate between untrusted from unprivileged and untrusted fuse? The existing fuse would continue to work, but on systems with IMA-appraisal enabled the new, unprivileged fuse would fail. > But regardless, disabling caching just seems broken in all situations > and never right, so I really don't want to pull that tree unless > somebody can point out where it makes sense. Agreed. Re-measuring/appraising the file would only detect well behaved malicious fuse. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c index 624f18b..0a9e516 100644 --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@ -1205,7 +1205,7 @@ static void fuse_kill_sb_anon(struct super_block *sb) static struct file_system_type fuse_fs_type = { .owner = THIS_MODULE, .name = "fuse", - .fs_flags = FS_HAS_SUBTYPE, + .fs_flags = FS_HAS_SUBTYPE | FS_IMA_NO_CACHE, .mount = fuse_mount, .kill_sb = fuse_kill_sb_anon, }; diff --git a/include/linux/fs.h b/include/linux/fs.h index 2a81556..8d3f97d 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2069,6 +2069,7 @@ struct file_system_type { #define FS_BINARY_MOUNTDATA 2 #define FS_HAS_SUBTYPE 4 #define FS_USERNS_MOUNT 8 /* Can be mounted by userns root */ +#define FS_IMA_NO_CACHE 16 /* Force IMA to re-measure, re-appraise, re-audit files */ #define FS_RENAME_DOES_D_MOVE 32768 /* FS will handle d_move() during rename() internally. */ struct dentry *(*mount) (struct file_system_type *, int, const char *, void *); diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 2cfb0c7..55d9149 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -25,6 +25,7 @@ #include <linux/xattr.h> #include <linux/ima.h> #include <linux/iversion.h> +#include <linux/fs.h> #include "ima.h" @@ -229,9 +230,19 @@ static int process_measurement(struct file *file, char *buf, loff_t size, IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK | IMA_ACTION_FLAGS); - if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) - /* reset all flags if ima_inode_setxattr was called */ + /* + * Reset the measure, appraise and audit cached flags either if: + * - ima_inode_setxattr was called, or + * - based on filesystem feature flag + * forcing the file to be re-evaluated. + */ + if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) { + iint->flags &= ~IMA_DONE_MASK; + } else if (inode->i_sb->s_type->fs_flags & FS_IMA_NO_CACHE) { iint->flags &= ~IMA_DONE_MASK; + if (action & IMA_MEASURE) + iint->measured_pcrs = 0; + } /* Determine if already appraised/measured based on bitmask * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
These patches ensure that IMA works correctly on FUSE filesystems, so that cached integrity data is not used. FUSE filesystems can change this data at any time without notifying the kernel and we now verify it for each use. This work is late in the kernel cycle, but they have had good review, testing, and acks. They only impact FUSE and IMA. Please consider pulling for v4.16. --- The following changes since commit 9a61df9e5f7471fe5be3e02bd0bed726b2761a54: Merge tag 'kbuild-v4.16-2' of git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild (2018-02-09 19:32:41 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git for-v4.16 for you to fetch changes up to 8f7765f67642424650e22c1077b149c2820e7d18: fuse: introduce new fs_type flag FS_IMA_NO_CACHE (2018-02-10 15:10:09 +1100) ---------------------------------------------------------------- Alban Crequy (2): ima: force re-appraisal on filesystems with FS_IMA_NO_CACHE fuse: introduce new fs_type flag FS_IMA_NO_CACHE fs/fuse/inode.c | 2 +- include/linux/fs.h | 1 + security/integrity/ima/ima_main.c | 15 +++++++++++++-- 3 files changed, 15 insertions(+), 3 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html