[ovs-dev,v1] conntrack: Fix ICMPV4 error data L4 length check.
diff mbox series

Message ID 1566836140-72882-1-git-send-email-dlu998@gmail.com
State New
Headers show
Series
  • [ovs-dev,v1] conntrack: Fix ICMPV4 error data L4 length check.
Related show

Commit Message

Darrell Ball Aug. 26, 2019, 4:15 p.m. UTC
The ICMP error data L4 length check was found to be too strict for TCP,
expecting a minimum of 20 rather than 8 bytes.  This worked by
hapenstance for other inner protocols.  The approach is to explicitly
handle the ICMPV4 error data L4 length check and to do this for all
supported inner protocols in the same way.  Making the code common
between protocols also allows the existing ICMP related UDP tests to
cover TCP and ICMP cases.

Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.")
CC: Daniele Di Proietto <diproiettod@ovn.org>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/361949.html
Reported-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
Co-authored-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
Signed-off-by: Darrell Ball <dlu998@gmail.com>
---
 lib/conntrack.c | 38 ++++++++++++++++++++++----------------
 lib/packets.h   |  3 +++
 2 files changed, 25 insertions(+), 16 deletions(-)

Comments

Vishal Deep Ajmera Aug. 27, 2019, 9:02 a.m. UTC | #1
Hi Darrell,

Thanks for the patch. When I applied the patch to latest master, 
I see that we take care of length check (< 8) only for ICMPv6 and 
not for ICMPv4. We need to do it for ICMPv4 as well.

Also, we are already using 'related' to skip or not to skip length check.

 * If 'related' is NULL, it means that we're already parsing a header nested
 * in an ICMP error.  In this case, we skip checksum and length validation.

However we continue to validate length in extract_l4_tcp (<8 or <20). 
I understand that check for minimum 8 bytes header is needed to make
sure we can extract tcp port numbers.

Can we instead try to converge all checks at one place and still take care 
of nested header? In my opinion it will simplify the code.

Warm Regards,
Vishal Ajmera
Darrell Ball Aug. 27, 2019, 8:22 p.m. UTC | #2
On Tue, Aug 27, 2019 at 2:02 AM Vishal Deep Ajmera <
vishal.deep.ajmera@ericsson.com> wrote:

> Hi Darrell,
>
> Thanks for the patch. When I applied the patch to latest master,
> I see that we take care of length check (< 8) only for ICMPv6 and
> not for ICMPv4.


That is interesting
i just tried applying on top of tree and I see that the git applies some
changes (2 lines)
in extract_l4_icmp6() rather the intended extract_l4_icmp() as in the patch
I sent out.
My guess is that the surrounding lines are identical in the 2 functions and
I had other
patches in the same branch shifting the patch downward, hence git applied
the changes
to extract_l4_icmp6() rather than extract_l4_icmp()

I'll make the changes on a clean branch and resend.

JTBC, the 8 byte ICMP error data L4 length restriction is only for V4.
ICMP6 does not have this restriction; see
https://tools.ietf.org/html/rfc4443



> We need to do it for ICMPv4 as well.
>
>
The intention is only for ICMPv4; see above.


> Also, we are already using 'related' to skip or not to skip length check.
>

We cannot use 'related' for this as ICMPv6 is different than ICMPv4


>  * If 'related' is NULL, it means that we're already parsing a header
> nested
>  * in an ICMP error.  In this case, we skip checksum and length validation.
>
> However we continue to validate length in extract_l4_tcp (<8 or <20).
> I understand that check for minimum 8 bytes header is needed to make
> sure we can extract tcp port numbers.
>

No, it is a sanity check like other sanity checks.
The length check varies depending on the context.


> Can we instead try to converge all checks at one place and still take care
> of nested header? In my opinion it will simplify the code.
>

See the information above


>
> Warm Regards,
> Vishal Ajmera
>
Vishal Deep Ajmera Aug. 28, 2019, 8:43 a.m. UTC | #3
That is interesting
i just tried applying on top of tree and I see that the git applies some changes (2 lines)
in extract_l4_icmp6() rather the intended extract_l4_icmp() as in the patch I sent out.
My guess is that the surrounding lines are identical in the 2 functions and I had other
patches in the same branch shifting the patch downward, hence git applied the changes
to extract_l4_icmp6() rather than extract_l4_icmp()

