| Message ID | 20220917213154.29285-1-a@unstable.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [Openvpn-devel] is_ipv_X: improve packet consistency checks | expand |
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
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
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
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
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); }
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(-)