diff mbox series

[ovs-dev] conntrack: Correct length check for tcp packet inside ICMP data.

Message ID 1566566417-17473-1-git-send-email-vishal.deep.ajmera@ericsson.com
State Superseded
Headers show
Series [ovs-dev] conntrack: Correct length check for tcp packet inside ICMP data. | expand

Commit Message

Vishal Deep Ajmera Aug. 23, 2019, 1:20 p.m. UTC 
An ICMP packet with type destination or host not reachable also carries
28 bytes of ICMP data field. This data field contains IP header and TCP
header (partial first 8 bytes) of the original packet for which ICMP
is being generated.

Conntrack module when processing these ICMP packets checks for TCP header
length (20 bytes). Since TCP header is partial the length check fails and
packet is erroneously dropped.

This patch fixes length check for TCP header when processing ICMP data
fields.

Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
---
 lib/conntrack.c | 14 +++++++++++---
 lib/packets.h   |  1 +
 2 files changed, 12 insertions(+), 3 deletions(-)

Comments

Darrell Ball Aug. 23, 2019, 4:09 p.m. UTC | #1 
Thanks for the patch

Goes back to release 2.6/day one :-).

I'll provide more feedback after today.

On Fri, Aug 23, 2019 at 6:20 AM Vishal Deep Ajmera <
vishal.deep.ajmera@ericsson.com> wrote:

> An ICMP packet with type destination or host not reachable also carries
> 28 bytes of ICMP data field. This data field contains IP header and TCP
> header (partial first 8 bytes) of the original packet for which ICMP
> is being generated.
>
> Conntrack module when processing these ICMP packets checks for TCP header
> length (20 bytes). Since TCP header is partial the length check fails and
> packet is erroneously dropped.
>
> This patch fixes length check for TCP header when processing ICMP data
> fields.
>
> Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
> ---
>  lib/conntrack.c | 14 +++++++++++---
>  lib/packets.h   |  1 +
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 5f60fea..0618fdd 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -1513,10 +1513,18 @@ check_l4_icmp6(const struct conn_key *key, const
> void *data, size_t size,
>      return validate_checksum ? checksum_valid(key, data, size, l3) : true;
>  }
>
> +/* If related is NULL, we are parsing nested TCP header  inside ICMP
> packet.
> + * Only 8 bytes of TCP header is required by RFC to be present in such
> case.
> + */
>  static inline bool
> -extract_l4_tcp(struct conn_key *key, const void *data, size_t size)
> +extract_l4_tcp(struct conn_key *key, const void *data, size_t size,
> +               bool *related)
>  {
> -    if (OVS_UNLIKELY(size < TCP_HEADER_LEN)) {
> +    if (!related) {
> +        if (size < ICMP_L4_DATA_LEN) {
> +            return false;
> +        }
> +    } else if (size < TCP_HEADER_LEN) {
>          return false;
>      }
>
> @@ -1750,7 +1758,7 @@ extract_l4(struct conn_key *key, const void *data,
> size_t size, bool *related,
>  {
>      if (key->nw_proto == IPPROTO_TCP) {
>          return (!related || check_l4_tcp(key, data, size, l3,
> -                validate_checksum)) && extract_l4_tcp(key, data, size);
> +              validate_checksum)) && extract_l4_tcp(key, data, size,
> related);
>      } else if (key->nw_proto == IPPROTO_UDP) {
>          return (!related || check_l4_udp(key, data, size, l3,
>                  validate_checksum)) && extract_l4_udp(key, data, size);
> diff --git a/lib/packets.h b/lib/packets.h
> index a4bee38..2bc65c9 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -886,6 +886,7 @@ struct tcp_header {
>      ovs_be16 tcp_urg;
>  };
>  BUILD_ASSERT_DECL(TCP_HEADER_LEN == sizeof(struct tcp_header));
> +#define ICMP_L4_DATA_LEN 8
>
>  /* Connection states.
>   *
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Vishal Deep Ajmera Aug. 26, 2019, 11:16 a.m. UTC | #2 
Hi Darrell,

Can we get this merged into master as well as backports to affected branches?

Warm Regards,
Vishal Ajmera
Darrell Ball Aug. 26, 2019, 4:20 p.m. UTC | #3 
On Fri, Aug 23, 2019 at 9:09 AM Darrell Ball <dlu998@gmail.com> wrote:

> Thanks for the patch
>
> Goes back to release 2.6/day one :-).
>
> I'll provide more feedback after today.
>

I sent an alternative patch here

https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/362013.html

pls take a look.


>
> On Fri, Aug 23, 2019 at 6:20 AM Vishal Deep Ajmera <
> vishal.deep.ajmera@ericsson.com> wrote:
>
>> An ICMP packet with type destination or host not reachable also carries
>> 28 bytes of ICMP data field. This data field contains IP header and TCP
>> header (partial first 8 bytes) of the original packet for which ICMP
>> is being generated.
>>
>> Conntrack module when processing these ICMP packets checks for TCP header
>> length (20 bytes). Since TCP header is partial the length check fails and
>> packet is erroneously dropped.
>>
>> This patch fixes length check for TCP header when processing ICMP data
>> fields.
>>
>> Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
>> ---
>>  lib/conntrack.c | 14 +++++++++++---
>>  lib/packets.h   |  1 +
>>  2 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index 5f60fea..0618fdd 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -1513,10 +1513,18 @@ check_l4_icmp6(const struct conn_key *key, const
>> void *data, size_t size,
>>      return validate_checksum ? checksum_valid(key, data, size, l3) :
>> true;
>>  }
>>
>> +/* If related is NULL, we are parsing nested TCP header  inside ICMP
>> packet.
>> + * Only 8 bytes of TCP header is required by RFC to be present in such
>> case.
>> + */
>>  static inline bool
>> -extract_l4_tcp(struct conn_key *key, const void *data, size_t size)
>> +extract_l4_tcp(struct conn_key *key, const void *data, size_t size,
>> +               bool *related)
>>  {
>> -    if (OVS_UNLIKELY(size < TCP_HEADER_LEN)) {
>> +    if (!related) {
>> +        if (size < ICMP_L4_DATA_LEN) {
>> +            return false;
>> +        }
>> +    } else if (size < TCP_HEADER_LEN) {
>>          return false;
>>      }
>>
>> @@ -1750,7 +1758,7 @@ extract_l4(struct conn_key *key, const void *data,
>> size_t size, bool *related,
>>  {
>>      if (key->nw_proto == IPPROTO_TCP) {
>>          return (!related || check_l4_tcp(key, data, size, l3,
>> -                validate_checksum)) && extract_l4_tcp(key, data, size);
>> +              validate_checksum)) && extract_l4_tcp(key, data, size,
>> related);
>>      } else if (key->nw_proto == IPPROTO_UDP) {
>>          return (!related || check_l4_udp(key, data, size, l3,
>>                  validate_checksum)) && extract_l4_udp(key, data, size);
>> diff --git a/lib/packets.h b/lib/packets.h
>> index a4bee38..2bc65c9 100644
>> --- a/lib/packets.h
>> +++ b/lib/packets.h
>> @@ -886,6 +886,7 @@ struct tcp_header {
>>      ovs_be16 tcp_urg;
>>  };
>>  BUILD_ASSERT_DECL(TCP_HEADER_LEN == sizeof(struct tcp_header));
>> +#define ICMP_L4_DATA_LEN 8
>>
>>  /* Connection states.
>>   *
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
diff mbox series

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 5f60fea..0618fdd 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1513,10 +1513,18 @@  check_l4_icmp6(const struct conn_key *key, const void *data, size_t size,
     return validate_checksum ? checksum_valid(key, data, size, l3) : true;
 }
 
+/* If related is NULL, we are parsing nested TCP header  inside ICMP packet.
+ * Only 8 bytes of TCP header is required by RFC to be present in such case.
+ */
 static inline bool
-extract_l4_tcp(struct conn_key *key, const void *data, size_t size)
+extract_l4_tcp(struct conn_key *key, const void *data, size_t size,
+               bool *related)
 {
-    if (OVS_UNLIKELY(size < TCP_HEADER_LEN)) {
+    if (!related) {
+        if (size < ICMP_L4_DATA_LEN) {
+            return false;
+        }
+    } else if (size < TCP_HEADER_LEN) {
         return false;
     }
 
@@ -1750,7 +1758,7 @@  extract_l4(struct conn_key *key, const void *data, size_t size, bool *related,
 {
     if (key->nw_proto == IPPROTO_TCP) {
         return (!related || check_l4_tcp(key, data, size, l3,
-                validate_checksum)) && extract_l4_tcp(key, data, size);
+              validate_checksum)) && extract_l4_tcp(key, data, size, related);
     } else if (key->nw_proto == IPPROTO_UDP) {
         return (!related || check_l4_udp(key, data, size, l3,
                 validate_checksum)) && extract_l4_udp(key, data, size);
diff --git a/lib/packets.h b/lib/packets.h
index a4bee38..2bc65c9 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -886,6 +886,7 @@  struct tcp_header {
     ovs_be16 tcp_urg;
 };
 BUILD_ASSERT_DECL(TCP_HEADER_LEN == sizeof(struct tcp_header));
+#define ICMP_L4_DATA_LEN 8
 
 /* Connection states.
  *