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

[Robert Swain]

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... :)

Rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20080730-2205-tns_max_order_checks.diff
Type: text/x-diff
Size: 2068 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080730/4edcde27/attachment.diff>