[PATCH v6 5/5] IMA: introduce a new policy option func=SETXATTR_CHECK

THOBY Simon
2021-08-04T09:20:24Z

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 | 6 ++-
security/integrity/ima/ima_appraise.c | 29 ++++++++--
security/integrity/ima/ima_main.c | 2 +-
security/integrity/ima/ima_policy.c | 76 +++++++++++++++++++++++----
5 files changed, 104 insertions(+), 18 deletions(-)

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 8db0238a0fd6..7f2aa875fd72 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,
@@ -288,7 +292,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
const char *func_data, unsigned int *allowed_hashes);
void ima_init_policy(void);
void ima_update_policy(void);
-void ima_update_policy_flag(void);
+void ima_update_policy_flags(void);
ssize_t ima_parse_add_rule(char *);
void ima_delete_rules(void);
int ima_check_policy(void);
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 9aa0334b8e54..4949aabce36c 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -597,12 +597,32 @@ static int validate_hash_algo(struct dentry *dentry,
int result = 0;
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(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 result;
+ allowed_hashes = atomic_read(&ima_setxattr_allowed_hash_algorithms);
+
+ if (allowed_hashes) {
+ /* success if the algorithm is allowed in the ima policy */
+ if (allowed_hashes & (1U << xattr_hash_algo))
+ return result;
+
+ /*
+ * 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 result;
+
+ /* allow any xattr using an algorithm built in the kernel */
+ if (crypto_has_alg(hash_algo_name[xattr_hash_algo], 0, 0))
+ return result;
+ }

result = -EACCES;

@@ -613,8 +633,7 @@ static int validate_hash_algo(struct dentry *dentry,
path = dentry_path(dentry, pathbuf, PATH_MAX);

integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry), path,
- "collect_data", "unavailable-hash-algorithm",
- result, 0);
+ "collect_data", errmsg, result, 0);

kfree(pathbuf);

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 205138e7016d..f6389f0ff81b 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -1052,7 +1052,7 @@ static int __init init_ima(void)
pr_warn("Couldn't register LSM notifier, error %d\n", error);

if (!error)
- ima_update_policy_flag();
+ ima_update_policy_flags();

return error;
}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 831ce0c690ec..c83f7c199610 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
@@ -720,24 +722,57 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
return action;
}

-/*
- * Initialize the ima_policy_flag variable based on the currently
- * loaded policy. Based on this flag, the decision to short circuit
- * out of a function or not call the function in the first place
- * can be made earlier.
+/**
+ * ima_update_policy_flags() - Update global IMA variables
+ *
+ * Update ima_policy_flag and ima_setxattr_allowed_hash_algorithms
+ * based on the currently loaded policy.
+ *
+ * With ima_policy_flag, the decision to short circuit out of a function
+ * or not call the function in the first place can be made earlier.
+ *
+ * With ima_setxattr_allowed_hash_algorithms, the policy can restrict the
+ * set of hash algorithms accepted when updating the security.ima xattr of
+ * a file.
+ *
+ * Context: called after a policy update and at system initialization.
*/
-void ima_update_policy_flag(void)
+void ima_update_policy_flags(void)
{
struct ima_rule_entry *entry;
+ int new_policy_flag = 0;

+ rcu_read_lock();
list_for_each_entry(entry, ima_rules, list) {
+ /*
+ * SETXATTR_CHECK rules do not implement a full policy check
+ * because rule checking would probably have an important
+ * performance impact on setxattr(). As a consequence, only one
+ * SETXATTR_CHECK can be active at a given time.
+ * Because we want to preserve that property, we set out to use
+ * atomic_cmpxchg. Either:
+ * - the atomic was non-zero: a setxattr hash policy is
+ * already enforced, we do nothing
+ * - the atomic was zero: no setxattr policy was set, enable
+ * the setxattr hash policy
+ */
+ if (entry->func == SETXATTR_CHECK) {
+ atomic_cmpxchg(&ima_setxattr_allowed_hash_algorithms,
+ 0, entry->allowed_hashes);
+ /* SETXATTR_CHECK doesn't impact ima_policy_flag */
+ continue;
+ }
+
if (entry->action & IMA_DO_MASK)
- ima_policy_flag |= entry->action;
+ new_policy_flag |= entry->action;
}
+ rcu_read_unlock();

ima_appraise |= (build_ima_appraise | temp_ima_appraise);
if (!ima_appraise)
- ima_policy_flag &= ~IMA_APPRAISE;
+ new_policy_flag &= ~IMA_APPRAISE;
+
+ ima_policy_flag = new_policy_flag;
}

static int ima_appraise_flag(enum ima_hooks func)
@@ -903,7 +938,9 @@ void __init ima_init_policy(void)
ARRAY_SIZE(critical_data_rules),
IMA_DEFAULT_POLICY);

- ima_update_policy_flag();
+ atomic_set(&ima_setxattr_allowed_hash_algorithms, 0);
+
+ ima_update_policy_flags();
}

/* Make sure we have a valid policy, at least containing some rules. */
@@ -943,7 +980,7 @@ void ima_update_policy(void)
*/
kfree(arch_policy_entry);
}
- ima_update_policy_flag();
+ ima_update_policy_flags();

/* Custom IMA policy has been loaded */
ima_process_queued_keys();
@@ -1176,6 +1213,23 @@ 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;
+
+ /* SETXATTR_CHECK requires an appraise_hash parameter */
+ if (!(entry->flags & IMA_VALIDATE_HASH))
+ 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 +1386,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)
--
2.31.1