Opened 8 years ago

Closed 6 years ago

#4773 closed defect (implemented)

Implement Extended OR port (part of proposal 180)

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: pt needs-proposal tor-bridge wants-mocking
Cc: dcf@… Actual Points:
Parent ID: #5408 Points:
Reviewer: Sponsor:

Description


Child Tickets

TicketStatusOwnerSummaryComponent
#7098closedAdd safe-cookie authentication to Extended ORPort and TransportControlPortCore Tor/Tor

Change History (39)

comment:1 Changed 8 years ago by asn

Nick, how would you implement this? I want to prepare an implementation plan before starting coding stuff, since it seems a bit messy.

My plan is to define a new listener type to handle 'Extended OR' connections, handle the 'Extended OR protocol' in connection_init_accepted_conn() and when the 'Extended OR protocol' is done, handle the connection like a CONN_TYPE_OR connection (maybe by calling connection_tls_start_handshake()).

comment:2 Changed 8 years ago by nickm

I would suggest not implementing it until a redesign of the whole extended-OR approach to accommodate the additional information we want to be able to communicate about OR connections. Probably, the right implementation will depend at least in part on what it is we're trying to implement.

comment:3 Changed 8 years ago by asn

Parent ID: #3591#4685

comment:4 Changed 8 years ago by asn

(The Extended ORPort redesign discussion began in #3587, and I currently have a proposal in the making.)

comment:5 Changed 8 years ago by asn

Let's talk a bit about our security threat model.

The current (180) Extended ORPort design allows a local "attacker" to connect to the Extended ORPort, spoof an arbitrary external address (using USERADDR), and send Tor data. Furthermore, if we do the "the Extended ORPort provides an identifier to be used in another port for metadata/configuration transfer between tor and the proxy" idea, a local "attacker" will be able to connect to the Extended ORPort, get an identifier, connect to the other port, and configure the transport proxy (for example, tweak its rate-limiting setup).

Would it be worth it to add some sort of authentication, so that only pluggable transport proxies can use the Extended ORPort? A silly way of doing it would be to add a key in a TOR_PT_* environment variable, but I'm not sure if that would be secure in a cross-platform fashion [0].
That would also kill external proxies from using the Extended ORPort.

Would a file that can only be read by the Tor user, containing a cookie, be better?

Are these "attacks" within our threat model?

[0]:
a related not-so-enlightening blog post
https://patternbuffer.wordpress.com/2008/05/05/unix-environment-variable-scopesecurity/
We also assume that if the local attacker has root, the game is lost.

comment:6 in reply to:  2 Changed 8 years ago by asn

Replying to nickm:

I would suggest not implementing it until a redesign of the whole extended-OR approach to accommodate the additional information we want to be able to communicate about OR connections. Probably, the right implementation will depend at least in part on what it is we're trying to implement.

Please see https://lists.torproject.org/pipermail/tor-dev/2012-January/003225.html for a draft.

comment:7 Changed 7 years ago by nickm

Just responded to the draft on tor-dev. Pending discussion, I'll do the edits.

comment:8 Changed 7 years ago by nickm

Status: newneeds_review

I've got a (not spec-conformant!) extended ORPort implementation. It implements the parts of the extended ORPort feature set that we knew we would need in proposal 180. It doesn't have a transportControlPort yet. It needs documentation!

It lives in branch "ext_orport" in my public repository.

comment:9 in reply to:  8 Changed 7 years ago by asn

Replying to nickm:

I've got a (not spec-conformant!) extended ORPort implementation. It implements the parts of the extended ORPort feature set that we knew we would need in proposal 180. It doesn't have a transportControlPort yet. It needs documentation!

It lives in branch "ext_orport" in my public repository.

Looks good to me.
We should leave it in needs_review till we implement the TransportControlPort too.

comment:10 Changed 7 years ago by asn

I made #5408 for implementing the rest of proposal 196.

comment:11 Changed 7 years ago by karsten

Parent ID: #4685

Removing parent ticket relationship to #4685 in order to close it.

comment:12 Changed 7 years ago by asn

Parent ID: #5408

comment:13 Changed 7 years ago by asn

Nick, what happens to people who want to run a pluggable transport proxy on a different box than tor? I know that skep wanted to do that.

We can't let the ExtendedORPort be globally reachable because people will be able to spoof IP addresses with USERADDR.

Should we add an authentication scheme ("...and now you have 1000 problems")? Should we say "this is not possible"? Should we simply log_warn() on startup and let the bridge operator do whatever he thinks is wise?

I'm not even sure if it's wise to have an un-authenticated Extended ORPort bound in localhost, since local users will still be able to spoof IP addresses (comment:5). We probably need to add a threat model to the proposal.

What do you say?

comment:14 Changed 7 years ago by nickm

Nick, what happens to people who want to run a pluggable transport proxy on a different box than tor?

If I really had to be in that situation, and the two boxes were on the same LAN, I'd use iptables or something so that only the box running the proxy could connect to the extended ORPort.

If they weren't on the same LAN, I'd be out of luck.

What do you say?

Adding a better threat model is indeed a reasonable thing to do.

comment:15 Changed 7 years ago by arma

I haven't been following this discussion or the proposal, but

  • I'd say there's no need yet to handle the case where the bridge is on a different machine than the obfsproxy.
  • If we do in fact have a sensitive TCP connection where we bind to localhost and think of that as our authentication mechanism, that's bad.

comment:16 in reply to:  15 Changed 7 years ago by asn

Replying to arma:

I haven't been following this discussion or the proposal, but

  • I'd say there's no need yet to handle the case where the bridge is on a different machine than the obfsproxy.
  • If we do in fact have a sensitive TCP connection where we bind to localhost and think of that as our authentication mechanism, that's bad.

Yes, I'm not sure what is the best thing to do here. We are basically opening ourselves to another round of http://www.remote.org/jochen/sec/hfpa/index.html.

comment:17 Changed 7 years ago by nickm

I must add documentation here before we can merge it. Still needs more review. I'm adding this to tor-next, but I won't feel comfortable merging it until we have a transport to test it with.

Needs documentation. Needs a changes file.

I think there is a missing EXT_OR_CMD_OKAY write in the handler for EXT_OR_CMD_USERADDR.

comment:18 in reply to:  17 Changed 7 years ago by asn

Replying to nickm:

I must add documentation here before we can merge it. Still needs more review. I'm adding this to tor-next, but I won't feel comfortable merging it until we have a transport to test it with.

I've been testing this with obfsproxy from #5453.

Also, I remember Roger feeling that there should be some sort of authentication protecting Extended ORPort.

Needs documentation. Needs a changes file.

I think there is a missing EXT_OR_CMD_OKAY write in the handler for EXT_OR_CMD_USERADDR.

comment:19 Changed 7 years ago by asn

Keywords: pt added
Status: needs_reviewneeds_revision

comment:20 Changed 7 years ago by asn

During the dev. meeting, Nick and I decided that using safe-cookie authentication to protect Extended ORPort might be a good idea.

comment:21 Changed 7 years ago by nickm

Keywords: needs-proposal added

comment:22 Changed 7 years ago by nickm

Keywords: tor-bridge added

comment:23 Changed 7 years ago by nickm

Component: Tor BridgeTor

comment:24 Changed 7 years ago by asn

Status: needs_revisionneeds_review

Finally! Check out branch bug4773_rebase in https://git.torproject.org/tor.git. It also implements #7098.

I also made a pyobfsproxy branch that implements #7098 and #4773. I will make a setup HOWTO to get some testing.

comment:25 Changed 7 years ago by dcf

Cc: dcf@… added

comment:26 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: unspecified

Talked with George; we think it's okay to merge this early in 0.2.5.

comment:27 Changed 7 years ago by asn

FWIW, Moritz currently runs tor and pyobfpsroxy with my Extended ORPort branches. GeoIP stats collcetion works correctly, and he hasn't had any memleaks or bugs till now.

I'm thinking of publishing my "how to setup a bridge with Extended ORPort support" in tor-dev.

comment:28 Changed 7 years ago by asn

Milestone: Tor: unspecifiedTor: 0.2.5.x-final

comment:29 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

I'm going to wade through the code tonight and tommorow. Some initial stuff to start on:

  • There should be unit tests for the new cell types and authentication features. There should be as many unit tests as possible.
  • There needs to be an accompanying torspec patch.

comment:30 Changed 6 years ago by nickm

562419bca0de212e1c1ffc3479dbf21da9591daf

  • The commit message is incomplete. It also replaces the 20-byte identifier with an 8-byte pointer. I don't agree with that change. And I almost missed it because the commit message didn't document it.

f915dc616cf4f9ec9058c3f69cdf3608cd360a75

  • The commit message is incomplete. It doesn't just create a cookie file. It also does some other stuff too. Please take the time to make your commit messages accurate as a courtesy to the people who will be reading them.
  • The cookie should probably be a uint8_t, not a char*, since it's cryptographic stuff. All new things that manipulate piles-of-bytes should take uint8_t.
  • get_ext_or_auth_cookie_file should be named ...filename(). It returns a filename, not a file.
  • The argument to init_ext_or_auth_cookie_authentication isn't documented, nor is its behavior. (Why does an "init" function disable something?)
  • I bet that init_ext_or_auth_cookie_authentication is substantially shared with its counterpart in control.c. Can they be one function? It would be great to have as little duplicated code here as possible.

6ee4b4e96769d7db96e6495ce15d47123755ad90

  • Client hash of the Extended ORPort authentication scheme -- what on earth does that documentation mean? When is this field set? Under what circumstance? What does it contain? How do you hash a scheme?

a01f8a02f39d87c8bf6dbcdab1ab68a4054211f7

  • Copy-and-paste is not an okay way to program. Use functions for abstraction; don't copy and paste code.
  • There's no point reviewing copy-and-paste code. I'll come back to this once it and control.c use a common set of functions for their common features.

99f1cdb1adefc8dbef689ec94c9a1a539ae3aec7

  • This is why you don't duplicate functionality or copy and paste: You fixed a bug in initializing the cookie (by checking crypto_rand's return value), but the same bug exists in control.c.

Random thought:

  • Do we have code somewhere to check that ExtORPort is on localhost? We really should.

comment:31 in reply to:  30 ; Changed 6 years ago by asn

Replying to nickm:

Thanks for the review. I have some questions on a subset of your comments:

562419bca0de212e1c1ffc3479dbf21da9591daf

  • The commit message is incomplete. It also replaces the 20-byte identifier with an 8-byte pointer. I don't agree with that change. And I almost missed it because the commit message didn't document it.

f915dc616cf4f9ec9058c3f69cdf3608cd360a75

  • The commit message is incomplete. It doesn't just create a cookie file. It also does some other stuff too. Please take the time to make your commit messages accurate as a courtesy to the people who will be reading them.

You are right about the commit messages. Sorry for the extra pain.
What should I do now? Do you prefer that I create a branch with better commit messages, or should I leave it like this?

  • The cookie should probably be a uint8_t, not a char*, since it's cryptographic stuff. All new things that manipulate piles-of-bytes should take uint8_t.

Right. I tried fixing this, but functions like crypto_rand() and crypto_hmac_sha256() expect char *. Should I just cast the uint8_t * to char *?

  • get_ext_or_auth_cookie_file should be named ...filename(). It returns a filename, not a file.
  • The argument to init_ext_or_auth_cookie_authentication isn't documented, nor is its behavior. (Why does an "init" function disable something?)
  • I bet that init_ext_or_auth_cookie_authentication is substantially shared with its counterpart in control.c. Can they be one function? It would be great to have as little duplicated code here as possible.

It's true. However, the cookie file format is different in each case, and each of them uses a different global variable (authentication_cookie_is_set and ext_or_auth_cookie_is_set). Making a function that will help both cases, might look weird.

6ee4b4e96769d7db96e6495ce15d47123755ad90

  • Client hash of the Extended ORPort authentication scheme -- what on earth does that documentation mean? When is this field set? Under what circumstance? What does it contain? How do you hash a scheme?

a01f8a02f39d87c8bf6dbcdab1ab68a4054211f7

  • Copy-and-paste is not an okay way to program. Use functions for abstraction; don't copy and paste code.
  • There's no point reviewing copy-and-paste code. I'll come back to this once it and control.c use a common set of functions for their common features.

