Re: buffer overflow

[Baurzhan Ismagulov]

Hello Pawel,

On Mon, Feb 19, 2007 at 08:34:25PM +0100, Pawel Kot wrote:
> Could you please give more details on the problems? Ie. show the debug
> output and the backtrace of the core dump. That would allow me to
> understand what went wrong easier.

There isn't a meaningful stack trace since the stack is corrupt:

Program received signal SIGSEGV, Segmentation fault.
0xb7ec9eb3 in AT_WritePhonebook (data=0x30303030, state=0x30303030)
    at atsoer.c:175
175                     tmp[len++] = '"'; tmp[len++] = '\r';
(gdb) bt
#0  0xb7ec9eb3 in AT_WritePhonebook (data=0x30303030, state=0x30303030)
    at atsoer.c:175
#1  0x30303030 in ?? ()
#2  0x30303030 in ?? ()
#3  0x30303030 in ?? ()
#4  0x00303030 in ?? ()
#5  0x0835deb5 in ?? ()
#6  0x0835deb6 in ?? ()
#7  0xb7ee9190 in ?? ()
   from 
/home/ibr/tmp/w/h/gnokii/src/gnokii-built.orig/common/.libs/libgnokii.so.3
#8  0xb7126258 in ?? ()
#9  0xb7e93021 in gn_phonebook_entry_sanitize (entry=Cannot access memory at 
address 0x30303038
) at gsm-common.c:166
Backtrace stopped: previous frame inner to this frame (corrupt stack?)


We've come here like this (which is actually not important):

Breakpoint 3, AT_WritePhonebook (data=0xb7126280, state=0x82d5608)
    at atsoer.c:174
174                     len = at_encode(drvinst->charset, tmp, 
data->phonebook_entry->name, len);
(gdb) bt
#0  AT_WritePhonebook (data=0xb7126280, state=0x82d5608) at atsoer.c:174
#1  0xb7ec3d9a in Functions (op=GN_OP_WritePhonebook, data=0xb7126280,
    state=0x82d5608) at atgen.c:269
#2  0xb7e7999e in gn_sm_functions (op=GN_OP_WritePhonebook, data=0xb7126280,
    sm=0x82d5608) at gsm-statemachine.c:347
#3  0x08085d70 in A_WriteMemoryLocation (data=0x8340cf0)
    at xgnokii_lowlevel.c:548
#4  0x08087676 in GUI_Connect (a=0x0) at xgnokii_lowlevel.c:1313
#5  0xb77990bd in start_thread () from /lib/tls/libpthread.so.0
#6  0xb772d92e in clone () from /lib/tls/libc.so.6
(gdb) p data->phonebook_entry->number
$2 = "xxxxxxxxxxxx", '\0' <repeats 37 times>
(gdb) p data->phonebook_entry->name
$3 = "xxxxxxxxxxxxxxx", '\0' <repeats 46 times>
(gdb) n
175                     tmp[len++] = '"'; tmp[len++] = '\r';

The last "next" causes the SIGSEGV, since at that moment the stack is
already corrupt.


You can see the problem if you analyse the at_encode -> char_ucs2_encode
-> char_uni_alphabet_encode -> char_mbtowc chain:

1. The name has 15 us-ascii characters, and at_encode requests to scan
   60 bytes.

2. char_ucs2_encode seems to be originally designed to work with src and
   len. Afterwards it seems to have been modified to check for the null
   byte -- but we don't need both length and zero termination. The
   second problem is not purely conceptual, since
   char_uni_alphabet_encode never returns zero: I HAVE_ICONV, and null
   byte is a valid multibyte sequence, so we scan all 60 bytes,
   successfully writing 240 digits to dest. Add 11 bytes for the AT
   command and 48 bytes for the number, and we are well beyond the
   256-byte buffer. Above you see the stack overwritten with '0'
   characters.

3. char_uni_alphabet_encode doesn't receive the length of the rest of
   the input and always scans MB_CUR_MAX. This means, the program reads
   uninitialized data, which could theoretically lead to a problem on
   some platform.

So, the patch addresses these issues:

1. at_encode passes the real input length to char_ucs2_encode.

2. char_ucs2_encode passes the input length to char_uni_alphabet_encode.

3. char_uni_alphabet_encode uses the actual input length.


There are some other issues I would like to discuss:

1. Looks like char_mbtowc would work inconsistently depending on the
   implementation. E.g., I don't see how it could return zero if iconv
   is used.

2. I don't understand the motivation behind preferring iconv if C99 API
   is available. Is iconv less buggy than mbtowc on an average system?

3. I'm not sure that char_ucs2_encode still needs the switch (length)
   case 0, but I've left it for now.

4. There are two implementations of AT_WritePhonebook. They are mostly
   the same, and will need fixing in two places. It would be better to
   merge them, but I haven't looked yet which class the functions belong
   to.

5. The test suite contains test cases that FAIL.

6. Does test suite run correctly without make install?

7. gnokii seems to call AT_WritePhonebook for non-existent entries,
   which results in an error 8. I see the AT_DeletePhonebook function,
   should it be called instead?


Hopefully, I've provided enough background for you to be able to review
the patch. As I already said, this is not the final version -- I need to
remove the debugging statements and rewrite the second
AT_WritePhonebook. I will do that if you approve the general approach.


In my experience, such basic things -- strings, encodings, etc. -- are
very fragile. It's very easy to break something for some use case or
platform you are not testing right now. If you are interested, we could
work on a test case for at_encode & Co. But first I have to get the
contacts onto my K750i :) .


With kind regards,
-- 
Baurzhan Ismagulov
http://www.kz-easy.com/