[PATCH] audit: always check the netlink payload length in audit_receive_msg()

[Richard Guy Briggs]

On 2020-02-24 17:58, Paul Moore wrote:
> On Mon, Feb 24, 2020 at 5:53 PM Paul Moore <paul at paul-moore.com> wrote:
> >
> > This patch ensures that we always check the netlink payload length
> > in audit_receive_msg() before we take any action on the payload
> > itself.
> >
> > Cc: stable at vger.kernel.org
> > Reported-by: syzbot+399c44bf1f43b8747403 at syzkaller.appspotmail.com
> > Reported-by: syzbot+e4b12d8d202701f08b6d at syzkaller.appspotmail.com
> > Signed-off-by: Paul Moore <paul at paul-moore.com>
> > ---
> >  kernel/audit.c |   43 +++++++++++++++++++++++--------------------
> >  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> ...
> 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 17b0d523afb3..6e8b176bdb68 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1334,26 +1339,24 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> >                                         break;
> >                         }
> >                         audit_log_user_recv_msg(&ab, msg_type);
> > -                       if (msg_type != AUDIT_USER_TTY)
> > +                       if (msg_type != AUDIT_USER_TTY) {
> > +                               /* ensure NULL termination */
> > +                               str[data_len - 1] = '\0';
> >                                 audit_log_format(ab, " msg='%.*s'",
> >                                                  AUDIT_MESSAGE_TEXT_MAX,
> > -                                                (char *)data);
> > -                       else {
> > -                               int size;
> > -
> > +                                                str);
> > +                       } else {
> >                                 audit_log_format(ab, " data=");
> > -                               size = nlmsg_len(nlh);
> > -                               if (size > 0 &&
> > -                                   ((unsigned char *)data)[size - 1] == '\0')
> > -                                       size--;
> > -                               audit_log_n_untrustedstring(ab, data, size);
> > +                               if (data_len > 0 && str[data_len - 1] == '\0')
> > +                                       data_len--;
> > +                               audit_log_n_untrustedstring(ab, data, data_len);
>                                                                   ^^^^
> ... and it looks like I didn't properly refresh my patch before
> sending, the second arg in the line above is "str" not "data".

Ok, better late than never.  This all looks reasonable to me, but I've
not tested it.

> paul moore

- 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