[tor-commits] [tor/master] When parsing a multiline controller command, be careful with linebreaks
dgoulet at torproject.org
dgoulet at torproject.org
Tue Apr 30 15:57:51 UTC 2019
commit dbfe1a14e44647a4d5f27f8d495f3468208d75dd
Author: Nick Mathewson <nickm at torproject.org>
Date: Tue Apr 2 10:41:12 2019 -0400
When parsing a multiline controller command, be careful with linebreaks
The first line break in particular was mishandled: it was discarded
if no arguments came before it, which made it impossible to
distinguish arguments from the first line of the body.
To solve this, we need to allocate a copy of the command rather than
using NUL to separate it, since we might have "COMMAND\n" as our input.
Fixes ticket 29984.
---
changes/ticket29984 | 5 ++++
scripts/maint/practracker/exceptions.txt | 2 +-
src/core/mainloop/connection.c | 1 +
src/feature/control/control.c | 45 ++++++++++++++++++++---------
src/feature/control/control.h | 3 +-
src/feature/control/control_cmd.c | 12 ++++----
src/feature/control/control_connection_st.h | 3 +-
7 files changed, 48 insertions(+), 23 deletions(-)
diff --git a/changes/ticket29984 b/changes/ticket29984
new file mode 100644
index 000000000..8631dff27
--- /dev/null
+++ b/changes/ticket29984
@@ -0,0 +1,5 @@
+ o Minor bugfixes (controller protocol):
+ - Teach the controller parser to correctly distinguish an object
+ preceded by an argument list from one without. Previously, it
+ couldn't distinguish an argument list from the first line of a
+ multiline object. Fixes bug 29984; bugfix on 0.2.3.8-alpha.
diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt
index 7d03bf27d..7582395fe 100644
--- a/scripts/maint/practracker/exceptions.txt
+++ b/scripts/maint/practracker/exceptions.txt
@@ -56,7 +56,7 @@ problem function-size /src/app/main/ntmain.c:nt_service_install() 125
problem include-count /src/app/main/shutdown.c 52
problem file-size /src/core/mainloop/connection.c 5558
problem include-count /src/core/mainloop/connection.c 61
-problem function-size /src/core/mainloop/connection.c:connection_free_minimal() 184
+problem function-size /src/core/mainloop/connection.c:connection_free_minimal() 185
problem function-size /src/core/mainloop/connection.c:connection_listener_new() 328
problem function-size /src/core/mainloop/connection.c:connection_handle_listener_read() 161
problem function-size /src/core/mainloop/connection.c:connection_connect_sockaddr() 103
diff --git a/src/core/mainloop/connection.c b/src/core/mainloop/connection.c
index 51c19b4c4..30504e4ed 100644
--- a/src/core/mainloop/connection.c
+++ b/src/core/mainloop/connection.c
@@ -697,6 +697,7 @@ connection_free_minimal(connection_t *conn)
control_connection_t *control_conn = TO_CONTROL_CONN(conn);
tor_free(control_conn->safecookie_client_hash);
tor_free(control_conn->incoming_cmd);
+ tor_free(control_conn->current_cmd);
if (control_conn->ephemeral_onion_services) {
SMARTLIST_FOREACH(control_conn->ephemeral_onion_services, char *, cp, {
memwipe(cp, 0, strlen(cp));
diff --git a/src/feature/control/control.c b/src/feature/control/control.c
index 308f1936b..23ef83ef9 100644
--- a/src/feature/control/control.c
+++ b/src/feature/control/control.c
@@ -276,25 +276,38 @@ peek_connection_has_http_command(connection_t *conn)
}
/**
- * Helper: take a nul-terminated command of given length, and find where
- * the command starts and the argument begins. Separate them with a NUL,
- * and return a pointer to the arguments.
+ * Helper: take a nul-terminated command of given length, and find where the
+ * command starts and the arguments begin. Separate them, allocate a new
+ * string in <b>current_cmd_out</b> for the command, and return a pointer
+ * to the arguments.
**/
STATIC char *
-control_split_incoming_command(char *incoming_cmd, size_t *data_len)
+control_split_incoming_command(char *incoming_cmd,
+ size_t *data_len,
+ char **current_cmd_out)
{
+ const bool is_multiline = *data_len && incoming_cmd[0] == '+';
size_t cmd_len = 0;
while (cmd_len < *data_len
&& !TOR_ISSPACE(incoming_cmd[cmd_len]))
++cmd_len;
- incoming_cmd[cmd_len]='\0';
- char *args = incoming_cmd+cmd_len+1;
- tor_assert(*data_len>cmd_len);
- *data_len -= (cmd_len+1); /* skip the command and NUL we added after it */
- while (TOR_ISSPACE(*args)) {
- ++args;
- --*data_len;
+ *current_cmd_out = tor_memdup_nulterm(incoming_cmd, cmd_len);
+ char *args = incoming_cmd+cmd_len;
+ tor_assert(*data_len>=cmd_len);
+ *data_len -= cmd_len;
+ if (is_multiline) {
+ // Only match horizontal space: any line after the first is data,
+ // not arguments.
+ while ((*args == '\t' || *args == ' ') && *data_len) {
+ ++args;
+ --*data_len;
+ }
+ } else {
+ while (TOR_ISSPACE(*args) && *data_len) {
+ ++args;
+ --*data_len;
+ }
}
return args;
@@ -429,7 +442,11 @@ connection_control_process_inbuf(control_connection_t *conn)
/* Okay, we now have a command sitting on conn->incoming_cmd. See if we
* recognize it.
*/
- args = control_split_incoming_command(conn->incoming_cmd, &data_len);
+ tor_free(conn->current_cmd);
+ args = control_split_incoming_command(conn->incoming_cmd, &data_len,
+ &conn->current_cmd);
+ if (BUG(!conn->current_cmd))
+ return -1;
/* If the connection is already closing, ignore further commands */
if (TO_CONN(conn)->marked_for_close) {
@@ -437,14 +454,14 @@ connection_control_process_inbuf(control_connection_t *conn)
}
/* Otherwise, Quit is always valid. */
- if (!strcasecmp(conn->incoming_cmd, "QUIT")) {
+ if (!strcasecmp(conn->current_cmd, "QUIT")) {
connection_write_str_to_buf("250 closing connection\r\n", conn);
connection_mark_and_flush(TO_CONN(conn));
return 0;
}
if (conn->base_.state == CONTROL_CONN_STATE_NEEDAUTH &&
- !is_valid_initial_command(conn, conn->incoming_cmd)) {
+ !is_valid_initial_command(conn, conn->current_cmd)) {
connection_write_str_to_buf("514 Authentication required.\r\n", conn);
connection_mark_for_close(TO_CONN(conn));
return 0;
diff --git a/src/feature/control/control.h b/src/feature/control/control.h
index 6fc1c40ca..8d3595d2e 100644
--- a/src/feature/control/control.h
+++ b/src/feature/control/control.h
@@ -62,7 +62,8 @@ void set_cached_network_liveness(int liveness);
#ifdef CONTROL_PRIVATE
STATIC char *control_split_incoming_command(char *incoming_cmd,
- size_t *data_len);
+ size_t *data_len,
+ char **current_cmd_out);
#endif
#endif /* !defined(TOR_CONTROL_H) */
diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c
index aa0bef513..53cbf2bd0 100644
--- a/src/feature/control/control_cmd.c
+++ b/src/feature/control/control_cmd.c
@@ -2299,7 +2299,7 @@ handle_control_obsolete(control_connection_t *conn,
{
(void)arg_len;
(void)args;
- char *command = tor_strdup(conn->incoming_cmd);
+ char *command = tor_strdup(conn->current_cmd);
tor_strupper(command);
connection_printf_to_buf(conn, "511 %s is obsolete.\r\n", command);
tor_free(command);
@@ -2490,14 +2490,14 @@ handle_single_control_command(const control_cmd_def_t *def,
control_cmd_args_t *parsed_args;
char *err=NULL;
tor_assert(def->syntax);
- parsed_args = control_cmd_parse_args(conn->incoming_cmd,
+ parsed_args = control_cmd_parse_args(conn->current_cmd,
def->syntax,
cmd_data_len, args,
&err);
if (!parsed_args) {
connection_printf_to_buf(conn,
"512 Bad arguments to %s: %s\r\n",
- conn->incoming_cmd, err?err:"");
+ conn->current_cmd, err?err:"");
tor_free(err);
} else {
if (BUG(err))
@@ -2519,7 +2519,7 @@ handle_single_control_command(const control_cmd_def_t *def,
}
/**
- * Run a given controller command, as selected by the incoming_cmd field of
+ * Run a given controller command, as selected by the current_cmd field of
* <b>conn</b>.
*/
int
@@ -2533,13 +2533,13 @@ handle_control_command(control_connection_t *conn,
for (unsigned i = 0; i < N_CONTROL_COMMANDS; ++i) {
const control_cmd_def_t *def = &CONTROL_COMMANDS[i];
- if (!strcasecmp(conn->incoming_cmd, def->name)) {
+ if (!strcasecmp(conn->current_cmd, def->name)) {
return handle_single_control_command(def, conn, cmd_data_len, args);
}
}
connection_printf_to_buf(conn, "510 Unrecognized command \"%s\"\r\n",
- conn->incoming_cmd);
+ conn->current_cmd);
return 0;
}
diff --git a/src/feature/control/control_connection_st.h b/src/feature/control/control_connection_st.h
index 177a91625..cace6bb36 100644
--- a/src/feature/control/control_connection_st.h
+++ b/src/feature/control/control_connection_st.h
@@ -40,7 +40,8 @@ struct control_connection_t {
/** A control command that we're reading from the inbuf, but which has not
* yet arrived completely. */
char *incoming_cmd;
+ /** The control command that we are currently processing. */
+ char *current_cmd;
};
#endif
-
More information about the tor-commits
mailing list