ScrambleSuit should make it easy for bridge admins to learn password

At this point, the password is stored in pt_state/scramblesuit/server_state.cpickle in Python's string encoding. However, bridge operators might want to test their bridge and we should make that easier.

Ideally, we would have a file descriptor.txt which contains a copy&pastable bridge descriptor including the password. That would also be consistent with how users learn about their hidden service descriptor.

added component::archived/obfsproxy owner::phw password priority::medium resolution::fixed scramblesuit secret shared status::closed type::enhancement labels

As pointed out in the description, I believe that a reasonable solution would be to have a file server_descriptor in the directory pt_state/scramblesuit/. It should be a ready-to-use descriptor such as:

Bridge scramblesuit 127.0.0.1:1234 password=XBA76LNRO2DAQV4YDXSHOYL34L2EP5D2

However, we need to modify obfsproxy so that it writes the bind address (which it gets from pyptlib) to TransportConfig(). I already have a patch for that. Does that sound reasonable? Are there better solutions?

Trac:
Status: new to needs_information
Cc: N/A to asn

Replying to phw:

As pointed out in the description, I believe that a reasonable solution would be to have a file server_descriptor in the directory pt_state/scramblesuit/. It should be a ready-to-use descriptor such as:

{{{ Bridge scramblesuit 127.0.0.1:1234 password=XBA76LNRO2DAQV4YDXSHOYL34L2EP5D2 }}}

However, we need to modify obfsproxy so that it writes the bind address (which it gets from pyptlib) to TransportConfig(). I already have a patch for that. Does that sound reasonable? Are there better solutions?

Sounds plausible. Where can I see that patch?

The patch is in branch bug_10887 in my user repository: https://gitweb.torproject.org/user/phw/obfsproxy.git/shortlog/refs/heads/bug_10887

Trac:
Status: needs_information to needs_review

Replying to phw:

The patch is in branch bug_10887 in my user repository: https://gitweb.torproject.org/user/phw/obfsproxy.git/shortlog/refs/heads/bug_10887

OK. This makes sense to me.

Would be nice if you could document bindAddr in transport_config.py (yeah, even though the rest of the things are not really documented), and maybe specify that it's server-side only?

Also, maybe you could setBindAddr in do_external_mode() too? Shouldn't be too hard I reckon.

Replying to asn:

Replying to phw:

The patch is in branch bug_10887 in my user repository: https://gitweb.torproject.org/user/phw/obfsproxy.git/shortlog/refs/heads/bug_10887

OK. This makes sense to me.

Would be nice if you could document bindAddr in transport_config.py (yeah, even though the rest of the things are not really documented), and maybe specify that it's server-side only?

Also, maybe you could setBindAddr in do_external_mode() too? Shouldn't be too hard I reckon.

Both fixed in branch bug_10887-2: https://gitweb.torproject.org/user/phw/obfsproxy.git/shortlog/refs/heads/bug_10887-2

Replying to phw:

Replying to asn:

Replying to phw:

The patch is in branch bug_10887 in my user repository: https://gitweb.torproject.org/user/phw/obfsproxy.git/shortlog/refs/heads/bug_10887

OK. This makes sense to me.

Would be nice if you could document bindAddr in transport_config.py (yeah, even though the rest of the things are not really documented), and maybe specify that it's server-side only?

Also, maybe you could setBindAddr in do_external_mode() too? Shouldn't be too hard I reckon.

Both fixed in branch bug_10887-2: https://gitweb.torproject.org/user/phw/obfsproxy.git/shortlog/refs/heads/bug_10887-2

Looks good to me. When you have the scramblesuit/obfsproxy branch (that creates the server_descriptor file), I will merge both branches.

Trac:
Component: Pluggable transport to Obfsproxy

This seems to break external mode in obfsproxy, because there is no state location set and we get:

|   File "/home/user/obfsproxy/obfsproxy/transports/scramblesuit/state.py", line 64, in writeServerDescriptor
|     assert const.STATE_LOCATION != ""
| AssertionError

Maybe we should only do writeServerDescriptor only when in managed mode? Or maybe we should set the state location to a dummy value when in external mode?

Trac:
Status: needs_review to needs_revision

For example this change makes it work (int tests pass):

--- a/obfsproxy/transports/scramblesuit/scramblesuit.py
+++ b/obfsproxy/transports/scramblesuit/scramblesuit.py
@@ -129,7 +129,7 @@ class ScrambleSuitTransport( base.BaseTransport ):
 
                 cls.uniformDHSecret = cls.uniformDHSecret.strip()
 
-        if cls.weAreServer:
+        if cls.weAreServer and not cls.weAreExternal:
             if not hasattr(cls, "uniformDHSecret"):
                 log.debug("Using fallback password for descriptor file.")
                 srv = state.load()

Also, the assert in writeServerDescriptor: assert len(password) == const.SHARED_SECRET_LENGTH will trigger if an operator specifies a different sized password in his torrc.

We should probaly not trigger an assert, and instead fail gracefully.

I fixed the above errors and another one (I caught b32decode() exceptions) and it can be found in bug10887 in my repo.

Unfortunately, it still doesn't work perfectly:

# cat pt_state/scramblesuit/server_descriptor 
Bridge scramblesuit 0.0.0.0:33647 password=S5JY6IRCLLNEGTWBWZMYVIXHFWTITZBE

That's because the bindaddr that is passed from Tor is 0.0.0.0 (IPADDR_ANY):

 'config': {'ORPort': ('127.0.0.1', 42331),
            'allTransportsEnabled': False,
            'authCookieFile': None,
            'extendedORPort': None,
            'managedTransportVer': ['1'],
            'serverBindAddr': {'obfs3': ('0.0.0.0', 40674),
                               'scramblesuit': ('0.0.0.0', 33647)},
            'serverTransportOptions': None,
            'stateLocation': '/usr/local/var/lib/tor2/data/pt_state/',
            'transports': ['obfs3', 'scramblesuit']},

However, I'm still tempted to merge this since it's the only way for people to get their automatically-generated passwords. However, maybe we should remove the whole Bridge line and just leave the password bit, so that we don't give users the illusion that that bridge line would actually work.

Also, on my way to fixing the above, I set the default state directory in external mode to be the current working directory. Is this a very bad idea that will open us to race conditions/symlink attacks etc.? Probably better than setting it to /tmp/.

Replying to asn:

I fixed the above errors and another one (I caught b32decode() exceptions) and it can be found in bug10887 in my repo.

ec61559 ACK. aa3a99c NACK for now. See discussion below. 9840bac ACK the change. Commit message should reflect what's being changed though.

Unfortunately, it still doesn't work perfectly: {{{

cat pt_state/scramblesuit/server_descriptor

Bridge scramblesuit 0.0.0.0:33647 password=S5JY6IRCLLNEGTWBWZMYVIXHFWTITZBE }}} That's because the bindaddr that is passed from Tor is 0.0.0.0 (IPADDR_ANY): {{{ 'config': {'ORPort': ('127.0.0.1', 42331), 'allTransportsEnabled': False, 'authCookieFile': None, 'extendedORPort': None, 'managedTransportVer': ['1'], 'serverBindAddr': {'obfs3': ('0.0.0.0', 40674), 'scramblesuit': ('0.0.0.0', 33647)}, 'serverTransportOptions': None, 'stateLocation': '/usr/local/var/lib/tor2/data/pt_state/', 'transports': ['obfs3', 'scramblesuit']}, }}}

However, I'm still tempted to merge this since it's the only way for people to get their automatically-generated passwords. However, maybe we should remove the whole Bridge line and just leave the password bit, so that we don't give users the illusion that that bridge line would actually work.

I would rather see the bridge line changed to only contain the password before merging (the generated bridge line also neglects to include a bridge fingerprint since the information is unavailable to the PT currently). Only including a password line is better than having a bridge line that is wrong and incomplete.

Also, on my way to fixing the above, I set the default state directory in external mode to be the current working directory. Is this a very bad idea that will open us to race conditions/symlink attacks etc.? Probably better than setting it to /tmp/.

It's better than /tmp but not by much. I would rather standalone servers failed to start without a user provided state directory, mostly so it doesn't put it's state in surpising locations when invoked from incorrect init scripts etc.

I agree with your comments.

I have some progress on bug10887_take2 but it's not done.

The unittests fail because they don't set a state location, and also I just noticed that setup() is run for every transport when in external mode. We should fix that.

I'm postponing this ticket for the future (in 2 weeks or so).