I'll make the changes on a clean branch and resend.

Thanks. I applied this patch and looks ok to me.

JTBC, the 8 byte ICMP error data L4 length restriction is only for V4.
ICMP6 does not have this restriction; see https://tools.ietf.org/html/rfc4443

In my opinion, we should limit the check to < 8 bytes even in case of ICMPv6 as that is all
is required from the TCP header to extract port numbers and aligns it with ICMPv4.
Specially because RFC is not mandating minimum size for L4 header in case of ICMPv6.
Darrell Ball Aug. 28, 2019, 4:37 p.m. UTC | #4
On Wed, Aug 28, 2019 at 1:43 AM Vishal Deep Ajmera <
vishal.deep.ajmera@ericsson.com> wrote:

> That is interesting
>
> i just tried applying on top of tree and I see that the git applies some
> changes (2 lines)
>
> in extract_l4_icmp6() rather the intended extract_l4_icmp() as in the
> patch I sent out.
>
> My guess is that the surrounding lines are identical in the 2 functions
> and I had other
>
> patches in the same branch shifting the patch downward, hence git applied
> the changes
>
> to extract_l4_icmp6() rather than extract_l4_icmp()
>
>
>
> I'll make the changes on a clean branch and resend.
>
>
>
> Thanks. I applied this patch and looks ok to me.
>

Thanks for confirming


>
>
> JTBC, the 8 byte ICMP error data L4 length restriction is only for V4.
>
> ICMP6 does not have this restriction; see
> https://tools.ietf.org/html/rfc4443
>
>
>
> In my opinion, we should limit the check to < 8 bytes even in case of
> ICMPv6 as that is all
>
> is required from the TCP header to extract port numbers and aligns it with
> ICMPv4.
>
> Specially because RFC is not mandating minimum size for L4 header in case
> of ICMPv6.
>

For V6, the ICMP error L4 length can even be zero, legitimately, with a
large extension header
presence, theoretically; practically, these are almost certainly crafted
packets. The existing
check for ICMP6 is mostly permissive but reasonable.

The ICMP6 sanity check can be enhanced. For the most part, it can be made
more strict while
handling the above corner case perfectly.

I don't think we should mess with enhancing ICMP6 as part of this bug fix
for ICMPv4.
Thats why this patch leaves ICMP6 unmodified.


>
>
Vishal Deep Ajmera Aug. 29, 2019, 4:53 a.m. UTC | #5
Thanks Darrell. Patch looks ok to me.

Warm Regards,
Vishal Ajmera

Patch
diff mbox series

diff --git a/lib/conntrack.c b/lib/conntrack.c
index ecf3bcc..de0ab9b 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1565,9 +1565,10 @@  check_l4_icmp6(const struct conn_key *key, const void *data, size_t size,
 }
 
 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,
