tor_addr_parse is overly permissive
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
Activity
changed milestone to %Tor: 0.4.0.x-final
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.
Trac:
fuzz_addr.cHere is a fuzzer for tor_addr_parse: fuzz_addr.c.
I ran it and didn't find any other unexpected inputs accepted by tor_addr_parse: fuzz_addr_findings.tar.gz
$ for a in fuzz_addr_findings/queue/*; do ./fuzz-addr --info < $a; done | grep -v errorTrac:
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
to
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
-
You can surface this bug from the command line:
$ tor-resolve -x '[138.201.14.1979' saxatile.torproject.orgThis 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_parseis throwing away the final character, and therefore failing to notice that the address is bad, because the string starts with[.Trac:
Cc: rl1987 to rl1987, neel@neelc.orgTrac:
Owner: neel to N/A
Cc: rl1987, neel@neelc.org to rl1987https://github.com/torproject/tor/pull/302
Trac:
Status: accepted to needs_reviewReplying to rl1987:
Great, rl1987! If you don't mind review from the peanut gallery:
- 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 != 0is equivalent totmp != NULL. But actually, I don't think I would try to remove thebrackets_detectedvariable, 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.+ /* 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_ptoninstead? Or is there a reason fortor_inet_ptonto accept such inputs? Maybe for compatibility withinet_pton? It's surprising to me that this check is necessary; I would have assumed (wrongly) that the underlying address parser would reject it.Replying to dcf:
Replying to rl1987:
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 != 0is equivalent totmp != NULL. But actually, I don't think I would try to remove thebrackets_detectedvariable, because it expresses clearly what it means.That's correct. However, I'm keeping
tmpvariable here, as we may need to free the temporary bracketless string thattor_strndupallocated 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_ptoninstead? Or is there a reason fortor_inet_ptonto accept such inputs? Maybe for compatibility withinet_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_ptonwill be able to benefit from that check (such asstring_is_valid_ipv6_address()function). So I moved the check in 5438ff3e13a803bf24ba98e11a0b1a6b3d839cc9. I'm not sure if that breaks any standard compatibility though.mentioned in issue #30721 (moved)
mentioned in issue #32314 (moved)
moved to tpo/core/tor#23082 (closed)
mentioned in issue tpo/core/tor#30721 (closed)
