A heap-use-after-free in icalreqstattype_as_string_r

[agustinmista]

Hello, we recently found a memory issue parsing and executing fuzzed ical file in last revision of libical (#19acf43794ad4c99f7e6687cb39424a82b737828).
We tested this issue on Ubuntu 14.04 but other configurations could be affected.
Technical details about the issue are:

==8739==ERROR: AddressSanitizer: heap-use-after-free on address 0x6020000057d4 at pc 0x0000004446ee bp 0x7fffffffc4a0 sp 0x7fffffffbc28
READ of size 2 at 0x6020000057d4 thread T0

gdb backtrace is as follows:

#0  0x00007ffff61cfc37 in __GI_raise (sig=sig@entry=6)
    at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007ffff61d3028 in __GI_abort () at abort.c:89
#2  0x00000000004b12b6 in __sanitizer::Abort() ()
#3  0x00000000004a1f97 in __asan::AsanDie() ()
#4  0x00000000004a89cf in __sanitizer::Die() ()
#5  0x00000000004a062b in __asan::ScopedInErrorReport::~ScopedInErrorReport() ()
#6  0x00000000004a0171 in __asan_report_error ()
#7  0x0000000000444709 in printf_common(void*, char const*, __va_list_tag*) ()
#8  0x0000000000444e14 in vsnprintf ()
#9  0x0000000000446151 in snprintf ()
#10 0x00007ffff7b51af9 in icalreqstattype_as_string_r (stat=...)
    at /home/agustin/Code/libical/src/libical/icaltypes.c:171
#11 0x00007ffff7b5a520 in icalvalue_as_ical_string_r (value=0x60e000004100)
    at /home/agustin/Code/libical/src/libical/icalvalue.c:1208
#12 0x00007ffff7add1c4 in icalproperty_as_ical_string_r (prop=0x606000009c80)
    at /home/agustin/Code/libical/src/libical/icalproperty.c:442
#13 0x00007ffff7a978ed in icalcomponent_as_ical_string_r (impl=0x607000008980)
    at /home/agustin/Code/libical/src/libical/icalcomponent.c:291
#14 0x00007ffff7a97b4b in icalcomponent_as_ical_string_r (impl=0x60700000ded0)
    at /home/agustin/Code/libical/src/libical/icalcomponent.c:300
#15 0x00007ffff7a96f12 in icalcomponent_as_ical_string (impl=0x60700000ded0)
    at /home/agustin/Code/libical/src/libical/icalcomponent.c:247
#16 0x00000000004b8cbd in main (argc=2, argv=0x7fffffffdf08)
    at /home/agustin/Code/libical/src/test/icaltestparser.c:108

This issue was found using QuickFuzz, the file to reproduce it is attached.
Regards.

[ksmurchison]

I'm not sure how to fix this one without breaking other stuff. icaltypes.h says the following about the debug string:

'The
"debug" string is a pointer into the string that the called passed
into to icalreqstattype_from_string. Don't try to free it either, and
don't use it after the original string has been freed.'

In the case of icaltestparser.c we are certainly freeing the string before using it.
We could actually make debug be a copy of the string, but a lot of other code depends on it being static and would leak it.

[winterz]

this patch using the ring buffer based memory handling maybe?

[1350]$ diff src/libical/icaltypes-asan.c src/libical/icaltypes.c
143c143
<         stat.debug = icalmemory_tmp_copy(p2 + 1);
---
>         stat.debug = p2 + 1;

[ksmurchison]

That would probably work, but I wonder if we'd have the same issue that we had with rscale in a multi-threaded app

[rhertzog]

This issue is know as CVE-2016-9584 according to http://www.openwall.com/lists/oss-security/2017/01/20/16

[lamby]

Does this fix both CVE-2016-9584 and CVE-2016-5824?

[winterz]

yes. from what I can tell, both CVEs point to the same code problem. the patch I showed earlier in this issue is not the best solution, and I don't know if multithreaded apps will work properly with it.

[lamby]

Any further thoughts? :)

[winterz]

we don't have a proper fix yet.

[winterz]

I committed the stat.debug = icalmemory_tmp_copy(p2 + 1); as shown in the earlier comments.
My local tests show that ASAN is happy now. should fix both CVEs. please test.

I don't know if threaded apps will barf on this.

let's close for now until someone finds a problem.

[anselmolsm]

A typo prevented the commit mentioned above to be linked to this issue. I'll add the link here to help future generations ;) 6b9438d