Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#4745 closed defect (fixed)

Possible flaws in sockaddr validation in connection_handle_listener_read()

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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?

Child Tickets

Change History (6)

comment:1 Changed 9 years ago by nickm

Milestone: Tor: 0.2.2.x-final

It looks like an extra "tor_close_socket(); return 0;" is in order here.

(This isn't actually likely to come up, since getsockname() seems to be only allowed to fail for reasons that can't actually happen with this code.... but then again, if everything that seems impossible never happened, we would have far fewer bugs.)

Oh! Also, this shouldn't be getsockname! It should be getpeername... which makes me think that whatever was making check_sockaddr trigger for arma back in 2005 is no longer happening. Opening a new bug for that one.

comment:2 Changed 9 years ago by nickm

Status: newneeds_review

The branch bug4745_4747 in my public repo has a fix for this bug, and for the related #4747 mentioned above. Please review?

comment:3 Changed 9 years ago by nickm

See #1240 instead.

comment:4 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Bug #1240 is now fixed, rendering this one irrelevant.

comment:5 Changed 8 years ago by nickm

Keywords: tor-relay added

comment:6 Changed 8 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.