On Tue, Mar 5, 2013 at 9:13 PM, Vivek Goyal <vgo...@redhat.com> 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 <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. >
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 -- 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/