diff --git a/changes/ticket25409 b/changes/ticket25409
new file mode 100644
index 000000000..f0006fbc5
--- /dev/null
+++ b/changes/ticket25409
@@ -0,0 +1,6 @@
+ o Code simplification and refactoring:
+ We remove the PortForwsrding and PortForwardingHelper options, related
+ functions, and the port_forwarding tests. These options were used by
+ the now-deprecated Vidalia to help ordinary users become Tor relays or
+ bridges. Closes ticket 25409. Patch by Neel Chauhan.
+
diff --git a/doc/tor.1.txt b/doc/tor.1.txt
index ccf6f8777..3d0e51f55 100644
--- a/doc/tor.1.txt
+++ b/doc/tor.1.txt
@@ -805,10 +805,9 @@ GENERAL OPTIONS
[[NoExec]] **NoExec** **0**|**1**::
If this option is set to 1, then Tor will never launch another
- executable, regardless of the settings of PortForwardingHelper,
- ClientTransportPlugin, or ServerTransportPlugin. Once this
- option has been set to 1, it cannot be set back to 0 without
- restarting Tor. (Default: 0)
+ executable, regardless of the settings of ClientTransportPlugin
+ or ServerTransportPlugin. Once this option has been set to 1,
+ it cannot be set back to 0 without restarting Tor. (Default: 0)
[[Schedulers]] **Schedulers** **KIST**|**KISTLite**|**Vanilla**::
Specify the scheduler type that tor should use. The scheduler is
@@ -2083,18 +2082,6 @@ is non-zero):
For obvious reasons, NoAdvertise and NoListen are mutually exclusive, and
IPv4Only and IPv6Only are mutually exclusive.
-[[PortForwarding]] **PortForwarding** **0**|**1**::
- Attempt to automatically forward the DirPort and ORPort on a NAT router
- connecting this Tor server to the Internet. If set, Tor will try both
- NAT-PMP (common on Apple routers) and UPnP (common on routers from other
- manufacturers). (Default: 0)
-
-[[PortForwardingHelper]] **PortForwardingHelper** __filename__|__pathname__::
- If PortForwarding is set, use this executable to configure the forwarding.
- If set to a filename, the system path will be searched for the executable.
- If set to a path, only the specified path will be executed.
- (Default: tor-fw-helper)
-
[[PublishServerDescriptor]] **PublishServerDescriptor** **0**|**1**|**v3**|**bridge**,**...**::
This option specifies which descriptors Tor will publish when acting as
a relay. You can
diff --git a/src/common/util.c b/src/common/util.c
index f63b12167..065b3245f 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -5111,30 +5111,6 @@ stream_status_to_string(enum stream_status stream_status)
}
}
-/* DOCDOC */
-static void
-log_portfw_spawn_error_message(const char *buf,
- const char *executable, int *child_status)
-{
- /* Parse error message */
- int retval, child_state, saved_errno;
- retval = tor_sscanf(buf, SPAWN_ERROR_MESSAGE "%x/%x",
- &child_state, &saved_errno);
- if (retval == 2) {
- log_warn(LD_GENERAL,
- "Failed to start child process \"%s\" in state %d: %s",
- executable, child_state, strerror(saved_errno));
- if (child_status)
- *child_status = 1;
- } else {
- /* Failed to parse message from child process, log it as a
- warning */
- log_warn(LD_GENERAL,
- "Unexpected message from port forwarding helper \"%s\": %s",
- executable, buf);
- }
-}
-
#ifdef _WIN32
/** Return a smartlist containing lines outputted from
@@ -5254,42 +5230,6 @@ tor_get_lines_from_handle, (int fd, enum stream_status *stream_status_out))
return lines;
}
-/** Read from fd, and send lines to log at the specified log level.
- * Returns 1 if stream is closed normally, -1 if there is a error reading, and
- * 0 otherwise. Handles lines from tor-fw-helper and
- * tor_spawn_background() specially.
- */
-static int
-log_from_pipe(int fd, int severity, const char *executable,
- int *child_status)
-{
- char buf[256];
- enum stream_status r;
-
- for (;;) {
- r = get_string_from_pipe(fd, buf, sizeof(buf) - 1);
-
- if (r == IO_STREAM_CLOSED) {
- return 1;
- } else if (r == IO_STREAM_EAGAIN) {
- return 0;
- } else if (r == IO_STREAM_TERM) {
- return -1;
- }
-
- tor_assert(r == IO_STREAM_OKAY);
-
- /* Check if buf starts with SPAWN_ERROR_MESSAGE */
- if (strcmpstart(buf, SPAWN_ERROR_MESSAGE) == 0) {
- log_portfw_spawn_error_message(buf, executable, child_status);
- } else {
- log_fn(severity, LD_GENERAL, "Port forwarding helper says: %s", buf);
- }
- }
-
- /* We should never get here */
- return -1;
-}
#endif /* defined(_WIN32) */
/** Reads from fd and stores input in buf_out making
@@ -5332,294 +5272,6 @@ get_string_from_pipe(int fd, char *buf_out, size_t count)
return IO_STREAM_OKAY;
}
-/** Parse a line from tor-fw-helper and issue an appropriate
- * log message to our user. */
-static void
-handle_fw_helper_line(const char *executable, const char *line)
-{
- smartlist_t *tokens = smartlist_new();
- char *message = NULL;
- char *message_for_log = NULL;
- const char *external_port = NULL;
- const char *internal_port = NULL;
- const char *result = NULL;
- int port = 0;
- int success = 0;
-
- if (strcmpstart(line, SPAWN_ERROR_MESSAGE) == 0) {
- /* We need to check for SPAWN_ERROR_MESSAGE again here, since it's
- * possible that it got sent after we tried to read it in log_from_pipe.
- *
- * XXX Ideally, we should be using one of stdout/stderr for the real
- * output, and one for the output of the startup code. We used to do that
- * before cd05f35d2c.
- */
- int child_status;
- log_portfw_spawn_error_message(line, executable, &child_status);
- goto done;
- }
-
- smartlist_split_string(tokens, line, NULL,
- SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, -1);
-
- if (smartlist_len(tokens) < 5)
- goto err;
-
- if (strcmp(smartlist_get(tokens, 0), "tor-fw-helper") ||
- strcmp(smartlist_get(tokens, 1), "tcp-forward"))
- goto err;
-
- external_port = smartlist_get(tokens, 2);
- internal_port = smartlist_get(tokens, 3);
- result = smartlist_get(tokens, 4);
-
- if (smartlist_len(tokens) > 5) {
- /* If there are more than 5 tokens, they are part of [].
- Let's use a second smartlist to form the whole message;
- strncat loops suck. */
- int i;
- int message_words_n = smartlist_len(tokens) - 5;
- smartlist_t *message_sl = smartlist_new();
- for (i = 0; i < message_words_n; i++)
- smartlist_add(message_sl, smartlist_get(tokens, 5+i));
-
- tor_assert(smartlist_len(message_sl) > 0);
- message = smartlist_join_strings(message_sl, " ", 0, NULL);
-
- /* wrap the message in log-friendly wrapping */
- tor_asprintf(&message_for_log, " ('%s')", message);
-
- smartlist_free(message_sl);
- }
-
- port = atoi(external_port);
- if (port < 1 || port > 65535)
- goto err;
-
- port = atoi(internal_port);
- if (port < 1 || port > 65535)
- goto err;
-
- if (!strcmp(result, "SUCCESS"))
- success = 1;
- else if (!strcmp(result, "FAIL"))
- success = 0;
- else
- goto err;
-
- if (!success) {
- log_warn(LD_GENERAL, "Tor was unable to forward TCP port '%s' to '%s'%s. "
- "Please make sure that your router supports port "
- "forwarding protocols (like NAT-PMP). Note that if '%s' is "
- "your ORPort, your relay will be unable to receive inbound "
- "traffic.", external_port, internal_port,
- message_for_log ? message_for_log : "",
- internal_port);
- } else {
- log_info(LD_GENERAL,
- "Tor successfully forwarded TCP port '%s' to '%s'%s.",
- external_port, internal_port,
- message_for_log ? message_for_log : "");
- }
-
- goto done;
-
- err:
- log_warn(LD_GENERAL, "tor-fw-helper sent us a string we could not "
- "parse (%s).", line);
-
- done:
- SMARTLIST_FOREACH(tokens, char *, cp, tor_free(cp));
- smartlist_free(tokens);
- tor_free(message);
- tor_free(message_for_log);
-}
-
-/** Read what tor-fw-helper has to say in its stdout and handle it
- * appropriately */
-static int
-handle_fw_helper_output(const char *executable,
- process_handle_t *process_handle)
-{
- smartlist_t *fw_helper_output = NULL;
- enum stream_status stream_status = 0;
-
- fw_helper_output =
- tor_get_lines_from_handle(tor_process_get_stdout_pipe(process_handle),
- &stream_status);
- if (!fw_helper_output) { /* didn't get any output from tor-fw-helper */
- /* if EAGAIN we should retry in the future */
- return (stream_status == IO_STREAM_EAGAIN) ? 0 : -1;
- }
-
- /* Handle the lines we got: */
- SMARTLIST_FOREACH_BEGIN(fw_helper_output, char *, line) {
- handle_fw_helper_line(executable, line);
- tor_free(line);
- } SMARTLIST_FOREACH_END(line);
-
- smartlist_free(fw_helper_output);
-
- return 0;
-}
-
-/** Spawn tor-fw-helper and ask it to forward the ports in
- * ports_to_forward. ports_to_forward contains strings
- * of the form ":", which is the format
- * that tor-fw-helper expects. */
-void
-tor_check_port_forwarding(const char *filename,
- smartlist_t *ports_to_forward,
- time_t now)
-{
-/* When fw-helper succeeds, how long do we wait until running it again */
-#define TIME_TO_EXEC_FWHELPER_SUCCESS 300
-/* When fw-helper failed to start, how long do we wait until running it again
- */
-#define TIME_TO_EXEC_FWHELPER_FAIL 60
-
- /* Static variables are initialized to zero, so child_handle.status=0
- * which corresponds to it not running on startup */
- static process_handle_t *child_handle=NULL;
-
- static time_t time_to_run_helper = 0;
- int stderr_status, retval;
- int stdout_status = 0;
-
- tor_assert(filename);
-
- /* Start the child, if it is not already running */
- if ((!child_handle || child_handle->status != PROCESS_STATUS_RUNNING) &&
- time_to_run_helper < now) {
- /*tor-fw-helper cli looks like this: tor_fw_helper -p :5555 -p 4555:1111 */
- const char **argv; /* cli arguments */
- int args_n, status;
- int argv_index = 0; /* index inside 'argv' */
-
- tor_assert(smartlist_len(ports_to_forward) > 0);
-
- /* check for overflow during 'argv' allocation:
- (len(ports_to_forward)*2 + 2)*sizeof(char*) > SIZE_MAX ==
- len(ports_to_forward) > (((SIZE_MAX/sizeof(char*)) - 2)/2) */
- if ((size_t) smartlist_len(ports_to_forward) >
- (((SIZE_MAX/sizeof(char*)) - 2)/2)) {
- log_warn(LD_GENERAL,
- "Overflow during argv allocation. This shouldn't happen.");
- return;
- }
- /* check for overflow during 'argv_index' increase:
- ((len(ports_to_forward)*2 + 2) > INT_MAX) ==
- len(ports_to_forward) > (INT_MAX - 2)/2 */
- if (smartlist_len(ports_to_forward) > (INT_MAX - 2)/2) {
- log_warn(LD_GENERAL,
- "Overflow during argv_index increase. This shouldn't happen.");
- return;
- }
-
- /* Calculate number of cli arguments: one for the filename, two
- for each smartlist element (one for "-p" and one for the
- ports), and one for the final NULL. */
- args_n = 1 + 2*smartlist_len(ports_to_forward) + 1;
- argv = tor_calloc(args_n, sizeof(char *));
-
- argv[argv_index++] = filename;
- SMARTLIST_FOREACH_BEGIN(ports_to_forward, const char *, port) {
- argv[argv_index++] = "-p";
- argv[argv_index++] = port;
- } SMARTLIST_FOREACH_END(port);
- argv[argv_index] = NULL;
-
- /* Assume tor-fw-helper will succeed, start it later*/
- time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_SUCCESS;
-
- if (child_handle) {
- tor_process_handle_destroy(child_handle, 1);
- child_handle = NULL;
- }
-
-#ifdef _WIN32
- /* Passing NULL as lpApplicationName makes Windows search for the .exe */
- status = tor_spawn_background(NULL, argv, NULL, &child_handle);
-#else
- status = tor_spawn_background(filename, argv, NULL, &child_handle);
-#endif /* defined(_WIN32) */
-
- tor_free_((void*)argv);
- argv=NULL;
-
- if (PROCESS_STATUS_ERROR == status) {
- log_warn(LD_GENERAL, "Failed to start port forwarding helper %s",
- filename);
- time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_FAIL;
- return;
- }
-
- log_info(LD_GENERAL,
- "Started port forwarding helper (%s) with pid '%d'",
- filename, tor_process_get_pid(child_handle));
- }
-
- /* If child is running, read from its stdout and stderr) */
- if (child_handle && PROCESS_STATUS_RUNNING == child_handle->status) {
- /* Read from stdout/stderr and log result */
- retval = 0;
-#ifdef _WIN32
- stderr_status = log_from_handle(child_handle->stderr_pipe, LOG_INFO);
-#else
- stderr_status = log_from_pipe(child_handle->stderr_pipe,
- LOG_INFO, filename, &retval);
-#endif /* defined(_WIN32) */
- if (handle_fw_helper_output(filename, child_handle) < 0) {
- log_warn(LD_GENERAL, "Failed to handle fw helper output.");
- stdout_status = -1;
- retval = -1;
- }
-
- if (retval) {
- /* There was a problem in the child process */
- time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_FAIL;
- }
-
- /* Combine the two statuses in order of severity */
- if (-1 == stdout_status || -1 == stderr_status)
- /* There was a failure */
- retval = -1;
-#ifdef _WIN32
- else if (!child_handle || tor_get_exit_code(child_handle, 0, NULL) !=
- PROCESS_EXIT_RUNNING) {
- /* process has exited or there was an error */
- /* TODO: Do something with the process return value */
- /* TODO: What if the process output something since
- * between log_from_handle and tor_get_exit_code? */
- retval = 1;
- }
-#else /* !(defined(_WIN32)) */
- else if (1 == stdout_status || 1 == stderr_status)
- /* stdout or stderr was closed, the process probably
- * exited. It will be reaped by waitpid() in main.c */
- /* TODO: Do something with the process return value */
- retval = 1;
-#endif /* defined(_WIN32) */
- else
- /* Both are fine */
- retval = 0;
-
- /* If either pipe indicates a failure, act on it */
- if (0 != retval) {
- if (1 == retval) {
- log_info(LD_GENERAL, "Port forwarding helper terminated");
- child_handle->status = PROCESS_STATUS_NOTRUNNING;
- } else {
- log_warn(LD_GENERAL, "Failed to read from port forwarding helper");
- child_handle->status = PROCESS_STATUS_ERROR;
- }
-
- /* TODO: The child might not actually be finished (maybe it failed or
- closed stdout/stderr), so maybe we shouldn't start another? */
- }
- }
-}
-
/** Initialize the insecure RNG rng from a seed value seed. */
void
tor_init_weak_random(tor_weak_rng_t *rng, unsigned seed)
diff --git a/src/common/util.h b/src/common/util.h
index aeed8e823..ae27e5f01 100644
--- a/src/common/util.h
+++ b/src/common/util.h
@@ -414,11 +414,6 @@ void start_daemon(void);
void finish_daemon(const char *desired_cwd);
int write_pidfile(const char *filename);
-/* Port forwarding */
-void tor_check_port_forwarding(const char *filename,
- struct smartlist_t *ports_to_forward,
- time_t now);
-
void tor_disable_spawning_background_processes(void);
typedef struct process_handle_t process_handle_t;
@@ -457,9 +452,7 @@ void set_environment_variable_in_smartlist(struct smartlist_t *env_vars,
void (*free_old)(void*),
int free_p);
-/* Values of process_handle_t.status. PROCESS_STATUS_NOTRUNNING must be
- * 0 because tor_check_port_forwarding depends on this being the initial
- * statue of the static instance of process_handle_t */
+/* Values of process_handle_t.status. */
#define PROCESS_STATUS_NOTRUNNING 0
#define PROCESS_STATUS_RUNNING 1
#define PROCESS_STATUS_ERROR -1
diff --git a/src/or/config.c b/src/or/config.c
index 212c6c6b9..a0890e278 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -492,8 +492,6 @@ static config_var_t option_vars_[] = {
V(TestingSigningKeySlop, INTERVAL, "1 day"),
V(OptimisticData, AUTOBOOL, "auto"),
- V(PortForwarding, BOOL, "0"),
- V(PortForwardingHelper, FILENAME, "tor-fw-helper"),
OBSOLETE("PreferTunneledDirConns"),
V(ProtocolWarnings, BOOL, "0"),
V(PublishServerDescriptor, CSV, "1"),
@@ -3907,15 +3905,6 @@ options_validate(or_options_t *old_options, or_options_t *options,
if (options->KeepalivePeriod < 1)
REJECT("KeepalivePeriod option must be positive.");
- if (options->PortForwarding && options->Sandbox) {
- REJECT("PortForwarding is not compatible with Sandbox; at most one can "
- "be set");
- }
- if (options->PortForwarding && options->NoExec) {
- COMPLAIN("Both PortForwarding and NoExec are set; PortForwarding will "
- "be ignored.");
- }
-
if (ensure_bandwidth_cap(&options->BandwidthRate,
"BandwidthRate", msg) < 0)
return -1;
diff --git a/src/or/main.c b/src/or/main.c
index a0d2ae075..0a208c2a5 100644
--- a/src/or/main.c
+++ b/src/or/main.c
@@ -1336,7 +1336,6 @@ CALLBACK(retry_listeners);
CALLBACK(expire_old_ciruits_serverside);
CALLBACK(check_dns_honesty);
CALLBACK(write_bridge_ns);
-CALLBACK(check_fw_helper_app);
CALLBACK(heartbeat);
CALLBACK(clean_consdiffmgr);
CALLBACK(reset_padding_counts);
@@ -1372,7 +1371,6 @@ static periodic_event_item_t periodic_events[] = {
CALLBACK(expire_old_ciruits_serverside),
CALLBACK(check_dns_honesty),
CALLBACK(write_bridge_ns),
- CALLBACK(check_fw_helper_app),
CALLBACK(heartbeat),
CALLBACK(clean_consdiffmgr),
CALLBACK(reset_padding_counts),
@@ -2186,33 +2184,6 @@ write_bridge_ns_callback(time_t now, const or_options_t *options)
return PERIODIC_EVENT_NO_UPDATE;
}
-/**
- * Periodic callback: poke the tor-fw-helper app if we're using one.
- */
-static int
-check_fw_helper_app_callback(time_t now, const or_options_t *options)
-{
- if (net_is_disabled() ||
- ! server_mode(options) ||
- ! options->PortForwarding ||
- options->NoExec) {
- return PERIODIC_EVENT_NO_UPDATE;
- }
- /* 11. check the port forwarding app */
-
-#define PORT_FORWARDING_CHECK_INTERVAL 5
- smartlist_t *ports_to_forward = get_list_of_ports_to_forward();
- if (ports_to_forward) {
- tor_check_port_forwarding(options->PortForwardingHelper,
- ports_to_forward,
- now);
-
- SMARTLIST_FOREACH(ports_to_forward, char *, cp, tor_free(cp));
- smartlist_free(ports_to_forward);
- }
- return PORT_FORWARDING_CHECK_INTERVAL;
-}
-
static int heartbeat_callback_first_time = 1;
/**
diff --git a/src/or/or.h b/src/or/or.h
index 25fc6cdf3..8cdfe76a9 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -4204,10 +4204,6 @@ typedef struct {
* testing our DNS server. */
int EnforceDistinctSubnets; /**< If true, don't allow multiple routers in the
* same network zone in the same circuit. */
- int PortForwarding; /**< If true, use NAT-PMP or UPnP to automatically
- * forward the DirPort and ORPort on the NAT device */
- char *PortForwardingHelper; /** < Filename or full path of the port
- forwarding helper executable */
int AllowNonRFC953Hostnames; /**< If true, we allow connections to hostnames
* with weird characters. */
/** If true, we try resolving hostnames with weird characters. */
diff --git a/src/test/test_options.c b/src/test/test_options.c
index eaf503439..af349ed01 100644
--- a/src/test/test_options.c
+++ b/src/test/test_options.c
@@ -2421,37 +2421,6 @@ test_options_validate__circuits(void *ignored)
tor_free(msg);
}
-static void
-test_options_validate__port_forwarding(void *ignored)
-{
- (void)ignored;
- int ret;
- char *msg;
- options_test_data_t *tdata = NULL;
-
- free_options_test_data(tdata);
- tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES
- "PortForwarding 1\nSandbox 1\n");
- ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
- tt_int_op(ret, OP_EQ, -1);
- tt_str_op(msg, OP_EQ, "PortForwarding is not compatible with Sandbox;"
- " at most one can be set");
- tor_free(msg);
-
- free_options_test_data(tdata);
- tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES
- "PortForwarding 1\nSandbox 0\n");
- ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
- tt_int_op(ret, OP_EQ, 0);
- tt_assert(!msg);
- tor_free(msg);
-
- done:
- free_options_test_data(tdata);
- policies_free_all();
- tor_free(msg);
-}
-
static void
test_options_validate__tor2web(void *ignored)
{
@@ -4261,7 +4230,6 @@ struct testcase_t options_tests[] = {
LOCAL_VALIDATE_TEST(path_bias),
LOCAL_VALIDATE_TEST(bandwidth),
LOCAL_VALIDATE_TEST(circuits),
- LOCAL_VALIDATE_TEST(port_forwarding),
LOCAL_VALIDATE_TEST(tor2web),
LOCAL_VALIDATE_TEST(rend),
LOCAL_VALIDATE_TEST(single_onion),
diff --git a/src/tools/tor-fw-helper/README b/src/tools/tor-fw-helper/README
deleted file mode 100644
index 6a1ecaa1e..000000000
--- a/src/tools/tor-fw-helper/README
+++ /dev/null
@@ -1,10 +0,0 @@
-
-We no longer recommend the use of this tool. Instead, please use the
-pure-Go version of tor-fw-helper available at
- https://gitweb.torproject.org/tor-fw-helper.git
-
-Why?
-
-The C code here was fine, but frankly: we don't trust the underlying
-libraries. They don't seem to have been written with network security
-in mind, and we have very little faith in their safety.