Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#18693 closed enhancement (implemented)

New SOCKS port restriction to only allow connections to .onion

Reported by: ioerror Owned by: dgoulet
Priority: Very Low Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, socks, review-group-8
Cc: special Actual Points: 0.5
Parent ID: Points: 0.5
Reviewer: Sponsor: SponsorR-can

Description

While working on Ricochet to make it the a post-TCP/IP IM client, special and I have been considering ways to sandbox it further. We decided it would be nice to have a way to mark a given tor SOCKS (unix socket) listener as only allowing connections to .onion addresses.

This is similar to setting the option to not allow IPv4 - except we don't want DNS, IPv6 or any connection except to an onion service.

Child Tickets

Change History (21)

comment:1 Changed 4 years ago by cypherpunks

#17221 requested the same functionality for Tor Browser.

comment:2 Changed 4 years ago by dgoulet

Keywords: tor-hs socks added
Milestone: Tor: 0.2.???
Points: small

comment:3 Changed 4 years ago by teor

Please see my branch feature-18693-v3 at https://github.com/teor2345/tor.git

It implements the OnionTrafficOnly Port flag, which disables all non-onion sites through that port.

It can be tested using:
src/or/tor DataDirectory /tmp/tor.$$ SOCKSPort "12345 OnionTrafficOnly"

Implementation details:

  • Adds the NoDNSRequest flag, which refuses requests for non-onion hostnames
  • Modifies the NoIPv4Traffic and NoIPv6Traffic flags so they refuse connections earlier, before attaching a stream
  • Adds the OnionTrafficOnly flag, which sets NoDNSRequest, NoIPv4Traffic, and NoIPv6Traffic, refusing all non-onion requests
  • Stops Tor's existing behaviour of allowing IPv4 and IPv6 traffic on all non-SOCKS Ports. This makes this feature usable with TransPort and NATDPort
  • Adds some unit tests and a manual page update
  • A few comment and non-functional tweaks

Features you didn't ask for:

  • Adds the NoOnionTraffic flag, which disables requests for onion hostnames (for completeness)

If this works for you, let me know (and do a code review!), and I can ask Nick and Isabela if we can get it in 0.2.9.

comment:4 Changed 4 years ago by dgoulet

Status: newneeds_review

comment:5 Changed 4 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

If the code is written, let's try to get it tested reviewed and merged

comment:6 Changed 4 years ago by teor

Actual Points: 6 hours
Points: smallsmall-remaining

comment:7 Changed 4 years ago by dgoulet

Reviewer: dgoulet

comment:8 Changed 4 years ago by special

6865f70446fa7f62e2d5dbb5a0691c673ec6eb33

nitpick to tor.1.txt: you specifically refer to SOCKS5, but these flags also apply for SOCKS4a

+ log_warn(LD_CONFIG, "You have a %sPort entry with DNSRequest enabled, "
+ "but IPv4 and IPv6 disabled; DNS-based sites won't work.",
+ portname);

This is a valid configuration for a SOCKS port that only handles RESOLVE requests, isn't it?

f63b322a77e41942546675f5229e134f50fc4b63

So if I understand correctly, this is a behavior change: NATD and Trans ports will no longer allow IPv6 traffic by default. Is that right?

26a041a71cb62708c458e61f09eb9512d75ae074
5af508e2b7e7c87bb04d4987a5e4d9063ebd9e41
a54bee889ed026e341ae945c65a4869080bbbaff
81b8a2b60f2f1cfcde86e3f3ffe9e9b6d8a895f7

OK

eafe73e6f2ba821ad465740ff7ea7e4b6fbabd11

Log message should be using safe_str_client. Also, this one is LD_NET, but the others were LD_APP.

--

I really wish we had automated tests to make sure connections actually fail when the port policy should reject them. I guess that might be hard to do right now.

Code looks ok to me other than the above. Haven't tested it myself yet.

comment:9 Changed 4 years ago by dgoulet

Reviewer: dgouletspecial
Sponsor: SponsorR-can
Status: needs_reviewneeds_revision

comment:10 Changed 3 years ago by nickm

Owner: set to teor
Status: needs_revisionassigned

comment:11 Changed 3 years ago by nickm

Status: assignedneeds_revision

comment:12 Changed 3 years ago by nickm

Actual Points: 6 hours.5
Points: small-remaining.5

comment:13 Changed 3 years ago by dgoulet

Status: needs_revisionneeds_information

Ok I jumped on this one so we can move it forward for 029. The code is good! I've rebased it on master and fixed special's comment in fixup commit c39110f.

I've also added two extra fixup commits for minor syntax issues in comments. I've tested all options and it works fine.

Although, I would really want an explanation for commit a6f8fe9 (like special mentioned). Why is this a fix all of a sudden? Maybe we can improve the comment there telling us _why_ we do that?

Branch: ticket18693_029_01

comment:14 in reply to:  8 Changed 3 years ago by teor

Owner: teor deleted
Status: needs_informationassigned

Replying to special:

+ log_warn(LD_CONFIG, "You have a %sPort entry with DNSRequest enabled, "
+ "but IPv4 and IPv6 disabled; DNS-based sites won't work.",
+ portname);

This is a valid configuration for a SOCKS port that only handles RESOLVE requests, isn't it?

Well, let's not do that then.
But don't they have to use IPv4 or IPv6 to process the resolve? Or do they just ask the Exit?

f63b322a77e41942546675f5229e134f50fc4b63

So if I understand correctly, this is a behavior change: NATD and Trans ports will no longer allow IPv6 traffic by default. Is that right?

Oops, we don't want that.

I think it's better to set these defaults when we process the port configuration line, because otherwise they override the settings in the port configuration itself (you can't turn IPv6 off, at least in the onion-only case, and maybe other cases as well).

This is complicated by the fact that port configs are initialised in 3 different places. It will be easier to keep the NATD and Trans behaviour if that's refactored into one place.

Un-assigning from me because I'm not sure if I can do this patch before 0.2.9.

comment:15 Changed 3 years ago by teor

Status: assignedneeds_revision

comment:16 Changed 3 years ago by dgoulet

Actual Points: .50.5
Owner: set to dgoulet
Points: .50.5
Reviewer: special
Status: needs_revisionaccepted

comment:17 Changed 3 years ago by dgoulet

Status: acceptedneeds_review

I addressed the latest from teor here. I've removed the NATD and Trans port restriction for IPv6. I've rebased this to current git master as well:

See branch ticket18693_029_01.

comment:18 Changed 3 years ago by nickm

Keywords: review-group-8 added

comment:19 Changed 3 years ago by nickm

In b311f82026d51141a2ef6dd4a709d41a0dd3c388 -- what should we do if IPv4Traffic and IPv6Traffic are both disabled, but DNSTraffic is enabled, and we get a hostname? Right now it looks like we accept the request. Is that right? If not, please open a ticket.

Otherwise looks good to me. Merging.

comment:20 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

comment:21 in reply to:  19 Changed 3 years ago by teor

Replying to nickm:

In b311f82026d51141a2ef6dd4a709d41a0dd3c388 -- what should we do if IPv4Traffic and IPv6Traffic are both disabled, but DNSTraffic is enabled, and we get a hostname? Right now it looks like we accept the request. Is that right? If not, please open a ticket.

Yes, this is the intended behaviour - some unusual clients might just want to look up names, and not transmit traffic.

Otherwise looks good to me. Merging.

Note: See TracTickets for help on using tickets.