Merge pull request #17 from LedgerHQ/develop

 Merge develop in main following version 1.1.1 deployment on P1

.github/workflows/guidelines_enforcer.yml

@@ -21,3 +21,5 @@ jobs:
   guidelines_enforcer:
     name: Call Ledger guidelines_enforcer
     uses: LedgerHQ/ledger-app-workflows/.github/workflows/reusable_guidelines_enforcer.yml@v1
+    with:
+      run_for_devices: '["nanox", "nanosp"]'

Makefile

@@ -34,7 +34,7 @@ APP_LOAD_PARAMS += --appFlags 0x040
 APP_LOAD_PARAMS += $(COMMON_LOAD_PARAMS)
 
 APPVERSION_M=1
-APPVERSION_N=0
+APPVERSION_N=1
 APPVERSION_P=1
 APPVERSION=$(APPVERSION_M).$(APPVERSION_N).$(APPVERSION_P)
 
@@ -144,6 +144,23 @@ DEFINES += HAVE_UX_STACK_INIT_KEEP_TICKER
 DEFINES += HAVE_COUNTER_MARKER
 APP_LOAD_PARAMS += --nocrc
 
+# Disable resetGeneration increment during ctap2 reset
+# This means credentials that are not discoverable won't be properly
+# revocated anymore. Now not that due to the fact this resetGeneration
+# counter was in NVM, it was reset to 0 after each app reinstallation (due
+# to an app update, a firmware update, or just a user triggered uninstall
+# then reinstall flow), and this reset was causing even more issues
+DEFINES += HAVE_NO_RESET_GENERATION_INCREMENT
+
+# Disable by default rk support and expose a setting to enable it
+# This means that by default user won't be able to create "Resident Keys",
+# which are also named "Discoverable Credentials".
+# This has been implemented to protect user from the nvram wipe mostly happening
+# during an app update which will erase their RK credentials we no possibility
+# to restore them.
+# Advanced user can still choose to enable this setting at their own risk.
+DEFINES += HAVE_RK_SUPPORT_SETTING
+
 DEFINES += HAVE_FIDO2_RPID_FILTER
 
 DEFINES += RK_SIZE=6144

README.md

@@ -59,6 +59,7 @@ Due to OS constraints, this Security Key App as some limitations:
   - If the user chooses to uninstall it from Ledger Live
   - If the user chooses to update the app to a new available version
   - If the user updates the OS version
