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