* [PATCH] monitor: Add message length check to nlmon_receice
@ 2020-01-19 16:31 Daniel Wagner
2020-01-21 22:36 ` Denis Kenzior
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Wagner @ 2020-01-19 16:31 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] monitor: Add message length check to nlmon_receice
2020-01-19 16:31 [PATCH] monitor: Add message length check to nlmon_receice Daniel Wagner
@ 2020-01-21 22:36 ` Denis Kenzior
2020-01-22 9:05 ` Daniel Wagner
0 siblings, 1 reply; 7+ messages in thread
From: Denis Kenzior @ 2020-01-21 22:36 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] monitor: Add message length check to nlmon_receice
2020-01-21 22:36 ` Denis Kenzior
@ 2020-01-22 9:05 ` Daniel Wagner
2020-01-22 16:21 ` Denis Kenzior
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Wagner @ 2020-01-22 9:05 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] monitor: Add message length check to nlmon_receice
2020-01-22 9:05 ` Daniel Wagner
@ 2020-01-22 16:21 ` Denis Kenzior
2020-01-22 17:26 ` Daniel Wagner
0 siblings, 1 reply; 7+ messages in thread
From: Denis Kenzior @ 2020-01-22 16:21 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] monitor: Add message length check to nlmon_receice
2020-01-22 16:21 ` Denis Kenzior
@ 2020-01-22 17:26 ` Daniel Wagner
2020-01-22 17:53 ` Denis Kenzior
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Wagner @ 2020-01-22 17:26 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] monitor: Add message length check to nlmon_receice
2020-01-22 17:26 ` Daniel Wagner
@ 2020-01-22 17:53 ` Denis Kenzior
2020-01-22 18:41 ` Daniel Wagner
0 siblings, 1 reply; 7+ messages in thread
From: Denis Kenzior @ 2020-01-22 17:53 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] monitor: Add message length check to nlmon_receice
2020-01-22 17:53 ` Denis Kenzior
@ 2020-01-22 18:41 ` Daniel Wagner
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Wagner @ 2020-01-22 18:41 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-01-22 18:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-19 16:31 [PATCH] monitor: Add message length check to nlmon_receice Daniel Wagner
2020-01-21 22:36 ` Denis Kenzior
2020-01-22 9:05 ` Daniel Wagner
2020-01-22 16:21 ` Denis Kenzior
2020-01-22 17:26 ` Daniel Wagner
2020-01-22 17:53 ` Denis Kenzior
2020-01-22 18:41 ` Daniel Wagner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.