[Qemu-devel] [PATCH 0/2] slirp: Fix heap buffer overflow during packet reassembly (CVE-2019-14378)

[Qemu-devel] [PATCH 0/2] slirp: Fix heap buffer overflow during packet reassembly (CVE-2019-14378)

[Philippe Mathieu-Daudé]

Hi, this is a SLiRP bug, however as QEMU comsumes the library, it is
directly concerned, so I'm cross-posting both QEMU and SLiRP lists
for a stricter review.

The 2nd patch is a PoC, to give QEMU a chance to shutdown properly
if SLiRP internals get corrupted. Simply sent as RFC.

The vulnerability is very well described here:
https://vishnudevtj.github.io/notes/qemu-vm-escape-cve-2019-14378

Exploit (used as reproducer):
https://github.com/vishnudevtj/exploits/blob/master/qemu/CVE-2019-14378/exp.c

Philippe Mathieu-Daudé (2):
  Do not reassemble fragments pointing outside of the original payload
  RFC Delay crash when mbufs are corrupted

 src/ip_input.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

-- 
2.20.1

[Qemu-devel] [PATCH 1/2] Do not reassemble fragments pointing outside of the original payload

[Philippe Mathieu-Daudé]

The vulnerability CVE-2019-14378 is well explained in [1]:

  The bug is triggered when large IPv4 fragmented packets are
  reassembled for processing.

  For the NAT translation if the incoming packets are fragmented
  they should be reassembled before they are edited and
  re-transmitted.
  This reassembly is done by the
  ip_reass(Slirp *slirp, struct ip *ip, struct ipq *fp) function.
  ip contains the current IP packet data, fp is a link list
  containing the fragmented packets.

  ip_reass() does the following:

  * If first fragment to arrive (fp==NULL), create a reassembly
    queue and insert ip into this queue.
  * Check if the fragment is overlapping with previous received
    fragments, then discard it.
  * If all the fragmented packets are received reassemble it.
    Create header for new ip packet by modifying header of first
    packet.

  The bug is at the calculation of the variable delta. The code
  assumes that the first fragmented packet will not be allocated in
  the external buffer (m_ext). The calculation q - m->dat is valid
  when the packet data is inside mbuf->m_dat (q will be inside m_dat)
  (q is structure containing link list of fragments and packet data).
  Otherwise if m_ext buffer was allocated, then q will be inside the
  external buffer and the calculation of the delta will be wrong.

  Later the newly calculated pointer q is converted into ip structure
  and values are modified, Due to the wrong calculation of the delta,
  ip will be pointing to incorrect location and ip_src and ip_dst can
  be used to write controlled data onto the calculated location. This
  may also crash qemu if the calculated ip is located in unmaped area.

Do not queue fragments pointing out of the original payload to avoid
to calculate the variable delta.

[1] https://vishnudevtj.github.io/notes/qemu-vm-escape-cve-2019-14378

Fixes: CVE-2019-14378
Reported-by: Vishnu Dev TJ <vishnudevtj@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 src/ip_input.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/ip_input.c b/src/ip_input.c
index 7364ce0..ee52085 100644
--- a/src/ip_input.c
+++ b/src/ip_input.c
@@ -304,6 +304,19 @@ static struct ip *ip_reass(Slirp *slirp, struct ip *ip, struct ipq *fp)
         ip_deq(q->ipf_prev);
     }
 
+    /*
+     * If we received the first fragment, we know the original
+     * payload size. Verify fragments are within our payload.
+     */
+    for (q = fp->frag_link.next; q != (struct ipasfrag*)&fp->frag_link;
+            q = q->ipf_next) {
+        if (!q->ipf_off && q->ipf_len) {
+            if (ip->ip_off + ip->ip_len >= q->ipf_len) {
+                goto dropfrag;
+            }
+        }
+    }
+
 insert:
     /*
      * Stick new segment in its place;
-- 
2.20.1

Re: [Qemu-devel] [Slirp] [PATCH 1/2] Do not reassemble fragments pointing outside of the original payload

[Samuel Thibault]

Hello,

Philippe Mathieu-Daudé, le jeu. 22 août 2019 16:41:33 +0200, a ecrit:
>   Later the newly calculated pointer q is converted into ip structure
>   and values are modified, Due to the wrong calculation of the delta,
>   ip will be pointing to incorrect location and ip_src and ip_dst can
>   be used to write controlled data onto the calculated location. This
>   may also crash qemu if the calculated ip is located in unmaped area.

That does not seem to be related to this:

> Do not queue fragments pointing out of the original payload to avoid
> to calculate the variable delta.

I don't understand the relation with having to calculate delta.

> diff --git a/src/ip_input.c b/src/ip_input.c
> index 7364ce0..ee52085 100644
> --- a/src/ip_input.c
> +++ b/src/ip_input.c
> @@ -304,6 +304,19 @@ static struct ip *ip_reass(Slirp *slirp, struct ip *ip, struct ipq *fp)
>          ip_deq(q->ipf_prev);
>      }
>  
> +    /*
> +     * If we received the first fragment, we know the original
> +     * payload size.

? We only know the total payload size when receiving the last fragment
(payload = offset*8 + size).

> Verify fragments are within our payload.

By construction of the protocol, fragments can only be within the
payload, since it's the last fragment which provides the payload size.

> +    for (q = fp->frag_link.next; q != (struct ipasfrag*)&fp->frag_link;
> +            q = q->ipf_next) {
> +        if (!q->ipf_off && q->ipf_len) {
> +            if (ip->ip_off + ip->ip_len >= q->ipf_len) {
> +                goto dropfrag;
> +            }
> +        }
> +    }

Fragments are kept in order, there is no need to go around the list to
find the fragment with offset zero, if it is there it is the first one.

Did you make your test with commit 126c04acbabd ("Fix heap overflow in
ip_reass on big packet input") applied?

Samuel

Re: [Qemu-devel] [Slirp] [PATCH 1/2] Do not reassemble fragments pointing outside of the original payload

[Philippe Mathieu-Daudé]

On 8/22/19 8:33 PM, Samuel Thibault wrote:
> Philippe Mathieu-Daudé, le jeu. 22 août 2019 16:41:33 +0200, a ecrit:
>>   Later the newly calculated pointer q is converted into ip structure
>>   and values are modified, Due to the wrong calculation of the delta,
>>   ip will be pointing to incorrect location and ip_src and ip_dst can
>>   be used to write controlled data onto the calculated location. This
>>   may also crash qemu if the calculated ip is located in unmaped area.
> 
> That does not seem to be related to this:

Indeed, I wonder if this is the same issue reported in the CVE.

>> Do not queue fragments pointing out of the original payload to avoid
>> to calculate the variable delta.
> 
> I don't understand the relation with having to calculate delta.
> 
>> diff --git a/src/ip_input.c b/src/ip_input.c
>> index 7364ce0..ee52085 100644
>> --- a/src/ip_input.c
>> +++ b/src/ip_input.c
>> @@ -304,6 +304,19 @@ static struct ip *ip_reass(Slirp *slirp, struct ip *ip, struct ipq *fp)
>>          ip_deq(q->ipf_prev);
>>      }
>>  
>> +    /*
>> +     * If we received the first fragment, we know the original
>> +     * payload size.
> 
> ? We only know the total payload size when receiving the last fragment
> (payload = offset*8 + size).
> 
>> Verify fragments are within our payload.
> 
> By construction of the protocol, fragments can only be within the
> payload, since it's the last fragment which provides the payload size.

I might have misunderstood the RFC, I'll read it again.

>> +    for (q = fp->frag_link.next; q != (struct ipasfrag*)&fp->frag_link;
>> +            q = q->ipf_next) {
>> +        if (!q->ipf_off && q->ipf_len) {
>> +            if (ip->ip_off + ip->ip_len >= q->ipf_len) {
>> +                goto dropfrag;
>> +            }
>> +        }
>> +    }
> 
> Fragments are kept in order, there is no need to go around the list to
> find the fragment with offset zero, if it is there it is the first one.

OK.

> Did you make your test with commit 126c04acbabd ("Fix heap overflow in
> ip_reass on big packet input") applied?

Yes, unfortunately it doesn't fix the issue.

Thanks,

Phil.

Re: [Qemu-devel] [Slirp] [PATCH 1/2] Do not reassemble fragments pointing outside of the original payload

[Samuel Thibault]

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

Hello,

Philippe Mathieu-Daudé, le ven. 23 août 2019 17:15:32 +0200, a ecrit:
> > Did you make your test with commit 126c04acbabd ("Fix heap overflow in
> > ip_reass on big packet input") applied?
> 
> Yes, unfortunately it doesn't fix the issue.

Ok.

Could you try the attached patch?  There was a use-after-free.  Without
it, I can indeed crash qemu with the given exploit.  With it I don't
seem to be able to crash it (trying in a loop for several minutes).

Samuel

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 828 bytes --]

diff --git a/src/ip_input.c b/src/ip_input.c
index 7364ce0..aa514ae 100644
--- a/src/ip_input.c
+++ b/src/ip_input.c
@@ -292,6 +292,7 @@ static struct ip *ip_reass(Slirp *slirp, struct ip *ip, struct ipq *fp)
      */
     while (q != (struct ipasfrag *)&fp->frag_link &&
            ip->ip_off + ip->ip_len > q->ipf_off) {
+        struct ipasfrag *prev;
         i = (ip->ip_off + ip->ip_len) - q->ipf_off;
         if (i < q->ipf_len) {
             q->ipf_len -= i;
@@ -299,9 +300,10 @@ static struct ip *ip_reass(Slirp *slirp, struct ip *ip, struct ipq *fp)
             m_adj(dtom(slirp, q), i);
             break;
         }
+        prev = q;
         q = q->ipf_next;
-        m_free(dtom(slirp, q->ipf_prev));
-        ip_deq(q->ipf_prev);
+        ip_deq(prev);
+        m_free(dtom(slirp, prev));
     }
 
 insert:

