Re: [PATCH 01/11] net: Add l_net_subnet_matches

Re: [PATCH 01/11] net: Add l_net_subnet_matches

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

Hi Andrew,

On 4/18/22 12:01, Andrew Zaborowski wrote:
> Hi Denis,
> 
> On Mon, 18 Apr 2022 at 18:34, Denis Kenzior <denkenz(a)gmail.com> wrote:
>>> +     return !bits ||
>>> +             ((((const uint8_t *) a)[bytes] ^ ((const uint8_t *) b)[bytes]) &
>>> +              (0xff00u >> bits)) == 0;
>>
>> There were a bit too many '()' for my liking.  I moved the casts to the variable
>> declaration block above which ended up being the same number of lines and looked
>> a bit cleaner.
> 
> Do we want to do the array[bytes] access after the !bits check to
> avoid accessing the byte after the end of the address for weird cases
> where the prefix_len is exactly the number of bits in the two buffers
> we received.

Aren't we guaranteed to be operating on 4 byte values?  So unless prefix_len == 
32, I don't see how we could perform out-of-bounds access?

Regards,
-Denis

Re: [PATCH 01/11] net: Add l_net_subnet_matches

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 963 bytes --]

Hi Denis,

On Thu, 21 Apr 2022 at 04:37, Denis Kenzior <denkenz(a)gmail.com> wrote:
> > I put the following comment in one of the patches:
> >
> > +        * TODO: check if we have a prefix route covering the IP,
> > +        * l_dhcp6_client doesn't guarantee that the received IP is in
> > +        * on of the subnets marked as on-link in the RA.
> >
>
> You keep repeating this, but I still don't believe this is true.  You could have
> RA gateway address being link-local, or subnet C.  The prefix info in the RA
> could be subnet B and the DHCPv6 address obtained could be in subnet A with no
> overlap.  Why would you want to? Who knows ;)

However complicated the setup is, you have access to all the routes on
the interface so it's not a problem.

But the point really is that whoever implements a check like this, and
other possible scenarios, if they choose to use ell they might end up
with an out of bounds access.

Best regards

Re: [PATCH 01/11] net: Add l_net_subnet_matches

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]

Hi Andrew,

> I put the following comment in one of the patches:
> 
> +        * TODO: check if we have a prefix route covering the IP,
> +        * l_dhcp6_client doesn't guarantee that the received IP is in
> +        * on of the subnets marked as on-link in the RA.
> 

You keep repeating this, but I still don't believe this is true.  You could have 
RA gateway address being link-local, or subnet C.  The prefix info in the RA 
could be subnet B and the DHCPv6 address obtained could be in subnet A with no 
overlap.  Why would you want to? Who knows ;)

The only 'requirement' is that the gateway is routable.  If it isn't, then 
obviously no traffic can get outside.

So I would simply drop this TODO, and any hope of doing any sort of validation. 
  Let the kernel do this.

> That's a different discussion.  We might want to warn or directly
> return an error from the D-Bus .Connect call if the connection is
> unusable, rather than ignore it and make the user discover it the hard
> way.  You could argue that it's usable for local traffic though.
> 

Let the higher layers deal with it.  If they need internet connectivity, then 
services like connman already implement these types of checks.

>> If there is no gateway and the DNS address is not 'onlink', what 'implicit'
>> route can we add that would make any sense?
> 
> You could add a direct route for that ip, assume it's 'onlink'.  But

Yeah, no :)

> it's really an error situation and should rather be treated that way
> IMHO.

Sure, but I'd still let the higher levels deal with it.

Regards,
-Denis

Re: [PATCH 01/11] net: Add l_net_subnet_matches

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2494 bytes --]

Hi Denis,

On Tue, 19 Apr 2022 at 22:12, Denis Kenzior <denkenz(a)gmail.com> wrote:
> > With IPv6 Router Advertisements you can have knowledge of multiple
> > on-link prefixes.  The gateway needs to match one of them in exactly
> > the same way as with IPv4.
>
> But netconfig is not doing this matching?

I put the following comment in one of the patches:

+        * TODO: check if we have a prefix route covering the IP,
+        * l_dhcp6_client doesn't guarantee that the received IP is in
+        * on of the subnets marked as on-link in the RA.

So it's not currently doing this but like I said l_net_subnet_match is
a public function so it's irrelevant whether netconfig is doing this.
We don't want to make public functions only work for some use cases
(without documenting which ones...) but rather for the generally
useful scenarios.

> The kernel is?  icmp6 simply puts in
> the default gateway info and the prefix routes into the routing table.  I don't
> think it is 'our problem' if the prefixes and route info from the RA results in
> the RA address not being routable.

That's a different discussion.  We might want to warn or directly
return an error from the D-Bus .Connect call if the connection is
unusable, rather than ignore it and make the user discover it the hard
way.  You could argue that it's usable for local traffic though.

>
>
> > Well, if that address you're trying to validate (e.g. let's say your
> > DNS IP) is on-link, which you'd find out from the RA, then you don't
> > need to add anything to the routing table because your RA handler will
> > already have added that route.  If it's not and there's no default
> > router, then either it's an error or you might want to implicitly add
> > a route for it.
>
> If there is no gateway and the DNS address is not 'onlink', what 'implicit'
> route can we add that would make any sense?

You could add a direct route for that ip, assume it's 'onlink'.  But
it's really an error situation and should rather be treated that way
IMHO.

>  Isn't the address simply unreachable?
>
> Why would we be 'validating' the DNS address in the first place?

To avoid wasting the user's time connecting to an unusable SSID and
possibly try the next network available.  But again that's a different
topic.

>  How would we
> validate it?

By checking if we have a route covering the address.  Basically same
as how we validate static configurations.

Best regards

Re: [PATCH 01/11] net: Add l_net_subnet_matches

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

Hi Andrew,

> With IPv6 Router Advertisements you can have knowledge of multiple
> on-link prefixes.  The gateway needs to match one of them in exactly
> the same way as with IPv4.

But netconfig is not doing this matching? The kernel is?  icmp6 simply puts in 
the default gateway info and the prefix routes into the routing table.  I don't 
think it is 'our problem' if the prefixes and route info from the RA results in 
the RA address not being routable.


> Well, if that address you're trying to validate (e.g. let's say your
> DNS IP) is on-link, which you'd find out from the RA, then you don't
> need to add anything to the routing table because your RA handler will
> already have added that route.  If it's not and there's no default
> router, then either it's an error or you might want to implicitly add
> a route for it.

If there is no gateway and the DNS address is not 'onlink', what 'implicit' 
route can we add that would make any sense?  Isn't the address simply unreachable?

Why would we be 'validating' the DNS address in the first place?  How would we 
validate it?

> 
> But the way you'd find out whether given IP is on-link is by calling
> l_net_subnet_matches for each prefix.  Their lengths vary from 0 to
> 128 like the RFC says.

Anyhow, just rename this function l_net_prefix_matches, since that is what you 
really intend.

Regards,
-Denis

Re: [PATCH 01/11] net: Add l_net_subnet_matches

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2678 bytes --]

On Tue, 19 Apr 2022 at 17:02, Denis Kenzior <denkenz(a)gmail.com> wrote:
> On 4/19/22 02:42, Andrew Zaborowski wrote:
> > On Mon, 18 Apr 2022 at 20:00, Denis Kenzior <denkenz(a)gmail.com> wrote:
> >> IPv6 doesn't really use subnets like IPv4 does though?  I'm not even sure this
> >> function would be relevant for IPv6?
> >
> > You need it to check if an address is within a subnet prefix?
> >
>
> IPv6 doesn't work the same as IPv4 though.  I'm fairly certain that the gateway
> and the static address do not need to be part of the same subnet at all.

Well, I was trying not to concentrate on just one use case since this
is public API.

But they kinda do work the same way.  The gateway always needs to be
in one of the on-link subnets or prefixes.

With static configuration (IPv4 and IPv6) or DHCP v4 you generally
have one on-link subnet, the one defined by your IP + prefix length.

With IPv6 Router Advertisements you can have knowledge of multiple
on-link prefixes.  The gateway needs to match one of them in exactly
the same way as with IPv4.

