[Openvpn-devel] is_ipv_X: improve packet consistency checks

Message ID 20220917213154.29285-1-a@unstable.cc
State New
Headers show
Series [Openvpn-devel] is_ipv_X: improve packet consistency checks | expand

Commit Message

Antonio Quartulli Sept. 17, 2022, 9:31 p.m. UTC
This patch brings the following improvements:
* check that ETH proto and version in IP header are consistent;
* check that length of the packet is enough to store the expected IP
  header (it may be an IPv4 or an IPv6 header)
* restyle a bit to improve readability;
* remove spaces before ')' in invocations.

Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 src/openvpn/proto.c | 91 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 69 insertions(+), 22 deletions(-)

Comments

Gert Doering Sept. 18, 2022, 10:12 a.m. UTC | #1
Hi,

On Sat, Sep 17, 2022 at 11:31:54PM +0200, Antonio Quartulli wrote:
> This patch brings the following improvements:
> * check that ETH proto and version in IP header are consistent;
> * check that length of the packet is enough to store the expected IP
>   header (it may be an IPv4 or an IPv6 header)

Since this is in the fast path - what is the motivation behind adding
all these extra checks?

gert
Antonio Quartulli Sept. 18, 2022, 7:47 p.m. UTC | #2
Hi,

On 18/09/2022 12:12, Gert Doering wrote:
> Hi,
> 
> On Sat, Sep 17, 2022 at 11:31:54PM +0200, Antonio Quartulli wrote:
>> This patch brings the following improvements:
>> * check that ETH proto and version in IP header are consistent;
>> * check that length of the packet is enough to store the expected IP
>>    header (it may be an IPv4 or an IPv6 header)
> 
> Since this is in the fast path - what is the motivation behind adding
> all these extra checks?

These extra checks are meant to avoid bogus packets to sneak in (i.e. a 
ETH packet saying IPv4, but then having an IPv6 header on top).

Additionally we accept IPv6 packets without really checking if the 
packet has enough room for an entire IPv6 header.

This said, I did not consider that this function is along the fast path, 
therefore I'll check if we can reduce the number of jumps.

In the worst case I will only address the second point of the list above 
(as we may later access an IPv6 header that is not fully allocated).

Cheers,

> 
> gert
Gert Doering Sept. 18, 2022, 8:31 p.m. UTC | #3
Hi,

On Sun, Sep 18, 2022 at 09:47:56PM +0200, Antonio Quartulli wrote:
> In the worst case I will only address the second point of the list above 
> (as we may later access an IPv6 header that is not fully allocated).

I think everything that actually looks further down the header *does*
check if the size is big enough.  MSS definitely does.

Why do we care about ethernet types?  What is the problem this check 
is supposed to address?  Does it work for 802.1q packets (different
ether type)?

gert
Antonio Quartulli Sept. 18, 2022, 9:19 p.m. UTC | #4
Hi,

On 18/09/2022 22:31, Gert Doering wrote:
> Hi,
> 
> On Sun, Sep 18, 2022 at 09:47:56PM +0200, Antonio Quartulli wrote:
>> In the worst case I will only address the second point of the list above
>> (as we may later access an IPv6 header that is not fully allocated).
> 
> I think everything that actually looks further down the header *does*
> check if the size is big enough.  MSS definitely does.
> 

Well, this function has a check like this:

  51         if (BLEN(buf) < sizeof(struct openvpn_iphdr))
  52         {
  53             return false;
  54         }

However this check is assuming that the network header is IPv4.
We don't check if the buffer is long enough to contain an IPv6 header, 
when the packet is IPv6.

The following code is trying to address this issue:

+    /* ensure that there is enough room for a header of the expected 
version */
+    size_t ih_len = 0;
+    switch (eth_ip_proto)
+    {
+        case OPENVPN_ETH_P_IPV4:
+            ih_len = sizeof(struct openvpn_iphdr);
+            break;
+
+        case OPENVPN_ETH_P_IPV6:
+            ih_len = sizeof(struct openvpn_ipv6hdr);
+            break;
+    }
+    if (BLEN(buf) < (offset + ih_len))
+    {
+        return false;
+    }

On the other hand, if we think that proper checks are done later in the 
code, we could remove this check entirely (and only make sure we have 
the required bytes to access the version_len field).

> Why do we care about ethernet types?  What is the problem this check
> is supposed to address?  Does it work for 802.1q packets (different
> ether type)?
> 

The ethernet type is just used to perform a consistency check: if 
Ethernet says the next header is IPv4, but then we have IPv6, this means 
the packet is bogus and should be discarded. (same for the other way around)

Does it make sense?

Cheers,

> gert

Patch