Re: [Qemu-devel] [Slirp] [PATCH 1/2] Do not reassemble fragments pointing outside of the original payload

[P J P]

+-- On Mon, 26 Aug 2019, Samuel Thibault wrote --+
| Philippe Mathieu-Daudé, le ven. 23 août 2019 17:15:32 +0200, a ecrit:
| > > Did you make your test with commit 126c04acbabd ("Fix heap overflow in
| > > ip_reass on big packet input") applied?
| > 
| > Yes, unfortunately it doesn't fix the issue.
| 
| Ok.
| 
| Could you try the attached patch?  There was a use-after-free.  Without
| it, I can indeed crash qemu with the given exploit.  With it I don't
| seem to be able to crash it (trying in a loop for several minutes).

Considering that earlier fix was released/pulled into upstream QEMU v4.1.0, we 
need to treat this one as a separate issue.

   commit c59279437eda91841b9d26079c70b8a540d41204
   Author: Samuel Thibault <samuel.thibault@ens-lyon.org>
   Date:   Mon Aug 26 00:55:03 2019 +0200

   ip_reass: Fix use after free
   
   Using ip_deq after m_free might read pointers from an allocation reuse.

I'll follow-up on that.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [Slirp] [PATCH 1/2] Do not reassemble fragments pointing outside of the original payload

[Philippe Mathieu-Daudé]

Hi Samuel,

On 8/26/19 12:54 AM, Samuel Thibault wrote:
> Hello,
> 
> Philippe Mathieu-Daudé, le ven. 23 août 2019 17:15:32 +0200, a ecrit:
>>> Did you make your test with commit 126c04acbabd ("Fix heap overflow in
>>> ip_reass on big packet input") applied?
>>
>> Yes, unfortunately it doesn't fix the issue.
> 
> Ok.
> 
> Could you try the attached patch?  There was a use-after-free.  Without
> it, I can indeed crash qemu with the given exploit.  With it I don't
> seem to be able to crash it (trying in a loop for several minutes).

No change with your patch applied:

Thread 4 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffe94c4700 (LWP 14031)]
0x0000555555e835c5 in icmp_input (m=0x0, hlen=20) at
qemu/slirp/src/ip_icmp.c:130
130         register struct ip *ip = mtod(m, struct ip *);
(gdb) bt
#0  0x0000555555e835c5 in icmp_input (m=0x0, hlen=20) at
qemu/slirp/src/ip_icmp.c:130
#1  0x0000555555e85450 in ip_input (m=0x0) at qemu/slirp/src/ip_input.c:205
#2  0x0000555555e723d2 in slirp_input (slirp=0x555556708170,
pkt=0x55555727fab0 "", pkt_len=1314) at qemu/slirp/src/slirp.c:785
#3  0x0000555555c83961 in net_slirp_receive (nc=0x555556707fa0,
buf=0x55555727fab0 "", size=1314) at qemu/net/slirp.c:126
#4  0x0000555555c788cb in nc_sendv_compat (nc=0x555556707fa0,
iov=0x7fffe94c0930, iovcnt=1, flags=0) at qemu/net/net.c:700
#5  0x0000555555c7898d in qemu_deliver_packet_iov
(sender=0x5555566a6440, flags=0, iov=0x7fffe94c0930, iovcnt=1,
opaque=0x555556707fa0) at qemu/net/net.c:728
#6  0x0000555555c7b49d in qemu_net_queue_deliver_iov
(queue=0x5555566a6260, sender=0x5555566a6440, flags=0,
iov=0x7fffe94c0930, iovcnt=1) at qemu/net/queue.c:179
#7  0x0000555555c7b60c in qemu_net_queue_send_iov (queue=0x5555566a6260,
sender=0x5555566a6440, flags=0, iov=0x7fffe94c0930, iovcnt=1,
sent_cb=0x0) at qemu/net/queue.c:224
#8  0x0000555555c78ad2 in qemu_sendv_packet_async
(sender=0x5555566a6440, iov=0x7fffe94c0930, iovcnt=1, sent_cb=0x0) at
qemu/net/net.c:769
#9  0x0000555555c78aff in qemu_sendv_packet (nc=0x5555566a6440,
iov=0x7fffe94c0930, iovcnt=1) at qemu/net/net.c:777
#10 0x0000555555c7c038 in net_hub_receive_iov (hub=0x5555566b1ab0,
source_port=0x5555566a67a0, iov=0x7fffe94c0930, iovcnt=1) at
qemu/net/hub.c:74
#11 0x0000555555c7c232 in net_hub_port_receive_iov (nc=0x5555566a67a0,
iov=0x7fffe94c0930, iovcnt=1) at qemu/net/hub.c:125
#12 0x0000555555c78972 in qemu_deliver_packet_iov
(sender=0x555557292860, flags=0, iov=0x7fffe94c0930, iovcnt=1,
opaque=0x5555566a67a0) at qemu/net/net.c:726
#13 0x0000555555c7b421 in qemu_net_queue_deliver (queue=0x5555566a6940,
sender=0x555557292860, flags=0, data=0x55555727fab0 "", size=1314) at
qemu/net/queue.c:164
#14 0x0000555555c7b53d in qemu_net_queue_send (queue=0x5555566a6940,
sender=0x555557292860, flags=0, data=0x55555727fab0 "", size=1314,
sent_cb=0x0) at qemu/net/queue.c:199
#15 0x0000555555c78733 in qemu_send_packet_async_with_flags
(sender=0x555557292860, flags=0, buf=0x55555727fab0 "", size=1314,
sent_cb=0x0) at qemu/net/net.c:654
#16 0x0000555555c7876b in qemu_send_packet_async (sender=0x555557292860,
buf=0x55555727fab0 "", size=1314, sent_cb=0x0) at qemu/net/net.c:661
#17 0x0000555555c78798 in qemu_send_packet (nc=0x555557292860,
buf=0x55555727fab0 "", size=1314) at qemu/net/net.c:667
#18 0x0000555555b32b67 in e1000_send_packet (s=0x55555725ce00,
buf=0x55555727fab0 "", size=1314) at qemu/hw/net/e1000.c:552
#19 0x0000555555b32fd3 in xmit_seg (s=0x55555725ce00) at
qemu/hw/net/e1000.c:615
#20 0x0000555555b33503 in process_tx_desc (s=0x55555725ce00,
dp=0x7fffe94c0b70) at qemu/hw/net/e1000.c:702
#21 0x0000555555b336fb in start_xmit (s=0x55555725ce00) at
qemu/hw/net/e1000.c:757
#22 0x0000555555b347b5 in set_tctl (s=0x55555725ce00, index=3590, val=8)
at qemu/hw/net/e1000.c:1128
#23 0x0000555555b34932 in e1000_mmio_write (opaque=0x55555725ce00,
addr=14360, val=8, size=4) at qemu/hw/net/e1000.c:1304
#24 0x000055555585b126 in memory_region_write_accessor
(mr=0x55555725f700, addr=14360, value=0x7fffe94c0cd8, size=4, shift=0,
mask=4294967295, attrs=...) at qemu/memory.c:507
#25 0x000055555585b336 in access_with_adjusted_size (addr=14360,
value=0x7fffe94c0cd8, size=4, access_size_min=4, access_size_max=4,
access_fn=0x55555585b03d <memory_region_write_accessor>,
mr=0x55555725f700, attrs=...)
    at qemu/memory.c:573
#26 0x000055555585e315 in memory_region_dispatch_write
(mr=0x55555725f700, addr=14360, data=8, size=4, attrs=...) at
qemu/memory.c:1509
#27 0x00005555557fcee2 in flatview_write_continue (fv=0x7fffe02307f0,
addr=4273747992, attrs=..., buf=0x7ffff7fcb028 "\b", len=4, addr1=14360,
l=4, mr=0x55555725f700) at qemu/exec.c:3367
#28 0x00005555557fd027 in flatview_write (fv=0x7fffe02307f0,
addr=4273747992, attrs=..., buf=0x7ffff7fcb028 "\b", len=4) at
qemu/exec.c:3406
#29 0x00005555557fd32c in address_space_write (as=0x55555641e640
<address_space_memory>, addr=4273747992, attrs=..., buf=0x7ffff7fcb028
"\b", len=4) at qemu/exec.c:3496
#30 0x00005555557fd37e in address_space_rw (as=0x55555641e640
<address_space_memory>, addr=4273747992, attrs=..., buf=0x7ffff7fcb028
"\b", len=4, is_write=true) at qemu/exec.c:3507
#31 0x0000555555876629 in kvm_cpu_exec (cpu=0x55555670e860) at
qemu/accel/kvm/kvm-all.c:2288
#32 0x000055555584c1d8 in qemu_kvm_cpu_thread_fn (arg=0x55555670e860) at
qemu/cpus.c:1290
#33 0x0000555555e48991 in qemu_thread_start (args=0x5555567328a0) at
qemu/util/qemu-thread-posix.c:502

