[FFmpeg-devel] Security issues?
Michael Niedermayer
michaelni
Wed Sep 23 14:45:32 CEST 2009
On Wed, Sep 23, 2009 at 12:31:51PM +0200, Reimar D?ffinger wrote:
> On Tue, Sep 22, 2009 at 08:09:08PM +0200, Michael Niedermayer wrote:
> > Hi
> >
> > lars has mailed me the following 2 links
> > http://www.heise.de/newsticker/Sicherheitsluecken-in-VLC-und-FFmpeg--/meldung/145655
> > http://secunia.com/advisories/36805/
>
> Ok, here is the stuff from
> http://src.chromium.org/viewvc/chrome/trunk/deps/third_party/ffmpeg/patches/to_upstream/?pathrev=26428
> Could someone find a contact from chromium and forward this to them?
> Because I already commented on some of these things, and I don't want to
> end up reviewing everything n times.
>
> 09_mov_stsz_int_oflow.patch:
[...]
> Fixed IMO in a better way in r19793
>
>
> =======================================================
>
> 10_vorbis_submap_indexes.patch:
> --- libavcodec/vorbis_dec.c.orig 2009-08-20 16:47:16.000000000 -0700
> +++ libavcodec/vorbis_dec.c 2009-08-20 19:33:13.000000000 -0700
> @@ -723,9 +723,20 @@
> }
>
> for(j=0;j<mapping_setup->submaps;++j) {
> + int bits;
> skip_bits(gb, 8); // FIXME check?
> - mapping_setup->submap_floor[j]=get_bits(gb, 8);
> - mapping_setup->submap_residue[j]=get_bits(gb, 8);
> + bits=get_bits(gb, 8);
> + if (bits>=vc->floor_count) {
> + av_log(vc->avccontext, AV_LOG_ERROR, "submap floor value %d out of range. \n", bits);
> + return 1;
> + }
> + mapping_setup->submap_floor[j]=bits;
> + bits=get_bits(gb, 8);
> + if (bits>=vc->residue_count) {
> + av_log(vc->avccontext, AV_LOG_ERROR, "submap residue value %d out of range. \n", bits);
> + return 1;
> + }
> + mapping_setup->submap_residue[j]=bits;
>
> AV_DEBUG(" %d mapping %d submap : floor %d, residue %d \n", i, j, mapping_setup->submap_floor[j], mapping_setup->submap_residue[j]);
> }
>
> Should either just use FFMIN and print no message at all,
i really think this is a bad idea, we have enough issues that need debuging
no need to get more because we dont print the error condition ...
> or check both
> after reading with a single if to keep code bloat low.
> FFMIN can not fix the floor_count == 0 or residue_count == 0 case,
> though if they are not valid that should be caught when reading them.
> I think each of these patches should add a comment in the struct
> declarations that specifies the valid values for each field.
i agree it should be added but i dont think this has to be in this
patch, that said, i think this patch here can be applied unless
someone will actually write a better within 24h.
>
> ==========================================
>
> 11_vorbis_residue_book_index:
[...]
> Ok IMO
agree, applied
>, though FFMIN instead of return 1 might be better, except it
> would not work for codebook_count == 0. Someone who
> knows the code should decide if the output would be useful for
> debugging, otherwise just using FFMIN without any av_log would be
> better.
>
> ====================================================
>
> 12_vorbis_mode_indexes:
[...]
> Same comment,
applied
> except that it needs careful checking that the return
> 1/return -1 really and completely aborts decoding.
the first seems to lead to a error return from
vorbis_parse_setup_hdr -> vorbis_decode_init
the second returns before the invalid value is stored in a struct
> Or just use FFMIN in
> addition to make sure the value stored in the context is valid.
> Note that wherever the fuzzed sample file can be played when just using
> FFMIN instead of return ..., that would be preferable - does not work
> for mapping_count == 0 or mode_count == 0 though.
> Keeping the error messages (though they should be warning instead of
> error then) is good except when it bloats the code too much.
>
> Note that it might be a good idea to just verify all values in one place
> after all parsing, something like vorbis_check_context after
> vorbis_parse_setup_hdr (as far as that can fix the issues, maybe some of
> those read values are already used within vorbis_parse_setup_hdr).
>
>
> ================================================
>
> 13_ogg_comment_parsing
> --- libavformat/oggparsevorbis.c.orig 2009-08-20 20:23:55.000000000 -0700
> +++ libavformat/oggparsevorbis.c 2009-08-24 21:56:22.000000000 -0700
> @@ -47,9 +47,12 @@
>
> p += s;
>
> + if (end - p < 4)
> + return -1;
> +
> n = bytestream_get_le32(&p);
>
> - while (p < end && n > 0) {
> + while ((end - p) >= 4 && n > 0) {
> const char *t, *v;
> int tl, vl;
>
> @@ -97,7 +100,7 @@
> }
>
> if (p != end)
> - av_log(as, AV_LOG_INFO, "%ti bytes of comment header remain\n", p-end);
> + av_log(as, AV_LOG_INFO, "%ti bytes of comment header remain\n", end-p);
> if (n > 0)
> av_log(as, AV_LOG_INFO,
> "truncated comment header, %i comments not found\n", n);
>
> The second hunk is unrelated and has been in my local tree for ages, so
> I just applied it.
> Part of what caused the issue here is also that s is unsigned.
> So I'd propose this instead:
> Index: libavformat/oggparsevorbis.c
> ===================================================================
> --- libavformat/oggparsevorbis.c (revision 19978)
> +++ libavformat/oggparsevorbis.c (working copy)
> @@ -50,27 +50,28 @@
> {
> const uint8_t *p = buf;
> const uint8_t *end = buf + size;
> - unsigned s, n, j;
> + unsigned n, j;
> + int s;
>
> if (size < 8) /* must have vendor_length and user_comment_list_length */
> return -1;
>
> s = bytestream_get_le32(&p);
>
> - if (end - p < s)
> + if (end - p - 4 < s || s < 0)
> return -1;
>
> p += s;
>
> n = bytestream_get_le32(&p);
>
> - while (p < end && n > 0) {
> + while (end - p >= 4 && n > 0) {
> const char *t, *v;
> int tl, vl;
>
> s = bytestream_get_le32(&p);
>
> - if (end - p < s)
> + if (end - p < s || s < 0)
> break;
>
> t = p;
>
> Since p is padded, making s signed and checking for s < 0 would be
> enough.
ok
>
> =========================================
>
> 14_floor_masterbook_index:
[...]
> Same comments as with all other vorbis_dec changes,
applied
> though I also notice
> that there might be a bit of an issue if codebook_count is 0, in which
> case my suggestion of just using FFMIN might not work...
> Maybe one more argument for validating everything as much as possible in
> one place, that might make it possible to fix things up properly instead
> of vorbis files becoming unplayable just because of a single bit error
> anywhere in the header.
its very unlikey that just a single bit has been flipped, but lets assume it
did, very likely it wont be detected when that bit is parsed but rather much
later when parsing became totally desynced and little but noise is left.
Droping such frames if they can be detected is likely a good idea.
but if your FFMIN idea leads to better subjective quality on randomly
damaged files then i surely dont mind at all but this is a seperate issue
we first should fix the security issues and crashes IMHO ...
>
> ==============================================
>
> 15_more_residue_book_indexes:
[...]
> Well, more of the same...
so applied
>
> ==========================================
>
> 18_fix_theora_header_bit_len:
[...]
> Seems obvious, IMO apply,
done
> though << 3 instead of *8 might be more
> consistent (not actually sure about it).
bikeshed ...
>
>
> ========================================
>
> 22_fix_theora_frag_fencepost:
> --- libavcodec/vp3.c.orig4 2009-08-27 21:00:20.000000000 -0700
> +++ libavcodec/vp3.c 2009-08-27 21:08:41.000000000 -0700
> @@ -993,7 +993,7 @@
> num_blocks_at_qpi += run_length;
>
> for (j = 0; j < run_length; i++) {
> - if (i > s->coded_fragment_list_index)
> + if (i >= s->coded_fragment_list_index)
> return -1;
>
> if (s->all_fragments[s->coded_fragment_list[i]].qpi == qpi) {
>
> Seems right,
applied
> though I have a suspicion that unpack_superblocks also
> lacks a check for coded_fragment_list_index < fragment_count is missing,
then please fix it
> also the code for the MODE_INTER_NO_MV case is duplicated in that
> function...
>
>
> ===================================
>
> 23_vorbis_sane_partition:
> --- libavcodec/vorbis_dec.c.orig 2009-09-03 20:36:13 -0700
> +++ libavcodec/vorbis_dec.c 2009-09-03 20:37:04 -0700
> @@ -37,6 +37,7 @@
> #define V_NB_BITS 8
> #define V_NB_BITS2 11
> #define V_MAX_VLCS (1<<16)
> +#define V_MAX_PARTITIONS (1<<20)
>
> #ifndef V_DEBUG
> #define AV_DEBUG(...)
> @@ -644,6 +645,14 @@
> res_setup->begin=get_bits(gb, 24);
> res_setup->end=get_bits(gb, 24);
> 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->blocksize[1]/(res_setup->type==2?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: %d, %d, %d, %d, %d\n", res_setup->type, res_setup->begin, res_setup->end, res_setup->partition_size, vc->blocksize[1]/2);
> + return 1;
> + }
> +
> res_setup->classifications=get_bits(gb, 6)+1;
> res_setup->classbook=get_bits(gb, 8);
> if (res_setup->classbook>=vc->codebook_count) {
>
> Oh god, more of the same vorbis stuff, just more ugly. This one
> definitely needs to be documented much better, e.g. mentioning
> the places where it would crash otherwise and also documenting the
> assumptions on begin and end where they are declared.
>
> ======================================================
>
> 24_vorbis_zero_channels:
[...]
> Applied in r19975, except using -1 as return value which is a bit
> inconsistent with the remaining code.
oops but IMHO they all should be -1
>
> =====================================================
>
> 25_vorbis_floor0_index:
[...]
> Wow, double stupid, validating data after using it and doing the
> validation off by one. Obviously ok IMO.
has been applied
>
>
> ================================================
>
> 26_vorbis_mag_angle_index:
[...]
> IMO only use one if/message for both.
has been applied
>
> ======================================================
>
> 27_vorbis_residue_loop_error:
[...]
> Was discussed elsewhere. _Seems_ obviously right, but one of the
> comments was if the mapping->submaps==1 check is really supposed to be
> there.
has been applied
>
> ==================================================
>
> 28_theora_malloc_checks:
> --- libavcodec/vp3.c.orig5 2009-08-31 10:45:06.000000000 -0700
> +++ libavcodec/vp3.c 2009-08-31 12:09:55.000000000 -0700
> @@ -43,6 +43,8 @@
>
> #define FRAGMENT_PIXELS 8
>
> +static av_cold int vp3_decode_end(AVCodecContext *avctx);
> +
> typedef struct Coeff {
> struct Coeff *next;
> DCTELEM coeff;
> @@ -1740,6 +1742,11 @@
> s->coeffs = av_malloc(s->fragment_count * sizeof(Coeff) * 65);
> s->coded_fragment_list = av_malloc(s->fragment_count * sizeof(int));
> s->pixel_addresses_initialized = 0;
> + if (!s->all_fragments || !s->coeff_counts || !s->coeffs ||
> + !s->coded_fragment_list) {
> + vp3_decode_end(avctx);
> + return -1;
> + }
>
> if (!s->theora_tables)
> {
> @@ -1845,6 +1852,11 @@
> s->superblock_macroblocks = av_malloc(s->superblock_count * 4 * sizeof(int));
> s->macroblock_fragments = av_malloc(s->macroblock_count * 6 * sizeof(int));
> s->macroblock_coding = av_malloc(s->macroblock_count + 1);
> + if (!s->superblock_fragments || !s->superblock_macroblocks ||
> + !s->macroblock_fragments || !s->macroblock_coding) {
> + vp3_decode_end(avctx);
> + return -1;
> + }
> init_block_mapping(s);
>
> for (i = 0; i < 3; i++) {
>
> Seems ok, but the vlc_fail path should probably call vp3_decode_end,
> too. And it should be AVERROR(ENOMEM), not -1.
iam thinking that
if(vp3_decode_init < 0)
vp3_decode_end()
would be a better idea
>
> ===================================================
>
> 29_mov_dref_looping:
> --- libavformat/mov.c.orig2 2009-08-31 15:41:36.000000000 -0700
> +++ libavformat/mov.c 2009-08-31 15:55:36.000000000 -0700
> @@ -261,6 +261,8 @@
> MOVDref *dref = &sc->drefs[i];
> uint32_t size = get_be32(pb);
> int64_t next = url_ftell(pb) + size - 4;
> + if (size < 8)
> + return -1;
>
> dref->type = get_le32(pb);
> get_be32(pb); // version + flags
>
> I do not like failing hard, using FFMAX to make sure size is at least
> e.g. 4 would be better IMO (no idea what the minimum size actually is).
left to baptiste
>
> =======================================================
>
> 31_mp3_outlen:
> --- libavcodec/mpegaudiodec.c.orig 2009-08-31 23:19:15.000000000 -0700
> +++ libavcodec/mpegaudiodec.c 2009-08-31 23:20:05.000000000 -0700
> @@ -2294,8 +2294,10 @@
> *data_size = out_size;
> avctx->sample_rate = s->sample_rate;
> //FIXME maybe move the other codec info stuff from above here too
> - }else
> + }else{
> av_log(avctx, AV_LOG_DEBUG, "Error while decoding MPEG audio frame.\n"); //FIXME return -1 / but also return the number of bytes consumed
> + *data_size = 0;
> + }
> s->frame_size = 0;
> return buf_size + skipped;
> }
>
> *data_size = 0 IMO should be done right at the top of the decode_frame
> function - assuming the case decode failed but not returning -1 is
> indeed as intended.
i think i fixed that one
>
> ========================================================
>
> 32_mov_stream_index:
[...]
> Seems not acceptable to me. I fear that the mov demuxer in quite a few
> places assumes a specific ordering of the atoms without having a good
> reason a leading to catastrophic failure, but this is just too much
> code.
> If there is no better idea (e.g. extending the atom parsing table to put
> constraints on the atom order) I'd propose to have the mov demuxer just
> create one dummy stream at the very beginning that will either be
> replaced by the first real stream or removed at the end of parsing if no
> stream was found.
> I admit I can't promise this will get less ugly, but I suspect so.
all mov patches left to baptiste ...
>
> ==================================================
>
> 34_h264_bad_timings:
[...]
> I guess it's the best that can be done, except that the fallback
> time_scale should be something sensible like e.g. 25, not 1.
should have been fixed
>
>
> ====================================================
>
> 35_mov_bad_timings:
[...]
> Looks ok to me.
>
>
> =============================================
>
> 39_vorbis_zero_dims:
> --- libavcodec/vorbis_dec.c.orig16 2009-09-01 22:31:46.000000000 -0700
> +++ libavcodec/vorbis_dec.c 2009-09-01 22:34:19.000000000 -0700
> @@ -250,8 +250,8 @@
> }
>
> codebook_setup->dimensions=get_bits(gb, 16);
> - if (codebook_setup->dimensions>16) {
> - av_log(vc->avccontext, AV_LOG_ERROR, " %"PRIdFAST16". Codebook's dimension is too large (%d). \n", cb, codebook_setup->dimensions);
> + if (codebook_setup->dimensions>16||codebook_setup->dimensions==0) {
> + av_log(vc->avccontext, AV_LOG_ERROR, " %"PRIdFAST16". Codebook's dimension is invalid (%d). \n", cb, codebook_setup->dimensions);
> goto error;
> }
> entries=get_bits(gb, 24);
>
> Cosmetically nicer:
> > if (!codebook_setup->dimensions || codebook_setup->dimensions>16) {
> Otherwise probably ok except that as with all these patches it really
> needs to be documented properly (i.e. in the declaration).
certainly, feel free to improve it ...
>
> ===========================================
>
> 40_ogg_missing_header:
> --- libavformat/oggdec.c.orig 2009-09-01 22:06:21.000000000 -0700
> +++ libavformat/oggdec.c 2009-09-01 22:21:42.000000000 -0700
> @@ -478,11 +478,16 @@
> {
> struct ogg *ogg = s->priv_data;
> ogg->curidx = -1;
> + int i;
> //linear headers seek from start
> if (ogg_get_headers (s) < 0){
> return -1;
> }
>
> + for (i = 0; i < ogg->nstreams; ++i)
> + if (ogg->streams[i].codec && !ogg->streams[i].private)
> + return -1;
> +
> //linear granulepos seek from end
> ogg_get_length (s);
>
>
> Completely failing just because one stream is broken is not acceptable,
agree
> also the parsers do not necessarily have to have a private field.
> There are several possible solutions, something like this (untested)
> might work for example:
> Index: oggdec.c
> ===================================================================
> --- oggdec.c (revision 19976)
> +++ oggdec.c (working copy)
> @@ -478,11 +478,16 @@
> {
> struct ogg *ogg = s->priv_data;
> ogg->curidx = -1;
> + int i;
> //linear headers seek from start
> if (ogg_get_headers (s) < 0){
> return -1;
> }
>
> + for (i = 0; i < ogg->nstreams; ++i)
> + if (ogg->streams[i].header < 0)
> + ogg->streams[i].codec = NULL;
> +
> //linear granulepos seek from end
> ogg_get_length (s);
>
>
> =========================================================
>
> 41_vorbis_zero_samplerate:
>
> Applied slightly differently in r19975
>
> =================================================
>
> 42_aac_zero_bands:
> --- libavcodec/aac.c.orig2 2009-09-02 13:27:42.000000000 -0700
> +++ libavcodec/aac.c 2009-09-02 14:46:13.000000000 -0700
> @@ -641,9 +641,9 @@
> while ((sect_len_incr = get_bits(gb, bits)) == (1 << bits)-1)
> sect_len += sect_len_incr;
> sect_len += sect_len_incr;
> - if (sect_len > ics->max_sfb) {
> + if (sect_len > ics->max_sfb || sect_len == 0) {
> av_log(ac->avccontext, AV_LOG_ERROR,
> - "Number of bands (%d) exceeds limit (%d).\n",
> + "Number of bands (%d) is invalid, limit (%d).\n",
> sect_len, ics->max_sfb);
> return -1;
> }
>
> Going by the explanation I think this is maybe the wrong approach.
> As I understand it, the issue is that an all-0 band_type_run_end will
> lead to an endless loop in decode_scalefactors.
> I think it would be more reliable to avoid this in decode_scalefactors.
>
>
> ===========================================
>
> 43_codec_type_mismatch:
[...]
> SVN errors out in this case now, while that is a good idea, this really
> needs to be fixed in the mov (and wherever else this can happen)
> demuxers.
>
> ========================================
>
> 44_respect_flac_configure:
[...]
> This is not ok, disabling the FLAC decode may not disable FLAC-in-Ogg
> demuxing. This is one of the unfortunate cases where libavformat is tied
> more closely to libavcodec than it should be, but I consider it the
> fault of Oggs extreme case of misdesign.
> It would be nice if the ogg demuxer only enabled compiling in of
> ff_flac_parse_streaminfo instead of the whole FLAC decoder though!
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- 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/20090923/2e29758d/attachment.pgp>
More information about the ffmpeg-devel
mailing list