-* Following FIDO2 spec, credentials can be revoked, but the revocation mechanism is based on a counter that - as discoverable credentials - will be wiped upon app deletion.
+  That is why they are not enabled by default, and should be manually enabled in the settings. See HAVE_RK_SUPPORT_SETTING section on the Makefile for more explanations.
+* Following FIDO2 spec, there should be a way to revoked credentials. A revocation mechanism has been implemented based on a counter that - as discoverable credentials - will be wiped upon app deletion. therefore, in order to avoid weird issue on user side, this counter as been disabled. See HAVE_NO_RESET_GENERATION_INCREMENT section on the Makefile for more explanations.
 
 Please look at the dedicated section at the end of [this blog post](https://blog.ledger.com/security-key/) for more detailed explanations.

include/config.h

@@ -19,6 +19,8 @@
 #ifndef __CONFIG_H__
 #define __CONFIG_H__
 
+#include "os.h"
+
 #define WRAPPING_KEY_PATH     0x80575241  // "WRA".encode("ascii").hex()
 #define PRIVATE_KEY_SEED_PATH 0x80504b53  // "PKS".encode("ascii").hex()
 
@@ -32,6 +34,9 @@ typedef struct config_t {
     uint8_t pin[16];
     uint8_t pinSet;
     uint8_t pinRetries;
+#ifdef HAVE_RK_SUPPORT_SETTING
+    uint8_t rk_enabled;
+#endif
 } config_t;
 
 extern config_t const N_u2f_real;
@@ -47,4 +52,9 @@ void config_set_ctap2_pin(uint8_t *pin);
 void config_decrease_ctap2_pin_retry_counter(void);
 void config_reset_ctap2_pin_retry_counter(void);
 
+#ifdef HAVE_RK_SUPPORT_SETTING
+void config_set_rk_enabled(bool enabled);
+bool config_get_rk_enabled(void);
+#endif
+
 #endif

src/config.c

@@ -25,8 +25,6 @@
 #include "rk_storage.h"
 #include "crypto.h"
 
-#define RNG_MODULO 5
-
 config_t const N_u2f_real;
 
 static void derive_and_store_keys(uint32_t resetGeneration) {
@@ -84,6 +82,12 @@ void config_init(void) {
         tmp8 = 0;
         nvm_write((void *) &N_u2f.pinSet, (void *) &tmp8, sizeof(uint8_t));
 
+#ifdef HAVE_RK_SUPPORT_SETTING
+        // Initialize rk_enable value: Disabled by default
+        tmp8 = 0;
+        nvm_write((void *) &N_u2f.rk_enabled, (void *) &tmp8, sizeof(uint8_t));
+#endif
+
         tmp8 = 1;
         nvm_write((void *) &N_u2f.initialized, (void *) &tmp8, sizeof(uint8_t));
     } else {
@@ -106,14 +110,16 @@ uint8_t config_increase_and_get_authentification_counter(uint8_t *buffer) {
 }
 
 void config_process_ctap2_reset(void) {
+#ifndef HAVE_NO_RESET_GENERATION_INCREMENT
     uint32_t resetGeneration = N_u2f.resetGeneration + 1;
-    uint8_t pinSet = 0;
 
     nvm_write((void *) &N_u2f.resetGeneration, (void *) &resetGeneration, sizeof(uint32_t));
 
     // Update keys derived from seed
     derive_and_store_keys(N_u2f.resetGeneration);
+#endif
 
+    uint8_t pinSet = 0;
     nvm_write((void *) &N_u2f.pinSet, (void *) &pinSet, sizeof(uint8_t));
 
     ctap2_client_pin_reset_ctx();
@@ -143,3 +149,14 @@ void config_reset_ctap2_pin_retry_counter(void) {
     uint8_t tmp = CTAP2_PIN_RETRIES;
     nvm_write((void *) &N_u2f.pinRetries, (void *) &tmp, sizeof(uint8_t));
 }
+
+#ifdef HAVE_RK_SUPPORT_SETTING
+void config_set_rk_enabled(bool enabled) {
+    uint8_t tmp8 = enabled ? 1 : 0;
+    nvm_write((void *) &N_u2f.rk_enabled, (void *) &tmp8, sizeof(uint8_t));
+}
+
+bool config_get_rk_enabled(void) {
+    return N_u2f.rk_enabled == 1;
+}
+#endif

src/ctap2_get_assertion.c

@@ -149,6 +149,8 @@ static int process_getAssert_authnr_allowList(cbipDecoder_t *decoder, cbipItem_t
     cbipItem_t tmpItem;
     int arrayLen;
     int status;
+    uint8_t *prevCredId = NULL;
+    uint32_t prevCredIdLen = 0;
 
     ctap2AssertData->allowListPresent = 0;
     ctap2AssertData->availableCredentials = 0;
@@ -174,6 +176,28 @@ static int process_getAssert_authnr_allowList(cbipDecoder_t *decoder, cbipItem_t
                 return status;
             }
 
+            /* Weird behavior seen on Safari on MacOs, allowList entries are duplicated.
+             * seen order is: 1, 2, ..., n, 1', 2', ..., n'.
+             * In order to improve user experience while this might be fix in Safari side,
+             * we decided to filter out the duplicate in a specific scenario:
+             * - they are only 2 credentials in the allowList
+             * - the first and second credentials are valid and are exactly the same.
+             */
+            if (arrayLen == 2) {
+                if (i == 0) {
+                    // Backup credId and credIdLen before parsing next credential
+                    prevCredId = ctap2AssertData->credId;
+                    prevCredIdLen = ctap2AssertData->credIdLen;
+                } else {
+                    if ((ctap2AssertData->availableCredentials == 1) &&
+                        (ctap2AssertData->credIdLen == prevCredIdLen) &&
+                        (memcmp(ctap2AssertData->credId, prevCredId, prevCredIdLen) == 0)) {
+                        // Just ignore this duplicate credential
+                        continue;
+                    }
+                }
+            }
+
             PRINTF("Valid candidate %d\n", i);
             ctap2AssertData->availableCredentials += 1;
         }
@@ -779,9 +803,9 @@ static int sign_and_build_getAssert_authData(uint8_t *authData,
             credentialLength = status;
         } else {
             // No allow list scenario, which mean the credential is already resident
-            credential_data_t credData;
+            credential_data_t tmpCredData;
 
-            status = credential_decode(&credData,
+            status = credential_decode(&tmpCredData,
                                        ctap2AssertData->credential,
                                        ctap2AssertData->credentialLen,
                                        false);
@@ -794,7 +818,7 @@ static int sign_and_build_getAssert_authData(uint8_t *authData,
             credentialLength = bufferLen - WRAPPED_CREDENTIAL_OFFSET;
             status = credential_wrap(ctap2AssertData->rpIdHash,
                                      ctap2AssertData->nonce,
-                                     &credData,
+                                     &tmpCredData,
                                      credential,
                                      credentialLength,
                                      true,