[PATCH] monitor: Add message length check to nlmon_receice

[PATCH] monitor: Add message length check to nlmon_receice

[Daniel Wagner]

[-- Attachment #1: Type: text/plain, Size: 1768 bytes --]

The NLMSG_NEXT macro calculates the next nlmsg and updates the len
field:

  #define NLMSG_NEXT(nlh,len)	 ((len) -= NLMSG_ALIGN((nlh)->nlmsg_len), \
				  (struct nlmsghdr*)(((char*)(nlh)) + NLMSG_ALIGN((nlh)->nlmsg_len)))

That means nlmsg_len needs to be an multiple of NLMSG_ALIGNTO to avoid
an underflow in len. But there are message which do not have a valid
lenght:

  Breakpoint 1, nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at monitor/nlmon.c:6947
  6947                            printf("malformed packet\n");
  (gdb) bt
  #0  nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at monitor/nlmon.c:6947
  #1  0x0000000000439a91 in io_callback (fd=7, events=1, user_data=0x4da210) at ell/io.c:126
  #2  0x0000000000438861 in l_main_iterate (timeout=-1) at ell/main.c:473
  #3  0x0000000000438968 in l_main_run () at ell/main.c:520
  #4  0x0000000000438c80 in l_main_run_with_signal (callback=0x4038e6 <signal_handler>, user_data=0x0) at ell/main.c:642
  #5  0x0000000000403cc9 in main (argc=3, argv=0x7fffffffec28) at monitor/main.c:806
  (gdb) p nlmsg->nlmsg_len
  $5 = 17

By adding an lenght check after each processed message garantees that
we do not underflow. The downside is that as soon an invalid length is
spotted the processing stops.
---
 monitor/nlmon.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/monitor/nlmon.c b/monitor/nlmon.c
index 6b0afef8a497..5498e9fa0f23 100644
--- a/monitor/nlmon.c
+++ b/monitor/nlmon.c
@@ -6941,6 +6941,11 @@ static bool nlmon_receive(struct l_io *io, void *user_data)
 			nlmon_message(nlmon, tv, tp, nlmsg);
 			break;
 		}
+
+		if (nlmsg->nlmsg_len & (NLMSG_ALIGNTO-1)) {
+			printf("malformed packet\n");
+			break;
+		}
 	}
 
 	return true;
-- 
2.24.1

Re: [PATCH] monitor: Add message length check to nlmon_receice

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2257 bytes --]

Hi Daniel,

On 1/19/20 10:31 AM, Daniel Wagner wrote:
> The NLMSG_NEXT macro calculates the next nlmsg and updates the len
> field:
> 
>    #define NLMSG_NEXT(nlh,len)	 ((len) -= NLMSG_ALIGN((nlh)->nlmsg_len), \
> 				  (struct nlmsghdr*)(((char*)(nlh)) + NLMSG_ALIGN((nlh)->nlmsg_len)))
> 
> That means nlmsg_len needs to be an multiple of NLMSG_ALIGNTO to avoid
> an underflow in len. But there are message which do not have a valid
> lenght:
> 

hmm, do you have a pcap of such a message?

>    Breakpoint 1, nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at monitor/nlmon.c:6947
>    6947                            printf("malformed packet\n");
>    (gdb) bt
>    #0  nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at monitor/nlmon.c:6947
>    #1  0x0000000000439a91 in io_callback (fd=7, events=1, user_data=0x4da210) at ell/io.c:126
>    #2  0x0000000000438861 in l_main_iterate (timeout=-1) at ell/main.c:473
>    #3  0x0000000000438968 in l_main_run () at ell/main.c:520
>    #4  0x0000000000438c80 in l_main_run_with_signal (callback=0x4038e6 <signal_handler>, user_data=0x0) at ell/main.c:642
>    #5  0x0000000000403cc9 in main (argc=3, argv=0x7fffffffec28) at monitor/main.c:806
>    (gdb) p nlmsg->nlmsg_len
>    $5 = 17
> 
> By adding an lenght check after each processed message garantees that
> we do not underflow. The downside is that as soon an invalid length is
> spotted the processing stops.

Hmm, isn't NLMSG_OK already enough for this?  It checks that length is 
positive and above a certain threshold?

> ---
>   monitor/nlmon.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/monitor/nlmon.c b/monitor/nlmon.c
> index 6b0afef8a497..5498e9fa0f23 100644
> --- a/monitor/nlmon.c
> +++ b/monitor/nlmon.c
> @@ -6941,6 +6941,11 @@ static bool nlmon_receive(struct l_io *io, void *user_data)
>   			nlmon_message(nlmon, tv, tp, nlmsg);
>   			break;
>   		}
> +
> +		if (nlmsg->nlmsg_len & (NLMSG_ALIGNTO-1)) {
> +			printf("malformed packet\n");
> +			break;
> +		}

Also I'm not sure whether this really holds for messages that are 
'last'.  There's no sense to check or enforce alignment in such a case/

>   	}
>   
>   	return true;
> 

Regards,
-Denis

Re: [PATCH] monitor: Add message length check to nlmon_receice

[Daniel Wagner]

[-- Attachment #1: Type: text/plain, Size: 2797 bytes --]

On Tue, Jan 21, 2020 at 04:36:37PM -0600, Denis Kenzior wrote:
> Hi Daniel,
> 
> On 1/19/20 10:31 AM, Daniel Wagner wrote:
> > The NLMSG_NEXT macro calculates the next nlmsg and updates the len
> > field:
> > 
> >    #define NLMSG_NEXT(nlh,len)	 ((len) -= NLMSG_ALIGN((nlh)->nlmsg_len), \
> > 				  (struct nlmsghdr*)(((char*)(nlh)) + NLMSG_ALIGN((nlh)->nlmsg_len)))
> > 
> > That means nlmsg_len needs to be an multiple of NLMSG_ALIGNTO to avoid
> > an underflow in len. But there are message which do not have a valid
> > lenght:
> > 
> 
> hmm, do you have a pcap of such a message?

I can try to capture one. The problem is that the monitor crashes
without this patch.

> >    Breakpoint 1, nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at monitor/nlmon.c:6947
> >    6947                            printf("malformed packet\n");
> >    (gdb) bt
> >    #0  nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at monitor/nlmon.c:6947
> >    #1  0x0000000000439a91 in io_callback (fd=7, events=1, user_data=0x4da210) at ell/io.c:126
> >    #2  0x0000000000438861 in l_main_iterate (timeout=-1) at ell/main.c:473
> >    #3  0x0000000000438968 in l_main_run () at ell/main.c:520
> >    #4  0x0000000000438c80 in l_main_run_with_signal (callback=0x4038e6 <signal_handler>, user_data=0x0) at ell/main.c:642
> >    #5  0x0000000000403cc9 in main (argc=3, argv=0x7fffffffec28) at monitor/main.c:806
> >    (gdb) p nlmsg->nlmsg_len
> >    $5 = 17
> > 
> > By adding an lenght check after each processed message garantees that
> > we do not underflow. The downside is that as soon an invalid length is
> > spotted the processing stops.
> 
> Hmm, isn't NLMSG_OK already enough for this?  It checks that length is
> positive and above a certain threshold?

No, the problem is how the len is calculated. In the above example len
will be -3

	len = 17 - 20

and from there the next pointer nlmsg pointer will be calculated,
which is invalid.


> > ---
> >   monitor/nlmon.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/monitor/nlmon.c b/monitor/nlmon.c
> > index 6b0afef8a497..5498e9fa0f23 100644
> > --- a/monitor/nlmon.c
> > +++ b/monitor/nlmon.c
> > @@ -6941,6 +6941,11 @@ static bool nlmon_receive(struct l_io *io, void *user_data)
> >   			nlmon_message(nlmon, tv, tp, nlmsg);
> >   			break;
> >   		}
> > +
> > +		if (nlmsg->nlmsg_len & (NLMSG_ALIGNTO-1)) {
> > +			printf("malformed packet\n");
> > +			break;
> > +		}
> 
> Also I'm not sure whether this really holds for messages that are 'last'.
> There's no sense to check or enforce alignment in such a case/

In my example it is the last message. I've printed out the nlmsg_len
and all of them seem to be a mulitple of NLMSG_ALIGNTO.