>
> >>   Is it?  Looks like we do this for FILS,
> >> but I'm not even certain that the check in ie.c in iwd is correct.
> >>
> >>> buffer of just long enough for the subnet address, or passing a
> >>> prefix_len of 32 / 128 for things like a route prefix.
> >>
> >> Well, prefix_len of 32/128 implies point-to-point routing (and even 31 in IPv4
> >> would be special).  So I'm not sure calling this function with such prefixes
> >> even makes sense?
> >
> > While an IPv4 31/32-bit subnet might not be a valid subnet, with IPv6
> > I don't know of a reason that single-address prefixes should be
> > treated specially.  RFC4191 for example says that an onlink prefix
> > length "value ranges from 0 to 128".
>
> Sure, but I still don't see how this figures into the subnet validation?
>
> An on-link with a prefix 128 just means that the host is on the local link.  You
> simply add it to the routing table and treat it as directly reachable (not via
> the default gateway).  I don't think there would be anything to validate?

Well, if that address you're trying to validate (e.g. let's say your
DNS IP) is on-link, which you'd find out from the RA, then you don't
need to add anything to the routing table because your RA handler will
already have added that route.  If it's not and there's no default
router, then either it's an error or you might want to implicitly add
a route for it.

But the way you'd find out whether given IP is on-link is by calling
l_net_subnet_matches for each prefix.  Their lengths vary from 0 to
128 like the RFC says.

Best regards

Re: [PATCH 01/11] net: Add l_net_subnet_matches

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2200 bytes --]

Hi Andrew,

On 4/19/22 02:42, Andrew Zaborowski wrote:
> Hi Denis,
> 
> On Mon, 18 Apr 2022 at 20:00, Denis Kenzior <denkenz(a)gmail.com> wrote:
>> On 4/18/22 12:21, Andrew Zaborowski wrote:
>>> On Mon, 18 Apr 2022 at 19:04, Denis Kenzior <denkenz(a)gmail.com> wrote:
>>>> On 4/18/22 12:01, Andrew Zaborowski wrote:
>>>>> Do we want to do the array[bytes] access after the !bits check to
>>>>> avoid accessing the byte after the end of the address for weird cases
>>>>> where the prefix_len is exactly the number of bits in the two buffers
>>>>> we received.
>>>>
>>>> Aren't we guaranteed to be operating on 4 byte values?  So unless prefix_len ==
>>>> 32, I don't see how we could perform out-of-bounds access?
>>>
>>> 4 or 16 bytes, in theory yes but I can imagine someone trying to use a
>>
>> IPv6 doesn't really use subnets like IPv4 does though?  I'm not even sure this
>> function would be relevant for IPv6?
> 
> You need it to check if an address is within a subnet prefix?
> 

IPv6 doesn't work the same as IPv4 though.  I'm fairly certain that the gateway 
and the static address do not need to be part of the same subnet at all.

>>   Is it?  Looks like we do this for FILS,
>> but I'm not even certain that the check in ie.c in iwd is correct.
>>
>>> buffer of just long enough for the subnet address, or passing a
>>> prefix_len of 32 / 128 for things like a route prefix.
>>
>> Well, prefix_len of 32/128 implies point-to-point routing (and even 31 in IPv4
>> would be special).  So I'm not sure calling this function with such prefixes
>> even makes sense?
> 
> While an IPv4 31/32-bit subnet might not be a valid subnet, with IPv6
> I don't know of a reason that single-address prefixes should be
> treated specially.  RFC4191 for example says that an onlink prefix
> length "value ranges from 0 to 128".

Sure, but I still don't see how this figures into the subnet validation?

An on-link with a prefix 128 just means that the host is on the local link.  You 
simply add it to the routing table and treat it as directly reachable (not via 
the default gateway).  I don't think there would be anything to validate?

Regards,
-Denis

Re: [PATCH 01/11] net: Add l_net_subnet_matches

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]

Hi Denis,

