| Message ID | 1565952299-31241-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn] pinctrl: Fix DNS packet parsing | expand |
On Fri, Aug 16, 2019 at 4:15 PM Dumitru Ceara <dceara@redhat.com> wrote: > Due to the use of a uint8_t to index inside the DNS payload we could end > up in an infinite loop when specific (invalid) DNS packets were > processed by ovn-controller. In the infinite loop we keep increasing the > query_name dynamic string until running out of memory. > > One way to replicate the issue is to configure DNS on the logical switch > and then inject a manually crafted DNS-like packet. For example, with > Scapy: > > >>> p = IP(dst='10.0.0.2',src='10.0.0.3')/UDP(dport=53)/('a'*364) > >>> send(p) > > Also add a sanity check on minimum L4 size of packets. > > CC: Numan Siddique <nusiddiq@redhat.com> > Fixes: 16cb4fb8ca49 ("ovn-controller: Add 'dns_lookup' action") > Reported-at: https://bugzilla.redhat.com/1740335 > Reported-by: Priscila <pveiga@redhat.com> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > Thanks for the fix Dumitru. I applied this patch to master. Can you please backport it to 2.12 (and other branches if you think it's required). Thanks Numan > controller/pinctrl.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index f27718f..365a0d1 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -1628,6 +1628,12 @@ pinctrl_handle_dns_lookup( > goto exit; > } > > + /* Check that the packet stores at least the minimal headers. */ > + if (dp_packet_l4_size(pkt_in) < (UDP_HEADER_LEN + DNS_HEADER_LEN)) { > + VLOG_WARN_RL(&rl, "truncated dns packet"); > + goto exit; > + } > + > /* Extract the DNS header */ > struct dns_header const *in_dns_header = > dp_packet_get_udp_payload(pkt_in); > if (!in_dns_header) { > @@ -1652,7 +1658,7 @@ pinctrl_handle_dns_lookup( > uint8_t *end = (uint8_t *)in_udp + MIN(udp_len, l4_len); > uint8_t *in_dns_data = (uint8_t *)(in_dns_header + 1); > uint8_t *in_queryname = in_dns_data; > - uint8_t idx = 0; > + uint16_t idx = 0; > struct ds query_name; > ds_init(&query_name); > /* Extract the query_name. If the query name is - 'www.ovn.org' it > would be > -- > 1.8.3.1 > >
On Fri, Aug 16, 2019 at 2:31 PM Numan Siddique <nusiddiq@redhat.com> wrote: > > > > On Fri, Aug 16, 2019 at 4:15 PM Dumitru Ceara <dceara@redhat.com> wrote: >> >> Due to the use of a uint8_t to index inside the DNS payload we could end >> up in an infinite loop when specific (invalid) DNS packets were >> processed by ovn-controller. In the infinite loop we keep increasing the >> query_name dynamic string until running out of memory. >> >> One way to replicate the issue is to configure DNS on the logical switch >> and then inject a manually crafted DNS-like packet. For example, with >> Scapy: >> >> >>> p = IP(dst='10.0.0.2',src='10.0.0.3')/UDP(dport=53)/('a'*364) >> >>> send(p) >> >> Also add a sanity check on minimum L4 size of packets. >> >> CC: Numan Siddique <nusiddiq@redhat.com> >> Fixes: 16cb4fb8ca49 ("ovn-controller: Add 'dns_lookup' action") >> Reported-at: https://bugzilla.redhat.com/1740335 >> Reported-by: Priscila <pveiga@redhat.com> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- > > > Thanks for the fix Dumitru. I applied this patch to master. > Can you please backport it to 2.12 (and other branches if you think it's required). > > Thanks > Numan Thanks for applying this. Yes, this should be backported at least down to 2.9. I'll send backport patches soon. Cheers, Dumitru > >> >> controller/pinctrl.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/controller/pinctrl.c b/controller/pinctrl.c >> index f27718f..365a0d1 100644 >> --- a/controller/pinctrl.c >> +++ b/controller/pinctrl.c >> @@ -1628,6 +1628,12 @@ pinctrl_handle_dns_lookup( >> goto exit; >> } >> >> + /* Check that the packet stores at least the minimal headers. */ >> + if (dp_packet_l4_size(pkt_in) < (UDP_HEADER_LEN + DNS_HEADER_LEN)) { >> + VLOG_WARN_RL(&rl, "truncated dns packet"); >> + goto exit; >> + } >> + >> /* Extract the DNS header */ >> struct dns_header const *in_dns_header = dp_packet_get_udp_payload(pkt_in); >> if (!in_dns_header) { >> @@ -1652,7 +1658,7 @@ pinctrl_handle_dns_lookup( >> uint8_t *end = (uint8_t *)in_udp + MIN(udp_len, l4_len); >> uint8_t *in_dns_data = (uint8_t *)(in_dns_header + 1); >> uint8_t *in_queryname = in_dns_data; >> - uint8_t idx = 0; >> + uint16_t idx = 0; >> struct ds query_name; >> ds_init(&query_name); >> /* Extract the query_name. If the query name is - 'www.ovn.org' it would be >> -- >> 1.8.3.1 >>
diff --git a/controller/pinctrl.c b/controller/pinctrl.c index f27718f..365a0d1 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -1628,6 +1628,12 @@ pinctrl_handle_dns_lookup( goto exit; } + /* Check that the packet stores at least the minimal headers. */ + if (dp_packet_l4_size(pkt_in) < (UDP_HEADER_LEN + DNS_HEADER_LEN)) { + VLOG_WARN_RL(&rl, "truncated dns packet"); + goto exit; + } + /* Extract the DNS header */ struct dns_header const *in_dns_header = dp_packet_get_udp_payload(pkt_in); if (!in_dns_header) { @@ -1652,7 +1658,7 @@ pinctrl_handle_dns_lookup( uint8_t *end = (uint8_t *)in_udp + MIN(udp_len, l4_len); uint8_t *in_dns_data = (uint8_t *)(in_dns_header + 1); uint8_t *in_queryname = in_dns_data; - uint8_t idx = 0; + uint16_t idx = 0; struct ds query_name; ds_init(&query_name); /* Extract the query_name. If the query name is - 'www.ovn.org' it would be