Re: mpg321: Out-of-bounds Write

[Ren Kimura <rkx1209dev () gmail com>]

2018年12月10日(月) 12:44 Matthew Fernandez <matthew.fernandez () gmail com>:
> On Sun, 9 Dec 2018 at 15:11, Ren Kimura <rkx1209dev () gmail com> wrote:
> > > 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
>
> Did you report this upstream?

Yes. I've reported it to Ubuntu security team.
But there is no response yet.

Ren Kimura