[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 rl1987):

 Replying to [comment:15 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 made some cleanups in ad165f7aef4e2feefd6d4cde5d57dfd4fa6d55f5.

 > 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.
 >

 That's correct. However, I'm keeping `tmp` variable here, as we may need
 to free the temporary bracketless string that `tor_strndup` allocated if
 brackets were detected. Let's have both variables.

 > 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.
 >

 Well, function documentation for `tor_addr_parse()` mentions that already.
 I don't think that additional comment is needed.

 > {{{#!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.

 That's a good idea, as other code that depends on `tor_inet_pton` will be
 able to benefit from that check (such as `string_is_valid_ipv6_address()`
 function). So I moved the check in
 5438ff3e13a803bf24ba98e11a0b1a6b3d839cc9. I'm not sure if that breaks any
 standard compatibility though.

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