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 <vgo...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
