[OpenSER-Devel] [ openser-Bugs-1873335 ] Possible buffer overflow in get_all_db_ucontacts

[SourceForge.net]

Bugs item #1873335, was opened at 2008-01-16 16:15
Message generated for change (Comment added) made by sipphonematt
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=743020&aid=1873335&group_id=139143

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: modules
Group: ver devel
Status: Open
Resolution: Fixed
Priority: 5
Private: No
Submitted By: Matt Reilly (sipphonematt)
Assigned to: Bogdan-Andrei Iancu (bogdan_iancu)
Summary: Possible buffer overflow in get_all_db_ucontacts

Initial Comment:
The get_all_db_ucontacts() function in modules/usrloc/dlist.c has a possible buffer overflow when adding the path column to the results.

The function is passed a buffer "buf" (which is copied into the variable "cp").

The size of this buffer is checked before adding the contact, socket and dbflags:

   needed = (int)(sizeof(p_len)+p_len+sizeof(sock)+sizeof(dbflags));
   if (len < needed) {
      shortage += needed ;
       continue;
   }

However, the size of the path is never checked before adding this to the buffer:

   /* path */
   p = (char*)VAL_STRING(ROW_VALUES(row)+4);
   if (VAL_NULL(ROW_VALUES(row)+4) || p==0 || p[0]==0){
        p = NULL;
        p_len = 0;
   } else {
        p_len = strlen(p);
   }

   /* write path */
   memcpy(cp, &p_len, sizeof(p_len));
   cp = (char*)cp + sizeof(p_len);
   memcpy(cp, p, p_len);
   cp = (char*)cp + p_len;


Not only is the buffer not check that it can hold the path, the length that's added is not subtracted from "len". i.e. there is no "len -= sizeof(p_len) + p_len;" 

To solve, "needed" should also add the space used by the path data.


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

>Comment By: Matt Reilly (sipphonematt)
Date: 2008-01-25 12:58

Message:
Logged In: YES 
user_id=1965181
Originator: YES

Looks good to me except for this line:
  memcpy(cp, &p1_len, sizeof(p_len));
should be:
  memcpy(cp, &p1_len, sizeof(p1_len));

i.e. change sizeof(p_len) to sizeof(p1_len)

It doesn't make a functional difference since sizeof(p_len) ==
sizeof(p1_len), but it would be cleaner.


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

Comment By: Bogdan-Andrei Iancu (bogdan_iancu)
Date: 2008-01-25 01:44

Message:
Logged In: YES 
user_id=1275325
Originator: NO

Hi Matt,

I made a fix on the trunk version - please take a look and let me know if
ok. If so, I will do the backport.

Thanks & Best regards,
Bogdan

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

Comment By: Bogdan-Andrei Iancu (bogdan_iancu)
Date: 2008-01-21 08:39

Message:
Logged In: YES 
user_id=1275325
Originator: NO

Hi Matt,

it sounds right to me - I will take a closer look and do a fix.

Thanks and regards,
Bogdan

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

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=743020&aid=1873335&group_id=139143