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.
Public now via DSA-1971-1: http://lists.debian.org/debian-security-announce/2010/msg00006.html
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().
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().
(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().
(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).
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.
Where should we fix this? glib, I guess?
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.
> 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.
(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?
(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/
(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))
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.
(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).
Making comment #11 and comment #12 public.
Proposed glib patch upstream: https://bugzilla.gnome.org/show_bug.cgi?id=608196
Guys Reproducers are available around?
(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.