[FFmpeg-devel] [PATCH] EA ADPCM R1, R2 and R3

[pross at xvid.org]

On Tue, Oct 23, 2007 at 12:50:06AM +0200, Michael Niedermayer wrote:
> On Mon, Oct 22, 2007 at 12:05:53AM +0200, Aurelien Jacobs wrote:
> > On Sat, 20 Oct 2007 20:11:58 +0200
> > Michael Niedermayer <michaelni at gmx.at> wrote:
> > > also this is not the most readable way to check this especially if more
> > > codecs get added ...
> > 
> > I agree. After a quick look, it seems this check is here to prevent
> > overflow of the status[] context var. As this patch increase the size
> > of status[] to 6, I think it's safe to simply check for channels > 6U.
> > I hope I'm right here ?
> 
> no, quick look is not enough, you (or me) would have to check all adpcm
> codecs for channel related security issues, maybe there are none but i
> dont feel good with removing this check after a quick look
> 
> also i dont think the codecs will work with more than 2 channels after
> this check is removed ...

none of the existing adpcm decoders are >2 channel aware. michael's original comment
concerned readability, so why not just expand the if statement to a switch.
example:

//===
    switch(avctx->codec->id) {
    case CODEC_ID_ADPCM_EA_R1:
    case CODEC_ID_ADPCM_EA_R2:
    case CODEC_ID_ADPCM_EA_R3:
    case CODEC_ID_ADPCM_EA_XAS: /* WiP */
        if(avctx->channels > 6U)
            return -1;
        break;
    default:
        if(avctx->channels > 2U)
            return -1;
    }
//====

> > > completely missing checks for array bounds, this should be exploitable
> > 
> > Right. Check added at the start of this decoding code.
> 
> have you considered that the *channels in this check can overflow and thus
> the check would fail ...

that would be caught earlier by adpcm_decode_init using the "if channels>6U fail" sanity check.

cheers,
-- pete