Bug 554416 (CVE-2009-4012) - CVE-2009-4012 libthai integer/heap overflow flaws
Summary: CVE-2009-4012 libthai integer/heap overflow flaws
Keywords:
Status: CLOSED ERRATA
Alias: CVE-2009-4012
Product: Security Response
Classification: Other
Component: vulnerability
Version: unspecified
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Red Hat Product Security
QA Contact:
URL:
Whiteboard:
Depends On: 556478 556481 589990
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-01-11 16:45 UTC by Josh Bressers
Modified: 2019-09-29 12:33 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-06-03 14:29:14 UTC


Attachments (Terms of Use)
Proposed patch (1.83 KB, patch)
2010-01-11 16:45 UTC, Josh Bressers
no flags Details | Diff

Description Josh Bressers 2010-01-11 16:45:24 UTC
Created attachment 383024 [details]
Proposed patch

Debian reported multiple integer overflow flaws in libthai.

If a very long string is passed to the library, it could overflow the calculated malloc size, and can lead to arbitrary code execution.

Comment 1 Tomas Hoger 2010-01-15 09:49:13 UTC
Public now via DSA-1971-1:
  http://lists.debian.org/debian-security-announce/2010/msg00006.html

Comment 2 Tomas Hoger 2010-01-18 15:00:26 UTC
Fixed upstream in 0.1.13:
  http://linux.thai.net/node/184
  svn diff -c 374 http://linux.thai.net/svn/software/libthai/trunk/

As far as I can see, these overflows can only be triggered when th_brk_line() line is called with very long untrusted input (1G+, + extra 1G must be allocated for the output buffer).  Doing a search, there does not seem to be many applications calling th_brk_line().

Comment 5 Theppitak Karoonboonyanan 2010-01-19 05:00:44 UTC
Vulnerable path can still be:

th_brk() -> brk_maximal_do() -> brk_maximal_do_impl() -> best_brk_new()

One can pass an extremely long string to th_brk() with pure Thai characters, thus making a very long chunk and eventually overflowing the malloc() call in best_brk_new().

