[FFmpeg-devel] [PATCH 1/3] avformat/mov: Increase support for common encryption.
Michael Niedermayer
michael at niedermayer.cc
Wed Jan 10 23:51:35 EET 2018
On Tue, Jan 09, 2018 at 10:25:12AM -0800, Jacob Trimble wrote:
> On Mon, Jan 8, 2018 at 5:19 PM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
> > 2018-01-08 23:16 GMT+01:00 Jacob Trimble <modmaker-at-google.com at ffmpeg.org>:
> >>> You can't remove API just like that without a deprecation period.
> >>> Add a new av_aes_ctr_set_full_iv() function, and either leave
> >>> av_aes_ctr_set_iv() alone or deprecate it if it effectively becomes
> >>> superfluous after adding the new function.
> >>>
> >>> Also, this patch needs to be split in three. One for the aes_crt
> >>> changes, one for the encryption_info changes, and one for mov changes,
> >>> with a minor libavutil version bump for the former two, and the
> >>> corresponding APIChanges entry.
> >>> Alternatively, you could also squash the new encryption_info API from
> >>> this patch into the one that actually introduces the entire feature.
> >>
> >> Whoops, I thought that was internal-only. Done and split into its own change.
> >>
> >> On Sat, Jan 6, 2018 at 7:30 AM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
> >>> 2018-01-05 20:49 GMT+01:00 Jacob Trimble <modmaker-at-google.com at ffmpeg.org>:
> >>>
> >>>> + if (!frag_stream_info->encryption_index) {
> >>>> + frag_stream_info->encryption_index = av_mallocz(sizeof(MOVEncryptionIndex));
> >>>
> >>> sizeof(variable), please.
> >
> > Do you disagree?
> > I have no strong opinion, but I thought it makes the code more robust...
>
> I did it most other places, but forgot it here. Done.
>
> >
> >>> [...]
> >>>
> >>>> + sample_count = avio_rb32(pb);
> >>>> +
> >>>> + encryption_index->encrypted_samples =
> >>>> av_mallocz_array(sizeof(AVEncryptionInfo*), sample_count);
> >>>
> >>> This should be avoided if possible, see below.
> >>>
> >>>> + if (!encryption_index->encrypted_samples) {
> >>>> return AVERROR(ENOMEM);
> >>>> }
> >>>> + encryption_index->nb_encrypted_samples = sample_count;
> >>>>
> >>>> - return av_aes_ctr_init(sc->cenc.aes_ctr, c->decryption_key);
> >>>> + for (i = 0; i < sample_count; i++) {
> >>>
> >>> Please check here for eof...
> >>>
> >>>> + ret = mov_read_sample_encryption_info(c, pb, sc,
> >>>> &encryption_index->encrypted_samples[i], use_subsamples);
> >>>
> >>> ... and insert a realloc here to avoid the large allocation above, see 1112ba01.
> >>
> >> Done.
> >
> >> + if (use_subsamples) {
> >> + subsample_count = avio_rb16(pb);
> >> + for (i = 0; i < subsample_count && !pb->eof_reached; i++) {
> >> + unsigned int min_subsamples = FFMIN(FFMAX(i, 1024 * 1024), subsample_count);
> >> + subsamples = av_fast_realloc((*sample)->subsamples, &alloc_size,
> >> + min_subsamples * sizeof(*subsamples));
> >
> > The reason I did not comment on this block in the last mail is that
> > iiuc, the maximum allocation here is INT16_MAX * 8 which seems
> > acceptable (and there cannot be a realloc with the chosen initial
> > limit).
>
> Ok, changed back.
>
> >
> >> + sample_count = avio_rb32(pb);
> >
> > You have to check here...
> >
> > + 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 this product could overflow for the final reallocation, see 1112ba01.
>
> Done
>
> >
> > Thank you, Carl Eugen
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at 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(-)
> 21ec6d9850961f491553278e64042530cecaacbe 0001-avformat-mov-Increase-support-for-v3.patch
> From e8261628e11347a8aa1b61de04c53cc1174cdae3 Mon Sep 17 00:00:00 2001
> From: Jacob Trimble <modmaker at google.com>
> Date: Wed, 6 Dec 2017 16:17:54 -0800
> Subject: [PATCH 1/3] avformat/mov: Increase support for common encryption.
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
[...]
--
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180110/a51c6ec8/attachment.sig>
More information about the ffmpeg-devel
mailing list