Thanks,
Daniel

Re: [PATCH] monitor: Add message length check to nlmon_receice

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3203 bytes --]

Hi Daniel,

On 1/22/20 3:05 AM, Daniel Wagner wrote:
> On Tue, Jan 21, 2020 at 04:36:37PM -0600, Denis Kenzior wrote:
>> Hi Daniel,
>>
>> On 1/19/20 10:31 AM, Daniel Wagner wrote:
>>> The NLMSG_NEXT macro calculates the next nlmsg and updates the len
>>> field:
>>>
>>>     #define NLMSG_NEXT(nlh,len)	 ((len) -= NLMSG_ALIGN((nlh)->nlmsg_len), \
>>> 				  (struct nlmsghdr*)(((char*)(nlh)) + NLMSG_ALIGN((nlh)->nlmsg_len)))
>>>
>>> That means nlmsg_len needs to be an multiple of NLMSG_ALIGNTO to avoid
>>> an underflow in len. But there are message which do not have a valid
>>> lenght:
>>>
>>
>> hmm, do you have a pcap of such a message?
> 
> I can try to capture one. The problem is that the monitor crashes
> without this patch.

Are you running the latest version btw? From your backtrace it seems 
like the nlmon.c file I have is different from yours.

> 
>>>     Breakpoint 1, nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at monitor/nlmon.c:6947
>>>     6947                            printf("malformed packet\n");
>>>     (gdb) bt
>>>     #0  nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at monitor/nlmon.c:6947
>>>     #1  0x0000000000439a91 in io_callback (fd=7, events=1, user_data=0x4da210) at ell/io.c:126
>>>     #2  0x0000000000438861 in l_main_iterate (timeout=-1) at ell/main.c:473
>>>     #3  0x0000000000438968 in l_main_run () at ell/main.c:520
>>>     #4  0x0000000000438c80 in l_main_run_with_signal (callback=0x4038e6 <signal_handler>, user_data=0x0) at ell/main.c:642
>>>     #5  0x0000000000403cc9 in main (argc=3, argv=0x7fffffffec28) at monitor/main.c:806
>>>     (gdb) p nlmsg->nlmsg_len
>>>     $5 = 17
>>>
>>> By adding an lenght check after each processed message garantees that
>>> we do not underflow. The downside is that as soon an invalid length is
>>> spotted the processing stops.
>>
>> Hmm, isn't NLMSG_OK already enough for this?  It checks that length is
>> positive and above a certain threshold?
> 
> No, the problem is how the len is calculated. In the above example len
> will be -3
> 
> 	len = 17 - 20

Right, I get that.  But I think the issue isn't what you think it is 
exactly.  Here's the rough loop inside nlmon_receive:

size_t nlmsg_len;

nlmsg_len = bytes_read;

for (nlmsg = foo; NLMSG_OK(nlmsg, nlmsg_len;
			nlmsg = NLMSG_NEXT(nlmsg, nlmsg_len)) {
	// process message nlmsg
}

NLMSG_OK and NLMSG_NEXT are macros in linux/netlink.h.  The problem is 
that NLMSG_OK and NLMSG_NEXT expect to operate on an int according to 
'man netlink', but they don't actually cast the argument to an int.

So what happens is that nlmsg_len underflows, wraps around to some large 
number and the crash happens.

See if changing the nlmsg_len type to an int fixes this problem.

<snip>

>>
>> Also I'm not sure whether this really holds for messages that are 'last'.
>> There's no sense to check or enforce alignment in such a case/
> 
> In my example it is the last message. I've printed out the nlmsg_len
> and all of them seem to be a mulitple of NLMSG_ALIGNTO.
> 

Then I don't see how a crash would happen, wouldn't nlmsg_len just 
become 0 then?

Regards,
-Denis

Re: [PATCH] monitor: Add message length check to nlmon_receice

[Daniel Wagner]

[-- Attachment #1: Type: text/plain, Size: 3135 bytes --]

Hi Denis,

On Wed, Jan 22, 2020 at 10:21:10AM -0600, Denis Kenzior wrote:
> On 1/22/20 3:05 AM, Daniel Wagner wrote:
> > On Tue, Jan 21, 2020 at 04:36:37PM -0600, Denis Kenzior wrote:
> > > Hi Daniel,
> > > 
> > > On 1/19/20 10:31 AM, Daniel Wagner wrote:
> > > > The NLMSG_NEXT macro calculates the next nlmsg and updates the len
> > > > field:
> > > > 
> > > >     #define NLMSG_NEXT(nlh,len)	 ((len) -= NLMSG_ALIGN((nlh)->nlmsg_len), \
> > > > 				  (struct nlmsghdr*)(((char*)(nlh)) + NLMSG_ALIGN((nlh)->nlmsg_len)))
> > > > 
> > > > That means nlmsg_len needs to be an multiple of NLMSG_ALIGNTO to avoid
> > > > an underflow in len. But there are message which do not have a valid
> > > > lenght:
> > > > 
> > > 
> > > hmm, do you have a pcap of such a message?
> > 
> > I can try to capture one. The problem is that the monitor crashes
> > without this patch.
> 
> Are you running the latest version btw? From your backtrace it seems like
> the nlmon.c file I have is different from yours.

Oh, I thought I did the patch against head, but well. It was against:

2a7c4a9c3d22 ("doc: Fix wrong interface name")

> Right, I get that.  But I think the issue isn't what you think it is
> exactly.  Here's the rough loop inside nlmon_receive:
> 
> size_t nlmsg_len;
> 
> nlmsg_len = bytes_read;
> 
> for (nlmsg = foo; NLMSG_OK(nlmsg, nlmsg_len;
> 			nlmsg = NLMSG_NEXT(nlmsg, nlmsg_len)) {
> 	// process message nlmsg
> }
> 
> NLMSG_OK and NLMSG_NEXT are macros in linux/netlink.h.  The problem is that
> NLMSG_OK and NLMSG_NEXT expect to operate on an int according to 'man
> netlink', but they don't actually cast the argument to an int.
> 
> So what happens is that nlmsg_len underflows, wraps around to some large
> number and the crash happens.

It's not the current message which triggers the crash, it's the next
loop iteration which triggers the crash.

> See if changing the nlmsg_len type to an int fixes this problem.

Indeed, changing the type fixed it:

$ git diff
diff --git a/monitor/nlmon.c b/monitor/nlmon.c
index 77f5dda42946..45bd37202f9d 100644
--- a/monitor/nlmon.c
+++ b/monitor/nlmon.c
@@ -6876,7 +6876,7 @@ static bool nlmon_receive(struct l_io *io, void *user_data)
        unsigned char buf[8192];
        unsigned char control[32];
        ssize_t bytes_read;
-       size_t nlmsg_len;
+       ssize_t nlmsg_len;
        int fd;
 
        fd = l_io_get_fd(io);

> > > Also I'm not sure whether this really holds for messages that are 'last'.
> > > There's no sense to check or enforce alignment in such a case/
> > 
> > In my example it is the last message. I've printed out the nlmsg_len
> > and all of them seem to be a mulitple of NLMSG_ALIGNTO.
> > 
> 
> Then I don't see how a crash would happen, wouldn't nlmsg_len just become 0
> then?

The check would just abort the loop as soon a the lenght check
triggers. I've tested the new version and nlmsg_len still got
underflow but the macro aborts the loop now.

Do you want me to send a new patch or are you going to fix it
yourself? I don't mind :)

Thanks,
Daniel

Re: [PATCH] monitor: Add message length check to nlmon_receice

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1553 bytes --]

Hi Daniel,

>> NLMSG_OK and NLMSG_NEXT are macros in linux/netlink.h.  The problem is that
>> NLMSG_OK and NLMSG_NEXT expect to operate on an int according to 'man
>> netlink', but they don't actually cast the argument to an int.
>>
>> So what happens is that nlmsg_len underflows, wraps around to some large
>> number and the crash happens.
> 
> It's not the current message which triggers the crash, it's the next
> loop iteration which triggers the crash.

Right.  So after the last valid message, NLMSG_NEXT gets called which 
causes the underflow.  But since the type is unsigned, nlmsg_len just 
becomes a large number.  Then NLMSG_OK is invoked and thinks nlmsg_len 
is still all okay.

>>
>> Then I don't see how a crash would happen, wouldn't nlmsg_len just become 0
>> then?
> 
> The check would just abort the loop as soon a the lenght check
> triggers. I've tested the new version and nlmsg_len still got
> underflow but the macro aborts the loop now.

Right, that's the intent of NLMSG_OK and why it uses ints.

I still think the last message isn't *actually* aligned, but without 
crash data I can't prove it :)  It may be that most messages are indeed 
aligned, but the ones you're seeing this on are somehow special. 
Otherwise I don't see how we haven't detected this crash for so long.

> 
> Do you want me to send a new patch or are you going to fix it
> yourself? I don't mind :)

I went ahead and pushed 8b489d5df283 ("monitor: Fix crash") with your 
name on the Reported-By.

Regards,
-Denis

Re: [PATCH] monitor: Add message length check to nlmon_receice

[Daniel Wagner]

[-- Attachment #1: Type: text/plain, Size: 2557 bytes --]

On Wed, Jan 22, 2020 at 11:53:47AM -0600, Denis Kenzior wrote:
> I still think the last message isn't *actually* aligned, but without crash
> data I can't prove it :)  It may be that most messages are indeed aligned,
> but the ones you're seeing this on are somehow special. Otherwise I don't
> see how we haven't detected this crash for so long.

Let me help out with that :)

(gdb) bt full
#0  0x000000000041ba55 in nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at monitor/nlmon.c:6930
        nlmon = 0x4d4cc0
        nlmsg = 0x800000ffd2f4
        msg = {msg_name = 0x7fffffffe920, msg_namelen = 20, msg_iov = 0x7fffffffe910, msg_iovlen = 1, 
          msg_control = 0x7fffffffc8c0, msg_controllen = 32, msg_flags = 0}
        sll = {sll_family = 17, sll_protocol = 0, sll_ifindex = 6, sll_hatype = 824, sll_pkttype = 4 '\004', 
          sll_halen = 0 '\000', sll_addr = "\000\000\000\000\000\000\000"}
        iov = {iov_base = 0x7fffffffc8e0, iov_len = 8192}
        cmsg = 0x0
        copy_tv = {tv_sec = 1579718221, tv_usec = 657974}
        copy_tp = {tp_status = 0, tp_len = 0, tp_snaplen = 0, tp_mac = 0, tp_net = 0, tp_vlan_tci = 0, 
          tp_vlan_tpid = 0}
        tv = 0x7fffffffe900
        tp = 0x0
        proto_type = 0
        buf = "\021\000\000\000\026\000\001\003\000\000\000\000wU\000\000\000\200\200\000\376\t\000\001\000\000\000\000\b\000\017\000\376\000\000\000\024\000\001\000*\002&\360\000@\000\000\000\000\000\000\027\333[\033\024\000\002", '\000' <repeats 17 times>, "\024\000\a\000\376\200\000\000\000\000\000\000\342\325^\377\376\343\036\024\f\000\b\000\b\000\n\000@\000\000\000\b\000\006\000\000\004\000\000\024\000\005\000\376\200\000\000\000\000\000\000\362\237\302\377\376¬\337\b\000\004\000\002\000\000\000$\000\f\000\002\000\000\000\000\316\065", '\000' <repeats 25 times>, "\005\000\024\000\001\000\000\000\024\000\006\000\377\377\377\377\377\377\377\377\375"...
        control = " \000\000\000\000\000\000\000\001\000\000\000\035\000\000\000M\226(^\000\000\000\000\066\n\n\000\000\000\000"
        bytes_read = 17
        nlmsg_len = 18446744073692771837
        fd = 7
(gdb) p iov.iov_base
$1 = (void *) 0x7fffffffc8e0
(gdb) p *(struct nlmsghdr *)iov.iov_base
$2 = {nlmsg_len = 17, nlmsg_type = 22, nlmsg_flags = 769, nlmsg_seq = 0, nlmsg_pid = 21879}

> > Do you want me to send a new patch or are you going to fix it
> > yourself? I don't mind :)
> 
> I went ahead and pushed 8b489d5df283 ("monitor: Fix crash") with your name
> on the Reported-By.

Thanks!
Daniel