[tor-bugs] #4745 [Tor Relay]: Possible flaws in sockaddr validation in connection_handle_listener_read()

[Tor Bug Tracker & Wiki]

#4745: Possible flaws in sockaddr validation in connection_handle_listener_read()
-----------------------+----------------------------------------------------
 Reporter:  asn        |          Owner:     
     Type:  defect     |         Status:  new
 Priority:  normal     |      Milestone:     
Component:  Tor Relay  |        Version:     
 Keywords:             |         Parent:     
   Points:             |   Actualpoints:     
-----------------------+----------------------------------------------------
 `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?

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/4745>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online