[Openvpn-devel] If IPv6 pool specification sets pool start to ::0 address, increment.

Message ID 20200911115916.16117-1-gert@greenie.muc.de
State Superseded
Headers show
Series
  • [Openvpn-devel] If IPv6 pool specification sets pool start to ::0 address, increment.
Related show

Commit Message

Gert Doering Sept. 11, 2020, 11:59 a.m.
The first IPv6 address in a subnet is not usable (IPv6 anycast address),
but our pool code ignored this.

Instead of assigning an unusable address or erroring out, just log the
fact, and increment the pool start to <pool_base>::1

NOTE: this is a bit simplistic.  A pool that is larger than /96 and
has non-0 bits in the "uppermost bits" will still get the increment
as we only look at the lowermost 32 bits.

NOTE2: if the pool is specified with "--server-ipv6 $base/$bits", this
is a non-issue, as the address for the pool start will be incremented
anyway.

Reported-by: NicolaF_ in Trac
Trac: #1282

Signed-off-by: Gert Doering <gert@greenie.muc.de>
---
 doc/man-sections/server-options.rst |  3 ++-
 src/openvpn/pool.c                  | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

Comments

Antonio Quartulli Sept. 14, 2020, 8:56 a.m. | #1
Hi,

On 11/09/2020 13:59, Gert Doering wrote:
> index 1f74ac57..2814ff46 100644
> --- a/src/openvpn/pool.c
> +++ b/src/openvpn/pool.c
> @@ -224,6 +224,21 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type type, in_addr_t start,
>          }
>  
>          pool->ipv6.base = ipv6_base;
> +
> +        /* if a pool starts at ::0, that first IPv6 address is not usable
> +         * first clients (subnet anycast address).  Start with 1, then.
> +         * NOTE: this will also fire for something like
> +         *    ifconfig-ipv6-pool 2001:db8:0:1:1234::0/64
> +         * as we only look at the rightmost 32 bits.  So be it...
> +         */
> +        if (base == 0)

why not memcmp'ing ipv6.base with in6addr_any (defined in netinet/in.h)?
This way you get rid of the first NOTE (unless this header is linux
specific..).

Regards,
Gert Doering Sept. 14, 2020, 9:06 a.m. | #2
Hi,

On Mon, Sep 14, 2020 at 10:56:59AM +0200, Antonio Quartulli wrote:
> >          pool->ipv6.base = ipv6_base;
> > +
> > +        /* if a pool starts at ::0, that first IPv6 address is not usable
> > +         * first clients (subnet anycast address).  Start with 1, then.
> > +         * NOTE: this will also fire for something like
> > +         *    ifconfig-ipv6-pool 2001:db8:0:1:1234::0/64
> > +         * as we only look at the rightmost 32 bits.  So be it...
> > +         */
> > +        if (base == 0)
> 
> why not memcmp'ing ipv6.base with in6addr_any (defined in netinet/in.h)?
> This way you get rid of the first NOTE (unless this header is linux
> specific..).

because ipv6.base is not "all zero"...  it's "the IPv6 address that
the pool starts on", host + network part...

If you have pools like this:

  --ifconfig-ipv6-pool 2001:db8:0:1:1234::0/64
  --ifconfig-ipv6-pool 2001:db8:0:1:1234::0/112

what would "comparing with in6addr_any" achieve?  "ipv6.base" is 
2001:db8:0:1:1234::0 in both cases.


Now, "base" is "an integer that contains the lower 32 bit of the host
part" - which is 0, for both these cases.

To fix this "totally perfect" we'd need to move "base" to 64 bit integer,
and then figure out how to do that portably across supported 32 bit
platforms (including MSVC) (or work with 2x 32 bit 'base_low', 'base_hi',
or do "full 128 bit on 16 uint8_t" stuff...).

Yes, this can be done, but I was way too lazy and went for a less intrusive 
fix instead...

gert
Antonio Quartulli Sept. 17, 2020, 6:55 a.m. | #3
Hi,

On 11/09/2020 13:59, Gert Doering wrote:
> The first IPv6 address in a subnet is not usable (IPv6 anycast address),
> but our pool code ignored this.
> 
> Instead of assigning an unusable address or erroring out, just log the
> fact, and increment the pool start to <pool_base>::1
> 
> NOTE: this is a bit simplistic.  A pool that is larger than /96 and
> has non-0 bits in the "uppermost bits" will still get the increment
> as we only look at the lowermost 32 bits.
> 
> NOTE2: if the pool is specified with "--server-ipv6 $base/$bits", this
> is a non-issue, as the address for the pool start will be incremented
> anyway.
> 
> Reported-by: NicolaF_ in Trac
> Trac: #1282
> 
> Signed-off-by: Gert Doering <gert@greenie.muc.de>
> ---
>  doc/man-sections/server-options.rst |  3 ++-
>  src/openvpn/pool.c                  | 15 +++++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
> index 2009953c..56ffff9a 100644
> --- a/doc/man-sections/server-options.rst
> +++ b/doc/man-sections/server-options.rst
> @@ -204,7 +204,8 @@ fast hardware. SSL/TLS authentication must be used in this mode.
>       ifconfig-ipv6-pool ipv6addr/bits
>  
>    The pool starts at ``ipv6addr`` and matches the offset determined from
> -  the start of the IPv4 pool.
> +  the start of the IPv4 pool.  If the host part of the given IPv6
> +  address is ``0``, the pool starts at ``ipv6addr`` +1.
>  
>  --ifconfig-pool-persist args
>    Persist/unpersist ifconfig-pool data to ``file``, at ``seconds``
> diff --git a/src/openvpn/pool.c b/src/openvpn/pool.c
> index 1f74ac57..2814ff46 100644
> --- a/src/openvpn/pool.c
> +++ b/src/openvpn/pool.c
> @@ -224,6 +224,21 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type type, in_addr_t start,
>          }
>  
>          pool->ipv6.base = ipv6_base;
> +
> +        /* if a pool starts at ::0, that first IPv6 address is not usable

can we reword a bit this comment? I.e.: "if the starting address of a
pool has the host part all zero, that first ...."

The "starts at ::0" confused me as if we were targeting pools starting
at [::].


> +         * first clients (subnet anycast address).  Start with 1, then.
> +         * NOTE: this will also fire for something like
> +         *    ifconfig-ipv6-pool 2001:db8:0:1:1234::0/64
> +         * as we only look at the rightmost 32 bits.  So be it...

wouldn't this test miserably fail when the host part is smaller than 32?
like for a 2001:db8:0:1:1234::0/124?


Regards,

> +         */
> +        if (base == 0)
> +        {
> +            msg(D_IFCONFIG_POOL, "IFCONFIG POOL IPv6: incrementing pool start "
> +		"to avoid ::0 assignment");
> +            base++;
> +            pool->ipv6.base.s6_addr[15]++;
> +        }
> +
>          pool_ipv6_size = ipv6_netbits >= 112
>                            ? (1 << (128 - ipv6_netbits)) - base
>                            : IFCONFIG_POOL_MAX;
>
Gert Doering Sept. 17, 2020, 7:01 a.m. | #4
Hi,

On Thu, Sep 17, 2020 at 08:55:07AM +0200, Antonio Quartulli wrote:
> >          }
> >  
> >          pool->ipv6.base = ipv6_base;
> > +
> > +        /* if a pool starts at ::0, that first IPv6 address is not usable
> 
> can we reword a bit this comment? I.e.: "if the starting address of a
> pool has the host part all zero, that first ...."
> 
> The "starts at ::0" confused me as if we were targeting pools starting
> at [::].

Okay.  I did not want to make this comment too long, but it seems more
verbosity is needed.  I'll send a v2

> > +         * first clients (subnet anycast address).  Start with 1, then.
> > +         * NOTE: this will also fire for something like
> > +         *    ifconfig-ipv6-pool 2001:db8:0:1:1234::0/64
> > +         * as we only look at the rightmost 32 bits.  So be it...
> 
> wouldn't this test miserably fail when the host part is smaller than 32?
> like for a 2001:db8:0:1:1234::0/124?

We look at "base", which is only the host part, but "at most 32 bits of
the host part".

(This is *your* code...!)

gert
Antonio Quartulli Sept. 17, 2020, 7:23 a.m. | #5
Hi,

On 17/09/2020 09:01, Gert Doering wrote:
> We look at "base", which is only the host part, but "at most 32 bits of
> the host part".
> 
> (This is *your* code...!)

(self-shaming dance mode=ON)

Riiight, then drop this comment. The patch looks good, except for the
comment that needs more verbosity.


> 
> gert
>

Patch

diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
index 2009953c..56ffff9a 100644
--- a/doc/man-sections/server-options.rst
+++ b/doc/man-sections/server-options.rst
@@ -204,7 +204,8 @@  fast hardware. SSL/TLS authentication must be used in this mode.
      ifconfig-ipv6-pool ipv6addr/bits
 
   The pool starts at ``ipv6addr`` and matches the offset determined from
-  the start of the IPv4 pool.
+  the start of the IPv4 pool.  If the host part of the given IPv6
+  address is ``0``, the pool starts at ``ipv6addr`` +1.
 
 --ifconfig-pool-persist args
   Persist/unpersist ifconfig-pool data to ``file``, at ``seconds``
diff --git a/src/openvpn/pool.c b/src/openvpn/pool.c
index 1f74ac57..2814ff46 100644
--- a/src/openvpn/pool.c
+++ b/src/openvpn/pool.c
@@ -224,6 +224,21 @@  ifconfig_pool_init(const bool ipv4_pool, enum pool_type type, in_addr_t start,
         }
 
         pool->ipv6.base = ipv6_base;
+
+        /* if a pool starts at ::0, that first IPv6 address is not usable
+         * first clients (subnet anycast address).  Start with 1, then.
+         * NOTE: this will also fire for something like
+         *    ifconfig-ipv6-pool 2001:db8:0:1:1234::0/64
+         * as we only look at the rightmost 32 bits.  So be it...
+         */
+        if (base == 0)
+        {
+            msg(D_IFCONFIG_POOL, "IFCONFIG POOL IPv6: incrementing pool start "
+		"to avoid ::0 assignment");
+            base++;
+            pool->ipv6.base.s6_addr[15]++;
+        }
+
         pool_ipv6_size = ipv6_netbits >= 112
                           ? (1 << (128 - ipv6_netbits)) - base
                           : IFCONFIG_POOL_MAX;