On Wed, Jan 10, 2018 at 05:08:09PM -0800, Jacob Trimble wrote: > On Wed, Jan 10, 2018 at 1:51 PM, Michael Niedermayer > <mich...@niedermayer.cc> wrote: > > [...] > > > > This causes a crash: > > > > ================================================================= > > ==4012==ERROR: AddressSanitizer: heap-buffer-overflow on address > > 0x60200000eb78 at pc 0x000000a944aa bp 0x7ffcd481ce70 sp 0x7ffcd481ce68 > > READ of size 8 at 0x60200000eb78 thread T0 > > #0 0xa944a9 in mov_free_encryption_index > > ffmpeg/libavformat/mov.c:6615:20 > > #1 0xa6fb2b in mov_read_close ffmpeg/libavformat/mov.c:6693:13 > > #2 0xa6d96f in mov_read_header ffmpeg/libavformat/mov.c:6867:13 > > #3 0xc4a6ed in avformat_open_input ffmpeg/libavformat/utils.c:613:20 > > #4 0x4db356 in open_input_file ffmpeg/fftools/ffmpeg_opt.c:1069:11 > > #5 0x4da0e7 in open_files ffmpeg/fftools/ffmpeg_opt.c:3202:15 > > #6 0x4d9d98 in ffmpeg_parse_options ffmpeg/fftools/ffmpeg_opt.c:3242:11 > > #7 0x50e98c in main ffmpeg/fftools/ffmpeg.c:4813:11 > > #8 0x7f9cf833cf44 in __libc_start_main > > /build/eglibc-SvCtMH/eglibc-2.19/csu/libc-start.c:287 > > #9 0x420da5 in _start (ffmpeg/ffmpeg_g+0x420da5) > > > > 0x60200000eb78 is located 4 bytes to the right of 4-byte region > > [0x60200000eb70,0x60200000eb74) > > allocated by thread T0 here: > > #0 0x4b126e in realloc (ffmpeg/ffmpeg_g+0x4b126e) > > #1 0x218bbfe in av_strdup ffmpeg/libavutil/mem.c:256:15 > > #2 0x215eec1 in av_dict_set ffmpeg/libavutil/dict.c:87:22 > > #3 0x215f6e2 in av_dict_set_int ffmpeg/libavutil/dict.c:153:12 > > #4 0xa7644c in mov_read_ftyp ffmpeg/libavformat/mov.c:1109:5 > > #5 0xa6b153 in mov_read_default ffmpeg/libavformat/mov.c:6327:23 > > #6 0xa6c543 in mov_read_header ffmpeg/libavformat/mov.c:6865:20 > > #7 0xc4a6ed in avformat_open_input ffmpeg/libavformat/utils.c:613:20 > > #8 0x4db356 in open_input_file ffmpeg/fftools/ffmpeg_opt.c:1069:11 > > #9 0x4da0e7 in open_files ffmpeg/fftools/ffmpeg_opt.c:3202:15 > > #10 0x4d9d98 in ffmpeg_parse_options ffmpeg/fftools/ffmpeg_opt.c:3242:11 > > #11 0x50e98c in main ffmpeg/fftools/ffmpeg.c:4813:11 > > #12 0x7f9cf833cf44 in __libc_start_main > > /build/eglibc-SvCtMH/eglibc-2.19/csu/libc-start.c:287 > > > > The input file should be here: > > https://bugs.chromium.org/p/chromium/issues/attachment?aid=177545 > > Fixed. > > > > > [...] > > > > -- > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > Many things microsoft did are stupid, but not doing something just because > > microsoft did it is even more stupid. If everything ms did were stupid they > > would be bankrupt already. > > > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >
> libavformat/isom.h | 20 - > libavformat/mov.c | 432 > ++++++++++++++++++++++----------- > tests/fate/mov.mak | 8 > tests/ref/fate/mov-frag-encrypted | 57 ++++ > tests/ref/fate/mov-tenc-only-encrypted | 57 ++++ > 5 files changed, 422 insertions(+), 152 deletions(-) > 8dad9875df608b84def95d81c5c641db5ff88d43 > 0001-avformat-mov-Increase-support-for-v4.patch > From 5f6411a92569d13524485627fa68e62e8fd63e50 Mon Sep 17 00:00:00 2001 > From: Jacob Trimble <modma...@google.com> > Date: Wed, 6 Dec 2017 16:17:54 -0800 > Subject: [PATCH] avformat/mov: Increase support for common encryption. > > - Parse schm atom to get different encryption schemes. > - Allow senc atom to appear in track fragments. > - Allow 16-byte IVs. > - Allow constant IVs (specified in tenc). > - Allow only tenc to specify encryption (i.e. no senc/saiz/saio). > - Use sample descriptor to detect clear fragments. > > This doesn't support: > - Different sample descriptor holding different encryption info. > - Only first sample descriptor can be encrypted. > - Encrypted sample groups (i.e. seig). > - Non-'cenc' encryption scheme when using -decryption_key. > > This removes support for saio/saiz atoms, but it was incorrect before. > A follow-up change will add correct support for those. This removal should be done by a seperate patch if it is done. diff has matched up the removed function with a added one making this hard to read as is > > Signed-off-by: Jacob Trimble <modma...@google.com> > --- > libavformat/isom.h | 20 +- > libavformat/mov.c | 432 > ++++++++++++++++++++++----------- > tests/fate/mov.mak | 8 + > tests/ref/fate/mov-frag-encrypted | 57 +++++ > tests/ref/fate/mov-tenc-only-encrypted | 57 +++++ > 5 files changed, 422 insertions(+), 152 deletions(-) > create mode 100644 tests/ref/fate/mov-frag-encrypted > create mode 100644 tests/ref/fate/mov-tenc-only-encrypted This depends on other patches you posted, this should be mentioned or all patches should be in the same patchset in order > > diff --git a/libavformat/isom.h b/libavformat/isom.h > index 65676fb0f5..3794b1f0fd 100644 > --- a/libavformat/isom.h > +++ b/libavformat/isom.h > @@ -27,6 +27,7 @@ > #include <stddef.h> > #include <stdint.h> > > +#include "libavutil/encryption_info.h" > #include "libavutil/mastering_display_metadata.h" > #include "libavutil/spherical.h" > #include "libavutil/stereo3d.h" > @@ -108,12 +109,20 @@ typedef struct MOVSbgp { > unsigned int index; > } MOVSbgp; > > +typedef struct MOVEncryptionIndex { > + // Individual encrypted samples. If there are no elements, then the > default > + // settings will be used. > + unsigned int nb_encrypted_samples; > + AVEncryptionInfo **encrypted_samples; > +} MOVEncryptionIndex; > + > typedef struct MOVFragmentStreamInfo { > int id; > int64_t sidx_pts; > int64_t first_tfra_pts; > int64_t tfdt_dts; > int index_entry; > + MOVEncryptionIndex *encryption_index; > } MOVFragmentStreamInfo; > > typedef struct MOVFragmentIndexItem { > @@ -214,15 +223,10 @@ typedef struct MOVStreamContext { > > int has_sidx; // If there is an sidx entry for this stream. > struct { > - int use_subsamples; > - uint8_t* auxiliary_info; > - uint8_t* auxiliary_info_end; > - uint8_t* auxiliary_info_pos; > - uint8_t auxiliary_info_default_size; > - uint8_t* auxiliary_info_sizes; > - size_t auxiliary_info_sizes_count; > - int64_t auxiliary_info_index; > struct AVAESCTR* aes_ctr; > + unsigned int per_sample_iv_size; // Either 0, 8, or 16. > + AVEncryptionInfo *default_encrypted_sample; > + MOVEncryptionIndex *encryption_index; > } cenc; > } MOVStreamContext; > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index 22faecfc17..37320af2f6 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -1324,6 +1324,7 @@ static int update_frag_index(MOVContext *c, int64_t > offset) > frag_stream_info[i].tfdt_dts = AV_NOPTS_VALUE; > frag_stream_info[i].first_tfra_pts = AV_NOPTS_VALUE; > frag_stream_info[i].index_entry = -1; > + frag_stream_info[i].encryption_index = NULL; > } > > if (index < c->frag_index.nb_items) > @@ -5710,117 +5711,252 @@ static int mov_read_frma(MOVContext *c, AVIOContext > *pb, MOVAtom atom) > return 0; > } > > -static int mov_read_senc(MOVContext *c, AVIOContext *pb, MOVAtom atom) > +/** > + * Gets the current encryption info and associated current stream context. > If > + * we are parsing a track fragment, this will return the specific encryption > + * info for this fragment; otherwise this will return the global encryption > + * info for the current stream. > + */ > +static int get_current_encryption_info(MOVContext *c, MOVEncryptionIndex > **encryption_index, MOVStreamContext **sc) > { > + MOVFragmentStreamInfo *frag_stream_info; > AVStream *st; > - MOVStreamContext *sc; > - size_t auxiliary_info_size; > + int i; > > - if (c->decryption_key_len == 0 || c->fc->nb_streams < 1) > - return 0; > + frag_stream_info = get_current_frag_stream_info(&c->frag_index); > + if (frag_stream_info) { > + for (i = 0; i < c->fc->nb_streams; i++) { > + if (c->fc->streams[i]->id == frag_stream_info->id) { > + st = c->fc->streams[i]; > + break; > + } > + } the indention is inconsistent here [...] > +static int mov_read_senc(MOVContext *c, AVIOContext *pb, MOVAtom atom) > +{ > + AVEncryptionInfo **encrypted_samples; > + MOVEncryptionIndex *encryption_index; > + MOVStreamContext *sc; > + int use_subsamples, ret; > + unsigned int sample_count, i, alloc_size = 0; > > - if (avio_read(pb, sc->cenc.auxiliary_info, auxiliary_info_size) != > auxiliary_info_size) { > - av_log(c->fc, AV_LOG_ERROR, "failed to read the auxiliary info"); > - return AVERROR_INVALIDDATA; > + ret = get_current_encryption_info(c, &encryption_index, &sc); > + if (ret != 1) > + return ret; > + > + if (encryption_index->nb_encrypted_samples) { > + // This can happen if we have both saio/saiz and senc atoms. > + av_log(c->fc, AV_LOG_DEBUG, "Ignoring duplicate encryption info in > senc\n"); > + return 0; > } > > - /* initialize the cipher */ > - sc->cenc.aes_ctr = av_aes_ctr_alloc(); > - if (!sc->cenc.aes_ctr) { > + avio_r8(pb); /* version */ > + use_subsamples = avio_rb24(pb) & 0x02; /* flags */ > + > + sample_count = avio_rb32(pb); > + if (sample_count >= INT_MAX / sizeof(*encrypted_samples)) > return AVERROR(ENOMEM); > + > + for (i = 0; i < sample_count && !pb->eof_reached; i++) { > + unsigned int min_samples = FFMIN(FFMAX(i, 1024 * 1024), > sample_count); > + encrypted_samples = > av_fast_realloc(encryption_index->encrypted_samples, &alloc_size, > + min_samples * > sizeof(*encrypted_samples)); > + if (!encrypted_samples) { > + ret = AVERROR(ENOMEM); > + goto end; > + } > + encryption_index->encrypted_samples = encrypted_samples; > + > + ret = mov_read_sample_encryption_info(c, pb, sc, > &encryption_index->encrypted_samples[i], use_subsamples); > + if (ret < 0) { > + goto end; > + } > + } > + > + if (pb->eof_reached) { > + av_log(c->fc, AV_LOG_ERROR, "Hit EOF while reading senc\n"); > + ret = AVERROR_INVALIDDATA; > } > > - return av_aes_ctr_init(sc->cenc.aes_ctr, c->decryption_key); > +end: > + if (ret < 0) { > + for (; i > 0; i--) > + av_encryption_info_free(encryption_index->encrypted_samples[i - > 1]); I think its a bit risky to use "i" here like this. if someone adds a goto end before i is first used this breaks if someone adds a loop after the main loop this breaks too [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Everything should be made as simple as possible, but not simpler. -- Albert Einstein
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel