Messages in this thread | | | Subject | Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional | From | Mimi Zohar <> | Date | Mon, 11 Feb 2013 17:10:14 -0500 |
| |
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)
| |