[PATCH v2 0/4] ima: unverifiable file signatures

[PATCH v2 0/4] ima: unverifiable file signatures

[Mimi Zohar]

For local filesystems, the kernel prevents files being executed from
being modified.  With IMA-measurement enabled, the kernel also emits
audit "time of measure, time of use" messages for files opened for
read, and subsequently opened for write.

Files on fuse are initially measured, appraised, and audited.  Although
the file data can change dynamically any time, making re-measuring,
re-appraising, or re-auditing pointless, this patch set attempts to
differentiate between unprivileged non-init root and privileged
mounted fuse filesystems.

This patch set addresses three different scenarios:
- Unprivileged non-init root mounted fuse filesystems are untrusted.
  Signature verification should always fail and re-measuring,
  re-appraising, re-auditing files makes no sense.

  Always enabled.

- For privileged mounted filesystems in a "secure" environment, with a
  correctly enforced security policy, which is willing to assume the
  inherent risk of specific fuse filesystems, it is reasonable to
  re-measure, re-appraise, and re-audit files.

  Enabled by default to prevent breaking existing systems.

- Privileged mounted filesystems unwilling to assume the risks and
  prefers to fail safe.

  Enabled based on policy.

Mimi

Mimi Zohar (4):
  ima: fail file signature verification on non-init mounted filesystems
  ima: re-evaluate files on privileged mounted filesystems
  ima: fail signature verification based on policy
  fuse: define the filesystem as untrusted

 Documentation/admin-guide/kernel-parameters.txt |  8 +++++++-
 fs/fuse/inode.c                                 |  3 +++
 include/linux/fs.h                              |  2 ++
 security/integrity/ima/ima_appraise.c           | 16 +++++++++++++++-
 security/integrity/ima/ima_main.c               | 14 ++++++++++++--
 security/integrity/ima/ima_policy.c             |  5 +++++
 security/integrity/integrity.h                  |  1 +
 7 files changed, 45 insertions(+), 4 deletions(-)

-- 
2.7.5

[PATCH v2 1/4] ima: fail file signature verification on non-init mounted filesystems

[Mimi Zohar]

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 addresses the new unprivileged non-init mounted filesystems,
which are untrusted, by failing the signature verification.

This patch defines two new flags SB_I_IMA_UNVERIFIABLE_SIGNATURE and
SB_I_UNTRUSTED_MOUNTER.

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>

Changelog v2:
- Limit patch to non-init mounted filesystems.
- Define 2 sb->s_iflags

Changelog v1:
- Merged the unprivileged and privileged patches.
- Dropped IMA fsname support.
- Introduced a new IMA builtin policy named "untrusted_fs".
- Replaced fs_type flag with sb->s_iflags flag.
---
 include/linux/fs.h                    |  2 ++
 security/integrity/ima/ima_appraise.c | 14 +++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2a815560fda0..4e1c76af7b68 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1320,6 +1320,8 @@ extern int send_sigurg(struct fown_struct *fown);
 
 /* sb->s_iflags to limit user namespace mounts */
 #define SB_I_USERNS_VISIBLE		0x00000010 /* fstype already mounted */
