[FFmpeg-devel] [PATCH 1/3] avformat/mov: Increase support for common encryption.
Michael Niedermayer
michael at niedermayer.cc
Tue Jan 23 05:38:15 EET 2018
On Wed, Jan 10, 2018 at 05:08:09PM -0800, Jacob Trimble wrote:
> On Wed, Jan 10, 2018 at 1:51 PM, Michael Niedermayer
> <michael at 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 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(-)
> 8dad9875df608b84def95d81c5c641db5ff88d43 0001-avformat-mov-Increase-support-for-v4.patch
> From 5f6411a92569d13524485627fa68e62e8fd63e50 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] 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 <modmaker at 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
-------------- 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/20180123/147d5fb7/attachment.sig>
More information about the ffmpeg-devel
mailing list