[FFmpeg-devel] [PATCH] Fix a couple of errors with bad Vorbis headers

[Michael Niedermayer]

On Mon, Jan 10, 2011 at 06:13:21PM -0800, Frank Barchard wrote:
> This update removes a shift, and addresses Reimar's safety concern.
> On corrupt files, vorbis_decode_init now returns with -1;

>  vorbis_dec.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 4a0bdc07bb1a5259fc8b42809666f2aa21a5d9df  21_vorbis_overflow.2.patch
> diff -wurp -N orig/libavcodec/vorbis_dec.c ffmpeg-mt/libavcodec/vorbis_dec.c
> --- orig/libavcodec/vorbis_dec.c	2011-01-10 17:27:28 -0800
> +++ ffmpeg-mt/libavcodec/vorbis_dec.c	2011-01-10 17:28:18 -0800
> @@ -483,6 +483,7 @@ static int vorbis_parse_setup_hdr_floors
>          if (floor_setup->floor_type == 1) {
>              int maximum_class = -1;
>              uint_fast8_t  rangebits;
> +            uint_fast32_t rangemax;
>              uint_fast16_t floor1_values = 2;
>  
>              floor_setup->decode = vorbis_floor1_decode;
> @@ -534,8 +535,15 @@ static int vorbis_parse_setup_hdr_floors
>  
>  
>              rangebits = get_bits(gb, 4);
> +            rangemax = (1 << rangebits);
> +            if (rangemax > vc->blocksize[1] / 2) {
> +                av_log(vc->avccontext, AV_LOG_ERROR,
> +                       "Floor value is too large for blocksize: %d (%d)\n",
> +                       rangemax, vc->blocksize[1] / 2);
> +                return -1;
> +            }
>              floor_setup->data.t1.list[0].x = 0;
> -            floor_setup->data.t1.list[1].x = (1 << rangebits);
> +            floor_setup->data.t1.list[1].x = rangemax;
>  
>              for (j = 0; j < floor_setup->data.t1.partitions; ++j) {
>                  for (k = 0; k < floor_setup->data.t1.class_dimensions[floor_setup->data.t1.partition_class[j]]; ++k, ++floor1_values) {

these 2 hunks look ok to me though ive not deeply investigated.
They definitly should be applied ASAP though as this is a security fix
Further investigation can be done once this is in


    > @@ -653,7 +661,7 @@ static int vorbis_parse_setup_hdr_residu
>          res_setup->partition_size = get_bits(gb, 24) + 1;
>          /* Validations to prevent a buffer overflow later. */
>          if (res_setup->begin>res_setup->end ||
> -            res_setup->end > vc->avccontext->channels * vc->blocksize[1] / (res_setup->type == 2 ? 1 : 2) ||
> +            res_setup->end > vc->avccontext->channels * vc->blocksize[1] / 2 ||
>              (res_setup->end-res_setup->begin) / res_setup->partition_size > V_MAX_PARTITIONS) {
>              av_log(vc->avccontext, AV_LOG_ERROR, "partition out of bounds: type, begin, end, size, blocksize: %"PRIdFAST16", %"PRIdFAST32", %"PRIdFAST32", %u, %"PRIdFAST32"\n", res_setup->type, res_setup->begin, res_setup->end, res_setup->partition_size, vc->blocksize[1] / 2);
>              return -1;

this is a mystery to me
what does this fix?

What i found when looking at the code is that ptns_to_read is uint_fast16_t
but values stored in there are tested against 
#define V_MAX_PARTITIONS (1 << 20)
thats definitly not ok



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

There seems to be only one solution to NIH syndrom, ... a shooting squad
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110115/0e52ff09/attachment.pgp>