Opened 5 months ago

Closed 3 months ago

#23080 closed defect (fixed)

connection_ext_or_handle_cmd_useraddr and proposal 196 disagree on the format of ExtORPort USERADDR

Reported by: dcf Owned by:
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-spec, pt-spec, needs-spec-change, spec
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

(Originally noticed in comment:3:ticket:18628.)

Proposal 196, which defines the ExtORPort protocol, implies that the USERADDR command must include a port number, here:

     [0x0001] USERADDR: an address:port string that represents the
       client's address.

and here:

3.1.2.1. USERADDR

  An ASCII string holding the TCP/IP address of the client of the
  pluggable transport proxy.

But connection_ext_or_handle_cmd_useraddr calls tor_addr_port_split, which makes the port number optional.

It seems that connection_ext_or_handle_cmd_useraddr in fact accepts all these formats for USERADDR:

  • 1.2.3.4 (implied port=0)
  • 1.2.3.4:5678
  • 1:2::3:4 (implied port=0)
  • [1:2::3:4] (implied port=0)
  • [1:2::3:4]:5678

If this is intended, then I'd like proposal 196 to say that the port is optional, and square brackets are optional in the case of IPv6.

For what it's worth, obfs4proxy and meek-server take proposal 196 at face value and always include a port in USERADDR (meek-server always uses the fictitious port number 1 because it doesn't know the true remote port).

Child Tickets

Attachments (1)

0001-Add-additional-tests-for-ExtORPort-USERADDR.patch (1.8 KB) - added by dcf 5 months ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 5 months ago by dcf

Here's a patch that adds tests for the additional USERADDR formats shown in the ticket description.

attachment:0001-Add-additional-tests-for-ExtORPort-USERADDR.patch

comment:2 Changed 5 months ago by asn

Milestone: Tor: 0.3.2.x-final

comment:3 Changed 5 months ago by nickm

I'd rather make the parser stricter to match the spec, then have the spec change to require this behavior forever.

comment:4 Changed 3 months ago by nickm

Keywords: spec added

Add 'spec' keyword to items that are just spec fixes. These can land after the feature-freeze.

comment:5 Changed 3 months ago by nickm

17d433247bdfb4 is the torspec patch.

comment:6 Changed 3 months ago by nickm

Status: newneeds_review

I have a diagnostic patch for review in bug23080. I suggest we take that one in 0.3.2, and then reject portless UserAddr commands in 0.3.3 or 0.3.4.

comment:7 in reply to:  5 Changed 3 months ago by dcf

Replying to nickm:

17d433247bdfb4 is the torspec patch.

17d433247bdfb4 seems fine.

Replying to nickm:

I have a diagnostic patch for review in bug23080. I suggest we take that one in 0.3.2, and then reject portless UserAddr commands in 0.3.3 or 0.3.4.

Branch bug23080 also looks fine.

comment:8 Changed 3 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Thanks, dcf! Merging now.

Note: See TracTickets for help on using tickets.