[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sun, 9 Dec 2018 18:11:00 -0500
From: Ren Kimura <rkx1209dev@...il.com>
To: oss-security@...ts.openwall.com, matthew.fernandez@...il.com
Subject: Re: mpg321: Out-of-bounds Write
> Did you report this one upstream? In trying to understand this, it looks to me like the problem isn’t that mpg321 fails
> to check the bitrate is positive, but rather that there’s an unchecked malloc elsewhere.
>
> The point where the OOB write occurs (mad.c:285) looks like the following:
>
> 282 /* update cached table of frames & times */
> 283 if (current_frame <= playbuf->num_frames) /* we only allocate enough for our estimate. */
> 284 {
> 285 playbuf->frames[current_frame] = playbuf->frames[current_frame-1] + (header->bitrate / 8 / 1000)
> 286 * mad_timer_count(header->duration, MAD_UNITS_MILLISECONDS);
> 287 playbuf->times[current_frame] = current_time;
>
> At this point, header->bitrate is 0 and playbuf->num_frames is the correct limit to check against for this buffer. The
> problem seems to stem from the point at which playbuf->frames was allocated (mpg321.c:990):
> 985 if ((options.maxframes != -1) && (options.maxframes <= playbuf.num_frames))
> 986 {
> 987 playbuf.max_frames = options.maxframes;
> 988 }
> 989
> 990 playbuf.frames = malloc((playbuf.num_frames + 1) * sizeof(void*));
> 991 playbuf.times = malloc((playbuf.num_frames + 1) * sizeof(mad_timer_t));
> 992 #ifdef __uClinux__
> 993 if((playbuf.buf = mmap(0, playbuf.length, PROT_READ, MAP_PRIVATE, fd, 0)) == MAP_FAILED)
> 994 #else
> 995 if((playbuf.buf = mmap(0, playbuf.length, PROT_READ, MAP_SHARED, fd, 0)) == MAP_FAILED)
> 996 #endif
>
> At this point, playbuf.num_frames is whatever the your platform happens to yield when ∞ is cast to a long (undefined
> behavior in C). AFAICT there is no check that malloc succeeded before the code later writes to the frames array (the
> same applies to playbuf.times). Poking around a bit more, this (unchecked malloc) seems common in the code.
checking malloc status is not enough, because playbuf.num_frames can
be very large value, in my environment Ubuntu 18.04, gcc 7.03,
it becomes 0x8000000000000000.
990 playbuf.frames = malloc((playbuf.num_frames + 1) *
sizeof(void*));
So at this point it try to calculate (0x8000000000000000 + 1) * 8 =
0x8 (INTEGER OVERFLOW).
As a result malloc succeed but it only allocate 0x8 byte buffer, lead
OOB write at following points.
283 if (current_frame <= playbuf->num_frames) /* we only allocate
enough for our estimate. */
285 playbuf->frames[current_frame] =
playbuf->frames[current_frame-1] + (header->bitrate / 8 / 1000)
286 * mad_timer_count(header->duration, MAD_UNITS_MILLISECONDS);
287 playbuf->times[current_frame] = current_time;
The value of playbuf.num_frames may depend on platform because it's
calculated from INF value. (undefined behavior)
I only tried to Ubuntu package of mpg321 (may be compiled by gcc?). At
least on ubuntu, OOB write always happen due to above reason.
Ren Kimura
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
