[RFC PATCH 0/6][v3] ima: Support a mode to appraise signed files only

[RFC PATCH 0/6][v3] ima: Support a mode to appraise signed files only

[Vivek Goyal]

Hi,

Currently ima appraises all the files as specified by the rule. So
if one wants to create a system where only few executables are
signed, that system will not work with IMA.

With secureboot, one needs to disable kexec so that unsigned kernels
can't be booted. To avoid this problem, it was proposed that sign
/sbin/kexec binary and if signatures are verified successfully, give
an special capability to the /sbin/kexec process. And in secureboot
mode processes with that special capability can invoke sys_kexec()
system call.

So there is a need for IMA to allow appraising only signed binaries.
Unsigned binaries will pass the appraisal too, but will not get the
special capability. (Capability patches for that are yet to be written).

This patch series adds new option, appraise_type=optional to allow
appraisal to pass even if no signatures are present on the file. If
signatures are present, then it has to be valid digital signature,
otherwise appraisal will fail.

v2: Changed patches based on Mimi's feedback.
v3: - Changed appraise_type=imasig_optional to appraise_type=optional
    - Introduced new error codes.

Thanks
Vivek

Vivek Goyal (6):
  ima: detect security xattrs not enabled
  ima: Return INTEGRITY_FAIL if digital signature can't be verified
  ima/evm: Differentiate between ima/evm nolabel return code
  ima: Introduce new integrity error code INTEGRITY_XATTR_NOTSUPP
  ima: Allow appraisal of digitally signed files only
  ima: With appraise_type=optional, audit log some messages as info

 Documentation/ABI/testing/ima_policy  |    2 +-
 include/linux/integrity.h             |    4 +++-
 security/integrity/evm/evm_main.c     |    2 +-
 security/integrity/ima/ima_appraise.c |   26 ++++++++++++++++++--------
 security/integrity/ima/ima_main.c     |   14 ++++++++++++--
 security/integrity/ima/ima_policy.c   |    2 ++
 security/integrity/integrity.h        |    1 +
 7 files changed, 38 insertions(+), 13 deletions(-)

-- 
1.7.7.6

[PATCH 1/6] ima: detect security xattrs not enabled

[Vivek Goyal]

vfs_getxattr_alloc() returns -EOPNOTSUPP if filesystem does not have
security label enabled. In that case there is no point in continuing
further and try to fix hashes (if ima_appraise=fix was specified) as
that will fail too. Return early

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_appraise.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 2d4beca..3710f44 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -134,6 +134,10 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
 	rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)&xattr_value,
 				0, GFP_NOFS);
 	if (rc <= 0) {
+		/* File system does not support security xattr */
+		if (rc == -EOPNOTSUPP)
+			return INTEGRITY_UNKNOWN;
+
 		if (rc && rc != -ENODATA)
 			goto out;
 
-- 
1.7.7.6

[PATCH 2/6] ima: Return INTEGRITY_FAIL if digital signature can't be verified

[Vivek Goyal]

Digital signature verification happens using integrity_digsig_verify().
Curently we set integrity to FAIL for all error codes except -EOPNOTSUPP.
This sounds out of line.

- If appropriate kernel code is not compiled in to verify signature of
  a file, then prractically it is a failed signature.

- For so many other possible errors we are setting the status to fail.
  For example, -EINVAL, -ENOKEY, -ENOMEM, -EINVAL, -ENOTSUPP etc, it
  beats me that why -EOPNOTSUPP is special.

This patch should make the semantics more consistent. That is, if digital
signature is present in security.ima, then any error happened during
signature processing leads to status INTEGRITY_FAIL.

AFAICS, it should not have any user visible effect on existing
application. In some cases we will start returning INTEGRITY_FAIL
instead of INTEGRITY_UNKNOWN. And process_measurement() will deny access
to file both in case of INTEGRITY_UNKNOWN and INTEGRITY_FAIL.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 security/integrity/ima/ima_appraise.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 3710f44..6f1eeb8 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -178,9 +178,7 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
 					     xattr_value->digest, rc - 1,
 					     iint->ima_xattr.digest,
 					     IMA_DIGEST_SIZE);
-		if (rc == -EOPNOTSUPP) {
-			status = INTEGRITY_UNKNOWN;
-		} else if (rc) {
+		if (rc) {
 			cause = "invalid-signature";
 			status = INTEGRITY_FAIL;
 		} else {
-- 
1.7.7.6

[PATCH 3/6] ima/evm: Differentiate between ima/evm nolabel return code

[Vivek Goyal]

Currently if no IMA/EVM label was present, both return error code
INTEGRITY_NOLABEL. This works fine so far as nobody cares to differentiate
between two error paths.

But with appraise_type=optional, we want to allow file access if IMA
security label is not present. But still disallow access if EVM
security label is not present.

Hence, start using different error codes for two different paths.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 include/linux/integrity.h             |    3 ++-
 security/integrity/evm/evm_main.c     |    2 +-
 security/integrity/ima/ima_appraise.c |    4 ++--
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/linux/integrity.h b/include/linux/integrity.h
index 66c5fe9..9be4a98 100644
--- a/include/linux/integrity.h
+++ b/include/linux/integrity.h
@@ -15,7 +15,8 @@
 enum integrity_status {
 	INTEGRITY_PASS = 0,
 	INTEGRITY_FAIL,
-	INTEGRITY_NOLABEL,
+	INTEGRITY_IMA_NOLABEL,
+	INTEGRITY_EVM_NOLABEL,
 	INTEGRITY_NOXATTRS,
 	INTEGRITY_UNKNOWN,
 };
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index a78a5e2..254eefe 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -111,7 +111,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 		else if (rc == -ENODATA) {
 			rc = evm_find_protected_xattrs(dentry);
 			if (rc > 0)
-				evm_status = INTEGRITY_NOLABEL;
+				evm_status = INTEGRITY_EVM_NOLABEL;
 			else if (rc == 0)
 				evm_status = INTEGRITY_NOXATTRS; /* new file */
 		}
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 6f1eeb8..1750556 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -143,13 +143,13 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
 
 		cause = "missing-hash";
 		status =
-		    (inode->i_size == 0) ? INTEGRITY_PASS : INTEGRITY_NOLABEL;
+		    (inode->i_size == 0) ? INTEGRITY_PASS : INTEGRITY_IMA_NOLABEL;
 		goto out;
 	}
 
 	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
 	if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
-		if ((status == INTEGRITY_NOLABEL)
+		if ((status == INTEGRITY_EVM_NOLABEL)
 		    || (status == INTEGRITY_NOXATTRS))
 			cause = "missing-HMAC";
 		else if (status == INTEGRITY_FAIL)
-- 
1.7.7.6

[PATCH 4/6] ima: Introduce new integrity error code INTEGRITY_XATTR_NOTSUPP

[Vivek Goyal]

Currently file system does not support xattr or security xattr are not
enabled, we return INTEGRITY_UNKNOWN. INTEGRITY_UNKNOWN is returned in
so many other conditions too, evm not initialized etc.

So far nobody cared but with appraise_type=optional, I need to
to differentiate between different error paths. For example, I want
to allow access to file when xattr are not enabled or specific security
attr is not enabled by file system. But I don't want to allow access
for all cases of INTEGRITY_UNKNOWN. For example,
if vfs_getxattr_alloc() returns -ENOMEM, then also INTEGRITY_UNKNOWN
will be returned and I don't think we want to allow access to file
in that case.

So to differentiate the errors where we can allow acccess to file,
introduce the new error code.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 include/linux/integrity.h             |    1 +
 security/integrity/ima/ima_appraise.c |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/integrity.h b/include/linux/integrity.h
index 9be4a98..37074be 100644
--- a/include/linux/integrity.h
+++ b/include/linux/integrity.h
@@ -19,6 +19,7 @@ enum integrity_status {
 	INTEGRITY_EVM_NOLABEL,
 	INTEGRITY_NOXATTRS,
 	INTEGRITY_UNKNOWN,
+	INTEGRITY_XATTR_NOTSUPP,
 };
 
 /* List of EVM protected security xattrs */
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 1750556..af39a08 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -129,14 +129,14 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
 	if (!ima_appraise)
 		return 0;
 	if (!inode->i_op->getxattr)
-		return INTEGRITY_UNKNOWN;
+		return INTEGRITY_XATTR_NOTSUPP;
 
 	rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)&xattr_value,
 				0, GFP_NOFS);
 	if (rc <= 0) {
 		/* File system does not support security xattr */
 		if (rc == -EOPNOTSUPP)
-			return INTEGRITY_UNKNOWN;
+			return INTEGRITY_XATTR_NOTSUPP;
 
 		if (rc && rc != -ENODATA)
 			goto out;
-- 
1.7.7.6

[PATCH 5/6] ima: Allow appraisal of digitally signed files only

[Vivek Goyal]

Currently ima appraises all the files as specified by the rule. So
if one wants to create a system where only few executables are
signed, that system will not work with IMA.

With secureboot, one needs to disable kexec so that unsigned kernels
can't be booted. To avoid this problem, it was proposed that sign
/sbin/kexec binary and if signatures are verified successfully, give
an special capability to the /sbin/kexec process. And in secureboot
mode processes with that special capability can invoke sys_kexec()
system call.

So there is a need for IMA to allow appraising only signed binaries.
Unsigned binaries will pass the appraisal too, but will not get the
special capability. (Capability patches for that are yet to be written).

This patch adds new option, appraise_type=imasig_optional to allow
appraisal to pass even if no signatures are present on the file. If
signatures are present, then it has to be valid digital signature,
otherwise appraisal will fail.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 Documentation/ABI/testing/ima_policy |    2 +-
 security/integrity/ima/ima_main.c    |   14 ++++++++++++--
 security/integrity/ima/ima_policy.c  |    2 ++
 security/integrity/integrity.h       |    1 +
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index de16de3..cc69872 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -30,7 +30,7 @@ Description:
 			uid:= decimal value
 			fowner:=decimal value
 		lsm:  	are LSM specific
-		option:	appraise_type:= [imasig]
+		option:	appraise_type:= [imasig] | [optional]
 
 		default policy:
 			# PROC_SUPER_MAGIC
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 3e751a9..da9e348 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -207,8 +207,18 @@ out_digsig:
 		rc = -EACCES;
 out:
 	mutex_unlock(&inode->i_mutex);
-	if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
-		return -EACCES;
+	if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+		/*
+		 * If IMA_APPRAISAL_OPT is set, then access is allowed
+		 * even if hash or digital signatures are not present.
+		 */
+		if ((iint->flags & IMA_APPRAISAL_OPT) &&
+		     (rc == INTEGRITY_XATTR_NOTSUPP ||
+		      rc == INTEGRITY_IMA_NOLABEL))
+			return 0;
+		else
+			return -EACCES;
+	}
 	return 0;
 }
 
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 4adcd0f..fd92dc3d4 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -598,6 +598,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			ima_log_string(ab, "appraise_type", args[0].from);
 			if ((strcmp(args[0].from, "imasig")) == 0)
 				entry->flags |= IMA_DIGSIG_REQUIRED;
