Opened 7 weeks ago

Last modified 5 weeks ago

#32315 needs_revision defect

Can't perform reverse DNS lookup for a (binary) IPv6 address

Reported by: liberat Owned by:
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version: Tor: 0.3.5.1-alpha
Severity: Normal Keywords: ipv6, dns, BugSmashFund, consider-backport-after-0425, 035-backport, 040-backport, 041-backport, 042-backport
Cc: Actual Points: 0.1
Parent ID: #26664 Points: 0.5
Reviewer: teor Sponsor:

Description

Tor provides the "RESOLVE_PTR" (0xF1) command as an extension to the SOCKS5 protocol, which can in theory be used to perform a reverse DNS lookup of an IPv4 or IPv6 address.

As with other SOCKS5 commands, this command takes an argument which can either be a binary IPv4 or IPv6 address, or an ASCII string.

Somewhat contrary to ticket #32314, this command works for IPv6 addresses only if the address is specified as an ASCII string (address type 3) with no brackets. If the address is specified in binary (address type 4), or as a string enclosed in brackets, Tor will reject it.

Child Tickets

Attachments (1)

test-resolve-ptr.py (743 bytes) - added by liberat 7 weeks ago.
Script demonstrating various SOCKS5 RESOLVE_PTR requests

Download all attachments as: .zip

Change History (12)

Changed 7 weeks ago by liberat

Attachment: test-resolve-ptr.py added

Script demonstrating various SOCKS5 RESOLVE_PTR requests

comment:1 Changed 6 weeks ago by liberat

This seems to be straightforward to fix:

--- a/src/core/proto/proto_socks.c
+++ b/src/core/proto/proto_socks.c
@@ -615,6 +615,7 @@ process_socks5_client_request(socks_request_t *req,
                               int safe_socks)
 {
   socks_result_t res = SOCKS_RESULT_DONE;
+  tor_addr_t tmpaddr;
 
   if (req->command != SOCKS_COMMAND_CONNECT &&
       req->command != SOCKS_COMMAND_RESOLVE &&
@@ -625,8 +626,7 @@ process_socks5_client_request(socks_request_t *req,
   }
 
   if (req->command == SOCKS_COMMAND_RESOLVE_PTR &&
-      !string_is_valid_ipv4_address(req->address) &&
-      !string_is_valid_ipv6_address(req->address)) {
+      tor_addr_parse(&tmpaddr, req->address) < 0) {
     socks_request_set_socks5_error(req, SOCKS5_ADDRESS_TYPE_NOT_SUPPORTED);
     log_warn(LD_APP, "socks5 received RESOLVE_PTR command with "
                      "hostname type. Rejecting.");

Eventually, in connection_ap_handshake_send_resolve, the string will be passed to tor_addr_parse_PTR_name, which accepts IPv6 literals either with or without brackets.

comment:2 Changed 6 weeks ago by nickm

Milestone: Tor: 0.4.3.x-final
Status: newneeds_review

comment:3 Changed 6 weeks ago by dgoulet

Keywords: ipv6 dns added
Reviewer: teor

comment:4 Changed 5 weeks ago by teor

Just to be clear, this fix doesn't allow binary addresses. It just allows IPv6 strings.

And we should also write tests for this feature, so we don't accidentally break it in future.

comment:5 Changed 5 weeks ago by teor

I'll review this ticket on Monday, I don't have a lot of time left this week.

Can you submit a GitHub pull request to https://github.com/torproject/tor ?
And write a changes file:
https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n95

If not, that's ok, I can do them on Monday.

comment:6 Changed 5 weeks ago by teor

We should also try to work out how old this bug is: if it goes back to 0.2.9 or 0.3.5, we are still maintaining those branches. So it should be based on maint-0.2.9 or maint-0.3.5.

comment:7 Changed 5 weeks ago by liberat

Sorry I didn't explain what's going on there.

If the client specifies a binary IPv4 or IPv6 address in the SOCKS request, it is converted into an ASCII string before any further processing is done. This conversion is done by parse_socks5_client_request, regardless of whether the request is a CONNECT, RESOLVE, or RESOLVE_PTR.

In the case of a binary IPv6 address, it is converted to a string by calling tor_addr_to_str with the 'decorate' parameter set to 1, which adds brackets around it.

Later on, process_socks5_client_request is called, and the above check takes place. string_is_valid_ipv6_address only accepts addresses without brackets, so the bracketed string is rejected.

So, no, the above patch should address both "binary" and "ASCII" client requests.

I'll try to write some tests and make a pull request this weekend.

comment:8 Changed 5 weeks ago by liberat

Looking at the latest maint-0.2.9 / maint-0.3.5 branches:

0.2.9 doesn't allow ASCII addresses for RESOLVE_PTR. It only allows binary v4 or v6 addresses, and both seem to work correctly. If you try to use an ASCII address with RESOLVE_PTR, it shows the error message:

socks5 received RESOLVE_PTR command with hostname type. Rejecting.

0.3.5 shows the same symptoms as described above for 0.4.1.6: it accepts either a binary address or an ASCII address. But in the case of IPv6, binary addresses are rejected with the same error message:

socks5 received RESOLVE_PTR command with hostname type. Rejecting.

which is now somewhat misleading.

Here's a unit test to check for correct handling of IPv6 binary addresses:

--- a/src/test/test_socks.c
+++ b/src/test/test_socks.c
@@ -277,6 +277,23 @@ test_socks_5_supported_commands(void *ptr)
   tt_str_op("2.2.2.5",OP_EQ, socks->address);
 
   tt_int_op(0,OP_EQ, buf_datalen(buf));
+  socks_request_clear(socks);
+
+  /* SOCKS 5 Send RESOLVE_PTR [F1] for an IPv6 address */
+  ADD_DATA(buf, "\x05\x01\x00");
+  ADD_DATA(buf, "\x05\xF1\x00\x04"
+           "\x20\x01\x0d\xb8\x85\xa3\x00\x00\x00\x00\x8a\x2e\x03\x70\x73\x34"
+           "\x12\x34");
+  tt_int_op(fetch_from_buf_socks(buf, socks, get_options()->TestSocks,
+                                 get_options()->SafeSocks),
+            OP_EQ, 1);
+  tt_int_op(5,OP_EQ, socks->socks_version);
+  tt_int_op(2,OP_EQ, socks->replylen);
+  tt_int_op(5,OP_EQ, socks->reply[0]);
+  tt_int_op(0,OP_EQ, socks->reply[1]);
+  tt_str_op("[2001:db8:85a3::8a2e:370:7334]",OP_EQ, socks->address);
+
+  tt_int_op(0,OP_EQ, buf_datalen(buf));
 
  done:
   ;

What's the best way to perform a more complete test?

comment:9 Changed 5 weeks ago by teor

Actual Points: 0.1
Keywords: BugSmashFund consider-backport-after-0425 035-backport 040-backport 041-backport 042-backport added
Points: 0.5
Status: needs_reviewneeds_revision
Version: Tor: 0.4.1.6Tor: 0.3.5.1-alpha

I think that's enough testing, but we should also test with an IPv6 ASCII string in brackets.

After you've done that test, please go ahead and make a pull request.

It looks like this bug was introduced in 0.3.5.1-alpha, in commit bcbd3fb71e. So please base your pull request on our maint-0.3.5 branch.

comment:10 Changed 5 weeks ago by teor

Parent ID: #26664

comment:11 Changed 5 weeks ago by liberat

Thanks! I've submitted this as pull request #1529.

Note: See TracTickets for help on using tickets.