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
--
Antonio Quartulli
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel