Opened 5 years ago

Closed 5 years ago

#15497 closed defect (fixed)

torsock's getpeername() implementation is broken.

Reported by: yawning Owned by: dgoulet
Priority: Medium Milestone:
Component: Core Tor/Torsocks Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This is incredibly wrong and breaks well written applications:

      /*
       * Extra check for addrlen since we are about to copy the connection
       * content into the given address.
       */
      if (*addrlen > sizeof(struct sockaddr)) {
              /* Ref to the manpage for the returned value here. */
              errno = EINVAL;
              ret = -1;
              goto end;
      }

http://pubs.opengroup.org/onlinepubs/9699919799/functions/getpeername.html

The address_len argument points to a socklen_t object which on input specifies the length of the supplied sockaddr structure, and on output specifies the length of the stored address. If the actual length of the address is greater than the length of the supplied sockaddr structure, the stored address shall be truncated.

This does not mean "reject address_len that's larger than sizeof(struct sockaddr)", and it is common to pass in a sockaddr_in6 or sockaddr_storage both which are larger than sockaddr.

Child Tickets

Change History (2)

comment:1 Changed 5 years ago by yawning

Status: newneeds_review

https://github.com/Yawning/torsocks/compare/getpeername

Update: The code was also neglecting to store the size of the stored address in address_len, I updated and re-pushed my branch to fix that as well.

Last edited 5 years ago by yawning (previous) (diff)

comment:2 Changed 5 years ago by dgoulet

Resolution: fixed
Status: needs_reviewclosed

Merged and upstream! Thanks!

Note: See TracTickets for help on using tickets.