Fixes signing with partial GOPs (#512)

This functionality has not worked properly if BUs were split in
parts or if the stream had FH or AUDs. Updates of the linked
hash failed.
Various tests for this has now been added.

Also, a API name change from signed_viedo_set_max_signing_frames
to signed_video_set_max_signing_frames. This has no impact on
API breaks since it has not been used anywhere.

The version is bumped to v2.3.1.

Co-authored-by: bjornvolcker <[email protected]>

Author: bjornvolcker

lib/src/includes/signed_video_sign.h

@@ -511,7 +511,7 @@ signed_video_set_hash_algo(signed_video_t *self, const char *name_or_oid);
  * @return An appropriate Signed Video Return Code.
  */
 SignedVideoReturnCode
-signed_viedo_set_max_signing_frames(signed_video_t *self, unsigned max_signing_frames);
+signed_video_set_max_signing_frames(signed_video_t *self, unsigned max_signing_frames);
 
 /**
  * @brief Sets an attestation report to the Signed Video session

lib/src/sv_auth.c

@@ -279,8 +279,10 @@ compute_gop_hash(signed_video_t *self, bu_list_item_t *sei)
       }
 
       // Stop adding Bitstream Units when exceeding the amount that the SEI has reported
-      // in the partial GOP if the SEI was triggered by a partial GOP.
-      if (gop_info->triggered_partial_gop && (num_in_partial_gop >= gop_info->num_sent)) {
+      // in the partial GOP if the SEI was triggered by a partial GOP. By design, one
+      // cannot break at a TG, since they are hashed together with an FH.
+      if (gop_info->triggered_partial_gop && (num_in_partial_gop >= gop_info->num_sent) &&
+          (item->bu->bu_type != BU_TYPE_TG)) {
         break;
       }
       // Skip TGs since they do not have their own hash.

lib/src/sv_common.c

@@ -945,6 +945,10 @@ sv_add_ongoing_hash(signed_video_t *self,
       // Hash |reference_hash| together with the |hash| and store in |bu_hash|.
       SV_THROW(sv_openssl_hash_data(
           self->crypto_handle, gop_info->hash_buddies, hash_size * 2, bu_hash));
+      if (gop_info->triggered_partial_gop && !self->authentication_started) {
+        // If the BU triggered a partial GOP the linked hash has to be updated.
+        SV_THROW(sv_update_linked_hash(self, bu_hash, hash_size));
+      }
     }
 #ifdef SIGNED_VIDEO_DEBUG
     sv_print_hex_data(bu_hash, hash_size, "Hash of %s: ", bu_type_to_str(prev_bu));

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.3.0"
+#define SIGNED_VIDEO_VERSION "v2.3.1"
 #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

@@ -632,6 +632,8 @@ signed_video_add_nalu_part_for_signing_with_timestamp(signed_video_t *self,
     }
     bu_info.hashable_data_size = bu_data_size;
   }
+  // Only completed primary slices, unless FH, can trigger actions.
+  bool is_actionable = bu_info.is_primary_slice && bu_info.is_last_bu_part && !bu_info.is_fh;
 
   status = SV_UNKNOWN_FAILURE;
   SV_TRY()
@@ -642,24 +644,23 @@ signed_video_add_nalu_part_for_signing_with_timestamp(signed_video_t *self,
     // Note that |recurrence| is counted in frames and not in BUs, hence we only increment the
     // 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 (is_actionable) {
       if ((self->frame_count % self->recurrence) == 0) {
         self->has_recurrent_data = true;
       }
       self->frame_count++;  // It is ok for this variable to wrap around
     }
 
     // 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;
+    // Check for a new GOP.
+    bool new_gop = bu_info.is_first_bu_in_gop && is_actionable;
     // 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));
     // Only trigger if this Bitstream Unit is hashable, hence will be added to the hash
     // list. Also, trigger on a primary slice. Otherwise two slices belonging to the same
     // frame will be part of different SEIs.
-    trigger_signing &= bu_info.is_hashable && bu_info.is_primary_slice;
+    trigger_signing &= bu_info.is_hashable && is_actionable;
     gop_info->triggered_partial_gop = false;
     // Depending on the input Bitstream Unit, different actions are taken. If the input is
     // an I-frame there is a transition to a new GOP. That triggers generating a SEI. If
@@ -709,7 +710,7 @@ signed_video_add_nalu_part_for_signing_with_timestamp(signed_video_t *self,
     }
     SV_THROW(sv_hash_and_add(self, &bu_info));
     // Increment frame counter after the incoming Bitstream Unit has been processed.
-    if (bu_info.is_primary_slice && bu_info.is_last_bu_part) {
+    if (is_actionable) {
       gop_info->num_frames_in_partial_gop++;
     }
 
@@ -1089,7 +1090,7 @@ signed_video_set_hash_algo(signed_video_t *self, const char *name_or_oid)
 }
 
 SignedVideoReturnCode
-signed_viedo_set_max_signing_frames(signed_video_t *self, unsigned max_signing_frames)
+signed_video_set_max_signing_frames(signed_video_t *self, unsigned max_signing_frames)
 {
   if (!self) {
     return SV_INVALID_PARAMETER;

meson.build

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

tests/check/check_codec_av1.c

@@ -159,6 +159,36 @@ START_TEST(scrap_first_gop_mixed_with_fh_td_and_obu_metdata)
 }
 END_TEST
 
+START_TEST(partial_gop_with_fh_in_parts)
+{
+  // Device side
+  struct sv_setting setting = settings_av1[_i];
+  const unsigned max_signing_frames = 4;  // Trigger signing after reaching 4 frames.
+  setting.max_signing_frames = max_signing_frames;
+  test_stream_t *list = create_signed_stream_splitted_bu(
+      "|It|Pt|Pt|Pt|Pt|It|Pt|It|Pt|Pt|Pt|Pt|Pt|Pt|Pt|Pt|Pt|It|Pt", setting);
+  test_stream_check_types(list, "|It|Pt|Pt|Pt|Pt|SIt|SPt|It|SPt|Pt|Pt|Pt|SPt|Pt|Pt|Pt|SPt|It|SPt");
+
+  // |It|Pt|Pt|Pt|Pt|SIt|SPt|It|SPt|Pt|Pt|Pt|SPt|Pt|Pt|Pt|SPt|It|SPt
+  //
+  // |It|Pt|Pt|Pt|Pt|S  _.._.._.._.._PP_.                                      (valid, 2 pending)
+  //              Pt|SIt|S           .._.PP_.                                            (v, 2 p)
+  //                  It|SPt|It|S        .._..._PP_.                                     (v, 2 p)
+  //                         It|SPt|Pt|Pt|Pt|S  .._..._.._.._PP_.                        (v, 2 p)
+  //                                      Pt|SPt|Pt|Pt|Pt|S  .._..._.._.._PP_.           (v, 2 p)
+  //                                                   Pt|SPt|It|S        .._..._PP_.    (v, 2 p)
+  //                                                                                        12 pend
+  //                                                          It|SPt             PP_.PP  (v, 6 p)
+  signed_video_accumulated_validation_t final_validation = {
+      SV_AUTH_RESULT_OK, false, 63, 57, 6, SV_PUBKEY_VALIDATION_NOT_FEASIBLE, true, 0, 0};
+  const struct validation_stats expected = {
+      .valid_gops = 6, .invalid_gops = 0, .pending_bu = 12, .final_validation = &final_validation};
+  validate_stream(NULL, list, expected, true);
+
+  test_stream_free(list);
+}
+END_TEST
+
 static Suite *
 signed_video_suite(void)
 {
@@ -178,6 +208,7 @@ signed_video_suite(void)
   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);
+  tcase_add_loop_test(tc, partial_gop_with_fh_in_parts, s, e);
 
   // Add test case to suit
   suite_add_tcase(suite, tc);

tests/check/check_signed_video_auth.c

@@ -2180,6 +2180,35 @@ START_TEST(sign_multislice_stream_partial_gops)
 }
 END_TEST
 
+START_TEST(sign_partial_gops_with_bu_in_parts)
+{
+  // Device side
+  struct sv_setting setting = settings[_i];
+  const unsigned max_signing_frames = 4;  // Trigger signing after reaching 4 frames.
+  setting.max_signing_frames = max_signing_frames;
+  test_stream_t *list = create_signed_stream_splitted_bu("IPPPPPIPPIPPPPPPPPPIP", setting);
+  test_stream_check_types(list, "IPPPPSPISPPISPPPPSPPPPSPISP");
+
+  // IPPPPSPISPPISPPPPSPPPPSPISP
+  //
+  // IPPPPS    ....P.                       (valid, 1 pending)
+  //     PSPIS     ...P.                    (valid, 1 pending)
+  //        ISPPIS    ....P.                (valid, 1 pending)
+  //            ISPPPPS   .....P.           (valid, 1 pending)
+  //                 PSPPPPS   .....P.      (valid, 1 pending)
+  //                      PSPIS     ...P.   (valid, 1 pending)
+  //                                                6 pending
+  //                         ISP       P.P  (valid, 3 pending)
+  signed_video_accumulated_validation_t final_validation = {
+      SV_AUTH_RESULT_OK, false, 27, 24, 3, SV_PUBKEY_VALIDATION_NOT_FEASIBLE, true, 0, 0};
+  const struct validation_stats expected = {
+      .valid_gops = 6, .pending_bu = 6, .final_validation = &final_validation};
+  validate_stream(NULL, list, expected, true);
+
+  test_stream_free(list);
+}
+END_TEST
+
 START_TEST(all_seis_arrive_late_partial_gops)
 {
   // Device side
@@ -3121,6 +3150,7 @@ signed_video_suite(void)
   // Signed partial GOPs
   tcase_add_loop_test(tc, sign_partial_gops, s, e);
   tcase_add_loop_test(tc, sign_multislice_stream_partial_gops, s, e);
+  tcase_add_loop_test(tc, sign_partial_gops_with_bu_in_parts, s, e);
   tcase_add_loop_test(tc, all_seis_arrive_late_partial_gops, s, e);
   tcase_add_loop_test(tc, file_export_and_scrubbing_partial_gops, s, e);
   tcase_add_loop_test(tc, modify_one_p_frame_partial_gops, s, e);

tests/check/check_signed_video_sign.c

@@ -220,9 +220,9 @@ START_TEST(api_inputs)
   ck_assert_int_eq(sv_rc, SV_OK);
 
   // Check setting max signing frames
-  sv_rc = signed_viedo_set_max_signing_frames(NULL, 1);
+  sv_rc = signed_video_set_max_signing_frames(NULL, 1);
   ck_assert_int_eq(sv_rc, SV_INVALID_PARAMETER);
-  sv_rc = signed_viedo_set_max_signing_frames(sv, 1);
+  sv_rc = signed_video_set_max_signing_frames(sv, 1);
   ck_assert_int_eq(sv_rc, SV_OK);
 
   // Setting validation level.
@@ -942,6 +942,20 @@ START_TEST(signing_mulitslice_stream_partial_gops)
 }
 END_TEST
 
+START_TEST(signing_partial_gops_with_bu_in_parts)
+{
+  // Device side
+  struct sv_setting setting = settings[_i];
+  const unsigned max_signing_frames = 4;  // Trigger signing after reaching 4 frames.
+  setting.max_signing_frames = max_signing_frames;
+  test_stream_t *list = create_signed_stream_splitted_bu("IPPPPPIPPIPPPPPPPPPIP", setting);
+  test_stream_check_types(list, "IPPPPSPISPPISPPPPSPPPPSPISP");
+  verify_seis(list, setting);
+
+  test_stream_free(list);
+}
+END_TEST
+
 static Suite *
 signed_video_suite(void)
 {
@@ -976,6 +990,7 @@ signed_video_suite(void)
   tcase_add_loop_test(tc, signing_multiple_gops, s, e);
   tcase_add_loop_test(tc, signing_partial_gops, s, e);
   tcase_add_loop_test(tc, signing_mulitslice_stream_partial_gops, s, e);
+  tcase_add_loop_test(tc, signing_partial_gops_with_bu_in_parts, s, e);
 
   // Add test case to suit
   suite_add_tcase(suite, tc);

tests/check/test_helpers.c

@@ -404,8 +404,8 @@ create_signed_stream_with_sv(signed_video_t *sv,
     if (!(!item->prev && sv->using_golden_sei)) {
       ck_assert(!signed_video_is_golden_sei(sv, item->data, item->data_size));
     }
-    // Only split Bitstream Units that are not generated SEIs.
-    if (split_bu && pulled_seis == 0) {
+    // Only split Bitstream Units that are not generated SEIs and large enough to be split.
+    if (split_bu && pulled_seis == 0 && item->data_size > 2) {
       // Split the Bitstream Unit into 2 parts, where the last part inlcudes the ID and the stop
       // bit.
       rc = signed_video_add_nalu_part_for_signing_with_timestamp(
@@ -519,7 +519,7 @@ get_initialized_signed_video(struct sv_setting settings, bool new_private_key)
   ck_assert_int_eq(signed_video_set_product_info(sv, HW_ID, FW_VER, SER_NO, MANUFACT, ADDR), SV_OK);
   ck_assert_int_eq(signed_video_set_authenticity_level(sv, settings.auth_level), SV_OK);
   ck_assert_int_eq(signed_video_set_max_sei_payload_size(sv, settings.max_sei_payload_size), SV_OK);
-  ck_assert_int_eq(signed_viedo_set_max_signing_frames(sv, settings.max_signing_frames), SV_OK);
+  ck_assert_int_eq(signed_video_set_max_signing_frames(sv, settings.max_signing_frames), SV_OK);
   ck_assert_int_eq(signed_video_set_signing_frequency(sv, settings.signing_frequency), SV_OK);
   ck_assert_int_eq(signed_video_set_hash_algo(sv, settings.hash_algo_name), SV_OK);
   if (settings.codec != SV_CODEC_AV1) {