Opened 20 months ago

Closed 5 months ago

Last modified 5 months ago

#23082 closed defect (fixed)

tor_addr_parse is overly permissive

Reported by: dcf Owned by: rl1987
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 032-unreached, ipv6
Cc: rl1987 Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description (last modified by dcf)

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

Child Tickets

Attachments (3)

0001-Add-tests-for-tor_addr_parse-separate-from-tor_addr_.patch (2.6 KB) - added by dcf 20 months ago.
fuzz_addr_findings.tar.gz (16.7 KB) - added by dcf 20 months ago.
Output of afl-fuzz -i src/test/fuzz/fuzz_addr_testcases -o src/test/fuzz/fuzz_addr_findings -- src/test/fuzz/fuzz-addr
fuzz_addr.c (711 bytes) - added by dcf 20 months ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 20 months ago 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.

comment:2 Changed 20 months ago by asn

Milestone: Tor: 0.3.2.x-final

Changed 20 months ago by dcf

Attachment: fuzz_addr_findings.tar.gz added

Output of afl-fuzz -i src/test/fuzz/fuzz_addr_testcases -o src/test/fuzz/fuzz_addr_findings -- src/test/fuzz/fuzz-addr

Changed 20 months ago by dcf

Attachment: fuzz_addr.c added

comment:3 Changed 20 months ago 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

comment:4 Changed 19 months ago by dcf

Description: modified (diff)

comment:5 Changed 18 months ago by nickm

Keywords: 032-unreached added
Milestone: Tor: 0.3.2.x-finalTor: unspecified

Mark a large number of tickets that I do not think we will do for 0.3.2.

comment:6 Changed 16 months ago 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 [.

comment:7 Changed 9 months ago by teor

Keywords: ipv6 added
Version: Tor: 0.3.1.5-alphaTor: unspecified

This bug was introduced long before 0.3.1.5.

comment:8 Changed 9 months ago by rl1987

Cc: rl1987 added

comment:9 Changed 8 months ago by neel

Owner: set to neel
Status: newassigned

comment:10 Changed 8 months ago by neel

Cc: neel@… added

comment:11 Changed 8 months ago by neel

Cc: neel@… removed
Owner: neel deleted

comment:12 Changed 8 months ago by neel

Status: assignednew

comment:13 Changed 7 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:14 Changed 7 months ago by rl1987

Status: acceptedneeds_review

comment:15 in reply to:  14 ; Changed 7 months ago by dcf

Replying to rl1987:

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

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

+  /* 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.

comment:16 in reply to:  15 Changed 7 months ago by rl1987

Replying to dcf:

Replying to rl1987:

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

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

+  /* 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.

comment:17 Changed 7 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.6.x-final
Version: Tor: unspecified

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

comment:18 Changed 5 months ago by dgoulet

Reviewer: catalyst

comment:19 Changed 5 months ago by asn

Reviewer: catalystnickm

comment:20 Changed 5 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

LGTM; merged!

comment:21 Changed 5 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

Note: See TracTickets for help on using tickets.