Implement Extended OR port (part of proposal 180)
Activity
changed milestone to %Tor: 0.2.5.x-final
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 aCONN_TYPE_ORconnection (maybe by callingconnection_tls_start_handshake()).-
Trac:
Parent: #3591 (moved) to #4685 (moved) -
(The Extended ORPort redesign discussion began in #3587 (moved), and I currently have a proposal in the making.)
-
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.
-
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.
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.
Trac:
Status: new to needs_review-
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_reviewtill we implement the TransportControlPort too. -
I made #5408 (moved) for implementing the rest of proposal 196.
Removing parent ticket relationship to #4685 (moved) in order to close it.
Trac:
Parent: #4685 (moved) to N/A-
Trac:
Parent: N/A to #5408 (moved) -
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?
-
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.
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.
-
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.
-
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.
-
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 (closed).
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.
-
Finally! Check out branch
bug4773_rebaseinhttps://git.torproject.org/tor.git. It also implements #7098 (moved).I also made a pyobfsproxy branch that implements #7098 (moved) and #4773 (moved). I will make a setup HOWTO to get some testing.
Trac:
Status: needs_revision to needs_review Trac:
Cc: N/A to dcf@torproject.org-
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.
Trac:
Status: needs_review to needs_revision -
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.
-
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()andcrypto_hmac_sha256()expectchar *. Should I just cast theuint8_t *tochar *?- 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_setandext_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 (moved), 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.
-
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()andcrypto_hmac_sha256()expectchar *. Should I just cast theuint8_t *tochar *?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_setandext_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 (moved), 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.
-
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_rebasedseems to work fine with the current master of obfsproxy. -
Here are my notes; I'm happy to make these changes myself before merging. I plan to do them myself once #8949 (moved) 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 (moved).
-
"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.
Trac:
Status: needs_review to needs_revision -
mentioned in issue #5040 (moved)
mentioned in issue #7098 (moved)
mentioned in issue #7620 (closed)
mentioned in issue #9651 (moved)
moved to tpo/core/tor#4773 (closed)
mentioned in issue tpo/core/tor#9651 (closed)