Opened 4 years ago

Last modified 12 months ago

#11010 needs_revision enhancement

add ClientConnectPolicy config option

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-client interface plaintextports
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

There should be a config option called ClientExitPolicy which specifies which destinations clients are allowed to build circuits to. The default value should be "accept *:*".

This will deprecate the RejectPlaintextPorts option as it will be able to provide the same functionality.

I am beginning to work on a patch for this.

Child Tickets

Attachments (2)

0001-add-ClientConnectPolicy-config-option-11010.patch (8.0 KB) - added by cypherpunks 4 years ago.
0001-add-ClientConnectPolicy-config-option-11010-take2.patch (8.1 KB) - added by cypherpunks 4 years ago.
second attempt

Download all attachments as: .zip

Change History (25)

comment:1 Changed 4 years ago by cypherpunks

Summary: add ClientExitPolicy config optionadd ClientConnectPolicy config option

ln5 suggests calling it ClientConnectPolicy instead

comment:2 Changed 4 years ago by nickm

Keywords: tor-client added
Milestone: Tor: unspecified

Sounds like a good idea. We can talk about the name some more once the patch is done.

The code could probably reuse the existing policies.c logic. If this is going to be port-based only, it should use the shortpolicy logic from that module.

(I'm not sure how you could reasonably make this cover address *and* port, since we don't know what the IP of a hostname will be before we have made a connection.)

comment:3 Changed 4 years ago by cypherpunks

The patch above is my very first for tor, and the most C I've written in a while, so I'm looking forward to hearing what I've done wrong :)

One thing I don't understand is what the expression !conn->use_begindir && !conn->chosen_exit_name && !circ in connection_edge.c is for - I just copied that from the consider_plaintext_ports code directly above the code I inserted. (Under which circumstances would that expression be expected to evaluate to false?)

Last edited 4 years ago by cypherpunks (previous) (diff)

comment:4 Changed 4 years ago by cypherpunks

Oops, I think the handling of connections to hostnames doesn't actually work the way I thought it did... will post an updated patch soon.

comment:5 Changed 4 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.5.x-final
Status: newneeds_review

Changed 4 years ago by cypherpunks

second attempt

comment:6 Changed 4 years ago by cypherpunks

I uploaded a second patch for review. I think this version is good, but I still don't understand the purpose or effect of the !conn->use_begindir && !conn->chosen_exit_name && !circ expression I copy+pasted from consider_plaintext_ports.

(Btw, the problem with the first patch was that I wasn't checking the result of tor_addr_parse, so when a hostname was parsed an uninitialized addr was being considered. The confusing thing was that, on my system, the uninitialized addr actually did match the reject *:* policy, which caused me to believe the patch worked. But when someone else tested it, on their system tor would log a warning tor_addr_is_null(): Bug: Called with unknown address family and would then fail open because the uninitialized addr didn't match any policy.)

comment:7 Changed 4 years ago by nickm

Hm. After looking at this, I don't think I understand why you're doing this with full addresses, and not just ports.

In other words, if the user allows "1.2.3.4/80", and then Tor receives a SOCKS connection for "www.example.com:80", should the code allow the request to be made or not? Keep in mind that a BEGIN cell does a lookup and a connect in one step: Tor won't know whether www.example.com resolved to 1.2.3.4 until the connection is made. With this patch, I think the answer will depend on whether the user said to allow 0.0.0.0, which can't really be the right behavior.

Given that address-based rules don't work the way that users might expect here, are we losing anything important by having this be address-and-port based rather than port-based alone?

comment:8 in reply to:  7 Changed 4 years ago by cypherpunks

Replying to nickm:

Hm. After looking at this, I don't think I understand why you're doing this with full addresses, and not just ports.

I mostly want this feature for port-based policies, but I included IP address support too because the policy parser does it already (so it was easy) and also because I think address-based policies might actually be useful in some cases. For instance, say I've got an system that I expect should only connect to 1.2.3.4:22 without using a hostname, I could have a ClientConnectPolicy of "accept 1.2.3.4:22, reject *:*". Attempts to connect to hostnames that resolve to 1.2.3.4 will be rejected, but connections to the IP will be allowed.

I'd rather have hostname support too, but this would be complicated because (a) the policy parser would need to support hostnames, and (b) applying IP-based policies to hostname-based connections isn't really possible due to what you say below. So, I figured that documenting Connections using hostnames will only be matched by policies with "*" as their IP address was a reasonable middle ground. Perhaps that could be said in a clearer way?

In other words, if the user allows "1.2.3.4/80", and then Tor receives a SOCKS connection for "www.example.com:80", should the code allow the request to be made or not? Keep in mind that a BEGIN cell does a lookup and a connect in one step: Tor won't know whether www.example.com resolved to 1.2.3.4 until the connection is made. With this patch, I think the answer will depend on whether the user said to allow 0.0.0.0, which can't really be the right behavior.

Correct, but I did think that would be OK behavior if sufficiently documented (which perhaps it isn't).

Given that address-based rules don't work the way that users might expect here, are we losing anything important by having this be address-and-port based rather than port-based alone?

I assume you mean the opposite, would we losing anything by having this be port based alone? We'd just be losing the ability to have IP-based policies which don't apply to hostname-based connections. That functionality was not my primary objective, but I do think it could be useful in some cases.

Let me know if you think the docs should/could be clarified about this, or if you want me to make a solely port-based version of the patch.

comment:9 Changed 4 years ago by nickm

Sorry, I missed that in the documentation. That's what I get for focusing on the code.

If we're okay with saying that connection-by-hostname is only matched by *, that's plausible. But your code doesn't do that, I think. In the patch you wrote, if I read it right, connection-by-hostname is matched by *, and by any other pattern that matches 0.0.0.0, which seems like a weird interface. Can we make it so that only * matches?

comment:10 Changed 4 years ago by andrea

Keywords: 025-triaged added

comment:11 Changed 4 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final
Status: needs_reviewneeds_revision

comment:12 Changed 4 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???

We can take these when they're revised, I think, but there's no need to block a release on revising them.

comment:13 Changed 3 years ago by nickm

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

These might also be worth looking at in 0.2.7

comment:14 Changed 3 years ago by nickm

Keywords: 027-triaged-1-out added

Marking triaged-out items from first round of 0.2.7 triage.

comment:15 Changed 3 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.???

Move *most* 0.2.7-triaged-1-out needs_revision items into 0.2.???. Keep a few based on my sense of the sensible.

comment:16 Changed 18 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:17 Changed 17 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:18 Changed 12 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:19 Changed 12 months ago by nickm

Keywords: 025-triaged removed

remove an old triage keyword.

comment:20 Changed 12 months ago by nickm

Keywords: 027-triaged-in added

comment:21 Changed 12 months ago by nickm

Keywords: 027-triaged-in removed

comment:22 Changed 12 months ago by nickm

Keywords: 027-triaged-1-out removed

comment:23 Changed 12 months ago by nickm

Keywords: interface plaintextports added
Severity: Normal
Note: See TracTickets for help on using tickets.