Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ziti_resolve faults if host is NULL #525

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tomc797
Copy link
Contributor

@tomc797 tomc797 commented Jun 14, 2023

The fix addresses a Ziti_resolve defects that triggers segfault. The getaddrinfo interface contract permits host == NULL, but Ziti_resolve assumes host != NULL. Additionally, Ziti_resolve leaks res and addr if error is signaled.

ubuntu@jammy:~$ zitify -i tomc.json -b 9999:zitify-nc-service nc -l 9999
b = 9999:zitify-nc-service
bindings = 
/usr/local/bin/zitify: line 101: 457642 Segmentation fault      (core dumped) LD_PRELOAD="$preload_lib" ZITI_BINDINGS="${bindings}" "$@"
ubuntu@jammy:~$ zitify -i tomc.json -b 9999:zitify-nc-service gdb nc
b = 9999:zitify-nc-service
bindings = 
GNU gdb (Ubuntu 12.1-0ubuntu1~22.04) 12.1
Copyright (C) 2022 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<https://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
    <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from nc...
(No debugging symbols found in nc)
(gdb) r -l 9999
Starting program: /usr/bin/nc -l 9999
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7ffff73ff640 (LWP 457774)]
[New Thread 0x7ffff6bfe640 (LWP 457775)]
[New Thread 0x7ffff63fd640 (LWP 457776)]
[New Thread 0x7ffff5bfc640 (LWP 457777)]
[New Thread 0x7ffff53fb640 (LWP 457778)]

Thread 1 "nc" received signal SIGSEGV, Segmentation fault.
__strchr_avx2 () at ../sysdeps/x86_64/multiarch/strchr-avx2.S:65
65	../sysdeps/x86_64/multiarch/strchr-avx2.S: No such file or directory.
(gdb) bt
#0  __strchr_avx2 () at ../sysdeps/x86_64/multiarch/strchr-avx2.S:65
#1  0x00007ffff7c94e5d in uv_ip6_addr (ip=0x0, port=9999, addr=0x5555555f16a0)
    at /home/tomc/Projects/ZT/vcpkg/buildtrees/libuv/src/1ba00a27b8-94fcdf9fd7.clean/src/uv-common.c:244
#2  0x00007ffff7c4431e in Ziti_resolve (host=0x0, port=0x7fffffffe239 "9999", hints=0x7fffffffcb70, addrlist=0x7fffffffcb10)
    at /home/tomc/Projects/ZT/zitify/build/_deps/ziti-src/library/zitilib.c:1257
#3  0x00007ffff7c3d1be in getaddrinfo(const char * __restrict__, const char * __restrict__, const addrinfo * __restrict__, addrinfo ** __restrict__) (name=0x0, service=0x7fffffffe239 "9999", hints=0x7fffffffcb70, pai=0x7fffffffcb10) at /home/tomc/Projects/ZT/zitify/src/zitify.cpp:113
#4  0x0000555555557c13 in ?? ()
#5  0x00007ffff7829d90 in __libc_start_call_main (main=main@entry=0x5555555567e0, argc=argc@entry=3, argv=argv@entry=0x7fffffffdea8)
    at ../sysdeps/nptl/libc_start_call_main.h:58
#6  0x00007ffff7829e40 in __libc_start_main_impl (main=0x5555555567e0, argc=3, argv=0x7fffffffdea8, init=<optimized out>, fini=<optimized out>, 
    rtld_fini=<optimized out>, stack_end=0x7fffffffde98) at ../csu/libc-start.c:392
#7  0x0000555555558af5 in ?? ()
(gdb) quit
A debugging session is active.

	Inferior 1 [process 457764] will be killed.

Quit anyway? (y or n) y

@tomc797 tomc797 requested a review from a team as a code owner June 14, 2023 05:06
@ekoby
Copy link
Member

ekoby commented Jun 22, 2023

thank you for your PR.

It compelled me to go look at getaddrinfo documentation since it is the call that this function is trying to emulate

to be more compliant, we should return INADDR_ANY/IN6ADDR_ANY instead of loopback in some cases, relevant doc

       If the AI_PASSIVE flag is specified in hints.ai_flags, and node
       is NULL, then the returned socket addresses will be suitable for
       bind(2)ing a socket that will accept(2) connections.  The
       returned socket address will contain the "wildcard address"
       (INADDR_ANY for IPv4 addresses, IN6ADDR_ANY_INIT for IPv6
       address).  The wildcard address is used by applications
       (typically servers) that intend to accept connections on any of
       the host's network addresses.  If node is not NULL, then the
       AI_PASSIVE flag is ignored.

       If the AI_PASSIVE flag is not set in hints.ai_flags, then the
       returned socket addresses will be suitable for use with
       connect(2), sendto(2), or sendmsg(2).  If node is NULL, then the
       network address will be set to the loopback interface address
       (INADDR_LOOPBACK for IPv4 addresses, IN6ADDR_LOOPBACK_INIT for
       IPv6 address); this is used by applications that intend to
       communicate with peers running on the same host.

if ((rc = uv_ip4_addr(host, portnum, addr4)) == 0) {
ZITI_LOG(DEBUG, "host[%s] port[%s] rc = %d", host, port, rc);
if ((res->ai_family == AF_UNSPEC || res->ai_family == AF_INET)
&& uv_ip4_addr(host ? host : "127.0.0.1", portnum, &addr->in4) == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if host == NULL we should just use INADDR_LOOPBACK or INADDR_ANY (see my other comment)
to avoid parsing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ekoby -

Reflecting on the ziti security model, does it make sense to allow a socket to bind to INADDR_ANY by default? If the user is zitifying, should we stop insecure practices?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are certain OSs where ``sockaddr``` has a length field. For example, FreeBSD defines

/* Socket address, internet style. */
struct sockaddr_in {
	uint8_t	sin_len;
	sa_family_t	sin_family;
	in_port_t	sin_port;
	struct	in_addr sin_addr;
	char	sin_zero[8];
};

The nice thing about uv_ip4_addr is that it hides the details. Otherwise, we need to employ a check for the existence of the field and then conditionally code for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turned out that sin_len checks are necessary as we are directly manipulating sockaddr_in members.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ekoby -

Reflecting on the ziti security model, does it make sense to allow a socket to bind to INADDR_ANY by default? If the user is zitifying, should we stop insecure practices?

zitify should not break current behavior of an app if connect/bind address is not something user is trying to zitify.

The fix addresses two defects. The getaddrinfo interface contract
allowed for host == NULL, but Ziti_resolve assumed host != NULL. zitify
nc -l 9999 would trigger a fault.  Additionally, hints was accessed
without checking to ensure it was NULL. Lastly, Ziti_resolve would leak
res and addr if the function wasn't successful.

Signed-off-by: Tom Carroll <[email protected]>
4.3BSD-Reno added length member. Check for the member and define
  variable if it exists.

Signed-off-by: Tom Carroll <[email protected]>
int rc = 0;
if ((rc = uv_ip4_addr(host, portnum, addr4)) == 0) {
ZITI_LOG(DEBUG, "host[%s] port[%s] rc = %d", host, port, rc);
if (!host) {
Copy link
Member

@ekoby ekoby Jun 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it is easier to call native getaddrinfo() here since without host it cannot be an intercepted service. And it won't incur a DNS lookup.

on the second thought we should just return EAI_NONAME (no service with this intercept). It is up to the caller(zitify) to do a fallback to getaddrinfo()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants