Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Michael Niedermayer]

On Wed, Dec 30, 2015 at 09:53:35PM +, Eran Kornblau wrote:
> > > Please let me know if you think that is ok, and I will resubmit the patch 
> > > with all fixes.
> > 
> > should be ok
> > 
> Updated patch attached, diff from previous patch is:
> 
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4026,6 +4026,14 @@ static int mov_read_frma(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  case MKTAG('e','n','c','v'):// encrypted video
>  case MKTAG('e','n','c','a'):// encrypted audio
>  id = mov_codec_id(st, format);
> +if (st->codec->codec_id != AV_CODEC_ID_NONE &&
> +st->codec->codec_id != id) {
> +av_log(c->fc, AV_LOG_WARNING,
> +   "ignoring 'frma' atom of '%.4s', stream has codec id 
> %d\n",
> +   (char*), st->codec->codec_id);
> +break;
> +}
> +
>  st->codec->codec_id = id;
>  sc->format = format;
>  break;
> @@ -4045,7 +4053,6 @@ static int mov_read_senc(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  AVStream *st;
>  MOVStreamContext *sc;
>  size_t auxiliary_info_size;
> -int ret;
>  
>  if (c->decryption_key_len == 0 || c->fc->nb_streams < 1)
>  return 0;
> @@ -4091,12 +4098,7 @@ static int mov_read_senc(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  return AVERROR(ENOMEM);
>  }
>  
> -ret = av_aes_ctr_init(sc->cenc.aes_ctr, c->decryption_key);
> -if (ret) {
> -return ret;
> -}
> -
> -return 0;
> +return av_aes_ctr_init(sc->cenc.aes_ctr, c->decryption_key);
>  }
>  
>  static int cenc_filter(MOVContext *c, MOVStreamContext *sc, uint8_t *input, 
> int size)
> @@ -4107,7 +4109,7 @@ static int cenc_filter(MOVContext *c, MOVStreamContext 
> *sc, uint8_t *input, int
>  uint8_t* input_end = input + size;
>  
>  /* read the iv */
> -if (sc->cenc.auxiliary_info_pos + AES_CTR_IV_SIZE > 
> sc->cenc.auxiliary_info_end) {
> +if (AES_CTR_IV_SIZE > sc->cenc.auxiliary_info_end - 
> sc->cenc.auxiliary_info_pos) {
>  av_log(c->fc, AV_LOG_ERROR, "failed to read iv from the auxiliary 
> info\n");
>  return AVERROR_INVALIDDATA;
>  }
> @@ -4123,7 +4125,7 @@ static int cenc_filter(MOVContext *c, MOVStreamContext 
> *sc, uint8_t *input, int
>  }
>  
>  /* read the subsample count */
> -if (sc->cenc.auxiliary_info_pos + sizeof(uint16_t) > 
> sc->cenc.auxiliary_info_end) {
> +if (sizeof(uint16_t) > sc->cenc.auxiliary_info_end - 
> sc->cenc.auxiliary_info_pos) {
>  av_log(c->fc, AV_LOG_ERROR, "failed to read subsample count from the 
> auxiliary info\n");
>  return AVERROR_INVALIDDATA;
>  }
> @@ -4133,7 +4135,7 @@ static int cenc_filter(MOVContext *c, MOVStreamContext 
> *sc, uint8_t *input, int
>  
>  for (; subsample_count > 0; subsample_count--)
>  {
> -if (sc->cenc.auxiliary_info_pos + 6 > sc->cenc.auxiliary_info_end) {
> +if (6 > sc->cenc.auxiliary_info_end - sc->cenc.auxiliary_info_pos) {
>  av_log(c->fc, AV_LOG_ERROR, "failed to read subsample from the 
> auxiliary info\n");
>  return AVERROR_INVALIDDATA;
>  }
> @@ -4144,7 +4146,7 @@ static int cenc_filter(MOVContext *c, MOVStreamContext 
> *sc, uint8_t *input, int
>  encrypted_bytes = AV_RB32(sc->cenc.auxiliary_info_pos);
>  sc->cenc.auxiliary_info_pos += sizeof(uint32_t);
>  
> -if (input + clear_bytes + encrypted_bytes > input_end) {
> +if ((uint64_t)clear_bytes + encrypted_bytes > input_end - input) {
>  av_log(c->fc, AV_LOG_ERROR, "subsample size exceeds the packet 
> size left\n");
>  return AVERROR_INVALIDDATA;
>  }
> 
> 
> > [...]
> > 
> > -- 
> > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> > 
> 
> Thanks, Michael !
> 
> Eran

>  Changelog  |1 
>  libavformat/isom.h |   13 +++
>  libavformat/mov.c  |  181 
> +
>  3 files changed, 195 insertions(+)
> 5974fab38debc4fae0595bcdfec63d500932495a  
> 0001-mov-support-cenc-common-encryption.patch
> From 2021b91bd195a20ae346b877810661dddfa73144 Mon Sep 17 00:00:00 2001
> From: erankor 
> Date: Mon, 7 Dec 2015 12:30:50 +0200
> Subject: [PATCH 1/2] mov: support cenc (common encryption)
> 
> support reading encrypted mp4 using aes-ctr, conforming to ISO/IEC
> 23001-7.
> 
> a new parameter was added:
> - decryption_key - 128 bit decryption key (hex)
> ---
>  Changelog  |   1 +
>  libavformat/isom.h |  13 
>  libavformat/mov.c  | 181 
> +
>  3 files changed, 195 insertions(+)

patch applied

thanks

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Michael Niedermayer]

On Tue, Dec 29, 2015 at 08:05:28PM +, Eran Kornblau wrote:
> > > > > +case MKTAG('e','n','c','v'):// encrypted video
> > > > > +case MKTAG('e','n','c','a'):// encrypted audio
> > > > > +id = mov_codec_id(st, format);
> > > > > +st->codec->codec_id = id;
> > > > 
> > > > this seems missing a check for st->codec->codec_id being "unset"
> > > > before setting it
> > > > 
> > > Will add it
> > > 
> While testing this change, I found that for audio, at this point codec_id is 
> actually already initialized - 
> it gets initialized when ff_mov_read_esds calls ff_mp4_read_dec_config_descr, 
> in this line:
> st->codec->codec_id= ff_codec_get_id(ff_mp4_obj_type, object_type_id);
> 
> What happens with my current code is that this outputs a warning that frma is 
> ignored because codec_id
> is not none. I think I will just skip the warning when st->codec->codec_id 
> "happens to be" the same as 
> the codec that was inferred by the frma atom.
>  
> This appears to work well:
> case MKTAG('e','n','c','v'):// encrypted video
> case MKTAG('e','n','c','a'):// encrypted audio
> id = mov_codec_id(st, format);
> if (st->codec->codec_id != AV_CODEC_ID_NONE &&
> st->codec->codec_id != id) {
> av_log(c->fc, AV_LOG_WARNING,
>"ignoring 'frma' atom of '%.4s', stream has codec id %d\n",
>(char*), st->codec->codec_id);
> break;
> }
> 
> st->codec->codec_id = id;
> sc->format = format;
> break;
> 
> Please let me know if you think that is ok, and I will resubmit the patch 
> with all fixes.

should be ok

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

> > Please let me know if you think that is ok, and I will resubmit the patch 
> > with all fixes.
> 
> should be ok
> 
Updated patch attached, diff from previous patch is:

--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4026,6 +4026,14 @@ static int mov_read_frma(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 case MKTAG('e','n','c','v'):// encrypted video
 case MKTAG('e','n','c','a'):// encrypted audio
 id = mov_codec_id(st, format);
+if (st->codec->codec_id != AV_CODEC_ID_NONE &&
+st->codec->codec_id != id) {
+av_log(c->fc, AV_LOG_WARNING,
+   "ignoring 'frma' atom of '%.4s', stream has codec id %d\n",
+   (char*), st->codec->codec_id);
+break;
+}
+
 st->codec->codec_id = id;
 sc->format = format;
 break;
@@ -4045,7 +4053,6 @@ static int mov_read_senc(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 AVStream *st;
 MOVStreamContext *sc;
 size_t auxiliary_info_size;
-int ret;
 
 if (c->decryption_key_len == 0 || c->fc->nb_streams < 1)
 return 0;
@@ -4091,12 +4098,7 @@ static int mov_read_senc(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 return AVERROR(ENOMEM);
 }
 
-ret = av_aes_ctr_init(sc->cenc.aes_ctr, c->decryption_key);
-if (ret) {
-return ret;
-}
-
-return 0;
+return av_aes_ctr_init(sc->cenc.aes_ctr, c->decryption_key);
 }
 
 static int cenc_filter(MOVContext *c, MOVStreamContext *sc, uint8_t *input, 
int size)
@@ -4107,7 +4109,7 @@ static int cenc_filter(MOVContext *c, MOVStreamContext 
*sc, uint8_t *input, int
 uint8_t* input_end = input + size;
 
 /* read the iv */
-if (sc->cenc.auxiliary_info_pos + AES_CTR_IV_SIZE > 
sc->cenc.auxiliary_info_end) {
+if (AES_CTR_IV_SIZE > sc->cenc.auxiliary_info_end - 
sc->cenc.auxiliary_info_pos) {
 av_log(c->fc, AV_LOG_ERROR, "failed to read iv from the auxiliary 
info\n");
 return AVERROR_INVALIDDATA;
 }
@@ -4123,7 +4125,7 @@ static int cenc_filter(MOVContext *c, MOVStreamContext 
*sc, uint8_t *input, int
 }
 
 /* read the subsample count */
-if (sc->cenc.auxiliary_info_pos + sizeof(uint16_t) > 
sc->cenc.auxiliary_info_end) {
+if (sizeof(uint16_t) > sc->cenc.auxiliary_info_end - 
sc->cenc.auxiliary_info_pos) {
 av_log(c->fc, AV_LOG_ERROR, "failed to read subsample count from the 
auxiliary info\n");
 return AVERROR_INVALIDDATA;
 }
@@ -4133,7 +4135,7 @@ static int cenc_filter(MOVContext *c, MOVStreamContext 
*sc, uint8_t *input, int
 
 for (; subsample_count > 0; subsample_count--)
 {
-if (sc->cenc.auxiliary_info_pos + 6 > sc->cenc.auxiliary_info_end) {
+if (6 > sc->cenc.auxiliary_info_end - sc->cenc.auxiliary_info_pos) {
 av_log(c->fc, AV_LOG_ERROR, "failed to read subsample from the 
auxiliary info\n");
 return AVERROR_INVALIDDATA;
 }
@@ -4144,7 +4146,7 @@ static int cenc_filter(MOVContext *c, MOVStreamContext 
*sc, uint8_t *input, int
 encrypted_bytes = AV_RB32(sc->cenc.auxiliary_info_pos);
 sc->cenc.auxiliary_info_pos += sizeof(uint32_t);
 
-if (input + clear_bytes + encrypted_bytes > input_end) {
+if ((uint64_t)clear_bytes + encrypted_bytes > input_end - input) {
 av_log(c->fc, AV_LOG_ERROR, "subsample size exceeds the packet 
size left\n");
 return AVERROR_INVALIDDATA;
 }


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

Thanks, Michael !

Eran


0001-mov-support-cenc-common-encryption.patch
Description: 0001-mov-support-cenc-common-encryption.patch
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

> > > > +case MKTAG('e','n','c','v'):// encrypted video
> > > > +case MKTAG('e','n','c','a'):// encrypted audio
> > > > +id = mov_codec_id(st, format);
> > > > +st->codec->codec_id = id;
> > > 
> > > this seems missing a check for st->codec->codec_id being "unset"
> > > before setting it
> > > 
> > Will add it
> > 
While testing this change, I found that for audio, at this point codec_id is 
actually already initialized - 
it gets initialized when ff_mov_read_esds calls ff_mp4_read_dec_config_descr, 
in this line:
st->codec->codec_id= ff_codec_get_id(ff_mp4_obj_type, object_type_id);

What happens with my current code is that this outputs a warning that frma is 
ignored because codec_id
is not none. I think I will just skip the warning when st->codec->codec_id 
"happens to be" the same as 
the codec that was inferred by the frma atom.
 
This appears to work well:
case MKTAG('e','n','c','v'):// encrypted video
case MKTAG('e','n','c','a'):// encrypted audio
id = mov_codec_id(st, format);
if (st->codec->codec_id != AV_CODEC_ID_NONE &&
st->codec->codec_id != id) {
av_log(c->fc, AV_LOG_WARNING,
   "ignoring 'frma' atom of '%.4s', stream has codec id %d\n",
   (char*), st->codec->codec_id);
break;
}

st->codec->codec_id = id;
sc->format = format;
break;

Please let me know if you think that is ok, and I will resubmit the patch with 
all fixes.

> > > 
> > > the way these are printed would lead to different results on big and
> > > little endian
> > > 
> > Correct, however it's already done this way in a few other places in this 
> > file (search for %.4s)
> > Do you prefer that I will print it with %c%c%c%c as in 
> > ff_mov_read_stsd_entries ?
> 
> whatever way you prefer
> 
Ok, so I think I'll just leave it as is - big endian environments will get the 
atom name in 
reverse order in the log message (I assume 99% of the people use ffmpeg on 
little endian anyway... :))

Thanks

Eran
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Michael Niedermayer]

On Mon, Dec 28, 2015 at 09:27:41PM +, Eran Kornblau wrote:
> > > +case MKTAG('e','n','c','v'):// encrypted video
> > > +case MKTAG('e','n','c','a'):// encrypted audio
> > > +id = mov_codec_id(st, format);
> > > +st->codec->codec_id = id;
> > 
> > this seems missing a check for st->codec->codec_id being "unset"
> > before setting it
> > 
> Will add it
> 

> > 
> > > +sc->format = format;
> > > +break;
> > > +
> > > +default:
> > > +av_log(c->fc, AV_LOG_WARNING,
> > > +   "ignoring 'frma' atom of '%.4s', stream format is 
> > > '%.4s'\n",
> > > +   (char*), (char*)>format);
> > 
> > the way these are printed would lead to different results on big and
> > little endian
> > 
> Correct, however it's already done this way in a few other places in this 
> file (search for %.4s)
> Do you prefer that I will print it with %c%c%c%c as in 
> ff_mov_read_stsd_entries ?

whatever way you prefer

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

> > +case MKTAG('e','n','c','v'):// encrypted video
> > +case MKTAG('e','n','c','a'):// encrypted audio
> > +id = mov_codec_id(st, format);
> > +st->codec->codec_id = id;
> 
> this seems missing a check for st->codec->codec_id being "unset"
> before setting it
> 
Will add it

> 
> > +sc->format = format;
> > +break;
> > +
> > +default:
> > +av_log(c->fc, AV_LOG_WARNING,
> > +   "ignoring 'frma' atom of '%.4s', stream format is '%.4s'\n",
> > +   (char*), (char*)>format);
> 
> the way these are printed would lead to different results on big and
> little endian
> 
Correct, however it's already done this way in a few other places in this file 
(search for %.4s)
Do you prefer that I will print it with %c%c%c%c as in ff_mov_read_stsd_entries 
?

> > +
> > +ret = av_aes_ctr_init(sc->cenc.aes_ctr, c->decryption_key);
> > +if (ret) {
> > +return ret;
> > +}
> > +
> > +return 0;
> 
> this could be simplfied
> also errors in ffmpeg are generally negative so
> if (ret < 0) seems more correct if a seperate check is left
> 
Ok, will change to 'return av_aes_ctr_init(...)'

> 
> > +/* read the subsample count */
> > +if (sc->cenc.auxiliary_info_pos + sizeof(uint16_t) > 
> > sc->cenc.auxiliary_info_end) {
> 
> the addition could overflow, which would be undefined behavior
> something like:
> sizeof(uint16_t) > sc->cenc.auxiliary_info_end - sc->cenc.auxiliary_info_pos
> is correcter
> 
Will change to what you suggested

> > +if (input + clear_bytes + encrypted_bytes > input_end) {
> 
> the additions can overflow
> 
Will change to: 
if ((uint64_t)clear_bytes + encrypted_bytes > input_end - input) {


> [...]
> 
> -- 
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
Thanks !

Eran
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Michael Niedermayer]

On Mon, Dec 28, 2015 at 05:29:35AM +, Eran Kornblau wrote:
> Bumping up this thread (latest patch attached)
> 
> Happy holidays !

happy holidays as well!

> 
> Eran
> 
[...]
> @@ -4007,6 +4009,162 @@ static int mov_read_free(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  return 0;
>  }
>  
> +static int mov_read_frma(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> +{
> +uint32_t format = avio_rl32(pb);
> +MOVStreamContext *sc;
> +enum AVCodecID id;
> +AVStream *st;
> +
> +if (c->fc->nb_streams < 1)
> +return 0;
> +st = c->fc->streams[c->fc->nb_streams - 1];
> +sc = st->priv_data;
> +
> +switch (sc->format)
> +{
> +case MKTAG('e','n','c','v'):// encrypted video
> +case MKTAG('e','n','c','a'):// encrypted audio
> +id = mov_codec_id(st, format);
> +st->codec->codec_id = id;

this seems missing a check for st->codec->codec_id being "unset"
before setting it


> +sc->format = format;
> +break;
> +
> +default:
> +av_log(c->fc, AV_LOG_WARNING,
> +   "ignoring 'frma' atom of '%.4s', stream format is '%.4s'\n",
> +   (char*), (char*)>format);

the way these are printed would lead to different results on big and
little endian


> +break;
> +}
> +
> +return 0;
> +}
> +
> +static int mov_read_senc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> +{
> +AVStream *st;
> +MOVStreamContext *sc;
> +size_t auxiliary_info_size;
> +int ret;
> +
> +if (c->decryption_key_len == 0 || c->fc->nb_streams < 1)
> +return 0;
> +
> +st = c->fc->streams[c->fc->nb_streams - 1];
> +sc = st->priv_data;
> +
> +if (sc->cenc.aes_ctr) {
> +av_log(c->fc, AV_LOG_ERROR, "duplicate senc atom\n");
> +return AVERROR_INVALIDDATA;
> +}
> +
> +avio_r8(pb); /* version */
> +sc->cenc.use_subsamples = avio_rb24(pb) & 0x02; /* flags */
> +
> +avio_rb32(pb);/* entries */
> +
> +if (atom.size < 8) {
> +av_log(c->fc, AV_LOG_ERROR, "senc atom size %"PRId64" too small\n", 
> atom.size);
> +return AVERROR_INVALIDDATA;
> +}
> +
> +/* save the auxiliary info as is */
> +auxiliary_info_size = atom.size - 8;
> +
> +sc->cenc.auxiliary_info = av_malloc(auxiliary_info_size);
> +if (!sc->cenc.auxiliary_info) {
> +return AVERROR(ENOMEM);
> +}
> +
> +sc->cenc.auxiliary_info_end = sc->cenc.auxiliary_info + 
> auxiliary_info_size;
> +
> +sc->cenc.auxiliary_info_pos = sc->cenc.auxiliary_info;
> +
> +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;
> +}
> +
> +/* initialize the cipher */
> +sc->cenc.aes_ctr = av_aes_ctr_alloc();
> +if (!sc->cenc.aes_ctr) {
> +return AVERROR(ENOMEM);
> +}
> +
> +ret = av_aes_ctr_init(sc->cenc.aes_ctr, c->decryption_key);


> +if (ret) {
> +return ret;
> +}
> +
> +return 0;

this could be simplfied
also errors in ffmpeg are generally negative so
if (ret < 0) seems more correct if a seperate check is left



> +}
> +
> +static int cenc_filter(MOVContext *c, MOVStreamContext *sc, uint8_t *input, 
> int size)
> +{
> +uint32_t encrypted_bytes;
> +uint16_t subsample_count;
> +uint16_t clear_bytes;
> +uint8_t* input_end = input + size;
> +
> +/* read the iv */
> +if (sc->cenc.auxiliary_info_pos + AES_CTR_IV_SIZE > 
> sc->cenc.auxiliary_info_end) {
> +av_log(c->fc, AV_LOG_ERROR, "failed to read iv from the auxiliary 
> info\n");
> +return AVERROR_INVALIDDATA;
> +}
> +
> +av_aes_ctr_set_iv(sc->cenc.aes_ctr, sc->cenc.auxiliary_info_pos);
> +sc->cenc.auxiliary_info_pos += AES_CTR_IV_SIZE;
> +
> +if (!sc->cenc.use_subsamples)
> +{
> +/* decrypt the whole packet */
> +av_aes_ctr_crypt(sc->cenc.aes_ctr, input, input, size);
> +return 0;
> +}
> +

> +/* read the subsample count */
> +if (sc->cenc.auxiliary_info_pos + sizeof(uint16_t) > 
> sc->cenc.auxiliary_info_end) {

the addition could overflow, which would be undefined behavior
something like:
sizeof(uint16_t) > sc->cenc.auxiliary_info_end - sc->cenc.auxiliary_info_pos
is correcter


> +av_log(c->fc, AV_LOG_ERROR, "failed to read subsample count from the 
> auxiliary info\n");
> +return AVERROR_INVALIDDATA;
> +}
> +
> +subsample_count = AV_RB16(sc->cenc.auxiliary_info_pos);
> +sc->cenc.auxiliary_info_pos += sizeof(uint16_t);
> +
> +for (; subsample_count > 0; subsample_count--)
> +{
> +if (sc->cenc.auxiliary_info_pos + 6 > sc->cenc.auxiliary_info_end) {
> +av_log(c->fc, AV_LOG_ERROR, "failed to read subsample from the 
> auxiliary info\n");
> +return AVERROR_INVALIDDATA;
> +}
> +
> +/* read the number of clear / 

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

Bumping up this thread (latest patch attached)

Happy holidays !

Eran



0001-mov-support-cenc-common-encryption.patch
Description: 0001-mov-support-cenc-common-encryption.patch
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

> you are not subscribed (with this email address you used here at least
> it seems)
>
Right, apparently the subscription confirmation message went to the junk 
folder...
Anyway, glad this one is now resolved :-) and sorry for the trouble.

> > > 
> > > > +id = mov_codec_id(st, format);
> > > > +st->codec->codec_id = id;
> > > 
> > > doesnt this allow changing the codec id to anything from anything ?
> > > this seems potentially risky
> > > 
> > I see options to address this:
> > 1. overwrite the codec_id only when it's AV_CODEC_ID_NONE
> > 2. (stricter) save the format 4cc (that is loaded in 
> > ff_mov_read_stsd_entries) to some member of 
> > MOVStreamContext, and overwrite the codec id only when the format is 'encv' 
> > or 'enca'
> > 
> > please let me know which one you prefer, and I will update the patch.
> 
> i think the strcter variant sounds more robust
>

Fixed, updated patch attached.
Also added a warning in case the frma atom is ignored because of this.

Pasting here the diff between this new patch and the previous one (if it helps):

diff --git a/libavformat/isom.h b/libavformat/isom.h
index 3b0230a..a5ca21b 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -170,6 +170,7 @@ typedef struct MOVStreamContext {
 int64_t duration_for_fps;

 int32_t *display_matrix;
+uint32_t format;

 struct {
 int use_subsamples;
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 728d8b0..94005cb 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2227,6 +2227,7 @@ int ff_mov_read_stsd_entries(MOVContext *c, AVIOContext 
*pb, int entries)

 sc->pseudo_stream_id = st->codec->codec_tag ? -1 : pseudo_stream_id;
 sc->dref_id= dref_id;
+sc->format = format;

 id = mov_codec_id(st, format);

@@ -4011,15 +4012,30 @@ static int mov_read_free(MOVContext *c, AVIOContext 
*pb, MOVAtom atom)
 static int mov_read_frma(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
 uint32_t format = avio_rl32(pb);
+MOVStreamContext *sc;
 enum AVCodecID id;
 AVStream *st;

 if (c->fc->nb_streams < 1)
 return 0;
 st = c->fc->streams[c->fc->nb_streams - 1];
+sc = st->priv_data;
+
+switch (sc->format)
+{
+case MKTAG('e','n','c','v'):// encrypted video
+case MKTAG('e','n','c','a'):// encrypted audio
+id = mov_codec_id(st, format);
+st->codec->codec_id = id;
+sc->format = format;
+break;

-id = mov_codec_id(st, format);
-st->codec->codec_id = id;
+default:
+av_log(c->fc, AV_LOG_WARNING,
+   "ignoring 'frma' atom of '%.4s', stream format is '%.4s'\n",
+   (char*), (char*)>format);
+break;

 return 0;
 }

Thanks

Eran


0001-mov-support-cenc-common-encryption.patch
Description: 0001-mov-support-cenc-common-encryption.patch
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Michael Niedermayer]

On Tue, Dec 22, 2015 at 07:36:41AM +, Eran Kornblau wrote:
> > > You're right, didn't know that.
> > > Please let me know if there are any changes you want me to apply in order 
> > > to push the third one (decryption support).
> > 
> > sounds like you missed:
> > http://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/185235.html
> > http://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/185238.html
> > 
> Correct, I missed it. It's just that I'm not getting any emails from this 
> mailing list 
> (verified they are not getting to the junk folder...). 

you are not subscribed (with this email address you used here at least
it seems)


> So I've been polling the page of the last message of this thread in 
> ffmpeg.org,
> and guess these posts were added to a different leaf of the message tree...
> Would really appreciate it if this could be resolved, either add my work 
> email -
> eran dot kornblau at kaltura dot com or my personal email - 
> erankor at gmail dot com.
> 
> > if you want to maintain the code in the future then please send a
> > patch that adds you to the maintainers file
> > 
> Attached

applied


> > 
> > > +id = mov_codec_id(st, format);
> > > +st->codec->codec_id = id;
> > 
> > doesnt this allow changing the codec id to anything from anything ?
> > this seems potentially risky
> > 
> I see options to address this:
> 1. overwrite the codec_id only when it's AV_CODEC_ID_NONE
> 2. (stricter) save the format 4cc (that is loaded in 
> ff_mov_read_stsd_entries) to some member of 
> MOVStreamContext, and overwrite the codec id only when the format is 'encv' 
> or 'enca'
> 
> please let me know which one you prefer, and I will update the patch.

i think the strcter variant sounds more robust

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

DNS cache poisoning attacks, popular search engine, Google internet authority
dont be evil, please


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Carl Eugen Hoyos]

Eran Kornblau  kaltura.com> writes:

> Bumping up this thread, would love to see this getting merged
> (attaching the same patch files)

Weren't patches 1&2 already applied?
http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=23ac99dc
http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=4469e8eb

Carl Eugen

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

> 
> Weren't patches 1&2 already applied?
> http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=23ac99dc
> http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=4469e8eb
> 
You're right, didn't know that.
Please let me know if there are any changes you want me to apply in order to 
push the third one (decryption support).

> Carl Eugen

Thanks Carl !

Eran 
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Michael Niedermayer]

On Mon, Dec 21, 2015 at 07:19:45PM +, Eran Kornblau wrote:
> > 
> > Weren't patches 1&2 already applied?
> > http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=23ac99dc
> > http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=4469e8eb
> > 
> You're right, didn't know that.
> Please let me know if there are any changes you want me to apply in order to 
> push the third one (decryption support).

sounds like you missed:
http://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/185235.html
http://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/185238.html


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

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

> > You're right, didn't know that.
> > Please let me know if there are any changes you want me to apply in order 
> > to push the third one (decryption support).
> 
> sounds like you missed:
> http://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/185235.html
> http://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/185238.html
> 
Correct, I missed it. It's just that I'm not getting any emails from this 
mailing list 
(verified they are not getting to the junk folder...). 
So I've been polling the page of the last message of this thread in ffmpeg.org,
and guess these posts were added to a different leaf of the message tree...
Would really appreciate it if this could be resolved, either add my work email -
eran dot kornblau at kaltura dot com or my personal email - 
erankor at gmail dot com.

> if you want to maintain the code in the future then please send a
> patch that adds you to the maintainers file
> 
Attached
> 
> > +id = mov_codec_id(st, format);
> > +st->codec->codec_id = id;
> 
> doesnt this allow changing the codec id to anything from anything ?
> this seems potentially risky
> 
I see options to address this:
1. overwrite the codec_id only when it's AV_CODEC_ID_NONE
2. (stricter) save the format 4cc (that is loaded in ff_mov_read_stsd_entries) 
to some member of 
MOVStreamContext, and overwrite the codec id only when the format is 'encv' or 
'enca'

please let me know which one you prefer, and I will update the patch.

> [...]
> -- 
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
Thanks a lot, Michael !

Eran


0001-add-Eran-Kornblau-to-maintainers.patch
Description: 0001-add-Eran-Kornblau-to-maintainers.patch
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

Bumping up this thread, would love to see this getting merged
(attaching the same patch files)

Thank you

Eran



0001-libavutil-add-aes-ctr-support.patch
Description: 0001-libavutil-add-aes-ctr-support.patch


0002-movenc-support-cenc-common-encryption.patch
Description: 0002-movenc-support-cenc-common-encryption.patch


0003-mov-support-cenc-common-encryption.patch
Description: 0003-mov-support-cenc-common-encryption.patch
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

> 
> using a random IV value would break any regression tests
> see AVFMT_FLAG_BITEXACT
> 
Fixed, only generating a random IV when bitexact is not enabled.
Updated patch files attached.

> 
> is this filling in a random IV that later is overridden ?
> random_seed() can be slow so it would be better not to call it if
> its value isnt used
> 
I didn't think it's too significant since it happens only once per decrypted 
track, but anyway, fixed it as well.
I changed av_aes_ctr_init so that it will initialize an empty IV, and exposed 
av_aes_ctr_set_random_iv.
This later function is called only by the encoder (and only when bitexact is 
disabled).

> -- 
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Thanks !

Eran


0001-libavutil-add-aes-ctr-support.patch
Description: 0001-libavutil-add-aes-ctr-support.patch


0002-movenc-support-cenc-common-encryption.patch
Description: 0002-movenc-support-cenc-common-encryption.patch


0003-mov-support-cenc-common-encryption.patch
Description: 0003-mov-support-cenc-common-encryption.patch
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

> are these encrypted mp4 files some kind of standard encryption?
> 
Yes, these files conform to the common encryption specification ISO/IEC 23001-7

> rephrased... will encrypted files created by ffmpeg be able to be
> decrypted/decoded and played by quicktime? or any other player?
> (assuming other player has the keys of course). 
> 
These files can be encrypted/decrypted by MP4Box:
https://gpac.wp.mines-telecom.fr/mp4box/encryption/common-encryption/

I'm not familiar with any player that can play it as is, but it can be played 
once repackaged to MPEG-DASH.
In this form, it can be played by Chrome with Widevine / Edge with PlayReady 
etc.
The nginx module we maintain (https://github.com/kaltura/nginx-vod-module/) can 
perform this repackaging
on the fly. If the file is encrypted on disk with the same key that should be 
used for playback, it will skip the
encryption / decryption and serve the data as is (if the keys are different, it 
has to decrypt + re-encrypt)

> thanks!
> 
> -compn

Thanks !

Eran
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

> >
> > +struct AVAESCTR;
> 
> Is this needed?
>
Indeed compiles for me without it, I matched the existing code in aes.h, and in 
general I prefer to define the structs in advance. 
If you prefer that I will remove it, please let me know
 
> > +subsample_count = AV_RB16(sc->cenc.auxiliary_info_pos);
> > +sc->cenc.auxiliary_info_pos += sizeof(uint16_t);
> 
> I find the following significantly easier to read and understand:
> 
> subsample_count = AV_RB16(sc->cenc.auxiliary_info_pos);
> sc->cenc.auxiliary_info_pos += 2;
> 
> But this may just be me, so feel free to ignore.
> 
When it's a simple type my preference is to go with the sizeof, when it becomes 
slightly more than that, e.g.:
+
+ctx->auxiliary_info_size += 6;
+ctx->subsample_count++;
+
I use a number (not sizeof(uint16_t) + sizeof(uint32_t)). But again it's all a 
matter of personal preference, 
if you want me to change it, let me know.

> I suspect a fate test will be needed but this may be an 
> independent patch.
> 
Ok, I will write one after this one gets approved :-)

> Thank you, Carl Eugen
>

Thanks !

Eran
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Michael Niedermayer]

On Tue, Dec 15, 2015 at 09:16:11AM +, Eran Kornblau wrote:
[...]

> @@ -4007,6 +4008,147 @@ static int mov_read_free(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  return 0;
>  }
>  
> +static int mov_read_frma(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> +{
> +uint32_t format = avio_rl32(pb);
> +enum AVCodecID id;
> +AVStream *st;
> +
> +if (c->fc->nb_streams < 1)
> +return 0;
> +st = c->fc->streams[c->fc->nb_streams - 1];
> +
> +id = mov_codec_id(st, format);
> +st->codec->codec_id = id;

doesnt this allow changing the codec id to anything from anything ?
this seems potentially risky

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 1
"Used only once"- "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Michael Niedermayer]

On Tue, Dec 15, 2015 at 09:16:11AM +, Eran Kornblau wrote:
> > 
> > using a random IV value would break any regression tests
> > see AVFMT_FLAG_BITEXACT
> > 
> Fixed, only generating a random IV when bitexact is not enabled.
> Updated patch files attached.
> 
> > 
> > is this filling in a random IV that later is overridden ?
> > random_seed() can be slow so it would be better not to call it if
> > its value isnt used
> > 
> I didn't think it's too significant since it happens only once per decrypted 
> track, but anyway, fixed it as well.
> I changed av_aes_ctr_init so that it will initialize an empty IV, and exposed 
> av_aes_ctr_set_random_iv.
> This later function is called only by the encoder (and only when bitexact is 
> disabled).
> 
> > -- 
> > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Thanks !
> 
> Eran

>  Makefile  |2 
>  aes_ctr.c |  129 
> ++
>  aes_ctr.h |   83 +++
>  version.h |2 
>  4 files changed, 215 insertions(+), 1 deletion(-)
> 1704fc6a8cfb2922b3806a1d1e122b668f743408  
> 0001-libavutil-add-aes-ctr-support.patch
> From 8e6bc361924b6942b879b833c419bb7ea03b516f Mon Sep 17 00:00:00 2001
> From: erankor 
> Date: Mon, 7 Dec 2015 11:58:41 +0200
> Subject: [PATCH 1/3] libavutil: add aes-ctr support

patch 1 and 2 applied

if you want to maintain the code in the future then please send a
patch that adds you to the maintainers file

thanks

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

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Michael Niedermayer]

On Mon, Dec 14, 2015 at 08:25:01AM +, Eran Kornblau wrote:
> Hi,
> 
> Bumping up this thread... please let me if you want me to make any additional 
> changes or this can be merged.
> Attaching the patch files again (same ones I sent in my last post)
[...]

> +int ff_mov_cenc_init(MOVMuxCencContext* ctx, uint8_t* encryption_key, int 
> use_subsamples)
> +{
> +int ret;
> +
> +ctx->aes_ctr = av_aes_ctr_alloc();
> +if (!ctx->aes_ctr) {
> +return AVERROR(ENOMEM);
> +}
> +
> +ret = av_aes_ctr_init(ctx->aes_ctr, encryption_key, NULL);

using a random IV value would break any regression tests
see AVFMT_FLAG_BITEXACT


> +if (ret != 0) {
> +return ret;
> +}
> +
> +ctx->use_subsamples = use_subsamples;
> +
> +return 0;
> +}
> +
> +void ff_mov_cenc_free(MOVMuxCencContext* ctx)
> +{
> +av_aes_ctr_free(ctx->aes_ctr);
> +}
[]
> +
> +static int mov_read_senc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> +{
> +AVStream *st;
> +MOVStreamContext *sc;
> +size_t auxiliary_info_size;
> +int ret;
> +
> +if (c->decryption_key_len == 0 || c->fc->nb_streams < 1)
> +return 0;
> +
> +st = c->fc->streams[c->fc->nb_streams - 1];
> +sc = st->priv_data;
> +
> +if (sc->cenc.aes_ctr) {
> +av_log(c->fc, AV_LOG_ERROR, "duplicate senc atom\n");
> +return AVERROR_INVALIDDATA;
> +}
> +
> +avio_r8(pb); /* version */
> +sc->cenc.use_subsamples = avio_rb24(pb) & 0x02; /* flags */
> +
> +avio_rb32(pb);/* entries */
> +
> +if (atom.size < 8) {
> +av_log(c->fc, AV_LOG_ERROR, "senc atom size %"PRId64" too small\n", 
> atom.size);
> +return AVERROR_INVALIDDATA;
> +}
> +
> +/* save the auxiliary info as is */
> +auxiliary_info_size = atom.size - 8;
> +
> +sc->cenc.auxiliary_info = av_malloc(auxiliary_info_size);
> +if (!sc->cenc.auxiliary_info) {
> +return AVERROR(ENOMEM);
> +}
> +
> +sc->cenc.auxiliary_info_end = sc->cenc.auxiliary_info + 
> auxiliary_info_size;
> +
> +sc->cenc.auxiliary_info_pos = sc->cenc.auxiliary_info;
> +
> +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;
> +}
> +
> +/* initialize the cipher */
> +sc->cenc.aes_ctr = av_aes_ctr_alloc();
> +if (!sc->cenc.aes_ctr) {
> +return AVERROR(ENOMEM);
> +}
> +
> +ret = av_aes_ctr_init(sc->cenc.aes_ctr, c->decryption_key, NULL);

is this filling in a random IV that later is overridden ?
random_seed() can be slow so it would be better not to call it if
its value isnt used

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

I have often repented speaking, but never of holding my tongue.
-- Xenocrates


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

Hi,

Bumping up this thread... please let me if you want me to make any additional 
changes or this can be merged.
Attaching the patch files again (same ones I sent in my last post)

Thank you

Eran


0001-libavutil-add-aes-ctr-support.patch
Description: 0001-libavutil-add-aes-ctr-support.patch


0002-movenc-support-cenc-common-encryption.patch
Description: 0002-movenc-support-cenc-common-encryption.patch


0003-mov-support-cenc-common-encryption.patch
Description: 0003-mov-support-cenc-common-encryption.patch
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Carl Eugen Hoyos]

Hi!

On Monday 14 December 2015 09:25:01 am Eran Kornblau wrote:

> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index a082e40..3b0230a 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -37,6 +37,8 @@ extern const AVCodecTag ff_codec_movsubtitle_tags[];
>  int ff_mov_iso639_to_lang(const char lang[4], int mp4);
>  int ff_mov_lang_to_iso639(unsigned code, char to[4]);
>
> +struct AVAESCTR;

Is this needed?

> +subsample_count = AV_RB16(sc->cenc.auxiliary_info_pos);
> +sc->cenc.auxiliary_info_pos += sizeof(uint16_t);

I find the following significantly easier to read and understand:

subsample_count = AV_RB16(sc->cenc.auxiliary_info_pos);
sc->cenc.auxiliary_info_pos += 2;

But this may just be me, so feel free to ignore.

I suspect a fate test will be needed but this may be an 
independent patch.

Thank you, Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[compn]

On Mon, 14 Dec 2015 08:25:01 +
Eran Kornblau  wrote:

> Hi,
> 
> Bumping up this thread... please let me if you want me to make any
> additional changes or this can be merged. Attaching the patch files
> again (same ones I sent in my last post)

dumb question 

are these encrypted mp4 files some kind of standard encryption?

rephrased... will encrypted files created by ffmpeg be able to be
decrypted/decoded and played by quicktime? or any other player?
(assuming other player has the keys of course). 

thanks!

-compn
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Jan Ekstrom]

On Mon, Dec 14, 2015 at 10:48 PM, compn  wrote:
> dumb question
>
> are these encrypted mp4 files some kind of standard encryption?
>
> rephrased... will encrypted files created by ffmpeg be able to be
> decrypted/decoded and played by quicktime? or any other player?
> (assuming other player has the keys of course).

If the patches follow the specification noted @
https://w3c.github.io/encrypted-media/cenc-format.html (standardized @
ISO as 23001-7 |
http://www.iso.org/iso/home/store/catalogue_ics/catalogue_detail_ics.htm?csnumber=65271
as far as I can tell), then yes - the encryption itself is standard.
Usually this sort of encryption is coupled with MPEG-DASH where you
stick the related information on how to get the keys into
ContentProtection blocks in the manifests.

Jan
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Ganesh Ajjanagadde]

On Mon, Dec 7, 2015 at 7:01 AM, Eran Kornblau  wrote:
>>
>> changes to libavutil and libavformat should likely be in seperate
>> patches/commits, more generally any independant changes should be
>> in seperate patches (i dont know if there are any other changes that
>> should be split off)
>>
> Done, new patch files attached, I also included the patch for decryption.
> The libavutil patch has to be applied before libavformat patches.
>
>>
>> fails to build
>>
>> ffmpeg/libavformat/movenccenc.c: In function ‘auxiliary_info_add_subsample’:
>> ffmpeg/libavformat/movenccenc.c:61:5: error: unknown type name ‘u_char’
>> ffmpeg/libavformat/movenccenc.c:73:7: warning: assignment from incompatible 
>> pointer type [enabled by default]
>>
> Replaced the u_char with uint8_t, don't know why I didn't get this error

u_char is a BSD thing, not POSIX. On GNU/Linux, _BSD_SOURCE must be
defined, requiring relevant include/#define/compiler flag. I ran into
a similar thing very recently:
http://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/184722.html.

>
>>
>> also a Changelog entry should probably be added
>
> Added, one for encoding and one for decoding (since they are in separate 
> patches)
>
>> and if libavformat uses a new feature from libavutil than libavutils
>> minor version needs to be bumped when that feature is added
>>
> Incremented LIBAVUTIL_VERSION_MINOR, please let me know if anything else is 
> needed
>
>>
>> [...]
>> --
>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>
> Thanks Michael !
>
> Eran
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

> 
> changes to libavutil and libavformat should likely be in seperate
> patches/commits, more generally any independant changes should be
> in seperate patches (i dont know if there are any other changes that
> should be split off)
> 
Done, new patch files attached, I also included the patch for decryption.
The libavutil patch has to be applied before libavformat patches.

> 
> fails to build
> 
> ffmpeg/libavformat/movenccenc.c: In function ‘auxiliary_info_add_subsample’:
> ffmpeg/libavformat/movenccenc.c:61:5: error: unknown type name ‘u_char’
> ffmpeg/libavformat/movenccenc.c:73:7: warning: assignment from incompatible 
> pointer type [enabled by default]
> 
Replaced the u_char with uint8_t, don't know why I didn't get this error

> 
> also a Changelog entry should probably be added

Added, one for encoding and one for decoding (since they are in separate 
patches)

> and if libavformat uses a new feature from libavutil than libavutils
> minor version needs to be bumped when that feature is added
> 
Incremented LIBAVUTIL_VERSION_MINOR, please let me know if anything else is 
needed

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

Thanks Michael !

Eran


0001-libavutil-add-aes-ctr-support.patch
Description: 0001-libavutil-add-aes-ctr-support.patch


0002-movenc-support-cenc-common-encryption.patch
Description: 0002-movenc-support-cenc-common-encryption.patch


0003-mov-support-cenc-common-encryption.patch
Description: 0003-mov-support-cenc-common-encryption.patch
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

Hi

> One more question: Is FFmpeg able to decrypt the files (if the keys are 
> available)? If not, it would be nice if you could also add the decryption 
> function...
> (I only realize now that the subject promises both.)
> In any case, a fate test will be needed (but is not necessarily part of 
> your patch).
> 
Yes, already wrote the code for that as well, I will send the patch once this 
one gets approved
(both share the new aes_ctr module that I added, so I can't send them in 
parallel)

> Carl Eugen

Thanks

Eran
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

Hi,

Sorry for spamming, ran some more tests and found a bug in my patch, updated 
patch file attached.
The bug was that in case subsample encryption was enabled (the default for AVC) 
the subsample size
reported in the 'saiz' atom was wrong - it did not include the size of the IV.
I originally tested my change by decrypting the file using MP4Box, and playing 
it, I guess MP4Box 
doesn't care about this discrepancy...

In case anyone started reviewing the patch, the fix is in mov_cenc_end_packet, 
the lines:
ctx->auxiliary_info_sizes[ctx->auxiliary_info_entries] =
ctx->auxiliary_info_size - ctx->auxiliary_info_subsample_start;

were changed to:
ctx->auxiliary_info_sizes[ctx->auxiliary_info_entries] =
AES_CTR_IV_SIZE + ctx->auxiliary_info_size - 
ctx->auxiliary_info_subsample_start;

Thanks

Eran


0001-movenc-support-cenc-common-encryption.patch
Description: 0001-movenc-support-cenc-common-encryption.patch
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Carl Eugen Hoyos]

Hi!

On Saturday 05 December 2015 09:16:22 pm Eran Kornblau wrote:
> Fixed the else convention and squashed all commits into one, the updated
> patch is attached.

Thank you!

One more question: Is FFmpeg able to decrypt the files (if the keys are 
available)? If not, it would be nice if you could also add the decryption 
function...
(I only realize now that the subject promises both.)
In any case, a fate test will be needed (but is not necessarily part of 
your patch).

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Michael Niedermayer]

On Sun, Dec 06, 2015 at 03:42:34PM +, Eran Kornblau wrote:
> Hi,
> 
> Sorry for spamming, ran some more tests and found a bug in my patch, updated 
> patch file attached.
> The bug was that in case subsample encryption was enabled (the default for 
> AVC) the subsample size
> reported in the 'saiz' atom was wrong - it did not include the size of the IV.
> I originally tested my change by decrypting the file using MP4Box, and 
> playing it, I guess MP4Box 
> doesn't care about this discrepancy...
> 
> In case anyone started reviewing the patch, the fix is in 
> mov_cenc_end_packet, the lines:
> ctx->auxiliary_info_sizes[ctx->auxiliary_info_entries] =
> ctx->auxiliary_info_size - ctx->auxiliary_info_subsample_start;
> 
> were changed to:
> ctx->auxiliary_info_sizes[ctx->auxiliary_info_entries] =
> AES_CTR_IV_SIZE + ctx->auxiliary_info_size - 
> ctx->auxiliary_info_subsample_start;
> 
> Thanks
> 
> Eran

>  libavformat/Makefile |2 
>  libavformat/movenc.c |   92 +-
>  libavformat/movenc.h |   16 +
>  libavformat/movenccenc.c |  410 
> +++
>  libavformat/movenccenc.h |   86 +

>  libavutil/Makefile   |2 
>  libavutil/aes_ctr.c  |  132 +++
>  libavutil/aes_ctr.h  |   79 +

changes to libavutil and libavformat should likely be in seperate
patches/commits, more generally any independant changes should be
in seperate patches (i dont know if there are any other changes that
should be split off)


>  8 files changed, 811 insertions(+), 8 deletions(-)
> 4d4e64b516df4d1b2cb518600550f836c94849bb  
> 0001-movenc-support-cenc-common-encryption.patch
> From 7ccfd1f4fcce2b3c301e14bd3173244241b822bd Mon Sep 17 00:00:00 2001
> From: erankor 
> Date: Thu, 3 Dec 2015 11:42:40 +0200
> Subject: [PATCH] movenc: support cenc (common encryption)

fails to build

ffmpeg/libavformat/movenccenc.c: In function ‘auxiliary_info_add_subsample’:
ffmpeg/libavformat/movenccenc.c:61:5: error: unknown type name ‘u_char’
ffmpeg/libavformat/movenccenc.c:73:7: warning: assignment from incompatible 
pointer type [enabled by default]


also a Changelog entry should probably be added
and if libavformat uses a new feature from libavutil than libavutils
minor version needs to be bumped when that feature is added


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

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Carl Eugen Hoyos]

Hi!

On Saturday 05 December 2015 12:17:56 am Eran Kornblau wrote:
> > Please send the patches to the mailing list. See
> > https://github.com/FFmpeg/FFmpeg/pull/153
>
> Attached, note there are two files, the second commit is just a tiny 
> fix to a bug in the first one, hope that's ok.

No, please merge them (without adding trailing whitespace, it cannot 
be committed to our repository).
While at it, please fix this nit (several times):
> +}
> +else {

should be on one line, there is tools/patcheck that should have told 
you (and also tells you about trailing whitespace).

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

Hi

> No, please merge them (without adding trailing whitespace, it cannot 
> be committed to our repository).
> While at it, please fix this nit (several times):
> > +}
> > +else {
> 
> should be on one line, there is tools/patcheck that should have told 
> you (and also tells you about trailing whitespace).
> 
Fixed the else convention and squashed all commits into one, the updated patch 
is attached.

Other than that, would it be possible to add me to the mailing list ? I 
submitted my request
to join a couple of days ago, and since was not accepted yet, I'm not getting 
back your emails.

> Carl Eugen

Thanks,

Eran



0001-movenc-support-cenc-common-encryption.patch
Description: 0001-movenc-support-cenc-common-encryption.patch
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

> Hi,
>
> Please do not top post.
>
Sorry, I hope this one comes out fine, didn't find a reasonable way to do it 
with Outlook ;-(

>
> Please send the patches to the mailing list. See
> https://github.com/FFmpeg/FFmpeg/pull/153
>
Attached, note there are two files, the second commit is just a tiny fix to a 
bug in the first one, hope that's ok.

Btw, in the contributing section here 
https://ffmpeg.org/developer.html#Contributing option 3 says:
"Committing changes to a git clone, for example on github.com or gitorious.org. 
And asking us to merge these changes."
May be worth dropping / updating this line if that is no longer a viable option.

>
> Our AES-NI implementation is a lot faster than OpenSSL's ;)
> https://gist.github.com/rcombs/2a91ab246c3a99c15b3a
>
Interesting, completely missed that one

> Timothy

Thanks

Eran


0001-movenc-support-cenc-common-encryption.patch
Description: 0001-movenc-support-cenc-common-encryption.patch


0002-bug-fix-invalid-encryption-key-kid-crashes.patch
Description: 0002-bug-fix-invalid-encryption-key-kid-crashes.patch
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Timothy Gu]

Hi,

Please do not top post.

On Fri, Dec 4, 2015 at 10:57 AM Eran Kornblau 
wrote:

> Thank you !
>
> Opened a pull request here: https://github.com/FFmpeg/FFmpeg/pull/162
>

Please send the patches to the mailing list. See
https://github.com/FFmpeg/FFmpeg/pull/153


> I used libavutil/aes as you suggested, however, as a side note, I think it
> makes sense to move ffmpeg to use
> openssl's implementation. The reason is that openssl's EVP API makes use
> of AES-NI (https://en.wikipedia.org/wiki/AES_instruction_set)
> and is therefore expected to perform significantly better on supported
> CPUs.
>

Our AES-NI implementation is a lot faster than OpenSSL's ;)
https://gist.github.com/rcombs/2a91ab246c3a99c15b3a

Timothy
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

Thank you !

Opened a pull request here: https://github.com/FFmpeg/FFmpeg/pull/162
Currently containing only the encoding part, I will start working on the 
decoding part and submit another
pull request once it's ready and this one gets merged (it will reuse code from 
this pull).
I tried to match the existing code standards as much as possible, hope I didn't 
miss anything :)

I used libavutil/aes as you suggested, however, as a side note, I think it 
makes sense to move ffmpeg to use 
openssl's implementation. The reason is that openssl's EVP API makes use of 
AES-NI (https://en.wikipedia.org/wiki/AES_instruction_set)
and is therefore expected to perform significantly better on supported CPUs.

Please let me know if anything else is needed to get this merged

Thanks again

Eran

-Original Message-
From: compn [mailto:te...@mi.rr.com] 
Sent: Wednesday, November 25, 2015 1:56 AM
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Cc: Eran Kornblau <eran.kornb...@kaltura.com>; Eran Etam <eran.e...@kaltura.com>
Subject: Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

On Tue, 24 Nov 2015 12:10:43 +
Eran Kornblau <eran.kornb...@kaltura.com> wrote:
> Before we start working on this feature, since we really prefer not
> to manage our own fork of the code, we wanted to check with you
> whether you will be willing to merge such a feature ? (assuming it
> conforms to the coding standards, naturally)

yes, we have support for encrypted wmv so there should be no problem.

try to reuse the crypt code from ffmpeg/libavutil/* where possible. or
consider adding whatever en/decrypt code you need, as we do not like to
use too many external libs.

the more communication you have with ffmpeg developers or the mailing
list, the less problems you will have when merging time happens.

you may also want to post any work in progress patches or do request
for comments on any ideas, so developers do not object about strange
api additions to the code.

feel free to ask any questions.

-compn
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[compn]

On Tue, 24 Nov 2015 12:10:43 +
Eran Kornblau  wrote:
> Before we start working on this feature, since we really prefer not
> to manage our own fork of the code, we wanted to check with you
> whether you will be willing to merge such a feature ? (assuming it
> conforms to the coding standards, naturally)

yes, we have support for encrypted wmv so there should be no problem.

try to reuse the crypt code from ffmpeg/libavutil/* where possible. or
consider adding whatever en/decrypt code you need, as we do not like to
use too many external libs.

the more communication you have with ffmpeg developers or the mailing
list, the less problems you will have when merging time happens.

you may also want to post any work in progress patches or do request
for comments on any ideas, so developers do not object about strange
api additions to the code.

feel free to ask any questions.

-compn
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] support for reading / writing encrypted MP4 files

[Paul B Mahol]

On 11/24/15, Eran Kornblau  wrote:
> Hi all,
>
> We're thinking about adding support for MP4 files encrypted using the CENC
> scheme to ffmpeg (such as the ones supported by GPAC -
> https://gpac.wp.mines-telecom.fr/mp4box/encryption/common-encryption/)
> The motivation is to be able to transcode files without having the media
> ever resident in the clear on disk (some of our customers have
> security regulations that forbid that).
>
> We'll probably add to movenc.c a parameter of encryption-scheme, supporting
> only the value cenc for now, and another parameter
> that will contain the encryption key.
> Similarly, mov.c will be added a parameter of encryption-key to enable
> reading encrypted MP4s.
> At first phase, only AAC / AVC codecs will be supported (this change is not
> codec agnostic since for AVC for example, the contents
> of the NAL units is encrypted, while the NAL size + type are not)
>
> Before we start working on this feature, since we really prefer not to
> manage our own fork of the code, we wanted to check with you
> whether you will be willing to merge such a feature ?
> (assuming it conforms to the coding standards, naturally)

If implementation is clean and does not break what was already working why not?

>
> Thank you
>
> Eran
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] support for reading / writing encrypted MP4 files

[Eran Kornblau]

Hi all,

We're thinking about adding support for MP4 files encrypted using the CENC 
scheme to ffmpeg (such as the ones supported by GPAC -
https://gpac.wp.mines-telecom.fr/mp4box/encryption/common-encryption/)
The motivation is to be able to transcode files without having the media ever 
resident in the clear on disk (some of our customers have
security regulations that forbid that).

We'll probably add to movenc.c a parameter of encryption-scheme, supporting 
only the value cenc for now, and another parameter
that will contain the encryption key.
Similarly, mov.c will be added a parameter of encryption-key to enable reading 
encrypted MP4s.
At first phase, only AAC / AVC codecs will be supported (this change is not 
codec agnostic since for AVC for example, the contents
of the NAL units is encrypted, while the NAL size + type are not)

Before we start working on this feature, since we really prefer not to manage 
our own fork of the code, we wanted to check with you
whether you will be willing to merge such a feature ?
(assuming it conforms to the coding standards, naturally)

Thank you

Eran

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel