Hi,

On Sat, May 30, 2020 at 02:05:55AM +0200, Antonio Quartulli wrote:
> From: Antonio Quartulli <anto...@openvpn.net>
> 
> With this change a server is allowed to allocate an
> IPv6-only pool. This is required to make it capable
> of managing an IPv6-only tunnel.
> 
> Trac: #208
> Signed-off-by: Antonio Quartulli <anto...@openvpn.net>
> 
> ---
> Changes from v3:
> - properly compute pool size taking into account the actual base address

Alas, we need a v5 of this patch.

One part is purely cosmetic...

> +        msg(D_IFCONFIG_POOL, "IFCONFIG POOL: base=%s size=%d",
> +            print_in_addr_t(pool->ipv4.base, 0, &gc), pool->ipv4.size);

I think it should print "IFCONFIG POOL IPv4: ..." here, to make it
symmetric with IPv6.  Which is what we want, two equally-well supported
protocols, not "the primary" and "IPv6" :-)


The other part is bigger, unfortunately...


Mon Jun  1 12:18:05 2020 us=584985 194.97.140.21:32015 [cron2-freebsd-tc-amd64] 
Peer Connection Initiated with [AF_INET6]::ffff:194.97.140.21:32015

Program received signal SIGSEGV, Segmentation fault.
ifconfig_pool_release (pool=0x0, hand=-1, hard=hard@entry=false) at pool.c:361
361         int pool_size = ifconfig_pool_size(pool);


if you have a server that has *no* pools configured, no v4 and no v6 
pool (because address management is done outside openvpn) it will crash
the first time a given client reconnects.

GDB backtrace:

(gdb) where
#0  ifconfig_pool_release (pool=0x0, hand=-1, hard=hard@entry=false)
    at pool.c:361
#1  0x000055555558cc20 in multi_close_instance (m=0x7fffffffc520, 
    mi=0x55555567d9a0, shutdown=<optimized out>) at multi.c:667
#2  0x000055555558e852 in multi_delete_dup (new_mi=0x5555556bf620, 
    m=0x7fffffffc520) at multi.c:1382
#3  multi_connection_established (m=m@entry=0x7fffffffc520, 
    mi=mi@entry=0x5555556bf620) at multi.c:1820
#4  0x000055555558fccd in multi_process_post (m=m@entry=0x7fffffffc520, 
    mi=0x5555556bf620, flags=flags@entry=5) at multi.c:2404
#5  0x00005555555901d2 in multi_process_incoming_link (
    m=m@entry=0x7fffffffc520, instance=instance@entry=0x0, 
    mpp_flags=mpp_flags@entry=5) at multi.c:2768
#6  0x000055555558a3dc in multi_process_io_udp (m=0x7fffffffc520) at mudp.c:231
#7  tunnel_server_udp_single_threaded (top=0x7fffffffd580) at mudp.c:356
#8  0x0000555555594fc9 in openvpn_main (argc=2, argv=0x7fffffffe588)
    at openvpn.c:309
#9  0x00007ffff7a86deb in __libc_start_main () from /lib64/libc.so.6
#10 0x000055555555e86a in _start () at mudp.c:63


Not sure what is happening here - that part of the code hasn't been 
touched.  Still, a "master" server with the same config (no pools,
client ifconfig by means of ccd/ files) does not crash.


The ccd file looks like this:

--------------------------------------
vlan-pvid 200

# braucht explizites ifconfig*push wegen "kein pool"
ifconfig-push 10.204.4.10 255.255.255.0
ifconfig-ipv6-push fd00:abcd:204:4::a:10/64 fd00:abcd:204:4::1

# for 2.3
push "tun-ipv6"
--------------------------------------


Interesting enough, if I run "master" with the same config and do the
same things, with a breakpoint set on ifconfig_pool_release(), I get
a call that looks just the same:

Breakpoint 1, ifconfig_pool_release (pool=0x0, hand=-1, hard=hard@entry=false)
    at pool.c:277

... but continue'ing will just do that, not crash.



Ah. So trivial...

ifconfig_pool_release(struct ifconfig_pool *pool, ifconfig_pool_handle hand, con
...
    int pool_size = ifconfig_pool_size(pool);

    if (pool && hand >= 0 && hand < pool_size)


...
ifconfig_pool_size(const struct ifconfig_pool *pool)
...
    if (!pool->ipv4.enabled && !pool->ipv6.enabled)
    {
        return 0;
    }


... but if *neither* is enabled, "pool" actually is NULL...  so this
needs to become something like this

    if (pool == NULL
        || (!pool->ipv4.enabled && !pool->ipv6.enabled) )
    {
        return 0;
    }


... which fixes the crash for good (all callers will verify that 
"hand < size" etc.).

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             g...@greenie.muc.de

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Reply via email to