[Home]

Summary:ASTERISK-20658: Blindly doing alloca (or strdupa) on potentially large user input is bad
Reporter:Walter Doekes (wdoekes)Labels:
Date Opened:2012-11-07 08:48:19.000-0600Date Closed:2013-01-02 16:21:05.000-0600
Priority:CriticalRegression?No
Status:Closed/CompleteComponents:
Versions:1.8.18.0 10.10.0 10.10.0-digiumphones 11.0.1 Frequency of
Occurrence
Related
Issues:
must be completed before resolvingASTERISK-20807 1.8.20.0 Blockers
must be completed before resolvingASTERISK-20808 10.12.0 Blockers
must be completed before resolvingASTERISK-20809 11.2.0 Blockers
Environment:Attachments:( 0) ASTERISK-20658_res_jabber.c.patch
( 1) issueA20658_dont_process_overlong_config_lines.patch
( 2) issueA20658_func_realtime_limit.patch
( 3) issueA20658_http_postvars_use_malloc2.patch
( 4) issueA20658_limit_sip_packet_size3.patch
( 5) issueA20658_sanitize_app_mysql.patch
( 6) issueA20658_view_allocas.patch
Description:I recently removed a potential user initiated crash by removing unnecessary ast_strdupa's in this patch.
http://lists.digium.com/pipermail/asterisk-commits/2012-October/057328.html

The problem is that alloca() does not check whether things will fit in the stack. If they don't fit, asterisk will simply segfault.
This wouldn't be so much of a problem, if it weren't for the limited stack size:
{code}
/* default 500kB or 110kB */
#define AST_STACKSIZE (((sizeof(void *) * 8 * 8) - 16) * 1024)
#define AST_BACKGROUND_STACKSIZE (((sizeof(void *) * 8 * 2) - 16) * 1024)
pthread_attr_setstacksize(attr, stacksize ? stacksize : AST_STACKSIZE);
{code}


Before the mentioned patch, sending a SIP body that was too large (e.g. 700kB) would trigger the crash. Unfortunately, that isn't the only place where user data is fed to alloca and friends directly.

Anywhere where one of ast_alloca, ast_str_alloca and ast_strdupa is used, case should be taken that the data isn't too large.

While checking for issues in the SIP module, I've found at least three, but I'm pretty sure there are more. All of those are only exploitable if you're using TCP (or TLS) because (fragmented) UDP won't allow packet sizes that big.

- Allow: BIG_STRING
- Content-Type: multipart/mixed;boundary="BIG_STRING"
- sdp body: o=BIG_STRING\nm=audio\n

The easy fix is to cap a SIP packet size to something reasonable like 20K. That's what the attached patch [1] does.

Patch [2] makes the alloca's visible. It could be used to scan for other potential problems. (I've rewritten the ast_verbose alloca to use C99 variable length arrays. That suffers from the same problem, but it declutters the output.)

[1] issueA20658_limit_sip_packet_size.patch
[2] issueA20658_view_allocas.patch

Regards,
Walter Doekes
OSSO B.V.
Comments:By: Mark Michelson (mmichelson) 2012-11-12 16:47:25.835-0600

I've been doing work on this to see what other places are subject to this.

My first thing I did was to grep for all SOCK_STREAM uses in the code. This would indicate where TCP sockets are being used. I found the following places:

netsock.c: Irrelevant. socket is created to find local IP address. No data is collected from remote end.

asterisk.c: Irrelevant. There is data taken in that can be strdupa'd. However, these are either command line options to Asterisk or data taken from remote consoles. In either case, the security threat here would be that the attacker got console access in the first place.

ooh323 files: Irrelevant. None of the stack allocation functions are used.

chan_mobile.c: Irrelevant. Stack allocation functions are only used for application and CLI data.

chan_skinny.c: Irrelevant. Stack allocation functions are only used for device state and configuration file data.

acl.c: Irrelevant. Socket is used for the same purpose as in netsock.c

app_festival.c: Irrelevant. Stack allocation functions are only used for application data.

app_nbscat.c: Irrelevant. No stack allocation functions used.

res_pktccops.c: Irrelevant. No stack allocation functions used.

res_agi.c: Irrelevant. Stack allocation is used on data read from a socket, but the size of any allocation is capped at 2048 bytes.

