[FFmpeg-devel] [PATCH] AAC Decoder round 4

[Michael Niedermayer]

On Wed, Jul 30, 2008 at 10:06:00PM +0100, Robert Swain wrote:
> 2008/7/30 Robert Swain <robert.swain at gmail.com>:
> > 2008/7/30 Robert Swain <robert.swain at gmail.com>:
> >> 2008/7/30 Michael Niedermayer <michaelni at gmx.at>:
> >>> On Wed, Jul 30, 2008 at 05:10:42PM +0100, Robert Swain wrote:
> >>>> 2008/7/30 Michael Niedermayer <michaelni at gmx.at>:
> >>>> > On Wed, Jul 30, 2008 at 01:41:54PM +0100, Robert Swain wrote:
> >>> [..]
> >>>> > [...]
> >>>> >> >> +    ics->num_window_groups = 1;
> >>>> >> >> +    ics->group_len[0] = 1;
> >>>> >> >> +    if (ics->window_sequence == EIGHT_SHORT_SEQUENCE) {
> >>>> >> >> +        int i;
> >>>> >> >> +        ics->max_sfb = get_bits(gb, 4);
> >>>> >> >> +        grouping = get_bits(gb, 7);
> >>>> >> >> +        for (i = 0; i < 7; i++) {
> >>>> >> >> +            if (grouping & (1<<(6-i))) {
> >>>> >> >> +                ics->group_len[ics->num_window_groups-1]++;
> >>>> >> >> +            } else {
> >>>> >> >> +                ics->num_window_groups++;
> >>>> >> >> +                ics->group_len[ics->num_window_groups-1] = 1;
> >>>> >> >> +            }
> >>>> >> >> +        }
> >>>> >> >> +        ics->swb_offset    =    swb_offset_128[ac->m4ac.sampling_index];
> >>>> >> >> +        ics->num_swb       =       num_swb_128[ac->m4ac.sampling_index];
> >>>> >> >
> >>>> >> >> +        if(ics->max_sfb > ics->num_swb) {
> >>>> >> >> +            av_log(ac->avccontext, AV_LOG_ERROR,
> >>>> >> >> +                "Number of scalefactor bands in group (%d) exceeds limit (%d).\n",
> >>>> >> >> +                ics->max_sfb, ics->num_swb);
> >>>> >> >> +            return -1;
> >>>> >> >> +        }
> >>>> >> >
> >>>> >> > is it safe to write invalid values in the context and then exit with an
> >>>> >> > error? are they gurranteed not to be used or that their use is harmless?
> >>>> >>
> >>>> >> If this function returns -1 it will fall through to aac_decode_frame
> >>>> >> returning -1.
> >>>> >
> >>>> > yes but i think it can end up being used in future frames without
> >>>> > the code above being reexecuted to clean it up.
> >>>> > These may be bugs elsewhere but still i dont like security related
> >>>> > code that depends on the absence of bugs in large amounts of
> >>>> > distant code.
> >>>>
> >>>> So how should this be handled? I apologise if this is a stupid
> >>>> question. Should the values be assigned to temporary local vars and
> >>>> written to the context after validation?
> >>>
> >>> thats an option, alternative is to zero them in the if(){return -1}
> >>
> >> I suppose that avoids more variables. I'll do that.
> >
> > Done.
> >
> >>>> Should this be done just for
> >>>> max_sfb and num_swb or are there others?
> >>>
> >>> does the code contain more that are security relevant?
> >>
> >> What makes a context variable security relevant? If it's used for array indices?
> >
> > I just 'audited' the code based on this premise looking for variables
> > used as limits in for() and while() loops and I've found one potential
> > access beyond the end of an array.
> >
> > In decode_tns(), tns->order[w][filt] can be either 3 or 5 bits read
> > from the bitstream. If it's 5 then it can be up to 31. Shortly after:
> >
> > for (i = 0; i < tns->order[w][filt]; i++)
> >    tns->coef[w][filt][i] = get_bits(gb, coef_len);
> >
> > ...but tns->coef is only [][][TNS_MAX_ORDER] and TNS_MAX_ORDER is 20.
> > So the value of order[][] should be checked against TNS_MAX_ORDER when
> > parsing.
> >
> > According to Table 4.137 ? Definition of TNS_MAX_ORDER depending on
> > AOT and windowing, TNS_MAX_ORDER should be 7 for short windows and for
> > long windows it should be 20 if the AOT is Main or 12 otherwise.
> >
> > I propose the attached. I don't actually think the
> > 'tns->order[w][filt] = 0;' is needed as a tns->present bit is read
> > before calling any TNS related stuff including this decoding function
> > and the bit shouldn't be set (I think) in the case scale_flag is set.
> 
> And the patch... :)

ok

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080731/31b675d1/attachment.pgp>