[Ffmpeg-devel] [RFC] MXF AES decryption

[Reimar Döffinger]

Hello,
On Thu, Jan 11, 2007 at 11:41:43AM +0100, Baptiste Coudurier wrote:
> Reimar D?ffinger wrote:
[...]
> > 2) Seeking does not work with encrypted files
> 
> An implementation using MXF Index would be nice.

The current seeking function IMO still should be kept and fixed.
I was thinking of making mxf_read_sync work more like the mpeg header
search stuff, i.e. just search for the next occurence of 0x06,0x0E,0x2B,0x34
and then returning the full 16 bytes from that position or so.
The search function could then first look for a mxf_essence_element_key
and then get the next either mxf_essence_element_key or
mxf_encrypted_triplet_key, whichever comes first. I think that should
work for both cases.

> > 3) SSL configure stuff is needed - or alternatively a standalone AES
> > implementation might be interesting as well...
> > 4) Code needs quite a bit of general cleanup, it seems much too
> > complicated to me for what it does
> 
> It seems pretty clear to me. What looks like complicated ? MXF is
> complicated, take a look at mxflib for example. Also code is prepared to
> support more complicated files (think about OP3C, OP2B, ...)

Exactly the function you complained about being too complicated.

> > @@ -173,6 +174,7 @@
> >  /* partial keys to match */
> >  static const uint8_t mxf_header_partition_pack_key[]       = { 0x06,0x0e,0x2b,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x02 };
> >  static const uint8_t mxf_essence_element_key[]             = { 0x06,0x0e,0x2b,0x34,0x01,0x02,0x01,0x01,0x0d,0x01,0x03,0x01 };
> > +static const uint8_t mxf_encrypted_triplet_key[]           = { 0x06,0x0e,0x2b,0x34,0x02,0x04,0x01,0x07,0x0d,0x01,0x03,0x01,0x02,0x7e,0x01,0x00 };
> >  
> 
> Key is not partial.

?? oh, you mean because of the comment? Well, should I add a "/* full
key to match */" or how?

> 
> >  #define IS_KLV_KEY(x, y) (!memcmp(x, y, sizeof(y)))
> >  
> > @@ -181,20 +183,8 @@
> >  
> >  static int64_t klv_decode_ber_length(ByteIOContext *pb)
> >  {
> > -    int64_t size = 0;
> > -    uint8_t length = get_byte(pb);
> > -    int type = length >> 7;
> > -
> > -    if (type) { /* long form */
> > -        int bytes_num = length & 0x7f;
> > -        /* SMPTE 379M 5.3.4 guarantee that bytes_num must not exceed 8 bytes */
> > -        if (bytes_num > 8)
> > -            return -1;
> > -        while (bytes_num--)
> > -            size = size << 8 | get_byte(pb);
> > -    } else {
> > -        size = length & 0x7f;
> > -    }
> > +    uint64_t size;
> > +    GET_BER(size, get_byte(pb), return -1;);
> >      return size;
> >  }
> 
> IMHO GET_BER is unneeded since it is only used in mxf.c, also return
> -1;); looks quite ugly.

Huh? Why "unneeded"? The problem is that klv_decode_ber_length can't be
used on memory, how else to solve this except making two different
functions? Though this change might be unneeded. Anyway, my GET_BER is a
bit simplified compared to this function, maybe you can say if you're
interested in me porting these simplifications (if you consider them
such).

> > +static int get_enc_src_klv(AVFormatContext *s, AVPacket *pkt, KLVPacket *klv)
> > +{
> > +    static const uint8_t checkv[16] = {0x43, 0x48, 0x55, 0x4b, 0x43, 0x48, 0x55, 0x4b, 0x43, 0x48, 0x55, 0x4b, 0x43, 0x48, 0x55, 0x4b};
> > +    uint64_t size;
> > +    uint64_t source_sz;
> > +    uint64_t ptoff;
> > +    uint8_t *p = pkt->data;
> > +    uint8_t *end = &pkt->data[pkt->size];
> > +    char ivec[16];
> > +    uint8_t dkey[16] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
> > +    AES_KEY key;
> > +    AES_set_decrypt_key(dkey, 128, &key);
> > +    // crypto context
> > +    if (&p[9 + 16] > end) return -1;
> > +    GET_BER(size, *p++ , return -1;);
> > +    if (size != 16) return -1;
> > +    p += size;
> > +    // plaintext offset
> > +    if (&p[9 + 8] > end) return -1;
> > +    GET_BER(size, *p++ , return -1;);
> > +    if (size != 8) return -1;
> > +    ptoff = (BE_32(p) << 32) | BE_32(p + 4);
> > +    p += size;
> > +    // source klv key
> > +    if (&p[9 + 16] > end) return -1;
> > +    GET_BER(size, *p++ , return -1;);
> > +    if (size != 16) return -1;
> > +    memcpy(&klv->key, p, 16);
> > +    p += 16;
> > +    // source size
> > +    if (&p[9 + 8] > end) return -1;
> > +    GET_BER(size, *p++ , return -1;);
> > +    if (size != 8) return -1;
> > +    source_sz = BE_32(p + 4);
> > +    p += size;
> > +    // enc. code
> > +    if (&p[9] > end) return -1;
> > +    GET_BER(size, *p++ , return -1;);
> > +    if (&p[size] > end || &p[size] < p) return -1;
> > +    if (size < 32) return -1;
> > +    memcpy(ivec, p, 16);
> > +    p += 16;
> > +    AES_cbc_encrypt(p, &pkt->data[ptoff], 16, &key, ivec, AES_DECRYPT);
> > +    if (memcmp(&pkt->data[ptoff], checkv, 16)) return -1;
> > +    p += 16;
> > +    size -= 32;
> > +    if (size < ptoff) return -1;
> > +    memcpy(pkt->data, p, ptoff);
> > +    size -= ptoff;
> > +    p += ptoff;
> > +    if (size < source_sz) return -1;
> > +    AES_cbc_encrypt(p, &pkt->data[ptoff], size, &key, ivec, AES_DECRYPT);
> > +    pkt->size = source_sz;
> > +    return 0;
> > +}
> > +
> 
> That function looks really ugly and obfuscated. Current code is
> deliberatly following naming convention from the specifications.
> Also prefix function with mxf_
> 
> mxf_decrypt_triplet might be good.

