Opened 12 months ago

Last modified 9 months ago

#32315 merge_ready defect

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

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


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) (743 bytes) - added by liberat 12 months ago.
Script demonstrating various SOCKS5 RESOLVE_PTR requests

Download all attachments as: .zip

Change History (18)

Changed 12 months ago by liberat

Attachment: added

Script demonstrating various SOCKS5 RESOLVE_PTR requests

comment:1 Changed 12 months 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 12 months ago by nickm

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

comment:3 Changed 12 months ago by dgoulet

Keywords: ipv6 dns added
Reviewer: teor

comment:4 Changed 12 months 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 12 months 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 ?
And write a changes file:

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

comment:6 Changed 12 months 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 12 months 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 12 months 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 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("",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));

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

comment:9 Changed 12 months 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:

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, in commit bcbd3fb71e. So please base your pull request on our maint-0.3.5 branch.

comment:10 Changed 12 months ago by teor

Parent ID: #26664

comment:11 Changed 11 months ago by liberat

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

comment:12 Changed 9 months ago by nickm

Keywords: 043-can added
Status: needs_revisionneeds_review

comment:13 Changed 9 months ago by teor

Keywords: consider-backport-after-043-stable added; consider-backport-after-0425 removed

Since this is obscure code, and network layer backports have given us trouble before, let's wait until after the next stable to backport.

comment:14 Changed 9 months ago by teor

Status: needs_reviewmerge_ready

Looks good, thanks for the tests!

comment:15 Changed 9 months ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: 0.4.2.x-final

Agreed; this is a solid patch. I've merged it to master, and am marking it for backport.

comment:16 Changed 9 months ago by nickm

Note that if/when we backport we'll also want to cherry-pick 4f88eb849c8f85c7cd70fc20250270401e986efd so that lint-changes passes.

comment:17 Changed 9 months ago by teor

I made a backport branch including 4f88eb849c8f85c7cd70fc20250270401e986efd:

Note: See TracTickets for help on using tickets.