| Message ID | 20230209153658.24001-1-ynezz@true.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] get_addr_generic: fix server crash in freeaddrinfo on musl libc | expand |
Am 09.02.23 um 16:36 schrieb Petr Štetiar: > Server can crash on systems using musl libc when client with comma in > commonName tries to connect: > > ifconfig_pool_read(), in='VPN Client, abc,192.168.1.2,' > RESOLVE: Cannot parse IP address: abc: (Name does not resolve) > > as this leads to NULL pointer dereference in freeaddrinfo(): > > Program received signal SIGSEGV, Segmentation fault. > 0x00007ffff7fbf81a in freeaddrinfo (p=0x0) at src/network/freeaddrinfo.c:10 > (gdb) bt > #0 0x00007ffff7fbf81a in freeaddrinfo (p=0x0) at src/network/freeaddrinfo.c:10 > #1 0x00000000004389ec in get_addr_generic (af=af@entry=2, flags=flags@entry=4, hostname=hostname@entry=0x7ffff7ee2988 " abc", network=network@entry=0x7fffffffcb7c, netbits=netbits@entry=0x0, > resolve_retry_seconds=resolve_retry_seconds@entry=0, signal_received=0x0, msglevel=64) at openvpn-2.5.7/src/openvpn/socket.c:186 > #2 0x0000000000438a2d in getaddr (flags=flags@entry=4, hostname=hostname@entry=0x7ffff7ee2988 " abc", resolve_retry_seconds=resolve_retry_seconds@entry=0, succeeded=succeeded@entry=0x7fffffffcba7, > signal_received=signal_received@entry=0x0) at openvpn-2.5.7/src/openvpn/socket.c:202 > #3 0x0000000000430ae5 in ifconfig_pool_read (persist=0x7ffff7ee4510, pool=0x7ffff7edd450) at openvpn-2.5.7/src/openvpn/pool.c:661 > > So fix it by checking if `struct addrinfo*` pointer is valid before > passing it down to freeaddrinfo(). I am bit surprised that freeadrinfo does not follow the usual semantics of calling freesomething on a nullptr is a noop. If this is really the case would should add similar code (with comments that freeaddrinfo is special here) also in gc_freeaddrinfo_callback and the other places that call it without checking for ai to be valid. Arne
Am 09.02.23 um 16:53 schrieb Arne Schwabe: > Am 09.02.23 um 16:36 schrieb Petr Štetiar: >> Server can crash on systems using musl libc when client with comma in >> commonName tries to connect: >> >> ifconfig_pool_read(), in='VPN Client, abc,192.168.1.2,' >> RESOLVE: Cannot parse IP address: abc: (Name does not resolve) >> >> as this leads to NULL pointer dereference in freeaddrinfo(): >> >> Program received signal SIGSEGV, Segmentation fault. >> 0x00007ffff7fbf81a in freeaddrinfo (p=0x0) at >> src/network/freeaddrinfo.c:10 >> (gdb) bt >> #0 0x00007ffff7fbf81a in freeaddrinfo (p=0x0) at >> src/network/freeaddrinfo.c:10 >> #1 0x00000000004389ec in get_addr_generic (af=af@entry=2, >> flags=flags@entry=4, hostname=hostname@entry=0x7ffff7ee2988 " abc", >> network=network@entry=0x7fffffffcb7c, netbits=netbits@entry=0x0, >> resolve_retry_seconds=resolve_retry_seconds@entry=0, >> signal_received=0x0, msglevel=64) at >> openvpn-2.5.7/src/openvpn/socket.c:186 >> #2 0x0000000000438a2d in getaddr (flags=flags@entry=4, >> hostname=hostname@entry=0x7ffff7ee2988 " abc", >> resolve_retry_seconds=resolve_retry_seconds@entry=0, >> succeeded=succeeded@entry=0x7fffffffcba7, >> signal_received=signal_received@entry=0x0) at >> openvpn-2.5.7/src/openvpn/socket.c:202 >> #3 0x0000000000430ae5 in ifconfig_pool_read >> (persist=0x7ffff7ee4510, pool=0x7ffff7edd450) at >> openvpn-2.5.7/src/openvpn/pool.c:661 >> >> So fix it by checking if `struct addrinfo*` pointer is valid before >> passing it down to freeaddrinfo(). > > I am bit surprised that freeadrinfo does not follow the usual semantics > of calling freesomething on a nullptr is a noop. > > If this is really the case would should add similar code (with comments > that freeaddrinfo is special here) also in gc_freeaddrinfo_callback and > the other places that call it without checking for ai to be valid. > So FreeBSD has: * - freeaddrinfo(NULL). RFC2553 is silent about it. XNET 5.2 says it is * invalid. Current code accepts NULL to be compatible with other OSes. and android bionic has a similar comment (bionic is also BSD libc based): * - freeaddrinfo(NULL). RFC2553 is silent about it. XNET 5.2 says it is * invalid. * current code - SEGV on freeaddrinfo(NULL) but then actually does nothing on ai == NULL: #if defined(__BIONIC__) if (ai == NULL) return; #else _DIAGASSERT(ai != NULL); #endif so it seems that apart from some implementations like musl this assumption that freeaddrinfo is a noop is kind of true. I would still open a report against musl to fix this as this seem to be better to be aligned with the rest of the world. Arne
Arne Schwabe <arne@rfc2549.org> [2023-02-09 17:03:31]: > Am 09.02.23 um 16:53 schrieb Arne Schwabe: > > Am 09.02.23 um 16:36 schrieb Petr Štetiar: > > > Server can crash on systems using musl libc when client with comma in > > > commonName tries to connect: > > > > > > ifconfig_pool_read(), in='VPN Client, abc,192.168.1.2,' > > > RESOLVE: Cannot parse IP address: abc: (Name does not resolve) > > > > > > as this leads to NULL pointer dereference in freeaddrinfo(): > > > > > > Program received signal SIGSEGV, Segmentation fault. > > > 0x00007ffff7fbf81a in freeaddrinfo (p=0x0) at > > > src/network/freeaddrinfo.c:10 > > > (gdb) bt > > > #0 0x00007ffff7fbf81a in freeaddrinfo (p=0x0) at > > > src/network/freeaddrinfo.c:10 > > > #1 0x00000000004389ec in get_addr_generic (af=af@entry=2, > > > flags=flags@entry=4, hostname=hostname@entry=0x7ffff7ee2988 " abc", > > > network=network@entry=0x7fffffffcb7c, netbits=netbits@entry=0x0, > > > resolve_retry_seconds=resolve_retry_seconds@entry=0, > > > signal_received=0x0, msglevel=64) at > > > openvpn-2.5.7/src/openvpn/socket.c:186 > > > #2 0x0000000000438a2d in getaddr (flags=flags@entry=4, > > > hostname=hostname@entry=0x7ffff7ee2988 " abc", > > > resolve_retry_seconds=resolve_retry_seconds@entry=0, > > > succeeded=succeeded@entry=0x7fffffffcba7, > > > signal_received=signal_received@entry=0x0) at > > > openvpn-2.5.7/src/openvpn/socket.c:202 > > > #3 0x0000000000430ae5 in ifconfig_pool_read > > > (persist=0x7ffff7ee4510, pool=0x7ffff7edd450) at > > > openvpn-2.5.7/src/openvpn/pool.c:661 > > > > > > So fix it by checking if `struct addrinfo*` pointer is valid before > > > passing it down to freeaddrinfo(). > > > > I am bit surprised that freeadrinfo does not follow the usual semantics > > of calling freesomething on a nullptr is a noop. > > > > If this is really the case would should add similar code (with comments > > that freeaddrinfo is special here) also in gc_freeaddrinfo_callback and > > the other places that call it without checking for ai to be valid. > > > > So FreeBSD has: > > * - freeaddrinfo(NULL). RFC2553 is silent about it. XNET 5.2 says it is > * invalid. Current code accepts NULL to be compatible with other OSes. > > and android bionic has a similar comment (bionic is also BSD libc based): > > * - freeaddrinfo(NULL). RFC2553 is silent about it. XNET 5.2 says it is > * invalid. > * current code - SEGV on freeaddrinfo(NULL) > > but then actually does nothing on ai == NULL: > > #if defined(__BIONIC__) > if (ai == NULL) return; > #else > _DIAGASSERT(ai != NULL); > #endif > > so it seems that apart from some implementations like musl this assumption > that freeaddrinfo is a noop is kind of true. I would still open a report > against musl to fix this as this seem to be better to be aligned with the > rest of the world. https://www.openwall.com/lists/musl/2019/03/26/18
Hi, On Thu, Feb 09, 2023 at 04:36:58PM +0100, Petr ??tetiar wrote: > diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c > index a883ac4a156c..d304554cefda 100644 > --- a/src/openvpn/socket.c > +++ b/src/openvpn/socket.c > @@ -172,7 +172,10 @@ get_addr_generic(sa_family_t af, unsigned int flags, const char *hostname, > *sep = '/'; > } > out: > - freeaddrinfo(ai); > + if (ai) > + { > + freeaddrinfo(ai); > + } > free(var_host); The patch itself is fine, but as Arne says, maybe we should then add this to all places where freeaddrinfo() is called and could be NULL - so we're at least consistent in our interpretation of "should we pass NULL?". gert
On 09.02.23 17:36, Petr Štetiar wrote: > Server can crash on systems using musl libc when client with comma in > commonName tries to connect: > > ifconfig_pool_read(), in='VPN Client, abc,192.168.1.2,' > RESOLVE: Cannot parse IP address: abc: (Name does not resolve) I also would like to empathize what exactly leads to the crash. It clearly tried to parse a part of client certificate's Common Name as an IP address. As I understand it, this IP address was supposed to be used to restore client's previous IP address. This is another bug, different from trying to "free" NULL pointer. Mykhailo
Hi, On Thu, Feb 09, 2023 at 07:40:06PM +0200, Mykhailo Mishchenko wrote: > On 09.02.23 17:36, Petr ??tetiar wrote: > > Server can crash on systems using musl libc when client with comma in > > commonName tries to connect: > > > > ifconfig_pool_read(), in='VPN Client, abc,192.168.1.2,' > > RESOLVE: Cannot parse IP address: abc: (Name does not resolve) > > I also would like to empathize what exactly leads to the crash. It > clearly tried to parse a part of client certificate's Common Name as an > IP address. As I understand it, this IP address was supposed to be used > to restore client's previous IP address. This is another bug, different > from trying to "free" NULL pointer. CNs with commas in them are a long-standing issue, because OpenVPN doesn't know how to deal with them. So, don't do that. (There is an old Trac ticket about it, but nobody went out and fixed the code yet - which is tricky, as you can't just change the format of ifconfig-pool-persist without breaking existing setups) gert
On 09.02.23 21:38, Gert Doering wrote: > CNs with commas in them are a long-standing issue, because OpenVPN > doesn't know how to deal with them. So, don't do that. Ugh... Sometimes it is just not realistic to reissue certificates. :( It would be great to at least have it mentioned in BUGS section of the man page. On 09.02.23 21:38, Gert Doering wrote: > (There is an old Trac ticket about it, but nobody went out and fixed > the code yet - which is tricky, as you can't just change the format > of ifconfig-pool-persist without breaking existing setups) I would escape the comma with backslash, for example. It would only break those setups, which are already broken because they have a comma in a CN of a client certificate. But maybe there are reasons why this is also problematic, and I just don't know about them. Mykhailo
Hi, On Thu, Feb 09, 2023 at 10:09:45PM +0200, Mykhailo Mishchenko wrote: > On 09.02.23 21:38, Gert Doering wrote: > > (There is an old Trac ticket about it, but nobody went out and fixed > > the code yet - which is tricky, as you can't just change the format > > of ifconfig-pool-persist without breaking existing setups) > > I would escape the comma with backslash, for example. It would only > break those setups, which are already broken because they have a comma > in a CN of a client certificate. But maybe there are reasons why this is > also problematic, and I just don't know about them. status files, management console output & associated consumers, lots of things inside OpenVPN are separated by commas. Yes, this can all be fixed, but for all the bits that are "for other consumers" (status files, management) those entities would know how to deal with escaped commas. ifconfig-pool-persist might actually be a fairly easy one to solve, by going backwards from the end of the line - so not taking "field 2 + 3" but "last, and second-last", so it doesn't matter if there are commas in the first section. OTOH we'd need to deal with old-style files that have no IPv6... Mmmh. "Someone needs to code it, and test all variants" gert
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index a883ac4a156c..d304554cefda 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -172,7 +172,10 @@ get_addr_generic(sa_family_t af, unsigned int flags, const char *hostname, *sep = '/'; } out: - freeaddrinfo(ai); + if (ai) + { + freeaddrinfo(ai); + } free(var_host); return ret;
Server can crash on systems using musl libc when client with comma in commonName tries to connect: ifconfig_pool_read(), in='VPN Client, abc,192.168.1.2,' RESOLVE: Cannot parse IP address: abc: (Name does not resolve) as this leads to NULL pointer dereference in freeaddrinfo(): Program received signal SIGSEGV, Segmentation fault. 0x00007ffff7fbf81a in freeaddrinfo (p=0x0) at src/network/freeaddrinfo.c:10 (gdb) bt #0 0x00007ffff7fbf81a in freeaddrinfo (p=0x0) at src/network/freeaddrinfo.c:10 #1 0x00000000004389ec in get_addr_generic (af=af@entry=2, flags=flags@entry=4, hostname=hostname@entry=0x7ffff7ee2988 " abc", network=network@entry=0x7fffffffcb7c, netbits=netbits@entry=0x0, resolve_retry_seconds=resolve_retry_seconds@entry=0, signal_received=0x0, msglevel=64) at openvpn-2.5.7/src/openvpn/socket.c:186 #2 0x0000000000438a2d in getaddr (flags=flags@entry=4, hostname=hostname@entry=0x7ffff7ee2988 " abc", resolve_retry_seconds=resolve_retry_seconds@entry=0, succeeded=succeeded@entry=0x7fffffffcba7, signal_received=signal_received@entry=0x0) at openvpn-2.5.7/src/openvpn/socket.c:202 #3 0x0000000000430ae5 in ifconfig_pool_read (persist=0x7ffff7ee4510, pool=0x7ffff7edd450) at openvpn-2.5.7/src/openvpn/pool.c:661 So fix it by checking if `struct addrinfo*` pointer is valid before passing it down to freeaddrinfo(). References: https://github.com/openwrt/openwrt/issues/11890 Signed-off-by: Petr Štetiar <ynezz@true.cz> --- src/openvpn/socket.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)