Bugfixes when signing with FH+TG (#510)

Some special cases did break the signing and vaildation
when using FH+TG. In particular three cases:
 1. If OBU Metadata is added to I-frames that "SEI" will belong
    to the previous GOP, since GOPs are triggered on primary
    slices. That itself is not a problem. The problematic part
    is the very first GOP. When triggering on the second I-frame
    this first GOP looks different from the others due to an
    extra OBU Metadata "SEI".
 2. Single Frame Headers (FH) without Tile Groups (TG) shall
    also be hashed. So far it has been expected to be a TG after
    every FH, but a stream can have FHs without TGs.
 3. On the signing side, TGs can come in parts, which
    complicated FH+TG a little bit.

This commit includes
 * 3 new tests
   - Sign with data split in parts
   - Stream has FHs without TGs
   - Scrap first GOP
 * 2 new dummy OBUs; 1) a no show FH (FH without TG), and 2)
   a TD.
 * SEIs cannot become linked hashes on auth side
 * Mark the first SEI as 'U' if not a Signed Video SEI and the
   current GOP is incomplete.
 * Include the show_existing_frame bit when determining frame
   type.
 * Add a no_hash hash function
 * Do not update frame counter on FH
 * Do not trigger a new GOP on FH
 * Generate SEI if at least one BU was hashed before the first
   I-frame.
 * Fixes start timestamp if stream starts with SEI

Also bumps the version to v2.2.7

Author: bjornvolcker

.github/workflows/approval-workflow.yml

@@ -1,13 +1,24 @@
 name: Signed Video Framework approval workflow
 on: pull_request_review
 jobs:
-  labelWhenApproved:
-    name: Label when approved
+  label-when-approved:
+    name: "Label when approved"
     runs-on: ubuntu-latest
+    outputs:
+      isApprovedByCommiters: ${{ steps.label-when-approved-by-commiters.outputs.isApproved }}
+      isApprovedByAnyone: ${{ steps.label-when-approved-by-anyone.outputs.isApproved }}
     steps:
-    - name: Label when approved
-      uses: pullreminders/label-when-approved-action@master
-      env:
-        APPROVALS: "2"
-        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-        ADD_LABEL: "signed-video-framework approved"
+      - name: Label when approved by commiters
+        uses: TobKed/[email protected]
+        id: label-when-approved-by-commiters
+        with:
+          token: ${{ secrets.GITHUB_TOKEN }}
+          label: 'ready to merge (committers)'
+          require_committers_approval: 'true'
+          remove_label_when_approval_missing: 'false'
+          comment: 'PR approved by at least one committer and no changes requested.'
+      - name: Label when approved by anyone
+        uses: TobKed/[email protected]
+        id: label-when-approved-by-anyone
+        with:
+          token: ${{ secrets.GITHUB_TOKEN }}

lib/src/sv_auth.c

@@ -216,6 +216,11 @@ update_link_hash(signed_video_t *self, const bu_list_item_t *sei)
     if (item == sei) {
       break;
     }
+    // A SEI cannot be a linked hash.
+    // TODO: Investigate if a SEI can become a linked hash when signing multiple GOPs.
+    if (item->bu && item->bu->bu_type == BU_TYPE_SEI) {
+      break;
+    }
 
     sv_update_linked_hash(self, item->hash, hash_size);
     break;
@@ -1256,6 +1261,17 @@ maybe_validate_gop(signed_video_t *self, bu_info_t *bu)
     }
 
     if (start_sei_in_sync != validation_flags->sei_in_sync) {
+      // For AV1 OBU Metadata (SEIs) are hashed. Since these prepend picture OBUs. When
+      // prepending an I-frame they, by design, belong to the previous GOP, but will
+      // still be present when exporting to file. If so, the first one is not feasible
+      // to validate and should be marked as 'U'.
+      // TODO: Find a more generic solution to this.
+      bu_list_item_t *item = self->bu_list->first_item;
+      bool is_other_sei = (item->bu && item->bu->bu_type == BU_TYPE_SEI && !item->bu->is_sv_sei);
+      if (item->tmp_validation_status == 'N' && is_other_sei) {
+        item->tmp_validation_status = 'U';
+      }
+
       // Some partial GOPs may have been discarded due to being out of sync at that time
       // because they were in fact correctly invalid. Get stats another time and update
       // |latest->authenticity|.

lib/src/sv_codec_av1.c

@@ -77,7 +77,8 @@ parse_av1_obu_header(bu_info_t *obu)
       obu_header_is_valid &= (obu_size == 0);
       break;
     case 3:  // 3 OBU_FRAME_HEADER
-      obu->bu_type = ((*obu_ptr & 0x60) >> 5) == 0 ? BU_TYPE_I : BU_TYPE_P;
+      // Read frame_type (2 bits), include also show_existing_frame since it must be 0
+      obu->bu_type = ((*obu_ptr & 0xf0) >> 5) == 0 ? BU_TYPE_I : BU_TYPE_P;
       obu->is_primary_slice = true;
       obu->ongoing_hash = obu->bu_type == BU_TYPE_I ? 2 : 1;
       obu->is_fh = true;

lib/src/sv_common.c

@@ -64,6 +64,8 @@ typedef svrc_t (*hash_wrapper_t)(signed_video_t *, const bu_info_t *, uint8_t *,
 static hash_wrapper_t
 get_hash_wrapper(signed_video_t *self, const bu_info_t *bu);
 static svrc_t
+no_hash(signed_video_t *self, const bu_info_t *bu, uint8_t *hash, size_t hash_size);
+static svrc_t
 update_hash(signed_video_t *self, const bu_info_t *bu, uint8_t *hash, size_t hash_size);
 #ifndef SIGNED_VIDEO_DEBUG
 static svrc_t
@@ -755,7 +757,9 @@ get_hash_wrapper(signed_video_t *self, const bu_info_t *bu)
 {
   assert(self && bu);
 
-  if (!bu->is_last_bu_part || bu->ongoing_hash) {
+  if (!bu->is_hashable) {
+    return no_hash;
+  } else if (!bu->is_last_bu_part || bu->ongoing_hash) {
     // If this is not the last part of a BU, update the hash. Or if there is an
     // |ongoing_hash| for AV1 FH+TG.
     return update_hash;
@@ -775,6 +779,18 @@ get_hash_wrapper(signed_video_t *self, const bu_info_t *bu)
 
 /* Hash wrapper functions */
 
+/* no_hash()
+ *
+ * performs no hashing. */
+static svrc_t
+no_hash(signed_video_t ATTR_UNUSED *self,
+    const bu_info_t ATTR_UNUSED *bu,
+    uint8_t ATTR_UNUSED *hash,
+    size_t ATTR_UNUSED hash_size)
+{
+  return SV_OK;
+}
+
 /* update_hash()
  *
  * takes the |hashable_data| from the Bitstream Unit, and updates the hash in |crypto_handle|. */
@@ -957,7 +973,7 @@ sv_hash_and_add(signed_video_t *self, const bu_info_t *bu)
   if (!self || !bu) return SV_INVALID_PARAMETER;
 
   if (!bu->is_hashable) {
-    DEBUG_LOG("This Bitstream Unit (type %d) was not hashed", bu->bu_type);
+    DEBUG_LOG("This Bitstream Unit (type %s) was not hashed", bu_type_to_str(bu));
     return SV_OK;
   }
 
@@ -968,10 +984,11 @@ sv_hash_and_add(signed_video_t *self, const bu_info_t *bu)
 
   svrc_t status = SV_UNKNOWN_FAILURE;
   SV_TRY()
-    if (bu->is_first_bu_part && (!bu->is_last_bu_part || bu->is_fh)) {
-      // If this is the first part of a non-complete BU, or an OBU_FRAME_HEADER (FH),
-      // initialize the |crypto_handle| to enable sequentially updating the hash with more
-      // parts.
+    if (bu->is_first_bu_part && (!bu->is_last_bu_part || bu->is_fh) &&
+        (bu->bu_type != BU_TYPE_TG)) {
+      // If this is the first part of a non-complete BU (Tile Group excluded), or an
+      // OBU_FRAME_HEADER (FH), initialize the |crypto_handle| to enable sequentially
+      // updating the hash with more parts.
       SV_THROW(sv_openssl_init_hash(self->crypto_handle, false));
     }
     // Select hash function, hash the BU and store as 'latest hash'
@@ -1003,8 +1020,9 @@ hash_and_add_for_auth(signed_video_t *self, bu_list_item_t *item)
   bu_info_t *bu = item->bu;
   if (!bu) return SV_INVALID_PARAMETER;
 
-  if (!bu->is_hashable) {
-    DEBUG_LOG("This Bitstream Unit (type %d) was not hashed.", bu->bu_type);
+  // Abort if this BU is not hashable unless there is an |ongoing_hash|.
+  if (!bu->is_hashable && !(item->prev && item->prev->bu && item->prev->bu->ongoing_hash)) {
+    DEBUG_LOG("This Bitstream Unit (type %s) was not hashed.", bu_type_to_str(bu));
     return SV_OK;
   }
   if (!self->validation_flags.hash_algo_known) {
@@ -1022,8 +1040,8 @@ hash_and_add_for_auth(signed_video_t *self, bu_list_item_t *item)
   bu_info_t *prev_bu = item->prev ? item->prev->bu : NULL;
 
   // Update |ongoing_hash| from the previous item.
-  if (bu->bu_type == BU_TYPE_TG && item->prev) {
-    bu->ongoing_hash = item->prev->bu->ongoing_hash;
+  if (bu->bu_type == BU_TYPE_TG && prev_bu) {
+    bu->ongoing_hash = prev_bu->ongoing_hash;
   } else {
     // Find the hash of the previous FH if there is an |ongoing_hash|.
     while (prev_item && prev_item->bu && prev_item->bu->ongoing_hash && !prev_item->bu->is_fh) {

lib/src/sv_internal.h

@@ -50,7 +50,7 @@
 #define DEFAULT_HASH_SIZE (256 / 8)
 
 #define SV_VERSION_BYTES 3
-#define SIGNED_VIDEO_VERSION "v2.2.6"
+#define SIGNED_VIDEO_VERSION "v2.2.7"
 #define SV_VERSION_MAX_STRLEN 19  // Longest possible string including 'ONVIF' prefix
 
 #define DEFAULT_AUTHENTICITY_LEVEL SV_AUTHENTICITY_LEVEL_FRAME

lib/src/sv_sign.c

@@ -610,6 +610,8 @@ signed_video_add_nalu_part_for_signing_with_timestamp(signed_video_t *self,
     // Update |ongoing_hash| from the previous BU.
     if (bu_info.bu_type == BU_TYPE_TG) {
       bu_info.ongoing_hash = self->last_bu->ongoing_hash;
+      bu_info.is_primary_slice = self->last_bu->is_primary_slice;
+      bu_info.is_first_bu_in_gop = self->last_bu->is_first_bu_in_gop;
     }
     // Finalize ongoing hash as soon as the incoming one is not a TG. Only TGs are
     // expected after an FH.
@@ -638,8 +640,9 @@ signed_video_add_nalu_part_for_signing_with_timestamp(signed_video_t *self,
     SV_THROW_IF(bu_info.is_valid < 0, SV_INVALID_PARAMETER);
 
     // Note that |recurrence| is counted in frames and not in BUs, hence we only increment the
-    // counter for primary slices.
-    if (bu_info.is_primary_slice && bu_info.is_last_bu_part) {
+    // counter for primary slices. Frame counting is updated at the end of a frame, hence
+    // shall be ignored for FH.
+    if (bu_info.is_primary_slice && bu_info.is_last_bu_part && !bu_info.is_fh) {
       if ((self->frame_count % self->recurrence) == 0) {
         self->has_recurrent_data = true;
       }
@@ -648,6 +651,8 @@ signed_video_add_nalu_part_for_signing_with_timestamp(signed_video_t *self,
 
     // Determine if a SEI should be generated.
     bool new_gop = (bu_info.is_first_bu_in_gop && bu_info.is_last_bu_part);
+    // Do not trigger on FH.
+    new_gop &= !bu_info.is_fh;
     // Trigger signing if number of frames exceeds the limit for a partial GOP.
     bool trigger_signing = ((self->max_signing_frames > 0) &&
         (gop_info->num_frames_in_partial_gop >= self->max_signing_frames));
@@ -669,11 +674,23 @@ signed_video_add_nalu_part_for_signing_with_timestamp(signed_video_t *self,
         gop_info->has_timestamp = true;
       }
 
-      if (self->sei_generation_enabled) {
+      // SEI creation is triggered by I-frames. Skip the first trigger unless there are
+      // already hashed BUs.
+      if (self->sei_generation_enabled || gop_info->list_idx > 0) {
         // If there are hashes added to the hash list, the |computed_gop_hash| can be finalized.
         SV_THROW(sv_openssl_finalize_hash(self->crypto_handle, gop_info->computed_gop_hash, true));
         // The previous GOP is now completed. The gop_hash was reset right after signing and
         // adding it to the SEI.
+
+        // Update GOP counter and |start_timestamp| if this first GOP started without an
+        // I-frame.
+        if (!self->sei_generation_enabled && gop_info->list_idx > 0) {
+          gop_info->start_timestamp = gop_info->end_timestamp;
+          if (gop_info->current_partial_gop < 0) {
+            gop_info->current_partial_gop = 0;
+          }
+          trigger_signing = true;
+        }
         SV_THROW(generate_sei_and_add_to_buffer(self, trigger_signing));
         if (new_gop && (self->num_gops_until_signing == 0)) {
           // Reset signing counter only upon new GOPs

meson.build

@@ -1,5 +1,5 @@
 project('signed-video-framework', 'c',
-  version : '2.2.6',
+  version : '2.2.7',
   meson_version : '>= 0.53.0',
   default_options : [ 'warning_level=2',
                       'werror=true',

tests/check/check_codec_av1.c

@@ -57,7 +57,6 @@ START_TEST(signed_stream_with_fh)
   // |settings_av1|; See signed_video_helpers.h.
 
   struct sv_setting setting = settings_av1[_i];
-  setting.with_fh = true;
   test_stream_t *list = create_signed_stream("ItPtPtItPtPtItPtPtItPtPtItPtPt", setting);
   test_stream_check_types(list, "ItPtPtItSPtPtItSPtPtItSPtPtItSPtPt");
 
@@ -79,6 +78,87 @@ START_TEST(signed_stream_with_fh)
 }
 END_TEST
 
+START_TEST(signed_stream_in_parts)
+{
+  test_stream_t *list =
+      create_signed_stream_splitted_bu("ItPtPtItPtPtItPtPtItPtPt", settings_av1[_i]);
+  test_stream_check_types(list, "ItPtPtItSPtPtItSPtPtItSPtPt");
+  // ItPtPtItSPtPtItSPtPtItSPtPt
+  //
+  // ItPtPtItS                  ......PP.                    (valid, 2 pending)
+  //       ItSPtPtItS                 .......PP.             (valid, 2 pending)
+  //              ItSPtPtItS                 .......PP.      (valid, 2 pending)
+  //                                                                 6 pending
+  //                     ItSPtPt                    PP.PPPP  (valid, 7 pending)
+  signed_video_accumulated_validation_t final_validation = {
+      SV_AUTH_RESULT_OK, false, 27, 20, 7, SV_PUBKEY_VALIDATION_NOT_FEASIBLE, true, 0, 0};
+  const struct validation_stats expected = {
+      .valid_gops = 3, .pending_bu = 6, .final_validation = &final_validation};
+  validate_stream(NULL, list, expected, true);
+
+  test_stream_free(list);
+}
+END_TEST
+
+START_TEST(has_fh_without_tg)
+{
+  // This test runs in a loop with loop index _i, corresponding to struct sv_setting _i in
+  // |settings_av1|; See signed_video_helpers.h.
+
+  struct sv_setting setting = settings_av1[_i];
+  test_stream_t *list = create_signed_stream("|ZIt|Ptf|Pt|It|Pt|Ptf|It|Pt|Pt|It|Pt", setting);
+  // Note that 'f' (single FH) turns into a 'P' when reading the BU.
+  test_stream_check_types(list, "|ZIt|SPtP|Pt|It|SPt|PtP|It|SPt|Pt|It|SPt");
+
+  // |ZIt|SPtP|Pt|It|SPt|PtP|It|SPt|Pt|It|SPt
+  //
+  // |ZIt|S                _.PP_.                                    (valid, 2 pending)
+  //   It|SPtP|Pt|It|S       .._...._.._PP_.                         (valid, 2 pending)
+  //              It|SPt|PtP|It|S       .._..._..._PP_.              (valid, 2 pending)
+  //                         It|SPt|Pt|It|S        .._..._.._PP_.    (valid, 2 pending)
+  //                                                                         8 pending
+  //                                   It|SPt                PP_.PP  (valid, 6 pending)
+  signed_video_accumulated_validation_t final_validation = {
+      SV_AUTH_RESULT_OK, false, 40, 34, 6, SV_PUBKEY_VALIDATION_NOT_FEASIBLE, true, 0, 0};
+  const struct validation_stats expected = {
+      .valid_gops = 4, .pending_bu = 8, .final_validation = &final_validation};
+  validate_stream(NULL, list, expected, true);
+
+  test_stream_free(list);
+}
+END_TEST
+
+START_TEST(scrap_first_gop_mixed_with_fh_td_and_obu_metdata)
+{
+  // This test runs in a loop with loop index _i, corresponding to struct sv_setting _i in
+  // |settings_av1|; See signed_video_helpers.h.
+
+  struct sv_setting setting = settings_av1[_i];
+  test_stream_t *list = create_signed_stream("|ZIt|Ptf|Pt|ZIt|Pt|Ptf|ZIt|Pt|Pt|ZIt|Pt", setting);
+  // Note that 'f' (single FH) turns into a 'P' when reading the BU.
+  test_stream_check_types(list, "|ZIt|SPtP|Pt|ZIt|SPt|PtP|ZIt|SPt|Pt|ZIt|SPt");
+  const int obus_to_remove = 12;
+  test_stream_t *scrapped_gop = test_stream_pop(list, obus_to_remove);
+  test_stream_free(scrapped_gop);
+  test_stream_check_types(list, "|ZIt|SPt|PtP|ZIt|SPt|Pt|ZIt|SPt");
+
+  // |ZIt|SPt|PtP|ZIt|SPt|Pt|ZIt|SPt
+  //
+  // |ZIt|S               _PPP_.                            (signed, 3 pending)
+  //  ZIt|SPt|PtP|ZIt|S    U.._..._..._.PP_.                ( valid, 2 pending)
+  //               It|SPt|Pt|ZIt|S      .._..._.._.PP_.     ( valid, 2 pending)
+  //                                                                 7 pending
+  //                          It|SPt               PP_.PP   ( valid, 6 pending)
+  signed_video_accumulated_validation_t final_validation = {
+      SV_AUTH_RESULT_OK, false, 31, 25, 6, SV_PUBKEY_VALIDATION_NOT_FEASIBLE, true, 0, 0};
+  const struct validation_stats expected = {
+      .valid_gops = 2, .pending_bu = 7, .has_signature = 1, .final_validation = &final_validation};
+  validate_stream(NULL, list, expected, true);
+
+  test_stream_free(list);
+}
+END_TEST
+
 static Suite *
 signed_video_suite(void)
 {
@@ -95,6 +175,9 @@ signed_video_suite(void)
 
   // Add tests
   tcase_add_loop_test(tc, signed_stream_with_fh, s, e);
+  tcase_add_loop_test(tc, signed_stream_in_parts, s, e);
+  tcase_add_loop_test(tc, has_fh_without_tg, s, e);
+  tcase_add_loop_test(tc, scrap_first_gop_mixed_with_fh_td_and_obu_metdata, s, e);
 
   // Add test case to suit
   suite_add_tcase(suite, tc);

tests/check/test_helpers.c

@@ -678,16 +678,20 @@ validate_stream(signed_video_t *sv,
           break;
       }
       public_key_has_changed |= latest->public_key_has_changed;
-      has_timestamp |= latest->has_timestamp;
 
       if (latest->has_timestamp) {
         if (sv->onvif || sv->legacy_sv) {
           // Media Signing and Legacy code only have one timestamp
           ck_assert_int_eq(latest->start_timestamp, latest->end_timestamp);
         } else {
-          ck_assert_int_lt(latest->start_timestamp, latest->end_timestamp);
+          if (has_timestamp) {
+            ck_assert_int_lt(latest->start_timestamp, latest->end_timestamp);
+          } else {
+            ck_assert_int_le(latest->start_timestamp, latest->end_timestamp);
+          }
         }
       }
+      has_timestamp |= latest->has_timestamp;
 
       // Check if product_info has been received and set correctly.
       if ((latest->authenticity != SV_AUTH_RESULT_NOT_SIGNED) &&