Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

[Serge E. Hallyn]

Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> Files on untrusted filesystems, such as fuse, can change at any time,
> making the measurement(s) and by extension signature verification
> meaningless.
> 
> 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.
> 
> This patch always fails the file signature verification on unprivileged
> and untrusted filesystems.  To also fail file signature verification on

Why only untrusted?  Fuse could cause the same issue if it just
messes up when mounted from init userns right?

> privileged, untrusted filesystems requires a custom policy.

(I'm not saying you shouldn't do this, but) does this mean that
a container whose rootfs is fuse-mounted by the unprivileged user
cannot possibly use IMA?

Good thing we can partially work around that by intercepting real
mount calls with Tycho's new patchset :)

> (This patch is based on Alban Crequy's use of fs_flags and patch
>  description.)
> 
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Seth Forshee <seth.forshee@canonical.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Dongsu Park <dongsu@kinvolk.io>
> Cc: Alban Crequy <alban@kinvolk.io>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> ---
>  include/linux/fs.h                    |  1 +
>  security/integrity/ima/ima_appraise.c | 10 +++++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2a815560fda0..faffe4aab43d 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_UNTRUSTED		16	/* Defined filesystem as untrusted */
>  #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_appraise.c b/security/integrity/ima/ima_appraise.c
> index f2803a40ff82..af8add31fe26 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -292,7 +292,14 @@ int ima_appraise_measurement(enum ima_hooks func,
>  	}
>  
>  out:
> -	if (status != INTEGRITY_PASS) {
> +	/* Fail untrusted and unpriviliged filesystems (eg FUSE) */
> +	if ((inode->i_sb->s_type->fs_flags & FS_UNTRUSTED) &&
> +	    (inode->i_sb->s_user_ns != &init_user_ns)) {
> +		status = INTEGRITY_FAIL;
> +		cause = "untrusted-filesystem";
> +		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> +				    op, cause, rc, 0);
> +	} else if (status != INTEGRITY_PASS) {
>  		if ((ima_appraise & IMA_APPRAISE_FIX) &&
>  		    (!xattr_value ||
>  		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> @@ -309,6 +316,7 @@ int ima_appraise_measurement(enum ima_hooks func,
>  	} else {
>  		ima_cache_flags(iint, func);
>  	}
> +
>  	ima_set_cache_status(iint, func, status);
>  	return status;
>  }
> -- 
> 2.7.5

Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

[Mimi Zohar]

On Wed, 2018-02-14 at 08:49 -0600, Serge E. Hallyn wrote:
> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > Files on untrusted filesystems, such as fuse, can change at any time,
> > making the measurement(s) and by extension signature verification
> > meaningless.
> > 
> > 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.
> > 
> > This patch always fails the file signature verification on unprivileged
> > and untrusted filesystems.  To also fail file signature verification on
> 
> Why only untrusted?  Fuse could cause the same issue if it just
> messes up when mounted from init userns right?

Right, whether it is an unprivileged mount or not, fuse can return
whatever it wants, whenever it wants.  IMA can calculate the file hash
based based on what it reads, but fuse can return whatever it wants on
subsequent reads.

Refer to the discussion with Linus - http://kernsec.org/pipermail/linu
x-security-module-archive/2018-February/005200.html

> > privileged, untrusted filesystems requires a custom policy.
> 
> (I'm not saying you shouldn't do this, but) does this mean that
> a container whose rootfs is fuse-mounted by the unprivileged user
> cannot possibly use IMA?

How would you suggest to differentiate between your unprivileged fuse
mounts from unintended, unintended malicious ones?

The remaining patches are policy based.

> Good thing we can partially work around that by intercepting real
> mount calls with Tycho's new patchset :)

Can you provide a little more details?

thanks,

Mimi

> 
> > (This patch is based on Alban Crequy's use of fs_flags and patch
> >  description.)
> > 
> > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > Cc: Miklos Szeredi <miklos@szeredi.hu>
> > Cc: Seth Forshee <seth.forshee@canonical.com>
> > Cc: Eric W. Biederman <ebiederm@xmission.com>
> > Cc: Dongsu Park <dongsu@kinvolk.io>
> > Cc: Alban Crequy <alban@kinvolk.io>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > ---
> >  include/linux/fs.h                    |  1 +
> >  security/integrity/ima/ima_appraise.c | 10 +++++++++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 2a815560fda0..faffe4aab43d 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_UNTRUSTED		16	/* Defined filesystem as untrusted */
> >  #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_appraise.c b/security/integrity/ima/ima_appraise.c
> > index f2803a40ff82..af8add31fe26 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -292,7 +292,14 @@ int ima_appraise_measurement(enum ima_hooks func,
> >  	}
> >  
> >  out:
> > -	if (status != INTEGRITY_PASS) {
> > +	/* Fail untrusted and unpriviliged filesystems (eg FUSE) */
> > +	if ((inode->i_sb->s_type->fs_flags & FS_UNTRUSTED) &&
> > +	    (inode->i_sb->s_user_ns != &init_user_ns)) {
> > +		status = INTEGRITY_FAIL;
> > +		cause = "untrusted-filesystem";
> > +		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> > +				    op, cause, rc, 0);
> > +	} else if (status != INTEGRITY_PASS) {
> >  		if ((ima_appraise & IMA_APPRAISE_FIX) &&
> >  		    (!xattr_value ||
> >  		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> > @@ -309,6 +316,7 @@ int ima_appraise_measurement(enum ima_hooks func,
> >  	} else {
> >  		ima_cache_flags(iint, func);
> >  	}
> > +
> >  	ima_set_cache_status(iint, func, status);
> >  	return status;
> >  }
> > -- 
> > 2.7.5
> 

Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

[Serge E. Hallyn]

Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> On Wed, 2018-02-14 at 08:49 -0600, Serge E. Hallyn wrote:
> > Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > > Files on untrusted filesystems, such as fuse, can change at any time,
> > > making the measurement(s) and by extension signature verification
> > > meaningless.
> > > 
> > > 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.
> > > 
> > > This patch always fails the file signature verification on unprivileged
> > > and untrusted filesystems.  To also fail file signature verification on
> > 
> > Why only untrusted?  Fuse could cause the same issue if it just
> > messes up when mounted from init userns right?
> 
> Right, whether it is an unprivileged mount or not, fuse can return
> whatever it wants, whenever it wants.  IMA can calculate the file hash
> based based on what it reads, but fuse can return whatever it wants on
> subsequent reads.

Ok but your patch seems to let privileged fuse mounts slide?  (see below)

> Refer to the discussion with Linus - http://kernsec.org/pipermail/linu
> x-security-module-archive/2018-February/005200.html
> 
> > > privileged, untrusted filesystems requires a custom policy.
> > 
> > (I'm not saying you shouldn't do this, but) does this mean that
> > a container whose rootfs is fuse-mounted by the unprivileged user
> > cannot possibly use IMA?
> 
> How would you suggest to differentiate between your unprivileged fuse
> mounts from unintended, unintended malicious ones?

I wouldn't.

> The remaining patches are policy based.
> 
> > Good thing we can partially work around that by intercepting real
> > mount calls with Tycho's new patchset :)
> 
> Can you provide a little more details?

It would allow a container runtime to intercept mount(2) and perform
a real mount on the user's behalf.  Assuming the runtime is privileged,
of course, otherwise it would intercept and do a fuse mount which is
no help here :)

https://lkml.org/lkml/2018/2/4/28

> thanks,
> 
> Mimi
> 
> > 
> > > (This patch is based on Alban Crequy's use of fs_flags and patch
> > >  description.)
> > > 
> > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > > Cc: Miklos Szeredi <miklos@szeredi.hu>
> > > Cc: Seth Forshee <seth.forshee@canonical.com>
> > > Cc: Eric W. Biederman <ebiederm@xmission.com>
> > > Cc: Dongsu Park <dongsu@kinvolk.io>
> > > Cc: Alban Crequy <alban@kinvolk.io>
> > > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > > ---
> > >  include/linux/fs.h                    |  1 +
> > >  security/integrity/ima/ima_appraise.c | 10 +++++++++-
> > >  2 files changed, 10 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index 2a815560fda0..faffe4aab43d 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_UNTRUSTED		16	/* Defined filesystem as untrusted */
> > >  #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_appraise.c b/security/integrity/ima/ima_appraise.c
> > > index f2803a40ff82..af8add31fe26 100644
> > > --- a/security/integrity/ima/ima_appraise.c
> > > +++ b/security/integrity/ima/ima_appraise.c
> > > @@ -292,7 +292,14 @@ int ima_appraise_measurement(enum ima_hooks func,
> > >  	}
> > >  
> > >  out:
> > > -	if (status != INTEGRITY_PASS) {
> > > +	/* Fail untrusted and unpriviliged filesystems (eg FUSE) */
> > > +	if ((inode->i_sb->s_type->fs_flags & FS_UNTRUSTED) &&
> > > +	    (inode->i_sb->s_user_ns != &init_user_ns)) {

^ if inode->i_sb->s_user_ns == &init_user_ns you don't fail.

> > > +		status = INTEGRITY_FAIL;
> > > +		cause = "untrusted-filesystem";
> > > +		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> > > +				    op, cause, rc, 0);
> > > +	} else if (status != INTEGRITY_PASS) {
> > >  		if ((ima_appraise & IMA_APPRAISE_FIX) &&
> > >  		    (!xattr_value ||
> > >  		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> > > @@ -309,6 +316,7 @@ int ima_appraise_measurement(enum ima_hooks func,
> > >  	} else {
> > >  		ima_cache_flags(iint, func);
> > >  	}
> > > +
> > >  	ima_set_cache_status(iint, func, status);
> > >  	return status;
> > >  }
> > > -- 
> > > 2.7.5
> > 

Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

[Mimi Zohar]

On Wed, 2018-02-14 at 09:16 -0600, Serge E. Hallyn wrote:
> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > On Wed, 2018-02-14 at 08:49 -0600, Serge E. Hallyn wrote:
> > > Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > > > Files on untrusted filesystems, such as fuse, can change at any time,
> > > > making the measurement(s) and by extension signature verification
> > > > meaningless.
> > > > 
> > > > 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.
> > > > 
> > > > This patch always fails the file signature verification on unprivileged
> > > > and untrusted filesystems.  To also fail file signature verification on
> > > 
> > > Why only untrusted?  Fuse could cause the same issue if it just
> > > messes up when mounted from init userns right?
> > 
> > Right, whether it is an unprivileged mount or not, fuse can return
> > whatever it wants, whenever it wants.  IMA can calculate the file hash
> > based based on what it reads, but fuse can return whatever it wants on
> > subsequent reads.
> 
> Ok but your patch seems to let privileged fuse mounts slide?  (see below)

Unprivileged fuse mounts hasn't been upstreamed yet, so we wouldn't be
breaking existing userspace.

> 
> > Refer to the discussion with Linus - http://kernsec.org/pipermail/linu
> > x-security-module-archive/2018-February/005200.html
> > 
> > > > privileged, untrusted filesystems requires a custom policy.
> > > 
> > > (I'm not saying you shouldn't do this, but) does this mean that
> > > a container whose rootfs is fuse-mounted by the unprivileged user
> > > cannot possibly use IMA?
> > 
> > How would you suggest to differentiate between your unprivileged fuse
> > mounts from unintended, unintended malicious ones?
> 
> I wouldn't.

What happened to the requirement that systems should be "fail-safe"?
Ok, hard coding this rule probably is not a good idea.  For those
wanting to take this liability on their systems, we can make this
configurable, like for privileged fuse mounts.  Unlike for privileged
fuse mounts, the builtin policies, I think, should be fail safe and
include the "fail" rule for unprivileged fuse mounts.

> 
> > The remaining patches are policy based.
> > 
> > > Good thing we can partially work around that by intercepting real
> > > mount calls with Tycho's new patchset :)
> > 
> > Can you provide a little more details?
> 
> It would allow a container runtime to intercept mount(2) and perform
> a real mount on the user's behalf.  Assuming the runtime is privileged,
> of course, otherwise it would intercept and do a fuse mount which is
> no help here :)
> 
> https://lkml.org/lkml/2018/2/4/28

thanks!

Mimi

Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

[Serge E. Hallyn]

Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> On Wed, 2018-02-14 at 09:16 -0600, Serge E. Hallyn wrote:
> > Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > > On Wed, 2018-02-14 at 08:49 -0600, Serge E. Hallyn wrote:
> > > > Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > > > > Files on untrusted filesystems, such as fuse, can change at any time,
> > > > > making the measurement(s) and by extension signature verification
> > > > > meaningless.
> > > > > 
> > > > > 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.
> > > > > 
> > > > > This patch always fails the file signature verification on unprivileged
> > > > > and untrusted filesystems.  To also fail file signature verification on
> > > > 
> > > > Why only untrusted?  Fuse could cause the same issue if it just
> > > > messes up when mounted from init userns right?
> > > 
> > > Right, whether it is an unprivileged mount or not, fuse can return
> > > whatever it wants, whenever it wants.  IMA can calculate the file hash
> > > based based on what it reads, but fuse can return whatever it wants on
> > > subsequent reads.
> > 
> > Ok but your patch seems to let privileged fuse mounts slide?  (see below)
> 
> Unprivileged fuse mounts hasn't been upstreamed yet, so we wouldn't be
> breaking existing userspace.

I don't think I'm being clear.

In your patch it looks like you mark unprivileged FUSE mounts as
INTEGRITY_FAIL.  I agree you should do that.  But you skip the
FS_UNTRUSTED check for privileged FUSE mounts.  I'm asking why
that's ok.

> > > Refer to the discussion with Linus - http://kernsec.org/pipermail/linu
> > > x-security-module-archive/2018-February/005200.html
> > > 
> > > > > privileged, untrusted filesystems requires a custom policy.
> > > > 
> > > > (I'm not saying you shouldn't do this, but) does this mean that
> > > > a container whose rootfs is fuse-mounted by the unprivileged user
> > > > cannot possibly use IMA?
> > > 
> > > How would you suggest to differentiate between your unprivileged fuse
> > > mounts from unintended, unintended malicious ones?
> > 
> > I wouldn't.
> 
> What happened to the requirement that systems should be "fail-safe"?

My point was - I was asking whether there was any way to have IMA be
meaningful with such containers, not saying I had any ideas, and
certainly not saying that just because you can't detect it means you
should allow it in all cases.  It's too bad that it has this effect,
but I agree with your patch.

I only didn't ack it because you're skipping the check for privileged
mounts which seems wrong.

-serge

Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

[Mimi Zohar]

On Wed, 2018-02-14 at 09:42 -0600, Serge E. Hallyn wrote:
> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > On Wed, 2018-02-14 at 09:16 -0600, Serge E. Hallyn wrote:
> > > Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > > > On Wed, 2018-02-14 at 08:49 -0600, Serge E. Hallyn wrote:
> > > > > Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > > > > > Files on untrusted filesystems, such as fuse, can change at any time,
> > > > > > making the measurement(s) and by extension signature verification
> > > > > > meaningless.
> > > > > > 
> > > > > > 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.
> > > > > > 
> > > > > > This patch always fails the file signature verification on unprivileged
> > > > > > and untrusted filesystems.  To also fail file signature verification on
> > > > > 
> > > > > Why only untrusted?  Fuse could cause the same issue if it just
> > > > > messes up when mounted from init userns right?
> > > > 
> > > > Right, whether it is an unprivileged mount or not, fuse can return
> > > > whatever it wants, whenever it wants.  IMA can calculate the file hash
> > > > based based on what it reads, but fuse can return whatever it wants on
> > > > subsequent reads.
> > > 
> > > Ok but your patch seems to let privileged fuse mounts slide?  (see below)
> > 
> > Unprivileged fuse mounts hasn't been upstreamed yet, so we wouldn't be
> > breaking existing userspace.
> 
> I don't think I'm being clear.
> 
> In your patch it looks like you mark unprivileged FUSE mounts as
> INTEGRITY_FAIL.  I agree you should do that.  But you skip the
> FS_UNTRUSTED check for privileged FUSE mounts.  I'm asking why
> that's ok.
> 
> > > > Refer to the discussion with Linus - http://kernsec.org/pipermail/linu
> > > > x-security-module-archive/2018-February/005200.html
> > > > 
> > > > > > privileged, untrusted filesystems requires a custom policy.
> > > > > 
> > > > > (I'm not saying you shouldn't do this, but) does this mean that
> > > > > a container whose rootfs is fuse-mounted by the unprivileged user
> > > > > cannot possibly use IMA?
> > > > 
> > > > How would you suggest to differentiate between your unprivileged fuse
> > > > mounts from unintended, unintended malicious ones?
> > > 
> > > I wouldn't.
> > 
> > What happened to the requirement that systems should be "fail-safe"?
> 
> My point was - I was asking whether there was any way to have IMA be
> meaningful with such containers, not saying I had any ideas, and
> certainly not saying that just because you can't detect it means you
> should allow it in all cases.  It's too bad that it has this effect,
> but I agree with your patch.
> 
> I only didn't ack it because you're skipping the check for privileged
> mounts which seems wrong.

Oh!  That is based on Linus' "request" not to break userspace.

Mimi

Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

[Serge E. Hallyn]

Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> On Wed, 2018-02-14 at 09:42 -0600, Serge E. Hallyn wrote:
> > Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > > On Wed, 2018-02-14 at 09:16 -0600, Serge E. Hallyn wrote:
> > > > Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > > > > On Wed, 2018-02-14 at 08:49 -0600, Serge E. Hallyn wrote:
> > > > > > Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > > > > > > Files on untrusted filesystems, such as fuse, can change at any time,
> > > > > > > making the measurement(s) and by extension signature verification
> > > > > > > meaningless.
> > > > > > > 
> > > > > > > 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.
> > > > > > > 
> > > > > > > This patch always fails the file signature verification on unprivileged
> > > > > > > and untrusted filesystems.  To also fail file signature verification on
> > > > > > 
> > > > > > Why only untrusted?  Fuse could cause the same issue if it just
> > > > > > messes up when mounted from init userns right?
> > > > > 
> > > > > Right, whether it is an unprivileged mount or not, fuse can return
> > > > > whatever it wants, whenever it wants.  IMA can calculate the file hash
> > > > > based based on what it reads, but fuse can return whatever it wants on
> > > > > subsequent reads.
> > > > 
> > > > Ok but your patch seems to let privileged fuse mounts slide?  (see below)
> > > 
> > > Unprivileged fuse mounts hasn't been upstreamed yet, so we wouldn't be
> > > breaking existing userspace.
> > 
> > I don't think I'm being clear.
> > 
> > In your patch it looks like you mark unprivileged FUSE mounts as
> > INTEGRITY_FAIL.  I agree you should do that.  But you skip the
> > FS_UNTRUSTED check for privileged FUSE mounts.  I'm asking why
> > that's ok.
> > 
> > > > > Refer to the discussion with Linus - http://kernsec.org/pipermail/linu
> > > > > x-security-module-archive/2018-February/005200.html
> > > > > 
> > > > > > > privileged, untrusted filesystems requires a custom policy.
> > > > > > 
> > > > > > (I'm not saying you shouldn't do this, but) does this mean that
> > > > > > a container whose rootfs is fuse-mounted by the unprivileged user
> > > > > > cannot possibly use IMA?
> > > > > 
> > > > > How would you suggest to differentiate between your unprivileged fuse
> > > > > mounts from unintended, unintended malicious ones?
> > > > 
> > > > I wouldn't.
> > > 
> > > What happened to the requirement that systems should be "fail-safe"?
> > 
> > My point was - I was asking whether there was any way to have IMA be
> > meaningful with such containers, not saying I had any ideas, and
> > certainly not saying that just because you can't detect it means you
> > should allow it in all cases.  It's too bad that it has this effect,
> > but I agree with your patch.
> > 
> > I only didn't ack it because you're skipping the check for privileged
> > mounts which seems wrong.
> 
> Oh!  That is based on Linus' "request" not to break userspace.

Oh, yeah, I guess that would do it :)  It seems so wrong that it's
probably worth putting a comment above that exception.

So probably not worth much but

Acked-by: Serge Hallyn <serge@hallyn.com>

thanks,
-serge

Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

[Eric W. Biederman]

Mimi Zohar <zohar@linux.vnet.ibm.com> writes:

> Files on untrusted filesystems, such as fuse, can change at any time,
> making the measurement(s) and by extension signature verification
> meaningless.
>
> 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.
>
> This patch always fails the file signature verification on unprivileged
> and untrusted filesystems.  To also fail file signature verification on
> privileged, untrusted filesystems requires a custom policy.
>
> (This patch is based on Alban Crequy's use of fs_flags and patch
>  description.)

This would be much better done based on a flag in s_iflags and then the
mounts that need this can set this.  That new flag can perhaps be called
SB_I_IMA_FAIL.

Among other things that should allow the policy of when to set this to
be set in fuse where it is obvious rather than in an magic location in
IMA.

Eric

> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Seth Forshee <seth.forshee@canonical.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Dongsu Park <dongsu@kinvolk.io>
> Cc: Alban Crequy <alban@kinvolk.io>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> ---
>  include/linux/fs.h                    |  1 +
>  security/integrity/ima/ima_appraise.c | 10 +++++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2a815560fda0..faffe4aab43d 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_UNTRUSTED		16	/* Defined filesystem as untrusted */
>  #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_appraise.c b/security/integrity/ima/ima_appraise.c
> index f2803a40ff82..af8add31fe26 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -292,7 +292,14 @@ int ima_appraise_measurement(enum ima_hooks func,
>  	}
>  
>  out:
> -	if (status != INTEGRITY_PASS) {
> +	/* Fail untrusted and unpriviliged filesystems (eg FUSE) */
> +	if ((inode->i_sb->s_type->fs_flags & FS_UNTRUSTED) &&
> +	    (inode->i_sb->s_user_ns != &init_user_ns)) {
> +		status = INTEGRITY_FAIL;
> +		cause = "untrusted-filesystem";
> +		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> +				    op, cause, rc, 0);
> +	} else if (status != INTEGRITY_PASS) {
>  		if ((ima_appraise & IMA_APPRAISE_FIX) &&
>  		    (!xattr_value ||
>  		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> @@ -309,6 +316,7 @@ int ima_appraise_measurement(enum ima_hooks func,
>  	} else {
>  		ima_cache_flags(iint, func);
>  	}
> +
>  	ima_set_cache_status(iint, func, status);
>  	return status;
>  }

Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

[Mimi Zohar]

On Wed, 2018-02-14 at 17:57 -0600, Eric W. Biederman wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> 
> > Files on untrusted filesystems, such as fuse, can change at any time,
> > making the measurement(s) and by extension signature verification
> > meaningless.
> >
> > 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.
> >
> > This patch always fails the file signature verification on unprivileged
> > and untrusted filesystems.  To also fail file signature verification on
> > privileged, untrusted filesystems requires a custom policy.
> >
> > (This patch is based on Alban Crequy's use of fs_flags and patch
> >  description.)
> 
> This would be much better done based on a flag in s_iflags and then the
> mounts that need this can set this.  That new flag can perhaps be called
> SB_I_IMA_FAIL.
> 
> Among other things that should allow the policy of when to set this to
> be set in fuse where it is obvious rather than in an magic location in
> IMA.

Using s_iflags instead of fs_flags is fine, but I'm not sure how this
affects the IMA policy.  This patch set assumes only unprivileged,
untrusted filesytems can automatically fail file signature
verification (2nd patch), as that hasn't yet been upstreamed and won't
break userspace.

Based on policy, IMA should additionally be able to fail the signature
verification for files on privileged, untrusted filesystems.

Mimi


> Eric
> 
> > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > Cc: Miklos Szeredi <miklos@szeredi.hu>
> > Cc: Seth Forshee <seth.forshee@canonical.com>
> > Cc: Eric W. Biederman <ebiederm@xmission.com>
> > Cc: Dongsu Park <dongsu@kinvolk.io>
> > Cc: Alban Crequy <alban@kinvolk.io>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > ---
> >  include/linux/fs.h                    |  1 +
> >  security/integrity/ima/ima_appraise.c | 10 +++++++++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 2a815560fda0..faffe4aab43d 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_UNTRUSTED		16	/* Defined filesystem as untrusted */
> >  #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_appraise.c b/security/integrity/ima/ima_appraise.c
> > index f2803a40ff82..af8add31fe26 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -292,7 +292,14 @@ int ima_appraise_measurement(enum ima_hooks func,
> >  	}
> >  
> >  out:
> > -	if (status != INTEGRITY_PASS) {
> > +	/* Fail untrusted and unpriviliged filesystems (eg FUSE) */
> > +	if ((inode->i_sb->s_type->fs_flags & FS_UNTRUSTED) &&
> > +	    (inode->i_sb->s_user_ns != &init_user_ns)) {
> > +		status = INTEGRITY_FAIL;
> > +		cause = "untrusted-filesystem";
> > +		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> > +				    op, cause, rc, 0);
> > +	} else if (status != INTEGRITY_PASS) {
> >  		if ((ima_appraise & IMA_APPRAISE_FIX) &&
> >  		    (!xattr_value ||
> >  		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> > @@ -309,6 +316,7 @@ int ima_appraise_measurement(enum ima_hooks func,
> >  	} else {
> >  		ima_cache_flags(iint, func);
> >  	}
> > +
> >  	ima_set_cache_status(iint, func, status);
> >  	return status;
> >  }
> 

Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

[Eric W. Biederman]

Mimi Zohar <zohar@linux.vnet.ibm.com> writes:

> On Wed, 2018-02-14 at 17:57 -0600, Eric W. Biederman wrote:
>> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
>> 
>> > Files on untrusted filesystems, such as fuse, can change at any time,
>> > making the measurement(s) and by extension signature verification
>> > meaningless.
>> >
>> > 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.
>> >
>> > This patch always fails the file signature verification on unprivileged
>> > and untrusted filesystems.  To also fail file signature verification on
>> > privileged, untrusted filesystems requires a custom policy.
>> >
>> > (This patch is based on Alban Crequy's use of fs_flags and patch
>> >  description.)
>> 
>> This would be much better done based on a flag in s_iflags and then the
>> mounts that need this can set this.  That new flag can perhaps be called
>> SB_I_IMA_FAIL.
>> 
>> Among other things that should allow the policy of when to set this to
>> be set in fuse where it is obvious rather than in an magic location in
>> IMA.
>
> Using s_iflags instead of fs_flags is fine, but I'm not sure how this
> affects the IMA policy.  This patch set assumes only unprivileged,
> untrusted filesytems can automatically fail file signature
> verification (2nd patch), as that hasn't yet been upstreamed and won't
> break userspace.
>
> Based on policy, IMA should additionally be able to fail the signature
> verification for files on privileged, untrusted filesystems.

Apologies ima has a very specific meaning of policy, as in the loaded
ima policy.  I was meaning the hard coded policy of which filesystems
we simply would not trust by default.

In code terms what I was thinking would look something like:

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -292,7 +292,14 @@ int ima_appraise_measurement(enum ima_hooks func,
 	}
  
 out:
-	if (status != INTEGRITY_PASS) {
+	/* Fail if we can't trust the fs enough to support ima xattrs (FUSE) */
+	if (inode->i_sb->s_iflags & SB_I_NOIMA) {
+		status = INTEGRITY_FAIL;
+		cause = "untrusted-filesystem";
+		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
+				    op, cause, rc, 0);
+	} else if (status != INTEGRITY_PASS) {
 		if ((ima_appraise & IMA_APPRAISE_FIX) &&
 		    (!xattr_value ||
  		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {

And somewhere in the fuse mount code it would say:

	if (sb->s_user_ns != &init_user_ns)
        	sb->s_iflags |= SB_I_NOIMA);

The point being that the logic for setting the flag can live in fuse or
a simpler filesystem and all ima proper needs to do is deal with the flag being
set.  That should be easier to maintainer and simpler to code all
around.

Eric

Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

[Mimi Zohar]

On Thu, 2018-02-15 at 10:47 -0600, Eric W. Biederman wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> 
> > On Wed, 2018-02-14 at 17:57 -0600, Eric W. Biederman wrote:
> >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> >> 
> >> > Files on untrusted filesystems, such as fuse, can change at any time,
> >> > making the measurement(s) and by extension signature verification
> >> > meaningless.
> >> >
> >> > 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.
> >> >
> >> > This patch always fails the file signature verification on unprivileged
> >> > and untrusted filesystems.  To also fail file signature verification on
> >> > privileged, untrusted filesystems requires a custom policy.
> >> >
> >> > (This patch is based on Alban Crequy's use of fs_flags and patch
> >> >  description.)
> >> 
> >> This would be much better done based on a flag in s_iflags and then the
> >> mounts that need this can set this.  That new flag can perhaps be called
> >> SB_I_IMA_FAIL.
> >> 
> >> Among other things that should allow the policy of when to set this to
> >> be set in fuse where it is obvious rather than in an magic location in
> >> IMA.
> >
> > Using s_iflags instead of fs_flags is fine, but I'm not sure how this
> > affects the IMA policy.  This patch set assumes only unprivileged,
> > untrusted filesytems can automatically fail file signature
> > verification (2nd patch), as that hasn't yet been upstreamed and won't
> > break userspace.
> >
> > Based on policy, IMA should additionally be able to fail the signature
> > verification for files on privileged, untrusted filesystems.
> 
> Apologies ima has a very specific meaning of policy, as in the loaded
> ima policy.  I was meaning the hard coded policy of which filesystems
> we simply would not trust by default.
> 
> In code terms what I was thinking would look something like:
> 
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -292,7 +292,14 @@ int ima_appraise_measurement(enum ima_hooks func,
>  	}
>   
>  out:
> -	if (status != INTEGRITY_PASS) {
> +	/* Fail if we can't trust the fs enough to support ima xattrs (FUSE) */
> +	if (inode->i_sb->s_iflags & SB_I_NOIMA) {
> +		status = INTEGRITY_FAIL;
> +		cause = "untrusted-filesystem";
> +		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> +				    op, cause, rc, 0);
> +	} else if (status != INTEGRITY_PASS) {
>  		if ((ima_appraise & IMA_APPRAISE_FIX) &&
>  		    (!xattr_value ||
>   		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> 
> And somewhere in the fuse mount code it would say:
> 
> 	if (sb->s_user_ns != &init_user_ns)
>         	sb->s_iflags |= SB_I_NOIMA);

SB_I_NOIMA would be really confusing, as we're not disabling IMA in
general, just failing the signature verification.  The measurement,
even if it is meaningless, is an indication in the measurement list
that the file was accessed/executed.

I would propose naming the flag SB_I_IMA_UNTRUSTED, meaning the kernel
is unaware of fs changes.

> 
> The point being that the logic for setting the flag can live in fuse or
> a simpler filesystem and all ima proper needs to do is deal with the flag being
> set.  That should be easier to maintainer and simpler to code all
> around.

The last patch (4/4) had 1 line, which set the fs_flags
unconditionally in fuse_fs_type.  Instead, we can set the sb->s_iflags 
in fuse_fill_supper(), again unconditionally, letting IMA-appraisal
differentiate between privileged and unprivileged.

Mimi

Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

[Eric W. Biederman]

Mimi Zohar <zohar@linux.vnet.ibm.com> writes:

> On Thu, 2018-02-15 at 10:47 -0600, Eric W. Biederman wrote:
>> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
>> 
>> > On Wed, 2018-02-14 at 17:57 -0600, Eric W. Biederman wrote:
>> >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
>> >> 
>> >> > Files on untrusted filesystems, such as fuse, can change at any time,
>> >> > making the measurement(s) and by extension signature verification
>> >> > meaningless.
>> >> >
>> >> > 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.
>> >> >
>> >> > This patch always fails the file signature verification on unprivileged
>> >> > and untrusted filesystems.  To also fail file signature verification on
>> >> > privileged, untrusted filesystems requires a custom policy.
>> >> >
>> >> > (This patch is based on Alban Crequy's use of fs_flags and patch
>> >> >  description.)
>> >> 
>> >> This would be much better done based on a flag in s_iflags and then the
>> >> mounts that need this can set this.  That new flag can perhaps be called
>> >> SB_I_IMA_FAIL.
>> >> 
>> >> Among other things that should allow the policy of when to set this to
>> >> be set in fuse where it is obvious rather than in an magic location in
>> >> IMA.
>> >
>> > Using s_iflags instead of fs_flags is fine, but I'm not sure how this
>> > affects the IMA policy.  This patch set assumes only unprivileged,
>> > untrusted filesytems can automatically fail file signature
>> > verification (2nd patch), as that hasn't yet been upstreamed and won't
>> > break userspace.
>> >
>> > Based on policy, IMA should additionally be able to fail the signature
>> > verification for files on privileged, untrusted filesystems.
>> 
>> Apologies ima has a very specific meaning of policy, as in the loaded
>> ima policy.  I was meaning the hard coded policy of which filesystems
>> we simply would not trust by default.
>> 
>> In code terms what I was thinking would look something like:
>> 
>> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>> --- a/security/integrity/ima/ima_appraise.c
>> +++ b/security/integrity/ima/ima_appraise.c
>> @@ -292,7 +292,14 @@ int ima_appraise_measurement(enum ima_hooks func,
>>  	}
>>   
>>  out:
>> -	if (status != INTEGRITY_PASS) {
>> +	/* Fail if we can't trust the fs enough to support ima xattrs (FUSE) */
>> +	if (inode->i_sb->s_iflags & SB_I_NOIMA) {
>> +		status = INTEGRITY_FAIL;
>> +		cause = "untrusted-filesystem";
>> +		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
>> +				    op, cause, rc, 0);
>> +	} else if (status != INTEGRITY_PASS) {
>>  		if ((ima_appraise & IMA_APPRAISE_FIX) &&
>>  		    (!xattr_value ||
>>   		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
>> 
>> And somewhere in the fuse mount code it would say:
>> 
>> 	if (sb->s_user_ns != &init_user_ns)
>>         	sb->s_iflags |= SB_I_NOIMA);
>
> SB_I_NOIMA would be really confusing, as we're not disabling IMA in
> general, just failing the signature verification.  The measurement,
> even if it is meaningless, is an indication in the measurement list
> that the file was accessed/executed.
>
> I would propose naming the flag SB_I_IMA_UNTRUSTED, meaning the kernel
> is unaware of fs changes.

Fair enough on the naming.  You understand the nuiances the better than
I.


>> The point being that the logic for setting the flag can live in fuse or
>> a simpler filesystem and all ima proper needs to do is deal with the flag being
>> set.  That should be easier to maintainer and simpler to code all
>> around.
>
> The last patch (4/4) had 1 line, which set the fs_flags
> unconditionally in fuse_fs_type.  Instead, we can set the sb->s_iflags 
> in fuse_fill_supper(), again unconditionally, letting IMA-appraisal
> differentiate between privileged and unprivileged.

I really think that is a bad way to structure the code.

I strongly suspect when everything settles down the test needs to be:

	if (!fc->allow_other || (sb->s_user_ns != &init_user_ns))
        	sb->s_iflags |= SB_I_IMA_UNTRUSTED;

fc->allow_other is only set on a very trusted mount of fuse.

Or it might be that fuse decides that breaking it's users by default is
a good idea and always sets SB_I_IMA_UNTRUSTED.  Unless a new mount
option is specified.

How much trust we place in the file server in general is a fuse specific
problem, that the fuse kernel code should handle.  So half of the test
should not reside in IMA and half of the test in the fuse filesystem
code.

The test should reside in fuse and the IMA code should honor it.

I suspect for nfs the situation will be different.  By default we will
trust the server but there will be situations we don't want to and
having a mount option that changes that default would be nice.  (Plus
maybe someday unprivileged nfs mounts that we never want to trust).

I can also see making decisions like this based on the question are the
network transfers authenticated aka signed.

Everything I am seeing says that it makes a lot of sense to have the
capability in the ima code, and the decision to use that capability in
the trust kernel part of the fs code.  As far as I can see that is a
proper separation of responsibilities.

Eric

Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

[Mimi Zohar]

On Fri, 2018-02-16 at 11:48 -0600, Eric W. Biederman wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> 
> > On Thu, 2018-02-15 at 10:47 -0600, Eric W. Biederman wrote:
> >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> >> 
> >> > On Wed, 2018-02-14 at 17:57 -0600, Eric W. Biederman wrote:
> >> >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> >> >> 
> >> >> > Files on untrusted filesystems, such as fuse, can change at any time,
> >> >> > making the measurement(s) and by extension signature verification
> >> >> > meaningless.
> >> >> >
> >> >> > 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.
> >> >> >
> >> >> > This patch always fails the file signature verification on unprivileged
> >> >> > and untrusted filesystems.  To also fail file signature verification on
> >> >> > privileged, untrusted filesystems requires a custom policy.
> >> >> >
> >> >> > (This patch is based on Alban Crequy's use of fs_flags and patch
> >> >> >  description.)
> >> >> 
> >> >> This would be much better done based on a flag in s_iflags and then the
> >> >> mounts that need this can set this.  That new flag can perhaps be called
> >> >> SB_I_IMA_FAIL.
> >> >> 
> >> >> Among other things that should allow the policy of when to set this to
> >> >> be set in fuse where it is obvious rather than in an magic location in
> >> >> IMA.
> >> >
> >> > Using s_iflags instead of fs_flags is fine, but I'm not sure how this
> >> > affects the IMA policy.  This patch set assumes only unprivileged,
> >> > untrusted filesytems can automatically fail file signature
> >> > verification (2nd patch), as that hasn't yet been upstreamed and won't
> >> > break userspace.
> >> >
> >> > Based on policy, IMA should additionally be able to fail the signature
> >> > verification for files on privileged, untrusted filesystems.
> >> 
> >> Apologies ima has a very specific meaning of policy, as in the loaded
> >> ima policy.  I was meaning the hard coded policy of which filesystems
> >> we simply would not trust by default.
> >> 
> >> In code terms what I was thinking would look something like:
> >> 
> >> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> >> --- a/security/integrity/ima/ima_appraise.c
> >> +++ b/security/integrity/ima/ima_appraise.c
> >> @@ -292,7 +292,14 @@ int ima_appraise_measurement(enum ima_hooks func,
> >>  	}
> >>   
> >>  out:
> >> -	if (status != INTEGRITY_PASS) {
> >> +	/* Fail if we can't trust the fs enough to support ima xattrs (FUSE) */
> >> +	if (inode->i_sb->s_iflags & SB_I_NOIMA) {
> >> +		status = INTEGRITY_FAIL;
> >> +		cause = "untrusted-filesystem";
> >> +		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> >> +				    op, cause, rc, 0);
> >> +	} else if (status != INTEGRITY_PASS) {
> >>  		if ((ima_appraise & IMA_APPRAISE_FIX) &&
> >>  		    (!xattr_value ||
> >>   		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> >> 
> >> And somewhere in the fuse mount code it would say:
> >> 
> >> 	if (sb->s_user_ns != &init_user_ns)
> >>         	sb->s_iflags |= SB_I_NOIMA);
> >
> > SB_I_NOIMA would be really confusing, as we're not disabling IMA in
> > general, just failing the signature verification.  The measurement,
> > even if it is meaningless, is an indication in the measurement list
> > that the file was accessed/executed.
> >
> > I would propose naming the flag SB_I_IMA_UNTRUSTED, meaning the kernel
> > is unaware of fs changes.
> 
> Fair enough on the naming.  You understand the nuiances the better than
> I.
> 
> 
> >> The point being that the logic for setting the flag can live in fuse or
> >> a simpler filesystem and all ima proper needs to do is deal with the flag being
> >> set.  That should be easier to maintainer and simpler to code all
> >> around.
> >
> > The last patch (4/4) had 1 line, which set the fs_flags
> > unconditionally in fuse_fs_type.  Instead, we can set the sb->s_iflags 
> > in fuse_fill_supper(), again unconditionally, letting IMA-appraisal
> > differentiate between privileged and unprivileged.
> 
> I really think that is a bad way to structure the code.
> 
> I strongly suspect when everything settles down the test needs to be:
> 
> 	if (!fc->allow_other || (sb->s_user_ns != &init_user_ns))
>         	sb->s_iflags |= SB_I_IMA_UNTRUSTED;
> 
> fc->allow_other is only set on a very trusted mount of fuse.
> 
> Or it might be that fuse decides that breaking it's users by default is
> a good idea and always sets SB_I_IMA_UNTRUSTED.  Unless a new mount
> option is specified.
> 
> How much trust we place in the file server in general is a fuse specific
> problem, that the fuse kernel code should handle.  So half of the test
> should not reside in IMA and half of the test in the fuse filesystem
> code.
> 
> The test should reside in fuse and the IMA code should honor it.

This would all make a lot of sense if the privileged use case was
really trusted, but it isn't.  We're just not automatically failing
the signature verification, because it "might" break existing
userspace.

This now puts the burden of knowing which file systems are inherently
not trusted on the IMA custom policy writer, requiring separate per
file system type or name rules. 

The current code first checks to see if the filesystem is untrusted,
before failing the signature verification.  So existing generic policy
rules could be rewritten, by simply appending "fail" to them.

> I suspect for nfs the situation will be different.  By default we will
> trust the server but there will be situations we don't want to and
> having a mount option that changes that default would be nice.  (Plus
> maybe someday unprivileged nfs mounts that we never want to trust).
> 
> I can also see making decisions like this based on the question are the
> network transfers authenticated aka signed.

At least for the time being, remote file systems are untrusted.  The
main use case for IMA has been in the embedded environment.

Mimi

> Everything I am seeing says that it makes a lot of sense to have the
> capability in the ima code, and the decision to use that capability in
> the trust kernel part of the fs code.  As far as I can see that is a
> proper separation of responsibilities.
> 
> Eric
> 

Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

[Eric W. Biederman]

Mimi Zohar <zohar@linux.vnet.ibm.com> writes:

> On Fri, 2018-02-16 at 11:48 -0600, Eric W. Biederman wrote:
>> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
>> 
>> > On Thu, 2018-02-15 at 10:47 -0600, Eric W. Biederman wrote:
>> >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
>> >> 
>> >> > On Wed, 2018-02-14 at 17:57 -0600, Eric W. Biederman wrote:
>> >> >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
>> >> >> 
>> >> >> > Files on untrusted filesystems, such as fuse, can change at any time,
>> >> >> > making the measurement(s) and by extension signature verification
>> >> >> > meaningless.
>> >> >> >
>> >> >> > 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.
>> >> >> >
>> >> >> > This patch always fails the file signature verification on unprivileged
>> >> >> > and untrusted filesystems.  To also fail file signature verification on
>> >> >> > privileged, untrusted filesystems requires a custom policy.
>> >> >> >
>> >> >> > (This patch is based on Alban Crequy's use of fs_flags and patch
>> >> >> >  description.)
>> >> >> 
>> >> >> This would be much better done based on a flag in s_iflags and then the
>> >> >> mounts that need this can set this.  That new flag can perhaps be called
>> >> >> SB_I_IMA_FAIL.
>> >> >> 
>> >> >> Among other things that should allow the policy of when to set this to
>> >> >> be set in fuse where it is obvious rather than in an magic location in
>> >> >> IMA.
>> >> >
>> >> > Using s_iflags instead of fs_flags is fine, but I'm not sure how this
>> >> > affects the IMA policy.  This patch set assumes only unprivileged,
>> >> > untrusted filesytems can automatically fail file signature
>> >> > verification (2nd patch), as that hasn't yet been upstreamed and won't
>> >> > break userspace.
>> >> >
>> >> > Based on policy, IMA should additionally be able to fail the signature
>> >> > verification for files on privileged, untrusted filesystems.
>> >> 
>> >> Apologies ima has a very specific meaning of policy, as in the loaded
>> >> ima policy.  I was meaning the hard coded policy of which filesystems
>> >> we simply would not trust by default.
>> >> 
>> >> In code terms what I was thinking would look something like:
>> >> 
>> >> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>> >> --- a/security/integrity/ima/ima_appraise.c
>> >> +++ b/security/integrity/ima/ima_appraise.c
>> >> @@ -292,7 +292,14 @@ int ima_appraise_measurement(enum ima_hooks func,
>> >>  	}
>> >>   
>> >>  out:
>> >> -	if (status != INTEGRITY_PASS) {
>> >> +	/* Fail if we can't trust the fs enough to support ima xattrs (FUSE) */
>> >> +	if (inode->i_sb->s_iflags & SB_I_NOIMA) {
>> >> +		status = INTEGRITY_FAIL;
>> >> +		cause = "untrusted-filesystem";
>> >> +		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
>> >> +				    op, cause, rc, 0);
>> >> +	} else if (status != INTEGRITY_PASS) {
>> >>  		if ((ima_appraise & IMA_APPRAISE_FIX) &&
>> >>  		    (!xattr_value ||
>> >>   		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
>> >> 
>> >> And somewhere in the fuse mount code it would say:
>> >> 
>> >> 	if (sb->s_user_ns != &init_user_ns)
>> >>         	sb->s_iflags |= SB_I_NOIMA);
>> >
>> > SB_I_NOIMA would be really confusing, as we're not disabling IMA in
>> > general, just failing the signature verification.  The measurement,
>> > even if it is meaningless, is an indication in the measurement list
>> > that the file was accessed/executed.
>> >
>> > I would propose naming the flag SB_I_IMA_UNTRUSTED, meaning the kernel
>> > is unaware of fs changes.
>> 
>> Fair enough on the naming.  You understand the nuiances the better than
>> I.
>> 
>> 
>> >> The point being that the logic for setting the flag can live in fuse or
>> >> a simpler filesystem and all ima proper needs to do is deal with the flag being
>> >> set.  That should be easier to maintainer and simpler to code all
>> >> around.
>> >
>> > The last patch (4/4) had 1 line, which set the fs_flags
>> > unconditionally in fuse_fs_type.  Instead, we can set the sb->s_iflags 
>> > in fuse_fill_supper(), again unconditionally, letting IMA-appraisal
>> > differentiate between privileged and unprivileged.
>> 
>> I really think that is a bad way to structure the code.
>> 
>> I strongly suspect when everything settles down the test needs to be:
>> 
>> 	if (!fc->allow_other || (sb->s_user_ns != &init_user_ns))
>>         	sb->s_iflags |= SB_I_IMA_UNTRUSTED;
>> 
>> fc->allow_other is only set on a very trusted mount of fuse.
>> 
>> Or it might be that fuse decides that breaking it's users by default is
>> a good idea and always sets SB_I_IMA_UNTRUSTED.  Unless a new mount
>> option is specified.
>> 
>> How much trust we place in the file server in general is a fuse specific
>> problem, that the fuse kernel code should handle.  So half of the test
>> should not reside in IMA and half of the test in the fuse filesystem
>> code.
>> 
>> The test should reside in fuse and the IMA code should honor it.
>
> This would all make a lot of sense if the privileged use case was
> really trusted, but it isn't.  We're just not automatically failing
> the signature verification, because it "might" break existing
> userspace.
>
> This now puts the burden of knowing which file systems are inherently
> not trusted on the IMA custom policy writer, requiring separate per
> file system type or name rules. 
>
> The current code first checks to see if the filesystem is untrusted,
> before failing the signature verification.  So existing generic policy
> rules could be rewritten, by simply appending "fail" to them.

Ugh.  That does seem to be writing the code the wrong way around to
avoid the wrath of Linus.

>> I suspect for nfs the situation will be different.  By default we will
>> trust the server but there will be situations we don't want to and
>> having a mount option that changes that default would be nice.  (Plus
>> maybe someday unprivileged nfs mounts that we never want to trust).
>> 
>> I can also see making decisions like this based on the question are the
>> network transfers authenticated aka signed.
>
> At least for the time being, remote file systems are untrusted.  The
> main use case for IMA has been in the embedded environment.

Then just set SB_I_IMA_UNTRUSTED on all of the remote filesystems.  One
patch per.  You know it people aren't doing that.  Tell Linus you know
no one is doing that, and revert the patch if someone reports a
regression.

If you would like I can carry the patches in my tree (with your
signed-off-by) and I can tell Linus's for you.

Linus has yelled at me in the past for making code overly complicated to
avoid the potential for regression when we know no one in that situation
is using the feature.  This appears to be precisely one of those times.

No one cares, so let's code this clean.

Eric

Re: [RFC PATCH 2/4] ima: fail signature verification on unprivileged & untrusted filesystems

[Mimi Zohar]

On Sat, 2018-02-17 at 08:20 -0600, Eric W. Biederman wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> 
> > On Fri, 2018-02-16 at 11:48 -0600, Eric W. Biederman wrote:
> >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> >> 
> >> > On Thu, 2018-02-15 at 10:47 -0600, Eric W. Biederman wrote:
> >> >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> >> >> 
> >> >> > On Wed, 2018-02-14 at 17:57 -0600, Eric W. Biederman wrote:
> >> >> >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> >> >> >> 
> >> >> >> > Files on untrusted filesystems, such as fuse, can change at any time,
> >> >> >> > making the measurement(s) and by extension signature verification
> >> >> >> > meaningless.
> >> >> >> >
> >> >> >> > 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.
> >> >> >> >
> >> >> >> > This patch always fails the file signature verification on unprivileged
> >> >> >> > and untrusted filesystems.  To also fail file signature verification on
> >> >> >> > privileged, untrusted filesystems requires a custom policy.
> >> >> >> >
> >> >> >> > (This patch is based on Alban Crequy's use of fs_flags and patch
> >> >> >> >  description.)
> >> >> >> 
> >> >> >> This would be much better done based on a flag in s_iflags and then the
> >> >> >> mounts that need this can set this.  That new flag can perhaps be called
> >> >> >> SB_I_IMA_FAIL.
> >> >> >> 
> >> >> >> Among other things that should allow the policy of when to set this to
> >> >> >> be set in fuse where it is obvious rather than in an magic location in
> >> >> >> IMA.
> >> >> >
> >> >> > Using s_iflags instead of fs_flags is fine, but I'm not sure how this
> >> >> > affects the IMA policy.  This patch set assumes only unprivileged,
> >> >> > untrusted filesytems can automatically fail file signature
> >> >> > verification (2nd patch), as that hasn't yet been upstreamed and won't
> >> >> > break userspace.
> >> >> >
> >> >> > Based on policy, IMA should additionally be able to fail the signature
> >> >> > verification for files on privileged, untrusted filesystems.
> >> >> 
> >> >> Apologies ima has a very specific meaning of policy, as in the loaded
> >> >> ima policy.  I was meaning the hard coded policy of which filesystems
> >> >> we simply would not trust by default.
> >> >> 
> >> >> In code terms what I was thinking would look something like:
> >> >> 
> >> >> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> >> >> --- a/security/integrity/ima/ima_appraise.c
> >> >> +++ b/security/integrity/ima/ima_appraise.c
> >> >> @@ -292,7 +292,14 @@ int ima_appraise_measurement(enum ima_hooks func,
> >> >>  	}
> >> >>   
> >> >>  out:
> >> >> -	if (status != INTEGRITY_PASS) {
> >> >> +	/* Fail if we can't trust the fs enough to support ima xattrs (FUSE) */
> >> >> +	if (inode->i_sb->s_iflags & SB_I_NOIMA) {
> >> >> +		status = INTEGRITY_FAIL;
> >> >> +		cause = "untrusted-filesystem";
> >> >> +		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> >> >> +				    op, cause, rc, 0);
> >> >> +	} else if (status != INTEGRITY_PASS) {
> >> >>  		if ((ima_appraise & IMA_APPRAISE_FIX) &&
> >> >>  		    (!xattr_value ||
> >> >>   		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> >> >> 
> >> >> And somewhere in the fuse mount code it would say:
> >> >> 
> >> >> 	if (sb->s_user_ns != &init_user_ns)
> >> >>         	sb->s_iflags |= SB_I_NOIMA);
> >> >
> >> > SB_I_NOIMA would be really confusing, as we're not disabling IMA in
> >> > general, just failing the signature verification.  The measurement,
> >> > even if it is meaningless, is an indication in the measurement list
> >> > that the file was accessed/executed.
> >> >
> >> > I would propose naming the flag SB_I_IMA_UNTRUSTED, meaning the kernel
> >> > is unaware of fs changes.
> >> 
> >> Fair enough on the naming.  You understand the nuiances the better than
> >> I.
> >> 
> >> 
> >> >> The point being that the logic for setting the flag can live in fuse or
> >> >> a simpler filesystem and all ima proper needs to do is deal with the flag being
> >> >> set.  That should be easier to maintainer and simpler to code all
> >> >> around.
> >> >
> >> > The last patch (4/4) had 1 line, which set the fs_flags
> >> > unconditionally in fuse_fs_type.  Instead, we can set the sb->s_iflags 
> >> > in fuse_fill_supper(), again unconditionally, letting IMA-appraisal
> >> > differentiate between privileged and unprivileged.
> >> 
> >> I really think that is a bad way to structure the code.
> >> 
> >> I strongly suspect when everything settles down the test needs to be:
> >> 
> >> 	if (!fc->allow_other || (sb->s_user_ns != &init_user_ns))
> >>         	sb->s_iflags |= SB_I_IMA_UNTRUSTED;
> >> 
> >> fc->allow_other is only set on a very trusted mount of fuse.
> >> 
> >> Or it might be that fuse decides that breaking it's users by default is
> >> a good idea and always sets SB_I_IMA_UNTRUSTED.  Unless a new mount
> >> option is specified.
> >> 
> >> How much trust we place in the file server in general is a fuse specific
> >> problem, that the fuse kernel code should handle.  So half of the test
> >> should not reside in IMA and half of the test in the fuse filesystem
> >> code.
> >> 
> >> The test should reside in fuse and the IMA code should honor it.
> >
> > This would all make a lot of sense if the privileged use case was
> > really trusted, but it isn't.  We're just not automatically failing
> > the signature verification, because it "might" break existing
> > userspace.
> >
> > This now puts the burden of knowing which file systems are inherently
> > not trusted on the IMA custom policy writer, requiring separate per
> > file system type or name rules. 
> >
> > The current code first checks to see if the filesystem is untrusted,
> > before failing the signature verification.  So existing generic policy
> > rules could be rewritten, by simply appending "fail" to them.
> 
> Ugh.  That does seem to be writing the code the wrong way around to
> avoid the wrath of Linus.

Linus doesn't have anything to do with this.  I'm the one trying to
differentiate between non-init unprivileged mounts, which hasn't yet
been upstreamed, from the setuid unprivileged and privileged mounts.

I've just posted v1 of this patch set, which is simpler and,
hopefully, clearer.

Mimi

> 
> >> I suspect for nfs the situation will be different.  By default we will
> >> trust the server but there will be situations we don't want to and
> >> having a mount option that changes that default would be nice.  (Plus
> >> maybe someday unprivileged nfs mounts that we never want to trust).
> >> 
> >> I can also see making decisions like this based on the question are the
> >> network transfers authenticated aka signed.
> >
> > At least for the time being, remote file systems are untrusted.  The
> > main use case for IMA has been in the embedded environment.
> 
> Then just set SB_I_IMA_UNTRUSTED on all of the remote filesystems.  One
> patch per.  You know it people aren't doing that.  Tell Linus you know
> no one is doing that, and revert the patch if someone reports a
> regression.
> 
> If you would like I can carry the patches in my tree (with your
> signed-off-by) and I can tell Linus's for you.
> 
> Linus has yelled at me in the past for making code overly complicated to
> avoid the potential for regression when we know no one in that situation
> is using the feature.  This appears to be precisely one of those times.
> 
> No one cares, so let's code this clean.
> 
> Eric
>