On Thu, Feb 14, 2013 at 02:55:44PM -0500, Vivek Goyal wrote:
> Currently ima appraises all the files as specified by the rule. So
> if one wants to create a system where only few executables are
> signed, that system will not work with IMA.
>
> With secureboot, one needs to disable kexec so that unsigned kernels
> can't be booted. To avoid this problem, it was proposed that sign
> /sbin/kexec binary and if signatures are verified successfully, give
> an special capability to the /sbin/kexec process. And in secureboot
> mode processes with that special capability can invoke sys_kexec()
> system call.
>
> So there is a need for IMA to allow appraising only signed binaries.
> Unsigned binaries will pass the appraisal too, but will not get the
> special capability. (Capability patches for that are yet to be written).
>
> This patch adds new option, appraise_type=imasig_optional to 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.
>
> Signed-off-by: Vivek Goyal <vgo...@redhat.com>
> ---
> Documentation/ABI/testing/ima_policy | 2 +-
> security/integrity/ima/ima_main.c | 14 ++++++++++++--
> security/integrity/ima/ima_policy.c | 2 ++
> security/integrity/integrity.h | 1 +
> 4 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy
> b/Documentation/ABI/testing/ima_policy
> index de16de3..cc69872 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] | [optional]
>
> default policy:
> # PROC_SUPER_MAGIC
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 3e751a9..da9e348 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -207,8 +207,18 @@ out_digsig:
> rc = -EACCES;
> out:
> mutex_unlock(&inode->i_mutex);
> - if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
> - return -EACCES;
> + if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> + /*
> + * If IMA_APPRAISAL_OPT is set, then access is allowed
> + * even if hash or digital signatures are not present.
> + */
> + if ((iint->flags & IMA_APPRAISAL_OPT) &&
> + (rc == INTEGRITY_XATTR_NOTSUPP ||
> + rc == INTEGRITY_IMA_NOLABEL))
> + return 0;
> + else
> + return -EACCES;
I think there is problem here. "appraise_type=optional" can be specified
per rule/hook. So two different hooks can specify two different rules.
appraise func=MMAP_CHECK appraise_type=optional
appraise func=BPRM_CHECK
I think if a file is first mmaped(), then appraisal will take place and
IMA_APPRAISAL_OPT will be set in iint->flags.
Later when BPRM_CHECK hook gets executed, and it will return success
based on IMA_APPRAISAL_OPT even if there was no label. And that's not
what exec() expects.
So storing IMA_APPRAISAL_OPT in iint->flags seems wrong (espectially as
part of IMA_ACTION_FLAGS bits). I think only bits which are valid
across all rules/hooks should be stored here.
Any property which is hook/rule specific should either not be stored
or should be stroed in hook specific property area.
We don't have enough space to store more hook specific properties, so
I will explore the option of passing around this flag when hook is being
executed and then discard it.
Thanks
Vivek
> + }
> return 0;
> }
>
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 4adcd0f..fd92dc3d4 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, "optional")) == 0)
> + entry->flags |= IMA_APPRAISAL_OPT;
> else
> result = -EINVAL;
> break;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 0ae08fc..4d330a7 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -29,6 +29,7 @@
> #define IMA_ACTION_FLAGS 0xff000000
> #define IMA_DIGSIG 0x01000000
> #define IMA_DIGSIG_REQUIRED 0x02000000
> +#define IMA_APPRAISAL_OPT 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 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/
