[tor-bugs] #23082 [Core Tor/Tor]: tor_addr_parse is overly permissive

[Tor Bug Tracker & Wiki]

#23082: tor_addr_parse is overly permissive
---------------------------------+----------------------------------
 Reporter:  dcf                  |          Owner:  rl1987
     Type:  defect               |         Status:  needs_review
 Priority:  Medium               |      Milestone:  Tor: unspecified
Component:  Core Tor/Tor         |        Version:  Tor: unspecified
 Severity:  Normal               |     Resolution:
 Keywords:  032-unreached, ipv6  |  Actual Points:
Parent ID:                       |         Points:
 Reviewer:                       |        Sponsor:
---------------------------------+----------------------------------

Comment (by dcf):

 Replying to [comment:14 rl1987]:
 > https://github.com/torproject/tor/pull/302

 Great, rl1987! If you don't mind review from the peanut gallery:
 {{{#!diff
 -  if (src[0] == '[' && src[1])
 +  if (src[0] == '[' && src[1] && src[strlen(src)-1] == ']') {
 }}}
 I think the condition `src[1]` is now redundant. It was meant to ensure
 that the string contains at least 2 characters, but the newly added
 condition also does that.

 I note that the condition `brackets_detected != 0` is equivalent to `tmp
 != NULL`. But actually, I don't think I would try to remove the
 `brackets_detected` variable, because it expresses clearly what it means.

 I think the `tor_inet_pton(AF_INET6, ...)` line could use a comment saying
 that IPv6 addresses are allowed to be with or without brackets.

 {{{#!diff
 +  /* Reject if src has needless trailing ':'. */
 +  if (len > 2 && src[len - 1] == ':' && src[len - 2] != ':') {
 +    result = -1;
 +  } else if (tor_inet_pton(AF_INET6, src, &in6_tmp) > 0) {
 }}}
 I wonder, does this check for a trailing colon belong in `tor_inet_pton`
 instead? Or is there a reason for `tor_inet_pton` to accept such inputs?
 Maybe for compatibility with `inet_pton`? It's surprising to me that this
 check is necessary; I would have assumed (wrongly) that the underlying
 address parser would reject it.

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