From 3934b0478d4962a81fcc5d20b75dbfc3aceb398e Mon Sep 17 00:00:00 2001 From: Matthew Jordan Date: Tue, 17 Apr 2012 21:08:05 +0000 Subject: [PATCH] Fix places in main where a negative return value could impact execution This patch addresses a number of modules in main that did not handle the negative return value from function calls adequately, or were not sufficiently clear that the conditions leading to improper handling of the return values could not occur. This includes: * asterisk.c: A negative return value from the read function would be used directly as an index into a buffer. We now check for success of the read function prior to using its result as an index. * manager.c: Check for failures in mkstemp and lseek when handling the temporary file created for processing data returned from a CLI command in action_command. Also check that the result of an lseek is sanitized prior to using it as the size of a memory map to allocate. (issue ASTERISK-19655) Reported by: Matt Jordan Review: https://reviewboard.asterisk.org/r/1863/ ........ Merged revisions 362359 from http://svn.asterisk.org/svn/asterisk/branches/1.8 ........ Merged revisions 362360 from http://svn.asterisk.org/svn/asterisk/branches/10 git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@362361 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- main/asterisk.c | 5 +++- main/manager.c | 61 ++++++++++++++++++++++++++++++++----------------- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/main/asterisk.c b/main/asterisk.c index 902a285bfa..aaa9d03fc6 100644 --- a/main/asterisk.c +++ b/main/asterisk.c @@ -2311,6 +2311,7 @@ static int ast_el_read_char(EditLine *editline, char *cp) quit_handler(0, SHUTDOWN_FAST, 0); } } + continue; } buf[res] = '\0'; @@ -2621,7 +2622,9 @@ static char *cli_complete(EditLine *editline, int ch) if (ast_opt_remote) { snprintf(buf, sizeof(buf), "_COMMAND NUMMATCHES \"%s\" \"%s\"", lf->buffer, ptr); fdsend(ast_consock, buf); - res = read(ast_consock, buf, sizeof(buf) - 1); + if ((res = read(ast_consock, buf, sizeof(buf) - 1)) < 0) { + return (char*)(CC_ERROR); + } buf[res] = '\0'; nummatches = atoi(buf); diff --git a/main/manager.c b/main/manager.c index 597498c154..be5227a6d2 100644 --- a/main/manager.c +++ b/main/manager.c @@ -3737,7 +3737,7 @@ static int action_command(struct mansession *s, const struct message *m) { const char *cmd = astman_get_header(m, "Command"); const char *id = astman_get_header(m, "ActionID"); - char *buf, *final_buf; + char *buf = NULL, *final_buf = NULL; char template[] = "/tmp/ast-ami-XXXXXX"; /* template for temporary file */ int fd; off_t l; @@ -3752,7 +3752,11 @@ static int action_command(struct mansession *s, const struct message *m) return 0; } - fd = mkstemp(template); + if ((fd = mkstemp(template)) < 0) { + ast_log(AST_LOG_WARNING, "Failed to create temporary file for command: %s\n", strerror(errno)); + astman_send_error(s, m, "Command response construction error"); + return 0; + } astman_append(s, "Response: Follows\r\nPrivilege: Command\r\n"); if (!ast_strlen_zero(id)) { @@ -3760,30 +3764,45 @@ static int action_command(struct mansession *s, const struct message *m) } /* FIXME: Wedge a ActionID response in here, waiting for later changes */ ast_cli_command(fd, cmd); /* XXX need to change this to use a FILE * */ - l = lseek(fd, 0, SEEK_END); /* how many chars available */ + /* Determine number of characters available */ + if ((l = lseek(fd, 0, SEEK_END)) < 0) { + ast_log(LOG_WARNING, "Failed to determine number of characters for command: %s\n", strerror(errno)); + goto action_command_cleanup; + } /* This has a potential to overflow the stack. Hence, use the heap. */ - buf = ast_calloc(1, l + 1); - final_buf = ast_calloc(1, l + 1); - if (buf) { - lseek(fd, 0, SEEK_SET); - if (read(fd, buf, l) < 0) { - ast_log(LOG_WARNING, "read() failed: %s\n", strerror(errno)); - } - buf[l] = '\0'; - if (final_buf) { - term_strip(final_buf, buf, l); - final_buf[l] = '\0'; - } - astman_append(s, "%s", S_OR(final_buf, buf)); - ast_free(buf); + buf = ast_malloc(l + 1); + final_buf = ast_malloc(l + 1); + + if (!buf || !final_buf) { + ast_log(LOG_WARNING, "Failed to allocate memory for temporary buffer\n"); + goto action_command_cleanup; + } + + if (lseek(fd, 0, SEEK_SET) < 0) { + ast_log(LOG_WARNING, "Failed to set position on temporary file for command: %s\n", strerror(errno)); + goto action_command_cleanup; } + + if (read(fd, buf, l) < 0) { + ast_log(LOG_WARNING, "read() failed: %s\n", strerror(errno)); + goto action_command_cleanup; + } + + buf[l] = '\0'; + term_strip(final_buf, buf, l); + final_buf[l] = '\0'; + astman_append(s, "%s", final_buf); + +action_command_cleanup: + close(fd); unlink(template); astman_append(s, "--END COMMAND--\r\n\r\n"); - if (final_buf) { - ast_free(final_buf); - } + + ast_free(buf); + ast_free(final_buf); + return 0; } @@ -5951,7 +5970,7 @@ static void process_output(struct mansession *s, struct ast_str **out, struct as fprintf(s->f, "%c", 0); fflush(s->f); - if ((l = ftell(s->f))) { + if ((l = ftell(s->f)) > 0) { if (MAP_FAILED == (buf = mmap(NULL, l, PROT_READ | PROT_WRITE, MAP_PRIVATE, s->fd, 0))) { ast_log(LOG_WARNING, "mmap failed. Manager output was not processed\n"); } else {