+#define SB_I_IMA_UNVERIFIABLE_SIGNATURE	0x00000020
+#define SB_I_UNTRUSTED_MOUNTER		0x00000040
 
 /* Possible states of 'frozen' field */
 enum {
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 1b177461f20e..f34901069e78 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -302,7 +302,18 @@ int ima_appraise_measurement(enum ima_hooks func,
 	}
 
 out:
-	if (status != INTEGRITY_PASS) {
+	/*
+	 * File signatures on some filesystems can not be properly verified.
+	 * On these filesytems, that are mounted by an untrusted mounter,
+	 * fail the file signature verification.
+	 */
+	if (inode->i_sb->s_iflags &
+	    (SB_I_IMA_UNVERIFIABLE_SIGNATURE | SB_I_UNTRUSTED_MOUNTER)) {
+		status = INTEGRITY_FAIL;
+		cause = "unverifiable-signature";
+		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)) {
@@ -319,6 +330,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

[PATCH v2 2/4] ima: re-evaluate files on privileged mounted filesystems

[Mimi Zohar]

This patch addresses the fuse privileged mounted filesystems in a "secure"
environment, with a correctly enforced security policy, which is willing
to assume the inherent risk of specific fuse filesystems that are well
defined and properly implemented.

As there is no way for the kernel to detect file changes, the kernel
ignores the cached file integrity results and re-measures, re-appraises,
and re-audits the file.

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>
---
 security/integrity/ima/ima_main.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index a5d225ffc388..f550f25294a3 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"
 
@@ -230,9 +231,17 @@ static int process_measurement(struct file *file, const struct cred *cred,
 				 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 */
+	/*
+	 * Re-evaulate the file if either the xattr has changed or the
+	 * kernel has no way of detecting file change on the filesystem.
+	 * (Limited to privileged mounted filesystems.)
+	 */
+	if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags) ||
+	    ((inode->i_sb->s_iflags & SB_I_IMA_UNVERIFIABLE_SIGNATURE) &&
+	     !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER))) {
 		iint->flags &= ~IMA_DONE_MASK;
+		iint->measured_pcrs = 0;
+	}
 
 	/* Determine if already appraised/measured based on bitmask
 	 * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
-- 
2.7.5

[PATCH v2 3/4] ima: fail signature verification based on policy

[Mimi Zohar]

This patch addresses the fuse privileged mounted filesystems in
environments which are unwilling to accept the risk of trusting the
signature verification and want to always fail safe, but are for
example using a pre-built kernel.

This patch defines a new builtin policy "unverifiable_sigs", which can
be specified on the boot command line as an argument to "ima_policy=".

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>

Changelog v2:
- address the fail safe environement
---
 Documentation/admin-guide/kernel-parameters.txt |  8 +++++++-
 security/integrity/ima/ima_appraise.c           | 10 ++++++----
 security/integrity/ima/ima_main.c               |  3 ++-
 security/integrity/ima/ima_policy.c             |  5 +++++
 security/integrity/integrity.h                  |  1 +
 5 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..c655cd8dbaa0 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1525,7 +1525,8 @@
 
 	ima_policy=	[IMA]
 			The builtin policies to load during IMA setup.
-			Format: "tcb | appraise_tcb | secure_boot"
+			Format: "tcb | appraise_tcb | secure_boot |
+				 unverifiable_sigs"
 
 			The "tcb" policy measures all programs exec'd, files
 			mmap'd for exec, and all files opened with the read
@@ -1540,6 +1541,11 @@
 			of files (eg. kexec kernel image, kernel modules,
 			firmware, policy, etc) based on file signatures.
 
+			The "unverifiable_sigs" policy forces file signature
+			verification failure on privileged mounted
+			filesystems with the SB_I_UNVERIFIABLE_SIGNATURE
+			flag.
+
 	ima_tcb		[IMA] Deprecated.  Use ima_policy= instead.
 			Load a policy which meets the needs of the Trusted
 			Computing Base.  This means IMA will measure all
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index f34901069e78..3034935e1eb3 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -304,11 +304,13 @@ int ima_appraise_measurement(enum ima_hooks func,
 out:
 	/*
 	 * File signatures on some filesystems can not be properly verified.
-	 * On these filesytems, that are mounted by an untrusted mounter,
-	 * fail the file signature verification.
+	 * On these filesytems, that are mounted by an untrusted mounter or
+	 * for systems not willing to accept the risk, fail the file signature
+	 * verification.
 	 */
-	if (inode->i_sb->s_iflags &
-	    (SB_I_IMA_UNVERIFIABLE_SIGNATURE | SB_I_UNTRUSTED_MOUNTER)) {
+	if ((inode->i_sb->s_iflags & SB_I_IMA_UNVERIFIABLE_SIGNATURE) &&
+	    ((inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) ||
+	     (iint->flags & IMA_FAIL_UNVERIFIABLE_SIGS))) {
 		status = INTEGRITY_FAIL;
 		cause = "unverifiable-signature";
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index f550f25294a3..5d122daf5c8a 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -238,7 +238,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
 	 */
 	if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags) ||
 	    ((inode->i_sb->s_iflags & SB_I_IMA_UNVERIFIABLE_SIGNATURE) &&
-	     !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER))) {
+	     !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) &&
+	     !(action & IMA_FAIL_UNVERIFIABLE_SIGS))) {
 		iint->flags &= ~IMA_DONE_MASK;
 		iint->measured_pcrs = 0;
 	}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e3da29af2c16..ead3f7fe6998 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -188,6 +188,7 @@ __setup("ima_tcb", default_measure_policy_setup);
 
 static bool ima_use_appraise_tcb __initdata;
 static bool ima_use_secure_boot __initdata;