Agreed in this case (though having the mxf_ prefix everywhere is not
that advantageous IMO), but in general the specs do have some quite
nonsensical names (like the use of the word "key"). Also I like to
understand what I do, which is why I don't read the specs while
programming but only before and after, so I of course choose my own
names.

> Is it needed to check for size every 2 lines ?

If esp. we decode directly there probably is no reason for this function
to operate on memory but instead could read directly from file. In this
case all the checks could be removed. As I said, the code needs some
serious cleanup, esp. this part.

> > -        if (IS_KLV_KEY(klv.key, mxf_essence_element_key)) {
> > -            int index = mxf_get_stream_index(s, &klv);
> > +        if (IS_KLV_KEY(klv.key, mxf_essence_element_key) ||
> > +            IS_KLV_KEY(klv.key, mxf_encrypted_triplet_key)) {
> > +            int index;
> > +            av_get_packet(&s->pb, pkt, klv.length);
> > +            if (IS_KLV_KEY(klv.key, mxf_encrypted_triplet_key) &&
> > +                get_enc_src_klv(s, pkt, &klv) < 0)
> > +                av_log(s, AV_LOG_ERROR, "invalid encoded triplet\n");
> > +            index = mxf_get_stream_index(s, &klv);
> >              if (index < 0) {
> >                  av_log(s, AV_LOG_ERROR, "error getting stream index\n");
> > -                url_fskip(&s->pb, klv.length);
> > +                av_free_packet(pkt);
> >                  return -1;
> >              }
> >              /* check for 8 channels AES3 element */
> >              if (klv.key[12] == 0x06 && klv.key[13] == 0x01 && klv.key[14] == 0x10) {
> >                  if (mxf_get_d10_aes3_packet(&s->pb, s->streams[index], pkt, klv.length) < 0) {
> >                      av_log(s, AV_LOG_ERROR, "error reading D-10 aes3 frame\n");
> > +                    av_free_packet(pkt);
> >                      return -1;
> >                  }
> > -            } else
> > -                av_get_packet(&s->pb, pkt, klv.length);
> > +            }
> >              pkt->stream_index = index;
> >              return 0;
> >          } else
> 
> Freeing packet is overkill, I have some files with broken track number,
> that's why code is that way. Useless allocating/freeing should be
> avoided where it can.

I was not aware that not freeing packets is acceptable and handled
correctly...

> > @@ -707,6 +760,7 @@
> >      { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x02,0x0D,0x01,0x03,0x01,0x02,0x04,0x60,0x01 }, CODEC_ID_MPEG2VIDEO, Frame }, /* MPEG-ES Frame wrapped */
> >      { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x02,0x0D,0x01,0x03,0x01,0x02,0x04,0xe0,0x02 }, CODEC_ID_MPEG2VIDEO,  Clip }, /* MPEG-ES Clip wrapped, 0xe0 MPV stream id */
> >      { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0D,0x01,0x03,0x01,0x02,0x04,0x61,0x07 }, CODEC_ID_MPEG2VIDEO,  Clip }, /* MPEG-ES Custom wrapped, 0x61 ??? stream id */
> > +    { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x07,0x0D,0x01,0x03,0x01,0x02,0x0b,0x01,0x00 }, CODEC_ID_MPEG2VIDEO, Frame }, /* MPEG-ES Frame wrapped, encrypted ??? */
> >      { { 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00 },       CODEC_ID_NONE, Frame },
> >  };
> 
> That key is Encrypted essence container label, also that key can be used
> with JPEG 2000 like my files are.

So where to get the codec id from??

Greetings,
Reimar D?ffinger