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

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

[Vivek Goyal]

Hi Mimi,

As we discussed, thhis is a RFC patch to extend current appraisal rules
to allow appraising signed files only. This should apply on top of
Dmitry's patches to support asymmetric key signatures.

Thanks
Vivek

Vivek Goyal (2):
  ima: Do not try to fix hash if file system does not support security
    xattr
  ima: Support appraise_type=imasig_optional

 Documentation/ABI/testing/ima_policy  |    2 +-
 security/integrity/ima/ima_appraise.c |   26 ++++++++++++++++++++++----
 security/integrity/ima/ima_policy.c   |    2 ++
 security/integrity/integrity.h        |    1 +
 4 files changed, 26 insertions(+), 5 deletions(-)

-- 
1.7.7.6

[PATCH 1/2] ima: Do not try to fix hash if file system does not support security xattr

[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

Re: [PATCH 1/2] ima: Do not try to fix hash if file system does not support security xattr

[Mimi Zohar]

On Mon, 2013-02-11 at 15:11 -0500, Vivek Goyal wrote:
> 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

There's a minor discrepancy between the patch title and the patch
description - not supported vs. not enabled.  Perhaps the title could be
abbreviated as well to something like "ima: detect security xattrs not
enabled".

thanks,

Mimi

Re: [PATCH 1/2] ima: Do not try to fix hash if file system does not support security xattr

[Vivek Goyal]

On Tue, Feb 12, 2013 at 06:45:06AM -0500, Mimi Zohar wrote:
> On Mon, 2013-02-11 at 15:11 -0500, Vivek Goyal wrote:
> > 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
> 
> There's a minor discrepancy between the patch title and the patch
> description - not supported vs. not enabled.  Perhaps the title could be
> abbreviated as well to something like "ima: detect security xattrs not
> enabled".

Sure will change it.

Thanks
Vivek

[PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

appraise_type=imasig_optional will 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.

This can allow to selectively sign executables in the system and based
on appraisal results, signed executables with valid signatures can be
given extra capability to perform priviliged operations in secureboot
mode.

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

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index de16de3..5ca0c23 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] | [imasig_optional]
 
 		default policy:
 			# PROC_SUPER_MAGIC
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 3710f44..222ade0 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -124,19 +124,26 @@ 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;
-	if (!inode->i_op->getxattr)
+	if (!inode->i_op->getxattr) {
+		/* getxattr not supported. file couldn't have been signed */
+		if (iint->flags & IMA_DIGSIG_OPTIONAL)
+			return INTEGRITY_PASS;
 		return INTEGRITY_UNKNOWN;
+	}
 
 	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)
+		if (rc == -EOPNOTSUPP) {
+			if (iint->flags & IMA_DIGSIG_OPTIONAL)
+				return INTEGRITY_PASS;
 			return INTEGRITY_UNKNOWN;
+		}
 
 		if (rc && rc != -ENODATA)
 			goto out;
@@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
 	}
 	switch (xattr_value->type) {
 	case IMA_XATTR_DIGEST:
-		if (iint->flags & IMA_DIGSIG_REQUIRED) {
+		if (iint->flags & IMA_DIGSIG_REQUIRED ||
+		    iint->flags & IMA_DIGSIG_OPTIONAL) {
 			cause = "IMA signature required";
 			status = INTEGRITY_FAIL;
 			break;
@@ -201,8 +209,14 @@ out:
 			if (!ima_fix_xattr(dentry, iint))
 				status = INTEGRITY_PASS;
 		}
+		if (status == INTEGRITY_NOLABEL &&
+		    iint->flags & IMA_DIGSIG_OPTIONAL) {
+			status = INTEGRITY_PASS;
+			/* Don't flood audit logs with skipped appraise */
+			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);
 	}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 4adcd0f..8b8cd5f 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, "imasig_optional")) == 0)
+				entry->flags |= IMA_DIGSIG_OPTIONAL;
 			else
 				result = -EINVAL;
 			break;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 84c37c4..2ba736b 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -30,6 +30,7 @@
 #define IMA_ACTION_FLAGS	0xff000000
 #define IMA_DIGSIG		0x01000000
 #define IMA_DIGSIG_REQUIRED	0x02000000
+#define IMA_DIGSIG_OPTIONAL	0x04000000
 
 #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
 				 IMA_APPRAISE_SUBMASK)
-- 
1.7.7.6

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Mon, 2013-02-11 at 15:11 -0500, Vivek Goyal wrote:
> appraise_type=imasig_optional will 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.
> 
> This can allow to selectively sign executables in the system and based
> on appraisal results, signed executables with valid signatures can be
> given extra capability to perform priviliged operations in secureboot
> mode.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Thanks, Vivek, the patch looks a lot better.  Here are a couple of
suggestions:  
- the patch description needs to start with the problem description, not
the solution.
- the patch name should reflect the problem.

A few comments are inline below.

thanks,

Mimi

> ---
>  Documentation/ABI/testing/ima_policy  |    2 +-
>  security/integrity/ima/ima_appraise.c |   24 +++++++++++++++++++-----
>  security/integrity/ima/ima_policy.c   |    2 ++
>  security/integrity/integrity.h        |    1 +
>  4 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index de16de3..5ca0c23 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] | [imasig_optional]
> 
>  		default policy:
>  			# PROC_SUPER_MAGIC
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 3710f44..222ade0 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -124,19 +124,26 @@ 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;
> -	if (!inode->i_op->getxattr)
> +	if (!inode->i_op->getxattr) {
> +		/* getxattr not supported. file couldn't have been signed */
> +		if (iint->flags & IMA_DIGSIG_OPTIONAL)
> +			return INTEGRITY_PASS;
>  		return INTEGRITY_UNKNOWN;
> +	}
> 

Please don't change the result of the appraisal like this.  A single
change can be made towards the bottom of process_measurement().

>  	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)
> +		if (rc == -EOPNOTSUPP) {
> +			if (iint->flags & IMA_DIGSIG_OPTIONAL)
> +				return INTEGRITY_PASS;
>  			return INTEGRITY_UNKNOWN;
> +		}

ditto 

> 
>  		if (rc && rc != -ENODATA)
>  			goto out;
> @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
>  	}
>  	switch (xattr_value->type) {
>  	case IMA_XATTR_DIGEST:
> -		if (iint->flags & IMA_DIGSIG_REQUIRED) {
> +		if (iint->flags & IMA_DIGSIG_REQUIRED ||
> +		    iint->flags & IMA_DIGSIG_OPTIONAL) {
>  			cause = "IMA signature required";
>  			status = INTEGRITY_FAIL;
>  			break;
> @@ -201,8 +209,14 @@ out:
>  			if (!ima_fix_xattr(dentry, iint))
>  				status = INTEGRITY_PASS;
>  		}
> +		if (status == INTEGRITY_NOLABEL &&
> +		    iint->flags & IMA_DIGSIG_OPTIONAL) {
> +			status = INTEGRITY_PASS;
> +			/* Don't flood audit logs with skipped appraise */
> +			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);
>  	}
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 4adcd0f..8b8cd5f 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, "imasig_optional")) == 0)
> +				entry->flags |= IMA_DIGSIG_OPTIONAL;

By setting IMA_DIGSIG_REQUIRED, here, as well, you'll be able to clean
up the code a bit more.

>  			else
>  				result = -EINVAL;
>  			break;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 84c37c4..2ba736b 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -30,6 +30,7 @@
>  #define IMA_ACTION_FLAGS	0xff000000
>  #define IMA_DIGSIG		0x01000000
>  #define IMA_DIGSIG_REQUIRED	0x02000000
> +#define IMA_DIGSIG_OPTIONAL	0x04000000
> 
>  #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
>  				 IMA_APPRAISE_SUBMASK)

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Mon, Feb 11, 2013 at 05:10:14PM -0500, Mimi Zohar wrote:
> On Mon, 2013-02-11 at 15:11 -0500, Vivek Goyal wrote:
> > appraise_type=imasig_optional will 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.
> > 
> > This can allow to selectively sign executables in the system and based
> > on appraisal results, signed executables with valid signatures can be
> > given extra capability to perform priviliged operations in secureboot
> > mode.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> 
> Thanks, Vivek, the patch looks a lot better.  Here are a couple of
> suggestions:  
> - the patch description needs to start with the problem description, not
> the solution.

Sure will do.

> - the patch name should reflect the problem.

Will change.

> 
> A few comments are inline below.
> 
> thanks,
> 
> Mimi
> 
> > ---
> >  Documentation/ABI/testing/ima_policy  |    2 +-
> >  security/integrity/ima/ima_appraise.c |   24 +++++++++++++++++++-----
> >  security/integrity/ima/ima_policy.c   |    2 ++
> >  security/integrity/integrity.h        |    1 +
> >  4 files changed, 23 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> > index de16de3..5ca0c23 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] | [imasig_optional]
> > 
> >  		default policy:
> >  			# PROC_SUPER_MAGIC
> > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> > index 3710f44..222ade0 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -124,19 +124,26 @@ 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;
> > -	if (!inode->i_op->getxattr)
> > +	if (!inode->i_op->getxattr) {
> > +		/* getxattr not supported. file couldn't have been signed */
> > +		if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > +			return INTEGRITY_PASS;
> >  		return INTEGRITY_UNKNOWN;
> > +	}
> > 
> 
> Please don't change the result of the appraisal like this.  A single
> change can be made towards the bottom of process_measurement().

I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
I can probably maintain a bool variable, say pass_appraisal, and set
that here and at the end of function, parse that variable and change
the status accordingly.

> 
> >  	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)
> > +		if (rc == -EOPNOTSUPP) {
> > +			if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > +				return INTEGRITY_PASS;
> >  			return INTEGRITY_UNKNOWN;
> > +		}
> 
> ditto 

Will do.

> 
> > 
> >  		if (rc && rc != -ENODATA)
> >  			goto out;
> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> >  	}
> >  	switch (xattr_value->type) {
> >  	case IMA_XATTR_DIGEST:
> > -		if (iint->flags & IMA_DIGSIG_REQUIRED) {
> > +		if (iint->flags & IMA_DIGSIG_REQUIRED ||
> > +		    iint->flags & IMA_DIGSIG_OPTIONAL) {
> >  			cause = "IMA signature required";
> >  			status = INTEGRITY_FAIL;
> >  			break;
> > @@ -201,8 +209,14 @@ out:
> >  			if (!ima_fix_xattr(dentry, iint))
> >  				status = INTEGRITY_PASS;
> >  		}
> > +		if (status == INTEGRITY_NOLABEL &&
> > +		    iint->flags & IMA_DIGSIG_OPTIONAL) {
> > +			status = INTEGRITY_PASS;
> > +			/* Don't flood audit logs with skipped appraise */
> > +			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);
> >  	}
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 4adcd0f..8b8cd5f 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, "imasig_optional")) == 0)
> > +				entry->flags |= IMA_DIGSIG_OPTIONAL;
> 
> By setting IMA_DIGSIG_REQUIRED, here, as well, you'll be able to clean
> up the code a bit more.

I don't understand this part. So imasig_optional sets both 
IMA_DIGSIG_REQUIRED as well as IMA_DIGSIG_OPTIONAL? That seems to be
quite contradictory for a reader. 

We only add one extra line and that is when "hash" is detected in
security.ima, we check for IMA_DIGSIG_OPTIONAL and return an error. So
we are probably not saving on code.

IMHO, not setting IMA_DIGSIG_REQUIRED makes sense in this context.

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Tue, 2013-02-12 at 09:26 -0500, Vivek Goyal wrote:
> On Mon, Feb 11, 2013 at 05:10:14PM -0500, Mimi Zohar wrote:
> > On Mon, 2013-02-11 at 15:11 -0500, Vivek Goyal wrote:
> > > appraise_type=imasig_optional will 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.
> > > 
> > > This can allow to selectively sign executables in the system and based
> > > on appraisal results, signed executables with valid signatures can be
> > > given extra capability to perform priviliged operations in secureboot
> > > mode.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > 
> > Thanks, Vivek, the patch looks a lot better.  Here are a couple of
> > suggestions:  
> > - the patch description needs to start with the problem description, not
> > the solution.
> 
> Sure will do.
> 
> > - the patch name should reflect the problem.
> 
> Will change.
> 
> > 
> > A few comments are inline below.
> > 
> > thanks,
> > 
> > Mimi
> > 
> > > ---
> > >  Documentation/ABI/testing/ima_policy  |    2 +-
> > >  security/integrity/ima/ima_appraise.c |   24 +++++++++++++++++++-----
> > >  security/integrity/ima/ima_policy.c   |    2 ++
> > >  security/integrity/integrity.h        |    1 +
> > >  4 files changed, 23 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> > > index de16de3..5ca0c23 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] | [imasig_optional]
> > > 
> > >  		default policy:
> > >  			# PROC_SUPER_MAGIC
> > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> > > index 3710f44..222ade0 100644
> > > --- a/security/integrity/ima/ima_appraise.c
> > > +++ b/security/integrity/ima/ima_appraise.c
> > > @@ -124,19 +124,26 @@ 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;
> > > -	if (!inode->i_op->getxattr)
> > > +	if (!inode->i_op->getxattr) {
> > > +		/* getxattr not supported. file couldn't have been signed */
> > > +		if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > > +			return INTEGRITY_PASS;
> > >  		return INTEGRITY_UNKNOWN;
> > > +	}
> > > 
> > 
> > Please don't change the result of the appraisal like this.  A single
> > change can be made towards the bottom of process_measurement().
> 
> I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
> I can probably maintain a bool variable, say pass_appraisal, and set
> that here and at the end of function, parse that variable and change
> the status accordingly.

process_measurement() is the only caller of ima_appraise_measurement().
Leave the results of ima_appraise_measurement() alone.  There's already
code at the end of process_measurement() which decides what to return.
Just modify it based on the appraisal results.

> > 
> > >  	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)
> > > +		if (rc == -EOPNOTSUPP) {
> > > +			if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > > +				return INTEGRITY_PASS;
> > >  			return INTEGRITY_UNKNOWN;
> > > +		}
> > 
> > ditto 
> 
> Will do.
> 
> > 
> > > 
> > >  		if (rc && rc != -ENODATA)
> > >  			goto out;
> > > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> > >  	}
> > >  	switch (xattr_value->type) {
> > >  	case IMA_XATTR_DIGEST:
> > > -		if (iint->flags & IMA_DIGSIG_REQUIRED) {
> > > +		if (iint->flags & IMA_DIGSIG_REQUIRED ||
> > > +		    iint->flags & IMA_DIGSIG_OPTIONAL) {
> > >  			cause = "IMA signature required";
> > >  			status = INTEGRITY_FAIL;
> > >  			break;
> > > @@ -201,8 +209,14 @@ out:
> > >  			if (!ima_fix_xattr(dentry, iint))
> > >  				status = INTEGRITY_PASS;
> > >  		}
> > > +		if (status == INTEGRITY_NOLABEL &&
> > > +		    iint->flags & IMA_DIGSIG_OPTIONAL) {
> > > +			status = INTEGRITY_PASS;
> > > +			/* Don't flood audit logs with skipped appraise */
> > > +			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);
> > >  	}
> > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > > index 4adcd0f..8b8cd5f 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, "imasig_optional")) == 0)
> > > +				entry->flags |= IMA_DIGSIG_OPTIONAL;
> > 
> > By setting IMA_DIGSIG_REQUIRED, here, as well, you'll be able to clean
> > up the code a bit more.
> 
> I don't understand this part. So imasig_optional sets both 
> IMA_DIGSIG_REQUIRED as well as IMA_DIGSIG_OPTIONAL? That seems to be
> quite contradictory for a reader. 

> We only add one extra line and that is when "hash" is detected in
> security.ima, we check for IMA_DIGSIG_OPTIONAL and return an error. So
> we are probably not saving on code.
> 
> IMHO, not setting IMA_DIGSIG_REQUIRED makes sense in this context.

'imasig_optional' does not only mean that the signature is optional, but
also implies that it has to be a digital signature, not a hash.  This
latter part is what IMA_DIGSIG_REQUIRED means.

Remember the rule 'action' determines whether or not the file needs to
be appraised.

thanks,

Mimi

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:

[..]
> > > > --- a/security/integrity/ima/ima_appraise.c
> > > > +++ b/security/integrity/ima/ima_appraise.c
> > > > @@ -124,19 +124,26 @@ 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;
> > > > -	if (!inode->i_op->getxattr)
> > > > +	if (!inode->i_op->getxattr) {
> > > > +		/* getxattr not supported. file couldn't have been signed */
> > > > +		if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > > > +			return INTEGRITY_PASS;
> > > >  		return INTEGRITY_UNKNOWN;
> > > > +	}
> > > > 
> > > 
> > > Please don't change the result of the appraisal like this.  A single
> > > change can be made towards the bottom of process_measurement().
> > 
> > I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
> > I can probably maintain a bool variable, say pass_appraisal, and set
> > that here and at the end of function, parse that variable and change
> > the status accordingly.
> 
> process_measurement() is the only caller of ima_appraise_measurement().
> Leave the results of ima_appraise_measurement() alone.  There's already
> code at the end of process_measurement() which decides what to return.
> Just modify it based on the appraisal results.

Ok, I can do that. There is a small concern though. That is what to do
when rc = INTEGRITY_UKNOWN and IMA_DIGSIG_OPTIONAL flag is set.

ima_appraise_measurement() returns INTEGRITY_UKNOWN when file system
does not support xattrs or if security xattr is not enabled. In this
case it is desirable to allow access if IMA_DIGSIG_OPTIONAL flag is
set.

But INTEGRITY_UNKNOWN is also also returned when integrity_digsig_verify()
fails and returns -EOPNOTSUPP. 

I feel that in this case it is not very appropriate to pass appraisal and
let executable run. If digital signatures are present but we can't verify
those (Say some algorithm is not supported in kernel). In that case I
think it makes sense to fail the signature. 

               rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
                                             xattr_value->digest, rc - 1,
                                             iint->ima_xattr.digest,
                                             IMA_DIGEST_SIZE);
                if (rc == -EOPNOTSUPP) {
                        status = INTEGRITY_UNKNOWN;


So how to handle this case.

I am wondering why do we reutrn INTEGRITY_UNKNOWN above and not
INTEGRITY_FAIL.

Will it make sense to fail signature in case of -EOPNOTSUPP.
               rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
                                             xattr_value->digest, rc - 1,
                                             iint->ima_xattr.digest,
                                             IMA_DIGEST_SIZE);
		if (rc)
			status = INTEGRITY_FAIL;
		else
			status = INTEGRITY_PASS;


[..]
> > > > --- 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, "imasig_optional")) == 0)
> > > > +				entry->flags |= IMA_DIGSIG_OPTIONAL;
> > > 
> > > By setting IMA_DIGSIG_REQUIRED, here, as well, you'll be able to clean
> > > up the code a bit more.
> > 
> > I don't understand this part. So imasig_optional sets both 
> > IMA_DIGSIG_REQUIRED as well as IMA_DIGSIG_OPTIONAL? That seems to be
> > quite contradictory for a reader. 
> 
> > We only add one extra line and that is when "hash" is detected in
> > security.ima, we check for IMA_DIGSIG_OPTIONAL and return an error. So
> > we are probably not saving on code.
> > 
> > IMHO, not setting IMA_DIGSIG_REQUIRED makes sense in this context.
> 
> 'imasig_optional' does not only mean that the signature is optional, but
> also implies that it has to be a digital signature, not a hash.  This
> latter part is what IMA_DIGSIG_REQUIRED means.
> 
> Remember the rule 'action' determines whether or not the file needs to
> be appraised.

Ok, I will change it and set IMA_DIGSIG_REQUIRED flag too.

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Tue, Feb 12, 2013 at 01:52:03PM -0500, Vivek Goyal wrote:
> On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
> 
> [..]
> > > > > --- a/security/integrity/ima/ima_appraise.c
> > > > > +++ b/security/integrity/ima/ima_appraise.c
> > > > > @@ -124,19 +124,26 @@ 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;
> > > > > -	if (!inode->i_op->getxattr)
> > > > > +	if (!inode->i_op->getxattr) {
> > > > > +		/* getxattr not supported. file couldn't have been signed */
> > > > > +		if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > > > > +			return INTEGRITY_PASS;
> > > > >  		return INTEGRITY_UNKNOWN;
> > > > > +	}
> > > > > 
> > > > 
> > > > Please don't change the result of the appraisal like this.  A single
> > > > change can be made towards the bottom of process_measurement().
> > > 
> > > I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
> > > I can probably maintain a bool variable, say pass_appraisal, and set
> > > that here and at the end of function, parse that variable and change
> > > the status accordingly.
> > 
> > process_measurement() is the only caller of ima_appraise_measurement().
> > Leave the results of ima_appraise_measurement() alone.  There's already
> > code at the end of process_measurement() which decides what to return.
> > Just modify it based on the appraisal results.
> 

If we do this, audit logs will be filled with integrity unknown failures.
As each unsigned executable file will fail appraisal with INTEGRITY_UNKNOWN
and an audit message will be logged.

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Kasatkin, Dmitry]

Hello Vivek,

Can you please send to us how your IMA policy looks like.

Thanks,
Dmitry

On Tue, Feb 12, 2013 at 8:57 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Feb 12, 2013 at 01:52:03PM -0500, Vivek Goyal wrote:
>> On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
>>
>> [..]
>> > > > > --- a/security/integrity/ima/ima_appraise.c
>> > > > > +++ b/security/integrity/ima/ima_appraise.c
>> > > > > @@ -124,19 +124,26 @@ 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;
>> > > > > -     if (!inode->i_op->getxattr)
>> > > > > +     if (!inode->i_op->getxattr) {
>> > > > > +             /* getxattr not supported. file couldn't have been signed */
>> > > > > +             if (iint->flags & IMA_DIGSIG_OPTIONAL)
>> > > > > +                     return INTEGRITY_PASS;
>> > > > >               return INTEGRITY_UNKNOWN;
>> > > > > +     }
>> > > > >
>> > > >
>> > > > Please don't change the result of the appraisal like this.  A single
>> > > > change can be made towards the bottom of process_measurement().
>> > >
>> > > I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
>> > > I can probably maintain a bool variable, say pass_appraisal, and set
>> > > that here and at the end of function, parse that variable and change
>> > > the status accordingly.
>> >
>> > process_measurement() is the only caller of ima_appraise_measurement().
>> > Leave the results of ima_appraise_measurement() alone.  There's already
>> > code at the end of process_measurement() which decides what to return.
>> > Just modify it based on the appraisal results.
>>
>
> If we do this, audit logs will be filled with integrity unknown failures.
> As each unsigned executable file will fail appraisal with INTEGRITY_UNKNOWN
> and an audit message will be logged.
>
> Thanks
> Vivek
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Wed, Feb 13, 2013 at 02:14:55PM +0200, Kasatkin, Dmitry wrote:
> Hello Vivek,
> 
> Can you please send to us how your IMA policy looks like.

Hi Dmitry,

For testing purposes, I am using following.

appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional

I set this using /sys/kernel/security/policy interface after boot.

Thanks
Vivek

> 
> Thanks,
> Dmitry
> 
> On Tue, Feb 12, 2013 at 8:57 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Tue, Feb 12, 2013 at 01:52:03PM -0500, Vivek Goyal wrote:
> >> On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
> >>
> >> [..]
> >> > > > > --- a/security/integrity/ima/ima_appraise.c
> >> > > > > +++ b/security/integrity/ima/ima_appraise.c
> >> > > > > @@ -124,19 +124,26 @@ 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;
> >> > > > > -     if (!inode->i_op->getxattr)
> >> > > > > +     if (!inode->i_op->getxattr) {
> >> > > > > +             /* getxattr not supported. file couldn't have been signed */
> >> > > > > +             if (iint->flags & IMA_DIGSIG_OPTIONAL)
> >> > > > > +                     return INTEGRITY_PASS;
> >> > > > >               return INTEGRITY_UNKNOWN;
> >> > > > > +     }
> >> > > > >
> >> > > >
> >> > > > Please don't change the result of the appraisal like this.  A single
> >> > > > change can be made towards the bottom of process_measurement().
> >> > >
> >> > > I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
> >> > > I can probably maintain a bool variable, say pass_appraisal, and set
> >> > > that here and at the end of function, parse that variable and change
> >> > > the status accordingly.
> >> >
> >> > process_measurement() is the only caller of ima_appraise_measurement().
> >> > Leave the results of ima_appraise_measurement() alone.  There's already
> >> > code at the end of process_measurement() which decides what to return.
> >> > Just modify it based on the appraisal results.
> >>
> >
> > If we do this, audit logs will be filled with integrity unknown failures.
> > As each unsigned executable file will fail appraisal with INTEGRITY_UNKNOWN
> > and an audit message will be logged.
> >
> > Thanks
> > Vivek
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Kasatkin, Dmitry]

It should not be the only line in the policy.
Can you share full policy?

Thanks,
Dmitry


On Wed, Feb 13, 2013 at 3:29 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Feb 13, 2013 at 02:14:55PM +0200, Kasatkin, Dmitry wrote:
>> Hello Vivek,
>>
>> Can you please send to us how your IMA policy looks like.
>
> Hi Dmitry,
>
> For testing purposes, I am using following.
>
> appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional
>
> I set this using /sys/kernel/security/policy interface after boot.
>
> Thanks
> Vivek
>
>>
>> Thanks,
>> Dmitry
>>
>> On Tue, Feb 12, 2013 at 8:57 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Tue, Feb 12, 2013 at 01:52:03PM -0500, Vivek Goyal wrote:
>> >> On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
>> >>
>> >> [..]
>> >> > > > > --- a/security/integrity/ima/ima_appraise.c
>> >> > > > > +++ b/security/integrity/ima/ima_appraise.c
>> >> > > > > @@ -124,19 +124,26 @@ 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;
>> >> > > > > -     if (!inode->i_op->getxattr)
>> >> > > > > +     if (!inode->i_op->getxattr) {
>> >> > > > > +             /* getxattr not supported. file couldn't have been signed */
>> >> > > > > +             if (iint->flags & IMA_DIGSIG_OPTIONAL)
>> >> > > > > +                     return INTEGRITY_PASS;
>> >> > > > >               return INTEGRITY_UNKNOWN;
>> >> > > > > +     }
>> >> > > > >
>> >> > > >
>> >> > > > Please don't change the result of the appraisal like this.  A single
>> >> > > > change can be made towards the bottom of process_measurement().
>> >> > >
>> >> > > I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
>> >> > > I can probably maintain a bool variable, say pass_appraisal, and set
>> >> > > that here and at the end of function, parse that variable and change
>> >> > > the status accordingly.
>> >> >
>> >> > process_measurement() is the only caller of ima_appraise_measurement().
>> >> > Leave the results of ima_appraise_measurement() alone.  There's already
>> >> > code at the end of process_measurement() which decides what to return.
>> >> > Just modify it based on the appraisal results.
>> >>
>> >
>> > If we do this, audit logs will be filled with integrity unknown failures.
>> > As each unsigned executable file will fail appraisal with INTEGRITY_UNKNOWN
>> > and an audit message will be logged.
>> >
>> > Thanks
>> > Vivek
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote:
> It should not be the only line in the policy.

So a single rule is not allowed or kernel has imposed more rules
internally.

> Can you share full policy?

How do I get to full policy. Is there an interface I can read it from?

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Wed, 2013-02-13 at 15:36 +0200, Kasatkin, Dmitry wrote:
> It should not be the only line in the policy.
> Can you share full policy?

> On Wed, Feb 13, 2013 at 3:29 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional

Different use cases require different policies.  Our concern is that
appraising file integrity not be added to the kernel in an ad-hoc
manner.

This rule implements the intent of the original patches Vivek posted.
We might personally disagree with the intent, but that is a totally
separate discussion - one that should have been raised when he posted
the original patches.

thanks,

Mimi

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote:
> It should not be the only line in the policy.
> Can you share full policy?

I verified by putting some printk. There is only single rule in
ima_policy_rules list after I have updated the rules through "policy"
file.

echo "appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional" >
/sys/kernel/security/policy

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Kasatkin, Dmitry]

On Wed, Feb 13, 2013 at 4:38 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote:
>> It should not be the only line in the policy.
>> Can you share full policy?
>
> I verified by putting some printk. There is only single rule in
> ima_policy_rules list after I have updated the rules through "policy"
> file.
>
> echo "appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional" >
> /sys/kernel/security/policy

There is a default policy which has several rules.

And when you do your "echo" you will replace all rules with that single rule.
But ok, you have one rule only and it is fine to have even a single rule...

>
> Thanks
> Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Kasatkin, Dmitry]

On Wed, Feb 13, 2013 at 5:26 PM, Kasatkin, Dmitry
<dmitry.kasatkin@intel.com> wrote:
> On Wed, Feb 13, 2013 at 4:38 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote:
>>> It should not be the only line in the policy.
>>> Can you share full policy?
>>
>> I verified by putting some printk. There is only single rule in
>> ima_policy_rules list after I have updated the rules through "policy"
>> file.
>>
>> echo "appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional" >
>> /sys/kernel/security/policy
>
> There is a default policy which has several rules.
>
> And when you do your "echo" you will replace all rules with that single rule.
> But ok, you have one rule only and it is fine to have even a single rule...
>

What I have not said yet it is that in your case, because you use only
BPRM_CHECK hook,
you do not need "dont_appraise" and "dont_measure" rules for pseudo filesystems.


>>
>> Thanks
>> Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Wed, Feb 13, 2013 at 05:29:43PM +0200, Kasatkin, Dmitry wrote:
> On Wed, Feb 13, 2013 at 5:26 PM, Kasatkin, Dmitry
> <dmitry.kasatkin@intel.com> wrote:
> > On Wed, Feb 13, 2013 at 4:38 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote:
> >>> It should not be the only line in the policy.
> >>> Can you share full policy?
> >>
> >> I verified by putting some printk. There is only single rule in
> >> ima_policy_rules list after I have updated the rules through "policy"
> >> file.
> >>
> >> echo "appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional" >
> >> /sys/kernel/security/policy
> >
> > There is a default policy which has several rules.
> >
> > And when you do your "echo" you will replace all rules with that single rule.
> > But ok, you have one rule only and it is fine to have even a single rule...
> >
> 
> What I have not said yet it is that in your case, because you use only
> BPRM_CHECK hook,
> you do not need "dont_appraise" and "dont_measure" rules for pseudo filesystems.

Agreed.

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Wed, Feb 13, 2013 at 05:26:27PM +0200, Kasatkin, Dmitry wrote:
> On Wed, Feb 13, 2013 at 4:38 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote:
> >> It should not be the only line in the policy.
> >> Can you share full policy?
> >
> > I verified by putting some printk. There is only single rule in
> > ima_policy_rules list after I have updated the rules through "policy"
> > file.
> >
> > echo "appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional" >
> > /sys/kernel/security/policy
> 
> There is a default policy which has several rules.
> 
> And when you do your "echo" you will replace all rules with that single rule.
> But ok, you have one rule only and it is fine to have even a single rule...

Yep, I got that. Default policy gets overruled when a new policy is
loaded.

In secureboot mode, somehow above rule needs to take effect by default.
One option would be that kernel can enforce above rule.
(I guess by adding it to both default_list as well as policy list).

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Wed, 2013-02-13 at 10:30 -0500, Vivek Goyal wrote:
> On Wed, Feb 13, 2013 at 05:26:27PM +0200, Kasatkin, Dmitry wrote:
> > On Wed, Feb 13, 2013 at 4:38 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > > On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote:
> > >> It should not be the only line in the policy.
> > >> Can you share full policy?
> > >
> > > I verified by putting some printk. There is only single rule in
> > > ima_policy_rules list after I have updated the rules through "policy"
> > > file.
> > >
> > > echo "appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional" >
> > > /sys/kernel/security/policy
> > 
> > There is a default policy which has several rules.
> > 
> > And when you do your "echo" you will replace all rules with that single rule.
> > But ok, you have one rule only and it is fine to have even a single rule...
> 
> Yep, I got that. Default policy gets overruled when a new policy is
> loaded.
> 
> In secureboot mode, somehow above rule needs to take effect by default.
> One option would be that kernel can enforce above rule.
> (I guess by adding it to both default_list as well as policy list).

The default policy is empty, but can be replaced with boot command line
options.  The existing options are ima_tcb and/ ima_appraise_tcb.
Please feel free to define an additional policy.

thanks,

Mimi

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Wed, Feb 13, 2013 at 05:27:01PM -0500, Mimi Zohar wrote:

[..]
> > Yep, I got that. Default policy gets overruled when a new policy is
> > loaded.
> > 
> > In secureboot mode, somehow above rule needs to take effect by default.
> > One option would be that kernel can enforce above rule.
> > (I guess by adding it to both default_list as well as policy list).
> 
> The default policy is empty, but can be replaced with boot command line
> options.  The existing options are ima_tcb and/ ima_appraise_tcb.
> Please feel free to define an additional policy.

I think just defining a new command line option is not sufficient
for secureboot use case.

- One can easily remove kernel command line option without breaking
  booting and easily bypass secureboot restrictions.

- I guess this is one mandated rule by secureboot. There might still
  be a user policy which can co-exist with this rule.

