Ziti_resolve faults if host is NULL #525
Conversation
tomc797
force-pushed
the
fix/host_null_segfault
branch
from
76c7d26
to
078b689
Compare
|
thank you for your PR. It compelled me to go look at to be more compliant, we should return |
library/zitilib.c
Outdated
| 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) { |
There was a problem hiding this comment.
if host == NULL we should just use INADDR_LOOPBACK or INADDR_ANY (see my other comment)
to avoid parsing
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Turned out that sin_len checks are necessary as we are directly manipulating sockaddr_in members.
There was a problem hiding this comment.
Hi @ekoby -
Reflecting on the ziti security model, does it make sense to allow a socket to bind to
INADDR_ANYby 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]>
Signed-off-by: Tom Carroll <[email protected]>
tomc797
force-pushed
the
fix/host_null_segfault
branch
from
078b689
to
8f876f9
Compare
| 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) { |
There was a problem hiding this comment.
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()
The fix addresses a
Ziti_resolvedefects that triggers segfault. Thegetaddrinfointerface contract permitshost == NULL, butZiti_resolveassumeshost != NULL. Additionally,Ziti_resolveleaksresandaddrif error is signaled.