From 5314c513f23688462d7f7937e5ae5e0d5cd4548e Mon Sep 17 00:00:00 2001 From: Neels Hofmeyr Date: Mon, 9 Jul 2018 23:22:21 +0200 Subject: vty: fix use-after-free and memleaks in is_cmd_ambiguous() vty_test: add test against ambiguous cmd causing use-after-free and memory leaks. Add this test along with the fix, because the new test triggers the memory use-after-free and leaks, causing build failures. Add cmd_deopt_with_ctx() to allow passing a specific talloc ctx. is_cmd_ambiguous(): keep all cmd_deopt() allocations until the function exits. Add a comment explaining why. Before this, if a command matched an optional "[arg]" with square brackets, we would keep it in local var 'matched', but we would free the string it points to at the end of that loop iteration; upon encountering another match, we would attempt to strcmp against the freed 'matched'. Instead of adding hard-to-read and -verify free/alloc dances to keep the 'matched' accurately freed/non-freed/..., just keep all cmd_deopt() string allocated until done. Needless to say that this should have been implemented on a lower level upon inventing optional args, but at least this is fixing a program crash. Related: OS#33903390 Change-Id: Ia71ba742108b5ff020997bfb612ad5eb30d04fcd --- src/vty/command.c | 63 +++++++++++++++++++++++++++++++++++---------------- tests/vty/vty_test.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/vty/vty_test.ok | 19 ++++++++++++++++ 3 files changed, 124 insertions(+), 20 deletions(-) diff --git a/src/vty/command.c b/src/vty/command.c index 51dece3a..43f974ce 100644 --- a/src/vty/command.c +++ b/src/vty/command.c @@ -1304,8 +1304,7 @@ static int cmd_range_match(const char *range, const char *str) } /* helper to retrieve the 'real' argument string from an optional argument */ -static char * -cmd_deopt(const char *str) +static char *cmd_deopt(void *ctx, const char *str) { /* we've got "[blah]". We want to strip off the []s and redo the * match check for "blah" @@ -1315,7 +1314,7 @@ cmd_deopt(const char *str) if (len < 3) return NULL; - return talloc_strndup(tall_vty_cmd_ctx, str + 1, len - 2); + return talloc_strndup(ctx, str + 1, len - 2); } static enum match_type @@ -1326,7 +1325,7 @@ cmd_match(const char *str, const char *command, if (recur && CMD_OPTION(str)) { enum match_type ret; - char *tmp = cmd_deopt(str); + char *tmp = cmd_deopt(tall_vty_cmd_ctx, str); /* this would be a bug in a command, however handle it gracefully * as it we only discover it if a user tries to run it @@ -1475,6 +1474,7 @@ cmd_filter(char *command, vector v, unsigned int index, enum match_type level) static int is_cmd_ambiguous(char *command, vector v, int index, enum match_type type) { + int ret = 0; unsigned int i; unsigned int j; struct cmd_element *cmd_element; @@ -1482,6 +1482,16 @@ is_cmd_ambiguous(char *command, vector v, int index, enum match_type type) vector descvec; struct desc *desc; + /* In this loop, when a match is found, 'matched' points to it. If on a later iteration, an + * identical match is found, the command is ambiguous. The trickiness is that a string may be + * enclosed in '[str]' square brackets, which get removed by a talloc_strndup(), via cmd_deopt(). + * Such a string is usually needed for one loop iteration, except when 'matched' points to it. In + * that case, the string must remain allocated until this function exits or another match comes + * around. This is sufficiently confusing to justify a separate talloc tree to store all of the + * odd allocations, and to free them all at the end. We are not expecting too many optional args + * or ambiguities to cause a noticeable memory footprint from keeping all allocations. */ + void *cmd_deopt_ctx = NULL; + for (i = 0; i < vector_active(v); i++) if ((cmd_element = vector_slot(v, i)) != NULL) { int match = 0; @@ -1493,9 +1503,15 @@ is_cmd_ambiguous(char *command, vector v, int index, enum match_type type) enum match_type ret; const char *str = desc->cmd; - if (CMD_OPTION(str)) - if ((str = cmd_deopt(str)) == NULL) + if (CMD_OPTION(str)) { + if (!cmd_deopt_ctx) + cmd_deopt_ctx = + talloc_named_const(tall_vty_cmd_ctx, 0, + __func__); + str = cmd_deopt(cmd_deopt_ctx, str); + if (str == NULL) continue; + } switch (type) { case exact_match: @@ -1509,9 +1525,10 @@ is_cmd_ambiguous(char *command, vector v, int index, enum match_type type) { if (matched && strcmp(matched, - str) != 0) - return 1; /* There is ambiguous match. */ - else + str) != 0) { + ret = 1; /* There is ambiguous match. */ + goto free_and_return; + } else matched = str; match++; } @@ -1521,9 +1538,10 @@ is_cmd_ambiguous(char *command, vector v, int index, enum match_type type) (str, command)) { if (matched && strcmp(matched, - str) != 0) - return 1; - else + str) != 0) { + ret = 1; + goto free_and_return; + } else matched = str; match++; } @@ -1537,8 +1555,10 @@ is_cmd_ambiguous(char *command, vector v, int index, enum match_type type) if ((ret = cmd_ipv6_prefix_match (command)) != no_match) { - if (ret == partly_match) - return 2; /* There is incomplete match. */ + if (ret == partly_match) { + ret = 2; /* There is incomplete match. */ + goto free_and_return; + } match++; } @@ -1552,8 +1572,10 @@ is_cmd_ambiguous(char *command, vector v, int index, enum match_type type) if ((ret = cmd_ipv4_prefix_match (command)) != no_match) { - if (ret == partly_match) - return 2; /* There is incomplete match. */ + if (ret == partly_match) { + ret = 2; /* There is incomplete match. */ + goto free_and_return; + } match++; } @@ -1566,14 +1588,15 @@ is_cmd_ambiguous(char *command, vector v, int index, enum match_type type) default: break; } - - if (CMD_OPTION(desc->cmd)) - talloc_free((void*)str); } if (!match) vector_slot(v, i) = NULL; } - return 0; + +free_and_return: + if (cmd_deopt_ctx) + talloc_free(cmd_deopt_ctx); + return ret; } /* If src matches dst return dst string, otherwise return NULL */ diff --git a/tests/vty/vty_test.c b/tests/vty/vty_test.c index a3478e1d..30efb9af 100644 --- a/tests/vty/vty_test.c +++ b/tests/vty/vty_test.c @@ -385,6 +385,42 @@ DEFUN(cfg_level3_child, cfg_level3_child_cmd, return CMD_SUCCESS; } +DEFUN(cfg_ambiguous_nr_1, cfg_ambiguous_nr_1_cmd, + "ambiguous_nr [<0-23>]", + "testing is_cmd_ambiguous()\n" + "optional number arg\n") +{ + printf("Called: 'ambiguous_nr [<0-23>]' (argc=%d)\n", argc); + return CMD_SUCCESS; +} + +DEFUN(cfg_ambiguous_nr_2, cfg_ambiguous_nr_2_cmd, + "ambiguous_nr <0-23> keyword", + "testing is_cmd_ambiguous()\n" + "optional number arg\n") +{ + printf("Called: 'ambiguous_nr <0-23> keyword'\n"); + return CMD_SUCCESS; +} + +DEFUN(cfg_ambiguous_str_1, cfg_ambiguous_str_1_cmd, + "ambiguous_str [ARG]", + "testing is_cmd_ambiguous()\n" + "optional string arg\n") +{ + printf("Called: 'ambiguous_str [ARG]' (argc=%d)\n", argc); + return CMD_SUCCESS; +} + +DEFUN(cfg_ambiguous_str_2, cfg_ambiguous_str_2_cmd, + "ambiguous_str ARG keyword", + "testing is_cmd_ambiguous()\n" + "optional string arg\n") +{ + printf("Called: 'ambiguous_str ARG keyword'\n"); + return CMD_SUCCESS; +} + void test_vty_add_cmds() { install_element(CONFIG_NODE, &cfg_level1_cmd); @@ -398,6 +434,30 @@ void test_vty_add_cmds() install_node(&level3_node, NULL); install_element(LEVEL3_NODE, &cfg_level3_child_cmd); + + install_element_ve(&cfg_ambiguous_nr_1_cmd); + install_element_ve(&cfg_ambiguous_nr_2_cmd); + install_element_ve(&cfg_ambiguous_str_1_cmd); + install_element_ve(&cfg_ambiguous_str_2_cmd); +} + +void test_is_cmd_ambiguous() +{ + struct vty *vty; + struct vty_test test; + + printf("Going to test is_cmd_ambiguous()\n"); + vty = create_test_vty(&test); + + OSMO_ASSERT(do_vty_command(vty, "ambiguous_nr") == CMD_SUCCESS); + OSMO_ASSERT(do_vty_command(vty, "ambiguous_nr 23") == CMD_SUCCESS); + OSMO_ASSERT(do_vty_command(vty, "ambiguous_nr 23 keyword") == CMD_SUCCESS); + + OSMO_ASSERT(do_vty_command(vty, "ambiguous_str") == CMD_SUCCESS); + OSMO_ASSERT(do_vty_command(vty, "ambiguous_str arg") == CMD_SUCCESS); + OSMO_ASSERT(do_vty_command(vty, "ambiguous_str arg keyword") == CMD_SUCCESS); + + destroy_test_vty(&test, vty); } static int go_parent_cb(struct vty *vty) @@ -465,6 +525,8 @@ int main(int argc, char **argv) test_exit_by_indent("ok_indented_root.cfg", 0); test_exit_by_indent("ok_empty_parent.cfg", 0); + test_is_cmd_ambiguous(); + /* Leak check */ OSMO_ASSERT(talloc_total_blocks(stats_ctx) == 1); diff --git a/tests/vty/vty_test.ok b/tests/vty/vty_test.ok index bd6c5d66..2f76ff91 100644 --- a/tests/vty/vty_test.ok +++ b/tests/vty/vty_test.ok @@ -286,4 +286,23 @@ called level2 node k called level3 node k called level1 child cmd k got rc=0 +Going to test is_cmd_ambiguous() +Going to execute 'ambiguous_nr' +Called: 'ambiguous_nr [<0-23>]' (argc=0) +Returned: 0, Current node: 1 '%s> ' +Going to execute 'ambiguous_nr 23' +Called: 'ambiguous_nr [<0-23>]' (argc=1) +Returned: 0, Current node: 1 '%s> ' +Going to execute 'ambiguous_nr 23 keyword' +Called: 'ambiguous_nr <0-23> keyword' +Returned: 0, Current node: 1 '%s> ' +Going to execute 'ambiguous_str' +Called: 'ambiguous_str [ARG]' (argc=0) +Returned: 0, Current node: 1 '%s> ' +Going to execute 'ambiguous_str arg' +Called: 'ambiguous_str [ARG]' (argc=1) +Returned: 0, Current node: 1 '%s> ' +Going to execute 'ambiguous_str arg keyword' +Called: 'ambiguous_str ARG keyword' +Returned: 0, Current node: 1 '%s> ' All tests passed -- cgit v1.2.3