Re: [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:  closed
 Priority:  Medium   |  Milestone:  Tor: 0.3.6.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:  fixed
 Keywords:  032-unreached, ipv6  |  Actual Points:
Parent ID:   | Points:
 Reviewer:  nickm|Sponsor:
-+
Changes (by nickm):

 * status:  needs_review => closed
 * resolution:   => fixed


Comment:

 LGTM; merged!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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: 0.3.6.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  032-unreached, ipv6  |  Actual Points:
Parent ID:   | Points:
 Reviewer:  nickm|Sponsor:
-+
Changes (by asn):

 * reviewer:  catalyst => nickm


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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: 0.3.6.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  032-unreached, ipv6  |  Actual Points:
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+
Changes (by teor):

 * version:  Tor: unspecified =>
 * milestone:  Tor: unspecified => Tor: 0.3.6.x-final


Comment:

 Assigning this to 0.3.6, because last time we changed our address code, we
 broke IPv6 exiting and didn't notice.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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, _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: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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, _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: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
-+--
Changes (by rl1987):

 * status:  accepted => needs_review


Comment:

 https://github.com/torproject/tor/pull/302

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  accepted
 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:
-+--
Changes (by rl1987):

 * owner:  (none) => rl1987
 * status:  new => accepted


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  (none)
 Type:  defect   | Status:  assigned
 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:
-+--
Changes (by neel):

 * cc: neel@… (removed)
 * owner:  neel => (none)


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  (none)
 Type:  defect   | Status:  new
 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:
-+--
Changes (by neel):

 * status:  assigned => new


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  neel
 Type:  defect   | Status:  assigned
 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:
-+--
Changes (by neel):

 * cc: neel@… (added)


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  neel
 Type:  defect   | Status:  assigned
 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:
-+--
Changes (by neel):

 * status:  new => assigned
 * owner:  (none) => neel


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  (none)
 Type:  defect   | Status:  new
 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:
-+--
Changes (by rl1987):

 * cc: rl1987 (added)


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  (none)
 Type:  defect   | Status:  new
 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:
-+--
Changes (by teor):

 * keywords:  032-unreached => 032-unreached, ipv6
 * version:  Tor: 0.3.1.5-alpha => Tor: unspecified


Comment:

 This bug was introduced long before 0.3.1.5.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  (none)
 Type:  defect | Status:  new
 Priority:  Medium |  Milestone:  Tor: unspecified
Component:  Core Tor/Tor   |Version:  Tor: 0.3.1.5-alpha
 Severity:  Normal | Resolution:
 Keywords:  032-unreached  |  Actual Points:
Parent ID: | Points:
 Reviewer: |Sponsor:
---+

Comment (by dcf):

 You can surface this bug from the command line:
 {{{
 $ tor-resolve -x '[138.201.14.1979'
 saxatile.torproject.org
 }}}
 This command should result in an error, but doesn't. Notice there are four
 digits in the last octet of the bogus address `[138.201.14.1979`.
 saxatile's IP address is 138.201.14.197. `tor_addr_parse` is throwing away
 the final character, and therefore failing to notice that the address is
 bad, because the string starts with `[`.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  (none)
 Type:  defect| Status:  new
 Priority:  Medium|  Milestone:  Tor: 0.3.2.x-final
Component:  Core Tor/Tor  |Version:  Tor: 0.3.1.5-alpha
 Severity:  Normal| Resolution:
 Keywords:|  Actual Points:
Parent ID:| Points:
 Reviewer:|Sponsor:
--+
Description changed by dcf:

Old description:

> tor_addr_parse allows these surprising address formats:
>  * `[192.0.2.1]` (IPv4 in square brackets) → 192.0.2.1
>  * `[11.22.33.44` (IPv4 with left square bracket only) → 11.22.33.4
>  * `[11:22::33:44` (IPv6 with left square bracket only) → 11:22::33:44
>  * `11:22::33:44:` (IPv6 with trailing colon) → 11:22::33:44

New description:

 tor_addr_parse allows these surprising address formats:
  * `[192.0.2.1]` (IPv4 in square brackets) → 192.0.2.1
  * `[11.22.33.44` (IPv4 with left square bracket only) → 11.22.33.4
  * `[11:22::33:44` (IPv6 with left square bracket only) → 11:22::33:4
  * `11:22::33:44:` (IPv6 with trailing colon) → 11:22::33:44

--

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 Type:  defect| Status:  new
 Priority:  Medium|  Milestone:  Tor: 0.3.2.x-final
Component:  Core Tor/Tor  |Version:  Tor: 0.3.1.5-alpha
 Severity:  Normal| Resolution:
 Keywords:|  Actual Points:
Parent ID:| Points:
 Reviewer:|Sponsor:
--+

Comment (by dcf):

 Here is a fuzzer for tor_addr_parse: attachment:fuzz_addr.c.

 I ran it and didn't find any other unexpected inputs accepted by
 tor_addr_parse: attachment:fuzz_addr_findings.tar.gz
 {{{
 $ for a in fuzz_addr_findings/queue/*; do ./fuzz-addr --info < $a; done |
 grep -v error
 }}}

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 Type:  defect| Status:  new
 Priority:  Medium|  Milestone:  Tor: 0.3.2.x-final
Component:  Core Tor/Tor  |Version:  Tor: 0.3.1.5-alpha
 Severity:  Normal| Resolution:
 Keywords:|  Actual Points:
Parent ID:| Points:
 Reviewer:|Sponsor:
--+
Changes (by asn):

 * milestone:   => Tor: 0.3.2.x-final


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 Type:  defect| Status:  new
 Priority:  Medium|  Milestone:
Component:  Core Tor/Tor  |Version:  Tor: 0.3.1.5-alpha
 Severity:  Normal| Resolution:
 Keywords:|  Actual Points:
Parent ID:| Points:
 Reviewer:|Sponsor:
--+

Comment (by dcf):

 attachment:0001-Add-tests-for-tor_addr_parse-separate-from-
 tor_addr_.patch​ adds tests for tor_addr_parse, including the cases in the
 ticket description.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

[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:
 Type:  defect| Status:  new
 Priority:  Medium|  Milestone:
Component:  Core Tor/Tor  |Version:  Tor: 0.3.1.5-alpha
 Severity:  Normal|   Keywords:
Actual Points:|  Parent ID:
   Points:|   Reviewer:
  Sponsor:|
--+
 tor_addr_parse allows these surprising address formats:
  * `[192.0.2.1]` (IPv4 in square brackets) → 192.0.2.1
  * `[11.22.33.44` (IPv4 with left square bracket only) → 11.22.33.4
  * `[11:22::33:44` (IPv6 with left square bracket only) → 11:22::33:44
  * `11:22::33:44:` (IPv6 with trailing colon) → 11:22::33:44

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs