[ovs-dev,branch-2.12] pinctrl: Fix DNS packet parsing
diff mbox series

Message ID 1565965888-30467-1-git-send-email-dceara@redhat.com
State New
Headers show
Series
  • [ovs-dev,branch-2.12] pinctrl: Fix DNS packet parsing
Related show

Commit Message

Dumitru Ceara Aug. 16, 2019, 2:31 p.m. UTC
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(-)

Comments

Dumitru Ceara Aug. 16, 2019, 2:35 p.m. UTC | #1
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
0-day Robot Aug. 19, 2019, 3:19 p.m. UTC | #2
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
Dumitru Ceara Aug. 23, 2019, 7:12 a.m. UTC | #3
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
Ben Pfaff Aug. 28, 2019, 7:39 p.m. UTC | #4
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.
Dumitru Ceara Sept. 12, 2019, 7:38 a.m. UTC | #5
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!

Patch
diff mbox series

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