On Mon, 18 Apr 2022 at 20:00, Denis Kenzior <denkenz(a)gmail.com> wrote:
> On 4/18/22 12:21, Andrew Zaborowski wrote:
> > On Mon, 18 Apr 2022 at 19:04, Denis Kenzior <denkenz(a)gmail.com> wrote:
> >> On 4/18/22 12:01, Andrew Zaborowski wrote:
> >>> Do we want to do the array[bytes] access after the !bits check to
> >>> avoid accessing the byte after the end of the address for weird cases
> >>> where the prefix_len is exactly the number of bits in the two buffers
> >>> we received.
> >>
> >> Aren't we guaranteed to be operating on 4 byte values?  So unless prefix_len ==
> >> 32, I don't see how we could perform out-of-bounds access?
> >
> > 4 or 16 bytes, in theory yes but I can imagine someone trying to use a
>
> IPv6 doesn't really use subnets like IPv4 does though?  I'm not even sure this
> function would be relevant for IPv6?

You need it to check if an address is within a subnet prefix?

>  Is it?  Looks like we do this for FILS,
> but I'm not even certain that the check in ie.c in iwd is correct.
>
> > buffer of just long enough for the subnet address, or passing a
> > prefix_len of 32 / 128 for things like a route prefix.
>
> Well, prefix_len of 32/128 implies point-to-point routing (and even 31 in IPv4
> would be special).  So I'm not sure calling this function with such prefixes
> even makes sense?

While an IPv4 31/32-bit subnet might not be a valid subnet, with IPv6
I don't know of a reason that single-address prefixes should be
treated specially.  RFC4191 for example says that an onlink prefix
length "value ranges from 0 to 128".

Best regards

Re: [PATCH 01/11] net: Add l_net_subnet_matches

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

Hi Andrew,

On 4/18/22 12:21, Andrew Zaborowski wrote:
> On Mon, 18 Apr 2022 at 19:04, Denis Kenzior <denkenz(a)gmail.com> wrote:
>> On 4/18/22 12:01, Andrew Zaborowski wrote:
>>>> There were a bit too many '()' for my liking.  I moved the casts to the variable
>>>> declaration block above which ended up being the same number of lines and looked
>>>> a bit cleaner.
>>>
>>> Do we want to do the array[bytes] access after the !bits check to
>>> avoid accessing the byte after the end of the address for weird cases
>>> where the prefix_len is exactly the number of bits in the two buffers
>>> we received.
>>
>> Aren't we guaranteed to be operating on 4 byte values?  So unless prefix_len ==
>> 32, I don't see how we could perform out-of-bounds access?
> 
> 4 or 16 bytes, in theory yes but I can imagine someone trying to use a

IPv6 doesn't really use subnets like IPv4 does though?  I'm not even sure this 
function would be relevant for IPv6?  Is it?  Looks like we do this for FILS, 
but I'm not even certain that the check in ie.c in iwd is correct.

> buffer of just long enough for the subnet address, or passing a
> prefix_len of 32 / 128 for things like a route prefix.

Well, prefix_len of 32/128 implies point-to-point routing (and even 31 in IPv4 
would be special).  So I'm not sure calling this function with such prefixes 
even makes sense?

Regards,
-Denis

Re: [PATCH 01/11] net: Add l_net_subnet_matches

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 916 bytes --]

On Mon, 18 Apr 2022 at 19:04, Denis Kenzior <denkenz(a)gmail.com> wrote:
> On 4/18/22 12:01, Andrew Zaborowski wrote:
> >> There were a bit too many '()' for my liking.  I moved the casts to the variable
> >> declaration block above which ended up being the same number of lines and looked
> >> a bit cleaner.
> >
> > Do we want to do the array[bytes] access after the !bits check to
> > avoid accessing the byte after the end of the address for weird cases
> > where the prefix_len is exactly the number of bits in the two buffers
> > we received.
>
> Aren't we guaranteed to be operating on 4 byte values?  So unless prefix_len ==
> 32, I don't see how we could perform out-of-bounds access?

4 or 16 bytes, in theory yes but I can imagine someone trying to use a
buffer of just long enough for the subnet address, or passing a
prefix_len of 32 / 128 for things like a route prefix.

Best regards

Re: [PATCH 01/11] net: Add l_net_subnet_matches

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

Hi Denis,