Note 1: To trigger this I have to build with:

   ./configure --extra-cflags=-ggdb --enable-debug --enable-sanitizers

Using different combinations I can not reproduce the crash.

Note 2: We miss some Makefile rules in QEMU with the libslirp split.

Checkouting branches in the slirp/ directory doesn't trigger recompiling
the slirp object, and even if I force the creation of the libslirp.a
archive, the QEMU binaries are not linked again with the refreshed archive.

Re: [Qemu-devel] [Slirp] [PATCH 1/2] Do not reassemble fragments pointing outside of the original payload

[Philippe Mathieu-Daudé]

On 8/29/19 5:43 PM, Philippe Mathieu-Daudé wrote:
> On 8/26/19 12:54 AM, Samuel Thibault wrote:
>> Philippe Mathieu-Daudé, le ven. 23 août 2019 17:15:32 +0200, a ecrit:
>>>> Did you make your test with commit 126c04acbabd ("Fix heap overflow in
>>>> ip_reass on big packet input") applied?
>>>
>>> Yes, unfortunately it doesn't fix the issue.
>>
>> Ok.
>>
>> Could you try the attached patch?  There was a use-after-free.  Without
>> it, I can indeed crash qemu with the given exploit.  With it I don't
>> seem to be able to crash it (trying in a loop for several minutes).
[...]
> 
> Note 2: We miss some Makefile rules in QEMU with the libslirp split.
> 
> Checkouting branches in the slirp/ directory doesn't trigger recompiling
> the slirp object, and even if I force the creation of the libslirp.a
> archive, the QEMU binaries are not linked again with the refreshed archive.

And I hit the same issue after applying your patch =)

