[Ffmpeg-devel] [RFC] MXF AES decryption

[Baptiste Coudurier]

Reimar D?ffinger wrote:
> 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.

Sounds good

>>> 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.

Ok

>>> @@ -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?

Yes

>>>  #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).

Yes please do.

>>>
>>> [...]
>>>

>> 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.

I feel like more comfortable with reading from the file directly.

>>> -        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...

Ok. Somehow I think that checking for encrypted key before, and calling
mxf_decrypt_triplet directly would be cleaner. At this point maybe using
function pointers like in read_header, and getting packet. There will be
3 ways of retrieving data now. What do you think ?

>>>
>>> [...]
>>>

>> 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??
> 

Picture essence coding ul in descriptor, if present. If not then I need
to dig further in specs.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
SMARTJOG S.A.                                    http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312