From d1f5957c4ea82d1622233c0dabd1e761df5200d4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 1 Apr 2019 17:27:08 -0400 Subject: [PATCH 01/24] Improve handling of controller commands Use a table-based lookup to find the right command handler. This will serve as the basement for several future improvements, as we improve the API for parsing commands. --- src/feature/control/control_cmd.c | 298 ++++++++++++++++++++---------- 1 file changed, 198 insertions(+), 100 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 95cf0d561e..4fcfa73105 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -82,7 +82,7 @@ handle_control_resetconf(control_connection_t *conn, uint32_t len, char *body) * reply with a CONFVALUE or an ERROR message */ static int handle_control_getconf(control_connection_t *conn, uint32_t body_len, - const char *body) + char *body) { smartlist_t *questions = smartlist_new(); smartlist_t *answers = smartlist_new(); @@ -2206,110 +2206,208 @@ handle_control_del_onion(control_connection_t *conn, return 0; } +/** + * Called when we get an obsolete command: tell the controller that it is + * obsolete. + */ +static int +handle_control_obsolete(control_connection_t *conn, + uint32_t arg_len, + const char *args) +{ + (void)arg_len; + (void)args; + char *command = tor_strdup(conn->incoming_cmd); + tor_strupper(command); + connection_printf_to_buf(conn, "511 %s is obsolete.\r\n", command); + tor_free(command); + return 0; +} + +/** + * Selects an API to a controller command. See handler_fn_t for the + * possible types. + **/ +typedef enum handler_type_t { + hnd_legacy, + hnd_legacy_mut +} handler_type_t; + +/** + * Union: a function pointer to a handler function for a controller command. + * + * This needs to be a union (rather than just a single pointer) since not + * all controller commands have the same type. + **/ +typedef union handler_fn_t { + /** + * A "legacy" handler takes a command's arguments as a nul-terminated + * string, and their length. It may not change the contents of the + * arguments. If the command is a multiline one, then the arguments may + * extend across multiple lines. + */ + int (*legacy)(control_connection_t *conn, + uint32_t arg_len, + const char *args); + /** + * A "legacy_mut" handler is the same as a "legacy" one, except that it may + * change the contents of the command's arguments -- for example, by + * inserting NULs. It may not deallocate them. + */ + int (*legacy_mut)(control_connection_t *conn, + uint32_t arg_len, + char *args); +} handler_fn_t; + +/** + * Definition for a controller command. + */ +typedef struct control_cmd_def_t { + /** + * The name of the command. If the command is multiline, the name must + * begin with "+". This is not case-sensitive. */ + const char *name; + /** + * Which API to use when calling the handler function. + */ + handler_type_t handler_type; + /** + * A function to execute the command. + */ + handler_fn_t handler; + /** + * Zero or more CMD_FL_* flags, or'd together. + */ + unsigned flags; +} control_cmd_def_t; + +/** + * Indicates that the command's arguments are sensitive, and should be + * memwiped after use. + */ +#define CMD_FL_WIPE (1u<<0) + +/** + * Macro: declare a command with a one-line argument and a given set of + * flags. + **/ +#define ONE_LINE(name, htype, flags) \ + { #name, \ + hnd_ ##htype, \ + { .htype = handle_control_ ##name }, \ + flags \ + } +/** + * Macro: declare a command with a multi-line argument and a given set of + * flags. + **/ +#define MULTLINE(name, htype, flags) \ + { "+"#name, \ + hnd_ ##htype, \ + { .htype = handle_control_ ##name }, \ + flags \ + } +/** + * Macro: declare an obsolete command. (Obsolete commands give a different + * error than non-existent ones.) + **/ +#define OBSOLETE(name) \ + { #name, \ + hnd_legacy, \ + { .legacy = handle_control_obsolete }, \ + 0 \ + } + +/** + * An array defining all the recognized controller commands. + **/ +static const control_cmd_def_t CONTROL_COMMANDS[] = +{ + ONE_LINE(setconf, legacy_mut, 0), + ONE_LINE(resetconf, legacy_mut, 0), + ONE_LINE(getconf, legacy_mut, 0), + MULTLINE(loadconf, legacy, 0), + ONE_LINE(setevents, legacy, 0), + ONE_LINE(authenticate, legacy, 0), + ONE_LINE(saveconf, legacy, 0), + ONE_LINE(signal, legacy, 0), + ONE_LINE(takeownership, legacy, 0), + ONE_LINE(dropownership, legacy, 0), + ONE_LINE(mapaddress, legacy, 0), + ONE_LINE(getinfo, legacy, 0), + ONE_LINE(extendcircuit, legacy, 0), + ONE_LINE(setcircuitpurpose, legacy, 0), + OBSOLETE(setrouterpurpose), + ONE_LINE(attachstream, legacy, 0), + MULTLINE(postdescriptor, legacy, 0), + ONE_LINE(redirectstream, legacy, 0), + ONE_LINE(closestream, legacy, 0), + ONE_LINE(closecircuit, legacy, 0), + ONE_LINE(usefeature, legacy, 0), + ONE_LINE(resolve, legacy, 0), + ONE_LINE(protocolinfo, legacy, 0), + ONE_LINE(authchallenge, legacy, 0), + ONE_LINE(dropguards, legacy, 0), + ONE_LINE(hsfetch, legacy, 0), + MULTLINE(hspost, legacy, 0), + ONE_LINE(add_onion, legacy, CMD_FL_WIPE), + ONE_LINE(del_onion, legacy, CMD_FL_WIPE), +}; + +/** + * The number of entries in CONTROL_COMMANDS. + **/ +static const size_t N_CONTROL_COMMANDS = ARRAY_LENGTH(CONTROL_COMMANDS); + +/** + * Run a single control command, as defined by a control_cmd_def_t, + * with a given set of arguments. + */ +static int +handle_single_control_command(const control_cmd_def_t *def, + control_connection_t *conn, + uint32_t cmd_data_len, + char *args) +{ + int rv = 0; + switch (def->handler_type) { + case hnd_legacy: + if (def->handler.legacy(conn, cmd_data_len, args)) + rv = -1; + break; + case hnd_legacy_mut: + if (def->handler.legacy_mut(conn, cmd_data_len, args)) + rv = -1; + break; + default: + tor_assert_unreached(); + } + + if (def->flags & CMD_FL_WIPE) + memwipe(args, 0, cmd_data_len); + + return rv; +} + +/** + * Run a given controller command, as selected by the incoming_cmd field of + * conn. + */ int handle_control_command(control_connection_t *conn, - uint32_t cmd_data_len, - char *args) + uint32_t cmd_data_len, + char *args) { - /* XXXX Why is this not implemented as a table like the GETINFO - * items are? Even handling the plus signs at the beginnings of - * commands wouldn't be very hard with proper macros. */ - - if (!strcasecmp(conn->incoming_cmd, "SETCONF")) { - if (handle_control_setconf(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "RESETCONF")) { - if (handle_control_resetconf(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "GETCONF")) { - if (handle_control_getconf(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "+LOADCONF")) { - if (handle_control_loadconf(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "SETEVENTS")) { - if (handle_control_setevents(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "AUTHENTICATE")) { - if (handle_control_authenticate(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "SAVECONF")) { - if (handle_control_saveconf(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "SIGNAL")) { - if (handle_control_signal(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "TAKEOWNERSHIP")) { - if (handle_control_takeownership(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "DROPOWNERSHIP")) { - if (handle_control_dropownership(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "MAPADDRESS")) { - if (handle_control_mapaddress(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "GETINFO")) { - if (handle_control_getinfo(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "EXTENDCIRCUIT")) { - if (handle_control_extendcircuit(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "SETCIRCUITPURPOSE")) { - if (handle_control_setcircuitpurpose(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "SETROUTERPURPOSE")) { - connection_write_str_to_buf("511 SETROUTERPURPOSE is obsolete.\r\n", conn); - } else if (!strcasecmp(conn->incoming_cmd, "ATTACHSTREAM")) { - if (handle_control_attachstream(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "+POSTDESCRIPTOR")) { - if (handle_control_postdescriptor(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "REDIRECTSTREAM")) { - if (handle_control_redirectstream(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "CLOSESTREAM")) { - if (handle_control_closestream(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "CLOSECIRCUIT")) { - if (handle_control_closecircuit(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "USEFEATURE")) { - if (handle_control_usefeature(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "RESOLVE")) { - if (handle_control_resolve(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "PROTOCOLINFO")) { - if (handle_control_protocolinfo(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "AUTHCHALLENGE")) { - if (handle_control_authchallenge(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "DROPGUARDS")) { - if (handle_control_dropguards(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "HSFETCH")) { - if (handle_control_hsfetch(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "+HSPOST")) { - if (handle_control_hspost(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "ADD_ONION")) { - int ret = handle_control_add_onion(conn, cmd_data_len, args); - memwipe(args, 0, cmd_data_len); /* Scrub the private key. */ - if (ret) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "DEL_ONION")) { - int ret = handle_control_del_onion(conn, cmd_data_len, args); - memwipe(args, 0, cmd_data_len); /* Scrub the service id/pk. */ - if (ret) - return -1; - } else { - connection_printf_to_buf(conn, "510 Unrecognized command \"%s\"\r\n", - conn->incoming_cmd); + 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)) { + 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); + return 0; } From f3bd0240a6089910f3075a779b32e7aed07338e0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 1 Apr 2019 17:29:46 -0400 Subject: [PATCH 02/24] Add assertions for correct input to handle_control_command. --- src/feature/control/control_cmd.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 4fcfa73105..56202a7ef4 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -2398,6 +2398,10 @@ handle_control_command(control_connection_t *conn, uint32_t cmd_data_len, char *args) { + tor_assert(conn); + tor_assert(args); + tor_assert(args[cmd_data_len] == '\0'); + 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)) { From e9ca904dbfc99ebef567d7bb2c6d87819d0d832c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 1 Apr 2019 17:30:46 -0400 Subject: [PATCH 03/24] Define two more commands as wipe-after-parse. --- src/feature/control/control_cmd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 56202a7ef4..930276b104 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -2328,7 +2328,7 @@ static const control_cmd_def_t CONTROL_COMMANDS[] = ONE_LINE(getconf, legacy_mut, 0), MULTLINE(loadconf, legacy, 0), ONE_LINE(setevents, legacy, 0), - ONE_LINE(authenticate, legacy, 0), + ONE_LINE(authenticate, legacy, CMD_FL_WIPE), ONE_LINE(saveconf, legacy, 0), ONE_LINE(signal, legacy, 0), ONE_LINE(takeownership, legacy, 0), @@ -2346,7 +2346,7 @@ static const control_cmd_def_t CONTROL_COMMANDS[] = ONE_LINE(usefeature, legacy, 0), ONE_LINE(resolve, legacy, 0), ONE_LINE(protocolinfo, legacy, 0), - ONE_LINE(authchallenge, legacy, 0), + ONE_LINE(authchallenge, legacy, CMD_FL_WIPE), ONE_LINE(dropguards, legacy, 0), ONE_LINE(hsfetch, legacy, 0), MULTLINE(hspost, legacy, 0), From de70eebc65d40d50f877b0f82df4d05ce670faa5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 2 Apr 2019 08:17:22 -0400 Subject: [PATCH 04/24] Start on a command-parsing tool for controller commands. There _is_ an underlying logic to these commands, but it isn't wholly uniform, given years of tweaks and changes. Fortunately I think there is a superset that will work. This commit adds a parser for some of the most basic cases -- the ones currently handled by getargs_helper() and some of the object-taking ones. Soon will come initial tests; then I'll start using the parser. After that, I'll expand the parser to handle the other cases that come up in the controller protocol. --- src/core/include.am | 1 + src/feature/control/control_cmd.c | 145 ++++++++++++++++++++-- src/feature/control/control_cmd.h | 39 ++++++ src/feature/control/control_cmd_args_st.h | 44 +++++++ 4 files changed, 221 insertions(+), 8 deletions(-) create mode 100644 src/feature/control/control_cmd_args_st.h diff --git a/src/core/include.am b/src/core/include.am index 9824601725..b927f17a9c 100644 --- a/src/core/include.am +++ b/src/core/include.am @@ -298,6 +298,7 @@ noinst_HEADERS += \ src/feature/control/control.h \ src/feature/control/control_auth.h \ src/feature/control/control_cmd.h \ + src/feature/control/control_cmd_args_st.h \ src/feature/control/control_connection_st.h \ src/feature/control/control_events.h \ src/feature/control/control_fmt.h \ diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 930276b104..aa0bef5135 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -45,6 +45,7 @@ #include "core/or/entry_connection_st.h" #include "core/or/origin_circuit_st.h" #include "core/or/socks_request_st.h" +#include "feature/control/control_cmd_args_st.h" #include "feature/control/control_connection_st.h" #include "feature/nodelist/node_st.h" #include "feature/nodelist/routerinfo_st.h" @@ -60,6 +61,87 @@ static int control_setconf_helper(control_connection_t *conn, uint32_t len, * finished authentication and is accepting commands. */ #define STATE_IS_OPEN(s) ((s) == CONTROL_CONN_STATE_OPEN) +/** + * Release all storage held in args + **/ +void +control_cmd_args_free_(control_cmd_args_t *args) +{ + if (! args) + return; + + if (args->args) { + SMARTLIST_FOREACH(args->args, char *, c, tor_free(c)); + smartlist_free(args->args); + } + tor_free(args->object); + + tor_free(args); +} + +/** + * Helper: parse the arguments to a command according to syntax. On + * success, set *error_out to NULL and return a newly allocated + * control_cmd_args_t. On failure, set *error_out to newly allocated + * error string, and return NULL. + **/ +STATIC control_cmd_args_t * +control_cmd_parse_args(const char *command, + const control_cmd_syntax_t *syntax, + size_t body_len, + const char *body, + char **error_out) +{ + *error_out = NULL; + control_cmd_args_t *result = tor_malloc_zero(sizeof(control_cmd_args_t)); + const char *cmdline; + char *cmdline_alloc = NULL; + + result->command = command; + + const char *eol = memchr(body, '\n', body_len); + if (syntax->want_object) { + if (! eol || (eol+1) == body+body_len) { + *error_out = tor_strdup("Empty body"); + goto err; + } + cmdline_alloc = tor_memdup_nulterm(body, eol-body); + cmdline = cmdline_alloc; + ++eol; + result->object_len = read_escaped_data(eol, (body+body_len)-eol, + &result->object); + } else { + if (eol && (eol+1) != body+body_len) { + *error_out = tor_strdup("Unexpected body"); + goto err; + } + cmdline = body; + } + + result->args = smartlist_new(); + smartlist_split_string(result->args, cmdline, " ", + SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); + size_t n_args = smartlist_len(result->args); + if (n_args < syntax->min_args) { + tor_asprintf(error_out, "Need at least %u argument(s)", + syntax->min_args); + goto err; + } else if (n_args > syntax->max_args) { + tor_asprintf(error_out, "Cannot accept more than %u argument(s)", + syntax->max_args); + goto err; + } + + tor_assert_nonfatal(*error_out == NULL); + goto done; + err: + tor_assert_nonfatal(*error_out != NULL); + control_cmd_args_free(result); + done: + tor_free(cmdline_alloc); + return result; +} + /** Called when we receive a SETCONF message: parse the body and try * to update our configuration. Reply with a DONE or ERROR message. * Modifies the contents of body.*/ @@ -2230,7 +2312,8 @@ handle_control_obsolete(control_connection_t *conn, **/ typedef enum handler_type_t { hnd_legacy, - hnd_legacy_mut + hnd_legacy_mut, + hnd_parsed, } handler_type_t; /** @@ -2257,6 +2340,13 @@ typedef union handler_fn_t { int (*legacy_mut)(control_connection_t *conn, uint32_t arg_len, char *args); + + /** + * A "parsed" handler expects its arguments in a pre-parsed format, in + * an immutable control_cmd_args_t *object. + **/ + int (*parsed)(control_connection_t *conn, + const control_cmd_args_t *args); } handler_fn_t; /** @@ -2279,6 +2369,10 @@ typedef struct control_cmd_def_t { * Zero or more CMD_FL_* flags, or'd together. */ unsigned flags; + /** + * For parsed command: a syntax description. + */ + const control_cmd_syntax_t *syntax; } control_cmd_def_t; /** @@ -2287,16 +2381,27 @@ typedef struct control_cmd_def_t { */ #define CMD_FL_WIPE (1u<<0) -/** - * Macro: declare a command with a one-line argument and a given set of - * flags. +#define SYNTAX_IGNORE { 0, UINT_MAX, false } + +/** Macro: declare a command with a one-line argument, a given set of flags, + * and a syntax definition. **/ -#define ONE_LINE(name, htype, flags) \ +#define ONE_LINE_(name, htype, flags, syntax) \ { #name, \ hnd_ ##htype, \ { .htype = handle_control_ ##name }, \ - flags \ + flags, \ + syntax, \ } + +/** Macro: declare a parsed command with a one-line argument, a given set of + * flags, and a syntax definition. + **/ +#define ONE_LINE(name, htype, flags) \ + ONE_LINE_(name, htype, flags, NULL) +#define ONE_LINE_PARSED(name, flags, syntax) \ + ONE_LINE_(name, parsed, flags, syntax) + /** * Macro: declare a command with a multi-line argument and a given set of * flags. @@ -2305,7 +2410,8 @@ typedef struct control_cmd_def_t { { "+"#name, \ hnd_ ##htype, \ { .htype = handle_control_ ##name }, \ - flags \ + flags, \ + NULL \ } /** * Macro: declare an obsolete command. (Obsolete commands give a different @@ -2315,7 +2421,8 @@ typedef struct control_cmd_def_t { { #name, \ hnd_legacy, \ { .legacy = handle_control_obsolete }, \ - 0 \ + 0, \ + NULL, \ } /** @@ -2379,6 +2486,28 @@ handle_single_control_command(const control_cmd_def_t *def, if (def->handler.legacy_mut(conn, cmd_data_len, args)) rv = -1; break; + case hnd_parsed: { + control_cmd_args_t *parsed_args; + char *err=NULL; + tor_assert(def->syntax); + parsed_args = control_cmd_parse_args(conn->incoming_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:""); + tor_free(err); + } else { + if (BUG(err)) + tor_free(err); + if (def->handler.parsed(conn, parsed_args)) + rv = 0; + control_cmd_args_free(parsed_args); + } + break; + } default: tor_assert_unreached(); } diff --git a/src/feature/control/control_cmd.h b/src/feature/control/control_cmd.h index a417e10da3..1070a9edb7 100644 --- a/src/feature/control/control_cmd.h +++ b/src/feature/control/control_cmd.h @@ -12,11 +12,19 @@ #ifndef TOR_CONTROL_CMD_H #define TOR_CONTROL_CMD_H +#include "lib/malloc/malloc.h" + int handle_control_command(control_connection_t *conn, uint32_t cmd_data_len, char *args); void control_cmd_free_all(void); +typedef struct control_cmd_args_t control_cmd_args_t; +void control_cmd_args_free_(control_cmd_args_t *args); + +#define control_cmd_args_free(v) \ + FREE_AND_NULL(control_cmd_args_t, control_cmd_args_free_, (v)) + #ifdef CONTROL_CMD_PRIVATE #include "lib/crypt_ops/crypto_ed25519.h" @@ -39,6 +47,37 @@ STATIC int add_onion_helper_keyarg(const char *arg, int discard_pk, STATIC rend_authorized_client_t *add_onion_helper_clientauth(const char *arg, int *created, char **err_msg_out); +/** + * Definition for the syntax of a controller command, as parsed by + * control_cmd_parse_args. + * + * WORK IN PROGRESS: This structure is going to get more complex as this + * branch goes on. + **/ +typedef struct control_cmd_syntax_t { + /** + * Lowest number of positional arguments that this command accepts. + * 0 for "it's okay not to have positional arguments." + **/ + unsigned int min_args; + /** + * Highest number of positional arguments that this command accepts. + * UINT_MAX for no limit. + **/ + unsigned int max_args; + /** + * True iff this command wants to be followed by a multiline object. + **/ + bool want_object; +} control_cmd_syntax_t; + +STATIC control_cmd_args_t *control_cmd_parse_args( + const char *command, + const control_cmd_syntax_t *syntax, + size_t body_len, + const char *body, + char **error_out); + #endif /* defined(CONTROL_CMD_PRIVATE) */ #ifdef CONTROL_MODULE_PRIVATE diff --git a/src/feature/control/control_cmd_args_st.h b/src/feature/control/control_cmd_args_st.h new file mode 100644 index 0000000000..9ecc5d750e --- /dev/null +++ b/src/feature/control/control_cmd_args_st.h @@ -0,0 +1,44 @@ +/* Copyright (c) 2001 Matej Pfajfar. + * Copyright (c) 2001-2004, Roger Dingledine. + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2019, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +/** + * \file control_cmd_args_st.h + * \brief Definition for control_cmd_args_t + **/ + +#ifndef TOR_CONTROL_CMD_ST_H +#define TOR_CONTROL_CMD_ST_H + +struct smartlist_t; +struct config_line_t; + +/** + * Parsed arguments for a control command. + * + * WORK IN PROGRESS: This structure is going to get more complex as this + * branch goes on. + **/ +struct control_cmd_args_t { + /** + * The command itself, as provided by the controller. Not owned by this + * structure. + **/ + const char *command; + /** + * Positional arguments to the command. + **/ + struct smartlist_t *args; + /** + * Number of bytes in object; 0 if object is not set. + **/ + size_t object_len; + /** + * A multiline object passed with this command. + **/ + char *object; +}; + +#endif /* !defined(TOR_CONTROL_CMD_ST_H) */ From f18b7dc4731bcb853db92a0faaa4ec03d6ef5586 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 2 Apr 2019 10:29:07 -0400 Subject: [PATCH 05/24] Extract the argument-splitting part of control.c's parser This is preliminary work for fixing 29984; no behavior has changed. --- src/feature/control/control.c | 42 +++++++++++++++++++++++------------ src/feature/control/control.h | 5 +++++ 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/src/feature/control/control.c b/src/feature/control/control.c index 41e21c0a14..308f1936b9 100644 --- a/src/feature/control/control.c +++ b/src/feature/control/control.c @@ -33,6 +33,7 @@ **/ #define CONTROL_MODULE_PRIVATE +#define CONTROL_PRIVATE #include "core/or/or.h" #include "app/config/config.h" @@ -274,6 +275,31 @@ peek_connection_has_http_command(connection_t *conn) return peek_buf_has_http_command(conn->inbuf); } +/** + * 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. + **/ +STATIC char * +control_split_incoming_command(char *incoming_cmd, size_t *data_len) +{ + 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; + } + + return args; +} + static const char CONTROLPORT_IS_NOT_AN_HTTP_PROXY_MSG[] = "HTTP/1.0 501 Tor ControlPort is not an HTTP proxy" "\r\nContent-Type: text/html; charset=iso-8859-1\r\n\r\n" @@ -308,7 +334,6 @@ connection_control_process_inbuf(control_connection_t *conn) { size_t data_len; uint32_t cmd_data_len; - int cmd_len; char *args; tor_assert(conn); @@ -400,22 +425,11 @@ connection_control_process_inbuf(control_connection_t *conn) /* Otherwise, read another line. */ } data_len = conn->incoming_cmd_cur_len; + /* Okay, we now have a command sitting on conn->incoming_cmd. See if we * recognize it. */ - cmd_len = 0; - while ((size_t)cmd_len < data_len - && !TOR_ISSPACE(conn->incoming_cmd[cmd_len])) - ++cmd_len; - - conn->incoming_cmd[cmd_len]='\0'; - args = conn->incoming_cmd+cmd_len+1; - tor_assert(data_len>(size_t)cmd_len); - data_len -= (cmd_len+1); /* skip the command and NUL we added after it */ - while (TOR_ISSPACE(*args)) { - ++args; - --data_len; - } + args = control_split_incoming_command(conn->incoming_cmd, &data_len); /* If the connection is already closing, ignore further commands */ if (TO_CONN(conn)->marked_for_close) { diff --git a/src/feature/control/control.h b/src/feature/control/control.h index 3083837931..6fc1c40cac 100644 --- a/src/feature/control/control.h +++ b/src/feature/control/control.h @@ -60,4 +60,9 @@ int get_cached_network_liveness(void); void set_cached_network_liveness(int liveness); #endif /* defined(CONTROL_MODULE_PRIVATE) */ +#ifdef CONTROL_PRIVATE +STATIC char *control_split_incoming_command(char *incoming_cmd, + size_t *data_len); +#endif + #endif /* !defined(TOR_CONTROL_H) */ From dbfe1a14e44647a4d5f27f8d495f3468208d75dd Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 2 Apr 2019 10:41:12 -0400 Subject: [PATCH 06/24] 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(-) create mode 100644 changes/ticket29984 diff --git a/changes/ticket29984 b/changes/ticket29984 new file mode 100644 index 0000000000..8631dff27b --- /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 7d03bf27d6..7582395fea 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 51c19b4c4c..30504e4edb 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 308f1936b9..23ef83ef95 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 current_cmd_out 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 6fc1c40cac..8d3595d2ed 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 aa0bef5135..53cbf2bd0f 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 * conn. */ 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 177a916257..cace6bb36f 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 - From cbd1a7e0534d6f5e693c65c8a8b239e35e6f3b2c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 2 Apr 2019 11:08:22 -0400 Subject: [PATCH 07/24] Unit tests for current control-command parser logic --- src/test/test_controller.c | 134 +++++++++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) diff --git a/src/test/test_controller.c b/src/test/test_controller.c index f3af6d2ec0..ca853367ae 100644 --- a/src/test/test_controller.c +++ b/src/test/test_controller.c @@ -20,10 +20,138 @@ #include "lib/net/resolve.h" #include "feature/control/control_connection_st.h" +#include "feature/control/control_cmd_args_st.h" #include "feature/dirclient/download_status_st.h" #include "feature/nodelist/microdesc_st.h" #include "feature/nodelist/node_st.h" +typedef struct { + const char *input; + const char *expected_parse; + const char *expected_error; +} parser_testcase_t; + +typedef struct { + const control_cmd_syntax_t *syntax; + size_t n_testcases; + const parser_testcase_t *testcases; +} parse_test_params_t; + +static char * +control_cmd_dump_args(const control_cmd_args_t *result) +{ + buf_t *buf = buf_new(); + buf_add_string(buf, "{ args=["); + if (result->args) { + if (smartlist_len(result->args)) { + buf_add_string(buf, " "); + } + SMARTLIST_FOREACH_BEGIN(result->args, const char *, s) { + const bool last = (s_sl_idx == smartlist_len(result->args)-1); + buf_add_printf(buf, "%s%s ", + escaped(s), + last ? "" : ","); + } SMARTLIST_FOREACH_END(s); + } + buf_add_string(buf, "]"); + if (result->object) { + buf_add_string(buf, ", obj="); + buf_add_string(buf, escaped(result->object)); + } + buf_add_string(buf, " }"); + + char *encoded = buf_extract(buf, NULL); + buf_free(buf); + return encoded; +} + +static void +test_controller_parse_cmd(void *arg) +{ + const parse_test_params_t *params = arg; + control_cmd_args_t *result = NULL; + char *error = NULL; + char *encoded = NULL; + + for (size_t i = 0; i < params->n_testcases; ++i) { + const parser_testcase_t *t = ¶ms->testcases[i]; + result = control_cmd_parse_args("EXAMPLE", + params->syntax, + strlen(t->input), + t->input, + &error); + // A valid test should expect exactly one parse or error. + tt_int_op((t->expected_parse == NULL), OP_NE, + (t->expected_error == NULL)); + // We get a result or an error, not both. + tt_int_op((result == NULL), OP_EQ, (error != NULL)); + // We got the one we expected. + tt_int_op((result == NULL), OP_EQ, (t->expected_parse == NULL)); + + if (result) { + encoded = control_cmd_dump_args(result); + tt_str_op(encoded, OP_EQ, t->expected_parse); + } else { + tt_str_op(error, OP_EQ, t->expected_error); + } + + tor_free(error); + tor_free(encoded); + control_cmd_args_free(result); + } + + done: + tor_free(error); + tor_free(encoded); + control_cmd_args_free(result); +} + +#define OK(inp, out) \ + { inp "\r\n", out, NULL } +#define ERR(inp, err) \ + { inp "\r\n", NULL, err } + +#define TESTPARAMS(syntax, array) \ + { &syntax, \ + ARRAY_LENGTH(array), \ + array } + +static const parser_testcase_t one_to_three_tests[] = { + ERR("", "Need at least 1 argument(s)"), + ERR(" \t", "Need at least 1 argument(s)"), + OK("hello", "{ args=[ \"hello\" ] }"), + OK("hello world", "{ args=[ \"hello\", \"world\" ] }"), + OK("hello world", "{ args=[ \"hello\", \"world\" ] }"), + OK(" hello world", "{ args=[ \"hello\", \"world\" ] }"), + OK(" hello world ", "{ args=[ \"hello\", \"world\" ] }"), + OK("hello there world", "{ args=[ \"hello\", \"there\", \"world\" ] }"), + ERR("why hello there world", "Cannot accept more than 3 argument(s)"), + ERR("hello\r\nworld.\r\n.", "Unexpected body"), +}; + +static const control_cmd_syntax_t one_to_three_syntax = { + .min_args=1, .max_args=3 +}; + +static const parse_test_params_t parse_one_to_three_params = + TESTPARAMS( one_to_three_syntax, one_to_three_tests ); + +static const parser_testcase_t no_args_one_obj_tests[] = { + ERR("Hi there!\r\n.", "Cannot accept more than 0 argument(s)"), + ERR("", "Empty body"), + OK("\r\n", "{ args=[], obj=\"\\n\" }"), + OK("\r\nHello world\r\n", "{ args=[], obj=\"Hello world\\n\\n\" }"), + OK("\r\nHello\r\nworld\r\n", "{ args=[], obj=\"Hello\\nworld\\n\\n\" }"), + OK("\r\nHello\r\n..\r\nworld\r\n", + "{ args=[], obj=\"Hello\\n.\\nworld\\n\\n\" }"), +}; +static const control_cmd_syntax_t no_args_one_obj_syntax = { + .min_args=0, .max_args=0, + .want_object=true, +}; +static const parse_test_params_t parse_no_args_one_obj_params = + TESTPARAMS( no_args_one_obj_syntax, no_args_one_obj_tests ); + static void test_add_onion_helper_keyarg_v3(void *arg) { @@ -1617,7 +1745,13 @@ test_getinfo_md_all(void *arg) return; } +#define PARSER_TEST(type) \ + { "parse/" #type, test_controller_parse_cmd, 0, &passthrough_setup, \ + (void*)&parse_ ## type ## _params } + struct testcase_t controller_tests[] = { + PARSER_TEST(one_to_three), + PARSER_TEST(no_args_one_obj), { "add_onion_helper_keyarg_v2", test_add_onion_helper_keyarg_v2, 0, NULL, NULL }, { "add_onion_helper_keyarg_v3", test_add_onion_helper_keyarg_v3, 0, From 01b07c548b93bcc58adac612a02c69dcb4b63b28 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 2 Apr 2019 19:00:23 -0400 Subject: [PATCH 08/24] Use parsing code for the simpler controller commands. (This should be all of the command that work nicely with positional arguments only.) Some of these commands should probably treat extra arguments as incorrect, but for now I'm trying to be careful not to break any existing users. --- scripts/maint/practracker/exceptions.txt | 4 +- src/feature/control/control_cmd.c | 239 +++++++++++------------ src/feature/control/control_cmd.h | 44 ++--- src/feature/control/control_getinfo.c | 16 +- src/feature/control/control_getinfo.h | 8 +- 5 files changed, 154 insertions(+), 157 deletions(-) diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index 7582395fea..438c1c582a 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -54,7 +54,7 @@ problem function-size /src/app/main/main.c:sandbox_init_filter() 291 problem function-size /src/app/main/main.c:run_tor_main_loop() 105 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 file-size /src/core/mainloop/connection.c 5559 problem include-count /src/core/mainloop/connection.c 61 problem function-size /src/core/mainloop/connection.c:connection_free_minimal() 185 problem function-size /src/core/mainloop/connection.c:connection_listener_new() 328 @@ -152,7 +152,7 @@ problem function-size /src/feature/control/control_cmd.c:handle_control_add_onio problem function-size /src/feature/control/control_cmd.c:add_onion_helper_keyarg() 125 problem function-size /src/feature/control/control_cmd.c:handle_control_command() 104 problem function-size /src/feature/control/control_events.c:control_event_stream_status() 119 -problem include-count /src/feature/control/control_getinfo.c 52 +problem include-count /src/feature/control/control_getinfo.c 53 problem function-size /src/feature/control/control_getinfo.c:getinfo_helper_misc() 109 problem function-size /src/feature/control/control_getinfo.c:getinfo_helper_dir() 304 problem function-size /src/feature/control/control_getinfo.c:getinfo_helper_events() 236 diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 53cbf2bd0f..f457e9fa54 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -160,13 +160,17 @@ handle_control_resetconf(control_connection_t *conn, uint32_t len, char *body) return control_setconf_helper(conn, len, body, 1); } +static const control_cmd_syntax_t getconf_syntax = { + .max_args=UINT_MAX +}; + /** Called when we receive a GETCONF message. Parse the request, and * reply with a CONFVALUE or an ERROR message */ static int -handle_control_getconf(control_connection_t *conn, uint32_t body_len, - char *body) +handle_control_getconf(control_connection_t *conn, + const control_cmd_args_t *args) { - smartlist_t *questions = smartlist_new(); + const smartlist_t *questions = args->args; smartlist_t *answers = smartlist_new(); smartlist_t *unrecognized = smartlist_new(); char *msg = NULL; @@ -174,9 +178,6 @@ handle_control_getconf(control_connection_t *conn, uint32_t body_len, const or_options_t *options = get_options(); int i, len; - (void) body_len; /* body is NUL-terminated; so we can ignore len. */ - smartlist_split_string(questions, body, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); SMARTLIST_FOREACH_BEGIN(questions, const char *, q) { if (!option_is_recognized(q)) { smartlist_add(unrecognized, (char*) q); @@ -221,8 +222,6 @@ handle_control_getconf(control_connection_t *conn, uint32_t body_len, SMARTLIST_FOREACH(answers, char *, cp, tor_free(cp)); smartlist_free(answers); - SMARTLIST_FOREACH(questions, char *, cp, tor_free(cp)); - smartlist_free(questions); smartlist_free(unrecognized); tor_free(msg); @@ -230,17 +229,21 @@ handle_control_getconf(control_connection_t *conn, uint32_t body_len, return 0; } +static const control_cmd_syntax_t loadconf_syntax = { + .want_object = true +}; + /** Called when we get a +LOADCONF message. */ static int -handle_control_loadconf(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_loadconf(control_connection_t *conn, + const control_cmd_args_t *args) { setopt_err_t retval; char *errstring = NULL; const char *msg = NULL; - (void) len; - retval = options_init_from_string(NULL, body, CMD_RUN_TOR, NULL, &errstring); + retval = options_init_from_string(NULL, args->object, + CMD_RUN_TOR, NULL, &errstring); if (retval != SETOPT_OK) log_warn(LD_CONTROL, @@ -276,20 +279,20 @@ handle_control_loadconf(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t setevents_syntax = { + .max_args = UINT_MAX +}; + /** Called when we get a SETEVENTS message: update conn->event_mask, * and reply with DONE or ERROR. */ static int -handle_control_setevents(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_setevents(control_connection_t *conn, + const control_cmd_args_t *args) { int event_code; event_mask_t event_mask = 0; - smartlist_t *events = smartlist_new(); + const smartlist_t *events = args->args; - (void) len; - - smartlist_split_string(events, body, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); SMARTLIST_FOREACH_BEGIN(events, const char *, ev) { if (!strcasecmp(ev, "EXTENDED") || @@ -311,16 +314,12 @@ handle_control_setevents(control_connection_t *conn, uint32_t len, if (event_code == -1) { connection_printf_to_buf(conn, "552 Unrecognized event \"%s\"\r\n", ev); - SMARTLIST_FOREACH(events, char *, e, tor_free(e)); - smartlist_free(events); return 0; } } event_mask |= (((event_mask_t)1) << event_code); } SMARTLIST_FOREACH_END(ev); - SMARTLIST_FOREACH(events, char *, e, tor_free(e)); - smartlist_free(events); conn->event_mask = event_mask; @@ -348,23 +347,23 @@ handle_control_saveconf(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t signal_syntax = { + .min_args = 1, + .max_args = 1, +}; + /** Called when we get a SIGNAL command. React to the provided signal, and * report success or failure. (If the signal results in a shutdown, success * may not be reported.) */ static int -handle_control_signal(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_signal(control_connection_t *conn, + const control_cmd_args_t *args) { int sig = -1; int i; - int n = 0; - char *s; - (void) len; - - while (body[n] && ! TOR_ISSPACE(body[n])) - ++n; - s = tor_strndup(body, n); + tor_assert(smartlist_len(args->args) == 1); + const char *s = smartlist_get(args->args, 0); for (i = 0; signal_table[i].signal_name != NULL; ++i) { if (!strcasecmp(s, signal_table[i].signal_name)) { @@ -376,7 +375,6 @@ handle_control_signal(control_connection_t *conn, uint32_t len, if (sig < 0) connection_printf_to_buf(conn, "552 Unrecognized signal code \"%s\"\r\n", s); - tor_free(s); if (sig < 0) return 0; @@ -390,15 +388,18 @@ handle_control_signal(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t takeownership_syntax = { + .max_args = UINT_MAX, // This should probably become zero. XXXXX +}; + /** Called when we get a TAKEOWNERSHIP command. Mark this connection * as an owning connection, so that we will exit if the connection * closes. */ static int -handle_control_takeownership(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_takeownership(control_connection_t *conn, + const control_cmd_args_t *args) { - (void)len; - (void)body; + (void)args; conn->is_owning_control_connection = 1; @@ -410,15 +411,18 @@ handle_control_takeownership(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t dropownership_syntax = { + .max_args = UINT_MAX, // This should probably become zero. XXXXX +}; + /** Called when we get a DROPOWNERSHIP command. Mark this connection * as a non-owning connection, so that we will not exit if the connection * closes. */ static int -handle_control_dropownership(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_dropownership(control_connection_t *conn, + const control_cmd_args_t *args) { - (void)len; - (void)body; + (void)args; conn->is_owning_control_connection = 0; @@ -1099,21 +1103,21 @@ handle_control_postdescriptor(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t redirectstream_syntax = { + .min_args = 2, + .max_args = UINT_MAX, // XXX should be 3. +}; + /** Called when we receive a REDIRECTSTERAM command. Try to change the target * address of the named AP stream, and report success or failure. */ static int -handle_control_redirectstream(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_redirectstream(control_connection_t *conn, + const control_cmd_args_t *cmd_args) { entry_connection_t *ap_conn = NULL; char *new_addr = NULL; uint16_t new_port = 0; - smartlist_t *args; - (void) len; - - args = getargs_helper("REDIRECTSTREAM", conn, body, 2, -1); - if (!args) - return 0; + const smartlist_t *args = cmd_args->args; if (!(ap_conn = get_stream(smartlist_get(args, 0))) || !ap_conn->socks_request) { @@ -1133,8 +1137,6 @@ handle_control_redirectstream(control_connection_t *conn, uint32_t len, } } - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); if (!new_addr) return 0; @@ -1147,23 +1149,26 @@ handle_control_redirectstream(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t closestream_syntax = { + .min_args = 2, + .max_args = UINT_MAX, /* XXXX This is the original behavior, but + * maybe we should change the spec. */ +}; + /** Called when we get a CLOSESTREAM command; try to close the named stream * and report success or failure. */ static int -handle_control_closestream(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_closestream(control_connection_t *conn, + const control_cmd_args_t *cmd_args) { entry_connection_t *ap_conn=NULL; uint8_t reason=0; - smartlist_t *args; int ok; - (void) len; + const smartlist_t *args = cmd_args->args; - args = getargs_helper("CLOSESTREAM", conn, body, 2, -1); - if (!args) - return 0; + tor_assert(smartlist_len(args) >= 2); - else if (!(ap_conn = get_stream(smartlist_get(args, 0)))) + if (!(ap_conn = get_stream(smartlist_get(args, 0)))) connection_printf_to_buf(conn, "552 Unknown stream \"%s\"\r\n", (char*)smartlist_get(args, 0)); else { @@ -1175,8 +1180,6 @@ handle_control_closestream(control_connection_t *conn, uint32_t len, ap_conn = NULL; } } - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); if (!ap_conn) return 0; @@ -1269,19 +1272,20 @@ handle_control_resolve(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t protocolinfo_syntax = { + .max_args = UINT_MAX +}; + /** Called when we get a PROTOCOLINFO command: send back a reply. */ static int -handle_control_protocolinfo(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_protocolinfo(control_connection_t *conn, + const control_cmd_args_t *cmd_args) { const char *bad_arg = NULL; - smartlist_t *args; - (void)len; + const smartlist_t *args = cmd_args->args; conn->have_sent_protocolinfo = 1; - args = smartlist_new(); - smartlist_split_string(args, body, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); + SMARTLIST_FOREACH(args, const char *, arg, { int ok; tor_parse_long(arg, 10, 0, LONG_MAX, &ok, NULL); @@ -1337,24 +1341,21 @@ handle_control_protocolinfo(control_connection_t *conn, uint32_t len, tor_free(esc_cfile); } done: - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); return 0; } +static const control_cmd_syntax_t usefeature_syntax = { + .max_args = UINT_MAX +}; + /** Called when we get a USEFEATURE command: parse the feature list, and * set up the control_connection's options properly. */ static int handle_control_usefeature(control_connection_t *conn, - uint32_t len, - const char *body) + const control_cmd_args_t *cmd_args) { - smartlist_t *args; + const smartlist_t *args = cmd_args->args; int bad = 0; - (void) len; /* body is nul-terminated; it's safe to ignore the length */ - args = smartlist_new(); - smartlist_split_string(args, body, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); SMARTLIST_FOREACH_BEGIN(args, const char *, arg) { if (!strcasecmp(arg, "VERBOSE_NAMES")) ; @@ -1372,22 +1373,19 @@ handle_control_usefeature(control_connection_t *conn, send_control_done(conn); } - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); return 0; } +static const control_cmd_syntax_t dropguards_syntax = { + .max_args = 0, +}; + /** Implementation for the DROPGUARDS command. */ static int handle_control_dropguards(control_connection_t *conn, - uint32_t len, - const char *body) + const control_cmd_args_t *args) { - smartlist_t *args; - (void) len; /* body is nul-terminated; it's safe to ignore the length */ - args = smartlist_new(); - smartlist_split_string(args, body, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); + (void) args; /* We don't take arguments. */ static int have_warned = 0; if (! have_warned) { @@ -1397,15 +1395,9 @@ handle_control_dropguards(control_connection_t *conn, have_warned = 1; } - if (smartlist_len(args)) { - connection_printf_to_buf(conn, "512 Too many arguments to DROPGUARDS\r\n"); - } else { - remove_all_entry_guards(); - send_control_done(conn); - } + remove_all_entry_guards(); + send_control_done(conn); - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); return 0; } @@ -2202,19 +2194,19 @@ add_onion_helper_clientauth(const char *arg, int *created, char **err_msg) return client; } +static const control_cmd_syntax_t del_onion_syntax = { + .min_args = 1, .max_args = 1, +}; + /** Called when we get a DEL_ONION command; parse the body, and remove * the existing ephemeral Onion Service. */ static int handle_control_del_onion(control_connection_t *conn, - uint32_t len, - const char *body) + const control_cmd_args_t *cmd_args) { int hs_version = 0; - smartlist_t *args; - (void) len; /* body is nul-terminated; it's safe to ignore the length */ - args = getargs_helper("DEL_ONION", conn, body, 1, 1); - if (!args) - return 0; + smartlist_t *args = cmd_args->args; + tor_assert(smartlist_len(args) == 1); const char *service_id = smartlist_get(args, 0); if (rend_valid_v2_service_id(service_id)) { @@ -2280,11 +2272,6 @@ handle_control_del_onion(control_connection_t *conn, } out: - SMARTLIST_FOREACH(args, char *, cp, { - memwipe(cp, 0, strlen(cp)); - tor_free(cp); - }); - smartlist_free(args); return 0; } @@ -2399,20 +2386,26 @@ typedef struct control_cmd_def_t { **/ #define ONE_LINE(name, htype, flags) \ ONE_LINE_(name, htype, flags, NULL) -#define ONE_LINE_PARSED(name, flags, syntax) \ - ONE_LINE_(name, parsed, flags, syntax) +#define ONE_LINE_PARSED(name, flags) \ + ONE_LINE_(name, parsed, flags, &name ##_syntax) /** * Macro: declare a command with a multi-line argument and a given set of * flags. **/ -#define MULTLINE(name, htype, flags) \ +#define MULTLINE_(name, htype, flags, syntax) \ { "+"#name, \ hnd_ ##htype, \ { .htype = handle_control_ ##name }, \ flags, \ - NULL \ + syntax \ } + +#define MULTLINE(name, htype, flags) \ + MULTLINE_(name, htype, flags, NULL) +#define MULTLINE_PARSED(name, flags) \ + MULTLINE_(name, parsed, flags, &name##_syntax) + /** * Macro: declare an obsolete command. (Obsolete commands give a different * error than non-existent ones.) @@ -2432,33 +2425,33 @@ static const control_cmd_def_t CONTROL_COMMANDS[] = { ONE_LINE(setconf, legacy_mut, 0), ONE_LINE(resetconf, legacy_mut, 0), - ONE_LINE(getconf, legacy_mut, 0), - MULTLINE(loadconf, legacy, 0), - ONE_LINE(setevents, legacy, 0), + ONE_LINE_PARSED(getconf, 0), + MULTLINE_PARSED(loadconf, 0), + ONE_LINE_PARSED(setevents, 0), ONE_LINE(authenticate, legacy, CMD_FL_WIPE), ONE_LINE(saveconf, legacy, 0), - ONE_LINE(signal, legacy, 0), - ONE_LINE(takeownership, legacy, 0), - ONE_LINE(dropownership, legacy, 0), + ONE_LINE_PARSED(signal, 0), + ONE_LINE_PARSED(takeownership, 0), + ONE_LINE_PARSED(dropownership, 0), ONE_LINE(mapaddress, legacy, 0), - ONE_LINE(getinfo, legacy, 0), + ONE_LINE_PARSED(getinfo, 0), ONE_LINE(extendcircuit, legacy, 0), ONE_LINE(setcircuitpurpose, legacy, 0), OBSOLETE(setrouterpurpose), ONE_LINE(attachstream, legacy, 0), MULTLINE(postdescriptor, legacy, 0), - ONE_LINE(redirectstream, legacy, 0), - ONE_LINE(closestream, legacy, 0), + ONE_LINE_PARSED(redirectstream, 0), + ONE_LINE_PARSED(closestream, 0), ONE_LINE(closecircuit, legacy, 0), - ONE_LINE(usefeature, legacy, 0), + ONE_LINE_PARSED(usefeature, 0), ONE_LINE(resolve, legacy, 0), - ONE_LINE(protocolinfo, legacy, 0), + ONE_LINE_PARSED(protocolinfo, 0), ONE_LINE(authchallenge, legacy, CMD_FL_WIPE), - ONE_LINE(dropguards, legacy, 0), + ONE_LINE_PARSED(dropguards, 0), ONE_LINE(hsfetch, legacy, 0), MULTLINE(hspost, legacy, 0), ONE_LINE(add_onion, legacy, CMD_FL_WIPE), - ONE_LINE(del_onion, legacy, CMD_FL_WIPE), + ONE_LINE_PARSED(del_onion, CMD_FL_WIPE), }; /** diff --git a/src/feature/control/control_cmd.h b/src/feature/control/control_cmd.h index 1070a9edb7..801bf43709 100644 --- a/src/feature/control/control_cmd.h +++ b/src/feature/control/control_cmd.h @@ -25,28 +25,6 @@ void control_cmd_args_free_(control_cmd_args_t *args); #define control_cmd_args_free(v) \ FREE_AND_NULL(control_cmd_args_t, control_cmd_args_free_, (v)) -#ifdef CONTROL_CMD_PRIVATE -#include "lib/crypt_ops/crypto_ed25519.h" - -/* ADD_ONION secret key to create an ephemeral service. The command supports - * multiple versions so this union stores the key and passes it to the HS - * subsystem depending on the requested version. */ -typedef union add_onion_secret_key_t { - /* Hidden service v2 secret key. */ - crypto_pk_t *v2; - /* Hidden service v3 secret key. */ - ed25519_secret_key_t *v3; -} add_onion_secret_key_t; - -STATIC int add_onion_helper_keyarg(const char *arg, int discard_pk, - const char **key_new_alg_out, - char **key_new_blob_out, - add_onion_secret_key_t *decoded_key, - int *hs_version, char **err_msg_out); - -STATIC rend_authorized_client_t *add_onion_helper_clientauth(const char *arg, - int *created, char **err_msg_out); - /** * Definition for the syntax of a controller command, as parsed by * control_cmd_parse_args. @@ -71,6 +49,28 @@ typedef struct control_cmd_syntax_t { bool want_object; } control_cmd_syntax_t; +#ifdef CONTROL_CMD_PRIVATE +#include "lib/crypt_ops/crypto_ed25519.h" + +/* ADD_ONION secret key to create an ephemeral service. The command supports + * multiple versions so this union stores the key and passes it to the HS + * subsystem depending on the requested version. */ +typedef union add_onion_secret_key_t { + /* Hidden service v2 secret key. */ + crypto_pk_t *v2; + /* Hidden service v3 secret key. */ + ed25519_secret_key_t *v3; +} add_onion_secret_key_t; + +STATIC int add_onion_helper_keyarg(const char *arg, int discard_pk, + const char **key_new_alg_out, + char **key_new_blob_out, + add_onion_secret_key_t *decoded_key, + int *hs_version, char **err_msg_out); + +STATIC rend_authorized_client_t *add_onion_helper_clientauth(const char *arg, + int *created, char **err_msg_out); + STATIC control_cmd_args_t *control_cmd_parse_args( const char *command, const control_cmd_syntax_t *syntax, diff --git a/src/feature/control/control_getinfo.c b/src/feature/control/control_getinfo.c index a7a85f2fdf..5c6a0d4aa2 100644 --- a/src/feature/control/control_getinfo.c +++ b/src/feature/control/control_getinfo.c @@ -55,6 +55,7 @@ #include "core/or/origin_circuit_st.h" #include "core/or/socks_request_st.h" #include "feature/control/control_connection_st.h" +#include "feature/control/control_cmd_args_st.h" #include "feature/dircache/cached_dir_st.h" #include "feature/nodelist/extrainfo_st.h" #include "feature/nodelist/microdesc_st.h" @@ -1584,21 +1585,22 @@ handle_getinfo_helper(control_connection_t *control_conn, return 0; /* unrecognized */ } +const control_cmd_syntax_t getinfo_syntax = { + .max_args = UINT_MAX, +}; + /** Called when we receive a GETINFO command. Try to fetch all requested * information, and reply with information or error message. */ int -handle_control_getinfo(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_getinfo(control_connection_t *conn, + const control_cmd_args_t *args) { - smartlist_t *questions = smartlist_new(); + const smartlist_t *questions = args->args; smartlist_t *answers = smartlist_new(); smartlist_t *unrecognized = smartlist_new(); char *ans = NULL; int i; - (void) len; /* body is NUL-terminated, so it's safe to ignore the length. */ - smartlist_split_string(questions, body, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); SMARTLIST_FOREACH_BEGIN(questions, const char *, q) { const char *errmsg = NULL; @@ -1653,8 +1655,6 @@ handle_control_getinfo(control_connection_t *conn, uint32_t len, done: SMARTLIST_FOREACH(answers, char *, cp, tor_free(cp)); smartlist_free(answers); - SMARTLIST_FOREACH(questions, char *, cp, tor_free(cp)); - smartlist_free(questions); SMARTLIST_FOREACH(unrecognized, char *, cp, tor_free(cp)); smartlist_free(unrecognized); diff --git a/src/feature/control/control_getinfo.h b/src/feature/control/control_getinfo.h index d5a2feb3e0..2d56586f6d 100644 --- a/src/feature/control/control_getinfo.h +++ b/src/feature/control/control_getinfo.h @@ -12,8 +12,12 @@ #ifndef TOR_CONTROL_GETINFO_H #define TOR_CONTROL_GETINFO_H -int handle_control_getinfo(control_connection_t *conn, uint32_t len, - const char *body); +struct control_cmd_syntax_t; +struct control_cmd_args_t; +extern const struct control_cmd_syntax_t getinfo_syntax; + +int handle_control_getinfo(control_connection_t *conn, + const struct control_cmd_args_t *args); #ifdef CONTROL_GETINFO_PRIVATE STATIC int getinfo_helper_onions( From 73df91bbb55498b05faae16b49ab49545fdffa8f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 5 Apr 2019 15:29:37 -0400 Subject: [PATCH 09/24] kvline: handle empty alues as well as empty keys The two options are mutually exclusive, since otherwise an entry like "Foo" would be ambiguous. We want to have the ability to treat entries like this as keys, though, since some controller commands interpret them as flags. --- src/lib/encoding/kvline.c | 48 +++++++++++++++++++++++++++++----- src/lib/encoding/kvline.h | 1 + src/test/test_config.c | 55 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 7 deletions(-) diff --git a/src/lib/encoding/kvline.c b/src/lib/encoding/kvline.c index 307adc3f12..806f9d3db0 100644 --- a/src/lib/encoding/kvline.c +++ b/src/lib/encoding/kvline.c @@ -53,6 +53,15 @@ line_has_no_key(const config_line_t *line) return line->key == NULL || strlen(line->key) == 0; } +/** + * Return true iff the value in line is not set. + **/ +static bool +line_has_no_val(const config_line_t *line) +{ + return line->value == NULL || strlen(line->value) == 0; +} + /** * Return true iff the all the lines in line can be encoded * using flags. @@ -98,6 +107,10 @@ kvline_can_encode_lines(const config_line_t *line, unsigned flags) * If KV_OMIT_KEYS is set in flags, then pairs with empty keys are * allowed, and are encoded as 'Value'. Otherwise, such pairs are not * allowed. + * + * If KV_OMIT_VALS is set in flags, then an empty value is + * encoded as 'Key', not as 'Key=' or 'Key=""'. Mutually exclusive with + * KV_OMIT_KEYS. */ char * kvline_encode(const config_line_t *line, @@ -106,6 +119,9 @@ kvline_encode(const config_line_t *line, if (!kvline_can_encode_lines(line, flags)) return NULL; + tor_assert((flags & (KV_OMIT_KEYS|KV_OMIT_VALS)) != + (KV_OMIT_KEYS|KV_OMIT_VALS)); + smartlist_t *elements = smartlist_new(); for (; line; line = line->next) { @@ -126,7 +142,10 @@ kvline_encode(const config_line_t *line, } } - if (esc) { + if ((flags & KV_OMIT_VALS) && line_has_no_val(line)) { + eq = ""; + v = ""; + } else if (esc) { tmp = esc_for_log(line->value); v = tmp; } else { @@ -155,13 +174,21 @@ kvline_encode(const config_line_t *line, * * If KV_OMIT_KEYS is set in flags, then values without keys are * allowed. Otherwise, such values are not allowed. + * + * If KV_OMIT_VALS is set in flags, then keys without values are + * allowed. Otherwise, such keys are not allowed. Mutually exclusive with + * KV_OMIT_KEYS. */ config_line_t * kvline_parse(const char *line, unsigned flags) { + tor_assert((flags & (KV_OMIT_KEYS|KV_OMIT_VALS)) != + (KV_OMIT_KEYS|KV_OMIT_VALS)); + const char *cp = line, *cplast = NULL; - bool omit_keys = (flags & KV_OMIT_KEYS) != 0; - bool quoted = (flags & KV_QUOTED) != 0; + const bool omit_keys = (flags & KV_OMIT_KEYS) != 0; + const bool omit_vals = (flags & KV_OMIT_VALS) != 0; + const bool quoted = (flags & KV_QUOTED) != 0; config_line_t *result = NULL; config_line_t **next_line = &result; @@ -171,27 +198,33 @@ kvline_parse(const char *line, unsigned flags) while (*cp) { key = val = NULL; + /* skip all spaces */ { size_t idx = strspn(cp, " \t\r\v\n"); cp += idx; } if (BUG(cp == cplast)) { - /* If we didn't parse anything, this code is broken. */ + /* If we didn't parse anything since the last loop, this code is + * broken. */ goto err; // LCOV_EXCL_LINE } cplast = cp; if (! *cp) break; /* End of string; we're done. */ - /* Possible formats are K=V, K="V", V, and "V", depending on flags. */ + /* Possible formats are K=V, K="V", K, V, and "V", depending on flags. */ - /* Find the key. */ + /* Find where the key ends */ if (*cp != '\"') { size_t idx = strcspn(cp, " \t\r\v\n="); if (cp[idx] == '=') { key = tor_memdup_nulterm(cp, idx); cp += idx + 1; + } else if (omit_vals) { + key = tor_memdup_nulterm(cp, idx); + cp += idx; + goto commit; } else { if (!omit_keys) goto err; @@ -214,6 +247,7 @@ kvline_parse(const char *line, unsigned flags) cp += idx; } + commit: if (key && strlen(key) == 0) { /* We don't allow empty keys. */ goto err; @@ -221,7 +255,7 @@ kvline_parse(const char *line, unsigned flags) *next_line = tor_malloc_zero(sizeof(config_line_t)); (*next_line)->key = key ? key : tor_strdup(""); - (*next_line)->value = val; + (*next_line)->value = val ? val : tor_strdup(""); next_line = &(*next_line)->next; key = val = NULL; } diff --git a/src/lib/encoding/kvline.h b/src/lib/encoding/kvline.h index 4eed30a223..6740f81d54 100644 --- a/src/lib/encoding/kvline.h +++ b/src/lib/encoding/kvline.h @@ -17,6 +17,7 @@ struct config_line_t; #define KV_QUOTED (1u<<0) #define KV_OMIT_KEYS (1u<<1) +#define KV_OMIT_VALS (1u<<2) struct config_line_t *kvline_parse(const char *line, unsigned flags); char *kvline_encode(const struct config_line_t *line, unsigned flags); diff --git a/src/test/test_config.c b/src/test/test_config.c index 72649dd9b1..6cfb7b764b 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -5886,6 +5886,61 @@ test_config_kvline_parse(void *arg) tt_assert(lines); tt_str_op(lines->key, OP_EQ, "AB"); tt_str_op(lines->value, OP_EQ, ""); + config_free_lines(lines); + + lines = kvline_parse("AB=", KV_OMIT_VALS); + tt_assert(lines); + tt_str_op(lines->key, OP_EQ, "AB"); + tt_str_op(lines->value, OP_EQ, ""); + config_free_lines(lines); + + lines = kvline_parse(" AB ", KV_OMIT_VALS); + tt_assert(lines); + tt_str_op(lines->key, OP_EQ, "AB"); + tt_str_op(lines->value, OP_EQ, ""); + config_free_lines(lines); + + lines = kvline_parse("AB", KV_OMIT_VALS); + tt_assert(lines); + tt_str_op(lines->key, OP_EQ, "AB"); + tt_str_op(lines->value, OP_EQ, ""); + enc = kvline_encode(lines, KV_OMIT_VALS); + tt_str_op(enc, OP_EQ, "AB"); + tor_free(enc); + config_free_lines(lines); + + lines = kvline_parse("AB=CD", KV_OMIT_VALS); + tt_assert(lines); + tt_str_op(lines->key, OP_EQ, "AB"); + tt_str_op(lines->value, OP_EQ, "CD"); + enc = kvline_encode(lines, KV_OMIT_VALS); + tt_str_op(enc, OP_EQ, "AB=CD"); + tor_free(enc); + config_free_lines(lines); + + lines = kvline_parse("AB=CD DE FGH=I", KV_OMIT_VALS); + tt_assert(lines); + tt_str_op(lines->key, OP_EQ, "AB"); + tt_str_op(lines->value, OP_EQ, "CD"); + tt_str_op(lines->next->key, OP_EQ, "DE"); + tt_str_op(lines->next->value, OP_EQ, ""); + tt_str_op(lines->next->next->key, OP_EQ, "FGH"); + tt_str_op(lines->next->next->value, OP_EQ, "I"); + enc = kvline_encode(lines, KV_OMIT_VALS); + tt_str_op(enc, OP_EQ, "AB=CD DE FGH=I"); + tor_free(enc); + config_free_lines(lines); + + lines = kvline_parse("AB=\"CD E\" DE FGH=\"I\"", KV_OMIT_VALS|KV_QUOTED); + tt_assert(lines); + tt_str_op(lines->key, OP_EQ, "AB"); + tt_str_op(lines->value, OP_EQ, "CD E"); + tt_str_op(lines->next->key, OP_EQ, "DE"); + tt_str_op(lines->next->value, OP_EQ, ""); + tt_str_op(lines->next->next->key, OP_EQ, "FGH"); + tt_str_op(lines->next->next->value, OP_EQ, "I"); + enc = kvline_encode(lines, KV_OMIT_VALS|KV_QUOTED); + tt_str_op(enc, OP_EQ, "AB=\"CD E\" DE FGH=I"); done: config_free_lines(lines); From bb37ad695729984553275880cce131c47361345f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 5 Apr 2019 15:55:52 -0400 Subject: [PATCH 10/24] Add fuzzing support for several more groups of kvlines flags --- src/test/fuzz/fuzz_strops.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/test/fuzz/fuzz_strops.c b/src/test/fuzz/fuzz_strops.c index a37cbb5be8..459b4e21aa 100644 --- a/src/test/fuzz/fuzz_strops.c +++ b/src/test/fuzz/fuzz_strops.c @@ -235,6 +235,18 @@ fuzz_main(const uint8_t *stdin_buf, size_t data_size) kv_flags = 0; ENCODE_ROUNDTRIP(kv_enc, kv_dec, config_free_lines_); break; + case 7: + kv_flags = KV_OMIT_VALS; + ENCODE_ROUNDTRIP(kv_enc, kv_dec, config_free_lines_); + break; + case 8: + kv_flags = KV_QUOTED; + ENCODE_ROUNDTRIP(kv_enc, kv_dec, config_free_lines_); + break; + case 9: + kv_flags = KV_QUOTED|KV_OMIT_VALS; + ENCODE_ROUNDTRIP(kv_enc, kv_dec, config_free_lines_); + break; } return 0; From 0841a69357d73353905f8012f455ec6128201131 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 5 Apr 2019 15:29:56 -0400 Subject: [PATCH 11/24] Allow kvlines in control commands. --- src/feature/control/control_cmd.c | 46 ++++++++++++++++++++++- src/feature/control/control_cmd.h | 17 +++++++++ src/feature/control/control_cmd_args_st.h | 4 ++ src/test/test_controller.c | 24 ++++++++++++ 4 files changed, 89 insertions(+), 2 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index f457e9fa54..727950a938 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -40,6 +40,7 @@ #include "lib/crypt_ops/crypto_rand.h" #include "lib/crypt_ops/crypto_util.h" #include "lib/encoding/confline.h" +#include "lib/encoding/kvline.h" #include "core/or/cpath_build_state_st.h" #include "core/or/entry_connection_st.h" @@ -74,11 +75,27 @@ control_cmd_args_free_(control_cmd_args_t *args) SMARTLIST_FOREACH(args->args, char *, c, tor_free(c)); smartlist_free(args->args); } + config_free_lines(args->kwargs); tor_free(args->object); tor_free(args); } +/** + * Return true iff any element of the NULL-terminated array matches + * kwd. Case-insensitive. + **/ +static bool +string_array_contains_keyword(const char **array, const char *kwd) +{ + for (unsigned i = 0; array[i]; ++i) { + if (! strcasecmp(array[i], kwd)) + return true; + } + return false; +} + + /** * Helper: parse the arguments to a command according to syntax. On * success, set *error_out to NULL and return a newly allocated @@ -96,6 +113,7 @@ control_cmd_parse_args(const char *command, control_cmd_args_t *result = tor_malloc_zero(sizeof(control_cmd_args_t)); const char *cmdline; char *cmdline_alloc = NULL; + tor_assert(syntax->max_args < INT_MAX || syntax->max_args == UINT_MAX); result->command = command; @@ -120,18 +138,42 @@ control_cmd_parse_args(const char *command, result->args = smartlist_new(); smartlist_split_string(result->args, cmdline, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); + SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, + (int)(syntax->max_args+1)); size_t n_args = smartlist_len(result->args); if (n_args < syntax->min_args) { tor_asprintf(error_out, "Need at least %u argument(s)", syntax->min_args); goto err; - } else if (n_args > syntax->max_args) { + } else if (n_args > syntax->max_args && ! syntax->accept_keywords) { tor_asprintf(error_out, "Cannot accept more than %u argument(s)", syntax->max_args); goto err; } + if (n_args > syntax->max_args) { + tor_assert(n_args == syntax->max_args + 1); + tor_assert(syntax->accept_keywords); + char *remainder = smartlist_pop_last(result->args); + result->kwargs = kvline_parse(remainder, syntax->kvline_flags); + tor_free(remainder); + if (result->kwargs == NULL) { + tor_asprintf(error_out, "Cannot parse keyword argument(s)"); + goto err; + } + if (syntax->allowed_keywords) { + /* Check for unpermitted arguments */ + const config_line_t *line; + for (line = result->kwargs; line; line = line->next) { + if (! string_array_contains_keyword(syntax->allowed_keywords, + line->key)) { + tor_asprintf(error_out, "Unrecognized keyword argument %s", + escaped(line->key)); + } + } + } + } + tor_assert_nonfatal(*error_out == NULL); goto done; err: diff --git a/src/feature/control/control_cmd.h b/src/feature/control/control_cmd.h index 801bf43709..6f35c74de0 100644 --- a/src/feature/control/control_cmd.h +++ b/src/feature/control/control_cmd.h @@ -43,6 +43,23 @@ typedef struct control_cmd_syntax_t { * UINT_MAX for no limit. **/ unsigned int max_args; + /** + * If true, we should parse options after the positional arguments + * as a set of unordered flags and key=value arguments. + * + * Requires that max_args is not UINT_MAX. + **/ + bool accept_keywords; + /** + * If accept_keywords is true, then only the keywords listed in this + * (NULL-terminated) array are valid keywords for this command. + **/ + const char **allowed_keywords; + /** + * If accept_keywords is true, this option is passed to kvline_parse() as + * its flags. + **/ + unsigned kvline_flags; /** * True iff this command wants to be followed by a multiline object. **/ diff --git a/src/feature/control/control_cmd_args_st.h b/src/feature/control/control_cmd_args_st.h index 9ecc5d750e..06e2a183ba 100644 --- a/src/feature/control/control_cmd_args_st.h +++ b/src/feature/control/control_cmd_args_st.h @@ -31,6 +31,10 @@ struct control_cmd_args_t { * Positional arguments to the command. **/ struct smartlist_t *args; + /** + * Keyword arguments to the command. + **/ + struct config_line_t *kwargs; /** * Number of bytes in object; 0 if object is not set. **/ diff --git a/src/test/test_controller.c b/src/test/test_controller.c index ca853367ae..dc286daccb 100644 --- a/src/test/test_controller.c +++ b/src/test/test_controller.c @@ -18,6 +18,8 @@ #include "test/test.h" #include "test/test_helpers.h" #include "lib/net/resolve.h" +#include "lib/encoding/confline.h" +#include "lib/encoding/kvline.h" #include "feature/control/control_connection_st.h" #include "feature/control/control_cmd_args_st.h" @@ -58,6 +60,16 @@ control_cmd_dump_args(const control_cmd_args_t *result) buf_add_string(buf, ", obj="); buf_add_string(buf, escaped(result->object)); } + if (result->kwargs) { + buf_add_string(buf, ", { "); + const config_line_t *line; + for (line = result->kwargs; line; line = line->next) { + const bool last = (line->next == NULL); + buf_add_printf(buf, "%s=%s%s ", line->key, escaped(line->value), + last ? "" : ","); + } + buf_add_string(buf, "}"); + } buf_add_string(buf, " }"); char *encoded = buf_extract(buf, NULL); @@ -152,6 +164,17 @@ static const control_cmd_syntax_t no_args_one_obj_syntax = { static const parse_test_params_t parse_no_args_one_obj_params = TESTPARAMS( no_args_one_obj_syntax, no_args_one_obj_tests ); +static const parser_testcase_t no_args_kwargs_tests[] = { + OK("", "{ args=[] }"), +}; +static const control_cmd_syntax_t no_args_kwargs_syntax = { + .min_args=0, .max_args=0, + .accept_keywords=true, + .kvline_flags=KV_OMIT_VALS +}; +static const parse_test_params_t parse_no_args_kwargs_params = + TESTPARAMS( no_args_kwargs_syntax, no_args_kwargs_tests ); + static void test_add_onion_helper_keyarg_v3(void *arg) { @@ -1752,6 +1775,7 @@ test_getinfo_md_all(void *arg) struct testcase_t controller_tests[] = { PARSER_TEST(one_to_three), PARSER_TEST(no_args_one_obj), + PARSER_TEST(no_args_kwargs), { "add_onion_helper_keyarg_v2", test_add_onion_helper_keyarg_v2, 0, NULL, NULL }, { "add_onion_helper_keyarg_v3", test_add_onion_helper_keyarg_v3, 0, From 9471391694168f3d82365ced07b14e466b32b612 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 8 Apr 2019 09:37:39 -0400 Subject: [PATCH 12/24] Add kvline support to controller command parser. This should let us handle all (or nearly all) of the remaining commands. --- src/feature/control/control_cmd.c | 2 +- src/test/test_controller.c | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 727950a938..4f67e31637 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -95,7 +95,6 @@ string_array_contains_keyword(const char **array, const char *kwd) return false; } - /** * Helper: parse the arguments to a command according to syntax. On * success, set *error_out to NULL and return a newly allocated @@ -169,6 +168,7 @@ control_cmd_parse_args(const char *command, line->key)) { tor_asprintf(error_out, "Unrecognized keyword argument %s", escaped(line->key)); + goto err; } } } diff --git a/src/test/test_controller.c b/src/test/test_controller.c index dc286daccb..fd4f26f086 100644 --- a/src/test/test_controller.c +++ b/src/test/test_controller.c @@ -148,6 +148,7 @@ static const control_cmd_syntax_t one_to_three_syntax = { static const parse_test_params_t parse_one_to_three_params = TESTPARAMS( one_to_three_syntax, one_to_three_tests ); +// = static const parser_testcase_t no_args_one_obj_tests[] = { ERR("Hi there!\r\n.", "Cannot accept more than 0 argument(s)"), ERR("", "Empty body"), @@ -166,6 +167,11 @@ static const parse_test_params_t parse_no_args_one_obj_params = static const parser_testcase_t no_args_kwargs_tests[] = { OK("", "{ args=[] }"), + OK(" ", "{ args=[] }"), + OK("hello there=world", "{ args=[], { hello=\"\", there=\"world\" } }"), + OK("hello there=world today", + "{ args=[], { hello=\"\", there=\"world\", today=\"\" } }"), + ERR("=Foo", "Cannot parse keyword argument(s)"), }; static const control_cmd_syntax_t no_args_kwargs_syntax = { .min_args=0, .max_args=0, @@ -175,6 +181,26 @@ static const control_cmd_syntax_t no_args_kwargs_syntax = { static const parse_test_params_t parse_no_args_kwargs_params = TESTPARAMS( no_args_kwargs_syntax, no_args_kwargs_tests ); +static const char *one_arg_kwargs_allow_keywords[] = { + "Hello", "world", NULL +}; +static const parser_testcase_t one_arg_kwargs_tests[] = { + ERR("", "Need at least 1 argument(s)"), + OK("Hi", "{ args=[ \"Hi\" ] }"), + ERR("hello there=world", "Unrecognized keyword argument \"there\""), + OK("Hi HELLO=foo", "{ args=[ \"Hi\" ], { HELLO=\"foo\" } }"), + OK("Hi world=\"bar baz\" hello ", + "{ args=[ \"Hi\" ], { world=\"bar baz\", hello=\"\" } }"), +}; +static const control_cmd_syntax_t one_arg_kwargs_syntax = { + .min_args=1, .max_args=1, + .accept_keywords=true, + .allowed_keywords=one_arg_kwargs_allow_keywords, + .kvline_flags=KV_OMIT_VALS|KV_QUOTED, +}; +static const parse_test_params_t parse_one_arg_kwargs_params = + TESTPARAMS( one_arg_kwargs_syntax, one_arg_kwargs_tests ); + static void test_add_onion_helper_keyarg_v3(void *arg) { @@ -1776,6 +1802,7 @@ struct testcase_t controller_tests[] = { PARSER_TEST(one_to_three), PARSER_TEST(no_args_one_obj), PARSER_TEST(no_args_kwargs), + PARSER_TEST(one_arg_kwargs), { "add_onion_helper_keyarg_v2", test_add_onion_helper_keyarg_v2, 0, NULL, NULL }, { "add_onion_helper_keyarg_v3", test_add_onion_helper_keyarg_v3, 0, From dab35386cafc837c29fd251213337dec092043fe Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 8 Apr 2019 10:28:56 -0400 Subject: [PATCH 13/24] Add a case-insensitive variant to config_line_find() --- src/lib/encoding/confline.c | 13 +++++++++++++ src/lib/encoding/confline.h | 2 ++ 2 files changed, 15 insertions(+) diff --git a/src/lib/encoding/confline.c b/src/lib/encoding/confline.c index 8110f3dd9c..fdb575e03f 100644 --- a/src/lib/encoding/confline.c +++ b/src/lib/encoding/confline.c @@ -82,6 +82,19 @@ config_line_find(const config_line_t *lines, return NULL; } +/** As config_line_find(), but perform a case-insensitive comparison. */ +const config_line_t * +config_line_find_case(const config_line_t *lines, + const char *key) +{ + const config_line_t *cl; + for (cl = lines; cl; cl = cl->next) { + if (!strcasecmp(cl->key, key)) + return cl; + } + return NULL; +} + /** Auxiliary function that does all the work of config_get_lines. * recursion_level is the count of how many nested %includes we have. * opened_lst will have a list of opened files if provided. diff --git a/src/lib/encoding/confline.h b/src/lib/encoding/confline.h index 3d9ae8a662..56ea36bf61 100644 --- a/src/lib/encoding/confline.h +++ b/src/lib/encoding/confline.h @@ -48,6 +48,8 @@ config_line_t *config_lines_dup_and_filter(const config_line_t *inp, const char *key); const config_line_t *config_line_find(const config_line_t *lines, const char *key); +const config_line_t *config_line_find_case(const config_line_t *lines, + const char *key); int config_lines_eq(config_line_t *a, config_line_t *b); int config_count_key(const config_line_t *a, const char *key); void config_free_lines_(config_line_t *front); From d8b3ec865de2738144bd6bbf9f6355662e64eb25 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 8 Apr 2019 10:23:21 -0400 Subject: [PATCH 14/24] Update more controller commands, now that we have kvline support --- src/feature/control/control_cmd.c | 547 +++++++++++++----------------- 1 file changed, 241 insertions(+), 306 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 4f67e31637..1848555351 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -184,6 +184,17 @@ control_cmd_parse_args(const char *command, return result; } +/** + * Return true iff lines contains flags as a no-value + * (keyword-only) entry. + **/ +static bool +config_lines_contain_flag(const config_line_t *lines, const char *flag) +{ + const config_line_t *line = config_line_find_case(lines, flag); + return line && !strcmp(line->value, ""); +} + /** Called when we receive a SETCONF message: parse the body and try * to update our configuration. Reply with a DONE or ERROR message. * Modifies the contents of body.*/ @@ -370,15 +381,19 @@ handle_control_setevents(control_connection_t *conn, return 0; } +static const control_cmd_syntax_t saveconf_syntax = { + .max_args = 0, + .accept_keywords = true, + .kvline_flags=KV_OMIT_VALS, +}; + /** Called when we get a SAVECONF command. Try to flush the current options to * disk, and report success or failure. */ static int -handle_control_saveconf(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_saveconf(control_connection_t *conn, + const control_cmd_args_t *args) { - (void) len; - - int force = !strcmpstart(body, "FORCE"); + bool force = config_lines_contain_flag(args->kwargs, "FORCE"); const or_options_t *options = get_options(); if ((!force && options->IncludeUsed) || options_save_current() < 0) { connection_write_str_to_buf( @@ -620,30 +635,27 @@ address_is_invalid_mapaddress_target(const char *addr) return address_is_invalid_destination(addr, 1); } +static const control_cmd_syntax_t mapaddress_syntax = { + .max_args=0, + .accept_keywords=true, +}; + /** Called when we get a MAPADDRESS command; try to bind all listed addresses, * and report success or failure. */ static int -handle_control_mapaddress(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_mapaddress(control_connection_t *conn, + const control_cmd_args_t *args) { - smartlist_t *elts; - smartlist_t *lines; smartlist_t *reply; char *r; size_t sz; - (void) len; /* body is NUL-terminated, so it's safe to ignore the length. */ - lines = smartlist_new(); - elts = smartlist_new(); reply = smartlist_new(); - smartlist_split_string(lines, body, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); - SMARTLIST_FOREACH_BEGIN(lines, char *, line) { - tor_strlower(line); - smartlist_split_string(elts, line, "=", 0, 2); - if (smartlist_len(elts) == 2) { - const char *from = smartlist_get(elts,0); - const char *to = smartlist_get(elts,1); + const config_line_t *line; + for (line = args->kwargs; line; line = line->next) { + const char *from = line->key; + const char *to = line->value; + { if (address_is_invalid_mapaddress_target(to)) { smartlist_add_asprintf(reply, "512-syntax error: invalid address '%s'", to); @@ -658,10 +670,10 @@ handle_control_mapaddress(control_connection_t *conn, uint32_t len, type, tor_strdup(to)); if (!address) { smartlist_add_asprintf(reply, - "451-resource exhausted: skipping '%s'", line); + "451-resource exhausted: skipping '%s=%s'", from,to); log_warn(LD_CONTROL, "Unable to allocate address for '%s' in MapAddress msg", - safe_str_client(line)); + safe_str_client(to)); } else { smartlist_add_asprintf(reply, "250-%s=%s", address, to); } @@ -671,27 +683,16 @@ handle_control_mapaddress(control_connection_t *conn, uint32_t len, ADDRMAPSRC_CONTROLLER, &msg) < 0) { smartlist_add_asprintf(reply, "512-syntax error: invalid address mapping " - " '%s': %s", line, msg); + " '%s=%s': %s", from, to, msg); log_warn(LD_CONTROL, - "Skipping invalid argument '%s' in MapAddress msg: %s", - line, msg); + "Skipping invalid argument '%s=%s' in MapAddress msg: %s", + from, to, msg); } else { - smartlist_add_asprintf(reply, "250-%s", line); + smartlist_add_asprintf(reply, "250-%s=%s", from, to); } } - } else { - smartlist_add_asprintf(reply, "512-syntax error: mapping '%s' is " - "not of expected form 'foo=bar'.", line); - log_info(LD_CONTROL, "Skipping MapAddress '%s': wrong " - "number of items.", - safe_str_client(line)); } - SMARTLIST_FOREACH(elts, char *, cp, tor_free(cp)); - smartlist_clear(elts); - } SMARTLIST_FOREACH_END(line); - SMARTLIST_FOREACH(lines, char *, cp, tor_free(cp)); - smartlist_free(lines); - smartlist_free(elts); + } if (smartlist_len(reply)) { ((char*)smartlist_get(reply,smartlist_len(reply)-1))[3] = ' '; @@ -931,36 +932,36 @@ handle_control_extendcircuit(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t setcircuitpurpose_syntax = { + .max_args=1, + .accept_keywords=true, +}; + /** Called when we get a SETCIRCUITPURPOSE message. If we can find the * circuit and it's a valid purpose, change it. */ static int handle_control_setcircuitpurpose(control_connection_t *conn, - uint32_t len, const char *body) + const control_cmd_args_t *args) { origin_circuit_t *circ = NULL; uint8_t new_purpose; - smartlist_t *args; - (void) len; /* body is NUL-terminated, so it's safe to ignore the length. */ + const char *circ_id = smartlist_get(args->args,0); - args = getargs_helper("SETCIRCUITPURPOSE", conn, body, 2, -1); - if (!args) - goto done; - - if (!(circ = get_circ(smartlist_get(args,0)))) { - connection_printf_to_buf(conn, "552 Unknown circuit \"%s\"\r\n", - (char*)smartlist_get(args, 0)); + if (!(circ = get_circ(circ_id))) { + connection_printf_to_buf(conn, "552 Unknown circuit \"%s\"\r\n", circ_id); goto done; } { - const char *purp = find_element_starting_with(args,1,"PURPOSE="); + const config_line_t *purp = config_line_find_case(args->kwargs, "PURPOSE"); if (!purp) { connection_write_str_to_buf("552 No purpose given\r\n", conn); goto done; } - new_purpose = circuit_purpose_from_string(purp); + new_purpose = circuit_purpose_from_string(purp->value); if (new_purpose == CIRCUIT_PURPOSE_UNKNOWN) { - connection_printf_to_buf(conn, "552 Unknown purpose \"%s\"\r\n", purp); + connection_printf_to_buf(conn, "552 Unknown purpose \"%s\"\r\n", + purp->value); goto done; } } @@ -969,54 +970,50 @@ handle_control_setcircuitpurpose(control_connection_t *conn, connection_write_str_to_buf("250 OK\r\n", conn); done: - if (args) { - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); - } return 0; } +static const char *attachstream_keywords[] = { + "HOP", NULL +}; +static const control_cmd_syntax_t attachstream_syntax = { + .min_args=2, .max_args=2, + .accept_keywords=true, + .allowed_keywords=attachstream_keywords +}; + /** Called when we get an ATTACHSTREAM message. Try to attach the requested * stream, and report success or failure. */ static int -handle_control_attachstream(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_attachstream(control_connection_t *conn, + const control_cmd_args_t *args) { entry_connection_t *ap_conn = NULL; origin_circuit_t *circ = NULL; - int zero_circ; - smartlist_t *args; crypt_path_t *cpath=NULL; int hop=0, hop_line_ok=1; - (void) len; + const char *stream_id = smartlist_get(args->args, 0); + const char *circ_id = smartlist_get(args->args, 1); + int zero_circ = !strcmp(circ_id, "0"); + const config_line_t *hoparg = config_line_find_case(args->kwargs, "HOP"); - args = getargs_helper("ATTACHSTREAM", conn, body, 2, -1); - if (!args) + if (!(ap_conn = get_stream(stream_id))) { + connection_printf_to_buf(conn, "552 Unknown stream \"%s\"\r\n", stream_id); + return 0; + } else if (!zero_circ && !(circ = get_circ(circ_id))) { + connection_printf_to_buf(conn, "552 Unknown circuit \"%s\"\r\n", circ_id); return 0; - - zero_circ = !strcmp("0", (char*)smartlist_get(args,1)); - - if (!(ap_conn = get_stream(smartlist_get(args, 0)))) { - connection_printf_to_buf(conn, "552 Unknown stream \"%s\"\r\n", - (char*)smartlist_get(args, 0)); - } else if (!zero_circ && !(circ = get_circ(smartlist_get(args, 1)))) { - connection_printf_to_buf(conn, "552 Unknown circuit \"%s\"\r\n", - (char*)smartlist_get(args, 1)); } else if (circ) { - const char *hopstring = find_element_starting_with(args,2,"HOP="); - if (hopstring) { - hopstring += strlen("HOP="); - hop = (int) tor_parse_ulong(hopstring, 10, 0, INT_MAX, + if (hoparg) { + hop = (int) tor_parse_ulong(hoparg->value, 10, 0, INT_MAX, &hop_line_ok, NULL); if (!hop_line_ok) { /* broken hop line */ - connection_printf_to_buf(conn, "552 Bad value hop=%s\r\n", hopstring); + connection_printf_to_buf(conn, "552 Bad value hop=%s\r\n", + hoparg->value); + return 0; } } } - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); - if (!ap_conn || (!zero_circ && !circ) || !hop_line_ok) - return 0; if (ENTRY_TO_CONN(ap_conn)->state != AP_CONN_STATE_CONTROLLER_WAIT && ENTRY_TO_CONN(ap_conn)->state != AP_CONN_STATE_CONNECT_WAIT && @@ -1071,59 +1068,49 @@ handle_control_attachstream(control_connection_t *conn, uint32_t len, return 0; } +static const char *postdescriptor_keywords[] = { + "cache", "purpose", NULL, +}; + +static const control_cmd_syntax_t postdescriptor_syntax = { + .max_args = 0, + .accept_keywords = true, + .allowed_keywords = postdescriptor_keywords, + .want_object = true, +}; + /** Called when we get a POSTDESCRIPTOR message. Try to learn the provided * descriptor, and report success or failure. */ static int -handle_control_postdescriptor(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_postdescriptor(control_connection_t *conn, + const control_cmd_args_t *args) { - char *desc; const char *msg=NULL; uint8_t purpose = ROUTER_PURPOSE_GENERAL; int cache = 0; /* eventually, we may switch this to 1 */ + const config_line_t *line; - const char *cp = memchr(body, '\n', len); - - if (cp == NULL) { - connection_printf_to_buf(conn, "251 Empty body\r\n"); - return 0; + line = config_line_find_case(args->kwargs, "purpose"); + if (line) { + purpose = router_purpose_from_string(line->value); + connection_printf_to_buf(conn, "552 Unknown purpose \"%s\"\r\n", + line->value); + goto done; } - ++cp; - - char *cmdline = tor_memdup_nulterm(body, cp-body); - smartlist_t *args = smartlist_new(); - smartlist_split_string(args, cmdline, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); - SMARTLIST_FOREACH_BEGIN(args, char *, option) { - if (!strcasecmpstart(option, "purpose=")) { - option += strlen("purpose="); - purpose = router_purpose_from_string(option); - if (purpose == ROUTER_PURPOSE_UNKNOWN) { - connection_printf_to_buf(conn, "552 Unknown purpose \"%s\"\r\n", - option); - goto done; - } - } else if (!strcasecmpstart(option, "cache=")) { - option += strlen("cache="); - if (!strcasecmp(option, "no")) - cache = 0; - else if (!strcasecmp(option, "yes")) - cache = 1; - else { - connection_printf_to_buf(conn, "552 Unknown cache request \"%s\"\r\n", - option); - goto done; - } - } else { /* unrecognized argument? */ - connection_printf_to_buf(conn, - "512 Unexpected argument \"%s\" to postdescriptor\r\n", option); + line = config_line_find_case(args->kwargs, "cache"); + if (line) { + if (!strcasecmp(line->value, "no")) + cache = 0; + else if (!strcasecmp(line->value, "yes")) + cache = 1; + else { + connection_printf_to_buf(conn, "552 Unknown cache request \"%s\"\r\n", + line->value); goto done; } - } SMARTLIST_FOREACH_END(option); + } - read_escaped_data(cp, len-(cp-body), &desc); - - switch (router_load_single_router(desc, purpose, cache, &msg)) { + switch (router_load_single_router(args->object, purpose, cache, &msg)) { case -1: if (!msg) msg = "Could not parse descriptor"; connection_printf_to_buf(conn, "554 %s\r\n", msg); @@ -1137,11 +1124,7 @@ handle_control_postdescriptor(control_connection_t *conn, uint32_t len, break; } - tor_free(desc); done: - SMARTLIST_FOREACH(args, char *, arg, tor_free(arg)); - smartlist_free(args); - tor_free(cmdline); return 0; } @@ -1230,38 +1213,29 @@ handle_control_closestream(control_connection_t *conn, return 0; } +static const control_cmd_syntax_t closecircuit_syntax = { + .min_args=1, .max_args=1, + .accept_keywords=true, + .kvline_flags=KV_OMIT_VALS, + // XXXX we might want to exclude unrecognized flags, but for now we + // XXXX just ignore them for backward compatibility. +}; + /** Called when we get a CLOSECIRCUIT command; try to close the named circuit * and report success or failure. */ static int -handle_control_closecircuit(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_closecircuit(control_connection_t *conn, + const control_cmd_args_t *args) { + const char *circ_id = smartlist_get(args->args, 0); origin_circuit_t *circ = NULL; - int safe = 0; - smartlist_t *args; - (void) len; - args = getargs_helper("CLOSECIRCUIT", conn, body, 1, -1); - if (!args) + if (!(circ=get_circ(circ_id))) { + connection_printf_to_buf(conn, "552 Unknown circuit \"%s\"\r\n", circ_id); return 0; - - if (!(circ=get_circ(smartlist_get(args, 0)))) - connection_printf_to_buf(conn, "552 Unknown circuit \"%s\"\r\n", - (char*)smartlist_get(args, 0)); - else { - int i; - for (i=1; i < smartlist_len(args); ++i) { - if (!strcasecmp(smartlist_get(args, i), "IfUnused")) - safe = 1; - else - log_info(LD_CONTROL, "Skipping unknown option %s", - (char*)smartlist_get(args,i)); - } } - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); - if (!circ) - return 0; + + bool safe = config_lines_contain_flag(args->kwargs, "IfUnused"); if (!safe || !circ->p_streams) { circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_REQUESTED); @@ -1271,36 +1245,43 @@ handle_control_closecircuit(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t resolve_syntax = { + .max_args=0, + .accept_keywords=true, + .kvline_flags=KV_OMIT_VALS, +}; + /** Called when we get a RESOLVE command: start trying to resolve * the listed addresses. */ static int -handle_control_resolve(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_resolve(control_connection_t *conn, + const control_cmd_args_t *args) { - smartlist_t *args, *failed; + smartlist_t *failed; int is_reverse = 0; - (void) len; /* body is nul-terminated; it's safe to ignore the length */ if (!(conn->event_mask & (((event_mask_t)1)<kwargs, "mode"); + if (modearg && !strcasecmp(modearg->value, "reverse")) is_reverse = 1; } failed = smartlist_new(); - SMARTLIST_FOREACH(args, const char *, arg, { - if (!is_keyval_pair(arg)) { - if (dnsserv_launch_request(arg, is_reverse, conn)<0) - smartlist_add(failed, (char*)arg); - } - }); + for (const config_line_t *line = args->kwargs; line; line = line->next) { + if (!strlen(line->value)) { + const char *addr = line->key; + if (dnsserv_launch_request(addr, is_reverse, conn)<0) + smartlist_add(failed, (char*)addr); + } else { + // XXXX arguably we should reject unrecognized keyword arguments, + // XXXX but the old implementation didn't do that. + } + } send_control_done(conn); SMARTLIST_FOREACH(failed, const char *, arg, { @@ -1308,8 +1289,6 @@ handle_control_resolve(control_connection_t *conn, uint32_t len, "internal", 0); }); - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); smartlist_free(failed); return 0; } @@ -1443,30 +1422,33 @@ handle_control_dropguards(control_connection_t *conn, return 0; } +static const char *hsfetch_keywords[] = { + "SERVER", NULL, +}; +static const control_cmd_syntax_t hsfetch_syntax = { + .min_args = 1, .max_args = 1, + .accept_keywords = true, + .allowed_keywords = hsfetch_keywords, + .want_object = true, +}; + /** Implementation for the HSFETCH command. */ static int -handle_control_hsfetch(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_hsfetch(control_connection_t *conn, + const control_cmd_args_t *args) + { - int i; - char digest[DIGEST_LEN], *hsaddress = NULL, *arg1 = NULL, *desc_id = NULL; - smartlist_t *args = NULL, *hsdirs = NULL; - (void) len; /* body is nul-terminated; it's safe to ignore the length */ - static const char *hsfetch_command = "HSFETCH"; + char digest[DIGEST_LEN], *desc_id = NULL; + smartlist_t *hsdirs = NULL; static const char *v2_str = "v2-"; const size_t v2_str_len = strlen(v2_str); rend_data_t *rend_query = NULL; ed25519_public_key_t v3_pk; uint32_t version; - - /* Make sure we have at least one argument, the HSAddress. */ - args = getargs_helper(hsfetch_command, conn, body, 1, -1); - if (!args) { - goto exit; - } + const char *hsaddress = NULL; /* Extract the first argument (either HSAddress or DescID). */ - arg1 = smartlist_get(args, 0); + const char *arg1 = smartlist_get(args->args, 0); /* Test if it's an HS address without the .onion part. */ if (rend_valid_v2_service_id(arg1)) { hsaddress = arg1; @@ -1490,18 +1472,11 @@ handle_control_hsfetch(control_connection_t *conn, uint32_t len, goto done; } - static const char *opt_server = "SERVER="; + for (const config_line_t *line = args->kwargs; line; line = line->next) { + if (!strcasecmp(line->key, "SERVER")) { + const char *server = line->value; - /* Skip first argument because it's the HSAddress or DescID. */ - for (i = 1; i < smartlist_len(args); ++i) { - const char *arg = smartlist_get(args, i); - const node_t *node; - - if (!strcasecmpstart(arg, opt_server)) { - const char *server; - - server = arg + strlen(opt_server); - node = node_get_by_hex_id(server, 0); + const node_t *node = node_get_by_hex_id(server, 0); if (!node) { connection_printf_to_buf(conn, "552 Server \"%s\" not found\r\n", server); @@ -1514,9 +1489,7 @@ handle_control_hsfetch(control_connection_t *conn, uint32_t len, /* Valid server, add it to our local list. */ smartlist_add(hsdirs, node->rs); } else { - connection_printf_to_buf(conn, "513 Unexpected argument \"%s\"\r\n", - arg); - goto done; + tor_assert_nonfatal_unreached(); } } @@ -1532,9 +1505,8 @@ handle_control_hsfetch(control_connection_t *conn, uint32_t len, /* Using a descriptor ID, we force the user to provide at least one * hsdir server using the SERVER= option. */ if (desc_id && (!hsdirs || !smartlist_len(hsdirs))) { - connection_printf_to_buf(conn, "512 %s option is required\r\n", - opt_server); - goto done; + connection_printf_to_buf(conn, "512 SERVER option is required\r\n"); + goto done; } /* We are about to trigger HSDir fetch so send the OK now because after @@ -1552,96 +1524,75 @@ handle_control_hsfetch(control_connection_t *conn, uint32_t len, } done: - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); /* Contains data pointer that we don't own thus no cleanup. */ smartlist_free(hsdirs); rend_data_free(rend_query); - exit: return 0; } +static const char *hspost_keywords[] = { + "SERVER", "HSADDRESS", NULL +}; +static const control_cmd_syntax_t hspost_syntax = { + .min_args = 0, .max_args = 0, + .accept_keywords = true, + .want_object = true, + .allowed_keywords = hspost_keywords +}; + /** Implementation for the HSPOST command. */ static int handle_control_hspost(control_connection_t *conn, - uint32_t len, - const char *body) + const control_cmd_args_t *args) { - static const char *opt_server = "SERVER="; - static const char *opt_hsaddress = "HSADDRESS="; smartlist_t *hs_dirs = NULL; - const char *encoded_desc = body; - size_t encoded_desc_len = len; + const char *encoded_desc = args->object; + size_t encoded_desc_len = args->object_len; const char *onion_address = NULL; + const config_line_t *line; - char *cp = memchr(body, '\n', len); - if (cp == NULL) { - connection_printf_to_buf(conn, "251 Empty body\r\n"); - return 0; - } - char *argline = tor_strndup(body, cp-body); + for (line = args->kwargs; line; line = line->next) { + if (!strcasecmpstart(line->key, "SERVER")) { + const char *server = line->value; + const node_t *node = node_get_by_hex_id(server, 0); - smartlist_t *args = smartlist_new(); - - /* If any SERVER= or HSADDRESS= options were specified, try to parse - * the options line. */ - if (!strcasecmpstart(argline, opt_server) || - !strcasecmpstart(argline, opt_hsaddress)) { - /* encoded_desc begins after a newline character */ - cp = cp + 1; - encoded_desc = cp; - encoded_desc_len = len-(cp-body); - - smartlist_split_string(args, argline, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); - SMARTLIST_FOREACH_BEGIN(args, const char *, arg) { - if (!strcasecmpstart(arg, opt_server)) { - const char *server = arg + strlen(opt_server); - const node_t *node = node_get_by_hex_id(server, 0); - - if (!node || !node->rs) { - connection_printf_to_buf(conn, "552 Server \"%s\" not found\r\n", - server); - goto done; - } - /* Valid server, add it to our local list. */ - if (!hs_dirs) - hs_dirs = smartlist_new(); - smartlist_add(hs_dirs, node->rs); - } else if (!strcasecmpstart(arg, opt_hsaddress)) { - const char *address = arg + strlen(opt_hsaddress); - if (!hs_address_is_valid(address)) { - connection_printf_to_buf(conn, "512 Malformed onion address\r\n"); - goto done; - } - onion_address = address; - } else { - connection_printf_to_buf(conn, "512 Unexpected argument \"%s\"\r\n", - arg); + if (!node || !node->rs) { + connection_printf_to_buf(conn, "552 Server \"%s\" not found\r\n", + server); goto done; } - } SMARTLIST_FOREACH_END(arg); + /* Valid server, add it to our local list. */ + if (!hs_dirs) + hs_dirs = smartlist_new(); + smartlist_add(hs_dirs, node->rs); + } else if (!strcasecmpstart(line->key, "HSADDRESS")) { + const char *address = line->value; + if (!hs_address_is_valid(address)) { + connection_printf_to_buf(conn, "512 Malformed onion address\r\n"); + goto done; + } + onion_address = address; + } else { + tor_assert_nonfatal_unreached(); + } } /* Handle the v3 case. */ if (onion_address) { - char *desc_str = NULL; - read_escaped_data(encoded_desc, encoded_desc_len, &desc_str); - if (hs_control_hspost_command(desc_str, onion_address, hs_dirs) < 0) { + if (hs_control_hspost_command(encoded_desc, onion_address, hs_dirs) < 0) { connection_printf_to_buf(conn, "554 Invalid descriptor\r\n"); } else { send_control_done(conn); } - tor_free(desc_str); goto done; } /* From this point on, it is only v2. */ - /* Read the dot encoded descriptor, and parse it. */ + /* parse it. */ rend_encoded_v2_service_descriptor_t *desc = tor_malloc_zero(sizeof(rend_encoded_v2_service_descriptor_t)); - read_escaped_data(encoded_desc, encoded_desc_len, &desc->desc_str); + desc->desc_str = tor_memdup_nulterm(encoded_desc, encoded_desc_len); rend_service_descriptor_t *parsed = NULL; char *intro_content = NULL; @@ -1675,10 +1626,7 @@ handle_control_hspost(control_connection_t *conn, tor_free(intro_content); rend_encoded_v2_service_descriptor_free(desc); done: - tor_free(argline); smartlist_free(hs_dirs); /* Contents belong to the rend service code. */ - SMARTLIST_FOREACH(args, char *, arg, tor_free(arg)); - smartlist_free(args); return 0; } @@ -1742,21 +1690,21 @@ get_detached_onion_services(void) return detached_onion_services; } +static const char *add_onion_keywords[] = { + "Port", "Flags", "MaxStreams", "ClientAuth", NULL +}; +static const control_cmd_syntax_t add_onion_syntax = { + .min_args = 1, .max_args = 1, + .accept_keywords = true, + .allowed_keywords = add_onion_keywords +}; + /** Called when we get a ADD_ONION command; parse the body, and set up * the new ephemeral Onion Service. */ static int handle_control_add_onion(control_connection_t *conn, - uint32_t len, - const char *body) + const control_cmd_args_t *args) { - smartlist_t *args; - int arg_len; - (void) len; /* body is nul-terminated; it's safe to ignore the length */ - args = getargs_helper("ADD_ONION", conn, body, 2, -1); - if (!args) - return 0; - arg_len = smartlist_len(args); - /* Parse all of the arguments that do not involve handling cryptographic * material first, since there's no reason to touch that at all if any of * the other arguments are malformed. @@ -1769,36 +1717,28 @@ handle_control_add_onion(control_connection_t *conn, int max_streams = 0; int max_streams_close_circuit = 0; rend_auth_type_t auth_type = REND_NO_AUTH; - /* Default to adding an anonymous hidden service if no flag is given */ int non_anonymous = 0; - for (int i = 1; i < arg_len; i++) { - static const char *port_prefix = "Port="; - static const char *flags_prefix = "Flags="; - static const char *max_s_prefix = "MaxStreams="; - static const char *auth_prefix = "ClientAuth="; + const config_line_t *arg; - const char *arg = smartlist_get(args, (int)i); - if (!strcasecmpstart(arg, port_prefix)) { + for (arg = args->kwargs; arg; arg = arg->next) { + if (!strcasecmp(arg->key, "Port")) { /* "Port=VIRTPORT[,TARGET]". */ - const char *port_str = arg + strlen(port_prefix); - rend_service_port_config_t *cfg = - rend_service_parse_port_config(port_str, ",", NULL); + rend_service_parse_port_config(arg->value, ",", NULL); if (!cfg) { connection_printf_to_buf(conn, "512 Invalid VIRTPORT/TARGET\r\n"); goto out; } smartlist_add(port_cfgs, cfg); - } else if (!strcasecmpstart(arg, max_s_prefix)) { + } else if (!strcasecmp(arg->key, "MaxStreams")) { /* "MaxStreams=[0..65535]". */ - const char *max_s_str = arg + strlen(max_s_prefix); int ok = 0; - max_streams = (int)tor_parse_long(max_s_str, 10, 0, 65535, &ok, NULL); + max_streams = (int)tor_parse_long(arg->value, 10, 0, 65535, &ok, NULL); if (!ok) { connection_printf_to_buf(conn, "512 Invalid MaxStreams\r\n"); goto out; } - } else if (!strcasecmpstart(arg, flags_prefix)) { + } else if (!strcasecmp(arg->key, "Flags")) { /* "Flags=Flag[,Flag]", where Flag can be: * * 'DiscardPK' - If tor generates the keypair, do not include it in * the response. @@ -1821,8 +1761,7 @@ handle_control_add_onion(control_connection_t *conn, smartlist_t *flags = smartlist_new(); int bad = 0; - smartlist_split_string(flags, arg + strlen(flags_prefix), ",", - SPLIT_IGNORE_BLANK, 0); + smartlist_split_string(flags, arg->value, ",", SPLIT_IGNORE_BLANK, 0); if (smartlist_len(flags) < 1) { connection_printf_to_buf(conn, "512 Invalid 'Flags' argument\r\n"); bad = 1; @@ -1851,11 +1790,12 @@ handle_control_add_onion(control_connection_t *conn, smartlist_free(flags); if (bad) goto out; - } else if (!strcasecmpstart(arg, auth_prefix)) { + + } else if (!strcasecmp(arg->key, "ClientAuth")) { char *err_msg = NULL; int created = 0; rend_authorized_client_t *client = - add_onion_helper_clientauth(arg + strlen(auth_prefix), + add_onion_helper_clientauth(arg->value, &created, &err_msg); if (!client) { if (err_msg) { @@ -1888,7 +1828,7 @@ handle_control_add_onion(control_connection_t *conn, smartlist_add(auth_created_clients, client); } } else { - connection_printf_to_buf(conn, "513 Invalid argument\r\n"); + tor_assert_nonfatal_unreached(); goto out; } } @@ -1929,7 +1869,8 @@ handle_control_add_onion(control_connection_t *conn, char *key_new_blob = NULL; char *err_msg = NULL; - if (add_onion_helper_keyarg(smartlist_get(args, 0), discard_pk, + const char *onionkey = smartlist_get(args->args, 0); + if (add_onion_helper_keyarg(onionkey, discard_pk, &key_new_alg, &key_new_blob, &pk, &hs_version, &err_msg) < 0) { if (err_msg) { @@ -2031,12 +1972,6 @@ handle_control_add_onion(control_connection_t *conn, // Do not free entries; they are the same as auth_clients smartlist_free(auth_created_clients); } - - SMARTLIST_FOREACH(args, char *, cp, { - memwipe(cp, 0, strlen(cp)); - tor_free(cp); - }); - smartlist_free(args); return 0; } @@ -2471,28 +2406,28 @@ static const control_cmd_def_t CONTROL_COMMANDS[] = MULTLINE_PARSED(loadconf, 0), ONE_LINE_PARSED(setevents, 0), ONE_LINE(authenticate, legacy, CMD_FL_WIPE), - ONE_LINE(saveconf, legacy, 0), + ONE_LINE_PARSED(saveconf, 0), ONE_LINE_PARSED(signal, 0), ONE_LINE_PARSED(takeownership, 0), ONE_LINE_PARSED(dropownership, 0), - ONE_LINE(mapaddress, legacy, 0), + ONE_LINE_PARSED(mapaddress, 0), ONE_LINE_PARSED(getinfo, 0), ONE_LINE(extendcircuit, legacy, 0), - ONE_LINE(setcircuitpurpose, legacy, 0), + ONE_LINE_PARSED(setcircuitpurpose, 0), OBSOLETE(setrouterpurpose), - ONE_LINE(attachstream, legacy, 0), - MULTLINE(postdescriptor, legacy, 0), + ONE_LINE_PARSED(attachstream, 0), + MULTLINE_PARSED(postdescriptor, 0), ONE_LINE_PARSED(redirectstream, 0), ONE_LINE_PARSED(closestream, 0), - ONE_LINE(closecircuit, legacy, 0), + ONE_LINE_PARSED(closecircuit, 0), ONE_LINE_PARSED(usefeature, 0), - ONE_LINE(resolve, legacy, 0), + ONE_LINE_PARSED(resolve, 0), ONE_LINE_PARSED(protocolinfo, 0), ONE_LINE(authchallenge, legacy, CMD_FL_WIPE), ONE_LINE_PARSED(dropguards, 0), - ONE_LINE(hsfetch, legacy, 0), - MULTLINE(hspost, legacy, 0), - ONE_LINE(add_onion, legacy, CMD_FL_WIPE), + ONE_LINE_PARSED(hsfetch, 0), + MULTLINE_PARSED(hspost, 0), + ONE_LINE_PARSED(add_onion, CMD_FL_WIPE), ONE_LINE_PARSED(del_onion, CMD_FL_WIPE), }; From 95afdb005cce04cfb87df6a75980944173c15ed7 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 8 Apr 2019 16:19:09 -0400 Subject: [PATCH 15/24] Use new parser logic for SETCONF/RESETCONF code. Here we get to throw away a LOT of unused code, since most of the old parsing was redundant with kvline. --- src/feature/control/control_cmd.c | 108 +++++++----------------------- src/feature/control/control_fmt.c | 14 ---- src/feature/control/control_fmt.h | 2 - 3 files changed, 26 insertions(+), 98 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 1848555351..cfd35e8096 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -54,8 +54,8 @@ #include "feature/rend/rend_encoded_v2_service_descriptor_st.h" #include "feature/rend/rend_service_descriptor_st.h" -static int control_setconf_helper(control_connection_t *conn, uint32_t len, - char *body, +static int control_setconf_helper(control_connection_t *conn, + const control_cmd_args_t *args, int use_defaults); /** Yield true iff s is the state of a control_connection_t that has @@ -195,22 +195,36 @@ config_lines_contain_flag(const config_line_t *lines, const char *flag) return line && !strcmp(line->value, ""); } +static const control_cmd_syntax_t setconf_syntax = { + .max_args=0, + .accept_keywords=true, + .kvline_flags=KV_OMIT_VALS|KV_QUOTED, +}; + /** Called when we receive a SETCONF message: parse the body and try * to update our configuration. Reply with a DONE or ERROR message. * Modifies the contents of body.*/ static int -handle_control_setconf(control_connection_t *conn, uint32_t len, char *body) +handle_control_setconf(control_connection_t *conn, + const control_cmd_args_t *args) { - return control_setconf_helper(conn, len, body, 0); + return control_setconf_helper(conn, args, 0); } +static const control_cmd_syntax_t resetconf_syntax = { + .max_args=0, + .accept_keywords=true, + .kvline_flags=KV_OMIT_VALS|KV_QUOTED, +}; + /** Called when we receive a RESETCONF message: parse the body and try * to update our configuration. Reply with a DONE or ERROR message. * Modifies the contents of body. */ static int -handle_control_resetconf(control_connection_t *conn, uint32_t len, char *body) +handle_control_resetconf(control_connection_t *conn, + const control_cmd_args_t *args) { - return control_setconf_helper(conn, len, body, 1); + return control_setconf_helper(conn, args, 1); } static const control_cmd_syntax_t getconf_syntax = { @@ -524,73 +538,17 @@ get_stream(const char *id) * contents of body. */ static int -control_setconf_helper(control_connection_t *conn, uint32_t len, char *body, +control_setconf_helper(control_connection_t *conn, + const control_cmd_args_t *args, int use_defaults) { setopt_err_t opt_err; - config_line_t *lines=NULL; - char *start = body; char *errstring = NULL; const unsigned flags = CAL_CLEAR_FIRST | (use_defaults ? CAL_USE_DEFAULTS : 0); - char *config; - smartlist_t *entries = smartlist_new(); - - /* We have a string, "body", of the format '(key(=val|="val")?)' entries - * separated by space. break it into a list of configuration entries. */ - while (*body) { - char *eq = body; - char *key; - char *entry; - while (!TOR_ISSPACE(*eq) && *eq != '=') - ++eq; - key = tor_strndup(body, eq-body); - body = eq+1; - if (*eq == '=') { - char *val=NULL; - size_t val_len=0; - if (*body != '\"') { - char *val_start = body; - while (!TOR_ISSPACE(*body)) - body++; - val = tor_strndup(val_start, body-val_start); - val_len = strlen(val); - } else { - body = (char*)extract_escaped_string(body, (len - (body-start)), - &val, &val_len); - if (!body) { - connection_write_str_to_buf("551 Couldn't parse string\r\n", conn); - SMARTLIST_FOREACH(entries, char *, cp, tor_free(cp)); - smartlist_free(entries); - tor_free(key); - return 0; - } - } - tor_asprintf(&entry, "%s %s", key, val); - tor_free(key); - tor_free(val); - } else { - entry = key; - } - smartlist_add(entries, entry); - while (TOR_ISSPACE(*body)) - ++body; - } - - smartlist_add_strdup(entries, ""); - config = smartlist_join_strings(entries, "\n", 0, NULL); - SMARTLIST_FOREACH(entries, char *, cp, tor_free(cp)); - smartlist_free(entries); - - if (config_get_lines(config, &lines, 0) < 0) { - log_warn(LD_CONTROL,"Controller gave us config lines we can't parse."); - connection_write_str_to_buf("551 Couldn't parse configuration\r\n", - conn); - tor_free(config); - return 0; - } - tor_free(config); + // We need a copy here, since confparse.c wants to canonicalize cases. + config_line_t *lines = config_lines_dup(args->kwargs); opt_err = options_trial_assign(lines, flags, &errstring); { @@ -2276,7 +2234,6 @@ handle_control_obsolete(control_connection_t *conn, **/ typedef enum handler_type_t { hnd_legacy, - hnd_legacy_mut, hnd_parsed, } handler_type_t; @@ -2296,15 +2253,6 @@ typedef union handler_fn_t { int (*legacy)(control_connection_t *conn, uint32_t arg_len, const char *args); - /** - * A "legacy_mut" handler is the same as a "legacy" one, except that it may - * change the contents of the command's arguments -- for example, by - * inserting NULs. It may not deallocate them. - */ - int (*legacy_mut)(control_connection_t *conn, - uint32_t arg_len, - char *args); - /** * A "parsed" handler expects its arguments in a pre-parsed format, in * an immutable control_cmd_args_t *object. @@ -2400,8 +2348,8 @@ typedef struct control_cmd_def_t { **/ static const control_cmd_def_t CONTROL_COMMANDS[] = { - ONE_LINE(setconf, legacy_mut, 0), - ONE_LINE(resetconf, legacy_mut, 0), + ONE_LINE_PARSED(setconf, 0), + ONE_LINE_PARSED(resetconf, 0), ONE_LINE_PARSED(getconf, 0), MULTLINE_PARSED(loadconf, 0), ONE_LINE_PARSED(setevents, 0), @@ -2452,10 +2400,6 @@ handle_single_control_command(const control_cmd_def_t *def, if (def->handler.legacy(conn, cmd_data_len, args)) rv = -1; break; - case hnd_legacy_mut: - if (def->handler.legacy_mut(conn, cmd_data_len, args)) - rv = -1; - break; case hnd_parsed: { control_cmd_args_t *parsed_args; char *err=NULL; diff --git a/src/feature/control/control_fmt.c b/src/feature/control/control_fmt.c index 71f9d82163..427f4288fc 100644 --- a/src/feature/control/control_fmt.c +++ b/src/feature/control/control_fmt.c @@ -342,20 +342,6 @@ get_escaped_string_length(const char *start, size_t in_len_max, return (int)(cp - start+1); } -/** As decode_escaped_string, but does not decode the string: copies the - * entire thing, including quotation marks. */ -const char * -extract_escaped_string(const char *start, size_t in_len_max, - char **out, size_t *out_len) -{ - int length = get_escaped_string_length(start, in_len_max, NULL); - if (length<0) - return NULL; - *out_len = length; - *out = tor_strndup(start, *out_len); - return start+length; -} - /** Given a pointer to a string starting at start containing * in_len_max characters, decode a string beginning with one double * quote, containing any number of non-quote characters or characters escaped diff --git a/src/feature/control/control_fmt.h b/src/feature/control/control_fmt.h index 74545eb309..08acf85181 100644 --- a/src/feature/control/control_fmt.h +++ b/src/feature/control/control_fmt.h @@ -25,8 +25,6 @@ char *circuit_describe_status_for_controller(origin_circuit_t *circ); size_t write_escaped_data(const char *data, size_t len, char **out); size_t read_escaped_data(const char *data, size_t len, char **out); -const char *extract_escaped_string(const char *start, size_t in_len_max, - char **out, size_t *out_len); const char *decode_escaped_string(const char *start, size_t in_len_max, char **out, size_t *out_len); void send_control_done(control_connection_t *conn); From 0c0b869ba450363e36e8dd0bdacb4a197e0f0019 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 9 Apr 2019 09:28:57 -0400 Subject: [PATCH 16/24] Use the new controller command parser for EXTENDCIRCUIT. This command does not fit perfectly with the others, since its second argument is optional and may contain equal signs. Still, it's probably better to squeeze it into the new metaformat, since doing so allows us to remove several pieces of the old command-parsing machinery. --- src/feature/control/control_cmd.c | 152 +++++++++++------------------- 1 file changed, 54 insertions(+), 98 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index cfd35e8096..0b4f3555ff 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -594,7 +594,7 @@ address_is_invalid_mapaddress_target(const char *addr) } static const control_cmd_syntax_t mapaddress_syntax = { - .max_args=0, + .max_args=1, .accept_keywords=true, }; @@ -683,94 +683,61 @@ circuit_purpose_from_string(const char *string) return CIRCUIT_PURPOSE_UNKNOWN; } -/** Return a newly allocated smartlist containing the arguments to the command - * waiting in body. If there are fewer than min_args arguments, - * or if max_args is nonnegative and there are more than - * max_args arguments, send a 512 error to the controller, using - * command as the command name in the error message. */ -static smartlist_t * -getargs_helper(const char *command, control_connection_t *conn, - const char *body, int min_args, int max_args) -{ - smartlist_t *args = smartlist_new(); - smartlist_split_string(args, body, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); - if (smartlist_len(args) < min_args) { - connection_printf_to_buf(conn, "512 Missing argument to %s\r\n",command); - goto err; - } else if (max_args >= 0 && smartlist_len(args) > max_args) { - connection_printf_to_buf(conn, "512 Too many arguments to %s\r\n",command); - goto err; - } - return args; - err: - SMARTLIST_FOREACH(args, char *, s, tor_free(s)); - smartlist_free(args); - return NULL; -} - -/** Helper. Return the first element of sl at index start_at or - * higher that starts with prefix, case-insensitive. Return NULL if no - * such element exists. */ -static const char * -find_element_starting_with(smartlist_t *sl, int start_at, const char *prefix) -{ - int i; - for (i = start_at; i < smartlist_len(sl); ++i) { - const char *elt = smartlist_get(sl, i); - if (!strcasecmpstart(elt, prefix)) - return elt; - } - return NULL; -} - -/** Helper. Return true iff s is an argument that we should treat as a - * key-value pair. */ -static int -is_keyval_pair(const char *s) -{ - /* An argument is a key-value pair if it has an =, and it isn't of the form - * $fingeprint=name */ - return strchr(s, '=') && s[0] != '$'; -} +static const control_cmd_syntax_t extendcircuit_syntax = { + .min_args=1, + .max_args=1, // see note in function + .accept_keywords=true, + .kvline_flags=KV_OMIT_VALS +}; /** Called when we get an EXTENDCIRCUIT message. Try to extend the listed * circuit, and report success or failure. */ static int -handle_control_extendcircuit(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_extendcircuit(control_connection_t *conn, + const control_cmd_args_t *args) { - smartlist_t *router_nicknames=NULL, *nodes=NULL; + smartlist_t *router_nicknames=smartlist_new(), *nodes=NULL; origin_circuit_t *circ = NULL; - int zero_circ; uint8_t intended_purpose = CIRCUIT_PURPOSE_C_GENERAL; - smartlist_t *args; - (void) len; + const config_line_t *kwargs = args->kwargs; + const char *circ_id = smartlist_get(args->args, 0); + const char *path_str = NULL; + char *path_str_alloc = NULL; - router_nicknames = smartlist_new(); + /* The syntax for this command is unfortunate. The second argument is + optional, and is a comma-separated list long-format fingerprints, which + can (historically!) contain an equals sign. - args = getargs_helper("EXTENDCIRCUIT", conn, body, 1, -1); - if (!args) - goto done; + Here we check the second argument to see if it's a path, and if so we + remove it from the kwargs list and put it in path_str. + */ + if (kwargs) { + const config_line_t *arg1 = kwargs; + if (!strcmp(arg1->value, "")) { + path_str = arg1->key; + kwargs = kwargs->next; + } else if (arg1->key[0] == '$') { + tor_asprintf(&path_str_alloc, "%s=%s", arg1->key, arg1->value); + path_str = path_str_alloc; + kwargs = kwargs->next; + } + } - zero_circ = !strcmp("0", (char*)smartlist_get(args,0)); + const config_line_t *purpose_line = config_line_find_case(kwargs, "PURPOSE"); + bool zero_circ = !strcmp("0", circ_id); + + if (purpose_line) { + intended_purpose = circuit_purpose_from_string(purpose_line->value); + if (intended_purpose == CIRCUIT_PURPOSE_UNKNOWN) { + connection_printf_to_buf(conn, "552 Unknown purpose \"%s\"\r\n", + purpose_line->value); + goto done; + } + } if (zero_circ) { - const char *purp = find_element_starting_with(args, 1, "PURPOSE="); - - if (purp) { - intended_purpose = circuit_purpose_from_string(purp); - if (intended_purpose == CIRCUIT_PURPOSE_UNKNOWN) { - connection_printf_to_buf(conn, "552 Unknown purpose \"%s\"\r\n", purp); - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); - goto done; - } - } - - if ((smartlist_len(args) == 1) || - (smartlist_len(args) >= 2 && is_keyval_pair(smartlist_get(args, 1)))) { - // "EXTENDCIRCUIT 0" || EXTENDCIRCUIT 0 foo=bar" + if (!path_str) { + // "EXTENDCIRCUIT 0" with no path. circ = circuit_launch(intended_purpose, CIRCLAUNCH_NEED_CAPACITY); if (!circ) { connection_write_str_to_buf("551 Couldn't start circuit\r\n", conn); @@ -778,37 +745,24 @@ handle_control_extendcircuit(control_connection_t *conn, uint32_t len, connection_printf_to_buf(conn, "250 EXTENDED %lu\r\n", (unsigned long)circ->global_identifier); } - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); goto done; } - // "EXTENDCIRCUIT 0 router1,router2" || - // "EXTENDCIRCUIT 0 router1,router2 PURPOSE=foo" } - if (!zero_circ && !(circ = get_circ(smartlist_get(args,0)))) { - connection_printf_to_buf(conn, "552 Unknown circuit \"%s\"\r\n", - (char*)smartlist_get(args, 0)); - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); + if (!zero_circ && !(circ = get_circ(circ_id))) { + connection_printf_to_buf(conn, "552 Unknown circuit \"%s\"\r\n", circ_id); goto done; } - if (smartlist_len(args) < 2) { - connection_printf_to_buf(conn, - "512 syntax error: not enough arguments.\r\n"); - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); + if (!path_str) { + connection_printf_to_buf(conn, "512 syntax error: path required.\r\n"); goto done; } - smartlist_split_string(router_nicknames, smartlist_get(args,1), ",", 0, 0); - - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); + smartlist_split_string(router_nicknames, path_str, ",", 0, 0); nodes = smartlist_new(); - int first_node = zero_circ; + bool first_node = zero_circ; SMARTLIST_FOREACH_BEGIN(router_nicknames, const char *, n) { const node_t *node = node_get_by_nickname(n, 0); if (!node) { @@ -820,8 +774,9 @@ handle_control_extendcircuit(control_connection_t *conn, uint32_t len, goto done; } smartlist_add(nodes, (void*)node); - first_node = 0; + first_node = false; } SMARTLIST_FOREACH_END(n); + if (!smartlist_len(nodes)) { connection_write_str_to_buf("512 No router names provided\r\n", conn); goto done; @@ -887,6 +842,7 @@ handle_control_extendcircuit(control_connection_t *conn, uint32_t len, SMARTLIST_FOREACH(router_nicknames, char *, n, tor_free(n)); smartlist_free(router_nicknames); smartlist_free(nodes); + tor_free(path_str_alloc); return 0; } @@ -2360,7 +2316,7 @@ static const control_cmd_def_t CONTROL_COMMANDS[] = ONE_LINE_PARSED(dropownership, 0), ONE_LINE_PARSED(mapaddress, 0), ONE_LINE_PARSED(getinfo, 0), - ONE_LINE(extendcircuit, legacy, 0), + ONE_LINE_PARSED(extendcircuit, 0), ONE_LINE_PARSED(setcircuitpurpose, 0), OBSOLETE(setrouterpurpose), ONE_LINE_PARSED(attachstream, 0), From ba05324242aebdbf646ebb8e8f3aaef45b1f29ec Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 9 Apr 2019 09:42:51 -0400 Subject: [PATCH 17/24] Move and rename decode_escaped_string() This function decodes something different from the usual c-escaped format. It is only used in controller authorization. --- src/feature/control/control_auth.c | 7 ++- src/feature/control/control_fmt.c | 74 ------------------------ src/feature/control/control_fmt.h | 2 - src/lib/encoding/include.am | 2 + src/lib/encoding/qstring.c | 90 ++++++++++++++++++++++++++++++ src/lib/encoding/qstring.h | 18 ++++++ 6 files changed, 114 insertions(+), 79 deletions(-) create mode 100644 src/lib/encoding/qstring.c create mode 100644 src/lib/encoding/qstring.h diff --git a/src/feature/control/control_auth.c b/src/feature/control/control_auth.c index 927115a308..8204290489 100644 --- a/src/feature/control/control_auth.c +++ b/src/feature/control/control_auth.c @@ -17,6 +17,7 @@ #include "lib/crypt_ops/crypto_rand.h" #include "lib/crypt_ops/crypto_util.h" #include "lib/encoding/confline.h" +#include "lib/encoding/qstring.h" #include "lib/crypt_ops/crypto_s2k.h" @@ -149,8 +150,8 @@ handle_control_authchallenge(control_connection_t *conn, uint32_t len, cp += strspn(cp, " \t\n\r"); if (*cp == '"') { const char *newcp = - decode_escaped_string(cp, len - (cp - body), - &client_nonce, &client_nonce_len); + decode_qstring(cp, len - (cp - body), + &client_nonce, &client_nonce_len); if (newcp == NULL) { connection_write_str_to_buf("513 Invalid quoted client nonce\r\n", conn); @@ -275,7 +276,7 @@ handle_control_authenticate(control_connection_t *conn, uint32_t len, return 0; } } else { - if (!decode_escaped_string(body, len, &password, &password_len)) { + if (!decode_qstring(body, len, &password, &password_len)) { connection_write_str_to_buf("551 Invalid quoted string. You need " "to put the password in double quotes.\r\n", conn); connection_mark_for_close(TO_CONN(conn)); diff --git a/src/feature/control/control_fmt.c b/src/feature/control/control_fmt.c index 427f4288fc..b2ab4f10bb 100644 --- a/src/feature/control/control_fmt.c +++ b/src/feature/control/control_fmt.c @@ -305,80 +305,6 @@ send_control_done(control_connection_t *conn) connection_write_str_to_buf("250 OK\r\n", conn); } -/** If the first in_len_max characters in start contain a - * double-quoted string with escaped characters, return the length of that - * string (as encoded, including quotes). Otherwise return -1. */ -static inline int -get_escaped_string_length(const char *start, size_t in_len_max, - int *chars_out) -{ - const char *cp, *end; - int chars = 0; - - if (*start != '\"') - return -1; - - cp = start+1; - end = start+in_len_max; - - /* Calculate length. */ - while (1) { - if (cp >= end) { - return -1; /* Too long. */ - } else if (*cp == '\\') { - if (++cp == end) - return -1; /* Can't escape EOS. */ - ++cp; - ++chars; - } else if (*cp == '\"') { - break; - } else { - ++cp; - ++chars; - } - } - if (chars_out) - *chars_out = chars; - return (int)(cp - start+1); -} - -/** Given a pointer to a string starting at start containing - * in_len_max characters, decode a string beginning with one double - * quote, containing any number of non-quote characters or characters escaped - * with a backslash, and ending with a final double quote. Place the resulting - * string (unquoted, unescaped) into a newly allocated string in *out; - * store its length in out_len. On success, return a pointer to the - * character immediately following the escaped string. On failure, return - * NULL. */ -const char * -decode_escaped_string(const char *start, size_t in_len_max, - char **out, size_t *out_len) -{ - const char *cp, *end; - char *outp; - int len, n_chars = 0; - - len = get_escaped_string_length(start, in_len_max, &n_chars); - if (len<0) - return NULL; - - end = start+len-1; /* Index of last quote. */ - tor_assert(*end == '\"'); - outp = *out = tor_malloc(len+1); - *out_len = n_chars; - - cp = start+1; - while (cp < end) { - if (*cp == '\\') - ++cp; - *outp++ = *cp++; - } - *outp = '\0'; - tor_assert((outp - *out) == (int)*out_len); - - return end+1; -} - /** Return a longname the node whose identity is id_digest. If * node_get_by_id() returns NULL, base 16 encoding of id_digest is * returned instead. diff --git a/src/feature/control/control_fmt.h b/src/feature/control/control_fmt.h index 08acf85181..8bbbaa95d0 100644 --- a/src/feature/control/control_fmt.h +++ b/src/feature/control/control_fmt.h @@ -25,8 +25,6 @@ char *circuit_describe_status_for_controller(origin_circuit_t *circ); size_t write_escaped_data(const char *data, size_t len, char **out); size_t read_escaped_data(const char *data, size_t len, char **out); -const char *decode_escaped_string(const char *start, size_t in_len_max, - char **out, size_t *out_len); void send_control_done(control_connection_t *conn); MOCK_DECL(const char *, node_describe_longname_by_id,(const char *id_digest)); diff --git a/src/lib/encoding/include.am b/src/lib/encoding/include.am index 83e9211b6f..8272e4e5fa 100644 --- a/src/lib/encoding/include.am +++ b/src/lib/encoding/include.am @@ -11,6 +11,7 @@ src_lib_libtor_encoding_a_SOURCES = \ src/lib/encoding/keyval.c \ src/lib/encoding/kvline.c \ src/lib/encoding/pem.c \ + src/lib/encoding/qstring.c \ src/lib/encoding/time_fmt.c src_lib_libtor_encoding_testing_a_SOURCES = \ @@ -25,4 +26,5 @@ noinst_HEADERS += \ src/lib/encoding/keyval.h \ src/lib/encoding/kvline.h \ src/lib/encoding/pem.h \ + src/lib/encoding/qstring.h \ src/lib/encoding/time_fmt.h diff --git a/src/lib/encoding/qstring.c b/src/lib/encoding/qstring.c new file mode 100644 index 0000000000..a92d28c706 --- /dev/null +++ b/src/lib/encoding/qstring.c @@ -0,0 +1,90 @@ +/* Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2019, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +/** + * \file qstring.c + * \brief Implement QuotedString parsing. + * + * Note that this is only used for controller authentication; do not + * create new users for this. Instead, prefer the cstring.c functions. + **/ + +#include "orconfig.h" +#include "lib/encoding/qstring.h" +#include "lib/malloc/malloc.h" +#include "lib/log/util_bug.h" + +/** If the first in_len_max characters in start contain a + * QuotedString, return the length of that + * string (as encoded, including quotes). Otherwise return -1. */ +static inline int +get_qstring_length(const char *start, size_t in_len_max, + int *chars_out) +{ + const char *cp, *end; + int chars = 0; + + if (*start != '\"') + return -1; + + cp = start+1; + end = start+in_len_max; + + /* Calculate length. */ + while (1) { + if (cp >= end) { + return -1; /* Too long. */ + } else if (*cp == '\\') { + if (++cp == end) + return -1; /* Can't escape EOS. */ + ++cp; + ++chars; + } else if (*cp == '\"') { + break; + } else { + ++cp; + ++chars; + } + } + if (chars_out) + *chars_out = chars; + return (int)(cp - start+1); +} + +/** Given a pointer to a string starting at start containing + * in_len_max characters, decode a string beginning with one double + * quote, containing any number of non-quote characters or characters escaped + * with a backslash, and ending with a final double quote. Place the resulting + * string (unquoted, unescaped) into a newly allocated string in *out; + * store its length in out_len. On success, return a pointer to the + * character immediately following the escaped string. On failure, return + * NULL. */ +const char * +decode_qstring(const char *start, size_t in_len_max, + char **out, size_t *out_len) +{ + const char *cp, *end; + char *outp; + int len, n_chars = 0; + + len = get_qstring_length(start, in_len_max, &n_chars); + if (len<0) + return NULL; + + end = start+len-1; /* Index of last quote. */ + tor_assert(*end == '\"'); + outp = *out = tor_malloc(len+1); + *out_len = n_chars; + + cp = start+1; + while (cp < end) { + if (*cp == '\\') + ++cp; + *outp++ = *cp++; + } + *outp = '\0'; + tor_assert((outp - *out) == (int)*out_len); + + return end+1; +} diff --git a/src/lib/encoding/qstring.h b/src/lib/encoding/qstring.h new file mode 100644 index 0000000000..fe15b655f1 --- /dev/null +++ b/src/lib/encoding/qstring.h @@ -0,0 +1,18 @@ +/* Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2019, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +/** + * \file qstring.h + * \brief Header for qstring.c + */ + +#ifndef TOR_ENCODING_QSTRING_H +#define TOR_ENCODING_QSTRING_H + +#include + +const char *decode_qstring(const char *start, size_t in_len_max, + char **out, size_t *out_len); + +#endif From 8799b4e805ed5495409b6036b82d08e4624bacd3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 9 Apr 2019 09:49:03 -0400 Subject: [PATCH 18/24] Add rudimentary qstring support to kvline.c --- src/lib/encoding/kvline.c | 26 +++++++++++++++++++++----- src/lib/encoding/kvline.h | 1 + 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/lib/encoding/kvline.c b/src/lib/encoding/kvline.c index 806f9d3db0..d4a8f510ba 100644 --- a/src/lib/encoding/kvline.c +++ b/src/lib/encoding/kvline.c @@ -16,6 +16,7 @@ #include "lib/encoding/confline.h" #include "lib/encoding/cstring.h" #include "lib/encoding/kvline.h" +#include "lib/encoding/qstring.h" #include "lib/malloc/malloc.h" #include "lib/string/compat_ctype.h" #include "lib/string/printf.h" @@ -111,11 +112,15 @@ kvline_can_encode_lines(const config_line_t *line, unsigned flags) * If KV_OMIT_VALS is set in flags, then an empty value is * encoded as 'Key', not as 'Key=' or 'Key=""'. Mutually exclusive with * KV_OMIT_KEYS. + * + * KV_QUOTED_QSTRING is not supported. */ char * kvline_encode(const config_line_t *line, unsigned flags) { + tor_assert(! (flags & KV_QUOTED_QSTRING)); + if (!kvline_can_encode_lines(line, flags)) return NULL; @@ -170,7 +175,7 @@ kvline_encode(const config_line_t *line, * allocated list of pairs on success, or NULL on failure. * * If KV_QUOTED is set in flags, then (double-)quoted values are - * allowed. Otherwise, such values are not allowed. + * allowed and handled as C strings. Otherwise, such values are not allowed. * * If KV_OMIT_KEYS is set in flags, then values without keys are * allowed. Otherwise, such values are not allowed. @@ -178,6 +183,10 @@ kvline_encode(const config_line_t *line, * If KV_OMIT_VALS is set in flags, then keys without values are * allowed. Otherwise, such keys are not allowed. Mutually exclusive with * KV_OMIT_KEYS. + * + * If KV_QUOTED_QSTRING is set in flags, then double-quoted values + * are allowed and handled as QuotedStrings per qstring.c. Do not add + * new users of this flag. */ config_line_t * kvline_parse(const char *line, unsigned flags) @@ -188,7 +197,8 @@ kvline_parse(const char *line, unsigned flags) const char *cp = line, *cplast = NULL; const bool omit_keys = (flags & KV_OMIT_KEYS) != 0; const bool omit_vals = (flags & KV_OMIT_VALS) != 0; - const bool quoted = (flags & KV_QUOTED) != 0; + const bool quoted = (flags & (KV_QUOTED|KV_QUOTED_QSTRING)) != 0; + const bool c_quoted = (flags & (KV_QUOTED)) != 0; config_line_t *result = NULL; config_line_t **next_line = &result; @@ -236,7 +246,11 @@ kvline_parse(const char *line, unsigned flags) if (!quoted) goto err; size_t len=0; - cp = unescape_string(cp, &val, &len); + if (c_quoted) { + cp = unescape_string(cp, &val, &len); + } else { + cp = decode_qstring(cp, strlen(cp), &val, &len); + } if (cp == NULL || len != strlen(val)) { // The string contains a NUL or is badly coded. goto err; @@ -260,8 +274,10 @@ kvline_parse(const char *line, unsigned flags) key = val = NULL; } - if (!kvline_can_encode_lines(result, flags)) { - goto err; + if (! (flags & KV_QUOTED_QSTRING)) { + if (!kvline_can_encode_lines(result, flags)) { + goto err; + } } return result; diff --git a/src/lib/encoding/kvline.h b/src/lib/encoding/kvline.h index 6740f81d54..dea2ce1809 100644 --- a/src/lib/encoding/kvline.h +++ b/src/lib/encoding/kvline.h @@ -18,6 +18,7 @@ struct config_line_t; #define KV_QUOTED (1u<<0) #define KV_OMIT_KEYS (1u<<1) #define KV_OMIT_VALS (1u<<2) +#define KV_QUOTED_QSTRING (1u<<3) struct config_line_t *kvline_parse(const char *line, unsigned flags); char *kvline_encode(const struct config_line_t *line, unsigned flags); From ddd33d39c7e115045d296dd88fd4d310b932a4e1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 9 Apr 2019 10:09:38 -0400 Subject: [PATCH 19/24] Port the authenticate and authchallenge commands to the new parser These two presented their own challenge, because of their use of QString, and their distinguished handling of quoted versus non-quoted values. --- src/feature/control/control_auth.c | 143 ++++++++++++---------- src/feature/control/control_auth.h | 13 +- src/feature/control/control_cmd.c | 9 +- src/feature/control/control_cmd.h | 7 ++ src/feature/control/control_cmd_args_st.h | 4 + 5 files changed, 102 insertions(+), 74 deletions(-) diff --git a/src/feature/control/control_auth.c b/src/feature/control/control_auth.c index 8204290489..a86442c21f 100644 --- a/src/feature/control/control_auth.c +++ b/src/feature/control/control_auth.c @@ -11,12 +11,15 @@ #include "app/config/config.h" #include "core/mainloop/connection.h" #include "feature/control/control.h" +#include "feature/control/control_cmd.h" #include "feature/control/control_auth.h" +#include "feature/control/control_cmd_args_st.h" #include "feature/control/control_connection_st.h" #include "feature/control/control_fmt.h" #include "lib/crypt_ops/crypto_rand.h" #include "lib/crypt_ops/crypto_util.h" #include "lib/encoding/confline.h" +#include "lib/encoding/kvline.h" #include "lib/encoding/qstring.h" #include "lib/crypt_ops/crypto_s2k.h" @@ -117,12 +120,19 @@ decode_hashed_passwords(config_line_t *passwords) return NULL; } +const control_cmd_syntax_t authchallenge_syntax = { + .min_args = 1, + .max_args = 1, + .accept_keywords=true, + .kvline_flags=KV_OMIT_KEYS|KV_QUOTED_QSTRING, + .store_raw_body=true +}; + /** Called when we get an AUTHCHALLENGE command. */ int -handle_control_authchallenge(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_authchallenge(control_connection_t *conn, + const control_cmd_args_t *args) { - const char *cp = body; char *client_nonce; size_t client_nonce_len; char server_hash[DIGEST256_LEN]; @@ -130,63 +140,50 @@ handle_control_authchallenge(control_connection_t *conn, uint32_t len, char server_nonce[SAFECOOKIE_SERVER_NONCE_LEN]; char server_nonce_encoded[(2*SAFECOOKIE_SERVER_NONCE_LEN) + 1]; - cp += strspn(cp, " \t\n\r"); - if (!strcasecmpstart(cp, "SAFECOOKIE")) { - cp += strlen("SAFECOOKIE"); - } else { + if (strcasecmp(smartlist_get(args->args, 0), "SAFECOOKIE")) { connection_write_str_to_buf("513 AUTHCHALLENGE only supports SAFECOOKIE " "authentication\r\n", conn); - connection_mark_for_close(TO_CONN(conn)); - return -1; + goto fail; } - if (!authentication_cookie_is_set) { connection_write_str_to_buf("515 Cookie authentication is disabled\r\n", conn); - connection_mark_for_close(TO_CONN(conn)); - return -1; + goto fail; + } + if (args->kwargs == NULL || args->kwargs->next != NULL) { + /* connection_write_str_to_buf("512 AUTHCHALLENGE requires exactly " + "2 arguments.\r\n", conn); + */ + connection_printf_to_buf(conn, + "512 AUTHCHALLENGE dislikes argument list %s\r\n", + escaped(args->raw_body)); + goto fail; + } + if (strcmp(args->kwargs->key, "")) { + connection_write_str_to_buf("512 AUTHCHALLENGE does not accept keyword " + "arguments.\r\n", conn); + goto fail; } - cp += strspn(cp, " \t\n\r"); - if (*cp == '"') { - const char *newcp = - decode_qstring(cp, len - (cp - body), - &client_nonce, &client_nonce_len); - if (newcp == NULL) { - connection_write_str_to_buf("513 Invalid quoted client nonce\r\n", - conn); - connection_mark_for_close(TO_CONN(conn)); - return -1; - } - cp = newcp; + bool contains_quote = strchr(args->raw_body, '\"'); + if (contains_quote) { + /* The nonce was quoted */ + client_nonce = tor_strdup(args->kwargs->value); + client_nonce_len = strlen(client_nonce); } else { - size_t client_nonce_encoded_len = strspn(cp, "0123456789ABCDEFabcdef"); - - client_nonce_len = client_nonce_encoded_len / 2; - client_nonce = tor_malloc_zero(client_nonce_len); - - if (base16_decode(client_nonce, client_nonce_len, - cp, client_nonce_encoded_len) - != (int) client_nonce_len) { + /* The nonce was should be in hex. */ + const char *hex_nonce = args->kwargs->value; + client_nonce_len = strlen(hex_nonce) / 2; + client_nonce = tor_malloc(client_nonce_len); + if (base16_decode(client_nonce, client_nonce_len, hex_nonce, + strlen(hex_nonce)) != (int)client_nonce_len) { connection_write_str_to_buf("513 Invalid base16 client nonce\r\n", conn); - connection_mark_for_close(TO_CONN(conn)); tor_free(client_nonce); - return -1; + goto fail; } - - cp += client_nonce_encoded_len; } - cp += strspn(cp, " \t\n\r"); - if (*cp != '\0' || - cp != body + len) { - connection_write_str_to_buf("513 Junk at end of AUTHCHALLENGE command\r\n", - conn); - connection_mark_for_close(TO_CONN(conn)); - tor_free(client_nonce); - return -1; - } crypto_rand(server_nonce, SAFECOOKIE_SERVER_NONCE_LEN); /* Now compute and send the server-to-controller response, and the @@ -234,38 +231,56 @@ handle_control_authchallenge(control_connection_t *conn, uint32_t len, tor_free(client_nonce); return 0; + fail: + connection_mark_for_close(TO_CONN(conn)); + return -1; } +const control_cmd_syntax_t authenticate_syntax = { + .max_args = 0, + .accept_keywords=true, + .kvline_flags=KV_OMIT_KEYS|KV_QUOTED_QSTRING, + .store_raw_body=true +}; + /** Called when we get an AUTHENTICATE message. Check whether the * authentication is valid, and if so, update the connection's state to * OPEN. Reply with DONE or ERROR. */ int -handle_control_authenticate(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_authenticate(control_connection_t *conn, + const control_cmd_args_t *args) { - int used_quoted_string = 0; + bool used_quoted_string = false; const or_options_t *options = get_options(); const char *errstr = "Unknown error"; char *password; size_t password_len; - const char *cp; - int i; int bad_cookie=0, bad_password=0; smartlist_t *sl = NULL; - if (!len) { + if (args->kwargs == NULL) { password = tor_strdup(""); password_len = 0; - } else if (TOR_ISXDIGIT(body[0])) { - cp = body; - while (TOR_ISXDIGIT(*cp)) - ++cp; - i = (int)(cp - body); - tor_assert(i>0); - password_len = i/2; - password = tor_malloc(password_len + 1); - if (base16_decode(password, password_len+1, body, i) + } else if (args->kwargs->next) { + connection_write_str_to_buf( + "512 Too many arguments to AUTHENTICATE.\r\n", conn); + connection_mark_for_close(TO_CONN(conn)); + return 0; + } else if (strcmp(args->kwargs->key, "")) { + connection_write_str_to_buf( + "512 AUTHENTICATE does not accept keyword arguments.\r\n", conn); + connection_mark_for_close(TO_CONN(conn)); + return 0; + } else if (strchr(args->raw_body, '\"')) { + used_quoted_string = true; + password = tor_strdup(args->kwargs->value); + password_len = strlen(password); + } else { + const char *hex_passwd = args->kwargs->value; + password_len = strlen(hex_passwd) / 2; + password = tor_malloc(password_len+1); + if (base16_decode(password, password_len+1, hex_passwd, strlen(hex_passwd)) != (int) password_len) { connection_write_str_to_buf( "551 Invalid hexadecimal encoding. Maybe you tried a plain text " @@ -275,14 +290,6 @@ handle_control_authenticate(control_connection_t *conn, uint32_t len, tor_free(password); return 0; } - } else { - if (!decode_qstring(body, len, &password, &password_len)) { - connection_write_str_to_buf("551 Invalid quoted string. You need " - "to put the password in double quotes.\r\n", conn); - connection_mark_for_close(TO_CONN(conn)); - return 0; - } - used_quoted_string = 1; } if (conn->safecookie_client_hash != NULL) { diff --git a/src/feature/control/control_auth.h b/src/feature/control/control_auth.h index f436482e4a..246e18ccbc 100644 --- a/src/feature/control/control_auth.h +++ b/src/feature/control/control_auth.h @@ -12,16 +12,21 @@ #ifndef TOR_CONTROL_AUTH_H #define TOR_CONTROL_AUTH_H +struct control_cmd_args_t; +struct control_cmd_syntax_t; + int init_control_cookie_authentication(int enabled); char *get_controller_cookie_file_name(void); struct config_line_t; smartlist_t *decode_hashed_passwords(struct config_line_t *passwords); -int handle_control_authchallenge(control_connection_t *conn, uint32_t len, - const char *body); +int handle_control_authchallenge(control_connection_t *conn, + const struct control_cmd_args_t *args); int handle_control_authenticate(control_connection_t *conn, - uint32_t cmd_data_len, - const char *args); + const struct control_cmd_args_t *args); void control_auth_free_all(void); +extern const struct control_cmd_syntax_t authchallenge_syntax; +extern const struct control_cmd_syntax_t authenticate_syntax; + #endif /* !defined(TOR_CONTROL_AUTH_H) */ diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 0b4f3555ff..a29ab72ffa 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -116,6 +116,11 @@ control_cmd_parse_args(const char *command, result->command = command; + if (syntax->store_raw_body) { + tor_assert(body[body_len] == 0); + result->raw_body = body; + } + const char *eol = memchr(body, '\n', body_len); if (syntax->want_object) { if (! eol || (eol+1) == body+body_len) { @@ -2309,7 +2314,7 @@ static const control_cmd_def_t CONTROL_COMMANDS[] = ONE_LINE_PARSED(getconf, 0), MULTLINE_PARSED(loadconf, 0), ONE_LINE_PARSED(setevents, 0), - ONE_LINE(authenticate, legacy, CMD_FL_WIPE), + ONE_LINE_PARSED(authenticate, CMD_FL_WIPE), ONE_LINE_PARSED(saveconf, 0), ONE_LINE_PARSED(signal, 0), ONE_LINE_PARSED(takeownership, 0), @@ -2327,7 +2332,7 @@ static const control_cmd_def_t CONTROL_COMMANDS[] = ONE_LINE_PARSED(usefeature, 0), ONE_LINE_PARSED(resolve, 0), ONE_LINE_PARSED(protocolinfo, 0), - ONE_LINE(authchallenge, legacy, CMD_FL_WIPE), + ONE_LINE_PARSED(authchallenge, CMD_FL_WIPE), ONE_LINE_PARSED(dropguards, 0), ONE_LINE_PARSED(hsfetch, 0), MULTLINE_PARSED(hspost, 0), diff --git a/src/feature/control/control_cmd.h b/src/feature/control/control_cmd.h index 6f35c74de0..b825d6da5c 100644 --- a/src/feature/control/control_cmd.h +++ b/src/feature/control/control_cmd.h @@ -64,6 +64,13 @@ typedef struct control_cmd_syntax_t { * True iff this command wants to be followed by a multiline object. **/ bool want_object; + /** + * True iff this command needs access to the raw body of the input. + * + * This should not be needed for pure commands; it is purely a legacy + * option. + **/ + bool store_raw_body; } control_cmd_syntax_t; #ifdef CONTROL_CMD_PRIVATE diff --git a/src/feature/control/control_cmd_args_st.h b/src/feature/control/control_cmd_args_st.h index 06e2a183ba..fb4fe64a08 100644 --- a/src/feature/control/control_cmd_args_st.h +++ b/src/feature/control/control_cmd_args_st.h @@ -43,6 +43,10 @@ struct control_cmd_args_t { * A multiline object passed with this command. **/ char *object; + /** + * If set, a nul-terminated string containing the raw unparsed arguments. + **/ + const char *raw_body; }; #endif /* !defined(TOR_CONTROL_CMD_ST_H) */ From 88d22b898efa5bfce82c614e454874d8807e2104 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 9 Apr 2019 10:58:42 -0400 Subject: [PATCH 20/24] Simplify handler logic in control_cmd.c Now that the legacy handlers are gone, we can simplify the structures and macros here. --- src/feature/control/control_cmd.c | 188 +++++++++++------------------- 1 file changed, 66 insertions(+), 122 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index a29ab72ffa..c3863c6461 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -2171,16 +2171,18 @@ handle_control_del_onion(control_connection_t *conn, return 0; } +static const control_cmd_syntax_t obsolete_syntax = { + .max_args = UINT_MAX +}; + /** * Called when we get an obsolete command: tell the controller that it is * obsolete. */ static int handle_control_obsolete(control_connection_t *conn, - uint32_t arg_len, - const char *args) + const control_cmd_args_t *args) { - (void)arg_len; (void)args; char *command = tor_strdup(conn->current_cmd); tor_strupper(command); @@ -2190,37 +2192,10 @@ handle_control_obsolete(control_connection_t *conn, } /** - * Selects an API to a controller command. See handler_fn_t for the - * possible types. + * Function pointer to a handler function for a controller command. **/ -typedef enum handler_type_t { - hnd_legacy, - hnd_parsed, -} handler_type_t; - -/** - * Union: a function pointer to a handler function for a controller command. - * - * This needs to be a union (rather than just a single pointer) since not - * all controller commands have the same type. - **/ -typedef union handler_fn_t { - /** - * A "legacy" handler takes a command's arguments as a nul-terminated - * string, and their length. It may not change the contents of the - * arguments. If the command is a multiline one, then the arguments may - * extend across multiple lines. - */ - int (*legacy)(control_connection_t *conn, - uint32_t arg_len, - const char *args); - /** - * A "parsed" handler expects its arguments in a pre-parsed format, in - * an immutable control_cmd_args_t *object. - **/ - int (*parsed)(control_connection_t *conn, - const control_cmd_args_t *args); -} handler_fn_t; +typedef int (*handler_fn_t) (control_connection_t *conn, + const control_cmd_args_t *args); /** * Definition for a controller command. @@ -2230,10 +2205,6 @@ typedef struct control_cmd_def_t { * The name of the command. If the command is multiline, the name must * begin with "+". This is not case-sensitive. */ const char *name; - /** - * Which API to use when calling the handler function. - */ - handler_type_t handler_type; /** * A function to execute the command. */ @@ -2254,54 +2225,37 @@ typedef struct control_cmd_def_t { */ #define CMD_FL_WIPE (1u<<0) -#define SYNTAX_IGNORE { 0, UINT_MAX, false } - /** Macro: declare a command with a one-line argument, a given set of flags, * and a syntax definition. **/ -#define ONE_LINE_(name, htype, flags, syntax) \ - { #name, \ - hnd_ ##htype, \ - { .htype = handle_control_ ##name }, \ +#define ONE_LINE(name, flags) \ + { \ + #name, \ + handle_control_ ##name, \ flags, \ - syntax, \ - } - -/** Macro: declare a parsed command with a one-line argument, a given set of - * flags, and a syntax definition. - **/ -#define ONE_LINE(name, htype, flags) \ - ONE_LINE_(name, htype, flags, NULL) -#define ONE_LINE_PARSED(name, flags) \ - ONE_LINE_(name, parsed, flags, &name ##_syntax) + &name##_syntax, \ + } /** * Macro: declare a command with a multi-line argument and a given set of * flags. **/ -#define MULTLINE_(name, htype, flags, syntax) \ +#define MULTLINE(name, flags) \ { "+"#name, \ - hnd_ ##htype, \ - { .htype = handle_control_ ##name }, \ + handle_control_ ##name, \ flags, \ - syntax \ + &name##_syntax \ } -#define MULTLINE(name, htype, flags) \ - MULTLINE_(name, htype, flags, NULL) -#define MULTLINE_PARSED(name, flags) \ - MULTLINE_(name, parsed, flags, &name##_syntax) - /** * Macro: declare an obsolete command. (Obsolete commands give a different * error than non-existent ones.) **/ #define OBSOLETE(name) \ { #name, \ - hnd_legacy, \ - { .legacy = handle_control_obsolete }, \ + handle_control_obsolete, \ 0, \ - NULL, \ + &obsolete_syntax, \ } /** @@ -2309,35 +2263,35 @@ typedef struct control_cmd_def_t { **/ static const control_cmd_def_t CONTROL_COMMANDS[] = { - ONE_LINE_PARSED(setconf, 0), - ONE_LINE_PARSED(resetconf, 0), - ONE_LINE_PARSED(getconf, 0), - MULTLINE_PARSED(loadconf, 0), - ONE_LINE_PARSED(setevents, 0), - ONE_LINE_PARSED(authenticate, CMD_FL_WIPE), - ONE_LINE_PARSED(saveconf, 0), - ONE_LINE_PARSED(signal, 0), - ONE_LINE_PARSED(takeownership, 0), - ONE_LINE_PARSED(dropownership, 0), - ONE_LINE_PARSED(mapaddress, 0), - ONE_LINE_PARSED(getinfo, 0), - ONE_LINE_PARSED(extendcircuit, 0), - ONE_LINE_PARSED(setcircuitpurpose, 0), + ONE_LINE(setconf, 0), + ONE_LINE(resetconf, 0), + ONE_LINE(getconf, 0), + MULTLINE(loadconf, 0), + ONE_LINE(setevents, 0), + ONE_LINE(authenticate, CMD_FL_WIPE), + ONE_LINE(saveconf, 0), + ONE_LINE(signal, 0), + ONE_LINE(takeownership, 0), + ONE_LINE(dropownership, 0), + ONE_LINE(mapaddress, 0), + ONE_LINE(getinfo, 0), + ONE_LINE(extendcircuit, 0), + ONE_LINE(setcircuitpurpose, 0), OBSOLETE(setrouterpurpose), - ONE_LINE_PARSED(attachstream, 0), - MULTLINE_PARSED(postdescriptor, 0), - ONE_LINE_PARSED(redirectstream, 0), - ONE_LINE_PARSED(closestream, 0), - ONE_LINE_PARSED(closecircuit, 0), - ONE_LINE_PARSED(usefeature, 0), - ONE_LINE_PARSED(resolve, 0), - ONE_LINE_PARSED(protocolinfo, 0), - ONE_LINE_PARSED(authchallenge, CMD_FL_WIPE), - ONE_LINE_PARSED(dropguards, 0), - ONE_LINE_PARSED(hsfetch, 0), - MULTLINE_PARSED(hspost, 0), - ONE_LINE_PARSED(add_onion, CMD_FL_WIPE), - ONE_LINE_PARSED(del_onion, CMD_FL_WIPE), + ONE_LINE(attachstream, 0), + MULTLINE(postdescriptor, 0), + ONE_LINE(redirectstream, 0), + ONE_LINE(closestream, 0), + ONE_LINE(closecircuit, 0), + ONE_LINE(usefeature, 0), + ONE_LINE(resolve, 0), + ONE_LINE(protocolinfo, 0), + ONE_LINE(authchallenge, CMD_FL_WIPE), + ONE_LINE(dropguards, 0), + ONE_LINE(hsfetch, 0), + MULTLINE(hspost, 0), + ONE_LINE(add_onion, CMD_FL_WIPE), + ONE_LINE(del_onion, CMD_FL_WIPE), }; /** @@ -2356,35 +2310,25 @@ handle_single_control_command(const control_cmd_def_t *def, char *args) { int rv = 0; - switch (def->handler_type) { - case hnd_legacy: - if (def->handler.legacy(conn, cmd_data_len, args)) - rv = -1; - break; - case hnd_parsed: { - control_cmd_args_t *parsed_args; - char *err=NULL; - tor_assert(def->syntax); - 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->current_cmd, err?err:""); - tor_free(err); - } else { - if (BUG(err)) - tor_free(err); - if (def->handler.parsed(conn, parsed_args)) - rv = 0; - control_cmd_args_free(parsed_args); - } - break; - } - default: - tor_assert_unreached(); + + control_cmd_args_t *parsed_args; + char *err=NULL; + tor_assert(def->syntax); + 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->current_cmd, err?err:""); + tor_free(err); + } else { + if (BUG(err)) + tor_free(err); + if (def->handler(conn, parsed_args)) + rv = 0; + control_cmd_args_free(parsed_args); } if (def->flags & CMD_FL_WIPE) From ff9ba7d6c4ba7072b9a96d527959350ef90406b3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 9 Apr 2019 11:10:26 -0400 Subject: [PATCH 21/24] expand CMD_FL_WIPE to wipe the parsed arguments too --- src/feature/control/control_cmd.c | 22 ++++++++++++++++++++++ src/feature/control/control_cmd.h | 1 + 2 files changed, 23 insertions(+) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index c3863c6461..7a9af19cd0 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -81,6 +81,24 @@ control_cmd_args_free_(control_cmd_args_t *args) tor_free(args); } +/** Erase all memory held in args. */ +void +control_cmd_args_wipe(control_cmd_args_t *args) +{ + if (!args) + return; + + if (args->args) { + SMARTLIST_FOREACH(args->args, char *, c, memwipe(c, 0, strlen(c))); + } + for (config_line_t *line = args->kwargs; line; line = line->next) { + memwipe(line->key, 0, strlen(line->key)); + memwipe(line->value, 0, strlen(line->value)); + } + if (args->object) + memwipe(args->object, 0, args->object_len); +} + /** * Return true iff any element of the NULL-terminated array matches * kwd. Case-insensitive. @@ -2328,6 +2346,10 @@ handle_single_control_command(const control_cmd_def_t *def, tor_free(err); if (def->handler(conn, parsed_args)) rv = 0; + + if (def->flags & CMD_FL_WIPE) + control_cmd_args_wipe(parsed_args); + control_cmd_args_free(parsed_args); } diff --git a/src/feature/control/control_cmd.h b/src/feature/control/control_cmd.h index b825d6da5c..986718887c 100644 --- a/src/feature/control/control_cmd.h +++ b/src/feature/control/control_cmd.h @@ -21,6 +21,7 @@ void control_cmd_free_all(void); typedef struct control_cmd_args_t control_cmd_args_t; void control_cmd_args_free_(control_cmd_args_t *args); +void control_cmd_args_wipe(control_cmd_args_t *args); #define control_cmd_args_free(v) \ FREE_AND_NULL(control_cmd_args_t, control_cmd_args_free_, (v)) From 3ed7ceeb856b3c1fbe5edf3b8c6ffe3f5e875622 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 9 Apr 2019 11:18:18 -0400 Subject: [PATCH 22/24] changes file for ticket 30091 (controller parsing refactor) --- changes/ticket30091 | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/ticket30091 diff --git a/changes/ticket30091 b/changes/ticket30091 new file mode 100644 index 0000000000..968ea01f4a --- /dev/null +++ b/changes/ticket30091 @@ -0,0 +1,4 @@ + o Major features (controller protocol): + - Controller commands are now parsed using a generalized parsing + subsystem. Previously, each controller command was responsible for + parsing its own input. Closes ticket 30091. From a0299cd240292d93b0f465bf1845a5e54889f61b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 24 Apr 2019 11:32:14 -0400 Subject: [PATCH 23/24] In control command api, rename "object" to "cmddata" This makes it match control-spec.txt. --- src/feature/control/control_cmd.c | 30 +++++++++++------------ src/feature/control/control_cmd.h | 2 +- src/feature/control/control_cmd_args_st.h | 6 ++--- src/test/test_controller.c | 6 ++--- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 7a9af19cd0..da9a95fc5d 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -76,7 +76,7 @@ control_cmd_args_free_(control_cmd_args_t *args) smartlist_free(args->args); } config_free_lines(args->kwargs); - tor_free(args->object); + tor_free(args->cmddata); tor_free(args); } @@ -95,8 +95,8 @@ control_cmd_args_wipe(control_cmd_args_t *args) memwipe(line->key, 0, strlen(line->key)); memwipe(line->value, 0, strlen(line->value)); } - if (args->object) - memwipe(args->object, 0, args->object_len); + if (args->cmddata) + memwipe(args->cmddata, 0, args->cmddata_len); } /** @@ -140,7 +140,7 @@ control_cmd_parse_args(const char *command, } const char *eol = memchr(body, '\n', body_len); - if (syntax->want_object) { + if (syntax->want_cmddata) { if (! eol || (eol+1) == body+body_len) { *error_out = tor_strdup("Empty body"); goto err; @@ -148,8 +148,8 @@ control_cmd_parse_args(const char *command, cmdline_alloc = tor_memdup_nulterm(body, eol-body); cmdline = cmdline_alloc; ++eol; - result->object_len = read_escaped_data(eol, (body+body_len)-eol, - &result->object); + result->cmddata_len = read_escaped_data(eol, (body+body_len)-eol, + &result->cmddata); } else { if (eol && (eol+1) != body+body_len) { *error_out = tor_strdup("Unexpected body"); @@ -320,7 +320,7 @@ handle_control_getconf(control_connection_t *conn, } static const control_cmd_syntax_t loadconf_syntax = { - .want_object = true + .want_cmddata = true }; /** Called when we get a +LOADCONF message. */ @@ -332,7 +332,7 @@ handle_control_loadconf(control_connection_t *conn, char *errstring = NULL; const char *msg = NULL; - retval = options_init_from_string(NULL, args->object, + retval = options_init_from_string(NULL, args->cmddata, CMD_RUN_TOR, NULL, &errstring); if (retval != SETOPT_OK) @@ -1013,7 +1013,7 @@ static const control_cmd_syntax_t postdescriptor_syntax = { .max_args = 0, .accept_keywords = true, .allowed_keywords = postdescriptor_keywords, - .want_object = true, + .want_cmddata = true, }; /** Called when we get a POSTDESCRIPTOR message. Try to learn the provided @@ -1047,7 +1047,7 @@ handle_control_postdescriptor(control_connection_t *conn, } } - switch (router_load_single_router(args->object, purpose, cache, &msg)) { + switch (router_load_single_router(args->cmddata, purpose, cache, &msg)) { case -1: if (!msg) msg = "Could not parse descriptor"; connection_printf_to_buf(conn, "554 %s\r\n", msg); @@ -1366,7 +1366,7 @@ static const control_cmd_syntax_t hsfetch_syntax = { .min_args = 1, .max_args = 1, .accept_keywords = true, .allowed_keywords = hsfetch_keywords, - .want_object = true, + .want_cmddata = true, }; /** Implementation for the HSFETCH command. */ @@ -1420,7 +1420,7 @@ handle_control_hsfetch(control_connection_t *conn, goto done; } if (!hsdirs) { - /* Stores routerstatus_t object for each specified server. */ + /* Stores routerstatus_t cmddata for each specified server. */ hsdirs = smartlist_new(); } /* Valid server, add it to our local list. */ @@ -1473,7 +1473,7 @@ static const char *hspost_keywords[] = { static const control_cmd_syntax_t hspost_syntax = { .min_args = 0, .max_args = 0, .accept_keywords = true, - .want_object = true, + .want_cmddata = true, .allowed_keywords = hspost_keywords }; @@ -1483,8 +1483,8 @@ handle_control_hspost(control_connection_t *conn, const control_cmd_args_t *args) { smartlist_t *hs_dirs = NULL; - const char *encoded_desc = args->object; - size_t encoded_desc_len = args->object_len; + const char *encoded_desc = args->cmddata; + size_t encoded_desc_len = args->cmddata_len; const char *onion_address = NULL; const config_line_t *line; diff --git a/src/feature/control/control_cmd.h b/src/feature/control/control_cmd.h index 986718887c..5c3d1a1cec 100644 --- a/src/feature/control/control_cmd.h +++ b/src/feature/control/control_cmd.h @@ -64,7 +64,7 @@ typedef struct control_cmd_syntax_t { /** * True iff this command wants to be followed by a multiline object. **/ - bool want_object; + bool want_cmddata; /** * True iff this command needs access to the raw body of the input. * diff --git a/src/feature/control/control_cmd_args_st.h b/src/feature/control/control_cmd_args_st.h index fb4fe64a08..8d7a4f55b3 100644 --- a/src/feature/control/control_cmd_args_st.h +++ b/src/feature/control/control_cmd_args_st.h @@ -36,13 +36,13 @@ struct control_cmd_args_t { **/ struct config_line_t *kwargs; /** - * Number of bytes in object; 0 if object is not set. + * Number of bytes in cmddata; 0 if cmddata is not set. **/ - size_t object_len; + size_t cmddata_len; /** * A multiline object passed with this command. **/ - char *object; + char *cmddata; /** * If set, a nul-terminated string containing the raw unparsed arguments. **/ diff --git a/src/test/test_controller.c b/src/test/test_controller.c index fd4f26f086..c130248592 100644 --- a/src/test/test_controller.c +++ b/src/test/test_controller.c @@ -56,9 +56,9 @@ control_cmd_dump_args(const control_cmd_args_t *result) } SMARTLIST_FOREACH_END(s); } buf_add_string(buf, "]"); - if (result->object) { + if (result->cmddata) { buf_add_string(buf, ", obj="); - buf_add_string(buf, escaped(result->object)); + buf_add_string(buf, escaped(result->cmddata)); } if (result->kwargs) { buf_add_string(buf, ", { "); @@ -160,7 +160,7 @@ static const parser_testcase_t no_args_one_obj_tests[] = { }; static const control_cmd_syntax_t no_args_one_obj_syntax = { .min_args=0, .max_args=0, - .want_object=true, + .want_cmddata=true, }; static const parse_test_params_t parse_no_args_one_obj_params = TESTPARAMS( no_args_one_obj_syntax, no_args_one_obj_tests ); From a5cced2b7acf4fb1818d6cd84c09a9ffe4dd1e89 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 24 Apr 2019 11:42:10 -0400 Subject: [PATCH 24/24] Extract keyword argument checking from argument parsing. --- src/feature/control/control_cmd.c | 53 +++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index da9a95fc5d..9afa734d86 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -113,6 +113,41 @@ string_array_contains_keyword(const char **array, const char *kwd) return false; } +/** Helper for argument parsing: check whether the keyword arguments just + * parsed in result were well-formed according to syntax. + * + * On success, return 0. On failure, return -1 and set *error_out + * to a newly allocated error string. + **/ +static int +kvline_check_keyword_args(const control_cmd_args_t *result, + const control_cmd_syntax_t *syntax, + char **error_out) +{ + if (result->kwargs == NULL) { + tor_asprintf(error_out, "Cannot parse keyword argument(s)"); + return -1; + } + + if (! syntax->allowed_keywords) { + /* All keywords are permitted. */ + return 0; + } + + /* Check for unpermitted arguments */ + const config_line_t *line; + for (line = result->kwargs; line; line = line->next) { + if (! string_array_contains_keyword(syntax->allowed_keywords, + line->key)) { + tor_asprintf(error_out, "Unrecognized keyword argument %s", + escaped(line->key)); + return -1; + } + } + + return 0; +} + /** * Helper: parse the arguments to a command according to syntax. On * success, set *error_out to NULL and return a newly allocated @@ -174,27 +209,17 @@ control_cmd_parse_args(const char *command, } if (n_args > syntax->max_args) { + /* We have extra arguments after the positional arguments, and we didn't + treat them as an error, so they must count as keyword arguments: Either + K=V pairs, or flags, or both. */ tor_assert(n_args == syntax->max_args + 1); tor_assert(syntax->accept_keywords); char *remainder = smartlist_pop_last(result->args); result->kwargs = kvline_parse(remainder, syntax->kvline_flags); tor_free(remainder); - if (result->kwargs == NULL) { - tor_asprintf(error_out, "Cannot parse keyword argument(s)"); + if (kvline_check_keyword_args(result, syntax, error_out) < 0) { goto err; } - if (syntax->allowed_keywords) { - /* Check for unpermitted arguments */ - const config_line_t *line; - for (line = result->kwargs; line; line = line->next) { - if (! string_array_contains_keyword(syntax->allowed_keywords, - line->key)) { - tor_asprintf(error_out, "Unrecognized keyword argument %s", - escaped(line->key)); - goto err; - } - } - } } tor_assert_nonfatal(*error_out == NULL);