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 +++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 20 deletions(-) (limited to 'src/vty/command.c') 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 */ -- cgit v1.2.3