[PATCH] audit: fix error handling in audit_data_to_entry()
Richard Guy Briggs
rgb at redhat.com
Tue Mar 17 20:42:13 UTC 2020
On 2020-03-17 16:37, Paul Moore wrote:
> On Tue, Mar 17, 2020 at 3:14 PM Richard Guy Briggs <rgb at redhat.com> wrote:
> > 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: stable at vger.kernel.org
> > > Fixes: 219ca39427bf ("audit: use union for audit_field values since they are mutually exclusive")
> > > Reported-by: syzbot+1f4d90ead370d72e450b at syzkaller.appspotmail.com
> > > Signed-off-by: Paul Moore <paul at paul-moore.com>
> > > ---
> > > 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?
>
> After audit_unpack_string() succeeds we store "str" in the audit_field
> struct which should be cleaned up by audit_free_rule() when we jump to
> exit_free.
Got it, cool. Everything else looked in place. Looks good to me.
> > > 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;
> > > }
> > > }
>
> --
> paul moore
> www.paul-moore.com
>
- RGB
--
Richard Guy Briggs <rgb at redhat.com>
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
More information about the Linux-audit
mailing list