Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#13202 closed defect (wontfix)

Figure out a way to deal with bridges missing PT arguments.

Reported by: yawning Owned by:
Priority: Medium Milestone:
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: bridgedb-dist, scramblesuit, isis2014Q3Q4, isis2015Q1Q2, bridgedb-0.3.2
Cc: isis, phw Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

From IRC, apparently approximately one in seven ScrambleSuit bridges do not have a password on the bridge line.

I am 99% certain this is due to people running ScrambleSuit on tor-0.2.4.x, as passing pluggable transport arguments via the extrainfo document is not supported in that version, and transports.c:parse_smethod_line() does not sufficiently validate the SMETHOD line, so the arguments are silently dropped.

This leads to Bridges that publish extrainfo documents containing Bridge lines that are totally useless (No ScrambleSuit password etc), and will never get used.

This may be something that has to be worked around on the BridgeDB side (teach it about transports that require arguments, ignore bridges with malformed bridge lines), but that seems suboptimal (people thinking they are contributing bridges, when they realistically are not).

Child Tickets

Change History (25)

comment:1 Changed 5 years ago by isis

Cc: isis added
Keywords: bridgedb-dist scramblesuit added

Perhaps obfsproxy/scramblesuit should check the Tor version when being called, and if there were an easy way for PTs to specify a set of Tor version which are compatible, then it could alert the bridge operator if the version is incompatible.

In the meantime, BridgeDB could have some temporary logic to not use scramblesuit transports which do not have passwords, since these are already deployed and there's not much we could do to fix them.

comment:2 in reply to:  1 ; Changed 5 years ago by yawning

Replying to isis:

Perhaps obfsproxy/scramblesuit should check the Tor version when being called, and if there were an easy way for PTs to specify a set of Tor version which are compatible, then it could alert the bridge operator if the version is incompatible.

In the hindsight is 20/20 department, we don't have anything like TOR_VERSION in the pt environment space, although that would be a great thing to add. I'm not fundamentally opposed to this, but this approach still leaves the problem of people running old tor/obfsproxy/obfs4proxy still publishing busted extrainfo documents.

Beyond the little-t tor changes required here (which would be quite trivial), this approach would also require changes to our pt code (and breaking working configs because they happened to upgrade the pt and not tor may be kind of rude).

In the meantime, BridgeDB could have some temporary logic to not use scramblesuit transports which do not have passwords, since these are already deployed and there's not much we could do to fix them.

That would be excellent (obfs4 will more than likely have the same problem).

For the record (yes I know it's unlikely to happen, just documenting it), a hypothetical maint-0.2.4 patch would be along the lines of adding:

  if (smartlist_len(items) > 3) {
    log_warn(LD_CONFIG, "Server managed proxy sent us a SMETHOD line "
             "with too many arguments.");
    goto err;
  }

This approach still would require filtering changes to BridgeDB for people that do not upgrade, but requires no pt side changes and won't break current 0.2.5.x configs that upgrade the pt but not tor (like changing ScrambleSuit/obfs4 to require a TOR_VERSION being set would).

comment:3 Changed 5 years ago by phw

Cc: phw added

comment:4 in reply to:  2 ; Changed 5 years ago by dcf

Replying to yawning:

we don't have anything like TOR_VERSION in the pt environment space,

Aside, if we want other projects to use pluggable transports, we shouldn't add a TOR_VERSION variable. Otherwise those other projects will need to lie and say they're a specific version of Tor, need to be aware of Tor's changelog, and need to match features with a particular Tor version. It's better to signal support for specific features than have them implied by a version number.

comment:5 in reply to:  4 Changed 5 years ago by yawning

Replying to dcf:

Replying to yawning:

we don't have anything like TOR_VERSION in the pt environment space,

Aside, if we want other projects to use pluggable transports, we shouldn't add a TOR_VERSION variable. Otherwise those other projects will need to lie and say they're a specific version of Tor, need to be aware of Tor's changelog, and need to match features with a particular Tor version. It's better to signal support for specific features than have them implied by a version number.

This is probably true. AFAIK we currently do not have 3rd parties using the pt config protocol, but it would be good to keep this in mind. Offtopic, I'm kicking around the idea of doing a pt protocol version 2 with feedback from the rest of the obfuscation people (quite a few of the warts in the existing pt protocol could be fixed in a cleaner manner if we did something like this, otherwise we would need a lot of feature signals).

So to sum up the current situation:

  • At a minimum, regardless of how we address this on the tor side, BridgeDB will need to filter out people running bridges that publish busted extrainfo descriptors (ScrambleSuit/obfs4 on tor 0.2.4.x), till affected tor versions are gone.
  • I'm not overly hopeful about contact info for the broken bridges being filled in (probably missing or invalid for the majority), but we should at least send a post to tor-relays@ highlighting this issue.
  • The ship on fixing this the right way (patching maint-0.2.4.x, yes, it is a bug) has sailed, so if we are going to patch tor, we need other options.
  • The current leading candidate for a tor side patch would be to add something like TOR_PT_EXTENSIONS=smethod-args (more to be added to the list as time goes on) to 0.2.5.x/0.2.6.x, and change obfsproxy/obfs4proxy to SMETHOD-ERROR if the env var is not present. This will fix new deployments, but will not address people that do not upgrade tor or obfsproxy/obfs4proxy. This will additionally break working configurations that upgrade to new obfsproxy/obfs4proxy without upgrading tor.

Does this seem accurate to people? I'm not particularly thrilled at the idea of patching 0.2.5.x/0.2.6.x for this because of the gotchas, but if other people think strongly that it would be a good idea, then I can start working on the relevant patches.

comment:6 Changed 5 years ago by asn

I'm also not very enthusiastic about adding workarounds in Tor for this.
The good thing is that 0.2.5 will soon get released and eventually become the stable release.

Adding transport-specific workarounds in BridgeDB is not great either, but maybe we can get away with it because BridgeDB is in Python (fix will be 3 lines) and has a more fluid development cycle.

Another good thing is that all those scramblesuit bridges are probably providing obfs3 too, so they are not entirely wasted.

Finally, I'm not entirely against TOR_VERSION because it seems like potentially useful information and can help us hotfix such issues in the future (it's also easy to implement).
Maybe instead of TOR_VERSION, we name it in a more general manner (like APP_VERSION) or something, and let other projects fill in their own things.

comment:7 in reply to:  6 Changed 5 years ago by dcf

Replying to asn:

Finally, I'm not entirely against TOR_VERSION because it seems like potentially useful information and can help us hotfix such issues in the future (it's also easy to implement).
Maybe instead of TOR_VERSION, we name it in a more general manner (like APP_VERSION) or something, and let other projects fill in their own things.

I don't know, I still think you might come to regret such a design... Even if the variable is called APP_VERSION, you're going to add code to ScrambleSuit that says

app, version = parse_app_version(APP_VERSION)
if app == "tor" and version >= 0.2.5:
    do something
else:
    do something else

If any other project wants to use ScrambleSuit, and wants ScrambleSuit to take the "do something" branch, it will have no choice but to say "I am tor 0.2.5." Or else ScrambleSuit needs to have a catalog of applications and version numbers within it. It all starts to take some of the "pluggable" out of pluggable transports.

Think about the case of pluggable transports using a proxy. Not all versions of tor support telling the pluggable transport to use a proxy. But those that do, advertise their support by setting the TOR_PT_PROXY variable. Pluggable transports that support the feature show that they support it by writing a "PROXY DONE" line. The design instead could have had the pluggable transport sniffing the version number to check for proxy support, or by declaring a new managed proxy protocol version and simply requiring that all pluggable transports need to support proxies. But the way it actually works is much nicer. It's backwards compatible and does what you want in every combination (including failing with an error when tor wants a proxy but the PT doesn't support it). Could we find a design like that?

To put in in perspective, remember we once had a situation where there were a bunch of useless obfs3 bridges in BridgeDB because people installed obfsproxy but didn't open a port on their firewall. That's basically the same situation we're in now. With 0.2.5 only going to become more common, maybe this is a problem that will resolve itself.

comment:8 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-final

comment:9 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.7.x-final
Status: newneeds_review

comment:10 Changed 5 years ago by nickm

Status: needs_reviewnew

comment:11 Changed 5 years ago by isis

Keywords: Isis2014Q3Q4 added

comment:12 Changed 5 years ago by isis

Keywords: isis2014Q3Q4 added; Isis2014Q3Q4 removed

comment:13 Changed 5 years ago by isis

Keywords: isis2015Q1Q2 added

comment:14 Changed 5 years ago by nickm

Status: newassigned

comment:15 Changed 5 years ago by nickm

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

Not much progress since the 0.2.5 release, and it appears that 0.2.5 solved this. Calling don't-need-to-do-for-0.2.7 for now.... but Isis, if you think this is still worth doing, then my change here could be totally wrong.

comment:16 in reply to:  15 Changed 5 years ago by isis

Replying to nickm:

Not much progress since the 0.2.5 release, and it appears that 0.2.5 solved this. Calling don't-need-to-do-for-0.2.7 for now.... but Isis, if you think this is still worth doing, then my change here could be totally wrong.


I'm not thrilled to have PT-specific code in BridgeDB, because it'll be a bit of a rabbithole and maintenance nightmare, but I still see at least a few hundred scramblesuit bridges without the password PT arg. The other transport deployments are doing much better than scramblesuit, e.g. there are currently 0 broken obfs4 lines in BridgeDB.

comment:17 Changed 5 years ago by nickm

Okay. So, Tor 0.2.7 comes out in September-ish (we hope) and will start reaching TB alphas sometime around then. Would fixing this on such a timeframe be worthwhile, or might we hope that the scramblesuit situation will be better by then?

comment:18 in reply to:  17 Changed 5 years ago by yawning

Replying to nickm:

Okay. So, Tor 0.2.7 comes out in September-ish (we hope) and will start reaching TB alphas sometime around then. Would fixing this on such a timeframe be worthwhile, or might we hope that the scramblesuit situation will be better by then?

I don't really see the situation changing from the last time I commented on this. The problem is people still running bridges with tor 0.2.4.x and pts that require 0.2.5.x, and the only sensible tor side fix for this is still "patch maint-0.2.4 to break all the broken bridges".

Exposing the tor version in the 0.2.7.x timeframe does nothing to fix this (and complicates all of the pt code). It is something I will seriously consider if/when a pt protocol 2.0 happens.

At this point I favor either "Kludge something in bridgedb till 0.2.4.x dies" or "do nothing, since this will only get better over time".

comment:19 Changed 5 years ago by isis

I just grepped for the contact lines for the broken bridges, and not a single broken PT bridge has contact info. (I was going to send emails to the operators before trashing their bridges.)

As a slight tangent, I think some of these bridges might have something extra wonky going on; I saw bridge extrainfo descriptors like:

[…]
transport scramblesuit 1.1.1.1:1111
[…]
bridge-ip-transports scramblesuit=8

which shouldn't be possible… unless these passwordless scramblesuit bridges are being used as private bridges and the operators wanted to send us statistics? Only some minority of the passwordless scramblesuit bridges had wonky bridge-ip-transports reporting scramblesuit usage (~1:20 broken ones). At least one broken scramblesuit bridge had bridge-ip-transports scramblesuit=16.

It's nice of them to send us statistics, I guess?

comment:20 Changed 4 years ago by isis

Keywords: bridgedb-0.3.2 added
Status: assignedneeds_review

Until Tor-0.2.4.x is deprecated and no longer in use, I've placed a temporary fix in BridgeDB. It isn't elegant or anything, but it works, and there's a note to this ticket saying that at some point in the future we can remove it.

The patch is in my fix/13202-missing-pt-args branch, and basically consists of adding the following method to the bridgedb.bridges.PluggableTransport class, which is called whenever the PluggableTransport's attributes are checked, which happens whenever it is updated:

+ def _checkArguments(self):
+     """This method is a temporary fix for PTs with missing arguments
+     (see `#13202 <https://bugs.torproject.org/13202`_). This method can
+     be removed after Tor-0.2.4.x is deprecated.
+     """
+     # obfs4 requires (iat-mode && (cert || (node-id && public-key))):
+     if self.methodname == 'obfs4':
+         if self.arguments.get('iat-mode'):
+             if (self.arguments.get('cert') or \
+                 (self.arguments.get('node-id') and self.arguments.get('public-key'))):
+                 return True
+     # scramblesuit requires (password):
+     elif self.methodname == 'scramblesuit':
+         if self.arguments.get('password'):
+             return True
+     else:
+         return True
+
+     return False

There are also unittests to ensure this behaviour works as expected.

comment:21 Changed 4 years ago by isis

Summary: Figure out a way to deal with bridges missing arguments.Figure out a way to deal with bridges missing PT arguments.

comment:22 Changed 4 years ago by isis

Status: needs_reviewneeds_information

I'm guessing that the remainder of the answer to this ticket is "wait until tor-0.2.4 is deprecated and continue to tell bridge operators to use tor>=0.2.5", right?

comment:23 Changed 4 years ago by yawning

Resolution: wontfix
Status: needs_informationclosed

Closing this, since we're on 0.2.7.x, and we haven't had issues with this recently. If we have a large swath of broken bridges, the operators will need to upgrade Tor...

comment:24 Changed 3 years ago by teor

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

Milestone renamed

comment:25 Changed 3 years ago by nickm

Milestone: Tor: 0.3.???

Milestone deleted

Note: See TracTickets for help on using tickets.