[fix] clientDataHash was not persistent over successive calls, leadin…

…g the signature returned by GET_NEXT_ASSERTION to be invalid

include/globals.h

@@ -99,9 +99,10 @@ typedef struct global_s {
     char buffer_20[20];
     char buffer1_65[NAME_BUFFER_SIZE];
     char buffer2_65[NAME_BUFFER_SIZE];
-    char display_status[131];
+    char displayed_message[131];
     bool is_nfc;
-    bool is_getNextAssertion;
+    bool display_status;
+    bool get_next_assertion_enabled;
 } global_t;
 
 extern global_t g;
@@ -164,7 +165,7 @@ void truncate_pairs_for_display(bool large);
  *
  * @param clean_buffer: always insert a '\0' character at the beginning of the buffer
  */
-void prepare_display_status(bool clean_buffer);
+void prepare_displayed_message(bool clean_buffer);
 
 void ctap2_display_copy_username(const char *name, uint8_t nameLength);
 void ctap2_display_copy_rp(const char *name, uint8_t nameLength);

src/ctap2/get_assertion/get_assertion.c

@@ -62,11 +62,12 @@ static int decode_clientDataHash(cbipDecoder_t *decoder, cbipItem_t *mapItem) {
     ctap2_assert_data_t *ctap2AssertData = globals_get_ctap2_assert_data();
     uint32_t itemLength;
     int status;
+    uint8_t *tmp_ptr;
 
     status = cbiph_get_map_key_bytes(decoder,
                                      mapItem,
                                      TAG_CLIENT_DATA_HASH,
-                                     &ctap2AssertData->clientDataHash,
+                                     &tmp_ptr,
                                      &itemLength);
     if (status != CBIPH_STATUS_FOUND) {
         PRINTF("Error fetching clientDataHash\n");
@@ -76,6 +77,9 @@ static int decode_clientDataHash(cbipDecoder_t *decoder, cbipItem_t *mapItem) {
         PRINTF("Invalid clientDataHash length\n");
         return ERROR_INVALID_CBOR;
     }
+    // The clientDataHash can be reused on successive calls (GET_ASSERTION / GET_NEXT_ASSERTION),
+    // thus it must be stored in static memory so its content  won't change across several calls
+    memcpy(ctap2AssertData->clientDataHash, tmp_ptr, CX_SHA256_SIZE);
     return 0;
 }
 
@@ -263,7 +267,9 @@ static void nfc_handle_get_assertion() {
         if (ctap2AssertData->availableCredentials > 1) {
             // This settings will disable the app_nbgl_status call (nothing displayed on SK)
             // Else, this would lead the app to respond too slowly, and the client to bug out
-            g.is_getNextAssertion = true;
+            g.display_status = false;
+            // This settings will allow the client to get info from following GET_NEXT_ASSERTION_calls
+            g.get_next_assertion_enabled = true;
         }
         if (ctap2AssertData->availableCredentials == 0) {
             send_cbor_error(&G_io_u2f, ERROR_NO_CREDENTIALS);
@@ -290,6 +296,8 @@ void ctap2_get_assertion_handle(u2f_service_t *service, uint8_t *buffer, uint16_
 
     PRINTF("CTAP2 get_assertion_handle\n");
 
+    // GET_NEXT_ASSERTION flow is disabled by default.
+    g.get_next_assertion_enabled = false;
     memset(ctap2AssertData, 0, sizeof(ctap2_assert_data_t));
     ctap2AssertData->buffer = buffer;
 

src/ctap2/get_assertion/get_assertion_struct.h

@@ -36,7 +36,7 @@ typedef struct ctap2_assert_data_s {
     uint8_t *buffer;  // pointer to the CBOR message in the APDU buffer
     char *rpId;
     uint32_t rpIdLen;
-    uint8_t *clientDataHash;  // size of CX_SHA256_SIZE
+    uint8_t clientDataHash[CX_SHA256_SIZE];  // Could be reused over successive GET_ASSERTION / GET_NEXT_ASSERTION
     uint8_t *credId;
     uint32_t credIdLen;
     uint8_t *nonce;

src/ctap2/get_assertion/get_assertion_utils.c

@@ -418,7 +418,7 @@ static int build_and_encode_getAssertion_response(uint8_t *buffer,
     }
     // If RK: encoding credential info
     if (credData->residentKey) {
-        const bool encode_username = (g.is_getNextAssertion && credData->userStr != NULL);
+        const bool encode_username = (!g.display_status && credData->userStr != NULL);
         cbip_add_int(&encoder, TAG_RESP_USER);
         cbip_add_map_header(&encoder, encode_username ? 3 : 1);
         cbip_add_string(&encoder, KEY_USER_ID, sizeof(KEY_USER_ID) - 1);

src/ctap2/get_next_assertion.c

@@ -28,8 +28,8 @@ void ctap2_get_next_assertion_handle(u2f_service_t *service, uint8_t *buffer, ui
     UNUSED(length);
     ctap2_assert_data_t *ctap2AssertData = globals_get_ctap2_assert_data();
 
-    if (ctap2AssertData->allowListPresent) {
-        PRINTF("GET_NEXT_ASSERTION not implemented for non-RK credentials.\n");
+    if (!g.get_next_assertion_enabled) {
+        PRINTF("GET_NEXT_ASSERTION only implemented for RK credentials over NFC.\n");
         send_cbor_error(service, ERROR_NOT_ALLOWED);
         return;
     } else {
@@ -45,7 +45,7 @@ void ctap2_get_next_assertion_handle(u2f_service_t *service, uint8_t *buffer, ui
             send_cbor_error(service, ERROR_NOT_ALLOWED);
             return;
         }
-        g.is_getNextAssertion = true;
+        g.display_status = false;
         get_assertion_send();
     }
 }

src/globals.c

@@ -68,14 +68,14 @@ void truncate_pairs_for_display(bool large) {
     PRINTF("buffer2_65 after truncation: '%s'\n", g.buffer2_65);
 }
 
-void prepare_display_status(bool clean_buffer) {
+void prepare_displayed_message(bool clean_buffer) {
     if (clean_buffer) {
         PRINTF("NO NFC or cleaning, so no display status for buffer1_65 '%s' and buffer2_65 '%s'\n",
                g.buffer1_65,
                g.buffer2_65);
-        g.display_status[0] = '\0';
+        g.displayed_message[0] = '\0';
         return;
     }
-    snprintf(g.display_status, sizeof(g.display_status), "%s\n%s", g.buffer1_65, g.buffer2_65);
-    PRINTF("NFC so display status is: '%s'\n", g.display_status);
+    snprintf(g.displayed_message, sizeof(g.displayed_message), "%s\n%s", g.buffer1_65, g.buffer2_65);
+    PRINTF("NFC so display status is: '%s'\n", g.displayed_message);
 }

src/nfc_io.c

@@ -81,10 +81,10 @@ int nfc_io_send_prepared_response() {
     }
 
     int ret = io_send_response_pointer(responseBuffer + start, size, sw);
-    if (sw == SW_NO_ERROR && nfc_status != NULL && !g.is_getNextAssertion) {
+    if (sw == SW_NO_ERROR && nfc_status != NULL && g.display_status) {
         app_nbgl_status(nfc_status, true, ui_idle);
     }
-    g.is_getNextAssertion = false;
+    g.display_status = true;
     return ret;
 }
 

src/ui_shared_nbgl.c

@@ -285,9 +285,9 @@ void app_nbgl_status(const char *message, bool is_success, nbgl_callback_t on_qu
         // Truncate display buffers for small police (hence `false`) then format them into the
         // display buffer (which is then used in `centeredInfo.text3`)
         truncate_pairs_for_display(false);
-        prepare_display_status(false);
+        prepare_displayed_message(false);
     } else {
-        prepare_display_status(true);
+        prepare_displayed_message(true);
     }
 
     if (is_success == true) {
@@ -300,7 +300,7 @@ void app_nbgl_status(const char *message, bool is_success, nbgl_callback_t on_qu
         .tickerValue = 3000    // 3 seconds
     };
     onQuit = on_quit;
-    PRINTF("Will be displayed: '%s'\n", g.display_status);
+    PRINTF("Will be displayed: '%s'\n", g.displayed_message);
     nbgl_pageInfoDescription_t info = {
         .bottomButtonStyle = NO_BUTTON_STYLE,
         .footerText = NULL,
@@ -311,7 +311,7 @@ void app_nbgl_status(const char *message, bool is_success, nbgl_callback_t on_qu
         .centeredInfo.text1 = message,
         .centeredInfo.text2 = NULL,
         /* .centeredInfo.text3 = NULL, */
-        .centeredInfo.text3 = g.display_status[0] == 0 ? NULL : &g.display_status[0],
+        .centeredInfo.text3 = g.displayed_message[0] == 0 ? NULL : &g.displayed_message[0],
         .tapActionText = NULL,
         .tapActionToken = QUIT_TOKEN,
         .topRightStyle = NO_BUTTON_STYLE,

tests/functional/client.py

@@ -159,3 +159,44 @@ def activate_rk_option(self):
 
         self.navigator.navigate(instructions,
                                 screen_change_before_first_instruction=False)
+
+    def enable_rk_option(self):
+      if self.ctap2.info.options["rk"]:
+          return
+
+      if self.firmware.is_nano:
+          instructions = [
+              # Enter in the settings
+              NavInsID.RIGHT_CLICK,
+              NavInsID.RIGHT_CLICK,
+              NavInsID.RIGHT_CLICK,
+              NavInsID.BOTH_CLICK,
+              # Enable and skip "Enabling" message
+              NavInsID.BOTH_CLICK
+          ]
+          if self.firmware is not Firmware.NANOS:
+              # Screen 0 -> 5
+              instructions += [NavInsID.RIGHT_CLICK] * 5
+          else:
+              # Screen 0 -> 13
+              instructions += [NavInsID.RIGHT_CLICK] * 13
+          instructions += [
+              NavInsID.BOTH_CLICK,
+              # Leave settings
+              NavInsID.RIGHT_CLICK,
+              NavInsID.BOTH_CLICK
+          ]
+      else:
+          instructions = [
+              # Enter in the settings
+              NavInsID.USE_CASE_HOME_SETTINGS,
+              # Enable and skip "Enabling" message
+              NavIns(NavInsID.CHOICE_CHOOSE, (1,)),
+              NavInsID.USE_CASE_CHOICE_CONFIRM,
+              NavInsID.USE_CASE_STATUS_DISMISS,
+              # Leave settings
+              NavInsID.USE_CASE_SETTINGS_MULTI_PAGE_EXIT,
+          ]
+
+      self.navigator.navigate(instructions, screen_change_before_first_instruction=False)
+      self.ctap2._info = self.ctap2.get_info()

tests/functional/ctap2/test_get_next_assertion.py

@@ -1,8 +1,14 @@
 import pytest
 from fido2.ctap import CtapError
 
-from ..utils import generate_random_bytes, ctap2_get_assertion
+from ..utils import generate_random_bytes, ctap2_get_assertion, ENABLE_RK_CONFIG_UI_SETTING, Nav
+from ..transport import TransportType
 
+# This tests reflects the difference of flows depending on NFC or not, RKs or not, AllowList or not,
+# when performing GET_(NEXT_)ASSERTION operations and *several IDs are available*
+# - Not NFC -> User can choose the ID on the screen -> Chosen ID is returned -> GET_NEXT_ASSERTION *not* enabled
+# - NFC with AllowList -> The first matching ID is returned -> GET_NEXT_ASSERTION *not* enabled
+# - NFC without AllowList, meaning the ID(s) *must* be RK(s) -> The first matching ID is returned -> GET_NEXT_ASSERTION enabled
 
 def test_get_next_assertion_no_context(client):
     # If there was no previous authenticatorGetAssertion request
@@ -14,6 +20,7 @@ def test_get_next_assertion_no_context(client):
 
 
 def test_get_next_assertion_two_credentials_allowlist(client):
+    # Only 'passwordless' (no AllowList) + NFC triggers GET_NEXT_ASSERTION
     t1 = ctap2_get_assertion(client)
     rp = t1.args.rp
     t2 = ctap2_get_assertion(client, rp=rp)
@@ -42,3 +49,50 @@ def test_get_next_assertion_two_credentials_allowlist(client):
     with pytest.raises(CtapError) as e:
         client.ctap2.get_next_assertion()
     assert e.value.code == CtapError.ERR.NOT_ALLOWED
+
+
+@pytest.mark.skipif(not ENABLE_RK_CONFIG_UI_SETTING, reason="settings not enable")
+def test_get_next_assertion_two_credentials_rk(client, transport):
+    # Only 'passwordless' (no AllowList) + NFC triggers GET_NEXT_ASSERTION
+    client.enable_rk_option()
+
+    t1 = ctap2_get_assertion(client, rk=True)
+    rp = t1.args.rp
+    t2 = ctap2_get_assertion(client, rp=rp, rk=True)
+    t3 = ctap2_get_assertion(client, rp=rp, rk=True)
+
+    client_data_hash = generate_random_bytes(32)
+
+    if transport is TransportType.NFC:
+        # nothing displayed in this case
+        assertion = client.ctap2.get_assertion(rp["id"], client_data_hash, navigation=Nav.NONE)
+        # GET_NEXT_ASSERTION is enabled!
+        # 3 credentials are available
+        assert assertion.number_of_credentials == 3
+        # they are sorted by age (youngest first)
+        assertion.verify(client_data_hash, t3.credential_data.public_key)
+
+        # then the two other credentials are returned (also sorted)
+        assertion = client.ctap2.get_next_assertion()
+        # only GET_ASSERTION fills the number of credentials
+        assert assertion.number_of_credentials is None
+        assertion.verify(client_data_hash, t2.credential_data.public_key)
+
+        assertion = client.ctap2.get_next_assertion()
+        assert assertion.number_of_credentials is None
+        assertion.verify(client_data_hash, t1.credential_data.public_key)
+
+        # Eventually all credentials are consumed, another call returns an error
+        with pytest.raises(CtapError) as e:
+            client.ctap2.get_next_assertion()
+        assert e.value.code == CtapError.ERR.NOT_ALLOWED
+    else:
+        assertion = client.ctap2.get_assertion(rp["id"], client_data_hash,
+                                               simple_login=False,
+                                               check_users=None)
+        assert assertion.number_of_credentials is None
+        assertion.verify(client_data_hash, t3.credential_data.public_key)
+
+        with pytest.raises(CtapError) as e:
+            client.ctap2.get_next_assertion()
+        assert e.value.code == CtapError.ERR.NOT_ALLOWED

tests/functional/ctap2/test_option_rk.py

@@ -13,8 +13,7 @@
     ctap2_get_assertion, ENABLE_RK_CONFIG_UI_SETTING, MakeCredentialArguments, Nav
 
 
-@pytest.mark.skipif(not ENABLE_RK_CONFIG_UI_SETTING,
-                    reason="settings not enable")
+@pytest.mark.skipif(not ENABLE_RK_CONFIG_UI_SETTING, reason="settings not enable")
 def test_option_rk_disabled(client):
     info = client.ctap2.info
     assert not info.options["rk"]
@@ -25,72 +24,17 @@ def test_option_rk_disabled(client):
         client.ctap2.make_credential(args, navigation=Nav.NONE, will_fail=True)
     assert e.value.code == CtapError.ERR.UNSUPPORTED_OPTION
 
-
-def enable_rk_option(client):
-    info = client.ctap2.info
-    if info.options["rk"]:
-        return
-
-    if not ENABLE_RK_CONFIG_UI_SETTING:
-        raise ValueError("rk and setting not enabled")
-
-    if client.firmware.is_nano:
-        instructions = [
-            # Enter in the settings
-            NavInsID.RIGHT_CLICK,
-            NavInsID.RIGHT_CLICK,
-            NavInsID.RIGHT_CLICK,
-            NavInsID.BOTH_CLICK,
-
-            # Enable and skip "Enabling" message
-            NavInsID.BOTH_CLICK
-        ]
-
-        if client.firmware is not Firmware.NANOS:
-            # Screen 0 -> 5
-            instructions += [NavInsID.RIGHT_CLICK] * 5
-        else:
-            # Screen 0 -> 13
-            instructions += [NavInsID.RIGHT_CLICK] * 13
-
-        instructions += [
-            NavInsID.BOTH_CLICK,
-
-            # Leave settings
-            NavInsID.RIGHT_CLICK,
-            NavInsID.BOTH_CLICK
-        ]
-    else:
-        instructions = [
-            # Enter in the settings
-            NavInsID.USE_CASE_HOME_SETTINGS,
-
-            # Enable and skip "Enabling" message
-            NavIns(NavInsID.CHOICE_CHOOSE, (1,)),
-            NavInsID.USE_CASE_CHOICE_CONFIRM,
-            NavInsID.USE_CASE_STATUS_DISMISS,
-
-            # Leave settings
-            NavInsID.USE_CASE_SETTINGS_MULTI_PAGE_EXIT,
-        ]
-
-    client.navigator.navigate(instructions,
-                              screen_change_before_first_instruction=False)
-
-    client.ctap2._info = client.ctap2.get_info()
-
-
 @pytest.mark.skipif(not ENABLE_RK_CONFIG_UI_SETTING, reason="settings not enable")
 def test_option_rk_enabled(client):
-    enable_rk_option(client)
+    client.enable_rk_option()
 
     info = client.ctap2.info
     assert info.options["rk"]
 
 
 @pytest.mark.skipif(not ENABLE_RK_CONFIG_UI_SETTING, reason="settings not enable")
 def test_option_rk_make_cred_exclude_refused(client, test_name):
-    enable_rk_option(client)
+    client.enable_rk_option()
 
     compare_args = (TESTS_SPECULOS_DIR, client.transported_path(test_name))
     # Spec says that:
@@ -131,7 +75,7 @@ def test_option_rk_make_cred_exclude_refused(client, test_name):
 @pytest.mark.skipif(not ENABLE_RK_CONFIG_UI_SETTING, reason="settings not enable")
 def test_option_rk_get_assertion(client, test_name, transport: TransportType):
     test_prefix = client.transported_path(test_name)
-    enable_rk_option(client)
+    client.enable_rk_option()
 
     user1 = generate_make_credentials_params(client, ref=1, rk=True)
     user2 = generate_make_credentials_params(client, ref=2, rk=True,
@@ -195,7 +139,7 @@ def test_option_rk_get_assertion(client, test_name, transport: TransportType):
 @pytest.mark.skipif("--fast" in sys.argv, reason="running in fast mode")
 @pytest.mark.skipif(not ENABLE_RK_CONFIG_UI_SETTING, reason="settings not enable")
 def test_option_rk_key_store_full(client, transport: TransportType):
-    enable_rk_option(client)
+    client.enable_rk_option()
 
     # Check that at some point KEY_STORE_FULL error is returned
     with pytest.raises(CtapError) as e:
@@ -219,7 +163,7 @@ def test_option_rk_key_store_full(client, transport: TransportType):
 @pytest.mark.skipif(not ENABLE_RK_CONFIG_UI_SETTING, reason="settings not enable")
 def test_option_rk_overwrite_get_assertion(client, test_name):
     test_prefix = client.transported_path(test_name)
-    enable_rk_option(client)
+    client.enable_rk_option()
 
     # Make a first "user1" credential
     args = generate_make_credentials_params(client, ref=1, rk=True)