On 2020-02-24 16:31, Paul Moore wrote:
> Commit 219ca39427bf ("audit: use union for audit_field values since
> they are mutually exclusive") combined a number of separate fields in
> the audit_field struct into a single union. Generally this worked
> just fine because they are generally mutually exclusive.
> Unfortunately in audit_data_to_entry() the overlap can be a problem
> when a specific error case is triggered that causes the error path
> code to attempt to cleanup an audit_field struct and the cleanup
> involves attempting to free a stored LSM string (the lsm_str field).
> Currently the code always has a non-NULL value in the
> audit_field.lsm_str field as the top of the for-loop transfers a
> value into audit_field.val (both .lsm_str and .val are part of the
> same union); if audit_data_to_entry() fails and the audit_field
> struct is specified to contain a LSM string, but the
> audit_field.lsm_str has not yet been properly set, the error handling
> code will attempt to free the bogus audit_field.lsm_str value that
> was set with audit_field.val at the top of the for-loop.
>
> This patch corrects this by ensuring that the audit_field.val is only
> set when needed (it is cleared when the audit_field struct is
> allocated with kcalloc()). It also corrects a few other issues to
> ensure that in case of error the proper error code is returned.
>
> Cc: [email protected]
> Fixes: 219ca39427bf ("audit: use union for audit_field values since they are
> mutually exclusive")
> Reported-by: [email protected]
> Signed-off-by: Paul Moore <[email protected]>
> ---
> kernel/auditfilter.c | 71
> +++++++++++++++++++++++++++-----------------------
> 1 file changed, 39 insertions(+), 32 deletions(-)
>
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index b0126e9c0743..026e34da4ace 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -456,6 +456,7 @@ static struct audit_entry *audit_data_to_entry(struct
> audit_rule_data *data,
> bufp = data->buf;
> for (i = 0; i < data->field_count; i++) {
> struct audit_field *f = &entry->rule.fields[i];
> + u32 f_val;
>
> err = -EINVAL;
>
> @@ -464,12 +465,12 @@ static struct audit_entry *audit_data_to_entry(struct
> audit_rule_data *data,
> goto exit_free;
>
> f->type = data->fields[i];
> - f->val = data->values[i];
> + f_val = data->values[i];
>
> /* Support legacy tests for a valid loginuid */
> - if ((f->type == AUDIT_LOGINUID) && (f->val == AUDIT_UID_UNSET))
> {
> + if ((f->type == AUDIT_LOGINUID) && (f_val == AUDIT_UID_UNSET)) {
> f->type = AUDIT_LOGINUID_SET;
> - f->val = 0;
> + f_val = 0;
> entry->rule.pflags |= AUDIT_LOGINUID_LEGACY;
> }
>
> @@ -485,7 +486,7 @@ static struct audit_entry *audit_data_to_entry(struct
> audit_rule_data *data,
> case AUDIT_SUID:
> case AUDIT_FSUID:
> case AUDIT_OBJ_UID:
> - f->uid = make_kuid(current_user_ns(), f->val);
> + f->uid = make_kuid(current_user_ns(), f_val);
> if (!uid_valid(f->uid))
> goto exit_free;
> break;
> @@ -494,11 +495,12 @@ static struct audit_entry *audit_data_to_entry(struct
> audit_rule_data *data,
> case AUDIT_SGID:
> case AUDIT_FSGID:
> case AUDIT_OBJ_GID:
> - f->gid = make_kgid(current_user_ns(), f->val);
> + f->gid = make_kgid(current_user_ns(), f_val);
> if (!gid_valid(f->gid))
> goto exit_free;
> break;
> case AUDIT_ARCH:
> + f->val = f_val;
> entry->rule.arch_f = f;
> break;
> case AUDIT_SUBJ_USER:
> @@ -511,11 +513,13 @@ static struct audit_entry *audit_data_to_entry(struct
> audit_rule_data *data,
> case AUDIT_OBJ_TYPE:
> case AUDIT_OBJ_LEV_LOW:
> case AUDIT_OBJ_LEV_HIGH:
> - str = audit_unpack_string(&bufp, &remain, f->val);
> - if (IS_ERR(str))
> + str = audit_unpack_string(&bufp, &remain, f_val);
> + if (IS_ERR(str)) {
> + err = PTR_ERR(str);
> goto exit_free;
> - entry->rule.buflen += f->val;
> -
> + }
> + entry->rule.buflen += f_val;
> + f->lsm_str = str;
> err = security_audit_rule_init(f->type, f->op, str,
> (void **)&f->lsm_rule);
> /* Keep currently invalid fields around in case they
> @@ -524,68 +528,71 @@ static struct audit_entry *audit_data_to_entry(struct
> audit_rule_data *data,
> pr_warn("audit rule for LSM \'%s\' is
> invalid\n",
> str);
> err = 0;
> - }
> - if (err) {
> - kfree(str);
> + } else if (err)
If there is an error from security_audit_rule_init() other than -EINVAL
(which could become valid after a policy reload), would the str passed
to it not need to be freed before we goto exit_free?
I think we need a
kfree(str);
here (with braces, of course).
> goto exit_free;
> - } else
> - f->lsm_str = str;
> break;
> case AUDIT_WATCH:
> - str = audit_unpack_string(&bufp, &remain, f->val);
> - if (IS_ERR(str))
> + str = audit_unpack_string(&bufp, &remain, f_val);
> + if (IS_ERR(str)) {
> + err = PTR_ERR(str);
> goto exit_free;
> - entry->rule.buflen += f->val;
> -
> - err = audit_to_watch(&entry->rule, str, f->val, f->op);
> + }
> + err = audit_to_watch(&entry->rule, str, f_val, f->op);
> if (err) {
> kfree(str);
> goto exit_free;
> }
> + entry->rule.buflen += f_val;
> break;
> case AUDIT_DIR:
> - str = audit_unpack_string(&bufp, &remain, f->val);
> - if (IS_ERR(str))
> + str = audit_unpack_string(&bufp, &remain, f_val);
> + if (IS_ERR(str)) {
> + err = PTR_ERR(str);
> goto exit_free;
> - entry->rule.buflen += f->val;
> -
> + }
> err = audit_make_tree(&entry->rule, str, f->op);
> kfree(str);
> if (err)
> goto exit_free;
> + entry->rule.buflen += f_val;
> break;
> case AUDIT_INODE:
> + f->val = f_val;
> err = audit_to_inode(&entry->rule, f);
> if (err)
> goto exit_free;
> break;
> case AUDIT_FILTERKEY:
> - if (entry->rule.filterkey || f->val > AUDIT_MAX_KEY_LEN)
> + if (entry->rule.filterkey || f_val > AUDIT_MAX_KEY_LEN)
> goto exit_free;
> - str = audit_unpack_string(&bufp, &remain, f->val);
> - if (IS_ERR(str))
> + str = audit_unpack_string(&bufp, &remain, f_val);
> + if (IS_ERR(str)) {
> + err = PTR_ERR(str);
> goto exit_free;
> - entry->rule.buflen += f->val;
> + }
> + entry->rule.buflen += f_val;
> entry->rule.filterkey = str;
> break;
> case AUDIT_EXE:
> - if (entry->rule.exe || f->val > PATH_MAX)
> + if (entry->rule.exe || f_val > PATH_MAX)
> goto exit_free;
> - str = audit_unpack_string(&bufp, &remain, f->val);
> + str = audit_unpack_string(&bufp, &remain, f_val);
> if (IS_ERR(str)) {
> err = PTR_ERR(str);
> goto exit_free;
> }
> - entry->rule.buflen += f->val;
> -
> - audit_mark = audit_alloc_mark(&entry->rule, str,
> f->val);
> + audit_mark = audit_alloc_mark(&entry->rule, str, f_val);
> if (IS_ERR(audit_mark)) {
> kfree(str);
> err = PTR_ERR(audit_mark);
> goto exit_free;
> }
> + entry->rule.buflen += f_val;
> entry->rule.exe = audit_mark;
> break;
> + default:
> + f->val = f_val;
> + break;
> }
> }
>
>
> --
> Linux-audit mailing list
> [email protected]
> https://www.redhat.com/mailman/listinfo/linux-audit
- RGB
--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit
