Opened 16 months ago

Last modified 6 months ago

#31011 needs_revision defect

Make the bridge authority reject private PT addresses when DirAllowPrivateAddresses is 0

Reported by: teor Owned by: cjb
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-pt
Cc: ahf, gaba, phw, cohosh, catalyst Actual Points:
Parent ID: #31009 Points: 1
Reviewer: ahf Sponsor: Sponsor28-can

Description

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 is backported,

Gaba, this ticket should go in the PT sponsor with #31009.

Child Tickets

Change History (28)

comment:1 Changed 16 months ago by 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 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.)

comment:2 Changed 16 months ago by teor

Sponsor: Sponsor28-can

Gaba set #31009 to Sponsor 28 can, making this related ticket match.

comment:3 Changed 15 months ago by gaba

Keywords: anti-censorship-roadmap-september added
Points: 0.51

comment:4 Changed 15 months ago by neel

Cc: neel added
Owner: set to neel
Status: newassigned

comment:5 Changed 14 months ago by neel

Cc: neel removed
Owner: neel deleted

comment:6 Changed 14 months ago by neel

Status: assignednew

comment:7 in reply to:  1 ; Changed 10 months ago by 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 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:

1) as in the summary, bridgeauth just refuses descriptors with internal addresses

2) arma's suggestion, bridgedb transforms internal addresses to external

3) 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?

comment:8 in reply to:  7 ; Changed 10 months ago by teor

Cc: phw added

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

1) 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.)

2) arma's suggestion, bridgedb transforms internal addresses to external

This option has the opposite tradeoff: more bridges, fast to deploy, potentially wrong data.

3) 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.

comment:9 in reply to:  8 ; Changed 10 months ago by phw

Cc: cohosh added

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

comment:10 in reply to:  9 ; Changed 10 months ago by teor

Replying 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 transport line.

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 transport line 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#L1615

Here'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

comment:11 Changed 10 months ago by cjb

Owner: set to cjb
Status: newassigned

comment:12 Changed 9 months ago by phw

Keywords: anti-censorship-roadmap-september removed

comment:13 in reply to:  10 Changed 9 months ago by cjb

Replying to teor:

We can add bridge authority code that rejects extra-info descriptors with a private address in any transport line.

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.1

How 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.

comment:14 Changed 9 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.4.x-final

comment:15 Changed 9 months ago by cjb

Status: assignedneeds_review

comment:16 Changed 9 months ago by 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 valid ServerTransportListenAddr that fails tor_addr_port_parse and didn't think we should reject the config in that case. Does that sound reasonable?

comment:17 in reply to:  16 ; Changed 9 months ago by teor

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 valid ServerTransportListenAddr that fails tor_addr_port_parse and 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. (ServerTransportListenAddr is 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.)

comment:18 Changed 9 months ago by dgoulet

Reviewer: ahf

comment:19 in reply to:  17 Changed 9 months ago by cjb

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

comment:20 Changed 8 months ago by 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.

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.

comment:21 in reply to:  20 Changed 8 months ago by cjb

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.

comment:22 Changed 8 months ago by teor

Thanks for this PR,

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,

There's also a few places that the logs could be improved.

comment:23 Changed 8 months ago by teor

Keywords: tor-pt added
Status: needs_reviewneeds_revision

comment:24 in reply to:  22 Changed 8 months ago by cjb

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 like router_parse_list_from_string(), signed_descriptor_get_body(), munge_extrainfo_into_routerinfo() (deprecated).

comment:25 Changed 8 months ago by teor

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:

  1. Add the server transport line to the token table at: https://github.com/torproject/tor/blob/4f02812242d1fd90d859eb98ac3fb1ed182f18cf/src/feature/dirparse/routerparse.c#L127
  2. 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
  3. Parse the server transport line when tor parses the extrainfo: https://github.com/torproject/tor/blob/4f02812242d1fd90d859eb98ac3fb1ed182f18cf/src/feature/dirparse/routerparse.c#L960
  4. Free the new string when an extrainfo struct is freed.

comment:26 Changed 8 months ago by cjb

Thank you! I think the line is just named transport in the extra-info doc, and there can be 0-to-many of them. So it sounds like a new transports field in the extrainfo_t struct should be a smartlist of transport line char *s, do you agree? And then as the bridgeauth we reject the entire descriptor during parsing if a single transport line 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?)

comment:27 in reply to:  26 Changed 8 months ago by teor

Replying to cjb:

Thank you! I think the line is just named transport in the extra-info doc, and there can be 0-to-many of them. So it sounds like a new transports field in the extrainfo_t struct should be a smartlist of transport line char *s, do you agree? And then as the bridgeauth we reject the entire descriptor during parsing if a single transport line 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.

comment:28 Changed 6 months ago by catalyst

Cc: catalyst added
Note: See TracTickets for help on using tickets.