+			else if ((strcmp(args[0].from, "optional")) == 0)
+				entry->flags |= IMA_APPRAISAL_OPT;
 			else
 				result = -EINVAL;
 			break;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 0ae08fc..4d330a7 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -29,6 +29,7 @@
 #define IMA_ACTION_FLAGS	0xff000000
 #define IMA_DIGSIG		0x01000000
 #define IMA_DIGSIG_REQUIRED	0x02000000
+#define IMA_APPRAISAL_OPT	0x04000000
 
 #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
 				 IMA_APPRAISE_SUBMASK)
-- 
1.7.7.6

[PATCH 6/6] ima: With appraise_type=optional, audit log some messages as info

[Vivek Goyal]

Currently, if there integrity status is not INTEGRITY_PASS, it is
logged in audit log (as non info). This is fine because we always
deny access to file for anything other than INTEGRITY_PASS.

But with appraise_type=optional, we will allow access to file even
if appraisal status is not INTEGRITY_PASS. For example, in the case
of INTEGRITY_IMA_NOLABEL. And on this system we don't want to log
each and every executed file which is not signed.

In a typical system we are anticipating that only 1-2 files will
be signed.

So don't flood the audit logs if appraise_type=optional and no
IMA label is present. These messages will still show up if somebody
chooses to enable audit info messages.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 security/integrity/ima/ima_appraise.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index af39a08..ddeadc7 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -124,7 +124,7 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
 	enum integrity_status status = INTEGRITY_UNKNOWN;
 	const char *op = "appraise_data";
 	char *cause = "unknown";
-	int rc;
+	int rc, audit_info = 0;
 
 	if (!ima_appraise)
 		return 0;
@@ -199,8 +199,16 @@ out:
 			if (!ima_fix_xattr(dentry, iint))
 				status = INTEGRITY_PASS;
 		}
+
+		/*
+		 * If appraisal is optional, and if no label is present,
+		 * log it is info. Don't flood audit logs.
+		 */
+		if ((iint->flags & IMA_APPRAISAL_OPT) &&
+		     status == INTEGRITY_IMA_NOLABEL)
+			audit_info = 1;
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
-				    op, cause, rc, 0);
+				    op, cause, rc, audit_info);
 	} else {
 		ima_cache_flags(iint, func);
 	}
-- 
1.7.7.6

Re: [RFC PATCH 0/6][v3] ima: Support a mode to appraise signed files only

[Mimi Zohar]

On Thu, 2013-02-14 at 14:55 -0500, Vivek Goyal wrote:
> Hi,
> 
> Currently ima appraises all the files as specified by the rule.