Hm, the common part between those two codebases is the part where I use the client's nonce. Unfortunately, both the protocol format and the HMAC construct are quite different between the control-safecookie and the extended-or-port authentication protocols. There was a big discussion on whether these two protocols should be different in #7098, and we decided to go with different protocols. I'm not sure how to merge handle_control_authchallenge() and `
connection_ext_or_auth_handle_client_nonce() in a nice way.

99f1cdb1adefc8dbef689ec94c9a1a539ae3aec7

  • This is why you don't duplicate functionality or copy and paste: You fixed a bug in initializing the cookie (by checking crypto_rand's return value), but the same bug exists in control.c.

Random thought:

  • Do we have code somewhere to check that ExtORPort is on localhost? We really should.

comment:32 in reply to:  31 Changed 6 years ago by nickm

Replying to asn:

Replying to nickm:

Thanks for the review. I have some questions on a subset of your comments:

562419bca0de212e1c1ffc3479dbf21da9591daf

  • The commit message is incomplete. It also replaces the 20-byte identifier with an 8-byte pointer. I don't agree with that change. And I almost missed it because the commit message didn't document it.

f915dc616cf4f9ec9058c3f69cdf3608cd360a75

  • The commit message is incomplete. It doesn't just create a cookie file. It also does some other stuff too. Please take the time to make your commit messages accurate as a courtesy to the people who will be reading them.

You are right about the commit messages. Sorry for the extra pain.
What should I do now? Do you prefer that I create a branch with better commit messages, or should I leave it like this?

Add to the branch a squash! commit for those commits, containing better messages. (You might need to set some command-line options to be able to make a commit that doesn't change any code.)

  • The cookie should probably be a uint8_t, not a char*, since it's cryptographic stuff. All new things that manipulate piles-of-bytes should take uint8_t.

Right. I tried fixing this, but functions like crypto_rand() and crypto_hmac_sha256() expect char *. Should I just cast the uint8_t * to char *?

Yes. Eventually we'll do a big uint8_t propagation and make all of those take uint8_t instead.

  • get_ext_or_auth_cookie_file should be named ...filename(). It returns a filename, not a file.
  • The argument to init_ext_or_auth_cookie_authentication isn't documented, nor is its behavior. (Why does an "init" function disable something?)
  • I bet that init_ext_or_auth_cookie_authentication is substantially shared with its counterpart in control.c. Can they be one function? It would be great to have as little duplicated code here as possible.

It's true. However, the cookie file format is different in each case, and each of them uses a different global variable (authentication_cookie_is_set and ext_or_auth_cookie_is_set). Making a function that will help both cases, might look weird.

Then add a flag to control the prefix of the cookie file format, and a pointer to which variable to set. Or something.

Duplicate code is very very bad indeed.

6ee4b4e96769d7db96e6495ce15d47123755ad90

  • Client hash of the Extended ORPort authentication scheme -- what on earth does that documentation mean? When is this field set? Under what circumstance? What does it contain? How do you hash a scheme?

a01f8a02f39d87c8bf6dbcdab1ab68a4054211f7

  • Copy-and-paste is not an okay way to program. Use functions for abstraction; don't copy and paste code.
  • There's no point reviewing copy-and-paste code. I'll come back to this once it and control.c use a common set of functions for their common features.

Hm, the common part between those two codebases is the part where I use the client's nonce. Unfortunately, both the protocol format and the HMAC construct are quite different between the control-safecookie and the extended-or-port authentication protocols. There was a big discussion on whether these two protocols should be different in #7098, and we decided to go with different protocols. I'm not sure how to merge handle_control_authchallenge() and `
connection_ext_or_auth_handle_client_nonce() in a nice way.

Ug. I didn't remember that. Okay, in that case what it needs is some comments. And a tor-spec patch.

99f1cdb1adefc8dbef689ec94c9a1a539ae3aec7

  • This is why you don't duplicate functionality or copy and paste: You fixed a bug in initializing the cookie (by checking crypto_rand's return value), but the same bug exists in control.c.

Random thought:

  • Do we have code somewhere to check that ExtORPort is on localhost? We really should.

comment:33 Changed 6 years ago by asn

Status: needs_revisionneeds_review

Sorry for the commits not being nicely organized or commented.

How does branch bug4773_rebase look like to you now?

comment:34 Changed 6 years ago by nickm

The changes look fine. Are any issues I mentioned above remaining?

If not, and if obfsproxy uses the new features successfully, I think this is okay for me to merge.

comment:35 in reply to:  34 Changed 6 years ago by asn

Replying to nickm:

The changes look fine. Are any issues I mentioned above remaining?

No. I believe I addressed all the issues you mentioned.

If not, and if obfsproxy uses the new features successfully, I think this is okay for me to merge.

Just tested it and bug4773_rebased seems to work fine with the current master of obfsproxy.

comment:36 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

Here are my notes; I'm happy to make these changes myself before merging. I plan to do them myself once #8949 is merged, unless somebody else does first. I'm just copying them here so I don't lose them.

  • Where are the tests?? There are literally no new tests for all this code. I'll write some after #8949.
  • "char cookie_file_str_len" -- what if the length is over 127? size_t would seem more sensible.
  • Wipe cookie_file_str before freeing it.
  • Do EXT_OR connections interoperate with channels correctly? I should investigate
  • connection_ext_or_auth_handle_client_nonce needs comments.
  • connection_ext_or_auth_handle_client_nonce should wipe.
  • connection_ext_or_auth_handle_client_hash should wipe
  • connection_ext_or_auth_process_inbuf state machine should be documented
  • wipe provided_client_hash.
  • In connection_ext_or_handle_useraddr -- Use tor_memdup_nulterm(), AND check for internal NULs.
  • The <= AUTH_MAX test seems obscure. We should add a function to describe what that set of states mean.
  • We should wipe ext_or_auth_cookie before freeing it.

comment:37 Changed 6 years ago by nickm

Keywords: wants-mocking added

comment:38 Changed 6 years ago by nickm

Remaining matters:

  • Do EXT_OR connections interoperate with channels correctly? I'm hoping so...

comment:39 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_revisionclosed

At last, merged.

Note: See TracTickets for help on using tickets.