Reimar D?ffinger wrote:
> Hello,
> attached patch is just a demonstration. I'd like to get comments, and
> preferably also some help for the actual implementation.
> Problems:
> 1) The key is hardcoded (dkey variable). Is adding uint8_t *key, int
> keylen to AVFormatContext an acceptable to "export" this?
Or adding a parameter to AVFormatParameters, and using av_set_parameters.
> 2) Seeking does not work with encrypted files
An implementation using MXF Index would be nice.
> 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, ...)
I am opposed to simplication leading to obfuscation and deep coding
style change.
> [...]
>
> +#include <openssl/aes.h>
> #include "avformat.h"
>
> typedef uint8_t UID[16];
> @@ -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.
> #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.
> @@ -210,6 +200,8 @@
> {
> int i;
>
> + if (!IS_KLV_KEY(klv->key, mxf_essence_element_key))
> + return -1;
> for (i = 0; i < s->nb_streams; i++) {
> MXFTrack *track = s->streams[i]->priv_data;
> /* SMPTE 379M 7.3 */
> @@ -228,8 +220,7 @@
>
> if (length > 61444) /* worst case PAL 1920 samples 8 channels */
> return -1;
> - get_buffer(pb, buffer, length);
> - av_new_packet(pkt, length);
> + memcpy(buffer, pkt->data, length);
> data_ptr = pkt->data;
> end_ptr = buffer + length;
> buf_ptr = buffer + 4; /* skip SMPTE 331M header */
> @@ -249,6 +240,62 @@
> return 0;
> }
>
> +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.
Is it needed to check for size every 2 lines ?
> static int mxf_read_packet(AVFormatContext *s, AVPacket *pkt)
> {
> KLVPacket klv;
> @@ -261,21 +308,27 @@
> #ifdef DEBUG
> PRINT_KEY("read packet", klv.key);
> #endif
> - 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.
> @@ -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.
Like I said previously demuxer is actually following MXF philosophy and
implement all basics principles to prepare handling complex files.
For encrypted files it might be necessary to prepare the code to handle
cryptographic framework to retrieve cipher algo, context id, and crypto
key id.
--
Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA
SMARTJOG S.A. http://www.smartjog.com
Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4