From 3da9aa6b6792407d57816ec4a4cdfd0b4b3434b8 Mon Sep 17 00:00:00 2001 From: Neels Hofmeyr Date: Tue, 26 Sep 2017 14:21:44 +0200 Subject: ctrl: tighten CTRL input parsing Validate that incoming CTRL commands... - have decimal IDs, - return error on trailing characters, - have invalid characters in variable identifiers, - send detailed error messages as reply to the requestor. Adjust ctrl_test.{c,ok}, which best show the change in behavior. Message handling causes log messages on stderr; previously, stderr was empty. Add '[ignore]' in testsuite.at so that the nonempty stderr doesn't cause test failures. Change-Id: I96a9b6b6a3a5e0b80513aa9eaa727ae8c9c7d7a1 --- src/ctrl/control_cmd.c | 77 ++++++++++++++++++++++++++++++--- tests/ctrl/ctrl_test.c | 110 ++++++++++++++++++++---------------------------- tests/ctrl/ctrl_test.ok | 76 ++++++++++++--------------------- tests/testsuite.at | 2 +- 4 files changed, 144 insertions(+), 121 deletions(-) diff --git a/src/ctrl/control_cmd.c b/src/ctrl/control_cmd.c index c2ce2be4..f32a200c 100644 --- a/src/ctrl/control_cmd.c +++ b/src/ctrl/control_cmd.c @@ -281,6 +281,15 @@ struct ctrl_cmd *ctrl_cmd_parse(void *ctx, struct msgb *msg) return res; } +static bool id_str_valid(const char *str) +{ + for (;*str;str++) { + if (!isdigit(*str)) + return false; + } + return true; +} + /*! Parse CTRL command struct from msgb, return ctrl->type == CTRL_TYPE_ERROR and an error message in * ctrl->reply on any error. * The caller is responsible to talloc_free() the returned struct pointer. */ @@ -306,6 +315,7 @@ struct ctrl_cmd *ctrl_cmd_parse2(void *ctx, struct msgb *msg) cmd->type = CTRL_TYPE_ERROR; cmd->id = "err"; cmd->reply = "Request malformed"; + LOGP(DLCTRL, LOGL_NOTICE, "Malformed request: \"%s\"\n", osmo_escape_str(str, -1)); goto err; } @@ -314,6 +324,7 @@ struct ctrl_cmd *ctrl_cmd_parse2(void *ctx, struct msgb *msg) cmd->type = CTRL_TYPE_ERROR; cmd->id = "err"; cmd->reply = "Request type unknown"; + LOGP(DLCTRL, LOGL_NOTICE, "Request type unknown: \"%s\"\n", osmo_escape_str(str, -1)); goto err; } @@ -323,6 +334,15 @@ struct ctrl_cmd *ctrl_cmd_parse2(void *ctx, struct msgb *msg) cmd->type = CTRL_TYPE_ERROR; cmd->id = "err"; cmd->reply = "Missing ID"; + LOGP(DLCTRL, LOGL_NOTICE, "Missing ID: \"%s\"\n", osmo_escape_str(str, -1)); + goto err; + } + + if (!id_str_valid(tmp)) { + cmd->type = CTRL_TYPE_ERROR; + cmd->id = "err"; + cmd->reply = "Invalid message ID number"; + LOGP(DLCTRL, LOGL_NOTICE, "Invalid message ID number: \"%s\"\n", osmo_escape_str(tmp, -1)); goto err; } cmd->id = talloc_strdup(cmd, tmp); @@ -331,14 +351,30 @@ struct ctrl_cmd *ctrl_cmd_parse2(void *ctx, struct msgb *msg) switch (cmd->type) { case CTRL_TYPE_GET: - var = strtok_r(NULL, " ", &saveptr); + var = strtok_r(NULL, " \n", &saveptr); if (!var) { cmd->type = CTRL_TYPE_ERROR; cmd->reply = "GET incomplete"; - LOGP(DLCTRL, LOGL_NOTICE, "GET Command incomplete\n"); + LOGP(DLCTRL, LOGL_NOTICE, "GET Command incomplete: \"%s\"\n", + osmo_escape_str(str, -1)); + goto err; + } + if (!osmo_separated_identifiers_valid(var, ".")) { + cmd->type = CTRL_TYPE_ERROR; + cmd->reply = "GET variable contains invalid characters"; + LOGP(DLCTRL, LOGL_NOTICE, "GET variable contains invalid characters: \"%s\"\n", + osmo_escape_str(var, -1)); goto err; } cmd->variable = talloc_strdup(cmd, var); + var = strtok_r(NULL, "", &saveptr); + if (var) { + cmd->type = CTRL_TYPE_ERROR; + cmd->reply = "GET with trailing characters"; + LOGP(DLCTRL, LOGL_NOTICE, "GET with trailing characters: \"%s\"\n", + osmo_escape_str(var, -1)); + goto err; + } LOGP(DLCTRL, LOGL_DEBUG, "Command: GET %s\n", cmd->variable); break; case CTRL_TYPE_SET: @@ -350,31 +386,57 @@ struct ctrl_cmd *ctrl_cmd_parse2(void *ctx, struct msgb *msg) LOGP(DLCTRL, LOGL_NOTICE, "SET Command incomplete\n"); goto err; } + if (!osmo_separated_identifiers_valid(var, ".")) { + cmd->type = CTRL_TYPE_ERROR; + cmd->reply = "SET variable contains invalid characters"; + LOGP(DLCTRL, LOGL_NOTICE, "SET variable contains invalid characters: \"%s\"\n", + osmo_escape_str(var, -1)); + goto err; + } cmd->variable = talloc_strdup(cmd, var); cmd->value = talloc_strdup(cmd, val); if (!cmd->variable || !cmd->value) goto oom; - LOGP(DLCTRL, LOGL_DEBUG, "Command: SET %s = %s\n", cmd->variable, cmd->value); + + var = strtok_r(NULL, "", &saveptr); + if (var) { + cmd->type = CTRL_TYPE_ERROR; + cmd->reply = "SET with trailing characters"; + LOGP(DLCTRL, LOGL_NOTICE, "SET with trailing characters: \"%s\"\n", + osmo_escape_str(var, -1)); + goto err; + } + + LOGP(DLCTRL, LOGL_DEBUG, "Command: SET %s = \"%s\"\n", cmd->variable, + osmo_escape_str(cmd->value, -1)); break; case CTRL_TYPE_GET_REPLY: case CTRL_TYPE_SET_REPLY: case CTRL_TYPE_TRAP: var = strtok_r(NULL, " ", &saveptr); - val = strtok_r(NULL, " ", &saveptr); + val = strtok_r(NULL, "", &saveptr); if (!var || !val) { cmd->type = CTRL_TYPE_ERROR; cmd->reply = "Trap/Reply incomplete"; LOGP(DLCTRL, LOGL_NOTICE, "Trap/Reply incomplete\n"); goto err; } + if (!osmo_separated_identifiers_valid(var, ".")) { + cmd->type = CTRL_TYPE_ERROR; + cmd->reply = "Trap/Reply variable contains invalid characters"; + LOGP(DLCTRL, LOGL_NOTICE, "Trap/Reply variable contains invalid characters: \"%s\"\n", + osmo_escape_str(var, -1)); + goto err; + } cmd->variable = talloc_strdup(cmd, var); cmd->reply = talloc_strdup(cmd, val); if (!cmd->variable || !cmd->reply) goto oom; - LOGP(DLCTRL, LOGL_DEBUG, "Command: TRAP/REPLY %s: %s\n", cmd->variable, cmd->reply); + LOGP(DLCTRL, LOGL_DEBUG, "Command: TRAP/REPLY %s: \"%s\"\n", cmd->variable, + osmo_escape_str(cmd->reply, -1)); break; case CTRL_TYPE_ERROR: - var = strtok_r(NULL, "\0", &saveptr); + var = strtok_r(NULL, "", &saveptr); if (!var) { cmd->reply = ""; goto err; @@ -382,7 +444,8 @@ struct ctrl_cmd *ctrl_cmd_parse2(void *ctx, struct msgb *msg) cmd->reply = talloc_strdup(cmd, var); if (!cmd->reply) goto oom; - LOGP(DLCTRL, LOGL_DEBUG, "Command: ERROR %s\n", cmd->reply); + LOGP(DLCTRL, LOGL_DEBUG, "Command: ERROR \"%s\"\n", + osmo_escape_str(cmd->reply, -1)); break; case CTRL_TYPE_UNKNOWN: default: diff --git a/tests/ctrl/ctrl_test.c b/tests/ctrl/ctrl_test.c index 39ec61a8..a38591f3 100644 --- a/tests/ctrl/ctrl_test.c +++ b/tests/ctrl/ctrl_test.c @@ -79,14 +79,19 @@ static void assert_test(struct ctrl_handle *ctrl, struct ctrl_connection *ccon, cmd = ctrl_cmd_parse2(ctx, msg); OSMO_ASSERT(cmd); - OSMO_ASSERT(t->expect_parsed.type == cmd->type); + if (t->expect_parsed.type != cmd->type) { + printf("type mismatch: got %s\n", get_value_string(ctrl_type_vals, cmd->type)); + OSMO_ASSERT(t->expect_parsed.type == cmd->type); + } #define ASSERT_SAME_STR(field) \ assert_same_str(#field, t->expect_parsed.field, cmd->field) ASSERT_SAME_STR(id); - ASSERT_SAME_STR(variable); - ASSERT_SAME_STR(value); + if (t->expect_parsed.type != CTRL_TYPE_ERROR) { + ASSERT_SAME_STR(variable); + ASSERT_SAME_STR(value); + } ASSERT_SAME_STR(reply); talloc_free(cmd); @@ -140,75 +145,66 @@ static const struct one_test test_messages_list[] = { { .type = CTRL_TYPE_GET, .id = "1", - .variable = "variable\n", /* current bug */ + .variable = "variable", }, "ERROR 1 Command not found", }, { "GET 1 var\ni\nable", { - .type = CTRL_TYPE_GET, + .type = CTRL_TYPE_ERROR, .id = "1", - .variable = "var\ni\nable", /* current bug */ + .reply = "GET with trailing characters", }, - "ERROR 1 Command not found", - + "ERROR 1 GET with trailing characters", }, { "GET 1 var\ti\table", { - .type = CTRL_TYPE_GET, + .type = CTRL_TYPE_ERROR, .id = "1", - .variable = "var\ti\table", /* current bug */ + .reply = "GET variable contains invalid characters", }, - "ERROR 1 Command not found", + "ERROR 1 GET variable contains invalid characters", }, { "GET 1 var\ri\rable", { - .type = CTRL_TYPE_GET, + .type = CTRL_TYPE_ERROR, .id = "1", - .variable = "var\ri\rable", /* current bug */ + .reply = "GET variable contains invalid characters", }, - "ERROR 1 Command not found", + "ERROR 1 GET variable contains invalid characters", }, { "GET 1 variable value", { - .type = CTRL_TYPE_GET, + .type = CTRL_TYPE_ERROR, .id = "1", - .variable = "variable", - .value = NULL, + .reply = "GET with trailing characters", }, - "ERROR 1 Command not found", - + "ERROR 1 GET with trailing characters", }, { "GET 1 variable value\n", { - .type = CTRL_TYPE_GET, + .type = CTRL_TYPE_ERROR, .id = "1", - .variable = "variable", - .value = NULL, + .reply = "GET with trailing characters", }, - "ERROR 1 Command not found", - + "ERROR 1 GET with trailing characters", }, { "GET 1 variable multiple value tokens", { - .type = CTRL_TYPE_GET, + .type = CTRL_TYPE_ERROR, .id = "1", - .variable = "variable", - .value = NULL, + .reply = "GET with trailing characters", }, - "ERROR 1 Command not found", - + "ERROR 1 GET with trailing characters", }, { "GET 1 variable multiple value tokens\n", { - .type = CTRL_TYPE_GET, + .type = CTRL_TYPE_ERROR, .id = "1", - .variable = "variable", - .value = NULL, + .reply = "GET with trailing characters", }, - "ERROR 1 Command not found", - + "ERROR 1 GET with trailing characters", }, { "SET 1 variable value", { @@ -218,7 +214,6 @@ static const struct one_test test_messages_list[] = { .value = "value", }, "ERROR 1 Command not found", - }, { "SET 1 variable value\n", { @@ -228,27 +223,22 @@ static const struct one_test test_messages_list[] = { .value = "value", }, "ERROR 1 Command not found", - }, { "SET weird_id variable value", { - .type = CTRL_TYPE_SET, - .id = "weird_id", - .variable = "variable", - .value = "value", + .type = CTRL_TYPE_ERROR, + .id = "err", + .reply = "Invalid message ID number", }, - "ERROR weird_id Command not found", - + "ERROR err Invalid message ID number", }, { "SET weird_id variable value\n", { - .type = CTRL_TYPE_SET, - .id = "weird_id", - .variable = "variable", - .value = "value", + .type = CTRL_TYPE_ERROR, + .id = "err", + .reply = "Invalid message ID number", }, - "ERROR weird_id Command not found", - + "ERROR err Invalid message ID number", }, { "SET 1 variable multiple value tokens", { @@ -278,7 +268,6 @@ static const struct one_test test_messages_list[] = { .value = "value_with_trailing_spaces ", }, "ERROR 1 Command not found", - }, { "SET 1 variable value_with_trailing_spaces \n", { @@ -288,27 +277,22 @@ static const struct one_test test_messages_list[] = { .value = "value_with_trailing_spaces ", }, "ERROR 1 Command not found", - }, { "SET \n special_char_id value", { - .type = CTRL_TYPE_SET, - .id = "\n", - .variable = "special_char_id", - .value = "value", + .type = CTRL_TYPE_ERROR, + .id = "err", + .reply = "Invalid message ID number", }, - "ERROR \n Command not found", - + "ERROR err Invalid message ID number", }, { "SET \t special_char_id value", { - .type = CTRL_TYPE_SET, - .id = "\t", - .variable = "special_char_id", - .value = "value", + .type = CTRL_TYPE_ERROR, + .id = "err", + .reply = "Invalid message ID number", }, - "ERROR \t Command not found", - + "ERROR err Invalid message ID number", }, { "GET_REPLY 1 variable OK", { @@ -317,7 +301,6 @@ static const struct one_test test_messages_list[] = { .variable = "variable", .reply = "OK", }, - .reply_str = NULL, }, { "SET_REPLY 1 variable OK", { @@ -326,7 +309,6 @@ static const struct one_test test_messages_list[] = { .variable = "variable", .reply = "OK", }, - .reply_str = NULL, }, }; diff --git a/tests/ctrl/ctrl_test.ok b/tests/ctrl/ctrl_test.ok index 4a3a1696..087ebbc2 100644 --- a/tests/ctrl/ctrl_test.ok +++ b/tests/ctrl/ctrl_test.ok @@ -19,7 +19,7 @@ ok test: 'GET 1 variable\n' parsing: id = '1' -variable = 'variable\n' +variable = 'variable' value = '(null)' reply = '(null)' handling: @@ -28,65 +28,51 @@ ok test: 'GET 1 var\ni\nable' parsing: id = '1' -variable = 'var\ni\nable' -value = '(null)' -reply = '(null)' +reply = 'GET with trailing characters' handling: -replied: 'ERROR 1 Command not found' +replied: 'ERROR 1 GET with trailing characters' ok test: 'GET 1 var\ti\table' parsing: id = '1' -variable = 'var\ti\table' -value = '(null)' -reply = '(null)' +reply = 'GET variable contains invalid characters' handling: -replied: 'ERROR 1 Command not found' +replied: 'ERROR 1 GET variable contains invalid characters' ok test: 'GET 1 var\ri\rable' parsing: id = '1' -variable = 'var\ri\rable' -value = '(null)' -reply = '(null)' +reply = 'GET variable contains invalid characters' handling: -replied: 'ERROR 1 Command not found' +replied: 'ERROR 1 GET variable contains invalid characters' ok test: 'GET 1 variable value' parsing: id = '1' -variable = 'variable' -value = '(null)' -reply = '(null)' +reply = 'GET with trailing characters' handling: -replied: 'ERROR 1 Command not found' +replied: 'ERROR 1 GET with trailing characters' ok test: 'GET 1 variable value\n' parsing: id = '1' -variable = 'variable' -value = '(null)' -reply = '(null)' +reply = 'GET with trailing characters' handling: -replied: 'ERROR 1 Command not found' +replied: 'ERROR 1 GET with trailing characters' ok test: 'GET 1 variable multiple value tokens' parsing: id = '1' -variable = 'variable' -value = '(null)' -reply = '(null)' +reply = 'GET with trailing characters' handling: -replied: 'ERROR 1 Command not found' +replied: 'ERROR 1 GET with trailing characters' ok test: 'GET 1 variable multiple value tokens\n' parsing: id = '1' -variable = 'variable' -value = '(null)' -reply = '(null)' +reply = 'GET with trailing characters' handling: -replied: 'ERROR 1 Command not found' +replied: 'ERROR 1 GET with trailing characters' ok test: 'SET 1 variable value' parsing: @@ -108,21 +94,17 @@ replied: 'ERROR 1 Command not found' ok test: 'SET weird_id variable value' parsing: -id = 'weird_id' -variable = 'variable' -value = 'value' -reply = '(null)' +id = 'err' +reply = 'Invalid message ID number' handling: -replied: 'ERROR weird_id Command not found' +replied: 'ERROR err Invalid message ID number' ok test: 'SET weird_id variable value\n' parsing: -id = 'weird_id' -variable = 'variable' -value = 'value' -reply = '(null)' +id = 'err' +reply = 'Invalid message ID number' handling: -replied: 'ERROR weird_id Command not found' +replied: 'ERROR err Invalid message ID number' ok test: 'SET 1 variable multiple value tokens' parsing: @@ -162,21 +144,17 @@ replied: 'ERROR 1 Command not found' ok test: 'SET \n special_char_id value' parsing: -id = '\n' -variable = 'special_char_id' -value = 'value' -reply = '(null)' +id = 'err' +reply = 'Invalid message ID number' handling: -replied: 'ERROR \n Command not found' +replied: 'ERROR err Invalid message ID number' ok test: 'SET \t special_char_id value' parsing: -id = '\t' -variable = 'special_char_id' -value = 'value' -reply = '(null)' +id = 'err' +reply = 'Invalid message ID number' handling: -replied: 'ERROR \t Command not found' +replied: 'ERROR err Invalid message ID number' ok test: 'GET_REPLY 1 variable OK' parsing: diff --git a/tests/testsuite.at b/tests/testsuite.at index 4a59b221..81730ee2 100644 --- a/tests/testsuite.at +++ b/tests/testsuite.at @@ -18,7 +18,7 @@ AT_CLEANUP AT_SETUP([ctrl]) AT_KEYWORDS([ctrl]) cat $abs_srcdir/ctrl/ctrl_test.ok > expout -AT_CHECK([$abs_top_builddir/tests/ctrl/ctrl_test], [0], [expout]) +AT_CHECK([$abs_top_builddir/tests/ctrl/ctrl_test], [0], [expout], [ignore]) AT_CLEANUP AT_SETUP([kasumi]) -- cgit v1.2.3