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

[Robert Swain]

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.

Will valgrind spot such oversights? How does one look for security
issues? For what else should I look?

Rob