Skip to content

Conversation

@liberat2
Copy link
Contributor

No description provided.

When a SOCKS5 client sends a RESOLVE_PTR request, it must include
either an IPv4 or IPv6 address.  In the past this was required to be a
binary address (address types 1 or 4), but since the refactoring of
SOCKS5 support in Tor 0.3.5.1-alpha, strings (address type 3) are also
allowed if they represent an IPv4 or IPv6 literal.

However, when a binary IPv6 address is provided,
parse_socks5_client_request converts it into a string enclosed in
brackets.  This doesn't match what string_is_valid_ipv6_address
expects, so this would fail with the error "socks5 received
RESOLVE_PTR command with hostname type. Rejecting."

By replacing string_is_valid_ipv4_address/string_is_valid_ipv6_address
with tor_addr_parse, we accept strings both with and without brackets.
This fixes the handling of binary addresses, and also improves
symmetry with CONNECT and RESOLVE requests.

Fixes bug 32315.
This tests the handling of binary v6 addresses, which works correctly
in older versions but was broken in 0.3.5.1-alpha.
This was not supported previously, but provides symmetry with other
SOCKS requests, which also support addresses written in brackets.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 7191

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.0006%) to 59.07%

Totals Coverage Status
Change from base Build 7182: -0.0006%
Covered Lines: 43144
Relevant Lines: 73039

💛 - Coveralls

@@ -0,0 +1,4 @@
o Major bugfixes (networking):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll probable end up categorising this as minor when we do the changelog, because no-one noticed it was broken for a few years.

Suggested change
o Major bugfixes (networking):
o Minor bugfixes (networking):

@teor2345
Copy link
Contributor

Backport branch is now #1674, with a small fix to the changes file.

@teor2345 teor2345 closed this Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants