#27620 closed enhancement (implemented)

Use trunnel to parse and generate SOCKS wire format in tor-resolve

Reported by: rl1987 Owned by: rl1987
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: trunnel wireformat socks tor-resolve security
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description


Child Tickets

Change History (14)

comment:1 Changed 12 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:3 Changed 12 months ago by rl1987

Status: acceptedneeds_review

comment:4 Changed 12 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.6.x-final

comment:5 Changed 11 months ago by dgoulet

Reviewer: catalyst

comment:6 Changed 11 months ago by asn

Reviewer: catalystdgoulet

comment:7 Changed 11 months ago by dgoulet

Status: needs_reviewneeds_revision

Code lgtm overall. Only issue I found is that when I test tor-resolve with SOCKS5, I get the IP in reverse order :)

$ ./src/tools/tor-resolve -4 torproject.org
95.216.163.36
$ ./src/tools/tor-resolve -5 torproject.org
36.163.216.95

So I'm assuming some place there is a ntohl() missing? :)

comment:8 Changed 11 months ago by rl1987

Fixed in 38390604a7e3215483d4a53d7a18b45f93e76828.

comment:9 Changed 11 months ago by rl1987

Status: needs_revisionneeds_review

comment:10 Changed 11 months ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;

(Needs autosquash)

comment:11 Changed 11 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:12 Changed 10 months ago by nickm

Made a squashed & rebased version as https://github.com/torproject/tor/pull/556 .

So far the only suggestions I have are:

  • We should be using strlcpy, not strncpy.
  • If we allow socks4_server_reply_version to be zero, then we'll need to explicitly set it to 4 in proto_socks.c so that we're sending the right value from Tor, right?
  • Do we have a ticket for adding tests for tor-resolve?

I'll make all the above changes before merging, if the CI passes.

comment:13 in reply to:  12 Changed 10 months ago by nickm

Replying to nickm:

Made a squashed & rebased version as https://github.com/torproject/tor/pull/556 .

So far the only suggestions I have are:

  • We should be using strlcpy, not strncpy.

Though actually, tor_strdup() would be fine here, since the trunnel "getstr" functions return nul-terminated strings.

  • If we allow socks4_server_reply_version to be zero, then we'll need to explicitly set it to 4 in proto_socks.c so that we're sending the right value from Tor, right?

Oh. apparently we don't do that. That's okay.

  • Do we have a ticket for adding tests for tor-resolve?

Yes, #27854, a child of this ticket.

I'll make all the above changes before merging, if the CI passes.

Done, merged.

comment:14 Changed 10 months ago by nickm

Resolution: implemented
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.