commit dbfe1a14e44647a4d5f27f8d495f3468208d75dd Author: Nick Mathewson nickm@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 -