Currently IMA appraises files based on policy.

>  So
> if one wants to create a system where only few executables are
> signed, that system will not work with IMA.

This statement misrepresents the IMA policy.  You can definitely define
a policy that only measures/appraises a few specific files.  In your
usecase scenario, you are not willing to rely on LSM labels.  Policy
rules can also be based on file owner.  We could also add support for
gid.

> With secureboot, one needs to disable kexec so that unsigned kernels
> can't be booted. To avoid this problem, it was proposed that sign
> /sbin/kexec binary and if signatures are verified successfully, give
> an special capability to the /sbin/kexec process. And in secureboot
> mode processes with that special capability can invoke sys_kexec()
> system call.

Please add here that you then rely on /sbin/kexec to verify the
integrity of the kernel image.

thanks,

Mimi

Re: [RFC PATCH 0/6][v3] ima: Support a mode to appraise signed files only

[Vivek Goyal]

On Thu, Feb 14, 2013 at 03:51:24PM -0500, Mimi Zohar wrote:
> On Thu, 2013-02-14 at 14:55 -0500, Vivek Goyal wrote:
> > Hi,
> > 
> > Currently ima appraises all the files as specified by the rule.
> 
> Currently IMA appraises files based on policy.

And policy is composed of multiple rules. Ok, will change it.

> 
> >  So
> > if one wants to create a system where only few executables are
> > signed, that system will not work with IMA.
> 
> This statement misrepresents the IMA policy.  You can definitely define
> a policy that only measures/appraises a few specific files. In your
> usecase scenario, you are not willing to rely on LSM labels.  Policy
> rules can also be based on file owner.  We could also add support for
> gid.

Ok, will change it. How about following.

We want to create a system where only few executables are signed. This
patch extends IMA policy syntax so that one can specify that signatures
are optional.

> 
> > With secureboot, one needs to disable kexec so that unsigned kernels
> > can't be booted. To avoid this problem, it was proposed that sign
> > /sbin/kexec binary and if signatures are verified successfully, give
> > an special capability to the /sbin/kexec process. And in secureboot
> > mode processes with that special capability can invoke sys_kexec()
> > system call.
> 
> Please add here that you then rely on /sbin/kexec to verify the
> integrity of the kernel image.

Ok, will do that. This is infact a grey area. Yet to be figured out
how /sbin/kexec will ensure a signed kernel is being loaded.

Thanks
Vivek

Re: [PATCH 2/6] ima: Return INTEGRITY_FAIL if digital signature can't be verified

[Mimi Zohar]

On Thu, 2013-02-14 at 14:55 -0500, Vivek Goyal wrote:
> Digital signature verification happens using integrity_digsig_verify().
> Curently we set integrity to FAIL for all error codes except -EOPNOTSUPP.
> This sounds out of line.
> 
> - If appropriate kernel code is not compiled in to verify signature of
>   a file, then prractically it is a failed signature.
> 
> - For so many other possible errors we are setting the status to fail.
>   For example, -EINVAL, -ENOKEY, -ENOMEM, -EINVAL, -ENOTSUPP etc, it
>   beats me that why -EOPNOTSUPP is special.
> 
> This patch should make the semantics more consistent. That is, if digital
> signature is present in security.ima, then any error happened during
> signature processing leads to status INTEGRITY_FAIL.
> 
> AFAICS, it should not have any user visible effect on existing
> application. In some cases we will start returning INTEGRITY_FAIL
> instead of INTEGRITY_UNKNOWN. And process_measurement() will deny access
> to file both in case of INTEGRITY_UNKNOWN and INTEGRITY_FAIL.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

A number of patches in this patchset more finely differentiate return
codes, which is good.  I agree with you totally that there is no good
reason for -EOPNOTSUPP to be handled differently.  Unfortunately, the
initramfs is CPIO, which doesn't support xattrs.  With the proposed
change and 'ima_appraise_tcb' flag enabled, we wouldn't be able to boot.
I really dislike hard coding policy in the kernel.

thanks,

Mimi

