Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#12378 closed defect (not a bug)

Tor configuration policies using network CIDR syntax should clamp mask bits appropriately

Reported by: anon Owned by:
Priority: Medium Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: config exit-policy
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Tor configuration policies using network CIDR syntax like 224.0.0.0/8 should clamp mask bits appropriately to IANA and network prefix.

An example bad configuration spotted in the wild:

224.0.0.0/3 which represents a binary

11100000.00000000.00000000.00000000 &
00011111.11111111.11111111.11111111
in

tor_addr_compare_masked

which results in a comparison of only the first three bits of any comparison network under test.

Improve Tor to implement a clamp mask, and warn on a configuration policy that specifies an invalid mask per network prefix.

The netmask clamp would ensure that mask bits number at least 8 bits or more, meaning a /8 or smaller network policy. See https://www.iana.org/assignments/ipv4-address-space/ipv4-address-space.xhtml

The netmask clamp would ensure that mask bits number at least the same number of bits in the network prefix, if the network prefix bits number 8 or more themselves.

Child Tickets

Change History (6)

comment:1 Changed 5 years ago by anon

Suggested tests to confirm working clamps:

224.0.0.0/3 clamped to 224.0.0.0/8
10.254.0.0/8 clamped to 10.254.0.0/16
10.11.12.13/32 NOT clamped (left at /32)
10.0.0.0/24 NOT clamped (leading zeroes not indicative of useful network prefix length)

Test that invalid CIDR netmasks result in warning but are accepted.

IPv6 CIDR tests? or perhaps 4in6 CIDR verifications? These may be useful.

comment:2 Changed 5 years ago by arma

coderman points to https://doxygen.torproject.org/address_8c_source.html#l00962 which is where our behavior diverges from what others might expect.

I noticed the behavior first in the tordienet relay:
https://atlas.torproject.org/#details/CB2BEB364E07CF431819F6C5349555425C60C6F3
which has a quite complex exit policy, but the exit policy summary is simply reject *:*, which doesn't seem like it is correct. Is that a different bug, or this bug?

comment:3 Changed 5 years ago by nickm

I think that warning in these cases makes more sense than clamping silently. If somebody says "a.b.0.0/8", I don't feel comfortable concluding that they obviously meant "a.b.0.0/16" instead of "a.0.0.0/8".

but the exit policy summary is simply reject *:*, which doesn't seem like it is correct. Is that a different bug, or this bug?

Could be "no bug". The rule for policy summaries is basically that if you reject a bunch of non-private_nets addresses on a port, you aren't summarized as supporting that port.

comment:4 Changed 5 years ago by anon

Resolution: not a bug
Status: newclosed

let me add two things, which are not clear. 1) initially it was thought that this could cause harm, if the math on the mask was done wrong. i don't see any harm, but i also didn't update ticket to make clear to roger that exit policies were not getting bypassed and two, the expamples are poor, and based on weasel's feedback, without harm, perhaps this should die.

<nickm> "After all, having "{addr}/{N}" mean "any address that matches the first 'N' bits of addr" has a certain lovely simplicity."

comment:5 Changed 5 years ago by anon

Two more pieces of context:
<coderman> i do see your point though. 224.0.0.0/3 could be seen as "all the rest to the top" mask.
<coderman> i am not saying 10.224.0.0/11 is invalid,
and "who gives a shit about IANA???" (paraphrasing :)

comment:6 Changed 5 years ago by tordienet

I maintain tordienet. FWIW, 224.0.0.0/3 is what I intended to express. If you only allow me to write rules that cover /8, I would have to add 32 rules to express the same thing, which seems silly.

You could safely interpret 10.254.0.0/8 as 10.0.0.0/8, since they mean the same thing. But you should never change the bitmask: 10.254.0.0/8 -> 10.254.0.0/16 is really not the same thing.

Note: See TracTickets for help on using tickets.