So to me this is not a new policy. It is just one mandatory rule which
gets appended to any policy in secureboot mode. Think of it as mandatory
rule imposed by kernel for any policy user can define. And in secureboot
mode a user can not get rid of this rule. (Otherwise it breaks user
space signing and one can bypass secureboot and boot into unsigned
kernel).

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Thu, 2013-02-14 at 10:03 -0500, Vivek Goyal wrote:
> On Wed, Feb 13, 2013 at 05:27:01PM -0500, Mimi Zohar wrote:
> 
> [..]
> > > Yep, I got that. Default policy gets overruled when a new policy is
> > > loaded.
> > > 
> > > In secureboot mode, somehow above rule needs to take effect by default.
> > > One option would be that kernel can enforce above rule.
> > > (I guess by adding it to both default_list as well as policy list).
> > 
> > The default policy is empty, but can be replaced with boot command line
> > options.  The existing options are ima_tcb and/ ima_appraise_tcb.
> > Please feel free to define an additional policy.
> 
> I think just defining a new command line option is not sufficient
> for secureboot use case.
> 
> - One can easily remove kernel command line option without breaking
>   booting and easily bypass secureboot restrictions.

> - I guess this is one mandated rule by secureboot. There might still
>   be a user policy which can co-exist with this rule.
> 
> So to me this is not a new policy. It is just one mandatory rule which
> gets appended to any policy in secureboot mode. Think of it as mandatory
> rule imposed by kernel for any policy user can define. And in secureboot
> mode a user can not get rid of this rule. (Otherwise it breaks user
> space signing and one can bypass secureboot and boot into unsigned
> kernel).

Your rule allows both signed and unsigned files to be executed.  Signed
files will just have more capabilities.  The ima_appraise_tcb option
requires all files owned by root to be signed, otherwise access is
denied.  The two policies simply can not co-exist.

How about defining your single rule as ima_secureboot and making it the
default policy.  Only if ima_appraise_tcb is specified on the kernel
command line, will the default policy be replaced.  This type of change,
going from a null policy to an ima_secureboot policy, would require
community approval.

thanks,

Mimi

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Thu, Feb 14, 2013 at 10:30:15AM -0500, Mimi Zohar wrote:
> On Thu, 2013-02-14 at 10:03 -0500, Vivek Goyal wrote:
> > On Wed, Feb 13, 2013 at 05:27:01PM -0500, Mimi Zohar wrote:
> > 
> > [..]
> > > > Yep, I got that. Default policy gets overruled when a new policy is
> > > > loaded.
> > > > 
> > > > In secureboot mode, somehow above rule needs to take effect by default.
> > > > One option would be that kernel can enforce above rule.
> > > > (I guess by adding it to both default_list as well as policy list).
> > > 
> > > The default policy is empty, but can be replaced with boot command line
> > > options.  The existing options are ima_tcb and/ ima_appraise_tcb.
> > > Please feel free to define an additional policy.
> > 
> > I think just defining a new command line option is not sufficient
> > for secureboot use case.
> > 
> > - One can easily remove kernel command line option without breaking
> >   booting and easily bypass secureboot restrictions.
> 
> > - I guess this is one mandated rule by secureboot. There might still
> >   be a user policy which can co-exist with this rule.
> > 
> > So to me this is not a new policy. It is just one mandatory rule which
> > gets appended to any policy in secureboot mode. Think of it as mandatory
> > rule imposed by kernel for any policy user can define. And in secureboot
> > mode a user can not get rid of this rule. (Otherwise it breaks user
> > space signing and one can bypass secureboot and boot into unsigned
> > kernel).
> 
> Your rule allows both signed and unsigned files to be executed.  Signed
> files will just have more capabilities.  The ima_appraise_tcb option
> requires all files owned by root to be signed, otherwise access is
> denied.  The two policies simply can not co-exist.

Thinking loud. I guess we might have to extend ima policy/rules to allow
multiple appraise rules to co-exist. And access permission will finally
depend on if all the rules in same category return success.

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Mon, 2013-02-18 at 13:21 -0500, Vivek Goyal wrote:
> On Thu, Feb 14, 2013 at 10:30:15AM -0500, Mimi Zohar wrote:
> > On Thu, 2013-02-14 at 10:03 -0500, Vivek Goyal wrote:
> > > On Wed, Feb 13, 2013 at 05:27:01PM -0500, Mimi Zohar wrote:
> > > 
> > > [..]
> > > > > Yep, I got that. Default policy gets overruled when a new policy is
> > > > > loaded.
> > > > > 
> > > > > In secureboot mode, somehow above rule needs to take effect by default.
> > > > > One option would be that kernel can enforce above rule.
> > > > > (I guess by adding it to both default_list as well as policy list).
> > > > 
> > > > The default policy is empty, but can be replaced with boot command line
> > > > options.  The existing options are ima_tcb and/ ima_appraise_tcb.
> > > > Please feel free to define an additional policy.
> > > 
> > > I think just defining a new command line option is not sufficient
> > > for secureboot use case.
> > > 
> > > - One can easily remove kernel command line option without breaking
> > >   booting and easily bypass secureboot restrictions.
> > 
> > > - I guess this is one mandated rule by secureboot. There might still
> > >   be a user policy which can co-exist with this rule.
> > > 
> > > So to me this is not a new policy. It is just one mandatory rule which
> > > gets appended to any policy in secureboot mode. Think of it as mandatory
> > > rule imposed by kernel for any policy user can define. And in secureboot
> > > mode a user can not get rid of this rule. (Otherwise it breaks user
> > > space signing and one can bypass secureboot and boot into unsigned
> > > kernel).
> > 
> > Your rule allows both signed and unsigned files to be executed.  Signed
> > files will just have more capabilities.  The ima_appraise_tcb option
> > requires all files owned by root to be signed, otherwise access is
> > denied.  The two policies simply can not co-exist.
> 
> Thinking loud. I guess we might have to extend ima policy/rules to allow
> multiple appraise rules to co-exist. And access permission will finally
> depend on if all the rules in same category return success.

ima_appraise_tcb rules:
dont_appraise  fsmagic=PROC_SUPER_MAGIC
.
.
.
dont_appraise fsmagic=SELINUX_MAGIC
appraise fowner=0

ima_secureboot:
appraise fowner=0 func=bprm appraise_type=optional

The ima_appraise_tcb appraise rule includes everything that would match
the ima_secureboot rule.  It isn't possible to combine the two policies.

Mimi

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Wed, 2013-02-13 at 09:38 -0500, Vivek Goyal wrote:
> On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote:
> > It should not be the only line in the policy.
> > Can you share full policy?
> 
> I verified by putting some printk. 

If anyone is interested in posting a patch to display the current
policy, it shouldn't be too difficult to do.

> There is only single rule in
> ima_policy_rules list after I have updated the rules through "policy"
> file.

> echo "appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional" >
> /sys/kernel/security/policy

Right, this replaces one policy with another one.

thanks,

Mimi

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Tue, 2013-02-12 at 13:52 -0500, Vivek Goyal wrote:
> On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
> 
> [..]
> > > > > --- a/security/integrity/ima/ima_appraise.c
> > > > > +++ b/security/integrity/ima/ima_appraise.c
> > > > > @@ -124,19 +124,26 @@ 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;
> > > > > -	if (!inode->i_op->getxattr)
> > > > > +	if (!inode->i_op->getxattr) {
> > > > > +		/* getxattr not supported. file couldn't have been signed */
> > > > > +		if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > > > > +			return INTEGRITY_PASS;
> > > > >  		return INTEGRITY_UNKNOWN;
> > > > > +	}
> > > > > 
> > > > 
> > > > Please don't change the result of the appraisal like this.  A single
> > > > change can be made towards the bottom of process_measurement().
> > > 
> > > I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
> > > I can probably maintain a bool variable, say pass_appraisal, and set
> > > that here and at the end of function, parse that variable and change
> > > the status accordingly.
> > 
> > process_measurement() is the only caller of ima_appraise_measurement().
> > Leave the results of ima_appraise_measurement() alone.  There's already
> > code at the end of process_measurement() which decides what to return.
> > Just modify it based on the appraisal results.
> 
> Ok, I can do that. There is a small concern though. That is what to do
> when rc = INTEGRITY_UKNOWN and IMA_DIGSIG_OPTIONAL flag is set.
> 
> ima_appraise_measurement() returns INTEGRITY_UKNOWN when file system
> does not support xattrs or if security xattr is not enabled. In this
> case it is desirable to allow access if IMA_DIGSIG_OPTIONAL flag is
> set.

Right, 'INTEGRITY_UNKNOWN' means that we can't reason, for whatever
reason, about the integrity of the file.

> But INTEGRITY_UNKNOWN is also also returned when integrity_digsig_verify()
> fails and returns -EOPNOTSUPP. 

In this case, it is Kconfig based. 

> I feel that in this case it is not very appropriate to pass appraisal and
> let executable run. If digital signatures are present but we can't verify
> those (Say some algorithm is not supported in kernel). In that case I
> think it makes sense to fail the signature. 
> 
>                rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
>                                              xattr_value->digest, rc - 1,
>                                              iint->ima_xattr.digest,
>                                              IMA_DIGEST_SIZE);
>                 if (rc == -EOPNOTSUPP) {
>                         status = INTEGRITY_UNKNOWN;
> 
> 
> So how to handle this case.
> 
> I am wondering why do we reutrn INTEGRITY_UNKNOWN above and not
> INTEGRITY_FAIL.

We still can't reason about the integrity of the file.  For all we know,
it could be a validly signed file, just verification wasn't enabled.

> Will it make sense to fail signature in case of -EOPNOTSUPP.
>                rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
>                                              xattr_value->digest, rc - 1,
>                                              iint->ima_xattr.digest,
>                                              IMA_DIGEST_SIZE);
> 		if (rc)
> 			status = INTEGRITY_FAIL;
> 		else
> 			status = INTEGRITY_PASS;
> 
> 

Please don't.

Mimi

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Kasatkin, Dmitry]

