OpenBSD 6.1: mbuf leak after receiving icmp/ip-Messages with IP-Options

[Gerlach, Hendrik]

Synopsis: OpenBSD 6.1: mbuf leak after receiving icmp/ip-Messages with IP-Options
Category: system IP-Stack

System : OpenBSD 6.1
Details : OpenBSD 6.1 (GENERIC) #291: Sat Apr 1 13:49:08 MDT 2017
***@i386.openbsd.org:/usr/src/sys/arch/i386/compile/GENERIC

Architecture: OpenBSD.i386
Machine : i386
When receiving icmp messages (e.g. ICMP ping) with specific IP-Options set
in the IP-Header a mbuf is lost. It is requested from the mbuf pool but never put
back to the pool mbufpl. So, if sending enough packets, the system will run out of
mbufs not be able to communicate any longer.
This happens for example with the stream ID option.
Seems the problem was introduced in OpenBSD's 4.9 icmp_reflect ()
(File ip_icmp.c Rev. 1.92) by removing m_free(opts) and moving the mbuf pointer
for the ip-options ("opts") one level up to icmp_input() without freeing the mbuf
there after the call to icmp_send().
Send a ICMP pings with the IP-Option Stream-ID option. This can easily done using
the following scapy script:
#!/usr/bin/python
from scapy.all import *
destip = "192.168.10.61"
icmp_payload = 'X'*10
ip_options= IPOption('\x88\x04\x00\x10') # stream ID option

ip_packet = IP(dst=destip, options=ip_options)/ICMP()/icmp_payload
i=1000000L
while i > 0:
send(ip_packet)
i=i-1
A possible fix maybe would calling m_free(opts) (if opts != NULL) after calling icmp_send()
(not verified) or reverting the code to the ip_icmp revision before 1.92 (which would be
more compatible to FreeBSD, MacOS where this problem not happens - maybe the original
reason for changing this doesn't exist any longer).


Best regards,
Hendrik

[Alexander Bluhm]

Post by Gerlach, Hendrik
A possible fix maybe would calling m_free(opts) (if opts != NULL) after calling icmp_send()

As the options are not allocated in icmp_send() I think it is better
to free them in icmp_input_if() after a successful call to
icmp_reflect().

ok?

bluhm

Index: netinet/ip_icmp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_icmp.c,v
retrieving revision 1.167
diff -u -p -r1.167 ip_icmp.c
--- netinet/ip_icmp.c 4 May 2017 17:58:46 -0000 1.167
+++ netinet/ip_icmp.c 22 May 2017 13:25:37 -0000
@@ -591,8 +591,10 @@ reflect:

icmpstat_inc(icps_reflect);
icmpstat_inc(icps_outhist + icp->icmp_type);
- if (!icmp_reflect(m, &opts, NULL))
+ if (!icmp_reflect(m, &opts, NULL)) {
icmp_send(m, opts);
+ m_free(opts);
+ }
return IPPROTO_DONE;

case ICMP_REDIRECT:

[Claudio Jeker]

Post by Alexander Bluhm

Post by Gerlach, Hendrik
A possible fix maybe would calling m_free(opts) (if opts != NULL) after calling icmp_send()

As the options are not allocated in icmp_send() I think it is better
to free them in icmp_input_if() after a successful call to
icmp_reflect().
ok?
bluhm
Index: netinet/ip_icmp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_icmp.c,v
retrieving revision 1.167
diff -u -p -r1.167 ip_icmp.c
--- netinet/ip_icmp.c 4 May 2017 17:58:46 -0000 1.167
+++ netinet/ip_icmp.c 22 May 2017 13:25:37 -0000
icmpstat_inc(icps_reflect);
icmpstat_inc(icps_outhist + icp->icmp_type);
- if (!icmp_reflect(m, &opts, NULL))
+ if (!icmp_reflect(m, &opts, NULL)) {
icmp_send(m, opts);
+ m_free(opts);
+ }
return IPPROTO_DONE;

[Peter Hessler]

On 2017 May 22 (Mon) at 15:54:38 +0200 (+0200), Alexander Bluhm wrote:
:On Mon, May 22, 2017 at 11:33:08AM +0000, Gerlach, Hendrik wrote:
:> A possible fix maybe would calling m_free(opts) (if opts != NULL) after calling icmp_send()
:
:As the options are not allocated in icmp_send() I think it is better
:to free them in icmp_input_if() after a successful call to
:icmp_reflect().
:
:ok?
:

OK


:bluhm
:
:Index: netinet/ip_icmp.c
:===================================================================
:RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_icmp.c,v
:retrieving revision 1.167
:diff -u -p -r1.167 ip_icmp.c
:--- netinet/ip_icmp.c 4 May 2017 17:58:46 -0000 1.167
:+++ netinet/ip_icmp.c 22 May 2017 13:25:37 -0000
:@@ -591,8 +591,10 @@ reflect:
:
: icmpstat_inc(icps_reflect);
: icmpstat_inc(icps_outhist + icp->icmp_type);
:- if (!icmp_reflect(m, &opts, NULL))
:+ if (!icmp_reflect(m, &opts, NULL)) {
: icmp_send(m, opts);
:+ m_free(opts);
:+ }
: return IPPROTO_DONE;
:
: case ICMP_REDIRECT:
:

[Gerlach, Hendrik]

Yes, of course. This is the right place in my opinion too.
Hendrik


-----Ursprüngliche Nachricht-----
Von: Alexander Bluhm [mailto:***@gmx.net]
Gesendet: Montag, 22. Mai 2017 15:55
An: Gerlach, Hendrik (DF FA AS DH FTH 6)
Cc: ***@openbsd.org
Betreff: Re: OpenBSD 6.1: mbuf leak after receiving icmp/ip-Messages with IP-Options

Post by Gerlach, Hendrik
A possible fix maybe would calling m_free(opts) (if opts != NULL)
after calling icmp_send()

As the options are not allocated in icmp_send() I think it is better to free them in icmp_input_if() after a successful call to icmp_reflect().

ok?

bluhm

Index: netinet/ip_icmp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_icmp.c,v
retrieving revision 1.167
diff -u -p -r1.167 ip_icmp.c
--- netinet/ip_icmp.c 4 May 2017 17:58:46 -0000 1.167
+++ netinet/ip_icmp.c 22 May 2017 13:25:37 -0000
@@ -591,8 +591,10 @@ reflect:

icmpstat_inc(icps_reflect);
icmpstat_inc(icps_outhist + icp->icmp_type);
- if (!icmp_reflect(m, &opts, NULL))
+ if (!icmp_reflect(m, &opts, NULL)) {
icmp_send(m, opts);
+ m_free(opts);
+ }
return IPPROTO_DONE;

case ICMP_REDIRECT:

[Todd C. Miller]

Post by Alexander Bluhm
As the options are not allocated in icmp_send() I think it is better
to free them in icmp_input_if() after a successful call to
icmp_reflect().

I had the same diff. OK millert@

- todd