Opened 6 years ago

Closed 6 years ago

#9013 closed defect (fixed)

BridgeDB should pass pluggable transport shared-secrets to clients

Reported by: asn Owned by: isis
Priority: High Milestone:
Component: Circumvention/BridgeDB Version:
Severity: Keywords: pt
Cc: sysrqb, isis, phw Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

To correctly deploy pluggable transports like scramblesuit (that use shared-secrets), BridgeDB will need to get the shared-secrets out of the extra-info descriptor, using the arglist argument of the transport line as explained here:
https://gitweb.torproject.org/torspec.git/blob/HEAD:/proposals/180-pluggable-transport.txt#l454

Child Tickets

Change History (14)

comment:1 Changed 6 years ago by asn

Cc: phw added

Oh, in case anyone cares, the relevant "Tor writes shared-secret to extra-info descriptor" ticket is #8978.

comment:2 Changed 6 years ago by isis

Owner: set to isis
Status: newassigned

Okay, I read over 642ea61bda70f2dafbd89e27aa91a5d452888358 in asn's bug8978 branch. It's just an unlimited number of unsanitised key=value pairs.

From #9445:

The [arglist] portion of an <strikethough>extra-info</strikethrough> descriptor transport string is somewhat problematic, with the current way that it is specified -- though it does make sense to be spec'd this way. Basically, tor does no sanitisation of the transport line [arglist] for a pluggable transport sending args, because it is within the treat model to assume that the transport is a trusted application.

However, this puts all the responsibility of parsing on BridgeDB. Which is also fine, and much more doable in Python than in C...it's just that writers of pluggable transports which they would like to see deployed need to create a spec, and need to create a ticket for BridgeDB that points to the spec and says exactly what BridgeDB should parse for.

Another minor source of confusion, in little-t tor, the [arglist] is carried around as a comma-separated string, whereas in the actual bridge extra-info descriptors it seems to be space-separated like this:

transport obfs3 11.22.33.44:443 [exec /usr/local/bin/obfsproxy managed]

and so it's not actually following the [arglist] specification. If I understood that spec correctly, it should be like this:

transport obfs3 11.22.33.44:443 [ARGS:exec=/usr/local/bin/obfsproxy,runtime=managed]

or something.

comment:3 Changed 6 years ago by isis

Priority: normalmajor
Status: assignedneeds_information

Changing priority to 'major' because #9445 is blocking for TBB-3.0.

Also, changing to 'needs more info' because I've no idea what the arglist in a transport line should look like in general, and less idea what it should look like for ScrambleSuit.

comment:4 Changed 6 years ago by asn

The idea was that the elements of the arglist are space-separated as in:

transport obfs3 11.22.33.44:443 shared-secret=asedqwe secondarg=lol

The spec is indeed unclear and should be fixed.

Do you think that:

transport obfs3 11.22.33.44:443 shared-secret=asedqwe,secondarg=lol

would have been better?

comment:5 in reply to:  4 ; Changed 6 years ago by isis

Replying to asn:

The spec is indeed unclear and should be fixed.

Do you think that:

transport obfs3 11.22.33.44:443 shared-secret=asedqwe,secondarg=lol

would have been better?

Yes, I think the latter one, with the commas in-between, is likely easier to parse since the arglist is of unspecified length.

Is there a different purpose for the [exec /usr/local/bin/obfsproxy managed] portion of the current descriptors?

comment:6 in reply to:  5 ; Changed 6 years ago by sysrqb

Replying to isis:

Is there a different purpose for the [exec /usr/local/bin/obfsproxy managed] portion of the current descriptors?

That's defined in the server-side's torrc. It isn't written to the extra-info doc.

An example is [0]

bridge trebuchet www.example.com:3333 keyid=09F911029D74E35BD84156C5635688C009F909F9 rocks=20 height=5.6m

And more generally [1]:

   Bridge method address:port [[keyid=]id-fingerprint] [k=v] [k=v] [k=v]

comment:7 in reply to:  4 Changed 6 years ago by sysrqb

Replying to asn:

The idea was that the elements of the arglist are space-separated as in:

transport obfs3 11.22.33.44:443 shared-secret=asedqwe secondarg=lol

The spec is indeed unclear and should be fixed.

Yup, I think the spec if contradictory. It presents the examples (as in my last comment) with a space-separated arglist but then it defines the options, afaict, as comma-separated.

The spec defines [2] the arglist as:

Bridges put the 'method' lines in their extra-info documents.
     transport SP <transportname> SP <address:port> [SP arglist] NL

  The address:port are as returned from an SMETHOD line (unless they are
  replaced by the FORWARD: directive).  The arglist is a K=V,... list as
  returned in the ARGS: part of the SMETHOD line's Options component.

Where ARGS: is

      - ARGS:K=V,K=V,K=V

        If this option is set, the K=V arguments are added to Tor's
        extrainfo document.

and that leads me to think extra-info documents present args as comma-separated. dir-spec does not specify the format of arglist, either. Did I misinterpret something? If not, then you prefer to change the spec? (I'm not opposed to this, just looking for clarity).

comment:8 in reply to:  6 Changed 6 years ago by isis

Replying to sysrqb:

Replying to isis:

Is there a different purpose for the [exec /usr/local/bin/obfsproxy managed] portion of the current descriptors?

That's defined in the server-side's torrc. It isn't written to the extra-info doc.

An example is [0]

bridge trebuchet www.example.com:3333 keyid=09F911029D74E35BD84156C5635688C009F909F9 rocks=20 height=5.6m

The above line is what BridgeDB is supposed to create for the the lines it hands out to clients (i.e. over the web interface and through email). Though actually, I was also misremembering what the transport lines of the extrainfo bridge descriptors look like. Currently, in the cached-extrainfo bridge descriptors, there are lines which look like this:

transport obfs3 6.6.6.6:6666

and I thought that the above line ended in:

[exec /usr/local/bin/obfsproxy managed]

though I must have been mixing up the spec and what I remembered of the descriptors.

And more generally [1]:

   Bridge method address:port [[keyid=]id-fingerprint] [k=v] [k=v] [k=v]

Again, for clarity, the above line is the spec for what BridgeDB needs to give to clients, not what BridgeDB should expect to find in a descriptor.

For the record, I like the idea of the bridge extrainfo descriptors looking like this:

transport obfs666 6.6.6.6:6666 argone=/usr/local/bin/obfsproxy,shared_secret=xyzzy

and for BridgeDB to take that information from the descriptor and distribute the following line to clients:

bridge obfs666 6.6.6.6:6666 keyid=0123456789abcdef0123456789abcdef01234567 argone=/usr/local/bin/obfsproxy shared_secret=xyzzy

where most of that info is taken from the transport line of the bridge extrainfo descriptor, all except for the fingerprint, which is taken from the bridge-server-descriptor.

comment:9 Changed 6 years ago by asn

Hi,

looking at the code again, the extrainfo line looks indeed like this:

transport obfs666 6.6.6.6:6666 argone=/usr/local/bin/obfsproxy,shared_secret=xyzzy

I also pushed branch bug9013 in https://git.torproject.org/user/asn/torspec.git. Maybe the added example will make things more comprehensible. I will ask Nick to merge it if you like it.

comment:10 in reply to:  9 ; Changed 6 years ago by sysrqb

Replying to asn:

looking at the code again, the extrainfo line looks indeed like this:

transport obfs666 6.6.6.6:6666 argone=/usr/local/bin/obfsproxy,shared_secret=xyzzy

I think that using the path to the binary in an example will confuse some people. Is there a reason a PT should ever send a file path as an argument?

I also pushed branch bug9013 in https://git.torproject.org/user/asn/torspec.git. Maybe the added example will make things more comprehensible. I will ask Nick to merge it if you like it.

I think that looks fine, unless isis has (or others have) objections I think it should be merged. It's simple but it reenforces the correct format.

comment:11 in reply to:  10 Changed 6 years ago by asn

Replying to sysrqb:

Replying to asn:

looking at the code again, the extrainfo line looks indeed like this:

transport obfs666 6.6.6.6:6666 argone=/usr/local/bin/obfsproxy,shared_secret=xyzzy

I think that using the path to the binary in an example will confuse some people. Is there a reason a PT should ever send a file path as an argument?

Ah, ok, here is another example:

transport obfs666 6.6.6.6:6666 argone=valueone,shared_secret=xyzzy

atm I can't think of a reason that a PT would ever send a file path, but it could as well happen in the future.

comment:12 Changed 6 years ago by phw

For the record: all that ScrambleSuit adds is a password argument which can look like this: password=LLDNOWV7I4P6RKFJMDEMIY2GNU2IQISA. The argument consists of 20 random bytes encoded with Base32.

In my client's torrc, I would put:

Bridge scramblesuit a.b.c.d:1234 password=LLDNOWV7I4P6RKFJMDEMIY2GNU2IQISA

In my server's torrc, I would put:

ServerTransportListenAddr scramblesuit a.b.c.d:1234
ServerTransportOptions scramblesuit password=LLDNOWV7I4P6RKFJMDEMIY2GNU2IQISA

Let me know if I can help with anything.

comment:13 Changed 6 years ago by sysrqb

Status: needs_informationneeds_review

Please review branch bug9013 in my public repo (sorry, it's been sitting locally for a while).

phw, thanks! I think we should be good to go as soon as this is merged.

comment:14 in reply to:  13 Changed 6 years ago by isis

Resolution: fixed
Status: needs_reviewclosed

Replying to sysrqb:

Please review branch bug9013 in my public repo (sorry, it's been sitting locally for a while).

Looks good!

I rebased it on top of develop in my branch fix/9013_r1. CI results are here and this is passing. I'm merging this in and closing as fixed.

Note: See TracTickets for help on using tickets.