diff --git a/src/openvpn/proto.c b/src/openvpn/proto.c
index 88abd199..345df341 100644
--- a/src/openvpn/proto.c
+++ b/src/openvpn/proto.c
@@ -41,31 +41,40 @@ 
 static bool
 is_ipv_X(int tunnel_type, struct buffer *buf, int ip_ver)
 {
+    uint16_t eth_ip_proto;
     int offset;
-    uint16_t proto;
-    const struct openvpn_iphdr *ih;
+
+    switch (ip_ver)
+    {
+        case 4:
+            eth_ip_proto = OPENVPN_ETH_P_IPV4;
+            break;
+
+        case 6:
+            eth_ip_proto = OPENVPN_ETH_P_IPV6;
+            break;
+
+        default:
+            /* invalid input provided */
+            return false;
+    }
 
     verify_align_4(buf);
     if (tunnel_type == DEV_TYPE_TUN)
     {
-        if (BLEN(buf) < sizeof(struct openvpn_iphdr))
-        {
-            return false;
-        }
         offset = 0;
     }
     else if (tunnel_type == DEV_TYPE_TAP)
     {
-        const struct openvpn_ethhdr *eh;
-        if (BLEN(buf) < (sizeof(struct openvpn_ethhdr)
-                         + sizeof(struct openvpn_iphdr)))
+        if (BLEN(buf) < sizeof(struct openvpn_ethhdr))
         {
             return false;
         }
-        eh = (const struct openvpn_ethhdr *)BPTR(buf);
 
-        /* start by assuming this is a standard Eth fram */
-        proto = eh->proto;
+        /* start by assuming this is a standard Eth frame */
+        const struct openvpn_ethhdr *eh;
+        eh = (const struct openvpn_ethhdr *)BPTR(buf);
+        uint16_t proto = eh->proto;
         offset = sizeof(struct openvpn_ethhdr);
 
         /* if this is a 802.1q frame, parse the header using the according
@@ -74,19 +83,17 @@  is_ipv_X(int tunnel_type, struct buffer *buf, int ip_ver)
         if (proto == htons(OPENVPN_ETH_P_8021Q))
         {
             const struct openvpn_8021qhdr *evh;
-            if (BLEN(buf) < (sizeof(struct openvpn_ethhdr)
-                             + sizeof(struct openvpn_iphdr)))
+            if (BLEN(buf) < sizeof(struct openvpn_8021qhdr))
             {
                 return false;
             }
 
             evh = (const struct openvpn_8021qhdr *)BPTR(buf);
-
             proto = evh->proto;
             offset = sizeof(struct openvpn_8021qhdr);
         }
 
-        if (ntohs(proto) != (ip_ver == 6 ? OPENVPN_ETH_P_IPV6 : OPENVPN_ETH_P_IPV4))
+        if (ntohs(proto) != eth_ip_proto)
         {
             return false;
         }
@@ -96,28 +103,68 @@  is_ipv_X(int tunnel_type, struct buffer *buf, int ip_ver)
         return false;
     }
 
-    ih = (const struct openvpn_iphdr *)(BPTR(buf) + offset);
+    /* ensure that there is enough room for a header of the expected version */
+    size_t ih_len = 0;
+    switch (eth_ip_proto)
+    {
+        case OPENVPN_ETH_P_IPV4:
+            ih_len = sizeof(struct openvpn_iphdr);
+            break;
+
+        case OPENVPN_ETH_P_IPV6:
+            ih_len = sizeof(struct openvpn_ipv6hdr);
+            break;
+    }
+    if (BLEN(buf) < (offset + ih_len))
+    {
+        return false;
+    }
 
     /* IP version is stored in the same bits for IPv4 or IPv6 header */
-    if (OPENVPN_IPH_GET_VER(ih->version_len) == ip_ver)
+    const struct openvpn_iphdr *ih;
+    ih = (const struct openvpn_iphdr *)(BPTR(buf) + offset);
+    uint8_t hdr_ip_ver = OPENVPN_IPH_GET_VER(ih->version_len);
+
+    if (tunnel_type == DEV_TYPE_TAP)
     {
-        return buf_advance(buf, offset);
+        /* ensure consistency between the version in the IP header and
+         * the Eth proto that was retrieved previously
+         */
+        switch (eth_ip_proto)
+        {
+            case OPENVPN_ETH_P_IPV4:
+                if (hdr_ip_ver != 4)
+                {
+                    return false;
+                }
+                break;
+
+            case OPENVPN_ETH_P_IPV6:
+                if (hdr_ip_ver != 6)
+                {
+                    return false;
+                }
+                break;
+        }
     }
-    else
+
+    if (hdr_ip_ver != ip_ver)
     {
         return false;
     }
+
+    return buf_advance(buf, offset);
 }
 
 bool
 is_ipv4(int tunnel_type, struct buffer *buf)
 {
-    return is_ipv_X( tunnel_type, buf, 4 );
+    return is_ipv_X(tunnel_type, buf, 4);
 }
 bool
 is_ipv6(int tunnel_type, struct buffer *buf)
 {
-    return is_ipv_X( tunnel_type, buf, 6 );
+    return is_ipv_X(tunnel_type, buf, 6);
 }