Make the bridge authority reject private PT addresses when DirAllowPrivateAddresses is 0
When DirAllowPrivateAddresses is 0, the bridge authority should reject extra info descriptors with private addresses in their PT lines.
We should add some text asking operators to upgrade, and deploy this change after #31009 (moved) is backported,
Gaba, this ticket should go in the PT sponsor with #31009 (moved).
Activity
changed milestone to %Tor: 0.4.4.x-final
Trac:
Parent Ticket: #31009 (moved)
Another option here is to leave the bridge authority alone, and teach bridgedb that if there's an internal address in the extrainfo descriptor, it should swap it out in favor of the public address in the descriptor.
Then once the #31009 (moved) fix is sufficiently deployed, it shouldn't matter anymore.
(That way we could make use of the current obfs4 bridges even if they haven't upgraded yet.)
Gaba set #31009 (moved) to Sponsor 28 can, making this related ticket match.
Trac:
Sponsor: N/A to Sponsor28-canReplying to arma:
Another option here is to leave the bridge authority alone, and teach bridgedb that if there's an internal address in the extrainfo descriptor, it should swap it out in favor of the public address in the descriptor.
Then once the #31009 (moved) fix is sufficiently deployed, it shouldn't matter anymore.
(That way we could make use of the current obfs4 bridges even if they haven't upgraded yet.)
I think I could volunteer to work on this ticket, but it looks like we still need to decide what to do. Options:
-
as in the summary, bridgeauth just refuses descriptors with internal addresses
-
arma's suggestion, bridgedb transforms internal addresses to external
-
Could we also consider having bridgeauth itself, rather than bridgedb downstream, perform that transformation? Or perhaps there's a reason why that's not a good idea?
-
I think we need to know how many bridges are affected by this issue, before we can make this decision.
Replying to cjb:
Replying to arma:
Another option here is to leave the bridge authority alone, and teach bridgedb that if there's an internal address in the extrainfo descriptor, it should swap it out in favor of the public address in the descriptor.
Then once the #31009 (moved) fix is sufficiently deployed, it shouldn't matter anymore.
(That way we could make use of the current obfs4 bridges even if they haven't upgraded yet.)
I think I could volunteer to work on this ticket, but it looks like we still need to decide what to do. Options:
There's a tradeoff here, so maybe we should ask the anti-censorship team what they'd like.
- as in the summary, bridgeauth just refuses descriptors with internal addresses
Rejecting descriptors means we have fewer bridges, until those bridges upgrade tor versions. But those bridges are more likely to have correct addresses. (Changing to an address the operator didn't provide means that port forwarding might not be set up.)
- arma's suggestion, bridgedb transforms internal addresses to external
This option has the opposite tradeoff: more bridges, fast to deploy, potentially wrong data.
- Could we also consider having bridgeauth itself, rather than bridgedb downstream, perform that transformation? Or perhaps there's a reason why that's not a good idea?
I'm not sure if we can do this, because extra-info descriptors are signed by the bridge.
Trac:
Cc: ahf, gaba to ahf, gaba, phw
Replying to teor:
I think we need to know how many bridges are affected by this issue, before we can make this decision.
As of Dec 17, 2019, 1,382 bridges support a pluggable transport. Among these, 10 bridges use an address in 10.0.0.0/8, 10 bridges use 192.168.0.0/16, and 2 bridges use 172.16.0.0/12, so a total of 24 bridges use private addresses.
Replying to cjb:
Replying to arma:
Another option here is to leave the bridge authority alone, and teach bridgedb that if there's an internal address in the extrainfo descriptor, it should swap it out in favor of the public address in the descriptor.
Then once the #31009 (moved) fix is sufficiently deployed, it shouldn't matter anymore.
(That way we could make use of the current obfs4 bridges even if they haven't upgraded yet.)
I think I could volunteer to work on this ticket, but it looks like we still need to decide what to do. Options:
There's a tradeoff here, so maybe we should ask the anti-censorship team what they'd like.
I prefer having the bridge authority reject descriptors with private addresses. In my opinion, a private address has no business being in the descriptor and we should reject such descriptors rather than guessing what the bridge operators meant to do.
In parallel, we could teach BridgeDB to rewrite private to public IP addresses but given that only 24 bridges are affected by this issue, I don't consider this a priority.
Trac:
Cc: ahf, gaba, phw to ahf, gaba, phw, cohoshReplying to phw:
I prefer having the bridge authority reject descriptors with private addresses. In my opinion, a private address has no business being in the descriptor and we should reject such descriptors rather than guessing what the bridge operators meant to do.
Thanks, that seems like a sensible decision.
We can add bridge authority code that rejects extra-info descriptors with a private address in any
transportline.We should probably also add a config error on the bridge side, if ServerTransportListenAddress is an internal address, compute_publishserverdescriptor() is bridge, and the bridge is using the default bridge authority.
Here's how the
transportline is created on the bridge side: https://github.com/torproject/tor/blob/f6c9ca3a1d1c29a293915612e26cdbfeb050c192/src/feature/relay/router.c#L3190 https://github.com/torproject/tor/blob/60d5ff303d65bb7caf5c064675c661faac4cecf1/src/feature/client/transports.c#L1615Here's where we reject extra-info descriptors in dirserv_add_extrainfo(): https://github.com/torproject/tor/blob/53bdd21179b3507b8d8aa2788e4955df8619f6db/src/feature/dirauth/process_descs.c#L789
See dirserv_router_has_valid_address() for some example code. This code rejects relay descriptors with private IPv4 or IPv6 addresses, when DirAllowPrivateAddresses is 0: https://github.com/torproject/tor/blob/53bdd21179b3507b8d8aa2788e4955df8619f6db/src/feature/dirauth/process_descs.c#L456
Replying to teor:
We can add bridge authority code that rejects extra-info descriptors with a private address in any
transportline.We should probably also add a config error on the bridge side, if ServerTransportListenAddress is an internal address, compute_publishserverdescriptor() is bridge, and the bridge is using the default bridge authority.
Thanks for the hints! Here's a patch that attempts to do these things:
From 0725e4a2848c8a6e41640d4af784e36417b37aa5 Mon Sep 17 00:00:00 2001 From: Chris Ball <chris@printf.net> Date: Tue, 28 Jan 2020 17:07:06 -0800 Subject: [PATCH] Refuse to publish a bridge with internal addr to bridgeauth If a PT: * has ServerTransportListenAddr set to an internal address * and it's a bridge * and it's publishing its descriptor to the default bridge authorities then fail the config as invalid, since we don't want internal IP info being sent to bridgeauths for distribution. And for clients that aren't running with this fix: if we're a bridgeauth, reject extra-info descs containing those internal addresses. --- src/feature/dirauth/process_descs.c | 9 +++++++++ src/feature/relay/transport_config.c | 12 +++++++++++- src/test/conf_examples/pt_10/error | 1 + src/test/conf_examples/pt_10/torrc | 9 +++++++++ 4 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 src/test/conf_examples/pt_10/error create mode 100644 src/test/conf_examples/pt_10/torrc diff --git a/src/feature/dirauth/process_descs.c b/src/feature/dirauth/process_descs.c index baf8f8c21..1c2c48ea3 100644 --- a/src/feature/dirauth/process_descs.c +++ b/src/feature/dirauth/process_descs.c @@ -895,6 +895,15 @@ dirserv_add_extrainfo(extrainfo_t *ei, const char **msg) goto fail; } + /* If the extrainfo descriptor contains an internal address, reject it. */ + if (dirserv_router_has_valid_address(ri) < 0) { + log_notice(LD_DIR, "Rejecting an extrainfo descriptor '%s' " + "with an internal address.", ri->nickname); + *msg = "Router extrainfo descriptor contained an internal address."; + rv = ROUTER_AUTHDIR_REJECTS; + goto fail; + } + if ((r = routerinfo_incompatible_with_extrainfo(ri->identity_pkey, ei, &ri->cache_info, msg))) { if (r<0) { diff --git a/src/feature/relay/transport_config.c b/src/feature/relay/transport_config.c index 7dcce70e3..0bd844bd9 100644 --- a/src/feature/relay/transport_config.c +++ b/src/feature/relay/transport_config.c @@ -16,7 +16,7 @@ #include "lib/encoding/confline.h" #include "lib/encoding/keyval.h" - +#include "lib/net/address.h" #include "lib/container/smartlist.h" /* Required for dirinfo_type_t in or_options_t */ @@ -230,6 +230,16 @@ options_validate_server_transport(const or_options_t *old_options, char *bindaddr = get_bindaddr_from_transport_listen_line(cl->value, NULL); if (!bindaddr) REJECT("ServerTransportListenAddr did not parse. See logs for details."); + + tor_addr_t tor_addr; + uint16_t tor_port; + if (tor_addr_port_parse(LOG_WARN, bindaddr, &tor_addr, &tor_port, 0) > -1 && + tor_addr_is_internal(&tor_addr, 0) && + options->PublishServerDescriptor_ == BRIDGE_DIRINFO && + !options->AlternateBridgeAuthority) + REJECT("ServerTransportListenAddr is an internal address, " + "refusing to publish this bridge to the default bridge authorities."); + tor_free(bindaddr); } diff --git a/src/test/conf_examples/pt_10/error b/src/test/conf_examples/pt_10/error new file mode 100644 index 000000000..17a200942 --- /dev/null +++ b/src/test/conf_examples/pt_10/error @@ -0,0 +1 @@ +ServerTransportListenAddr is an internal address, refusing to publish this bridge to the default bridge authorities. diff --git a/src/test/conf_examples/pt_10/torrc b/src/test/conf_examples/pt_10/torrc new file mode 100644 index 000000000..c08f34091 --- /dev/null +++ b/src/test/conf_examples/pt_10/torrc @@ -0,0 +1,9 @@ +# Relay PT tests +# Options from relay/transport_config.c +# Try a valid minimal config, with a bad ServerTransportListenAddr +ORPort 2 +ExtORPort 1 +BridgeRelay 1 +ServerTransportPlugin bad3 exec / +ServerTransportListenAddr bad3 10.1.2.3:4567 +PublishServerDescriptor 1 -- 2.20.1How does it look? I'm not sure how to get setup to test the bridgeauth side of the change. It looks like this is my first Tor patch since 2010 so please give extra scrutiny. :) I'm still catching up with codebase and process.
Oh, something to mention:
if (tor_addr_port_parse(LOG_WARN, bindaddr, &tor_addr, &tor_port, 0) > -1 &&This patch will only reject an internal address if it successfully passes
tor_addr_port_parse. I was worried that there might be a validServerTransportListenAddrthat failstor_addr_port_parseand didn't think we should reject the config in that case. Does that sound reasonable?Replying to cjb:
Oh, something to mention:
if (tor_addr_port_parse(LOG_WARN, bindaddr, &tor_addr, &tor_port, 0) > -1 &&This patch will only reject an internal address if it successfully passes
tor_addr_port_parse. I was worried that there might be a validServerTransportListenAddrthat failstor_addr_port_parseand didn't think we should reject the config in that case. Does that sound reasonable?We shouldn't restrict future pluggable transport addresses too much. (
ServerTransportListenAddris parsed by the PT, so it's possible that tor won't understand it.)In this case, I think we should log a warning or notice-level message. Because it is still likely to be a misconfiguration. (Im pretty sure tor understands all current PT addresses.)
Replying to teor:
In this case, I think we should log a warning or notice-level message. Because it is still likely to be a misconfiguration. (Im pretty sure tor understands all current PT addresses.)
Thanks, here's a v2 of the patch that adds this notice.
From be21a19923793836918386fa8672932befb9e645 Mon Sep 17 00:00:00 2001 From: Chris Ball <chris@printf.net> Date: Tue, 28 Jan 2020 17:07:06 -0800 Subject: [PATCH] Refuse to publish a bridge with internal addr to bridgeauth If a PT: * has ServerTransportListenAddr set to an internal address * and it's a bridge * and it's publishing its descriptor to the default bridge authorities then fail the config as invalid, since we don't want internal IP info being sent to bridgeauths for distribution. And for clients that aren't running with this fix: if we're a bridgeauth, reject extra-info descs containing those internal addresses. --- src/feature/dirauth/process_descs.c | 9 +++++++++ src/feature/relay/transport_config.c | 17 ++++++++++++++++- src/test/conf_examples/pt_10/error | 1 + src/test/conf_examples/pt_10/torrc | 9 +++++++++ 4 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 src/test/conf_examples/pt_10/error create mode 100644 src/test/conf_examples/pt_10/torrc diff --git a/src/feature/dirauth/process_descs.c b/src/feature/dirauth/process_descs.c index baf8f8c21..1c2c48ea3 100644 --- a/src/feature/dirauth/process_descs.c +++ b/src/feature/dirauth/process_descs.c @@ -895,6 +895,15 @@ dirserv_add_extrainfo(extrainfo_t *ei, const char **msg) goto fail; } + /* If the extrainfo descriptor contains an internal address, reject it. */ + if (dirserv_router_has_valid_address(ri) < 0) { + log_notice(LD_DIR, "Rejecting an extrainfo descriptor '%s' " + "with an internal address.", ri->nickname); + *msg = "Router extrainfo descriptor contained an internal address."; + rv = ROUTER_AUTHDIR_REJECTS; + goto fail; + } + if ((r = routerinfo_incompatible_with_extrainfo(ri->identity_pkey, ei, &ri->cache_info, msg))) { if (r<0) { diff --git a/src/feature/relay/transport_config.c b/src/feature/relay/transport_config.c index 7dcce70e3..7cf023bec 100644 --- a/src/feature/relay/transport_config.c +++ b/src/feature/relay/transport_config.c @@ -16,7 +16,7 @@ #include "lib/encoding/confline.h" #include "lib/encoding/keyval.h" - +#include "lib/net/address.h" #include "lib/container/smartlist.h" /* Required for dirinfo_type_t in or_options_t */ @@ -230,6 +230,21 @@ options_validate_server_transport(const or_options_t *old_options, char *bindaddr = get_bindaddr_from_transport_listen_line(cl->value, NULL); if (!bindaddr) REJECT("ServerTransportListenAddr did not parse. See logs for details."); + + tor_addr_t tor_addr; + uint16_t tor_port; + int parse_rv = tor_addr_port_parse(LOG_WARN, bindaddr, &tor_addr, &tor_port, 0); + if (parse_rv < 0) { + log_notice(LD_CONFIG, "Your ServerTransportListenAddr failed to parse " + "as a Tor address; check your configuration."); + } + if (parse_rv == 0 && + tor_addr_is_internal(&tor_addr, 0) && + options->PublishServerDescriptor_ == BRIDGE_DIRINFO && + !options->AlternateBridgeAuthority) + REJECT("ServerTransportListenAddr is an internal address, " + "refusing to publish this bridge to the default bridge authorities."); + tor_free(bindaddr); } diff --git a/src/test/conf_examples/pt_10/error b/src/test/conf_examples/pt_10/error new file mode 100644 index 000000000..17a200942 --- /dev/null +++ b/src/test/conf_examples/pt_10/error @@ -0,0 +1 @@ +ServerTransportListenAddr is an internal address, refusing to publish this bridge to the default bridge authorities. diff --git a/src/test/conf_examples/pt_10/torrc b/src/test/conf_examples/pt_10/torrc new file mode 100644 index 000000000..c08f34091 --- /dev/null +++ b/src/test/conf_examples/pt_10/torrc @@ -0,0 +1,9 @@ +# Relay PT tests +# Options from relay/transport_config.c +# Try a valid minimal config, with a bad ServerTransportListenAddr +ORPort 2 +ExtORPort 1 +BridgeRelay 1 +ServerTransportPlugin bad3 exec / +ServerTransportListenAddr bad3 10.1.2.3:4567 +PublishServerDescriptor 1 -- 2.24.0
cjb: Hey, thanks for the patch. Normally for more complicated stuff, we prefer if people create pull requests on Github rather than upload patches directly on Trac.
This allows our CI to run before we start diving too much into the code. Is this something you are up for doing or do you want me to try it out first? :-)
Dropping a link to a PR on a ticket is perfectly fine.
Replying to ahf:
cjb: Hey, thanks for the patch. Normally for more complicated stuff, we prefer if people create pull requests on Github rather than upload patches directly on Trac.
Thanks, created https://github.com/torproject/tor/pull/1742.
Replying to teor:
I did a quick review, and saw that the bridge authority is checking the address in the routerinfo, not the address in the ServerTransport line in the extrainfo,
Oops, thanks! Please could you point me towards a preferred way to access extrainfo lines from an
ei? I couldn't find any similar examples in the codebase. So far I've found things likerouter_parse_list_from_string(),signed_descriptor_get_body(),munge_extrainfo_into_routerinfo()(deprecated).Good question!
It looks like extrainfo documents aren't really parsed by tor right now. Instead, they're parsed by BridgeDB and metrics.
Here's how you can change that:
- Add the server transport line to the token table at: https://github.com/torproject/tor/blob/4f02812242d1fd90d859eb98ac3fb1ed182f18cf/src/feature/dirparse/routerparse.c#L127
- Add a server transport line variable to the extrainfo struct at: https://github.com/torproject/tor/blob/4f02812242d1fd90d859eb98ac3fb1ed182f18cf/src/feature/nodelist/extrainfo_st.h#L18
- Parse the server transport line when tor parses the extrainfo: https://github.com/torproject/tor/blob/4f02812242d1fd90d859eb98ac3fb1ed182f18cf/src/feature/dirparse/routerparse.c#L960
- Free the new string when an extrainfo struct is freed.
Thank you! I think the line is just named
transportin the extra-info doc, and there can be 0-to-many of them. So it sounds like a newtransportsfield in theextrainfo_tstruct should be a smartlist oftransportlinechar *s, do you agree? And then as the bridgeauth we reject the entire descriptor during parsing if a singletransportline contains an internal address?(If this takes me a while, perhaps it might make sense to split out the bridge-side change from the bridgeauth-side change and merge them independently?)
Replying to cjb:
Thank you! I think the line is just named
transportin the extra-info doc, and there can be 0-to-many of them. So it sounds like a newtransportsfield in theextrainfo_tstruct should be a smartlist oftransportlinechar *s, do you agree? And then as the bridgeauth we reject the entire descriptor during parsing if a singletransportline contains an internal address?Yes, I think that's our standard design for lists of strings.
(If this takes me a while, perhaps it might make sense to split out the bridge-side change from the bridgeauth-side change and merge them independently?)
Splitting changes is usually helpful. Particularly when they need to be merged to separate repositories.
mentioned in issue #31009 (moved)
moved to tpo/core/tor#31011 (closed)
