| Message ID | 20190822144134.23521-2-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | slirp: Fix heap buffer overflow during packet reassembly (CVE-2019-14378) | expand |
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
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.
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 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:
+-- 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
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.
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.
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
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;
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(+)