oss-sec mailing list archives
Re: mpg321: Out-of-bounds Write
From: Matthew Fernandez <matthew.fernandez () gmail com>
Date: Sat, 8 Dec 2018 09:04:17 -0800
On Dec 7, 2018, at 19:16, Ren Kimura <rkx1209dev () gmail com> wrote: Hi. mpg321 is a free command-line mp3 player that is commonly available on many Linux distributions. For example, in ubuntu you can download the latest mpg321 by "apt-get install mpg321." latest mpg321 0.3.2, in scan() in mad.c calculate the number of frames using bit rate. If crafted mp3 whose bit rate equal 0 is taken, sampling time become INF value due to floating point division by 0. As a result, the frame number become a very large (1<<63), leading out of bounds write, memory corruption at mad.c:285. note. frames buffer have been allocated only 8-byte at mpg321.c:990.
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. I’m not familiar with the mpg321 code base and the above is based on a cursory inspection, so please correct me if I am wrong.
Current thread:
- mpg321: Out-of-bounds Write Ren Kimura (Dec 08)
- Re: mpg321: Out-of-bounds Write Matthew Fernandez (Dec 08)
- Re: mpg321: Out-of-bounds Write Ren Kimura (Dec 10)
- Message not available
- Re: mpg321: Out-of-bounds Write Ren Kimura (Dec 10)
- Message not available