Replying to yawning:

I would rather see the bridge line changed to only contain the password before merging (the generated bridge line also neglects to include a bridge fingerprint since the information is unavailable to the PT currently). Only including a password line is better than having a bridge line that is wrong and incomplete.

Yes, ideally including a comment that the password should be appended to the bridge descriptor.

The unittests fail because they don't set a state location

I would modify writeServerDescriptor() so that instead of triggering an assert error, it should simply not write a server descriptor if the state location is not known (and perhaps log a warning message). That way, we also don't need a default state directory?

Did some work on this and pushed it at bug10887_take3. Might be ready for merge.

Some problems I had to tackle:

• scramblesuit unittests had /tmp hardcoded as the state location. I don't like this since it might allow for symlink attacks in weird setups. I started using the tempfile module and hopefully replaced all the occurences of this.
• scramblesuit unittests were not cleaning up their state location afterwards, which left /tmp dirty. I used shutil.rmtree to delete those directories.
• Since we are not trying to print the whole Bridge line anymore, we don't care about the bindaddr so I ignored all the changes wrt bindaddr in Philipp's old branch.
• If we are starting up in external mode, we should only call the setup() method of the transport we are going to launch, not of all transports.

I tested the changes and they seem to work.

I'm still not fully satisfied with the code quality, but I think I spent enough time on this for now.

Trac:
Status: needs_revision to needs_review

Ok, review time:

• ee185b8904a6463e925e27df7e18e3e64e77b9fc - "Fix path joining in scramblesuit/state.py."

  • Trivially correct, ACK.

• 49af49f55243f38df8d4053445c56b9e6bca0050 - "Use temporary files instead of "/tmp" in scramblesuit unittests."

  • TicketTest will leave the temp dir if it fails (use setUp/tearDown here, and handle exceptions in the tearDown).
  • Likewise ScrambleSuitTransportTest.test3_get_public_server_options can leak the temp dir.

• 3a1d00693c0c6069aff95aac37f75fd0615579e1 - "Remove a broken unittest.":

  • Trivially correct, ACK.

• 521a88f1034d7d48af1b68ffdfdaf94a06c7487b - "Write password to a file, instead of the whole Bridge line."

  • Please do not perpetuate people using bridge lines without fingerprints (may be hard to fit for size reasons).
  • Use 192.0.2.1 for the example IP (TEST-NET-1).
  • ACK, but change the example bridge line in the password file if you think it will look ok.

• 8f9a0aaa95308cb8c249c40d578f79ff70dc3cd0 - "Catch some exceptions in scramblesuit's setup() and fail gracefully."

  • Looks good to me, ACK.

• 5b83c8f007f7827231d1cea98d165ae23208e6cb - "When in external mode, only call setup() of the transports we are launching."

  • ChangeLog entry?
  • Code is trivially correct, ACK.

• 72f5423ee717b40c24fc04928f6ed6e0accc0d85 - "Add a ChangeLog entry for the password file."

  • ChangeLog, ACK.

Trac:
Status: needs_review to needs_revision

I think I fixed all your comments.

Thanks for the review.

Please review, and if it is up to your satisfaction, I will merge.

Trac:
Status: needs_revision to needs_review

Round 2:

• ed4f43f21a2be32ecdb0fd0ba6dbccfe8188309e - "Ensure files get removed in unittests (using the tearDown() method)."

  • ACK.

• f20141be653872d5ef3243618e0e4274c8ba8131 - "fixup! Write password to a file, instead of the whole Bridge line."

  • "Bridge [] : [] [=] [=] [=]". Password and fingerprint on the example bridgeline are flipped. I would use "EXAMPLEFINGERPRINTNOTREAL" there instead of an opaque hex identifier.

• 4e46bce1ac0e368368dd0171aad205b0267c0706 - "Add a ChangeLog entry about the new run_transport_setup() behavior."

  • ACK.

After you flip the fingerprint and password so the example actually conforms to the pt-spec, I think this is ok to merge. Thanks for all the work!

Trac:
Status: needs_review to needs_revision

Hah good find.

Fixed the bridge line, squashed a bit and merged!

Thanks!

Trac:
Status: needs_revision to closed
Resolution: N/A to fixed

closed

mentioned in issue #11271 (closed)