Bug#867283: Crash in glibc's mktime in low-memory situations
(Mail was initially only sent to Carlos, sorry for that :)) > If you have the opportunity please file a match bug with upstream at > sourceware.org/bugzilla. That way upstream is made aware of the issue. The bug is now being tracked at: https://sourceware.org/bugzilla/show_bug.cgi?id=21716
Bug#867283: Crash in glibc's mktime in low-memory situations
Am 05.07.2017 um 21:56 schrieb Carlos O'Donell: I agree. If you have the opportunity please file a match bug with upstream at sourceware.org/bugzilla. That way upstream is made aware of the issue. Sure, I'll report the bug there. Cheers, Johannes
Bug#867283: Crash in glibc's mktime in low-memory situations
> None of the internal assertions in tzfile.c have to do with low
> memory, they have to do with logical consistency and expected
> outcomes.
Okay, so let's look at the stack trace again and where it failed.
The failing line 779 in __tzfile_compute is:
if (__tzname[0] == NULL)
{
assert (num_types == 1); // <-- boom
So, where is __tzname[0] being set? Depending on the path taken, it can
be either of these:
line 627: __tzname[0] = NULL; // initial value
line 646: __tzname[0] = __tzstring (_names[types[i].idx]);
line 686: __tzname[0] = __tzstring (zone_names);
line 756: __tzname[dst] = __tzstring (_names[idx]);
Internally, __tzstring calls malloc. If malloc fails, it returns NULL.
So it is entirely possible that this assertion will fail because of an
out-of-memory situation.
No of course this is bad, but so far the integrity has not been
compromised. It just means that the function really should return with
an error now. As far as I can see there are currently no facilities for
returning an error in this particular function, but I guess it really
should be able to propagate allocation failures to the library functions
that call it, so that those can return an error to the user (if that is
part of their API contract, that is).
Cheers,
Johannes
Bug#867283: Crash in glibc's mktime in low-memory situations
Package: glibc Version: 2.19-18+deb8u10 By fuzzing my own software using American Fuzzy Lop and its provided libdislocator (an "abusive allocator" library), I found a code path in glibc that causes a SIGABRT where it probably shouldn't. In a low-memory situation, I got the following stack trace: #0 0x76319067 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x7631a448 in __GI_abort () at abort.c:89 #2 0x76312266 in __assert_fail_base (fmt=0x7644af18 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x7644893f "num_types == 1", file=file@entry=0x76448936 "tzfile.c", line=line@entry=779, function=function@entry=0x7644fc90 <__PRETTY_FUNCTION__.6261> "__tzfile_compute") at assert.c:92 #3 0x76312312 in __GI___assert_fail (assertion=assertion@entry=0x7644893f "num_types == 1", file=file@entry=0x76448936 "tzfile.c", line=line@entry=779, function=function@entry=0x7644fc90 <__PRETTY_FUNCTION__.6261> "__tzfile_compute") at assert.c:101 #4 0x76391907 in __tzfile_compute (timer=1447111074, use_localtime=use_localtime@entry=1, leap_correct=leap_correct@entry=0x7fffd848, leap_hit=leap_hit@entry=0x7fffd844, tp=tp@entry=0x7fffd960) at tzfile.c:779 #5 0x76390429 in __tz_convert (timer=0x7fffd948, use_localtime=1, tp=0x7fffd960) at tzset.c:635 #6 0x7638eab0 in ranged_convert (convert=0x7638e940 <__localtime_r>, t=0x7fffd948, tp=0x7fffd960) at mktime.c:310 #7 0x7638edd5 in __mktime_internal (tp=0x7fffdab0, convert=0x7638e940 <__localtime_r>, offset=0x7668bab8 ) at mktime.c:478 #8 0x76c02083 in OpenMPT::mpt::Date::Unix::FromUTC (timeUtc=...) at common/mptTime.cpp:115 mktime is supposed to return -1 and, according to cplusplus.com, has a no-throw guarantee for C++ code. So even if some internal memory cannot be allocated, I expect mktime to return with an error value and not cause a SIGABRT. I found it difficult to reproduce this outside my afl-instrumented environment, but I hope the stack trace helps with verifying whether this is a valid bug. If you need a test case to reproduce, let me know. Cheers, Johannes
Bug#864195: libopenmpt: Security updates libopenmpt-0.2.7386-beta20.3-p7 available
Am 12.06.2017 um 18:17 schrieb James Cowgill: On 08/06/17 23:10, Johannes Schultz wrote: Hi, I guess it depends on what you define as "reasonable". Depending on the malformed file and setup, they may take minutes to load (given that enough (virtual) memory is available to load all the truncated samples). The test cases that were generated by American Fuzzy Lop were about 5KB in size and took about 10 seconds to load on my machine, which I would say is quite excessive for such a tiny file, but those examples can be modified (by adding more malformed sample slots) to extend the loading time from seconds to minutes while still being about 5KB. To give some perspective, I don't just use libopenmpt on Desktop systems but also to scan module data in a server application where users can upload their own modules, and if a user could keep a server request busy for a minute (while also wasting tons of memory and CPU time), that server could be DOSed very easily. Thus I'd strongly suggest to include those two patches. I see, that is quite a long time to wait! Do you have these testcases so I can try them? My only concern is that p3 is pretty big so it has the potential of causing a regression. p5 looks fine. I have attached some manually-crafted worst-case files to demonstrate the issue. For maximum effect, a 64-bit build has to be used, because otherwise memory allocation will fail fairly quickly and the malformed sample data won't even be attempted to be decoded. p3 is smaller than it looks since some of it is just moving around things and as a consequence re-indenting them. The actual decoding logic is not modified. A diff without whitespaces should show this. Thanks. Do any of these issues have CVE numbers? Not that I'm aware of. We didn't request any. Cheers, Johannes
Bug#864195: libopenmpt: Security updates libopenmpt-0.2.7386-beta20.3-p7 available
Hi, I guess it depends on what you define as "reasonable". Depending on the malformed file and setup, they may take minutes to load (given that enough (virtual) memory is available to load all the truncated samples). The test cases that were generated by American Fuzzy Lop were about 5KB in size and took about 10 seconds to load on my machine, which I would say is quite excessive for such a tiny file, but those examples can be modified (by adding more malformed sample slots) to extend the loading time from seconds to minutes while still being about 5KB. To give some perspective, I don't just use libopenmpt on Desktop systems but also to scan module data in a server application where users can upload their own modules, and if a user could keep a server request busy for a minute (while also wasting tons of memory and CPU time), that server could be DOSed very easily. Thus I'd strongly suggest to include those two patches. I see, that is quite a long time to wait! Do you have these testcases so I can try them? My only concern is that p3 is pretty big so it has the potential of causing a regression. p5 looks fine. I have attached some manually-crafted worst-case files to demonstrate the issue. For maximum effect, a 64-bit build has to be used, because otherwise memory allocation will fail fairly quickly and the malformed sample data won't even be attempted to be decoded. p3 is smaller than it looks since some of it is just moving around things and as a consequence re-indenting them. The actual decoding logic is not modified. A diff without whitespaces should show this. Cheers, Johannes hangs.7z Description: Binary data
Bug#864195: libopenmpt: Security updates libopenmpt-0.2.7386-beta20.3-p7 available
I don't understand patch p6 well enough to say how serious it is (depends on where the invalid pointer being dereferenced comes from). As far as I know, it is just a NULL pointer. Johannes did the analysis and might be able to elaborate (CCed). Correct. I am not sure if it is possible at all to trigger that field to be NULL in libopenmpt but it is possible in OpenMPT in some live editing situations. I cannot be 100% sure if libopenmpt would ever be able to trigger this crash but it should be obvious that adding a null-pointer check should do no harm to the library. p3-excessive-cpu-consumption-on-malformed-files-dmf-mdl.patch p5-excessive-cpu-consumption-on-malformed-files-ams.patch Are these actually security bugs? As long as the code finishes in a reasonable amount of time and produces the right results, then there's not much harm in leaving the code as it is. Again, Johannes knows more about these. I guess it depends on what you define as "reasonable". Depending on the malformed file and setup, they may take minutes to load (given that enough (virtual) memory is available to load all the truncated samples). The test cases that were generated by American Fuzzy Lop were about 5KB in size and took about 10 seconds to load on my machine, which I would say is quite excessive for such a tiny file, but those examples can be modified (by adding more malformed sample slots) to extend the loading time from seconds to minutes while still being about 5KB. To give some perspective, I don't just use libopenmpt on Desktop systems but also to scan module data in a server application where users can upload their own modules, and if a user could keep a server request busy for a minute (while also wasting tons of memory and CPU time), that server could be DOSed very easily. Thus I'd strongly suggest to include those two patches. Cheers, Johannes
