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

[Denis Kenzior <denkenz@gmail.com>]

[-- 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