> ---
>  security/integrity/ima/ima_appraise.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 3710f44..6f1eeb8 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -178,9 +178,7 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
>  					     xattr_value->digest, rc - 1,
>  					     iint->ima_xattr.digest,
>  					     IMA_DIGEST_SIZE);
> -		if (rc == -EOPNOTSUPP) {
> -			status = INTEGRITY_UNKNOWN;
> -		} else if (rc) {
> +		if (rc) {
>  			cause = "invalid-signature";
>  			status = INTEGRITY_FAIL;
>  		} else {

Re: [PATCH 2/6] ima: Return INTEGRITY_FAIL if digital signature can't be verified

[Vivek Goyal]

On Mon, Mar 04, 2013 at 08:48:36AM -0500, Mimi Zohar wrote:
> On Thu, 2013-02-14 at 14:55 -0500, Vivek Goyal wrote:
> > Digital signature verification happens using integrity_digsig_verify().
> > Curently we set integrity to FAIL for all error codes except -EOPNOTSUPP.
> > This sounds out of line.
> > 
> > - If appropriate kernel code is not compiled in to verify signature of
> >   a file, then prractically it is a failed signature.
> > 
> > - For so many other possible errors we are setting the status to fail.
> >   For example, -EINVAL, -ENOKEY, -ENOMEM, -EINVAL, -ENOTSUPP etc, it
> >   beats me that why -EOPNOTSUPP is special.
> > 
> > This patch should make the semantics more consistent. That is, if digital
> > signature is present in security.ima, then any error happened during
> > signature processing leads to status INTEGRITY_FAIL.
> > 
> > AFAICS, it should not have any user visible effect on existing
> > application. In some cases we will start returning INTEGRITY_FAIL
> > instead of INTEGRITY_UNKNOWN. And process_measurement() will deny access
> > to file both in case of INTEGRITY_UNKNOWN and INTEGRITY_FAIL.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> 
> A number of patches in this patchset more finely differentiate return
> codes, which is good.  I agree with you totally that there is no good
> reason for -EOPNOTSUPP to be handled differently.  Unfortunately, the
> initramfs is CPIO, which doesn't support xattrs.  With the proposed
> change and 'ima_appraise_tcb' flag enabled, we wouldn't be able to boot.
> I really dislike hard coding policy in the kernel.

Hi Mimi,

If there are no xattr, then we will not even hit this code. We will
bail out early in vfs_getxattr_alloc().

I thought that one of the DON_APPRAISE rules will kick in for initramfs
and files in initramfs will not be appraised and boot will continue.

{.action = DONT_APPRAISE,.fsmagic = TMPFS_MAGIC,.flags = IMA_FSMAGIC},
{.action = DONT_APPRAISE,.fsmagic = RAMFS_MAGIC,.flags = IMA_FSMAGIC},

Is that not the case here?

Thanks
Vivek

Re: [PATCH 2/6] ima: Return INTEGRITY_FAIL if digital signature can't be verified

[Mimi Zohar]

On Mon, 2013-03-04 at 11:20 -0500, Vivek Goyal wrote:
> On Mon, Mar 04, 2013 at 08:48:36AM -0500, Mimi Zohar wrote:
> > On Thu, 2013-02-14 at 14:55 -0500, Vivek Goyal wrote:
> > > Digital signature verification happens using integrity_digsig_verify().
> > > Curently we set integrity to FAIL for all error codes except -EOPNOTSUPP.
> > > This sounds out of line.
> > > 
> > > - If appropriate kernel code is not compiled in to verify signature of
> > >   a file, then prractically it is a failed signature.
> > > 
> > > - For so many other possible errors we are setting the status to fail.
> > >   For example, -EINVAL, -ENOKEY, -ENOMEM, -EINVAL, -ENOTSUPP etc, it
> > >   beats me that why -EOPNOTSUPP is special.
> > > 
> > > This patch should make the semantics more consistent. That is, if digital
> > > signature is present in security.ima, then any error happened during
> > > signature processing leads to status INTEGRITY_FAIL.
> > > 
> > > AFAICS, it should not have any user visible effect on existing
> > > application. In some cases we will start returning INTEGRITY_FAIL
> > > instead of INTEGRITY_UNKNOWN. And process_measurement() will deny access
> > > to file both in case of INTEGRITY_UNKNOWN and INTEGRITY_FAIL.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > 
> > A number of patches in this patchset more finely differentiate return
> > codes, which is good.  I agree with you totally that there is no good
> > reason for -EOPNOTSUPP to be handled differently.  Unfortunately, the
> > initramfs is CPIO, which doesn't support xattrs.  With the proposed
> > change and 'ima_appraise_tcb' flag enabled, we wouldn't be able to boot.
> > I really dislike hard coding policy in the kernel.
> 
> Hi Mimi,
> 
> If there are no xattr, then we will not even hit this code. We will
> bail out early in vfs_getxattr_alloc().
> 
> I thought that one of the DON_APPRAISE rules will kick in for initramfs
> and files in initramfs will not be appraised and boot will continue.
> 
> {.action = DONT_APPRAISE,.fsmagic = TMPFS_MAGIC,.flags = IMA_FSMAGIC},
> {.action = DONT_APPRAISE,.fsmagic = RAMFS_MAGIC,.flags = IMA_FSMAGIC},
> 
> Is that not the case here?

Right, thanks for the clarification.  Perhaps we could abbreviate the
patch description like:
 
Digital signature verification happens using integrity_digsig_verify().
If a digital signature is present in security.ima, then any error, which
happens during signature verification, should lead to status
INTEGRITY_FAIL.  In the future we might want to differentiate between
persistent (eg. -ENOMEM) vs. non-persistent errors, in order to cache
failures.  This patch removes the unnecessary -EOPNOTSUPP test.
 
thanks,

Mimi

Re: [PATCH 2/6] ima: Return INTEGRITY_FAIL if digital signature can't be verified

[Mimi Zohar]

On Tue, 2013-03-05 at 08:30 -0500, Mimi Zohar wrote:

> Digital signature verification happens using integrity_digsig_verify().
> If a digital signature is present in security.ima, then any error, which
> happens during signature verification, should lead to status
> INTEGRITY_FAIL.  In the future we might want to differentiate between
> persistent (eg. -ENOMEM) vs. non-persistent errors, in order to cache
> failures.  This patch removes the unnecessary -EOPNOTSUPP test.

correction, "persistent vs. non-persistent(eg. -ENOMEM)"

Mimi

Re: [PATCH 2/6] ima: Return INTEGRITY_FAIL if digital signature can't be verified

[Vivek Goyal]

On Tue, Mar 05, 2013 at 08:30:53AM -0500, Mimi Zohar wrote:
> On Mon, 2013-03-04 at 11:20 -0500, Vivek Goyal wrote:
> > On Mon, Mar 04, 2013 at 08:48:36AM -0500, Mimi Zohar wrote:
> > > On Thu, 2013-02-14 at 14:55 -0500, Vivek Goyal wrote:
> > > > Digital signature verification happens using integrity_digsig_verify().
> > > > Curently we set integrity to FAIL for all error codes except -EOPNOTSUPP.
> > > > This sounds out of line.
> > > > 
> > > > - If appropriate kernel code is not compiled in to verify signature of
> > > >   a file, then prractically it is a failed signature.
> > > > 
> > > > - For so many other possible errors we are setting the status to fail.
> > > >   For example, -EINVAL, -ENOKEY, -ENOMEM, -EINVAL, -ENOTSUPP etc, it
> > > >   beats me that why -EOPNOTSUPP is special.
> > > > 
> > > > This patch should make the semantics more consistent. That is, if digital
> > > > signature is present in security.ima, then any error happened during
> > > > signature processing leads to status INTEGRITY_FAIL.
> > > > 
> > > > AFAICS, it should not have any user visible effect on existing
> > > > application. In some cases we will start returning INTEGRITY_FAIL
> > > > instead of INTEGRITY_UNKNOWN. And process_measurement() will deny access
> > > > to file both in case of INTEGRITY_UNKNOWN and INTEGRITY_FAIL.
> > > > 
> > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > 
> > > A number of patches in this patchset more finely differentiate return
> > > codes, which is good.  I agree with you totally that there is no good
> > > reason for -EOPNOTSUPP to be handled differently.  Unfortunately, the
> > > initramfs is CPIO, which doesn't support xattrs.  With the proposed
> > > change and 'ima_appraise_tcb' flag enabled, we wouldn't be able to boot.
> > > I really dislike hard coding policy in the kernel.
> > 
> > Hi Mimi,
> > 
> > If there are no xattr, then we will not even hit this code. We will
> > bail out early in vfs_getxattr_alloc().
> > 
> > I thought that one of the DON_APPRAISE rules will kick in for initramfs
> > and files in initramfs will not be appraised and boot will continue.
> > 
> > {.action = DONT_APPRAISE,.fsmagic = TMPFS_MAGIC,.flags = IMA_FSMAGIC},
> > {.action = DONT_APPRAISE,.fsmagic = RAMFS_MAGIC,.flags = IMA_FSMAGIC},
> > 
> > Is that not the case here?
> 
> Right, thanks for the clarification.  Perhaps we could abbreviate the
> patch description like:
>  
> Digital signature verification happens using integrity_digsig_verify().
> If a digital signature is present in security.ima, then any error, which
> happens during signature verification, should lead to status
> INTEGRITY_FAIL.  In the future we might want to differentiate between
> persistent (eg. -ENOMEM) vs. non-persistent errors, in order to cache
> failures.  This patch removes the unnecessary -EOPNOTSUPP test.

Sure. I will modify the changelog for future posting.

Thanks
Vivek

Re: [PATCH 5/6] ima: Allow appraisal of digitally signed files only

[Vivek Goyal]

On Thu, Feb 14, 2013 at 02:55:44PM -0500, Vivek Goyal wrote:
> Currently ima appraises all the files as specified by the rule. So
> if one wants to create a system where only few executables are
> signed, that system will not work with IMA.
> 
> With secureboot, one needs to disable kexec so that unsigned kernels
> can't be booted. To avoid this problem, it was proposed that sign
> /sbin/kexec binary and if signatures are verified successfully, give
> an special capability to the /sbin/kexec process. And in secureboot
> mode processes with that special capability can invoke sys_kexec()
> system call.
> 
> So there is a need for IMA to allow appraising only signed binaries.
> Unsigned binaries will pass the appraisal too, but will not get the
> special capability. (Capability patches for that are yet to be written).
> 
> This patch adds new option, appraise_type=imasig_optional to allow
> appraisal to pass even if no signatures are present on the file. If
> signatures are present, then it has to be valid digital signature,
> otherwise appraisal will fail.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  Documentation/ABI/testing/ima_policy |    2 +-
>  security/integrity/ima/ima_main.c    |   14 ++++++++++++--
>  security/integrity/ima/ima_policy.c  |    2 ++
>  security/integrity/integrity.h       |    1 +
>  4 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index de16de3..cc69872 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -30,7 +30,7 @@ Description:
>  			uid:= decimal value
>  			fowner:=decimal value
>  		lsm:  	are LSM specific
> -		option:	appraise_type:= [imasig]
> +		option:	appraise_type:= [imasig] | [optional]
>  
>  		default policy:
>  			# PROC_SUPER_MAGIC
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 3e751a9..da9e348 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -207,8 +207,18 @@ out_digsig:
>  		rc = -EACCES;
>  out:
>  	mutex_unlock(&inode->i_mutex);
> -	if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
> -		return -EACCES;
> +	if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> +		/*
> +		 * If IMA_APPRAISAL_OPT is set, then access is allowed
> +		 * even if hash or digital signatures are not present.
> +		 */
> +		if ((iint->flags & IMA_APPRAISAL_OPT) &&
> +		     (rc == INTEGRITY_XATTR_NOTSUPP ||
> +		      rc == INTEGRITY_IMA_NOLABEL))
> +			return 0;
> +		else
> +			return -EACCES;

I think there is problem here. "appraise_type=optional" can be specified
per rule/hook. So two different hooks can specify two different rules.

appraise func=MMAP_CHECK appraise_type=optional
appraise func=BPRM_CHECK

I think if a file is first mmaped(), then appraisal will take place and
IMA_APPRAISAL_OPT will be set in iint->flags.

Later when BPRM_CHECK hook gets executed, and it will return success
based on IMA_APPRAISAL_OPT even if there was no label. And that's not
what exec() expects.

So storing IMA_APPRAISAL_OPT in iint->flags seems wrong (espectially as
part of IMA_ACTION_FLAGS bits). I think only bits which are valid
across all rules/hooks should be stored here.

Any property which is hook/rule specific should either not be stored
or should be stroed in hook specific property area.

We don't have enough space to store more hook specific properties, so 
I will explore the option of passing around this flag when hook is being
executed and then discard it.

Thanks
Vivek
 
> +	}
>  	return 0;
>  }
>  
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 4adcd0f..fd92dc3d4 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -598,6 +598,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>  			ima_log_string(ab, "appraise_type", args[0].from);
>  			if ((strcmp(args[0].from, "imasig")) == 0)
>  				entry->flags |= IMA_DIGSIG_REQUIRED;
> +			else if ((strcmp(args[0].from, "optional")) == 0)
> +				entry->flags |= IMA_APPRAISAL_OPT;
>  			else
>  				result = -EINVAL;
>  			break;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 0ae08fc..4d330a7 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -29,6 +29,7 @@
>  #define IMA_ACTION_FLAGS	0xff000000
>  #define IMA_DIGSIG		0x01000000
>  #define IMA_DIGSIG_REQUIRED	0x02000000
> +#define IMA_APPRAISAL_OPT	0x04000000
>  
>  #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
>  				 IMA_APPRAISE_SUBMASK)
> -- 
> 1.7.7.6