main/tcptls.c Locally Irrelevant, however it provides an API that can be abused perhaps (in fact this is what chan_sip uses).

There were other uses of SOCK_STREAM, but they either were not setting up sockets or it was used by code external to Asterisk. Next, I did a grep for "tcptls" to find users of the TCP/TLS API to find if any were abusing like chan_sip was.

main/manager.c Irrelevant. Stack allocation is only used on individual manager headers. The length of a header is capped at 1024 characters. There is also a stack allocation function used when getting configuration via JSON. The input to this is data from a file, though.

chan_sip.c: Walter has taken care of the issue there with his initial patch.

main/http.c: There appears to be a potential issue here. In ast_http_get_post_vars(), the Content-Length header is read and ast_alloca() is used to allocate a stack buffer of that size. Even though the maximum data size for an HTTP request is capped, a bogus Content-Length header may be used to allocate a stack buffer that is large enough to overflow the stack. I don't actually know if allocation of this buffer is enough to crash Asterisk or if data actually has to be written to this buffer past a certain point to make it happen, but the fact that we blindly allocate a buffer large enough is pretty terrible.

app_externalivr.c: Irrelevant. Stack allocation functions are used on input from the remote end but message sizes are capped.

My next task will be to look through users of UDP to make sure that they do not concatenate multiple datagrams together and then attempt to duplicate them on the stack or anything similar.

By: Walter Doekes (wdoekes) 2012-11-13 03:45:36.017-0600

Thanks for the update!

I had indeed come to the same conclusion with regards to the manager.c. (BTW I updated my initial patch to a second version which checks both the header size and the body size -- the first one forgot the body size in the tcp case -- and replaces the word SSL with TLS, while I was in the neighbourhood.)

app_externalivr.c is indeed safe.

Regarding main/http.c:
{code}        buf = ast_alloca(content_length);
       if (!fgets(buf, content_length, ser->f)) {
               return NULL;
       }
{code}
I don't see any explicit cap, other than that fgets might not read 700kB in a single go? Attached a (compile-tested only) patch that changes it to malloc instead (and removes the Content-Length + 1 oddity).

By: Mark Michelson (mmichelson) 2012-11-13 12:38:33.718-0600

Yeah, you're right that there isn't necessarily a cap. The http code will ensure that all headers it reads are capped at 4096 chars (!) but when reading the content, there is no explicit cap. I don't know if there is an implied upper bound on fgets() though.

The reason why content_length was set to the value of the Content-Length header + 1 is because fgets, according to the man page for it "reads in at most one less than size characters from stream and stores them into the buffer pointed to by s." If you feed the actual Content-Length header value to fgets(), you'd have an off-by-one error.

I think your http.c patch has a flaw in it. Since you've switched to using fread(), it means that you're no longer reading a string, and so you no longer have the null-terminator at the end of what you read. I think you'll need to be sure to allocate enough space to read the content and have it null-terminated.

By: Walter Doekes (wdoekes) 2012-11-13 13:32:19.735-0600

Ouch.. that was sloppy indeed. Patch updated.

By: Walter Doekes (wdoekes) 2012-11-13 14:04:20.052-0600

(For the record: fgets/fread will gladly read a *lot* of bytes without complaining, fgets is significantly less performant than fread, and doing too much alloca without writing to it will still crash.)

By: Mark Michelson (mmichelson) 2012-11-13 14:21:38.636-0600

Is there a possibility that the '\0' you write to the end of the buffer is going to overwrite an important character?

By: Mark Michelson (mmichelson) 2012-11-13 15:07:04.804-0600

Walter answered my question on IRC (I had missed the +1 in his ast_malloc() call).

I haven't seen any cases where data read in over UDP could cause this problem.

By: Walter Doekes (wdoekes) 2012-11-13 15:33:20.580-0600

I've been browsing through funcs/ for strdupa's and alloca's, and I see this:

(a) func_realtime: if you manage to fill a column with a large longblob

{noformat}
exten => 1,1,Set(x=${REALTIME(family,id,1)}) ; with column data=large
{noformat}
{noformat}
[Nov 13 22:28:24] NOTICE[12423]: func_realtime.c:222 function_realtime_read: STACK: ast_alloca(524324) at func_realtime.c:222: function_realtime_read
Segmentation fault
{noformat}

(b) func_strings: I suspect that if you can trick someone else to input large values into ~ODBCFIELDS~ (hash) and read them using hashkeys_read, that will explode too.

By: Walter Doekes (wdoekes) 2012-11-13 16:13:39.544-0600

Ok.. never mind the func_strings. Those are assigned to with ast_str_set with "-1" which apparently means "at most the available space".

By: Mark Michelson (mmichelson) 2012-11-15 15:52:54.944-0600

Found a potential trouble spot in res_jabber in 1.8 and 10 and in res_xmpp in 11.

In acf_jabberreceive_read(), we ast_strdupa() the contents of the XMPP message body. This seems exploitable since I can find no stated maximum XMPP stanza size in the XMPP-CORE RFC. It simply says that maximum stanza size is configurable.

Other than that, the res and pbx directories are clean as far as I can tell. I'll add a patch for that.

By: Mark Michelson (mmichelson) 2012-11-15 16:18:48.990-0600

The res_jabber fix is really easy since the message was going to be truncated anyway. Just copy directly into the return buffer instead of duping it first.

By: Walter Doekes (wdoekes) 2012-11-16 04:45:55.100-0600

{quote}Other than that, the res and pbx directories are clean as far as I can tell. I'll add a patch for that.{quote}

Cool.

The patch looks good.

To summarize, these dirs (in 1.8) contain strdupa's/alloca's:
{noformat}
$ wgrep . -E '(alloca|strdupa)\(' -l | sed -e 's/^..//;s/\/.*//' | sort -u
addons <-- checked
apps
cdr <-- checked
cel <-- checked
channels <-- glanced through, looks ok
funcs <-- checked
include <-- checked
main
menuselect <-- irrelevant
pbx <-- checked
res <-- checked
tests <-- irrelevant
utils <-- irrelevant (extconf.c is not used)
{noformat}

(Of course you can still often break things if you get access to the config files and are able to write very long lines, like a very large "static" value in cdr_adaptive_odc. But I suppose that's not worth doing anything about.)

By: Walter Doekes (wdoekes) 2012-11-16 05:11:07.802-0600

Ok. /addons/ looks good too.

issueA20658_sanitize_app_mysql.patch isn't a security fix, just eye sore (and one duplicate ast_strdupa) fix.

By: Walter Doekes (wdoekes) 2012-11-16 07:36:40.408-0600

issueA20658_dont_process_overlong_config_lines.patch doesn't fix a security issue either, but it makes sure that long config lines aren't processed at all.

Previously it would break up a long line into individual lines:
{noformat}
[Nov 16 14:17:04] WARNING[20588]: config.c:1362 process_text_line: No '=' (equal sign) in line 5 of /etc/asterisk/sip.conf
[Nov 16 14:17:04] WARNING[20588]: config.c:1362 process_text_line: No '=' (equal sign) in line 6 of /etc/asterisk/sip.conf
{noformat}

I did confirm that the loading of text config files already has length limitations in place :)

By: Mark Michelson (mmichelson) 2012-11-19 09:49:06.395-0600

I've been giving looks at this and I'm having difficulty finding anything further that is in need of immediate care.

I plan on fixing this issue in four separate commits. In one commit, we get the security fixes:

1) chan_sip.c (except the s/SSL/TLS/ changes)
2) http.c
3) res_jabber.c

In the second, we get the other crashes that are not necessarily security fixes:

1) func_realtime.c
2) config.c There is a grammatical error in the error message ("with with") that I will fix when I commit.

The third will be a trunk-only commit that will sanitize app_mysql.c.

The fourth will be a trunk-only commit that will replace "SSL" with "TLS" in log messages in chan_sip.c.

By: Walter Doekes (wdoekes) 2012-11-19 09:58:48.511-0600

Sounds good to me.

By: Walter Doekes (wdoekes) 2012-12-10 03:07:31.389-0600

Bump.

I see that we have new releases, but this isn't in it. Status update, anyone?

/w

By: Mark Michelson (mmichelson) 2012-12-12 07:42:44.859-0600

We're at a stage right now where test developers at Digium are ensuring that the patches provided are working. They're also working on a more difficult security vulnerability as well.

I'm not sure when a release will be out with this fix since coordination has become difficult due to lots of people taking vacation this time of year. Don't worry though, this issue hasn't been forgotten about.

By: Walter Doekes (wdoekes) 2012-12-12 10:12:39.617-0600

Oki. Thanks for the update.