[GIT PULL] Integrity: IMA FUSE fixes

[GIT PULL] Integrity: IMA FUSE fixes

[James Morris]

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(-)

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

[GIT PULL] Integrity: IMA FUSE fixes

[Linus Torvalds]

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[GIT PULL] Integrity: IMA FUSE fixes

[Mimi Zohar]

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[GIT PULL] Integrity: IMA FUSE fixes

[Linus Torvalds]

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] Integrity: IMA FUSE fixes

[James Morris]

[-- Attachment #1: Type: text/plain, Size: 2065 bytes --]

On Sat, 10 Feb 2018, Mimi Zohar wrote:

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

Really?  I thought the whole idea of IMA was that it only detects offline 
tampering and it specifically does not protect against runtime attacks.

Any file could change after measurement on a compromised or misconfigured 
system.  e.g. via direct write to the block device.

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

I don't understand this -- if a file passes signature verification, it 
passes.  If it was modified and still passes, the problem is elsewhere.

I don't think the FUSE measurements are inherently useless, or more 
useless than any others, at least.  You can misconfigure all kinds of 
things on a system which would undermine IMA, and I would count allowing 
unprivileged use of FUSE with critical files as such.

The point of the patches, IIUC, was that FUSE had no useful way to notify 
IMA that a file had changed, so, always measure.  IMA assumes that changes 
to a running system are made under the control of a correctly enforced 
security policy.  If you're using FUSE and IMA, then you should understand 
the security implications of that.  Or am I missing something?


- James
-- 
James Morris
<jmorris@namei.org>

Re: [GIT PULL] Integrity: IMA FUSE fixes

[Mimi Zohar]

On Mon, 2018-02-12 at 00:19 +1100, James Morris wrote:
> On Sat, 10 Feb 2018, Mimi Zohar wrote:
> > 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.
> 
> I don't understand this -- if a file passes signature verification, it 
> passes.  If it was modified and still passes, the problem is elsewhere.
> 
> I don't think the FUSE measurements are inherently useless, or more 
> useless than any others, at least.  You can misconfigure all kinds of 
> things on a system which would undermine IMA, and I would count allowing 
> unprivileged use of FUSE with critical files as such.

The problem is that fuse can provide whatever it wants, whenever it
wants.  It could provide different pages for the initial file hash
calculation and then for usage.

If the file is first read into a buffer, like for the kernel_read_file_xxx() hooks, then the file data would be the same for the file calculation and usage, but for most of the IMA hooks, the file isn't first read into a buffer.

> The point of the patches, IIUC, was that FUSE had no useful way to notify 
> IMA that a file had changed, so, always measure.  IMA assumes that changes 
> to a running system are made under the control of a correctly enforced 
> security policy.  If you're using FUSE and IMA, then you should understand 
> the security implications of that.  Or am I missing something?

That's all true, but I think the intent of these patches was to detect
malicious file change, where the fuse filesystem itself is malicious.

>From the patch description:    
    It is useful in FUSE because the userspace FUSE process can change the
    underlying files at any time without notifying the kernel. FUSE can be
    mounted by unprivileged users either today with fusermount installed
    with setuid, or soon with the upcoming patches to allow FUSE mounts in
    a non-init user namespace. That makes the issue more visible than for
    network filesystems where unprivileged users cannot mount.

For the example test provided, re-measuring the file works properly. I
had missed that if the fuse filesystem could provide different files,
at least in theory, it could also be able to provide different pages.

Mimi

[GIT PULL] Integrity: IMA FUSE fixes

[Mimi Zohar]

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html