Opened 4 months ago

Last modified 8 weeks ago

#33768 assigned defect

Make tor_inet_pton() handle bad addresses consistently on Windows

Reported by: teor Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 044-should, 035-backport, 041-backport, 042-backport, 043-backport, outreachy-ipv6, ipv6, windows, postfreeze-ok
Cc: MrSquanchee Actual Points:
Parent ID: #33049 Points: 1
Reviewer: Sponsor: Sponsor55-must

Description

tor_inet_pton() handles bad addresses differently on Windows and Linux/macOS.

For example, the address: "2000::1a00::1000:fc098" (two "::") fails this test on Windows, but succeeds on Linux and macOS:
https://github.com/torproject/tor/pull/1831/commits/05f4f93722d46c0e8f1d09b4dea4bf5d1743d93f#diff-048243cd6d9ed36dda0944181d8ec8abR1729

Let's fix this bug and backport it.

In general, we should make all the functions in this file behave identically:

  • zero any out parameters at the start of the function
  • zero any out parameters on failure

Child Tickets

TicketStatusOwnerSummaryComponent
#33788closedCheck the return value of tor_inet_ntop() and tor_inet_ntoa()Core Tor/Tor

Change History (5)

comment:1 Changed 4 months ago by teor

Let's also work out why the data in the returned array is different. We might have a bigger underlying bug here.

comment:2 Changed 4 months ago by teor

Parent ID: #33049
Sponsor: Sponsor55-canSponsor55-must

This might be a security issue on Windows relays, if they are getting IPv6 addresses from untrusted sources.

comment:3 Changed 4 months ago by teor

Keywords: security-low removed

I have confirmed that this is not currently a security issue: all calls to tor_inet_pton() check the return value.

All the other functions in that file can't return incomplete results, because they don't build the result incrementally in the returned variable(s).

I've noticed some places where return values aren't checked, I'll make child tickets.

comment:4 Changed 2 months ago by nickm

Keywords: postfreeze-ok added

Mark tickets which are important or safe enough to look at post-freeze for 0.4.4.

comment:5 Changed 8 weeks ago by nickm

Owner: set to dgoulet
Status: newassigned

It's possible part of this is already done in the child ticket.

Note: See TracTickets for help on using tickets.