+               size_t *chk_len)
 {
-    if (OVS_UNLIKELY(size < TCP_HEADER_LEN)) {
+    if (OVS_UNLIKELY(size < (chk_len ? *chk_len : TCP_HEADER_LEN))) {
         return false;
     }
 
@@ -1580,9 +1581,10 @@  extract_l4_tcp(struct conn_key *key, const void *data, size_t size)
 }
 
 static inline bool
-extract_l4_udp(struct conn_key *key, const void *data, size_t size)
+extract_l4_udp(struct conn_key *key, const void *data, size_t size,
+               size_t *chk_len)
 {
-    if (OVS_UNLIKELY(size < UDP_HEADER_LEN)) {
+    if (OVS_UNLIKELY(size < (chk_len ? *chk_len : UDP_HEADER_LEN))) {
         return false;
     }
 
@@ -1596,7 +1598,7 @@  extract_l4_udp(struct conn_key *key, const void *data, size_t size)
 
 static inline bool extract_l4(struct conn_key *key, const void *data,
                               size_t size, bool *related, const void *l3,
-                              bool validate_checksum);
+                              bool validate_checksum, size_t *chk_len);
 
 static uint8_t
 reverse_icmp_type(uint8_t type)
@@ -1628,9 +1630,9 @@  reverse_icmp_type(uint8_t type)
  * possible */
 static inline int
 extract_l4_icmp(struct conn_key *key, const void *data, size_t size,
-                bool *related)
+                bool *related, size_t *chk_len)
 {
-    if (OVS_UNLIKELY(size < ICMP_HEADER_LEN)) {
+    if (OVS_UNLIKELY(size < (chk_len ? *chk_len : ICMP_HEADER_LEN))) {
         return false;
     }
 
@@ -1681,8 +1683,9 @@  extract_l4_icmp(struct conn_key *key, const void *data, size_t size,
         key->src = inner_key.src;
         key->dst = inner_key.dst;
         key->nw_proto = inner_key.nw_proto;
+        size_t check_len = ICMP_ERROR_DATA_L4_LEN;
 
-        ok = extract_l4(key, l4, tail - l4, NULL, l3, false);
+        ok = extract_l4(key, l4, tail - l4, NULL, l3, false, &check_len);
         if (ok) {
             conn_key_reverse(key);
             *related = true;
@@ -1769,7 +1772,7 @@  extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
         key->dst = inner_key.dst;
         key->nw_proto = inner_key.nw_proto;
 
-        ok = extract_l4(key, l4, tail - l4, NULL, l3, false);
+        ok = extract_l4(key, l4, tail - l4, NULL, l3, false, NULL);
         if (ok) {
             conn_key_reverse(key);
             *related = true;
@@ -1797,23 +1800,25 @@  extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
  * in an ICMP error.  In this case, we skip checksum and length validation. */
 static inline bool
 extract_l4(struct conn_key *key, const void *data, size_t size, bool *related,
-           const void *l3, bool validate_checksum)
+           const void *l3, bool validate_checksum, size_t *chk_len)
 {
     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, chk_len);
     } else if (key->nw_proto == IPPROTO_UDP) {
         return (!related || check_l4_udp(key, data, size, l3,
-                validate_checksum)) && extract_l4_udp(key, data, size);
+                validate_checksum))
+               && extract_l4_udp(key, data, size, chk_len);
     } else if (key->dl_type == htons(ETH_TYPE_IP)
                && key->nw_proto == IPPROTO_ICMP) {
         return (!related || check_l4_icmp(data, size, validate_checksum))
-               && extract_l4_icmp(key, data, size, related);
+               && extract_l4_icmp(key, data, size, related, chk_len);
     } else if (key->dl_type == htons(ETH_TYPE_IPV6)
                && key->nw_proto == IPPROTO_ICMPV6) {
         return (!related || check_l4_icmp6(key, data, size, l3,
-                validate_checksum)) && extract_l4_icmp6(key, data, size,
-                related);
+                validate_checksum))
+               && extract_l4_icmp6(key, data, size, related);
     } else {
         return false;
     }
@@ -1892,7 +1897,8 @@  conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type,
             bool  hwol_good_l4_csum = dp_packet_l4_checksum_valid(pkt);
             /* Validate the checksum only when hwol is not supported. */
             if (extract_l4(&ctx->key, l4, dp_packet_l4_size(pkt),
-                           &ctx->icmp_related, l3, !hwol_good_l4_csum)) {
+                           &ctx->icmp_related, l3, !hwol_good_l4_csum,
+                           NULL)) {
                 ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis);
                 return true;
             }
diff --git a/lib/packets.h b/lib/packets.h
index 0b3e3a2..c78defb 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -780,6 +780,9 @@  struct icmp_header {
 };
 BUILD_ASSERT_DECL(ICMP_HEADER_LEN == sizeof(struct icmp_header));
 
+/* ICMPV4 */
+#define ICMP_ERROR_DATA_L4_LEN 8
+
 #define IGMP_HEADER_LEN 8
 struct igmp_header {
     uint8_t igmp_type;