+static bool ima_fail_unverifiable_sigs __ro_after_init;
 static int __init policy_setup(char *str)
 {
 	char *p;
@@ -201,6 +202,8 @@ static int __init policy_setup(char *str)
 			ima_use_appraise_tcb = true;
 		else if (strcmp(p, "secure_boot") == 0)
 			ima_use_secure_boot = true;
+		else if (strcmp(p, "unverifiable_sigs") == 0)
+			ima_fail_unverifiable_sigs = true;
 	}
 
 	return 1;
@@ -390,6 +393,8 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
 		if (entry->action & IMA_APPRAISE) {
 			action |= get_subaction(entry, func);
 			action ^= IMA_HASH;
+			if (ima_fail_unverifiable_sigs)
+				action |= IMA_FAIL_UNVERIFIABLE_SIGS;
 		}
 
 		if (entry->action & IMA_DO_MASK)
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 843ae23ba0ac..8224880935e0 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -35,6 +35,7 @@
 #define IMA_PERMIT_DIRECTIO	0x02000000
 #define IMA_NEW_FILE		0x04000000
 #define EVM_IMMUTABLE_DIGSIG	0x08000000
+#define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
 
 #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
 				 IMA_HASH | IMA_APPRAISE_SUBMASK)
-- 
2.7.5

[PATCH v2 4/4] fuse: define the filesystem as untrusted

[Mimi Zohar]

Files on FUSE can change at any point in time without IMA being able
to detect it.  The file data read for the file signature verification
could be totally different from what is subsequently read, making the
signature verification useless.

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 sets the SB_I_IMA_UNVERIFIABLE_SIGNATURE flag and when
appropriate sets the SB_I_UNTRUSTED_MOUNTER flag.

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>
---
 fs/fuse/inode.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 624f18bbfd2b..ef309958e060 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1080,6 +1080,9 @@ static int fuse_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_maxbytes = MAX_LFS_FILESIZE;
 	sb->s_time_gran = 1;
 	sb->s_export_op = &fuse_export_operations;
+	sb->s_iflags |= SB_I_IMA_UNVERIFIABLE_SIGNATURE;
+	if (sb->s_user_ns != &init_user_ns)
+		sb->s_iflags |= SB_I_UNTRUSTED_MOUNTER;
 
 	file = fget(d.fd);
 	err = -EINVAL;
-- 
2.7.5

Re: [PATCH v2 0/4] ima: unverifiable file signatures

[James Morris]

On Thu, 22 Feb 2018, Mimi Zohar wrote:

> For local filesystems, the kernel prevents files being executed from
> being modified.  With IMA-measurement enabled, the kernel also emits
> audit "time of measure, time of use" messages for files opened for
> read, and subsequently opened for write.
> 
> Files on fuse are initially measured, appraised, and audited.  Although
> the file data can change dynamically any time, making re-measuring,
> re-appraising, or re-auditing pointless, this patch set attempts to
> differentiate between unprivileged non-init root and privileged
> mounted fuse filesystems.
> 
> This patch set addresses three different scenarios:
> - Unprivileged non-init root mounted fuse filesystems are untrusted.
>   Signature verification should always fail and re-measuring,
>   re-appraising, re-auditing files makes no sense.
> 
>   Always enabled.
> 
> - For privileged mounted filesystems in a "secure" environment, with a
>   correctly enforced security policy, which is willing to assume the
>   inherent risk of specific fuse filesystems, it is reasonable to
>   re-measure, re-appraise, and re-audit files.
> 
>   Enabled by default to prevent breaking existing systems.
> 
> - Privileged mounted filesystems unwilling to assume the risks and
>   prefers to fail safe.
> 
>   Enabled based on policy.

I like this approach.


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH v2 1/4] ima: fail file signature verification on non-init mounted filesystems

[Eric W. Biederman]

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

> 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 addresses the new unprivileged non-init mounted filesystems,
> which are untrusted, by failing the signature verification.
>
> This patch defines two new flags SB_I_IMA_UNVERIFIABLE_SIGNATURE and
> SB_I_UNTRUSTED_MOUNTER.

I don't belive this patch matches your intent.

> 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>
>
> Changelog v2:
> - Limit patch to non-init mounted filesystems.
> - Define 2 sb->s_iflags
>
> Changelog v1:
> - Merged the unprivileged and privileged patches.
> - Dropped IMA fsname support.
> - Introduced a new IMA builtin policy named "untrusted_fs".
> - Replaced fs_type flag with sb->s_iflags flag.
> ---
>  include/linux/fs.h                    |  2 ++
>  security/integrity/ima/ima_appraise.c | 14 +++++++++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2a815560fda0..4e1c76af7b68 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1320,6 +1320,8 @@ extern int send_sigurg(struct fown_struct *fown);
>  
>  /* sb->s_iflags to limit user namespace mounts */
>  #define SB_I_USERNS_VISIBLE		0x00000010 /* fstype already mounted */
> +#define SB_I_IMA_UNVERIFIABLE_SIGNATURE	0x00000020
> +#define SB_I_UNTRUSTED_MOUNTER		0x00000040
>  
>  /* Possible states of 'frozen' field */
>  enum {
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 1b177461f20e..f34901069e78 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -302,7 +302,18 @@ int ima_appraise_measurement(enum ima_hooks func,
>  	}
>  
>  out:
> -	if (status != INTEGRITY_PASS) {
> +	/*
> +	 * File signatures on some filesystems can not be properly verified.
> +	 * On these filesytems, that are mounted by an untrusted mounter,
> +	 * fail the file signature verification.
> +	 */
> +	if (inode->i_sb->s_iflags &
> +	    (SB_I_IMA_UNVERIFIABLE_SIGNATURE | SB_I_UNTRUSTED_MOUNTER))
> {

I like this test.

This test does not match your comments.  This test returns true if
either SB_I_IMA_UNVERIFIABLE_SIGNATURE or SB_I_UNTRUSTED_MOUNTER.

> +		status = INTEGRITY_FAIL;
> +		cause = "unverifiable-signature";
> +		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)) {

Eric

Re: [PATCH v2 0/4] ima: unverifiable file signatures

[Eric W. Biederman]

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

> For local filesystems, the kernel prevents files being executed from
> being modified.  With IMA-measurement enabled, the kernel also emits
> audit "time of measure, time of use" messages for files opened for
> read, and subsequently opened for write.
>
> Files on fuse are initially measured, appraised, and audited.  Although
> the file data can change dynamically any time, making re-measuring,
> re-appraising, or re-auditing pointless, this patch set attempts to
> differentiate between unprivileged non-init root and privileged
> mounted fuse filesystems.
>
> This patch set addresses three different scenarios:
> - Unprivileged non-init root mounted fuse filesystems are untrusted.
>   Signature verification should always fail and re-measuring,
>   re-appraising, re-auditing files makes no sense.
>
>   Always enabled.
>
> - For privileged mounted filesystems in a "secure" environment, with a
>   correctly enforced security policy, which is willing to assume the
>   inherent risk of specific fuse filesystems, it is reasonable to
>   re-measure, re-appraise, and re-audit files.
>
>   Enabled by default to prevent breaking existing systems.
>
> - Privileged mounted filesystems unwilling to assume the risks and
>   prefers to fail safe.
>
>   Enabled based on policy.

I really like the way the flags work in this patchset.

There is a lot of other nit-picking and bike-shedding I would like to
do.  However those are details specific to IMA.  So my opion really
doesn't count.

Those two flags set as you have them in the last patch make it possible
to slightly alter details of when they get set, that are in the perview
of filesystems without having too big a debate over them.

The changes I imagine most easily are:
In fuse_fill_super:
	if (!fc->allow_other)
		sb->s_iflags |= SB_I_UNTRUSTED_MOUNTER;

In sget_user_ns:
	if (sb->s_user_ns != &init_user_ns)
        	sb->s_iflags |= SB_I_UNTRUSTED_MOUNTER;


My biggest nitpick is I would be inclined to flip the sense of the
unverifiable_sigs policy.  By default not trust and trust only if
a special command line option was given.   But I realize this could
run into backwards compatibility concerns.  And it is IMA specific so
not really my call.

But the important part is what winds up in the core of ima.  Baring
typo's I think you have something we can all live with.

So double check yourself and let's start getting this merged.

Eric

Re: [PATCH v2 1/4] ima: fail file signature verification on non-init mounted filesystems

[Mimi Zohar]

On Mon, 2018-02-26 at 19:47 -0600, Eric W. Biederman wrote:

> > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> > index 1b177461f20e..f34901069e78 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -302,7 +302,18 @@ int ima_appraise_measurement(enum ima_hooks func,
> >  	}
> >  
> >  out:
> > -	if (status != INTEGRITY_PASS) {
> > +	/*
> > +	 * File signatures on some filesystems can not be properly verified.
> > +	 * On these filesytems, that are mounted by an untrusted mounter,
> > +	 * fail the file signature verification.
> > +	 */
> > +	if (inode->i_sb->s_iflags &
> > +	    (SB_I_IMA_UNVERIFIABLE_SIGNATURE | SB_I_UNTRUSTED_MOUNTER))
> > {
> 
> I like this test.
> 
> This test does not match your comments.  This test returns true if
> either SB_I_IMA_UNVERIFIABLE_SIGNATURE or SB_I_UNTRUSTED_MOUNTER.

Thanks, you're right.  The test should have been:

        if ((inode->i_sb->s_iflags &
            (SB_I_IMA_UNVERIFIABLE_SIGNATURE | SB_I_UNTRUSTED_MOUNTER)) ==
            (SB_I_IMA_UNVERIFIABLE_SIGNATURE | SB_I_UNTRUSTED_MOUNTER)) {

Mimi

> 
> > +		status = INTEGRITY_FAIL;
> > +		cause = "unverifiable-signature";
> > +		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)) {
> 
> Eric
> 

Re: [PATCH v2 0/4] ima: unverifiable file signatures

[Mimi Zohar]

On Mon, 2018-02-26 at 20:08 -0600, Eric W. Biederman wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> 
> > For local filesystems, the kernel prevents files being executed from
> > being modified.  With IMA-measurement enabled, the kernel also emits
> > audit "time of measure, time of use" messages for files opened for
> > read, and subsequently opened for write.
> >
> > Files on fuse are initially measured, appraised, and audited.  Although
> > the file data can change dynamically any time, making re-measuring,
> > re-appraising, or re-auditing pointless, this patch set attempts to
> > differentiate between unprivileged non-init root and privileged
> > mounted fuse filesystems.
> >
> > This patch set addresses three different scenarios:
> > - Unprivileged non-init root mounted fuse filesystems are untrusted.
> >   Signature verification should always fail and re-measuring,
> >   re-appraising, re-auditing files makes no sense.
> >
> >   Always enabled.
> >
> > - For privileged mounted filesystems in a "secure" environment, with a
> >   correctly enforced security policy, which is willing to assume the
> >   inherent risk of specific fuse filesystems, it is reasonable to
> >   re-measure, re-appraise, and re-audit files.
> >
> >   Enabled by default to prevent breaking existing systems.
> >
> > - Privileged mounted filesystems unwilling to assume the risks and
> >   prefers to fail safe.
> >
> >   Enabled based on policy.
> 
> I really like the way the flags work in this patchset.
> 
> There is a lot of other nit-picking and bike-shedding I would like to
> do.  However those are details specific to IMA.  So my opion really
> doesn't count.
> 
> Those two flags set as you have them in the last patch make it possible
> to slightly alter details of when they get set, that are in the perview
> of filesystems without having too big a debate over them.
> 
> The changes I imagine most easily are:
> In fuse_fill_super:
> 	if (!fc->allow_other)
> 		sb->s_iflags |= SB_I_UNTRUSTED_MOUNTER;

Right, as described, above, as the 2nd senario.

> 
> In sget_user_ns:
> 	if (sb->s_user_ns != &init_user_ns)
>         	sb->s_iflags |= SB_I_UNTRUSTED_MOUNTER;

The filesystems would then only set SB_I_IMA_UNVERIFIABLE_SIGNATURE.

> 
> My biggest nitpick is I would be inclined to flip the sense of the
> unverifiable_sigs policy.  By default not trust and trust only if
> a special command line option was given.   But I realize this could
> run into backwards compatibility concerns.  And it is IMA specific so
> not really my call.

The boot command line policy option forces the system to fail safe.
Reversing the default to fail unverifiable_sigs and only allow them
based on policy, would not be a simple command line option, but would
require a per filesystem rule.

I agree with you, but as we're not breaking existing userspace, our
only option is to audit/log the concern, as suggested by Linus in
other threads.  It would be nice if we could audit/log it once per
each mount.

> 
> But the important part is what winds up in the core of ima.  Baring
> typo's I think you have something we can all live with.
> 
> So double check yourself and let's start getting this merged.

Sure.

Mimi

Re: [PATCH v2 3/4] ima: fail signature verification based on policy

[Serge E. Hallyn]

Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> This patch addresses the fuse privileged mounted filesystems in
> environments which are unwilling to accept the risk of trusting the
> signature verification and want to always fail safe, but are for
> example using a pre-built kernel.
> 
> This patch defines a new builtin policy "unverifiable_sigs", which can

How about recalc_unverifiable_sigs?  It's long, but unverifiable_sigs
is  not clear about whether the intent is to accept or recalculate them.

(or fail_unverifiable_sigs like the flag)

> be specified on the boot command line as an argument to "ima_policy=".
> 
> 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>
> 
> Changelog v2:
> - address the fail safe environement
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  8 +++++++-
>  security/integrity/ima/ima_appraise.c           | 10 ++++++----
>  security/integrity/ima/ima_main.c               |  3 ++-
>  security/integrity/ima/ima_policy.c             |  5 +++++
>  security/integrity/integrity.h                  |  1 +
>  5 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 1d1d53f85ddd..c655cd8dbaa0 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1525,7 +1525,8 @@
>  
>  	ima_policy=	[IMA]
>  			The builtin policies to load during IMA setup.
> -			Format: "tcb | appraise_tcb | secure_boot"
> +			Format: "tcb | appraise_tcb | secure_boot |
> +				 unverifiable_sigs"
>  
>  			The "tcb" policy measures all programs exec'd, files
>  			mmap'd for exec, and all files opened with the read
> @@ -1540,6 +1541,11 @@
>  			of files (eg. kexec kernel image, kernel modules,
>  			firmware, policy, etc) based on file signatures.
>  
> +			The "unverifiable_sigs" policy forces file signature
> +			verification failure on privileged mounted
> +			filesystems with the SB_I_UNVERIFIABLE_SIGNATURE
> +			flag.
> +
>  	ima_tcb		[IMA] Deprecated.  Use ima_policy= instead.
>  			Load a policy which meets the needs of the Trusted
>  			Computing Base.  This means IMA will measure all
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index f34901069e78..3034935e1eb3 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -304,11 +304,13 @@ int ima_appraise_measurement(enum ima_hooks func,
>  out:
>  	/*
>  	 * File signatures on some filesystems can not be properly verified.
> -	 * On these filesytems, that are mounted by an untrusted mounter,
> -	 * fail the file signature verification.
> +	 * On these filesytems, that are mounted by an untrusted mounter or
> +	 * for systems not willing to accept the risk, fail the file signature
> +	 * verification.
>  	 */
> -	if (inode->i_sb->s_iflags &
> -	    (SB_I_IMA_UNVERIFIABLE_SIGNATURE | SB_I_UNTRUSTED_MOUNTER)) {
> +	if ((inode->i_sb->s_iflags & SB_I_IMA_UNVERIFIABLE_SIGNATURE) &&
> +	    ((inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) ||
> +	     (iint->flags & IMA_FAIL_UNVERIFIABLE_SIGS))) {
>  		status = INTEGRITY_FAIL;
>  		cause = "unverifiable-signature";
>  		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index f550f25294a3..5d122daf5c8a 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -238,7 +238,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
>  	 */
>  	if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags) ||
>  	    ((inode->i_sb->s_iflags & SB_I_IMA_UNVERIFIABLE_SIGNATURE) &&
> -	     !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER))) {
> +	     !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) &&
> +	     !(action & IMA_FAIL_UNVERIFIABLE_SIGS))) {
>  		iint->flags &= ~IMA_DONE_MASK;
>  		iint->measured_pcrs = 0;
>  	}
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index e3da29af2c16..ead3f7fe6998 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -188,6 +188,7 @@ __setup("ima_tcb", default_measure_policy_setup);
>  
>  static bool ima_use_appraise_tcb __initdata;
>  static bool ima_use_secure_boot __initdata;
> +static bool ima_fail_unverifiable_sigs __ro_after_init;
>  static int __init policy_setup(char *str)
>  {
>  	char *p;
> @@ -201,6 +202,8 @@ static int __init policy_setup(char *str)
>  			ima_use_appraise_tcb = true;
>  		else if (strcmp(p, "secure_boot") == 0)
>  			ima_use_secure_boot = true;
> +		else if (strcmp(p, "unverifiable_sigs") == 0)
> +			ima_fail_unverifiable_sigs = true;
>  	}
>  
>  	return 1;
> @@ -390,6 +393,8 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
>  		if (entry->action & IMA_APPRAISE) {
>  			action |= get_subaction(entry, func);
>  			action ^= IMA_HASH;
> +			if (ima_fail_unverifiable_sigs)
> +				action |= IMA_FAIL_UNVERIFIABLE_SIGS;
>  		}
>  
>  		if (entry->action & IMA_DO_MASK)
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 843ae23ba0ac..8224880935e0 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -35,6 +35,7 @@
>  #define IMA_PERMIT_DIRECTIO	0x02000000
>  #define IMA_NEW_FILE		0x04000000
>  #define EVM_IMMUTABLE_DIGSIG	0x08000000
> +#define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
>  
>  #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
>  				 IMA_HASH | IMA_APPRAISE_SUBMASK)
> -- 
> 2.7.5

Re: [PATCH v2 3/4] ima: fail signature verification based on policy

[Mimi Zohar]

On Tue, 2018-02-27 at 16:35 -0600, Serge E. Hallyn wrote:
> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > This patch addresses the fuse privileged mounted filesystems in
> > environments which are unwilling to accept the risk of trusting the
> > signature verification and want to always fail safe, but are for
> > example using a pre-built kernel.
> > 
> > This patch defines a new builtin policy "unverifiable_sigs", which can
> 
> How about recalc_unverifiable_sigs?

Cute, I really like that name, but in this case we're failing the
signature verification.

> It's long, but unverifiable_sigs
> is  not clear about whether the intent is to accept or recalculate them.
> 
> (or fail_unverifiable_sigs like the flag)

Could we abbreviate it to "fail_usigs"?  Or perhaps allow both
"fail_unverifiable_sigs" and "fail_usigs".

Mimi

> 
> > be specified on the boot command line as an argument to "ima_policy=".
> > 
> > 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>
> > 
> > Changelog v2:
> > - address the fail safe environement
> > ---
> >  Documentation/admin-guide/kernel-parameters.txt |  8 +++++++-
> >  security/integrity/ima/ima_appraise.c           | 10 ++++++----
> >  security/integrity/ima/ima_main.c               |  3 ++-
> >  security/integrity/ima/ima_policy.c             |  5 +++++
> >  security/integrity/integrity.h                  |  1 +
> >  5 files changed, 21 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 1d1d53f85ddd..c655cd8dbaa0 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -1525,7 +1525,8 @@
> >  
> >  	ima_policy=	[IMA]
> >  			The builtin policies to load during IMA setup.
> > -			Format: "tcb | appraise_tcb | secure_boot"
> > +			Format: "tcb | appraise_tcb | secure_boot |
> > +				 unverifiable_sigs"
> >  
> >  			The "tcb" policy measures all programs exec'd, files
> >  			mmap'd for exec, and all files opened with the read
> > @@ -1540,6 +1541,11 @@
> >  			of files (eg. kexec kernel image, kernel modules,
> >  			firmware, policy, etc) based on file signatures.
> >  
> > +			The "unverifiable_sigs" policy forces file signature
> > +			verification failure on privileged mounted
> > +			filesystems with the SB_I_UNVERIFIABLE_SIGNATURE
> > +			flag.
> > +
> >  	ima_tcb		[IMA] Deprecated.  Use ima_policy= instead.
> >  			Load a policy which meets the needs of the Trusted
> >  			Computing Base.  This means IMA will measure all
> > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> > index f34901069e78..3034935e1eb3 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -304,11 +304,13 @@ int ima_appraise_measurement(enum ima_hooks func,
> >  out:
> >  	/*
> >  	 * File signatures on some filesystems can not be properly verified.
> > -	 * On these filesytems, that are mounted by an untrusted mounter,
> > -	 * fail the file signature verification.
> > +	 * On these filesytems, that are mounted by an untrusted mounter or
> > +	 * for systems not willing to accept the risk, fail the file signature
> > +	 * verification.
> >  	 */
> > -	if (inode->i_sb->s_iflags &
> > -	    (SB_I_IMA_UNVERIFIABLE_SIGNATURE | SB_I_UNTRUSTED_MOUNTER)) {
> > +	if ((inode->i_sb->s_iflags & SB_I_IMA_UNVERIFIABLE_SIGNATURE) &&
> > +	    ((inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) ||
> > +	     (iint->flags & IMA_FAIL_UNVERIFIABLE_SIGS))) {
> >  		status = INTEGRITY_FAIL;
> >  		cause = "unverifiable-signature";
> >  		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index f550f25294a3..5d122daf5c8a 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -238,7 +238,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
> >  	 */
> >  	if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags) ||
> >  	    ((inode->i_sb->s_iflags & SB_I_IMA_UNVERIFIABLE_SIGNATURE) &&
> > -	     !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER))) {
> > +	     !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) &&
> > +	     !(action & IMA_FAIL_UNVERIFIABLE_SIGS))) {
> >  		iint->flags &= ~IMA_DONE_MASK;
> >  		iint->measured_pcrs = 0;
> >  	}
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index e3da29af2c16..ead3f7fe6998 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -188,6 +188,7 @@ __setup("ima_tcb", default_measure_policy_setup);
> >  
> >  static bool ima_use_appraise_tcb __initdata;
> >  static bool ima_use_secure_boot __initdata;
> > +static bool ima_fail_unverifiable_sigs __ro_after_init;
> >  static int __init policy_setup(char *str)
> >  {
> >  	char *p;
> > @@ -201,6 +202,8 @@ static int __init policy_setup(char *str)
> >  			ima_use_appraise_tcb = true;
> >  		else if (strcmp(p, "secure_boot") == 0)
> >  			ima_use_secure_boot = true;
> > +		else if (strcmp(p, "unverifiable_sigs") == 0)
> > +			ima_fail_unverifiable_sigs = true;
> >  	}
> >  
> >  	return 1;
> > @@ -390,6 +393,8 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
> >  		if (entry->action & IMA_APPRAISE) {
> >  			action |= get_subaction(entry, func);
> >  			action ^= IMA_HASH;
> > +			if (ima_fail_unverifiable_sigs)
> > +				action |= IMA_FAIL_UNVERIFIABLE_SIGS;
> >  		}
> >  
> >  		if (entry->action & IMA_DO_MASK)
> > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> > index 843ae23ba0ac..8224880935e0 100644
> > --- a/security/integrity/integrity.h
> > +++ b/security/integrity/integrity.h
> > @@ -35,6 +35,7 @@
> >  #define IMA_PERMIT_DIRECTIO	0x02000000
> >  #define IMA_NEW_FILE		0x04000000
> >  #define EVM_IMMUTABLE_DIGSIG	0x08000000
> > +#define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
> >  
> >  #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
> >  				 IMA_HASH | IMA_APPRAISE_SUBMASK)
> > -- 
> > 2.7.5
> 