So, using a clean workspace, I can not reproduce the null deref anymore.

If you send a proper patch, feel free to add:

Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!

Phil.

Re: [Qemu-devel] [Slirp] [PATCH 1/2] Do not reassemble fragments pointing outside of the original payload

[Philippe Mathieu-Daudé]

On 8/29/19 5:53 PM, Philippe Mathieu-Daudé wrote:
> On 8/29/19 5:43 PM, Philippe Mathieu-Daudé wrote:
>> On 8/26/19 12:54 AM, Samuel Thibault wrote:
>>> Philippe Mathieu-Daudé, le ven. 23 août 2019 17:15:32 +0200, a ecrit:
>>>>> Did you make your test with commit 126c04acbabd ("Fix heap overflow in
>>>>> ip_reass on big packet input") applied?
>>>>
>>>> Yes, unfortunately it doesn't fix the issue.
>>>
>>> Ok.
>>>
>>> Could you try the attached patch?  There was a use-after-free.  Without
>>> it, I can indeed crash qemu with the given exploit.  With it I don't
>>> seem to be able to crash it (trying in a loop for several minutes).
> [...]
>>
>> Note 2: We miss some Makefile rules in QEMU with the libslirp split.
>>
>> Checkouting branches in the slirp/ directory doesn't trigger recompiling
>> the slirp object, and even if I force the creation of the libslirp.a
>> archive, the QEMU binaries are not linked again with the refreshed archive.
> 
> And I hit the same issue after applying your patch =)
> 
> So, using a clean workspace, I can not reproduce the null deref anymore.
> 
> If you send a proper patch, feel free to add:
> 
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

I was going to suggest to also add:

  Reported-by: Quan Wenli <wquan@redhat.com>

But you answered in another thread that this patch was already commited
3 days ago as:

https://gitlab.freedesktop.org/slirp/libslirp/commit/d203c81b

[Qemu-devel] [RFC PATCH 2/2] Delay crash when mbufs are corrupted

[Philippe Mathieu-Daudé]

If the mbufs linked list is corrupted, give the application
a chance to properly shutdown itself without causing dataloss.

For now, this means slowly leaking the heap.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 src/ip_input.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/ip_input.c b/src/ip_input.c
index ee52085..36962b5 100644
--- a/src/ip_input.c
+++ b/src/ip_input.c
@@ -185,6 +185,15 @@ void ip_input(struct mbuf *m)
             if (ip == NULL)
                 return;
             m = dtom(slirp, ip);
+            if (m == NULL) {
+                /* The mbufs linked list is corrupted, we
+                 * should properly shutdown the application.
+                 * We prefer to leak a mbuf on the heap
+                 * rather than allowing null dereference
+                 * and eventual crash (data corruption).
+                 */
+                return; /* TODO warn the library consumer! */
+            }
         } else if (fp)
             ip_freef(slirp, fp);
 
-- 
2.20.1