Re: [PATCH 5/6] ima: Allow appraisal of digitally signed files only

[Kasatkin, Dmitry]

On Tue, Mar 5, 2013 at 9:13 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Feb 14, 2013 at 02:55:44PM -0500, Vivek Goyal wrote:
>> Currently ima appraises all the files as specified by the rule. So
>> if one wants to create a system where only few executables are
>> signed, that system will not work with IMA.
>>
>> With secureboot, one needs to disable kexec so that unsigned kernels
>> can't be booted. To avoid this problem, it was proposed that sign
>> /sbin/kexec binary and if signatures are verified successfully, give
>> an special capability to the /sbin/kexec process. And in secureboot
>> mode processes with that special capability can invoke sys_kexec()
>> system call.
>>
>> So there is a need for IMA to allow appraising only signed binaries.
>> Unsigned binaries will pass the appraisal too, but will not get the
>> special capability. (Capability patches for that are yet to be written).
>>
>> This patch adds new option, appraise_type=imasig_optional to allow
>> appraisal to pass even if no signatures are present on the file. If
>> signatures are present, then it has to be valid digital signature,
>> otherwise appraisal will fail.
>>
>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> ---
>>  Documentation/ABI/testing/ima_policy |    2 +-
>>  security/integrity/ima/ima_main.c    |   14 ++++++++++++--
>>  security/integrity/ima/ima_policy.c  |    2 ++
>>  security/integrity/integrity.h       |    1 +
>>  4 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
>> index de16de3..cc69872 100644
>> --- a/Documentation/ABI/testing/ima_policy
>> +++ b/Documentation/ABI/testing/ima_policy
>> @@ -30,7 +30,7 @@ Description:
>>                       uid:= decimal value
>>                       fowner:=decimal value
>>               lsm:    are LSM specific
>> -             option: appraise_type:= [imasig]
>> +             option: appraise_type:= [imasig] | [optional]
>>
>>               default policy:
>>                       # PROC_SUPER_MAGIC
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index 3e751a9..da9e348 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -207,8 +207,18 @@ out_digsig:
>>               rc = -EACCES;
>>  out:
>>       mutex_unlock(&inode->i_mutex);
>> -     if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
>> -             return -EACCES;
>> +     if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) {
>> +             /*
>> +              * If IMA_APPRAISAL_OPT is set, then access is allowed
>> +              * even if hash or digital signatures are not present.
>> +              */
>> +             if ((iint->flags & IMA_APPRAISAL_OPT) &&
>> +                  (rc == INTEGRITY_XATTR_NOTSUPP ||
>> +                   rc == INTEGRITY_IMA_NOLABEL))
>> +                     return 0;
>> +             else
>> +                     return -EACCES;
>
> I think there is problem here. "appraise_type=optional" can be specified
> per rule/hook. So two different hooks can specify two different rules.
>
> appraise func=MMAP_CHECK appraise_type=optional
> appraise func=BPRM_CHECK
>
> I think if a file is first mmaped(), then appraisal will take place and
> IMA_APPRAISAL_OPT will be set in iint->flags.
>
> Later when BPRM_CHECK hook gets executed, and it will return success
> based on IMA_APPRAISAL_OPT even if there was no label. And that's not
> what exec() expects.
>
> So storing IMA_APPRAISAL_OPT in iint->flags seems wrong (espectially as
> part of IMA_ACTION_FLAGS bits). I think only bits which are valid
> across all rules/hooks should be stored here.
>
> Any property which is hook/rule specific should either not be stored
> or should be stroed in hook specific property area.
>
> We don't have enough space to store more hook specific properties, so
> I will explore the option of passing around this flag when hook is being
> executed and then discard it.
>

Hi,

I think there is no need to store optional in iint->flags.
Just test "action" which is returned by ima_get_action().
Then it will be rule specific...

- Dmitry

> Thanks
> Vivek
>
>> +     }
>>       return 0;
>>  }
>>
>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> index 4adcd0f..fd92dc3d4 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -598,6 +598,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>>                       ima_log_string(ab, "appraise_type", args[0].from);
>>                       if ((strcmp(args[0].from, "imasig")) == 0)
>>                               entry->flags |= IMA_DIGSIG_REQUIRED;
>> +                     else if ((strcmp(args[0].from, "optional")) == 0)
>> +                             entry->flags |= IMA_APPRAISAL_OPT;
>>                       else
>>                               result = -EINVAL;
>>                       break;
>> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
>> index 0ae08fc..4d330a7 100644
>> --- a/security/integrity/integrity.h
>> +++ b/security/integrity/integrity.h
>> @@ -29,6 +29,7 @@
>>  #define IMA_ACTION_FLAGS     0xff000000
>>  #define IMA_DIGSIG           0x01000000
>>  #define IMA_DIGSIG_REQUIRED  0x02000000
>> +#define IMA_APPRAISAL_OPT    0x04000000
>>
>>  #define IMA_DO_MASK          (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
>>                                IMA_APPRAISE_SUBMASK)
>> --
>> 1.7.7.6