New SOCKS port restriction to only allow connections to .onion

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.

changed milestone to %Tor: 0.2.9.x-final in legacy/trac

added actualpoints::0.5 in Legacy / Trac component::core tor/tor in Legacy / Trac milestone::Tor: 0.2.9.x-final in Legacy / Trac owner::dgoulet in Legacy / Trac points::0.5 in Legacy / Trac priority::very low in Legacy / Trac resolution::implemented in Legacy / Trac review-group-8 in Legacy / Trac severity::normal in Legacy / Trac socks in Legacy / Trac sponsor::R-can in Legacy / Trac status::closed in Legacy / Trac tor-hs in Legacy / Trac type::enhancement in Legacy / Trac labels

legacy/trac#17221 (moved) requested the same functionality for Tor Browser.

Trac:
Points: N/A to small
Milestone: N/A to Tor: 0.2.???
Keywords: N/A deleted, socks, tor-hs added

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.

Trac:
Status: new to needs_review

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

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

Trac:
Points: small to small-remaining
Actualpoints: N/A to 6 hours

Trac:
Reviewer: N/A to dgoulet

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.

Trac:
Reviewer: dgoulet to special
Status: needs_review to needs_revision
Sponsor: N/A to SponsorR-can

Trac:
Owner: N/A to teor
Status: needs_revision to assigned

Trac:
Status: assigned to needs_revision

Trac:
Actualpoints: 6 hours to .5
Points: small-remaining to .5

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

Trac:
Status: needs_revision to needs_information

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.

Trac:
Owner: teor to N/A
Status: needs_information to assigned

Trac:
Status: assigned to needs_revision

Trac:
Owner: N/A to dgoulet
Status: needs_revision to accepted
Reviewer: special to N/A
Points: .5 to 0.5
Actualpoints: .5 to 0.5

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.

Trac:
Status: accepted to needs_review

Trac:
Keywords: N/A deleted, review-group-8 added

In b311f820 -- 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.

Trac:
Status: needs_review to closed
Resolution: N/A to implemented

Replying to nickm:

In b311f820 -- 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.

closed

changed time estimate to 4h

added 4h of time spent

mentioned in issue legacy/trac#20109 (moved)

mentioned in issue legacy/trac#25617 (moved)

moved from legacy/trac#18693 (moved)

added Feature label and removed 1 deleted label

added 1 deleted label and removed 1 deleted label