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
