| Message ID | 20210728132112.258606-6-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 Wed, 2021-07-28 at 13:21 +0000, THOBY Simon wrote: > @@ -914,6 +918,42 @@ int ima_check_policy(void) > return 0; > } > > +/** update_allowed_hash_algorithms - update the hash algorithms allowed The first line of kernel-doc is just "/**" by itself, followed by the function name and a brief description. The brief description should not wrap to the next line. Refer to Documentation/doc-guide/kernel- doc.rst. > + * for setxattr writes > + * > + * Update the atomic variable holding the set of allowed hash algorithms > + * that can be used to update the security.ima xattr of a file. > + * > + * Context: called when updating the IMA policy. > + * > + * SETXATTR_CHECK rules do not implement a full policy check because of > + * the performance impact performing rules checking on setxattr() would > + * have. The consequence is that only one SETXATTR_CHECK can be active at > + * a time. > + */ > +static void update_allowed_hash_algorithms(void) > +{ > + struct ima_rule_entry *entry; > + > + /* > + * We scan in reverse order because only the last entry with the > + * 'func=SETXATTR_CHECK' apply: this allows runtime upgrades of the > + * digest algorithm policy, unlike the other IMA rules that are > + * usually append-only. Old rules will still be present in the > + * ruleset, but inactive. > + */ Oh, my! I really hope this won't be used as precedent. Before agreeing to this, the existing policy rules must require loading of only signed IMA policies. thanks, Mimi > + rcu_read_lock(); > + list_for_each_entry_reverse(entry, ima_rules, list) { > + if (entry->func != SETXATTR_CHECK) > + continue; > + > + atomic_xchg(&ima_setxattr_allowed_hash_algorithms, > + entry->allowed_hashes); > + break; > + } > + rcu_read_unlock(); > +} > +
Hi Mimi, On 7/29/21 12:57 AM, Mimi Zohar wrote: > Hi Simon, > > On Wed, 2021-07-28 at 13:21 +0000, THOBY Simon wrote: > >> @@ -914,6 +918,42 @@ int ima_check_policy(void) >> return 0; >> } >> >> +/** update_allowed_hash_algorithms - update the hash algorithms allowed > > The first line of kernel-doc is just "/**" by itself, followed by the > function name and a brief description. The brief description should > not wrap to the next line. Refer to Documentation/doc-guide/kernel- > doc.rst. > Thanks, I will fix that in the next revision. >> + * for setxattr writes >> + * >> + * Update the atomic variable holding the set of allowed hash algorithms >> + * that can be used to update the security.ima xattr of a file. >> + * >> + * Context: called when updating the IMA policy. >> + * >> + * SETXATTR_CHECK rules do not implement a full policy check because of >> + * the performance impact performing rules checking on setxattr() would >> + * have. The consequence is that only one SETXATTR_CHECK can be active at >> + * a time. >> + */ >> +static void update_allowed_hash_algorithms(void) >> +{ >> + struct ima_rule_entry *entry; >> + >> + /* >> + * We scan in reverse order because only the last entry with the >> + * 'func=SETXATTR_CHECK' apply: this allows runtime upgrades of the >> + * digest algorithm policy, unlike the other IMA rules that are >> + * usually append-only. Old rules will still be present in the >> + * ruleset, but inactive. >> + */ > > Oh, my! I really hope this won't be used as precedent. Before > agreeing to this, the existing policy rules must require loading of > only signed IMA policies. > After some though, I think you're right, there is not much point to do anything different from the other IMA rules, Below is the modified version that I will submit in the next patch. However given the similarities between this function and ima_update_policy_flag, I think maybe I should merge them together: they are mostly called from the same places and they both serve the same role: updating some of the global ima variables after a policy update or at system initialization. Do you think it would be ok to add that functionality to ima_update_policy_flag? Maybe updating the name to reflect that the function updates multiples flags? +/** + * update_allowed_hash_algorithms() - update the hash allowlist for setxattr + * + * Update the atomic variable holding the set of allowed hash algorithms + * that can be used to update the security.ima xattr of a file. + * + * SETXATTR_CHECK rules do not implement a full policy check because + * of the performance impact performing rules checking on setxattr() + * would have. The consequence is that only one SETXATTR_CHECK can be + * active at a given time. + * + * Context: Called when updating the IMA policy. + */ +static void update_allowed_hash_algorithms(void) +{ + struct ima_rule_entry *entry; + + rcu_read_lock(); + list_for_each_entry(entry, ima_rules, list) { + if (entry->func != SETXATTR_CHECK) + continue; + + /* + * Two possibilities: + * - the atomic was non-zero: a setxattr hash policy is + * already enforced -> do nothing + * - the atomic was zero -> enable the setxattr hash policy + * + * While we could check if the atomic is non-zero at the + * beginning of the function, doing it here prevent TOCTOU + * races (not that I think there would be much interest for + * an attacker to perform a TOCTOU race here) + */ + atomic_cmpxchg(&ima_setxattr_allowed_hash_algorithms, 0, + entry->allowed_hashes); + break; + } + rcu_read_unlock(); +} + As a side note on the patchset, maybe I should refrain from posting for a few days to give people time to comment on it, instead of sending new versions in such a quick succession? Thanks, Simon
Hi Simon, On Thu, 2021-07-29 at 07:47 +0000, THOBY Simon wrote: > > >> + * for setxattr writes > >> + * > >> + * Update the atomic variable holding the set of allowed hash algorithms > >> + * that can be used to update the security.ima xattr of a file. > >> + * > >> + * Context: called when updating the IMA policy. > >> + * > >> + * SETXATTR_CHECK rules do not implement a full policy check because of > >> + * the performance impact performing rules checking on setxattr() would > >> + * have. The consequence is that only one SETXATTR_CHECK can be active at > >> + * a time. > >> + */ > >> +static void update_allowed_hash_algorithms(void) > >> +{ > >> + struct ima_rule_entry *entry; > >> + > >> + /* > >> + * We scan in reverse order because only the last entry with the > >> + * 'func=SETXATTR_CHECK' apply: this allows runtime upgrades of the > >> + * digest algorithm policy, unlike the other IMA rules that are > >> + * usually append-only. Old rules will still be present in the > >> + * ruleset, but inactive. > >> + */ > > > > Oh, my! I really hope this won't be used as precedent. Before > > agreeing to this, the existing policy rules must require loading of > > only signed IMA policies. > > > > > After some though, I think you're right, there is not much point to do anything > different from the other IMA rules, > > Below is the modified version that I will submit in the next patch. > > However given the similarities between this function and ima_update_policy_flag, > I think maybe I should merge them together: they are mostly called from the > same places and they both serve the same role: updating some of the global ima > variables after a policy update or at system initialization. > > Do you think it would be ok to add that functionality to ima_update_policy_flag? > Maybe updating the name to reflect that the function updates multiples flags? We've gone through a couple of iterations of this patch. At this point, it's defining a single set of setxattr permitted hash algorithms. Renaming and adding the change to ima_update_policy_flags() definitely makes sense. At the same time, I'd appreciate your fixing the RCU locking there. > As a side note on the patchset, maybe I should refrain from posting for a few days to give people time > to comment on it, instead of sending new versions in such a quick succession? Definitely. thanks, Mimi
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy index aeb622698047..537be0e1720e 100644 --- a/Documentation/ABI/testing/ima_policy +++ b/Documentation/ABI/testing/ima_policy @@ -30,9 +30,10 @@ Description: [appraise_flag=] [appraise_hash=] [keyrings=] base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK] - [FIRMWARE_CHECK] + [FIRMWARE_CHECK] [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK] [KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA] + [SETXATTR_CHECK] mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND] [[^]MAY_EXEC] fsmagic:= hex value @@ -138,3 +139,9 @@ Description: keys added to .builtin_trusted_keys or .ima keyring: measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima + + Example of the special SETXATTR_CHECK appraise rule, that + restricts the hash algorithms allowed when writing to the + security.ima xattr of a file: + + appraise func=SETXATTR_CHECK appraise_hash=sha256,sha384,sha512 diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 7ef1b214d358..aeb3bf30c0f9 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -46,6 +46,9 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 }; /* current content of the policy */ extern int ima_policy_flag; +/* bitset of digests algorithms allowed in the setxattr hook */ +extern atomic_t ima_setxattr_allowed_hash_algorithms; + /* set during initialization */ extern int ima_hash_algo __ro_after_init; extern int ima_sha1_idx __ro_after_init; @@ -198,6 +201,7 @@ static inline unsigned int ima_hash_key(u8 *digest) hook(KEXEC_CMDLINE, kexec_cmdline) \ hook(KEY_CHECK, key) \ hook(CRITICAL_DATA, critical_data) \ + hook(SETXATTR_CHECK, setxattr_check) \ hook(MAX_CHECK, none) #define __ima_hook_enumify(ENUM, str) ENUM, diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 12d406b5ab35..c4a43c84da5f 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -596,13 +596,33 @@ static int validate_hash_algo(struct dentry *dentry, { char *path = NULL, *pathbuf = NULL; enum hash_algo xattr_hash_algo; + const char *errmsg = "unavailable-hash-algorithm"; + unsigned int allowed_hashes; xattr_hash_algo = ima_get_hash_algo((struct evm_ima_xattr_data *)xattr_value, xattr_value_len); - if (likely(xattr_hash_algo == ima_hash_algo || - crypto_has_alg(hash_algo_name[xattr_hash_algo], 0, 0))) - return 0; + allowed_hashes = atomic_read(&ima_setxattr_allowed_hash_algorithms); + + if (allowed_hashes) { + /* success if the algorithm is whitelisted in the ima policy */ + if (allowed_hashes & (1U << xattr_hash_algo)) + return 0; + + /* + * We use a different audit message when the hash algorithm + * is denied by a policy rule, instead of not being built + * in the kernel image + */ + errmsg = "denied-hash-algorithm"; + } else { + if (likely(xattr_hash_algo == ima_hash_algo)) + return 0; + + /* allow any xattr using an algorithm built in the kernel */ + if (crypto_has_alg(hash_algo_name[xattr_hash_algo], 0, 0)) + return 0; + } pathbuf = kmalloc(PATH_MAX, GFP_KERNEL); if (pathbuf) { @@ -610,11 +630,10 @@ static int validate_hash_algo(struct dentry *dentry, /* * disallow xattr writes with algorithms disabled in the - * kernel configuration + * kernel configuration or denied by policy */ integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry), - path, "collect_data", - "unavailable-hash-algorithm", + path, "collect_data", errmsg, -EACCES, 0); kfree(pathbuf); diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index f944c3e7f340..4e2410b826e2 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -53,6 +53,8 @@ int ima_policy_flag; static int temp_ima_appraise; static int build_ima_appraise __ro_after_init; +atomic_t ima_setxattr_allowed_hash_algorithms; + #define MAX_LSM_RULES 6 enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE, LSM_SUBJ_USER, LSM_SUBJ_ROLE, LSM_SUBJ_TYPE @@ -903,6 +905,8 @@ void __init ima_init_policy(void) ARRAY_SIZE(critical_data_rules), IMA_DEFAULT_POLICY); + atomic_xchg(&ima_setxattr_allowed_hash_algorithms, 0); + ima_update_policy_flag(); } @@ -914,6 +918,42 @@ int ima_check_policy(void) return 0; } +/** update_allowed_hash_algorithms - update the hash algorithms allowed + * for setxattr writes + * + * Update the atomic variable holding the set of allowed hash algorithms + * that can be used to update the security.ima xattr of a file. + * + * Context: called when updating the IMA policy. + * + * SETXATTR_CHECK rules do not implement a full policy check because of + * the performance impact performing rules checking on setxattr() would + * have. The consequence is that only one SETXATTR_CHECK can be active at + * a time. + */ +static void update_allowed_hash_algorithms(void) +{ + struct ima_rule_entry *entry; + + /* + * We scan in reverse order because only the last entry with the + * 'func=SETXATTR_CHECK' apply: this allows runtime upgrades of the + * digest algorithm policy, unlike the other IMA rules that are + * usually append-only. Old rules will still be present in the + * ruleset, but inactive. + */ + rcu_read_lock(); + list_for_each_entry_reverse(entry, ima_rules, list) { + if (entry->func != SETXATTR_CHECK) + continue; + + atomic_xchg(&ima_setxattr_allowed_hash_algorithms, + entry->allowed_hashes); + break; + } + rcu_read_unlock(); +} + /** * ima_update_policy - update default_rules with new measure rules * @@ -931,6 +971,7 @@ void ima_update_policy(void) list_splice_tail_init_rcu(&ima_temp_rules, policy, synchronize_rcu); + if (ima_rules != policy) { ima_policy_flag = 0; ima_rules = policy; @@ -944,6 +985,7 @@ void ima_update_policy(void) kfree(arch_policy_entry); } ima_update_policy_flag(); + update_allowed_hash_algorithms(); /* Custom IMA policy has been loaded */ ima_process_queued_keys(); @@ -1176,6 +1218,19 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) if (ima_rule_contains_lsm_cond(entry)) return false; + break; + case SETXATTR_CHECK: + /* any action other than APPRAISE is unsupported */ + if (entry->action != APPRAISE) + return false; + + /* + * full policies are not supported, they would have too + * much of a performance impact + */ + if (entry->flags & ~(IMA_FUNC | IMA_VALIDATE_HASH)) + return false; + break; default: return false; @@ -1332,6 +1387,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) entry->func = KEY_CHECK; else if (strcmp(args[0].from, "CRITICAL_DATA") == 0) entry->func = CRITICAL_DATA; + else if (strcmp(args[0].from, "SETXATTR_CHECK") == 0) + entry->func = SETXATTR_CHECK; else result = -EINVAL; if (!result)
While users can restrict the accepted hash algorithms for the security.ima xattr file signature when appraising said file, users cannot restrict the algorithms that can be set on that attribute: any algorithm built in the kernel is accepted on a write. Define a new value for the ima policy option 'func' that restricts globally the hash algorithms accepted when writing the security.ima xattr. When a policy contains a rule of the form appraise func=SETXATTR_CHECK appraise_hash=sha256,sha384,sha512 only values corresponding to one of these three digest algorithms will be accepted for writing the security.ima xattr. Attempting to write the attribute using another algorithm (or "free-form" data) will be denied with an audit log message. In the absence of such a policy rule, the default is still to only accept hash algorithms built in the kernel (with all the limitations that entails). Signed-off-by: Simon Thoby <simon.thoby@viveris.fr> --- Documentation/ABI/testing/ima_policy | 9 ++++- security/integrity/ima/ima.h | 4 ++ security/integrity/ima/ima_appraise.c | 31 ++++++++++++--- security/integrity/ima/ima_policy.c | 57 +++++++++++++++++++++++++++ 4 files changed, 94 insertions(+), 7 deletions(-)