Comment 6 Tomas Hoger 2010-01-19 06:53:22 UTC
(In reply to comment #5)

> th_brk() -> brk_maximal_do() -> brk_maximal_do_impl() -> best_brk_new()

n_brk_pos passed to best_brk_new() in such a case should be less than or equal to n passed to th_brk().  th_brk() also needs int pos[], which needs to have a size of n.  Hence n should be either low enough to be safe, or there must be an overflow in pos[] allocation in the function calling th_brk(), as in th_brk_line().

Comment 7 Theppitak Karoonboonyanan 2010-01-19 09:00:42 UTC
(In reply to comment #6)

> n_brk_pos passed to best_brk_new() in such a case should be less than or equal
> to n passed to th_brk().  th_brk() also needs int pos[], which needs to have a
> size of n.  Hence n should be either low enough to be safe, or there must be an
> overflow in pos[] allocation in the function calling th_brk(), as in
> th_brk_line().    

Does this mean pango-thai-lang is vulnerable?

pango/modules/thai/thai-lang.c (thai_engine_break).

Comment 8 Tomas Hoger 2010-01-19 09:46:40 UTC
Reference:
  http://git.gnome.org/browse/pango/tree/modules/thai/thai-lang.c

Seems to be affected in a similar way as th_brk_line(), given the g_new implementation.

Comment 9 Theppitak Karoonboonyanan 2010-01-19 13:44:32 UTC
Where should we fix this? glib, I guess?

Comment 10 Tomas Hoger 2010-01-22 09:31:07 UTC
I agree it makes sense to have safe g_*new* in glib, but it seems that can only be achieved by introducing some sort of *_n (as g_malloc_n) variants of functions wrapped by g_*new* macros.  Those functions need to accept sizeof(struct_type) (instead of struct_type) and n_structs, do overflow check and call appropriate g_*malloc* function.  Fewer functions may do, if they do accept function pointer as an additional argument (i.e. single _n function for g_new and g_new0, possibly also for _try_ versions).  Adding Matthias to CC for some ideas / comments from glib point of view.

Theppitak, shouldn't brk_maximal_do_impl() check best_brk_new() return value too to avoid possible NULL deref crashes?  Yeah, I know, nitpick, as short pos[] may cause corruption in th_brk() already.

Comment 11 Matthias Clasen 2010-01-22 13:34:52 UTC
> Those functions need to accept
> sizeof(struct_type) (instead of struct_type) and n_structs, do overflow check
> and call appropriate g_*malloc* function.

Why ? Whats the problem doing overflow checking in the current g_new macro ?

But I kinda disagree with doing overflow checks for every g_new in the first place. 99% of all g_new calls have a second parameter of 1.

Comment 12 Tomas Hoger 2010-01-22 14:11:47 UTC
(In reply to comment #11)

> Why ? Whats the problem doing overflow checking in the current g_new macro ?

Well, depends on what you want to do in the bad case.  Ternary operator may help if the bad case is expected to return NULL (probably not the right fix) or call g_malloc(G_MAXSIZE) (to trigger malloc error).

Ok, so something like this can work without extra function call:

#define g_new(struct_type, n_structs)      \
    ((n_structs > G_MAXSIZE / sizeof (struct_type)) ? \
        ((struct_type *) g_malloc (G_MAXSIZE)) :  \
        ((struct_type *) g_malloc (((gsize) sizeof (struct_type)) \
            * ((gsize) (n_structs)))) \
    )

> But I kinda disagree with doing overflow checks for every g_new in the first
> place. 99% of all g_new calls have a second parameter of 1.    

Whenever the second argument is known at compile time, compiler should be able to optimize the extra code out, right?

Comment 13 Theppitak Karoonboonyanan 2010-01-25 15:55:09 UTC
(In reply to comment #10)
> I agree it makes sense to have safe g_*new* in glib, but it seems that can only
> be achieved by introducing some sort of *_n (as g_malloc_n) variants of
> functions wrapped by g_*new* macros.  Those functions need to accept
> sizeof(struct_type) (instead of struct_type) and n_structs, do overflow check
> and call appropriate g_*malloc* function.  Fewer functions may do, if they do
> accept function pointer as an additional argument (i.e. single _n function for
> g_new and g_new0, possibly also for _try_ versions).  Adding Matthias to CC for
> some ideas / comments from glib point of view.

I initially thought about something like:

#define g_new(struct_type, n_structs)           \
    ((gsize) (n_structs) > SIZE_MAX / ((gsize) sizeof (struct_type)) ? \
        NULL : (struct_type *) g_malloc (((gsize) sizeof (struct_type)) * ((gsize) (n_structs))))

Probably, it expands and causes too much additional binary?

> Theppitak, shouldn't brk_maximal_do_impl() check best_brk_new() return value
> too to avoid possible NULL deref crashes?  Yeah, I know, nitpick, as short
> pos[] may cause corruption in th_brk() already.    

I agree that the return code should be checked. Done:

  svn diff -c 390 http://linux.thai.net/svn/software/libthai/trunk/

Comment 14 Tomas Hoger 2010-01-26 09:59:30 UTC
(In reply to comment #13)

> #define g_new(struct_type, n_structs)           \
>     ((gsize) (n_structs) > SIZE_MAX / ((gsize) sizeof (struct_type)) ? \
>         NULL : (struct_type *) g_malloc (((gsize) sizeof (struct_type)) *
> ((gsize) (n_structs))))

This should be right for g_try_new, g_new is documented to only return NULL when n_structs == 0.  I proposed this for the overflow case:

  ((struct_type *) g_malloc (G_MAXSIZE))

Comment 16 Theppitak Karoonboonyanan 2010-01-26 11:13:19 UTC
Ah, I see. g_new() aborts the program on failure. That's why you proposed the g_malloc_n() in the first place.

Shouldn't overflow be handled as failure? With G_MAXSIZE allocation, I'm afraid the callers which assume the successful return will overflow the heap when accessing it anyway.

Comment 17 Tomas Hoger 2010-01-26 11:44:20 UTC
(In reply to comment #16)
> Ah, I see. g_new() aborts the program on failure. That's why you proposed the
> g_malloc_n() in the first place.

Yes, _n would do overflow check and g_error() with a specific error message.

> Shouldn't overflow be handled as failure? With G_MAXSIZE allocation, I'm afraid
> the callers which assume the successful return will overflow the heap when
> accessing it anyway.    

G_MAXSIZE allocation should always fail (it requests buffer spanning almost whole address space).

Comment 19 Tomas Hoger 2010-01-26 14:48:20 UTC
Making comment #11 and comment #12 public.

Comment 21 Behdad Esfahbod 2010-01-26 22:47:09 UTC
Proposed glib patch upstream:
https://bugzilla.gnome.org/show_bug.cgi?id=608196

Comment 22 Kenichi Takemura 2010-08-03 04:47:34 UTC
Guys

Reproducers are available around?

Comment 23 Vincent Danen 2010-08-17 20:28:29 UTC
(In reply to comment #22)
> Guys
> 
> Reproducers are available around?

Not that I'm aware of.  I did a quick look and I didn't see anything either.


Note You need to log in before you can comment on or make changes to this bug.