Re: [PATCH v2 3/4] ima: fail signature verification based on policy

[Serge E. Hallyn]

Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> On Tue, 2018-02-27 at 16:35 -0600, Serge E. Hallyn wrote:
> > Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > > This patch addresses the fuse privileged mounted filesystems in
> > > environments which are unwilling to accept the risk of trusting the
> > > signature verification and want to always fail safe, but are for
> > > example using a pre-built kernel.
> > > 
> > > This patch defines a new builtin policy "unverifiable_sigs", which can
> > 
> > How about recalc_unverifiable_sigs?
> 
> Cute, I really like that name, but in this case we're failing the
> signature verification.
> 
> > It's long, but unverifiable_sigs
> > is  not clear about whether the intent is to accept or recalculate them.
> > 
> > (or fail_unverifiable_sigs like the flag)
> 
> Could we abbreviate it to "fail_usigs"? �Or perhaps allow both
> "fail_unverifiable_sigs" and "fail_usigs".

That sounds good.  Or fail_unverified?  But so long as 'fail' is somehow
clearly implied by the name.

Re: [PATCH v2 3/4] ima: fail signature verification based on policy

[Mimi Zohar]

On Wed, 2018-02-28 at 09:30 -0600, Serge E. Hallyn wrote:
> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > On Tue, 2018-02-27 at 16:35 -0600, Serge E. Hallyn wrote:
> > > Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > > > This patch addresses the fuse privileged mounted filesystems in
> > > > environments which are unwilling to accept the risk of trusting the
> > > > signature verification and want to always fail safe, but are for
> > > > example using a pre-built kernel.
> > > > 
> > > > This patch defines a new builtin policy "unverifiable_sigs", which can
> > > 
> > > How about recalc_unverifiable_sigs?
> > 
> > Cute, I really like that name, but in this case we're failing the
> > signature verification.
> > 
> > > It's long, but unverifiable_sigs
> > > is  not clear about whether the intent is to accept or recalculate them.
> > > 
> > > (or fail_unverifiable_sigs like the flag)
> > 
> > Could we abbreviate it to "fail_usigs"?  Or perhaps allow both
> > "fail_unverifiable_sigs" and "fail_usigs".
> 
> That sounds good.  Or fail_unverified?  But so long as 'fail' is somehow
> clearly implied by the name.

None of these names mean anything to anyone but us.  How about
"fail_safe"?  That at least has some meaning to some people.

Mimi