Possible flaws in sockaddr validation in connection_handle_listener_read()
connection_handle_listener_read()
:
...
if (conn->socket_family == AF_INET || conn->socket_family == AF_INET6) {
tor_addr_t addr;
uint16_t port;
if (check_sockaddr(remote, remotelen, LOG_INFO)<0) {
log_info(LD_NET,
"accept() returned a strange address; trying getsockname().");
remotelen=sizeof(addrbuf);
memset(addrbuf, 0, sizeof(addrbuf));
if (getsockname(news, remote, &remotelen)<0) {
int e = tor_socket_errno(news);
log_warn(LD_NET, "getsockname() for new connection failed: %s",
tor_socket_strerror(e));
} else {
if (check_sockaddr((struct sockaddr*)addrbuf, remotelen,
LOG_WARN) < 0) {
log_warn(LD_NET,"Something's wrong with this conn. Closing it.");
tor_close_socket(news);
return 0;
}
}
}
...
If both check_sockaddr()
and getsockname()` fail, we continue with a corrupted remote (and sockaddr), instead of closing the socket and returning 0.
I'm not sure if this is a bug, but it feels weird, since if the second check_sockaddr()
fails, we close the socket and return 0, which is what I feel that should happen.
I'm also not sure how someone can make both check_sockaddr()
and getsockname()fail, and I'm not sure how
remote` would have to look in this case or what would happen afterwards.
All in all, is the validation here intended to be like this?