Skip to content

Pr1529 changes fix #1674

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: maint-0.3.5
Choose a base branch
from

Conversation

teor2345
Copy link
Contributor

No description provided.

liberat2 and others added 4 commits November 11, 2019 15:34
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.
@teor2345 teor2345 mentioned this pull request Jan 20, 2020
@coveralls
Copy link

Pull Request Test Coverage Report for Build 7823

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.5%) to 59.733%

Totals Coverage Status
Change from base Build 7797: 0.5%
Covered Lines: 43704
Relevant Lines: 73166

💛 - Coveralls

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.

4 participants