[PATCH iproute2] iproute2: fix use-after-free

[PATCH iproute2] iproute2: fix use-after-free

[Mahesh Bandewar]

From: Mahesh Bandewar <maheshb@google.com>

A local program using iproute2 lib pointed out the issue and looking
at the code it is pretty obvious -

    a = (struct nlmsghdr *)b;
    ...
    free(b);
    if (a->nlmsg_seq == seq)
    ...

Fixes: 86bf43c7c2fd ("lib/libnetlink: update rtnl_talk to support malloc buff at run time")
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
 lib/libnetlink.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 928de1dd16d8..016a5f0bcfb6 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -661,17 +661,24 @@ next:
 				if (l < sizeof(struct nlmsgerr)) {
 					fprintf(stderr, "ERROR truncated\n");
 				} else if (!err->error) {
+					unsigned int tmp_seq;
+
 					/* check messages from kernel */
 					nl_dump_ext_ack(h, errfn);
 
-					if (answer)
+					tmp_seq = h->nlmsg_seq;
+					if (answer) {
 						*answer = (struct nlmsghdr *)buf;
-					else
+					} else {
 						free(buf);
-					if (h->nlmsg_seq == seq)
+						buf = NULL;
+					}
+					if (tmp_seq == seq) {
 						return 0;
-					else if (i < iovlen)
+					} else if (i < iovlen) {
+						free(buf);
 						goto next;
+					}
 					return 0;
 				}
 
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

Re: [PATCH iproute2] iproute2: fix use-after-free

[Stephen Hemminger]

On Wed, 12 Sep 2018 16:29:28 -0700
Mahesh Bandewar <mahesh@bandewar.net> wrote:

> From: Mahesh Bandewar <maheshb@google.com>
> 
> A local program using iproute2 lib pointed out the issue and looking
> at the code it is pretty obvious -
> 
>     a = (struct nlmsghdr *)b;
>     ...
>     free(b);
>     if (a->nlmsg_seq == seq)
>     ...
> 
> Fixes: 86bf43c7c2fd ("lib/libnetlink: update rtnl_talk to support malloc buff at run time")
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>

Yes, this is a real problem.