On Mon, 18 Apr 2022 at 18:34, Denis Kenzior <denkenz(a)gmail.com> wrote:
> > +     return !bits ||
> > +             ((((const uint8_t *) a)[bytes] ^ ((const uint8_t *) b)[bytes]) &
> > +              (0xff00u >> bits)) == 0;
>
> There were a bit too many '()' for my liking.  I moved the casts to the variable
> declaration block above which ended up being the same number of lines and looked
> a bit cleaner.

Do we want to do the array[bytes] access after the !bits check to
avoid accessing the byte after the end of the address for weird cases
where the prefix_len is exactly the number of bits in the two buffers
we received.

Best regards

Re: [PATCH 01/11] net: Add l_net_subnet_matches

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1796 bytes --]

Hi Andrew,

On 4/11/22 09:20, Andrew Zaborowski wrote:
> Add inline function to check if two IP addresses are in the same subnet.
> ---
>   ell/net.h | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/ell/net.h b/ell/net.h
> index c97d9c9..837209c 100644
> --- a/ell/net.h
> +++ b/ell/net.h
> @@ -25,6 +25,7 @@
>   
>   #include <stdbool.h>
>   #include <stdint.h>
> +#include <string.h>
>   
>   struct in_addr;
>   struct in6_addr;
> @@ -40,6 +41,26 @@ bool l_net_hostname_is_localhost(const char *hostname);
>   bool l_net_get_address(int ifindex, struct in_addr *out);
>   bool l_net_get_link_local_address(int ifindex, struct in6_addr *out);
>   
> +static inline bool l_net_subnet_matches(const void *a, const void *b,
> +					uint8_t prefix_len)
> +{
> +	uint8_t bytes = prefix_len / 8;
> +	uint8_t bits = prefix_len & 7;
> +
> +	/*
> +	 * @a and @b are network byte order IPv4 or IPv6 addresses.
> +	 * We want to check if the initial (top) @prefix_len bits match.
> +	 * memcmp the whole bytes, then compare the final byte's top
> +	 * bits by anding with a mask.
> +	 */
> +	if (bytes && memcmp(a, b, bytes))

It looks like memcmp with a zero length is indeed guaranteed to return 0.  So 
this was simplified to if (memcmp(..))

> +		return false;
> +
> +	return !bits ||
> +		((((const uint8_t *) a)[bytes] ^ ((const uint8_t *) b)[bytes]) &
> +		 (0xff00u >> bits)) == 0;

There were a bit too many '()' for my liking.  I moved the casts to the variable 
declaration block above which ended up being the same number of lines and looked 
a bit cleaner.  Please double check that I didn't screw anything up.

> +}
> +
>   #ifdef __cplusplus
>   }
>   #endif
> 

Applied, thanks.

Regards,
-Denis

[PATCH 01/11] net: Add l_net_subnet_matches

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]

Add inline function to check if two IP addresses are in the same subnet.
---
 ell/net.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/ell/net.h b/ell/net.h
index c97d9c9..837209c 100644
--- a/ell/net.h
+++ b/ell/net.h
@@ -25,6 +25,7 @@
 
 #include <stdbool.h>
 #include <stdint.h>
+#include <string.h>
 
 struct in_addr;
 struct in6_addr;
@@ -40,6 +41,26 @@ bool l_net_hostname_is_localhost(const char *hostname);
 bool l_net_get_address(int ifindex, struct in_addr *out);
 bool l_net_get_link_local_address(int ifindex, struct in6_addr *out);
 
+static inline bool l_net_subnet_matches(const void *a, const void *b,
+					uint8_t prefix_len)
+{
+	uint8_t bytes = prefix_len / 8;
+	uint8_t bits = prefix_len & 7;
+
+	/*
+	 * @a and @b are network byte order IPv4 or IPv6 addresses.
+	 * We want to check if the initial (top) @prefix_len bits match.
+	 * memcmp the whole bytes, then compare the final byte's top
+	 * bits by anding with a mask.
+	 */
+	if (bytes && memcmp(a, b, bytes))
+		return false;
+
+	return !bits ||
+		((((const uint8_t *) a)[bytes] ^ ((const uint8_t *) b)[bytes]) &
+		 (0xff00u >> bits)) == 0;
+}
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.32.0