diff mbox series

[Openvpn-devel] get_addr_generic: fix server crash in freeaddrinfo on musl libc

Message ID 20230209153658.24001-1-ynezz@true.cz
State Superseded
Headers
Series [Openvpn-devel] get_addr_generic: fix server crash in freeaddrinfo on musl libc |

Commit Message

Petr Štetiar Feb. 9, 2023, 3:36 p.m. UTC 
  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(-)

Comments

Arne Schwabe Feb. 9, 2023, 3:53 p.m. UTC | #1 
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
Arne Schwabe Feb. 9, 2023, 4:03 p.m. UTC | #2 
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
Petr Štetiar Feb. 9, 2023, 4:33 p.m. UTC | #3 
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
Gert Doering Feb. 9, 2023, 4:47 p.m. UTC | #4 
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
Mykhailo Mishchenko Feb. 9, 2023, 5:40 p.m. UTC | #5 
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
Gert Doering Feb. 9, 2023, 7:38 p.m. UTC | #6 
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
Mykhailo Mishchenko Feb. 9, 2023, 8:09 p.m. UTC | #7 
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
Gert Doering Feb. 9, 2023, 8:59 p.m. UTC | #8 
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 mbox series

Patch

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;