From: Jason Parker Date: Thu, 29 Mar 2012 21:49:14 +0000 (+0000) Subject: Multiple revisions 359656,359706,359979 X-Git-Tag: certified/1.8.11-cert1~3^2~11 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e854851dc33d449d30cb56c864a235571a687fbd;p=thirdparty%2Fasterisk.git Multiple revisions 359656,359706,359979 ........ r359656 | mjordan | 2012-03-15 13:35:59 -0500 (Thu, 15 Mar 2012) | 22 lines Fix remotely exploitable stack overrun in Milliwatt Milliwatt is vulnerable to a remotely exploitable stack overrun when using the 'o' option. This occurs due to the milliwatt_generate function not accounting for AST_FRIENDLY_OFFSET when calculating the maximum number of samples it can put in the output buffer. This patch resolves this issue by taking into account AST_FRIENDLY_OFFSET when determining the maximum number of samples allowed. Note that at no point is remote code execution possible. The data that is written into the buffer is the pre-defined Milliwatt data, and not custom data. (closes issue ASTERISK-19541) Reported by: Russell Bryant Tested by: Matt Jordan Patches: milliwatt_stack_overrun.rev1.txt by Russell Bryant (license 6283) Note that this patch was written by Russell, even though Matt uploaded it ........ Merged revisions 359645 from http://svn.asterisk.org/svn/asterisk/branches/1.6.2 ........ r359706 | mjordan | 2012-03-15 14:01:22 -0500 (Thu, 15 Mar 2012) | 16 lines Fix remotely exploitable stack overflow in HTTP manager There exists a remotely exploitable stack buffer overflow in HTTP digest authentication handling in Asterisk. The particular method in question is only utilized by HTTP AMI. When parsing the digest information, the length of the string is not checked when it is copied into temporary buffers allocated on the stack. This patch fixes this behavior by parsing out pre-defined key/value pairs and avoiding unnecessary copies to the stack. (closes issue ASTERISK-19542) Reported by: Russell Bryant Tested by: Matt Jordan ........ r359979 | rmudgett | 2012-03-20 12:21:16 -0500 (Tue, 20 Mar 2012) | 28 lines Allow AMI action callback to be reentrant. Fix AMI module reload deadlock regression from ASTERISK-18479 when it tried to fix the race between calling an AMI action callback and unregistering that action. Refixes ASTERISK-13784 broken by ASTERISK-17785 change. Locking the ao2 object guaranteed that there were no active callbacks that mattered when ast_manager_unregister() was called. Unfortunately, this causes the deadlock situation. The patch stops locking the ao2 object to allow multiple threads to invoke the callback re-entrantly. There is no way to guarantee a module unload will not crash because of an active callback. The code attempts to minimize the chance with the registered flag and the maximum 5 second delay before ast_manager_unregister() returns. The trunk version of the patch changes the API to fix the race condition correctly to prevent the module code from unloading from memory while an action callback is active. * Don't hold the lock while calling the AMI action callback. (closes issue ASTERISK-19487) Reported by: Philippe Lindheimer Review: https://reviewboard.asterisk.org/r/1818/ Review: https://reviewboard.asterisk.org/r/1820/ ........ Merged revisions 359656,359706,359979 from http://svn.asterisk.org/svn/asterisk/branches/1.8 git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/1.8-digiumphones@360826 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- diff --git a/apps/app_milliwatt.c b/apps/app_milliwatt.c index 6130a02e5e..30b3d51241 100644 --- a/apps/app_milliwatt.c +++ b/apps/app_milliwatt.c @@ -78,7 +78,7 @@ static void milliwatt_release(struct ast_channel *chan, void *data) static int milliwatt_generate(struct ast_channel *chan, void *data, int len, int samples) { unsigned char buf[AST_FRIENDLY_OFFSET + 640]; - const int maxsamples = ARRAY_LEN(buf); + const int maxsamples = ARRAY_LEN(buf) - (AST_FRIENDLY_OFFSET / sizeof(buf[0])); int i, *indexp = (int *) data; struct ast_frame wf = { .frametype = AST_FRAME_VOICE, diff --git a/include/asterisk/manager.h b/include/asterisk/manager.h index 3b77a8caeb..c20cdbdb91 100644 --- a/include/asterisk/manager.h +++ b/include/asterisk/manager.h @@ -161,6 +161,8 @@ struct manager_action { * function and unregestring the AMI action object. */ unsigned int registered:1; + /*! Number of active func() calls in progress. */ + unsigned int active_count; }; /*! \brief External routines may register/unregister manager callbacks this way diff --git a/main/manager.c b/main/manager.c index da8fdba2be..0a9f0c76fb 100644 --- a/main/manager.c +++ b/main/manager.c @@ -1936,7 +1936,11 @@ int ast_hook_send_action(struct manager_custom_hook *hook, const char *msg) ao2_lock(act_found); if (act_found->registered && act_found->func) { + ++act_found->active_count; + ao2_unlock(act_found); ret = act_found->func(&s, &m); + ao2_lock(act_found); + --act_found->active_count; } else { ret = -1; } @@ -4636,8 +4640,12 @@ static int process_message(struct mansession *s, const struct message *m) ao2_lock(act_found); if (act_found->registered && act_found->func) { ast_debug(1, "Running action '%s'\n", act_found->action); + ++act_found->active_count; + ao2_unlock(act_found); ret = act_found->func(s, m); acted = 1; + ao2_lock(act_found); + --act_found->active_count; } ao2_unlock(act_found); } @@ -5145,6 +5153,8 @@ int ast_manager_unregister(char *action) AST_RWLIST_UNLOCK(&actions); if (cur) { + time_t now; + /* * We have removed the action object from the container so we * are no longer in a hurry. @@ -5153,6 +5163,23 @@ int ast_manager_unregister(char *action) cur->registered = 0; ao2_unlock(cur); + /* + * Wait up to 5 seconds for any active invocations to complete + * before returning. We have to wait instead of blocking + * because we may be waiting for ourself to complete. + */ + now = time(NULL); + while (cur->active_count) { + if (5 <= time(NULL) - now) { + ast_debug(1, + "Unregister manager action %s timed out waiting for %d active instances to complete\n", + action, cur->active_count); + break; + } + + sched_yield(); + } + ao2_t_ref(cur, -1, "action object removed from list"); ast_verb(2, "Manager unregistered action %s\n", action); } diff --git a/main/utils.c b/main/utils.c index 7c9478bb32..fb75872738 100644 --- a/main/utils.c +++ b/main/utils.c @@ -1985,10 +1985,29 @@ int ast_utils_init(void) * pedantic arg can be set to nonzero if we need to do addition Digest check. */ int ast_parse_digest(const char *digest, struct ast_http_digest *d, int request, int pedantic) { - int i; - char *c, key[512], val[512]; + char *c; struct ast_str *str = ast_str_create(16); + /* table of recognised keywords, and places where they should be copied */ + const struct x { + const char *key; + const ast_string_field *field; + } *i, keys[] = { + { "username=", &d->username }, + { "realm=", &d->realm }, + { "nonce=", &d->nonce }, + { "uri=", &d->uri }, + { "domain=", &d->domain }, + { "response=", &d->response }, + { "cnonce=", &d->cnonce }, + { "opaque=", &d->opaque }, + /* Special cases that cannot be directly copied */ + { "algorithm=", NULL }, + { "qop=", NULL }, + { "nc=", NULL }, + { NULL, 0 }, + }; + if (ast_strlen_zero(digest) || !d || !str) { ast_free(str); return -1; @@ -2006,72 +2025,55 @@ int ast_parse_digest(const char *digest, struct ast_http_digest *d, int request, c += strlen("Digest "); /* lookup for keys/value pair */ - while (*c && *(c = ast_skip_blanks(c))) { + while (c && *c && *(c = ast_skip_blanks(c))) { /* find key */ - i = 0; - while (*c && *c != '=' && *c != ',' && !isspace(*c)) { - key[i++] = *c++; - } - key[i] = '\0'; - c = ast_skip_blanks(c); - if (*c == '=') { - c = ast_skip_blanks(++c); - i = 0; - if (*c == '\"') { - /* in quotes. Skip first and look for last */ - c++; - while (*c && *c != '\"') { - if (*c == '\\' && c[1] != '\0') { /* unescape chars */ - c++; - } - val[i++] = *c++; - } - } else { - /* token */ - while (*c && *c != ',' && !isspace(*c)) { - val[i++] = *c++; - } + for (i = keys; i->key != NULL; i++) { + char *src, *separator; + int unescape = 0; + if (strncasecmp(c, i->key, strlen(i->key)) != 0) { + continue; } - val[i] = '\0'; - } - while (*c && *c != ',') { - c++; - } - if (*c) { - c++; - } - - if (!strcasecmp(key, "username")) { - ast_string_field_set(d, username, val); - } else if (!strcasecmp(key, "realm")) { - ast_string_field_set(d, realm, val); - } else if (!strcasecmp(key, "nonce")) { - ast_string_field_set(d, nonce, val); - } else if (!strcasecmp(key, "uri")) { - ast_string_field_set(d, uri, val); - } else if (!strcasecmp(key, "domain")) { - ast_string_field_set(d, domain, val); - } else if (!strcasecmp(key, "response")) { - ast_string_field_set(d, response, val); - } else if (!strcasecmp(key, "algorithm")) { - if (strcasecmp(val, "MD5")) { - ast_log(LOG_WARNING, "Digest algorithm: \"%s\" not supported.\n", val); - return -1; + /* Found. Skip keyword, take text in quotes or up to the separator. */ + c += strlen(i->key); + if (*c == '"') { + src = ++c; + separator = "\""; + unescape = 1; + } else { + src = c; + separator = ","; } - } else if (!strcasecmp(key, "cnonce")) { - ast_string_field_set(d, cnonce, val); - } else if (!strcasecmp(key, "opaque")) { - ast_string_field_set(d, opaque, val); - } else if (!strcasecmp(key, "qop") && !strcasecmp(val, "auth")) { - d->qop = 1; - } else if (!strcasecmp(key, "nc")) { - unsigned long u; - if (sscanf(val, "%30lx", &u) != 1) { - ast_log(LOG_WARNING, "Incorrect Digest nc value: \"%s\".\n", val); - return -1; + strsep(&c, separator); /* clear separator and move ptr */ + if (unescape) { + ast_unescape_c(src); + } + if (i->field) { + ast_string_field_ptr_set(d, i->field, src); + } else { + /* Special cases that require additional procesing */ + if (!strcasecmp(i->key, "algorithm=")) { + if (strcasecmp(src, "MD5")) { + ast_log(LOG_WARNING, "Digest algorithm: \"%s\" not supported.\n", src); + ast_free(str); + return -1; + } + } else if (!strcasecmp(i->key, "qop=") && !strcasecmp(src, "auth")) { + d->qop = 1; + } else if (!strcasecmp(i->key, "nc=")) { + unsigned long u; + if (sscanf(src, "%30lx", &u) != 1) { + ast_log(LOG_WARNING, "Incorrect Digest nc value: \"%s\".\n", src); + ast_free(str); + return -1; + } + ast_string_field_set(d, nc, src); + } } - ast_string_field_set(d, nc, val); + break; + } + if (i->key == NULL) { /* not found, try ',' */ + strsep(&c, ","); } } ast_free(str);