| Message ID | 20210728132112.258606-4-simon.thoby@viveris.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IMA: restrict the accepted digest algorithms for the security.ima xattr | expand |
Hi Simon, On 7/28/2021 6:21 AM, THOBY Simon wrote: > The kernel accepts any hash algorithm as a value for the security.ima > xattr. Users may wish to restrict the accepted algorithms to only > support strong cryptographic ones. > > Provide the plumbing to restrict the permitted set of hash algorithms > used for verifying file hashes and digest algorithms stored in > security.ima xattr. > > This do not apply only to IMA in hash mode, it also works with digital > signatures, in which case it checks that the hash (which was then > signed by the trusted private key) have been generated with one of > the algortihms whitelisted for this specific rule. typo: algortihms => algorithms Also, suggest using "allowed list" (for digest algorithms allowed) > > Signed-off-by: Simon Thoby <simon.thoby@viveris.fr> > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> > --- > security/integrity/ima/ima.h | 6 +++--- > security/integrity/ima/ima_api.c | 6 ++++-- > security/integrity/ima/ima_appraise.c | 5 +++-- > security/integrity/ima/ima_main.c | 17 ++++++++++++++--- > security/integrity/ima/ima_policy.c | 18 ++++++++++++++++-- > 5 files changed, 40 insertions(+), 12 deletions(-) > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index f0e448ed1f9f..7ef1b214d358 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -47,7 +47,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 }; > extern int ima_policy_flag; > > /* set during initialization */ > -extern int ima_hash_algo; > +extern int ima_hash_algo __ro_after_init; > extern int ima_sha1_idx __ro_after_init; > extern int ima_hash_algo_idx __ro_after_init; > extern int ima_extra_slots __ro_after_init; > @@ -254,7 +254,7 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode, > const struct cred *cred, u32 secid, int mask, > enum ima_hooks func, int *pcr, > struct ima_template_desc **template_desc, > - const char *func_data); > + const char *func_data, unsigned int *allowed_hashes); > int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func); > int ima_collect_measurement(struct integrity_iint_cache *iint, > struct file *file, void *buf, loff_t size, > @@ -285,7 +285,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, > const struct cred *cred, u32 secid, enum ima_hooks func, > int mask, int flags, int *pcr, > struct ima_template_desc **template_desc, > - const char *func_data); > + const char *func_data, unsigned int *allowed_hashes); > void ima_init_policy(void); > void ima_update_policy(void); > void ima_update_policy_flag(void); > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c > index d8e321cc6936..c91c2c402498 100644 > --- a/security/integrity/ima/ima_api.c > +++ b/security/integrity/ima/ima_api.c > @@ -172,6 +172,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename, > * @pcr: pointer filled in if matched measure policy sets pcr= > * @template_desc: pointer filled in if matched measure policy sets template= > * @func_data: func specific data, may be NULL > + * @allowed_hashes: whitelist of hash algorithms allowed for the IMA xattr "@allowed_hashes: allowed list of hash algorithms for the IMA xattr" > * > * The policy is defined in terms of keypairs: > * subj=, obj=, type=, func=, mask=, fsmagic= > @@ -188,14 +189,15 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode, > const struct cred *cred, u32 secid, int mask, > enum ima_hooks func, int *pcr, > struct ima_template_desc **template_desc, > - const char *func_data) > + const char *func_data, unsigned int *allowed_hashes) > { > int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH; > > flags &= ima_policy_flag; > > return ima_match_policy(mnt_userns, inode, cred, secid, func, mask, > - flags, pcr, template_desc, func_data); > + flags, pcr, template_desc, func_data, > + allowed_hashes); > } > > /* > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index a5e0d3400bd1..12d406b5ab35 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -77,8 +77,9 @@ int ima_must_appraise(struct user_namespace *mnt_userns, struct inode *inode, > return 0; > > security_task_getsecid_subj(current, &secid); > - return ima_match_policy(mnt_userns, inode, current_cred(), secid, func, > - mask, IMA_APPRAISE | IMA_HASH, NULL, NULL, NULL); > + return ima_match_policy(mnt_userns, inode, current_cred(), secid, > + func, mask, IMA_APPRAISE | IMA_HASH, NULL, > + NULL, NULL, NULL); > } > > static int ima_fix_xattr(struct dentry *dentry, > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 7f2310f29789..85b079c1a19b 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -210,6 +210,7 @@ static int process_measurement(struct file *file, const struct cred *cred, > int xattr_len = 0; > bool violation_check; > enum hash_algo hash_algo; > + unsigned int allowed_hashes = 0; > > if (!ima_policy_flag || !S_ISREG(inode->i_mode)) > return 0; > @@ -219,7 +220,8 @@ static int process_measurement(struct file *file, const struct cred *cred, > * Included is the appraise submask. > */ > action = ima_get_action(file_mnt_user_ns(file), inode, cred, secid, > - mask, func, &pcr, &template_desc, NULL); > + mask, func, &pcr, &template_desc, NULL, > + &allowed_hashes); > violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) && > (ima_policy_flag & IMA_MEASURE)); > if (!action && !violation_check) > @@ -356,6 +358,15 @@ static int process_measurement(struct file *file, const struct cred *cred, > > if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO)) > rc = 0; > + > + /* Ensure the digest was generated using an allowed algorithm */ > + if (rc == 0 && must_appraise && allowed_hashes != 0 && > + (allowed_hashes & (1U << hash_algo)) == 0) { > + rc = -EACCES; > + > + integrity_audit_msg(AUDIT_INTEGRITY_DATA, file_inode(file), > + pathname, "collect_data", "forbidden-hash-algorithm", rc, 0); > + } > out_locked: > if ((mask & MAY_WRITE) && test_bit(IMA_DIGSIG, &iint->atomic_flags) && > !(iint->flags & IMA_NEW_FILE)) > @@ -433,7 +444,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) > inode = file_inode(vma->vm_file); > action = ima_get_action(file_mnt_user_ns(vma->vm_file), inode, > current_cred(), secid, MAY_EXEC, MMAP_CHECK, > - &pcr, &template, NULL); > + &pcr, &template, NULL, NULL); > > /* Is the mmap'ed file in policy? */ > if (!(action & (IMA_MEASURE | IMA_APPRAISE_SUBMASK))) > @@ -882,7 +893,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns, > security_task_getsecid_subj(current, &secid); > action = ima_get_action(mnt_userns, inode, current_cred(), > secid, 0, func, &pcr, &template, > - func_data); > + func_data, NULL); > if (!(action & IMA_MEASURE)) > return; > } > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index fd5d46e511f1..344b5b0dc1a1 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -35,6 +35,7 @@ > #define IMA_FSNAME 0x0200 > #define IMA_KEYRINGS 0x0400 > #define IMA_LABEL 0x0800 > +#define IMA_VALIDATE_HASH 0x1000 > > #define UNKNOWN 0 > #define MEASURE 0x0001 /* same as IMA_MEASURE */ > @@ -79,6 +80,7 @@ struct ima_rule_entry { > bool (*uid_op)(kuid_t, kuid_t); /* Handlers for operators */ > bool (*fowner_op)(kuid_t, kuid_t); /* uid_eq(), uid_gt(), uid_lt() */ > int pcr; > + unsigned int allowed_hashes; > struct { > void *rule; /* LSM file metadata specific */ > char *args_p; /* audit value */ > @@ -90,6 +92,14 @@ struct ima_rule_entry { > struct ima_template_desc *template; > }; > > +/* > + * sanity check in case the kernels gains more hash algorithms that can > + * fit in an unsigned int > + */ > +static_assert( > + 8 * sizeof(unsigned int) >= HASH_ALGO__LAST, > + "The bitfield allowed_hashes in ima_rule_entry is too small to contain all the supported hash algorithms, consider using a bigger type"); > + > /* > * Without LSM specific knowledge, the default policy can only be > * written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner > @@ -646,6 +656,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func) > * @pcr: set the pcr to extend > * @template_desc: the template that should be used for this rule > * @func_data: func specific data, may be NULL > + * @allowed_hashes: whitelist of hash algorithms allowed for the IMA xattr "@allowed_hashes: allowed list of hash algorithms for the IMA xattr" -lakshmi > * > * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type) > * conditions. > @@ -658,7 +669,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, > const struct cred *cred, u32 secid, enum ima_hooks func, > int mask, int flags, int *pcr, > struct ima_template_desc **template_desc, > - const char *func_data) > + const char *func_data, unsigned int *allowed_hashes) > { > struct ima_rule_entry *entry; > int action = 0, actmask = flags | (flags << 1); > @@ -684,8 +695,11 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, > action &= ~IMA_HASH; > if (ima_fail_unverifiable_sigs) > action |= IMA_FAIL_UNVERIFIABLE_SIGS; > - } > > + if (allowed_hashes && > + entry->flags & IMA_VALIDATE_HASH) > + *allowed_hashes = entry->allowed_hashes; > + } > > if (entry->action & IMA_DO_MASK) > actmask &= ~(entry->action | entry->action << 1); >
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index f0e448ed1f9f..7ef1b214d358 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -47,7 +47,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 }; extern int ima_policy_flag; /* set during initialization */ -extern int ima_hash_algo; +extern int ima_hash_algo __ro_after_init; extern int ima_sha1_idx __ro_after_init; extern int ima_hash_algo_idx __ro_after_init; extern int ima_extra_slots __ro_after_init; @@ -254,7 +254,7 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode, const struct cred *cred, u32 secid, int mask, enum ima_hooks func, int *pcr, struct ima_template_desc **template_desc, - const char *func_data); + const char *func_data, unsigned int *allowed_hashes); int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func); int ima_collect_measurement(struct integrity_iint_cache *iint, struct file *file, void *buf, loff_t size, @@ -285,7 +285,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, const struct cred *cred, u32 secid, enum ima_hooks func, int mask, int flags, int *pcr, struct ima_template_desc **template_desc, - const char *func_data); + const char *func_data, unsigned int *allowed_hashes); void ima_init_policy(void); void ima_update_policy(void); void ima_update_policy_flag(void); diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index d8e321cc6936..c91c2c402498 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -172,6 +172,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename, * @pcr: pointer filled in if matched measure policy sets pcr= * @template_desc: pointer filled in if matched measure policy sets template= * @func_data: func specific data, may be NULL + * @allowed_hashes: whitelist of hash algorithms allowed for the IMA xattr * * The policy is defined in terms of keypairs: * subj=, obj=, type=, func=, mask=, fsmagic= @@ -188,14 +189,15 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode, const struct cred *cred, u32 secid, int mask, enum ima_hooks func, int *pcr, struct ima_template_desc **template_desc, - const char *func_data) + const char *func_data, unsigned int *allowed_hashes) { int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH; flags &= ima_policy_flag; return ima_match_policy(mnt_userns, inode, cred, secid, func, mask, - flags, pcr, template_desc, func_data); + flags, pcr, template_desc, func_data, + allowed_hashes); } /* diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index a5e0d3400bd1..12d406b5ab35 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -77,8 +77,9 @@ int ima_must_appraise(struct user_namespace *mnt_userns, struct inode *inode, return 0; security_task_getsecid_subj(current, &secid); - return ima_match_policy(mnt_userns, inode, current_cred(), secid, func, - mask, IMA_APPRAISE | IMA_HASH, NULL, NULL, NULL); + return ima_match_policy(mnt_userns, inode, current_cred(), secid, + func, mask, IMA_APPRAISE | IMA_HASH, NULL, + NULL, NULL, NULL); } static int ima_fix_xattr(struct dentry *dentry, diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 7f2310f29789..85b079c1a19b 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -210,6 +210,7 @@ static int process_measurement(struct file *file, const struct cred *cred, int xattr_len = 0; bool violation_check; enum hash_algo hash_algo; + unsigned int allowed_hashes = 0; if (!ima_policy_flag || !S_ISREG(inode->i_mode)) return 0; @@ -219,7 +220,8 @@ static int process_measurement(struct file *file, const struct cred *cred, * Included is the appraise submask. */ action = ima_get_action(file_mnt_user_ns(file), inode, cred, secid, - mask, func, &pcr, &template_desc, NULL); + mask, func, &pcr, &template_desc, NULL, + &allowed_hashes); violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) && (ima_policy_flag & IMA_MEASURE)); if (!action && !violation_check) @@ -356,6 +358,15 @@ static int process_measurement(struct file *file, const struct cred *cred, if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO)) rc = 0; + + /* Ensure the digest was generated using an allowed algorithm */ + if (rc == 0 && must_appraise && allowed_hashes != 0 && + (allowed_hashes & (1U << hash_algo)) == 0) { + rc = -EACCES; + + integrity_audit_msg(AUDIT_INTEGRITY_DATA, file_inode(file), + pathname, "collect_data", "forbidden-hash-algorithm", rc, 0); + } out_locked: if ((mask & MAY_WRITE) && test_bit(IMA_DIGSIG, &iint->atomic_flags) && !(iint->flags & IMA_NEW_FILE)) @@ -433,7 +444,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) inode = file_inode(vma->vm_file); action = ima_get_action(file_mnt_user_ns(vma->vm_file), inode, current_cred(), secid, MAY_EXEC, MMAP_CHECK, - &pcr, &template, NULL); + &pcr, &template, NULL, NULL); /* Is the mmap'ed file in policy? */ if (!(action & (IMA_MEASURE | IMA_APPRAISE_SUBMASK))) @@ -882,7 +893,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns, security_task_getsecid_subj(current, &secid); action = ima_get_action(mnt_userns, inode, current_cred(), secid, 0, func, &pcr, &template, - func_data); + func_data, NULL); if (!(action & IMA_MEASURE)) return; } diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index fd5d46e511f1..344b5b0dc1a1 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -35,6 +35,7 @@ #define IMA_FSNAME 0x0200 #define IMA_KEYRINGS 0x0400 #define IMA_LABEL 0x0800 +#define IMA_VALIDATE_HASH 0x1000 #define UNKNOWN 0 #define MEASURE 0x0001 /* same as IMA_MEASURE */ @@ -79,6 +80,7 @@ struct ima_rule_entry { bool (*uid_op)(kuid_t, kuid_t); /* Handlers for operators */ bool (*fowner_op)(kuid_t, kuid_t); /* uid_eq(), uid_gt(), uid_lt() */ int pcr; + unsigned int allowed_hashes; struct { void *rule; /* LSM file metadata specific */ char *args_p; /* audit value */ @@ -90,6 +92,14 @@ struct ima_rule_entry { struct ima_template_desc *template; }; +/* + * sanity check in case the kernels gains more hash algorithms that can + * fit in an unsigned int + */ +static_assert( + 8 * sizeof(unsigned int) >= HASH_ALGO__LAST, + "The bitfield allowed_hashes in ima_rule_entry is too small to contain all the supported hash algorithms, consider using a bigger type"); + /* * Without LSM specific knowledge, the default policy can only be * written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner @@ -646,6 +656,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func) * @pcr: set the pcr to extend * @template_desc: the template that should be used for this rule * @func_data: func specific data, may be NULL + * @allowed_hashes: whitelist of hash algorithms allowed for the IMA xattr * * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type) * conditions. @@ -658,7 +669,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, const struct cred *cred, u32 secid, enum ima_hooks func, int mask, int flags, int *pcr, struct ima_template_desc **template_desc, - const char *func_data) + const char *func_data, unsigned int *allowed_hashes) { struct ima_rule_entry *entry; int action = 0, actmask = flags | (flags << 1); @@ -684,8 +695,11 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, action &= ~IMA_HASH; if (ima_fail_unverifiable_sigs) action |= IMA_FAIL_UNVERIFIABLE_SIGS; - } + if (allowed_hashes && + entry->flags & IMA_VALIDATE_HASH) + *allowed_hashes = entry->allowed_hashes; + } if (entry->action & IMA_DO_MASK) actmask &= ~(entry->action | entry->action << 1);