Maybe a minimal patch like this would be enough:
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 928de1dd16d8..ab2d8452e4a1 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -661,6 +661,8 @@ next:
 				if (l < sizeof(struct nlmsgerr)) {
 					fprintf(stderr, "ERROR truncated\n");
 				} else if (!err->error) {
+					__u32 err_seq = h->nlmsg_seq;
+
 					/* check messages from kernel */
 					nl_dump_ext_ack(h, errfn);
 
@@ -668,7 +670,8 @@ next:
 						*answer = (struct nlmsghdr *)buf;
 					else
 						free(buf);
-					if (h->nlmsg_seq == seq)
+
+					if (err_seq == seq)
 						return 0;
 					else if (i < iovlen)
 						goto next;

Re: [PATCH iproute2] iproute2: fix use-after-free

[Mahesh Bandewar (महेश बंडेवार)]

On Wed, Sep 12, 2018 at 5:33 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Wed, 12 Sep 2018 16:29:28 -0700
> Mahesh Bandewar <mahesh@bandewar.net> wrote:
>
> > From: Mahesh Bandewar <maheshb@google.com>
> >
> > A local program using iproute2 lib pointed out the issue and looking
> > at the code it is pretty obvious -
> >
> >     a = (struct nlmsghdr *)b;
> >     ...
> >     free(b);
> >     if (a->nlmsg_seq == seq)
> >     ...
> >
> > Fixes: 86bf43c7c2fd ("lib/libnetlink: update rtnl_talk to support malloc buff at run time")
> > Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>
> Yes, this is a real problem.
>
> Maybe a minimal patch like this would be enough:
actually that will leave the memory leak at the 'goto next' line (just
few lines below) since that jump will overwrite the buf.

> diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> index 928de1dd16d8..ab2d8452e4a1 100644
> --- a/lib/libnetlink.c
> +++ b/lib/libnetlink.c
> @@ -661,6 +661,8 @@ next:
>                                 if (l < sizeof(struct nlmsgerr)) {
>                                         fprintf(stderr, "ERROR truncated\n");
>                                 } else if (!err->error) {
> +                                       __u32 err_seq = h->nlmsg_seq;
> +
>                                         /* check messages from kernel */
>                                         nl_dump_ext_ack(h, errfn);
>
> @@ -668,7 +670,8 @@ next:
>                                                 *answer = (struct nlmsghdr *)buf;
>                                         else
>                                                 free(buf);
> -                                       if (h->nlmsg_seq == seq)
> +
> +                                       if (err_seq == seq)
>                                                 return 0;
>                                         else if (i < iovlen)
>                                                 goto next;
>

Re: [PATCH iproute2] iproute2: fix use-after-free

[Stephen Hemminger]

On Wed, 12 Sep 2018 23:07:20 -0700
Mahesh Bandewar (महेश बंडेवार) <maheshb@google.com> wrote:

> On Wed, Sep 12, 2018 at 5:33 PM, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Wed, 12 Sep 2018 16:29:28 -0700
> > Mahesh Bandewar <mahesh@bandewar.net> wrote:
> >  
> > > From: Mahesh Bandewar <maheshb@google.com>
> > >
> > > A local program using iproute2 lib pointed out the issue and looking
> > > at the code it is pretty obvious -
> > >
> > >     a = (struct nlmsghdr *)b;
> > >     ...
> > >     free(b);
> > >     if (a->nlmsg_seq == seq)
> > >     ...
> > >
> > > Fixes: 86bf43c7c2fd ("lib/libnetlink: update rtnl_talk to support malloc buff at run time")
> > > Signed-off-by: Mahesh Bandewar <maheshb@google.com>  
> >
> > Yes, this is a real problem.
> >
> > Maybe a minimal patch like this would be enough:  
> actually that will leave the memory leak at the 'goto next' line (just
> few lines below) since that jump will overwrite the buf.

It looks like everytime in the while loop. a new buffer is allocated.
So yes, it looks like old, my patch, and your patch would leak there
was an error followed by other data in response.
Though I doubt kernel would ever do that.

The only user of iov style messages to the kernel is in tc batching.
My gut feeling is that if one message in batch has error, then
the netlink code should return that error and stop processing more.

Re: [PATCH iproute2] iproute2: fix use-after-free

[Mahesh Bandewar (महेश बंडेवार)]

On Thu, Sep 13, 2018 at 8:19 AM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Wed, 12 Sep 2018 23:07:20 -0700
> Mahesh Bandewar (महेश बंडेवार) <maheshb@google.com> wrote:
>
> > On Wed, Sep 12, 2018 at 5:33 PM, Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> > >
> > > On Wed, 12 Sep 2018 16:29:28 -0700
> > > Mahesh Bandewar <mahesh@bandewar.net> wrote:
> > >
> > > > From: Mahesh Bandewar <maheshb@google.com>
> > > >
> > > > A local program using iproute2 lib pointed out the issue and looking
> > > > at the code it is pretty obvious -
> > > >
> > > >     a = (struct nlmsghdr *)b;
> > > >     ...
> > > >     free(b);
> > > >     if (a->nlmsg_seq == seq)
> > > >     ...
> > > >
> > > > Fixes: 86bf43c7c2fd ("lib/libnetlink: update rtnl_talk to support malloc buff at run time")
> > > > Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> > >
> > > Yes, this is a real problem.
> > >
> > > Maybe a minimal patch like this would be enough:
> > actually that will leave the memory leak at the 'goto next' line (just
> > few lines below) since that jump will overwrite the buf.
>
> It looks like everytime in the while loop. a new buffer is allocated.
> So yes, it looks like old, my patch, and your patch would leak there
> was an error followed by other data in response.
> Though I doubt kernel would ever do that.
>
I started fixing the issue that I reported and then found-out the
memory leak and hence the first attempt of simple fix went into fixing
free-after-use as well as memory leak (in my patch). I'm not going to
claim that I know how and where this gets used, but my attempt was to
simply fix those two issues. I don't mind which fix you apply as long
as these issues get addressed.

> The only user of iov style messages to the kernel is in tc batching.
> My gut feeling is that if one message in batch has error, then
> the netlink code should return that error and stop processing more.