On Wed, 2021-08-04 at 09:20 +0000, THOBY Simon wrote:
> By default, writes to the extended attributes security.ima will be
> allowed even if the hash algorithm used for the xattr is not compiled
> in the kernel (which does not make sense because the kernel would not
> be able to appraise that file as it lacks support for validating the
> hash).
>
> Prevent writes to the security.ima xattr if the hash algorithm used is
> not available in the current kernel. Lo an audit message if such an
> operation is attempted.
Both "log" and "audit" can be used as verbs. Perhaps update the first
line to be "Prevent and audit writes...", making the last line
unnecessary.
>
> Signed-off-by: Simon Thoby <simon.thoby@xxxxxxxxxx>
> ---
> security/integrity/ima/ima.h | 2 +-
> security/integrity/ima/ima_appraise.c | 49 ++++++++++++++++++++++++++-
> 2 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 2f4c20b16ad7..829478dabeeb 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -319,7 +319,7 @@ int ima_must_appraise(struct user_namespace *mnt_userns, struct inode *inode,
> void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
> enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
> enum ima_hooks func);
> -enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
> +enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value,
> int xattr_len);
> int ima_read_xattr(struct dentry *dentry,
> struct evm_ima_xattr_data **xattr_value);
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 63bec42c353f..ed1a98f6ee19 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -171,7 +171,7 @@ static void ima_cache_flags(struct integrity_iint_cache *iint,
> }
> }
>
> -enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
> +enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value,
> int xattr_len)
> {
> struct signature_v2_hdr *sig;
> @@ -575,6 +575,51 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)
> clear_bit(IMA_DIGSIG, &iint->atomic_flags);
> }
>
> +/**
Just a reminder, static functions don't require kernel-doc.
> + * validate_hash_algo() - Block setxattr with invalid digests
The digest might be valid, but the algorithm is unsupported. Perhaps
"Block setxattr with unsupported hash algorithms" would be more
accurate.
> + * @dentry: object being setxattr()'ed
^object of the setxattr()
> + * @xattr_value: value supplied by userland for the xattr
userland supplied xattr value
> + * @xattr_value_len: length of xattr_value
> + *
> + * Context: called when the user tries to write the security.ima xattr.
Hm, "context" is probably unnecessary.
>From here:
> + * The xattr value is mapped to some hash algorithm, and this algorithm
> + * must be built in the kernel for the setxattr to be allowed.
> + *
> + * Emit an audit message when the algorithm is invalid.
> + *
to here: is the longer summary, which would be before the "Context:" if
it is defined.
> + * Return: 0 on success, else an error.
> + */
> +static int validate_hash_algo(struct dentry *dentry,
> + const struct evm_ima_xattr_data *xattr_value,
> + size_t xattr_value_len)
> +{
> + int result = 0;
> + char *path = NULL, *pathbuf = NULL;
> + enum hash_algo xattr_hash_algo;
> +
> + 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;
> +
> + result = -EACCES;
Unless there is a common function exit, I would just hard code the
return value.
> +
> + pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> + if (!pathbuf)
> + return result;
> +
> + path = dentry_path(dentry, pathbuf, PATH_MAX);
> +
> + integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry), path,
> + "collect_data", "unavailable-hash-algorithm",
> + result, 0);
> +
> + kfree(pathbuf);
> +
> + return result;
> +}
> +
> int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
> const void *xattr_value, size_t xattr_value_len)
> {
> @@ -595,6 +640,8 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
> ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
> if (result == 1)
> result = 0;
> +
> + result = validate_hash_algo(dentry, xvalue, xattr_value_len);
On failure to validate the hash algorithm, the file will unnecessarily
be re-verified. Only on success the appraise flags should be reset.
thanks,
Mimi
> }
> return result;
> }
