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

Message ID 1565952299-31241-1-git-send-email-dceara@redhat.com
State Accepted
Headers show
Series
  • [ovs-dev,ovn] pinctrl: Fix DNS packet parsing
Related show

Commit Message

Dumitru Ceara Aug. 16, 2019, 10:44 a.m. UTC
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>
---
 controller/pinctrl.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Numan Siddique Aug. 16, 2019, 12:31 p.m. UTC | #1
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
>
>
Dumitru Ceara Aug. 16, 2019, 2:04 p.m. UTC | #2
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
>>

Patch
diff mbox series

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