[Home]

Summary:ASTERISK-10526: [patch] multiple buffer problems with DUNDi
Reporter:Dmitry Andrianov (dimas)Labels:
Date Opened:2007-10-14 16:17:37Date Closed:2007-10-15 10:34:42
Priority:MinorRegression?No
Status:Closed/CompleteComponents:PBX/pbx_dundi
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) dundiparser.patch
( 1) pbxdundi.patch
Description:During debuggint DUNDi stuff I found several strange things in debug output which led me to several fixes:

1. pbx/pbx_dundi.c does not initialize nor zaro-terminate buffer before passing it to ast_canmatch_extension. I'm not sure what it can affect I just noticed that packet debug output HINT sometimes contains garbage.

2. There were lines like:

       ANSWER          : [EXISTS|CANMATCH] 0 <IAX/dundi@XX.XX.XX.XX/4444@dundi-users�> from [00:15:17:13:a6:c3]

note the extra character after the context name. I looked to pbx/dundi-parser.c code and found the following. In the dump_* functions:

1. There are many constants like in:

memcpy(tmp + strlen(tmp), value + 2, len - 2)

in dump_hint(). The "2" is sizeof unsigned short - the flags field. I have no idea why the code is full of these when you have properly defined struct for that (dundi_hint).

2. There is alot of "buffer magic". Instead of doing simple snprintf, there are places using several buffers to concatenate intermediate results and then finally copy to the output buffer.

3. There are potential buffer overruns (I do not know exploitable or not). For example, in the dump_hint, the code:

memcpy(tmp + strlen(tmp), value + 2, len - 2)

does not verify there is enough space in the "tmp" buffer.

****** ADDITIONAL INFORMATION ******

I put patches into two separate files because the one pbxdundi.patch is just a clear bugfix while dundiparser.patch in addition to bugfixes modifies the way how string is constructed and you may consider this to be more like feature request.
Comments:By: Dmitry Andrianov (dimas) 2007-10-15 02:30:19

From my experience, running Asterisk under valgrind was so far the best way to catch these nasty memory related issues which are very difficult to track otherwise.
However it slows system down alot so maybe you won't be able doing it in production...
By: Russell Bryant (russell) 2007-10-15 07:32:52

Thanks a lot for pointing these out.  I'm going to mark the issue private for now while I look at the issues in detail to determine if there are any exploitable issues here.
By: Dmitry Andrianov (dimas) 2007-10-15 07:33:29

oops. my comment about valgrind was for completely unrelated issue :)
By: Russell Bryant (russell) 2007-10-15 09:05:45

It's ok, I don't think there is ever a bad time to plug using valgrind.  It rocks.  :)
By: Dmitry Andrianov (dimas) 2007-10-15 09:44:49

True. But that time I was suggesting using valgring to submitter of some other issue - there were signs of memory corruption. And somehow I typed it in wrong window :)
Will find proper issue for my comment and copy it there :)
By: Russell Bryant (russell) 2007-10-15 10:00:46

(marked as public again)

I do not think any of the potential overflows you have pointed out are exploitable, mostly because the code makes assumptions about the maximum information element length that can be passed in.  The DUNDi protocol only allows a maximum of 255 byte information element payloads.

I still like your second patch, though, and will apply it to trunk.  The bug fix will go to both 1.4 and trunk.
By: Digium Subversion (svnbot) 2007-10-15 10:20:04

Repository: asterisk
Revision: 85556

U   branches/1.4/pbx/pbx_dundi.c

------------------------------------------------------------------------
r85556 | russell | 2007-10-15 10:20:03 -0500 (Mon, 15 Oct 2007) | 9 lines

Ensure the buffer passed to ast_canmatch_extension() is properly initialized so
that it is null terminated.

(issue ASTERISK-10526)
Reported by: dimas
Patches:
     pbxdundi.patch uploaded by dimas (license 88)
   - small mods by me

------------------------------------------------------------------------
By: Digium Subversion (svnbot) 2007-10-15 10:27:05

Repository: asterisk
Revision: 85557

_U  trunk/
U   trunk/pbx/pbx_dundi.c

------------------------------------------------------------------------
r85557 | russell | 2007-10-15 10:27:04 -0500 (Mon, 15 Oct 2007) | 17 lines

Merged revisions 85556 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r85556 | russell | 2007-10-15 10:40:45 -0500 (Mon, 15 Oct 2007) | 9 lines

Ensure the buffer passed to ast_canmatch_extension() is properly initialized so
that it is null terminated.

(issue ASTERISK-10526)
Reported by: dimas
Patches:
     pbxdundi.patch uploaded by dimas (license 88)
   - small mods by me

........

------------------------------------------------------------------------
By: Digium Subversion (svnbot) 2007-10-15 10:34:42

Repository: asterisk
Revision: 85558

U   trunk/pbx/dundi-parser.c

------------------------------------------------------------------------
r85558 | russell | 2007-10-15 10:34:41 -0500 (Mon, 15 Oct 2007) | 8 lines

Simplify buffer handling in dundi-parser.c.  This also makes the code a bit
safer by removing various assumptions about sizes. (No vulnerabilities, though)

(closes issue ASTERISK-10526)
Reported by: dimas
Patches:
     dundiparser.patch uploaded by dimas (license 88)

------------------------------------------------------------------------