On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> appraise_type=imasig_optional will 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.
>
> This can allow to selectively sign executables in the system and based
> on appraisal results, signed executables with valid signatures can be
> given extra capability to perform priviliged operations in secureboot
> mode.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  Documentation/ABI/testing/ima_policy  |    2 +-
>  security/integrity/ima/ima_appraise.c |   24 +++++++++++++++++++-----
>  security/integrity/ima/ima_policy.c   |    2 ++
>  security/integrity/integrity.h        |    1 +
>  4 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index de16de3..5ca0c23 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] | [imasig_optional]
>
>                 default policy:
>                         # PROC_SUPER_MAGIC
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 3710f44..222ade0 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -124,19 +124,26 @@ 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;
> -       if (!inode->i_op->getxattr)
> +       if (!inode->i_op->getxattr) {
> +               /* getxattr not supported. file couldn't have been signed */
> +               if (iint->flags & IMA_DIGSIG_OPTIONAL)
> +                       return INTEGRITY_PASS;
>                 return INTEGRITY_UNKNOWN;
> +       }
>
>         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)
> +               if (rc == -EOPNOTSUPP) {
> +                       if (iint->flags & IMA_DIGSIG_OPTIONAL)
> +                               return INTEGRITY_PASS;
>                         return INTEGRITY_UNKNOWN;
> +               }
>
>                 if (rc && rc != -ENODATA)
>                         goto out;
> @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
>         }
>         switch (xattr_value->type) {
>         case IMA_XATTR_DIGEST:
> -               if (iint->flags & IMA_DIGSIG_REQUIRED) {
> +               if (iint->flags & IMA_DIGSIG_REQUIRED ||
> +                   iint->flags & IMA_DIGSIG_OPTIONAL) {
>                         cause = "IMA signature required";
>                         status = INTEGRITY_FAIL;
>                         break;

This looks a bit odd... If "optional" signature is missing  - we fail..
It is optional... Why we should fail?

Mimi?

> @@ -201,8 +209,14 @@ out:
>                         if (!ima_fix_xattr(dentry, iint))
>                                 status = INTEGRITY_PASS;
>                 }
> +               if (status == INTEGRITY_NOLABEL &&
> +                   iint->flags & IMA_DIGSIG_OPTIONAL) {
> +                       status = INTEGRITY_PASS;
> +                       /* Don't flood audit logs with skipped appraise */
> +                       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);
>         }
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 4adcd0f..8b8cd5f 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, "imasig_optional")) == 0)
> +                               entry->flags |= IMA_DIGSIG_OPTIONAL;
>                         else
>                                 result = -EINVAL;
>                         break;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 84c37c4..2ba736b 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -30,6 +30,7 @@
>  #define IMA_ACTION_FLAGS       0xff000000
>  #define IMA_DIGSIG             0x01000000
>  #define IMA_DIGSIG_REQUIRED    0x02000000
> +#define IMA_DIGSIG_OPTIONAL    0x04000000
>
>  #define IMA_DO_MASK            (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
>                                  IMA_APPRAISE_SUBMASK)
> --
> 1.7.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
> On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal <vgoyal@redhat.com> wrote:

> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> >         }
> >         switch (xattr_value->type) {
> >         case IMA_XATTR_DIGEST:
> > -               if (iint->flags & IMA_DIGSIG_REQUIRED) {
> > +               if (iint->flags & IMA_DIGSIG_REQUIRED ||
> > +                   iint->flags & IMA_DIGSIG_OPTIONAL) {
> >                         cause = "IMA signature required";
> >                         status = INTEGRITY_FAIL;
> >                         break;
> 
> This looks a bit odd... If "optional" signature is missing  - we fail..
> It is optional... Why we should fail?

'imasig_optional' does not only mean that the signature is optional, but
also implies that it has to be a digital signature, not a hash.  This
latter part is what IMA_DIGSIG_REQUIRED means.

If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.

IMA_DIGSIG_REQUIRED would enforce that it is a signature.
IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.

thanks,

Mimi

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Kasatkin, Dmitry]

On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
>> On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>
>> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
>> >         }
>> >         switch (xattr_value->type) {
>> >         case IMA_XATTR_DIGEST:
>> > -               if (iint->flags & IMA_DIGSIG_REQUIRED) {
>> > +               if (iint->flags & IMA_DIGSIG_REQUIRED ||
>> > +                   iint->flags & IMA_DIGSIG_OPTIONAL) {
>> >                         cause = "IMA signature required";
>> >                         status = INTEGRITY_FAIL;
>> >                         break;
>>
>> This looks a bit odd... If "optional" signature is missing  - we fail..
>> It is optional... Why we should fail?
>
> 'imasig_optional' does not only mean that the signature is optional, but
> also implies that it has to be a digital signature, not a hash.  This
> latter part is what IMA_DIGSIG_REQUIRED means.
>
> If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
> 'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
>
> IMA_DIGSIG_REQUIRED would enforce that it is a signature.
> IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
>

It sounds odd that optional signature is actually required.
Optional for me means that it may be there or may be not.
If it is not there, then it may be hash or nothing.

I see it is more logical if it is "appraise_type=optional",
which means that we might have no xattr value, hash or signature.
It if happens to be a signature, then IMA_DIGSIG flag will be set.

I asked Vivek to send a policy file he uses in his system.
I would like to see how system configured as a whole...

- Dmitry

> thanks,
>
> Mimi
>

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Wed, 2013-02-13 at 15:13 +0200, Kasatkin, Dmitry wrote:
> On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
> >> On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> >> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> >> >         }
> >> >         switch (xattr_value->type) {
> >> >         case IMA_XATTR_DIGEST:
> >> > -               if (iint->flags & IMA_DIGSIG_REQUIRED) {
> >> > +               if (iint->flags & IMA_DIGSIG_REQUIRED ||
> >> > +                   iint->flags & IMA_DIGSIG_OPTIONAL) {
> >> >                         cause = "IMA signature required";
> >> >                         status = INTEGRITY_FAIL;
> >> >                         break;
> >>
> >> This looks a bit odd... If "optional" signature is missing  - we fail..
> >> It is optional... Why we should fail?
> >
> > 'imasig_optional' does not only mean that the signature is optional, but
> > also implies that it has to be a digital signature, not a hash.  This
> > latter part is what IMA_DIGSIG_REQUIRED means.
> >
> > If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
> > 'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
> >
> > IMA_DIGSIG_REQUIRED would enforce that it is a signature.
> > IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
> >
> 
> It sounds odd that optional signature is actually required.
> Optional for me means that it may be there or may be not.
> If it is not there, then it may be hash or nothing.
> 
> I see it is more logical if it is "appraise_type=optional",
> which means that we might have no xattr value, hash or signature.
> It if happens to be a signature, then IMA_DIGSIG flag will be set.

Right, 'appraise_type=' could have been defined either as a comma
separated list of options (eg. appraise_type=imassig,optional) or, as
Vivek implemented, a new option.  Eventually, we will need to extend
'appraise_type=' (eg. required algorithm), but for now I don't have a
problem with the new option.

thanks,

Mimi

> I asked Vivek to send a policy file he uses in his system.
> I would like to see how system configured as a whole...
> 
> - Dmitry

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Wed, Feb 13, 2013 at 08:44:04AM -0500, Mimi Zohar wrote:

[..]
> > I see it is more logical if it is "appraise_type=optional",
> > which means that we might have no xattr value, hash or signature.
> > It if happens to be a signature, then IMA_DIGSIG flag will be set.
> 
> Right, 'appraise_type=' could have been defined either as a comma
> separated list of options (eg. appraise_type=imassig,optional) or, as
> Vivek implemented, a new option.  Eventually, we will need to extend
> 'appraise_type=' (eg. required algorithm), but for now I don't have a
> problem with the new option.

Ok, I will cleanup the code to do above. Just wanted to clear up one
point. 

Above option will not have any effect on evm behavior? This only impacts
IMA appraisal behavior. For example, if security.ima is not present it
is fine and file access is allowed. But if EVM is enabled and initialized
and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns
INTEGRITY_NOXATTRS, file access should still be denied?

BTW, what's the difference between INTEGRITY_NOLABEL and
INTEGRITY_NOXATTRS (as returned by evm_verifyxattr()).

If yes, we probbaly need to differentiate between IMA/EVM nolabel. Say,
INTEGRITY_IMA_NOLABEL and INTEGRITY_EVM_NOLABEL. appraise_type=optional
should allow file access if rc = INTEGRITY_IMA_NOLABEL but deny it
when rc = INTEGRITY_EVM_NOLABEL?

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Wed, 2013-02-13 at 11:59 -0500, Vivek Goyal wrote:
> On Wed, Feb 13, 2013 at 08:44:04AM -0500, Mimi Zohar wrote:
> 
> [..]
> > > I see it is more logical if it is "appraise_type=optional",
> > > which means that we might have no xattr value, hash or signature.
> > > It if happens to be a signature, then IMA_DIGSIG flag will be set.
> > 
> > Right, 'appraise_type=' could have been defined either as a comma
> > separated list of options (eg. appraise_type=imassig,optional) or, as
> > Vivek implemented, a new option.  Eventually, we will need to extend
> > 'appraise_type=' (eg. required algorithm), but for now I don't have a
> > problem with the new option.
> 
> Ok, I will cleanup the code to do above. Just wanted to clear up one
> point. 
> 
> Above option will not have any effect on evm behavior? This only impacts
> IMA appraisal behavior. For example, if security.ima is not present it
> is fine and file access is allowed. But if EVM is enabled and initialized
> and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns
> INTEGRITY_NOXATTRS, file access should still be denied?

Can't happen.  evm_verifyxattr() is called from
ima_appraise_measurement(), only if 'security.ima' exists.

> BTW, what's the difference between INTEGRITY_NOLABEL and
> INTEGRITY_NOXATTRS (as returned by evm_verifyxattr()).

INTEGRITY_NOLABEL indicates the requested xattr doesn't exist, while
INTEGRITY_NOXATTRS implies no EVM protected xattrs exist.  The latter
normally occurs when a file is first created.

Mimi

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote:

[..]
> > Ok, I will cleanup the code to do above. Just wanted to clear up one
> > point. 
> > 
> > Above option will not have any effect on evm behavior? This only impacts
> > IMA appraisal behavior. For example, if security.ima is not present it
> > is fine and file access is allowed. But if EVM is enabled and initialized
> > and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns
> > INTEGRITY_NOXATTRS, file access should still be denied?
> 
> Can't happen.  evm_verifyxattr() is called from
> ima_appraise_measurement(), only if 'security.ima' exists.

Actually what I meant is following.

Currently in process_measurement(), I will allow access if
ima_appraise_measurement() returns INTEGRITY_NOLABEL.

Now this could mean 2 things.

- security.ima was not present.
- security.ima was there but security.evm was not present.

With appraise_type=optional, I think we would want to allow access in
first case but not the second one. IOW, appraise_type= affects behavior
of IMA and not EVM.

That means we need to introduce new codes.

INTEGRITY_IMA_NOLABEL and INTEGRITY_EVM_NOLABEL to differentiate between
above two cases?

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Thu, 2013-02-14 at 10:23 -0500, Vivek Goyal wrote:
> On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote:
> 
> [..]
> > > Ok, I will cleanup the code to do above. Just wanted to clear up one
> > > point. 
> > > 
> > > Above option will not have any effect on evm behavior? This only impacts
> > > IMA appraisal behavior. For example, if security.ima is not present it
> > > is fine and file access is allowed. But if EVM is enabled and initialized
> > > and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns
> > > INTEGRITY_NOXATTRS, file access should still be denied?
> > 
> > Can't happen.  evm_verifyxattr() is called from
> > ima_appraise_measurement(), only if 'security.ima' exists.
> 
> Actually what I meant is following.
> 
> Currently in process_measurement(), I will allow access if
> ima_appraise_measurement() returns INTEGRITY_NOLABEL.

I think you're making this more complicated than it needs to be.  Allow
the execution unless the file failed signature verification.  The
additional capability is given only if the signature verification
succeeds.

thanks,

Mimi

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Thu, Feb 14, 2013 at 10:35:59AM -0500, Mimi Zohar wrote:
> On Thu, 2013-02-14 at 10:23 -0500, Vivek Goyal wrote:
> > On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote:
> > 
> > [..]
> > > > Ok, I will cleanup the code to do above. Just wanted to clear up one
> > > > point. 
> > > > 
> > > > Above option will not have any effect on evm behavior? This only impacts
> > > > IMA appraisal behavior. For example, if security.ima is not present it
> > > > is fine and file access is allowed. But if EVM is enabled and initialized
> > > > and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns
> > > > INTEGRITY_NOXATTRS, file access should still be denied?
> > > 
> > > Can't happen.  evm_verifyxattr() is called from
> > > ima_appraise_measurement(), only if 'security.ima' exists.
> > 
> > Actually what I meant is following.
> > 
> > Currently in process_measurement(), I will allow access if
> > ima_appraise_measurement() returns INTEGRITY_NOLABEL.
> 
> I think you're making this more complicated than it needs to be.  Allow
> the execution unless the file failed signature verification.  The
> additional capability is given only if the signature verification
> succeeds.

I am just trying to bring it inline with module signature verification.
There also module loading fails if signatures are present but kernel
can't verify it.

Following behavior is strange.

                rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
                                             xattr_value->digest, rc - 1,
                                             iint->ima_xattr.digest,
                                             IMA_DIGEST_SIZE);
                if (rc == -EOPNOTSUPP) {
                        status = INTEGRITY_UNKNOWN;
                } else if (rc) {
                        cause = "invalid-signature";
                        status = INTEGRITY_FAIL;
                } else {
                        status = INTEGRITY_PASS;
                }

signature verification can fail for so many reasons.

- EINVAL
- keyring is not present
- key is not present -ENOKEY
- ENOTSUPP
- ENOMEM
..
..

And in all these cases we return INTEGRITY_FAIL. But only in case of
-EOPNOTSUPP we return INTEGRITY_UNKNOWN. So why this discrepancy.

So to me it makes sense to return INTEGRITY_FAIL if rc == -EOPNOTSUPP.
This will bring it inline with other error codes.  And then in
process_measurement() I can allow access in every case except
INTEGRITY_FAIL.

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Thu, Feb 14, 2013 at 11:17:19AM -0500, Vivek Goyal wrote:
> On Thu, Feb 14, 2013 at 10:35:59AM -0500, Mimi Zohar wrote:
> > On Thu, 2013-02-14 at 10:23 -0500, Vivek Goyal wrote:
> > > On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote:
> > > 
> > > [..]
> > > > > Ok, I will cleanup the code to do above. Just wanted to clear up one
> > > > > point. 
> > > > > 
> > > > > Above option will not have any effect on evm behavior? This only impacts
> > > > > IMA appraisal behavior. For example, if security.ima is not present it
> > > > > is fine and file access is allowed. But if EVM is enabled and initialized
> > > > > and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns
> > > > > INTEGRITY_NOXATTRS, file access should still be denied?
> > > > 
> > > > Can't happen.  evm_verifyxattr() is called from
> > > > ima_appraise_measurement(), only if 'security.ima' exists.
> > > 
> > > Actually what I meant is following.
> > > 
> > > Currently in process_measurement(), I will allow access if
> > > ima_appraise_measurement() returns INTEGRITY_NOLABEL.
> > 
> > I think you're making this more complicated than it needs to be.  Allow
> > the execution unless the file failed signature verification.  The
> > additional capability is given only if the signature verification
> > succeeds.
> 
> I am just trying to bring it inline with module signature verification.
> There also module loading fails if signatures are present but kernel
> can't verify it.
> 
> Following behavior is strange.
> 
>                 rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
>                                              xattr_value->digest, rc - 1,
>                                              iint->ima_xattr.digest,
>                                              IMA_DIGEST_SIZE);
>                 if (rc == -EOPNOTSUPP) {
>                         status = INTEGRITY_UNKNOWN;
>                 } else if (rc) {
>                         cause = "invalid-signature";
>                         status = INTEGRITY_FAIL;
>                 } else {
>                         status = INTEGRITY_PASS;
>                 }
> 
> signature verification can fail for so many reasons.
> 
> - EINVAL
> - keyring is not present
> - key is not present -ENOKEY
> - ENOTSUPP
> - ENOMEM
> ..
> ..
> 
> And in all these cases we return INTEGRITY_FAIL. But only in case of
> -EOPNOTSUPP we return INTEGRITY_UNKNOWN. So why this discrepancy.
> 
> So to me it makes sense to return INTEGRITY_FAIL if rc == -EOPNOTSUPP.
> This will bring it inline with other error codes.  And then in
> process_measurement() I can allow access in every case except
> INTEGRITY_FAIL.

Actually even this is not sufficient. Because if security.ima is present
and it contains digital signature but security.evm is not present (assume
EVM is enabled), then appraise_measurement() will return NOLABEL or
NOXATTRS and access to file will be allowed. 

But what I am trying to implement is that if digital signatures are
present, then they have to be valid. Any failure to assess the validity
of digital signature should result in failure of exec().

So to me it is important to come to a common understanding that
appraise_type=optional affects only IMA behavior and not EVM behavior. And
then there is a need to separate out IMA and EVM return codes so that
one can enforce appraise_type=optional properly. Otherwise we are leaving
lots of grey areas behind.

Thanks
Vivek

> 
> Thanks
> Vivek
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Thu, 2013-02-14 at 11:17 -0500, Vivek Goyal wrote:
> On Thu, Feb 14, 2013 at 10:35:59AM -0500, Mimi Zohar wrote:
> > On Thu, 2013-02-14 at 10:23 -0500, Vivek Goyal wrote:
> > > On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote:
> > > 
> > > [..]
> > > > > Ok, I will cleanup the code to do above. Just wanted to clear up one
> > > > > point. 
> > > > > 
> > > > > Above option will not have any effect on evm behavior? This only impacts
> > > > > IMA appraisal behavior. For example, if security.ima is not present it
> > > > > is fine and file access is allowed. But if EVM is enabled and initialized
> > > > > and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns
> > > > > INTEGRITY_NOXATTRS, file access should still be denied?
> > > > 
> > > > Can't happen.  evm_verifyxattr() is called from
> > > > ima_appraise_measurement(), only if 'security.ima' exists.
> > > 
> > > Actually what I meant is following.
> > > 
> > > Currently in process_measurement(), I will allow access if
> > > ima_appraise_measurement() returns INTEGRITY_NOLABEL.
> > 
> > I think you're making this more complicated than it needs to be.  Allow
> > the execution unless the file failed signature verification.  The
> > additional capability is given only if the signature verification
> > succeeds.
> 
> I am just trying to bring it inline with module signature verification.
> There also module loading fails if signatures are present but kernel
> can't verify it.

A specific hook is defined for kernel module signature verification,
which is enabled/disabled in Kconfig.  When enabled, only signed modules
are loaded.  The kernel module hook does not verify the integrity of the
userspace application (eg. insmod, modprobe), but of the kernel module
being loaded.

Your original patches verified the integrity of the userspace
application kexec, not the image being loaded.  ima_bprm_check()
verifies the integrity of executables.  To permit both signed and
unsigned files to execute, we defined the 'optional' IMA policy flag,
with the intention of giving more capability to signed executables.

Unless we define a kexec specific hook for verifying kernel images, it's
not the same.

> Following behavior is strange.
> 
>                 rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
>                                              xattr_value->digest, rc - 1,
>                                              iint->ima_xattr.digest,
>                                              IMA_DIGEST_SIZE);
>                 if (rc == -EOPNOTSUPP) {
>                         status = INTEGRITY_UNKNOWN;
>                 } else if (rc) {
>                         cause = "invalid-signature";
>                         status = INTEGRITY_FAIL;
>                 } else {
>                         status = INTEGRITY_PASS;
>                 }
> 
> signature verification can fail for so many reasons.
> 
> - EINVAL
> - keyring is not present
> - key is not present -ENOKEY
> - ENOTSUPP
> - ENOMEM
> ..
> ..
> 
> And in all these cases we return INTEGRITY_FAIL. But only in case of
> -EOPNOTSUPP we return INTEGRITY_UNKNOWN. So why this discrepancy.

This exception is for filesystems that don't support extended
attributes, such as CPIO.  We assumed, correctly/incorrectly, that the
initramfs would already be measured and appraised.

> So to me it makes sense to return INTEGRITY_FAIL if rc == -EOPNOTSUPP.

Perhaps, but unless the initramfs supports xattrs, we wouldn't be able
to boot.

> This will bring it inline with other error codes.  And then in
> process_measurement() I can allow access in every case except
> INTEGRITY_FAIL. 

thanks,

Mimi

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Thu, Feb 14, 2013 at 02:49:16PM -0500, Mimi Zohar wrote:

[..]
> > > I think you're making this more complicated than it needs to be.  Allow
> > > the execution unless the file failed signature verification.  The
> > > additional capability is given only if the signature verification
> > > succeeds.
> > 
> > I am just trying to bring it inline with module signature verification.
> > There also module loading fails if signatures are present but kernel
> > can't verify it.
> 
> A specific hook is defined for kernel module signature verification,
> which is enabled/disabled in Kconfig.  When enabled, only signed modules
> are loaded.  The kernel module hook does not verify the integrity of the
> userspace application (eg. insmod, modprobe), but of the kernel module
> being loaded.
> 
> Your original patches verified the integrity of the userspace
> application kexec, not the image being loaded.  ima_bprm_check()
> verifies the integrity of executables.  To permit both signed and
> unsigned files to execute, we defined the 'optional' IMA policy flag,
> with the intention of giving more capability to signed executables.
> 
> Unless we define a kexec specific hook for verifying kernel images, it's
> not the same.

I think we are talking of two different things here.

I am referring to kernel module signing where signatures are appended
to module (not IMA hook).

Also I am just referring to behavior about what happens if some error
happens while signature verification.

- If signature verification fails, it is clear what to do.
- If signature verification passes, it is clear what to do.
- Grey area is, what happens if some error is encountered during signature
  verification. Should the module loading be allowed/disallowed. Looking
  at the module loading code, once it is determined that module has
  signature appended to it, module loading fails if some error occurs
  during signature verification.

So I am just referring to that fact and trying to draw parallels between
error handling during module signature verification and error handling
when file appraisal happens in IMA. 

There can be two options.

- Disallow execution only if signature verification fails. If some error
  happens during verification, ignore it, let the executable continue.
  Just that it does not get extra capability.

- Disallow execution only if executable is not signed or it has valid
  signature. If executable is signed and some error happens during the
  process of verifying signature, execution is denied.

In V3 posting of my patches I have taken second appraoch right now. Though
it is up for detbate.


> 
> > Following behavior is strange.
> > 
> >                 rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
> >                                              xattr_value->digest, rc - 1,
> >                                              iint->ima_xattr.digest,
> >                                              IMA_DIGEST_SIZE);
> >                 if (rc == -EOPNOTSUPP) {
> >                         status = INTEGRITY_UNKNOWN;
> >                 } else if (rc) {
> >                         cause = "invalid-signature";
> >                         status = INTEGRITY_FAIL;
> >                 } else {
> >                         status = INTEGRITY_PASS;
> >                 }
> > 
> > signature verification can fail for so many reasons.
> > 
> > - EINVAL
> > - keyring is not present
> > - key is not present -ENOKEY
> > - ENOTSUPP
> > - ENOMEM
> > ..
> > ..
> > 
> > And in all these cases we return INTEGRITY_FAIL. But only in case of
> > -EOPNOTSUPP we return INTEGRITY_UNKNOWN. So why this discrepancy.
> 
> This exception is for filesystems that don't support extended
> attributes, such as CPIO.  We assumed, correctly/incorrectly, that the
> initramfs would already be measured and appraised.

Who makes use of it. ima_appraise_measurement() will return
INTEGRITY_UNKNOWN if filesystem does not support xattr. And
process_measurement() will deny access for INTEGRITY_UNKNOWN.

        if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
                return -EACCES;

So IIUC, if file system does not support xattr, appraisal will anyway
fail. So why to special case -EOPNOTSUPP with integrity_digsig_verify().


> 
> > So to me it makes sense to return INTEGRITY_FAIL if rc == -EOPNOTSUPP.
> 
> Perhaps, but unless the initramfs supports xattrs, we wouldn't be able
> to boot.

Am I reading the code wrong? Given above code snippet, if rc is non
zero, -EACCSS will be returned. So if initramfs does not support
xattr, appraisal will fail.

I suspect following default rule saves us there as it will avoid
appraisal on tmpfs.

action = DONT_APPRAISE,.fsmagic = TMPFS_MAGIC,.flags = IMA_FSMAGIC

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Thu, Feb 14, 2013 at 03:54:45PM -0500, Vivek Goyal wrote:
> On Thu, Feb 14, 2013 at 02:49:16PM -0500, Mimi Zohar wrote:
> 
> [..]
> > > > I think you're making this more complicated than it needs to be.  Allow
> > > > the execution unless the file failed signature verification.  The
> > > > additional capability is given only if the signature verification
> > > > succeeds.
> > > 
> > > I am just trying to bring it inline with module signature verification.
> > > There also module loading fails if signatures are present but kernel
> > > can't verify it.
> > 
> > A specific hook is defined for kernel module signature verification,
> > which is enabled/disabled in Kconfig.  When enabled, only signed modules
> > are loaded.  The kernel module hook does not verify the integrity of the
> > userspace application (eg. insmod, modprobe), but of the kernel module
> > being loaded.
> > 
> > Your original patches verified the integrity of the userspace
> > application kexec, not the image being loaded.  ima_bprm_check()
> > verifies the integrity of executables.  To permit both signed and
> > unsigned files to execute, we defined the 'optional' IMA policy flag,
> > with the intention of giving more capability to signed executables.
> > 
> > Unless we define a kexec specific hook for verifying kernel images, it's
> > not the same.
> 
> I think we are talking of two different things here.
> 
> I am referring to kernel module signing where signatures are appended
> to module (not IMA hook).
> 
> Also I am just referring to behavior about what happens if some error
> happens while signature verification.
> 
> - If signature verification fails, it is clear what to do.
> - If signature verification passes, it is clear what to do.
> - Grey area is, what happens if some error is encountered during signature
>   verification. Should the module loading be allowed/disallowed. Looking
>   at the module loading code, once it is determined that module has
>   signature appended to it, module loading fails if some error occurs
>   during signature verification.
> 
> So I am just referring to that fact and trying to draw parallels between
> error handling during module signature verification and error handling
> when file appraisal happens in IMA. 
> 
> There can be two options.
> 
> - Disallow execution only if signature verification fails. If some error
>   happens during verification, ignore it, let the executable continue.
>   Just that it does not get extra capability.
> 
> - Disallow execution only if executable is not signed or it has valid
>   signature. If executable is signed and some error happens during the
>   process of verifying signature, execution is denied.
> 

Little typo in second option. I meant "Allow execution only if executable
is not signed or it has valid signatures".

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Thu, 2013-02-14 at 15:57 -0500, Vivek Goyal wrote:
> On Thu, Feb 14, 2013 at 03:54:45PM -0500, Vivek Goyal wrote:
> > On Thu, Feb 14, 2013 at 02:49:16PM -0500, Mimi Zohar wrote:
> > 
> > [..]
> > > > > I think you're making this more complicated than it needs to be.  Allow
> > > > > the execution unless the file failed signature verification.  The
> > > > > additional capability is given only if the signature verification
> > > > > succeeds.
> > > > 
> > > > I am just trying to bring it inline with module signature verification.
> > > > There also module loading fails if signatures are present but kernel
> > > > can't verify it.
> > > 
> > > A specific hook is defined for kernel module signature verification,
> > > which is enabled/disabled in Kconfig.  When enabled, only signed modules
> > > are loaded.  The kernel module hook does not verify the integrity of the
> > > userspace application (eg. insmod, modprobe), but of the kernel module
> > > being loaded.
> > > 
> > > Your original patches verified the integrity of the userspace
> > > application kexec, not the image being loaded.  ima_bprm_check()
> > > verifies the integrity of executables.  To permit both signed and
> > > unsigned files to execute, we defined the 'optional' IMA policy flag,
> > > with the intention of giving more capability to signed executables.
> > > 
> > > Unless we define a kexec specific hook for verifying kernel images, it's
> > > not the same.
> > 
> > I think we are talking of two different things here.
> > 
> > I am referring to kernel module signing where signatures are appended
> > to module (not IMA hook).
> > 
> > Also I am just referring to behavior about what happens if some error
> > happens while signature verification.
> > 
> > - If signature verification fails, it is clear what to do.
> > - If signature verification passes, it is clear what to do.
> > - Grey area is, what happens if some error is encountered during signature
> >   verification. Should the module loading be allowed/disallowed. Looking
> >   at the module loading code, once it is determined that module has
> >   signature appended to it, module loading fails if some error occurs
> >   during signature verification.

There better not be any gray area, if CONFIG_MODULE_SIG_FORCE is
enabled, only validly signed modules should be loaded.

> > So I am just referring to that fact and trying to draw parallels between
> > error handling during module signature verification and error handling
> > when file appraisal happens in IMA. 

> > There can be two options.
> > 
> > - Disallow execution only if signature verification fails. If some error
> >   happens during verification, ignore it, let the executable continue.
> >   Just that it does not get extra capability.
> > 
> > - Disallow execution only if executable is not signed or it has valid
> >   signature. If executable is signed and some error happens during the
> >   process of verifying signature, execution is denied.
> > 
> 
> Little typo in second option. I meant "Allow execution only if executable
> is not signed or it has valid signatures".

Executables will be run with or without a valid signature.  The only
fair comparison would be between loading the kernel module and setting a
capability.  Both are only done based on a valid signature.  Of the two
options, I would choose the second.

Mimi

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Kasatkin, Dmitry]

On Wed, Feb 13, 2013 at 3:44 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Wed, 2013-02-13 at 15:13 +0200, Kasatkin, Dmitry wrote:
>> On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> > On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
>> >> On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >
>> >> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
>> >> >         }
>> >> >         switch (xattr_value->type) {
>> >> >         case IMA_XATTR_DIGEST:
>> >> > -               if (iint->flags & IMA_DIGSIG_REQUIRED) {
>> >> > +               if (iint->flags & IMA_DIGSIG_REQUIRED ||
>> >> > +                   iint->flags & IMA_DIGSIG_OPTIONAL) {
>> >> >                         cause = "IMA signature required";
>> >> >                         status = INTEGRITY_FAIL;
>> >> >                         break;
>> >>
>> >> This looks a bit odd... If "optional" signature is missing  - we fail..
>> >> It is optional... Why we should fail?
>> >
>> > 'imasig_optional' does not only mean that the signature is optional, but
>> > also implies that it has to be a digital signature, not a hash.  This
>> > latter part is what IMA_DIGSIG_REQUIRED means.
>> >
>> > If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
>> > 'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
>> >
>> > IMA_DIGSIG_REQUIRED would enforce that it is a signature.
>> > IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
>> >
>>
>> It sounds odd that optional signature is actually required.
>> Optional for me means that it may be there or may be not.
>> If it is not there, then it may be hash or nothing.
>>
>> I see it is more logical if it is "appraise_type=optional",
>> which means that we might have no xattr value, hash or signature.
>> It if happens to be a signature, then IMA_DIGSIG flag will be set.
>
> Right, 'appraise_type=' could have been defined either as a comma
> separated list of options (eg. appraise_type=imassig,optional) or, as
> Vivek implemented, a new option.  Eventually, we will need to extend
> 'appraise_type=' (eg. required algorithm), but for now I don't have a
> problem with the new option.
>

It is not exactly what I meant. IOW, I do not want
appraise_type=imasig,optional.

Optional for me is that xattr value is optional. It might be nothing,
hash or imasig...

If it would happen that it contains signature, then IMA_DIGSIG flag
would be set,
and process could get needed capability as Vivek wants.

- Dmitry


> thanks,
>
> Mimi
>
>> I asked Vivek to send a policy file he uses in his system.
>> I would like to see how system configured as a whole...
>>
>> - Dmitry
>
>

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Wed, Feb 13, 2013 at 07:33:13PM +0200, Kasatkin, Dmitry wrote:
> On Wed, Feb 13, 2013 at 3:44 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Wed, 2013-02-13 at 15:13 +0200, Kasatkin, Dmitry wrote:
> >> On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >> > On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
> >> >> On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> >
> >> >> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> >> >> >         }
> >> >> >         switch (xattr_value->type) {
> >> >> >         case IMA_XATTR_DIGEST:
> >> >> > -               if (iint->flags & IMA_DIGSIG_REQUIRED) {
> >> >> > +               if (iint->flags & IMA_DIGSIG_REQUIRED ||
> >> >> > +                   iint->flags & IMA_DIGSIG_OPTIONAL) {
> >> >> >                         cause = "IMA signature required";
> >> >> >                         status = INTEGRITY_FAIL;
> >> >> >                         break;
> >> >>
> >> >> This looks a bit odd... If "optional" signature is missing  - we fail..
> >> >> It is optional... Why we should fail?
> >> >
> >> > 'imasig_optional' does not only mean that the signature is optional, but
> >> > also implies that it has to be a digital signature, not a hash.  This
> >> > latter part is what IMA_DIGSIG_REQUIRED means.
> >> >
> >> > If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
> >> > 'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
> >> >
> >> > IMA_DIGSIG_REQUIRED would enforce that it is a signature.
> >> > IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
> >> >
> >>
> >> It sounds odd that optional signature is actually required.
> >> Optional for me means that it may be there or may be not.
> >> If it is not there, then it may be hash or nothing.
> >>
> >> I see it is more logical if it is "appraise_type=optional",
> >> which means that we might have no xattr value, hash or signature.
> >> It if happens to be a signature, then IMA_DIGSIG flag will be set.
> >
> > Right, 'appraise_type=' could have been defined either as a comma
> > separated list of options (eg. appraise_type=imassig,optional) or, as
> > Vivek implemented, a new option.  Eventually, we will need to extend
> > 'appraise_type=' (eg. required algorithm), but for now I don't have a
> > problem with the new option.
> >
> 
> It is not exactly what I meant. IOW, I do not want
> appraise_type=imasig,optional.
> 
> Optional for me is that xattr value is optional. It might be nothing,
> hash or imasig...
> 
> If it would happen that it contains signature, then IMA_DIGSIG flag
> would be set,
> and process could get needed capability as Vivek wants.

Hi Dmitry,

While we are at it, I thought I will mention what else is going in my
mind w.r.t next step.

I looked at your patch. That patch looks at IMG_DIGSIG
flag and sets bprm->unsafe with appropriate flag. It works well if 
signature verification before loading executable alone is sufficient.

But this leaves a small window open where somebody could write to the
disk block directly (bypass filesystem) after IMA signature verification.
Then modified image will be loaded by the binary handler.

To avoid that, I think I will need to do appraisal after loading the file
(with VM_LOCKED flag).

I guess I will need to introduce another hook say bprm_post_load() to
verify integrity of file.

So this raises few questions.

- Are two appraisals really necessary. From my use case perspective
  initially I just need to know whether digital signature are present
  or not and appraisal of file is not required. I guess it is one
  optimization area to keep appraisal overhead minimum in exec() path.

- When I go for second appraisal after loading file, current IMA code
  will have success result in iint->flags. I need to figure a way out
  to reset cache result and force reappraisal.

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Kasatkin, Dmitry]

On Wed, Feb 13, 2013 at 7:51 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Feb 13, 2013 at 07:33:13PM +0200, Kasatkin, Dmitry wrote:
>> On Wed, Feb 13, 2013 at 3:44 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> > On Wed, 2013-02-13 at 15:13 +0200, Kasatkin, Dmitry wrote:
>> >> On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> >> > On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
>> >> >> On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >> >
>> >> >> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
>> >> >> >         }
>> >> >> >         switch (xattr_value->type) {
>> >> >> >         case IMA_XATTR_DIGEST:
>> >> >> > -               if (iint->flags & IMA_DIGSIG_REQUIRED) {
>> >> >> > +               if (iint->flags & IMA_DIGSIG_REQUIRED ||
>> >> >> > +                   iint->flags & IMA_DIGSIG_OPTIONAL) {
>> >> >> >                         cause = "IMA signature required";
>> >> >> >                         status = INTEGRITY_FAIL;
>> >> >> >                         break;
>> >> >>
>> >> >> This looks a bit odd... If "optional" signature is missing  - we fail..
>> >> >> It is optional... Why we should fail?
>> >> >
>> >> > 'imasig_optional' does not only mean that the signature is optional, but
>> >> > also implies that it has to be a digital signature, not a hash.  This
>> >> > latter part is what IMA_DIGSIG_REQUIRED means.
>> >> >
>> >> > If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
>> >> > 'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
>> >> >
>> >> > IMA_DIGSIG_REQUIRED would enforce that it is a signature.
>> >> > IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
>> >> >
>> >>
>> >> It sounds odd that optional signature is actually required.
>> >> Optional for me means that it may be there or may be not.
>> >> If it is not there, then it may be hash or nothing.
>> >>
>> >> I see it is more logical if it is "appraise_type=optional",
>> >> which means that we might have no xattr value, hash or signature.
>> >> It if happens to be a signature, then IMA_DIGSIG flag will be set.
>> >
>> > Right, 'appraise_type=' could have been defined either as a comma
>> > separated list of options (eg. appraise_type=imassig,optional) or, as
>> > Vivek implemented, a new option.  Eventually, we will need to extend
>> > 'appraise_type=' (eg. required algorithm), but for now I don't have a
>> > problem with the new option.
>> >
>>
>> It is not exactly what I meant. IOW, I do not want
>> appraise_type=imasig,optional.
>>
>> Optional for me is that xattr value is optional. It might be nothing,
>> hash or imasig...
>>
>> If it would happen that it contains signature, then IMA_DIGSIG flag
>> would be set,
>> and process could get needed capability as Vivek wants.
>
> Hi Dmitry,
>
> While we are at it, I thought I will mention what else is going in my
> mind w.r.t next step.
>
> I looked at your patch. That patch looks at IMG_DIGSIG
> flag and sets bprm->unsafe with appropriate flag. It works well if
> signature verification before loading executable alone is sufficient.
>
> But this leaves a small window open where somebody could write to the
> disk block directly (bypass filesystem) after IMA signature verification.
> Then modified image will be loaded by the binary handler.
>
> To avoid that, I think I will need to do appraisal after loading the file
> (with VM_LOCKED flag).
>
> I guess I will need to introduce another hook say bprm_post_load() to
> verify integrity of file.
>
> So this raises few questions.
>
> - Are two appraisals really necessary. From my use case perspective
>   initially I just need to know whether digital signature are present
>   or not and appraisal of file is not required. I guess it is one
>   optimization area to keep appraisal overhead minimum in exec() path.
>
> - When I go for second appraisal after loading file, current IMA code
>   will have success result in iint->flags. I need to figure a way out
>   to reset cache result and force reappraisal.
>
> Thanks
> Vivek

Hello,

My patch was just a 5 minute example how to go with that.
Sure, when we deal with MAP_LOCKED, we might do appraisal after
loading the binary.
Yes, it looks like that we really need another hook for that...

But then process_measurement might do things a bit differently.
It might check the policy, allocated iint and return if locking is
needed (or another hook will be called).
Next hook will just collect and appriase...

- Dmitry

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Wed, 2013-02-13 at 19:33 +0200, Kasatkin, Dmitry wrote:
> On Wed, Feb 13, 2013 at 3:44 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Wed, 2013-02-13 at 15:13 +0200, Kasatkin, Dmitry wrote:
> >> On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >> > On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
> >> >> On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> >
> >> >> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> >> >> >         }
> >> >> >         switch (xattr_value->type) {
> >> >> >         case IMA_XATTR_DIGEST:
> >> >> > -               if (iint->flags & IMA_DIGSIG_REQUIRED) {
> >> >> > +               if (iint->flags & IMA_DIGSIG_REQUIRED ||
> >> >> > +                   iint->flags & IMA_DIGSIG_OPTIONAL) {
> >> >> >                         cause = "IMA signature required";
> >> >> >                         status = INTEGRITY_FAIL;
> >> >> >                         break;
> >> >>
> >> >> This looks a bit odd... If "optional" signature is missing  - we fail..
> >> >> It is optional... Why we should fail?
> >> >
> >> > 'imasig_optional' does not only mean that the signature is optional, but
> >> > also implies that it has to be a digital signature, not a hash.  This
> >> > latter part is what IMA_DIGSIG_REQUIRED means.
> >> >
> >> > If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
> >> > 'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
> >> >
> >> > IMA_DIGSIG_REQUIRED would enforce that it is a signature.
> >> > IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
> >> >
> >>
> >> It sounds odd that optional signature is actually required.
> >> Optional for me means that it may be there or may be not.
> >> If it is not there, then it may be hash or nothing.
> >>
> >> I see it is more logical if it is "appraise_type=optional",
> >> which means that we might have no xattr value, hash or signature.
> >> It if happens to be a signature, then IMA_DIGSIG flag will be set.
> >
> > Right, 'appraise_type=' could have been defined either as a comma
> > separated list of options (eg. appraise_type=imassig,optional) or, as
> > Vivek implemented, a new option.  Eventually, we will need to extend
> > 'appraise_type=' (eg. required algorithm), but for now I don't have a
> > problem with the new option.
> >
> 
> It is not exactly what I meant. IOW, I do not want
> appraise_type=imasig,optional.
> 
> Optional for me is that xattr value is optional. It might be nothing,
> hash or imasig...

To summarize:

Option 1: appraise_type:= [[imasig] | [imahash]]  [optional]

sample rules:
# require either hash or signature
func=bprm
 
# require signature
func=bprm appraise_type=imasig

# permit nothing, hash, or signature
func=bprm appraise_type=optional

# permit nothing or hash
func=bprm appraise_type=imahash, optional

# permit nothing or signature
func=bprm appraise_type=imasig, optional


Option 2: appraise_type:= [imasig] | [imasig_optional]

As I don't see a use case for either just 'optional' or 'imahash,
optional', this leaves 'imasig,optional', which could be expressed as
'imasig_optional'.


Option 3: appraise_type:= [imasig] | [imahash] | [optional]

Dmitry is recommending this syntax, as IMA_DIGSIG will be set in the
iint flags.


Any of these options should work.

> If it would happen that it contains signature, then IMA_DIGSIG flag
> would be set,
> and process could get needed capability as Vivek wants.

With the 'optional' condition, both unsigned and validly signed files
will succeed.  One way of making this information accessible to an LSM,
would be to define a new integrity capability and set it here.  The new
integrity capability would indicate the file was validly signed. 

thanks,

Mimi

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Vivek Goyal]

On Wed, Feb 13, 2013 at 04:45:23PM -0500, Mimi Zohar wrote:

[..]
> Option 3: appraise_type:= [imasig] | [imahash] | [optional]
> 
> Dmitry is recommending this syntax, as IMA_DIGSIG will be set in the
> iint flags.

I like option 3. If there is a use case down the line where definition
of optional needs to be refined in such a way that we want to force
signature or hash then we can extend appraise_type to also include
following.

Option 3: appraise_type:= [imasig] | [imahash] | [optional] |
				[imasig_optional] | [imahash_optional]

> 
> 
> Any of these options should work.
> 
> > If it would happen that it contains signature, then IMA_DIGSIG flag
> > would be set,
> > and process could get needed capability as Vivek wants.
> 
> With the 'optional' condition, both unsigned and validly signed files
> will succeed.  One way of making this information accessible to an LSM,
> would be to define a new integrity capability and set it here.  The new
> integrity capability would indicate the file was validly signed. 

Thinking loud.

The problem with integrity capability is that it goes only so far. If
we provide capability in exec() path, then that capability means much
more in the sense, we know file is locked to run from memory. An integrity
capability just means file is validly signed.

So exec() code might have to do another capability on top which will
also ensure that file is executable is locked in memory and signature
verification is done after loading in memory so that it is not open
to writing to disk block attacks.

And based on this capability we probably need to deny write access to file
till file is open for exec() (I noticed that after load, we seem to be
allowing access to write access).

Thanks
Vivek

Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar]

On Thu, 2013-02-14 at 09:40 -0500, Vivek Goyal wrote:
> On Wed, Feb 13, 2013 at 04:45:23PM -0500, Mimi Zohar wrote:
> 
[..]

> > > If it would happen that it contains signature, then IMA_DIGSIG flag
> > > would be set,
> > > and process could get needed capability as Vivek wants.
> > 
> > With the 'optional' condition, both unsigned and validly signed files
> > will succeed.  One way of making this information accessible to an LSM,
> > would be to define a new integrity capability and set it here.  The new
> > integrity capability would indicate the file was validly signed. 
> 
> Thinking loud.
> 
> The problem with integrity capability is that it goes only so far. If
> we provide capability in exec() path, then that capability means much
> more in the sense, we know file is locked to run from memory. An integrity
> capability just means file is validly signed.

> So exec() code might have to do another capability on top which will
> also ensure that file is executable is locked in memory and signature
> verification is done after loading in memory so that it is not open
> to writing to disk block attacks.
> 
> And based on this capability we probably need to deny write access to file
> till file is open for exec() (I noticed that after load, we seem to be
> allowing access to write access).

I think we're back to my original comment that the bprm_check hook might
need to be moved or an additional hook added, as the existing bprm_check
hook is located before the file is locked from modification.  :)

thanks,

Mimi