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.
- 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)
--
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/
