Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#5082 closed task (fixed)

Tor cleans out environment before launching obfsproxy

Reported by: Sebastian Owned by:
Priority: High Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: twilde@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This means obfsproxy can't launch in TBB, because LD_LIBRARY_PATH is not used. We should probably fix this along with #5076 by just using Tor's environment, and appending the new env vars we need.

Child Tickets

Change History (14)

comment:1 Changed 9 years ago by ioerror

So basically, you want tor to pass environ to execve or CreateProcess?

comment:2 Changed 9 years ago by ioerror

Doesn't set_managed_proxy_environment do exactly this?

comment:3 Changed 9 years ago by Sebastian

Status: newneeds_review

Only on Windows.

bug5082 in my repo for a fix for this and also for #5076. Please review

comment:4 Changed 9 years ago by Sebastian

Along with this goes an update for proposal 180 in my prop180 branch in my torspec repo

comment:5 Changed 9 years ago by arma

I think Tor should not fail to try launching obfsproxy just because it doesn't find HOME or PATH in its env. That's definitely a bug in Tor.

Does obfsproxy actually need HOME or PATH? I guess there's a question of which one should try to reconstruct it if it isn't defined. But I think currently it doesn't, so maybe we don't need to answer that question yet.

Is it dangerous to pass Tor's env vars to obfsproxy? I'm not sure. I think the benefits outweight the risks.

You'll definitely want to change the sentence "We can do this only if the spec change to require HOME and PATH to be set is accepted" because it implies that you want to change the spec to require HOME and PATH to be set.

I'd suggest a follow-up cleanup commit to get rid of the r, and change the function to return a void, clean up launch_managed_proxy() so it knows set_managed_proxy_environment() can't fail, etc.

I have not evaluated the fiddly pointer arithmetic yet.

comment:6 Changed 9 years ago by twilde

Cc: twilde@… added

comment:7 Changed 9 years ago by arma

Pointer arithmetic (ugh) looks plausible.

I also tested it and it works.

comment:8 Changed 9 years ago by arma

Status: needs_reviewneeds_revision

comment:9 in reply to:  5 Changed 9 years ago by Sebastian

Status: needs_revisionneeds_review

Replying to arma:

Does obfsproxy actually need HOME or PATH? I guess there's a question of which one should try to reconstruct it if it isn't defined. But I think currently it doesn't, so maybe we don't need to answer that question yet.

At least currently, it doesn't. Needing HOME would be quite strange indeed for a daemon at least. And what would it do with PATH?

Is it dangerous to pass Tor's env vars to obfsproxy? I'm not sure. I think the benefits outweight the risks.

I can't think of a reason why it'd be dangerous.

You'll definitely want to change the sentence "We can do this only if the spec change to require HOME and PATH to be set is accepted" because it implies that you want to change the spec to require HOME and PATH to be set.

fixed. that doesn't belong in the changelog anyway.

I'd suggest a follow-up cleanup commit to get rid of the r, and change the function to return a void, clean up launch_managed_proxy() so it knows set_managed_proxy_environment() can't fail, etc.

added

I have not evaluated the fiddly pointer arithmetic yet.

also changed as suggested on irc.

fixup + follow up commit added.

comment:10 Changed 9 years ago by arma

Update looks good to me. I'll let Nick merge it since he should opine on the "get rid of $HOME and $PATH requirement" too.

comment:11 in reply to:  7 Changed 9 years ago by arma

Replying to arma:

I also tested it and it works.

For posterity, here was my obfsproxy patch for testing it:

diff --git a/src/managed.c b/src/managed.c
index 01814f8..ffa40a8 100644
--- a/src/managed.c
+++ b/src/managed.c
@@ -26,6 +26,9 @@
 
 #include "container.h"
 
+#include <unistd.h>
+extern char **environ;
+
 #include <event2/event.h>
 #include <event2/listener.h>
 
@@ -430,6 +433,12 @@ handle_environment(managed_proxy_t *proxy)
   char *tmp;
   enum env_parsing_status status=ST_ENV_SMOOTH;
 
+  char **environ_tmp = environ;
+  while (*environ_tmp) {
+    log_info("env: %s", *environ_tmp);
+    environ_tmp++;
+  }
+
   tmp = getenv("TOR_PT_STATE_LOCATION");
   if (!tmp) {
     status = ST_ENV_FAIL_STATE_LOC;

comment:12 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Squashed and merged this. #5105 should clean it up a fair bit, too. Thanks, all!

comment:13 Changed 8 years ago by nickm

Keywords: tor-client added

comment:14 Changed 8 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.