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 <[email protected]>
> ---
> 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
On Tue, Mar 5, 2013 at 9:13 PM, Vivek Goyal <[email protected]> wrote:
> 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 <[email protected]>
>> ---
>> 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.
>
Hi,
I think there is no need to store optional in iint->flags.
Just test "action" which is returned by ima_get_action().
Then it will be rule specific...
- Dmitry
> 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