| Message ID | 1565965888-30467-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,branch-2.12] pinctrl: Fix DNS packet parsing | expand |
On Fri, Aug 16, 2019 at 4:32 PM Dumitru Ceara <dceara@redhat.com> wrote: > > From: Dumitru Ceara <dceara@redhat.com> > > 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> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > > (cherry-picked from ovn commit - 7fbdeaade826da299c20c05050627ebea65fe8c2) > --- > ovn/controller/pinctrl.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c > index e443449..e388bbc 100644 > --- a/ovn/controller/pinctrl.c > +++ b/ovn/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 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Hi, Please backport this to 2.11, 2.10, 2.9 at least. Thanks, Dumitru
Bleep bloop. Greetings Dumitru Ceara, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Numan Siddique <nusiddiq@redhat.com> Lines checked: 60, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On Fri, Aug 16, 2019 at 4:35 PM Dumitru Ceara <dceara@redhat.com> wrote: > > On Fri, Aug 16, 2019 at 4:32 PM Dumitru Ceara <dceara@redhat.com> wrote: > > > > From: Dumitru Ceara <dceara@redhat.com> > > > > 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> > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > > > > (cherry-picked from ovn commit - 7fbdeaade826da299c20c05050627ebea65fe8c2) > > --- > > ovn/controller/pinctrl.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c > > index e443449..e388bbc 100644 > > --- a/ovn/controller/pinctrl.c > > +++ b/ovn/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 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > Hi, > > Please backport this to 2.11, 2.10, 2.9 at least. > > Thanks, > Dumitru Hi Ben, It would be great if we could have this bug fix in the 2.12 branch before the release. The fix is already in OVN master. Thanks, Dumitru
On Fri, Aug 16, 2019 at 04:35:52PM +0200, Dumitru Ceara wrote:
> Please backport this to 2.11, 2.10, 2.9 at least.
Done.
On Wed, Aug 28, 2019 at 9:39 PM Ben Pfaff <blp@ovn.org> wrote: > > On Fri, Aug 16, 2019 at 04:35:52PM +0200, Dumitru Ceara wrote: > > Please backport this to 2.11, 2.10, 2.9 at least. > > Done. Thank you!
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c index e443449..e388bbc 100644 --- a/ovn/controller/pinctrl.c +++ b/ovn/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