B2CA-2795: Security review

.github/workflows/codeql_checks.yml

@@ -16,4 +16,5 @@ jobs:
   analyse:
     name: Call Ledger CodeQL analysis
     uses: LedgerHQ/ledger-app-workflows/.github/workflows/reusable_codeql_checks.yml@v1
-    secrets: inherit
+    secrets:
+      token: ${{ secrets.GITHUB_TOKEN }}

.pre-commit-config.yaml

@@ -47,8 +47,8 @@ repos:
     hooks:
       # Run the linter.
       - id: ruff-check
-        types_or: [ python, pyi ]
-        args: [ --fix ]
+        types_or: [python, pyi]
+        args: [--fix]
       # Run the formatter.
       - id: ruff-format
-        types_or: [ python, pyi ]
+        types_or: [python, pyi]

Makefile

@@ -30,7 +30,7 @@ APPNAME = OpenPGP
 # Application version
 APPVERSION_M = 2
 APPVERSION_N = 7
-APPVERSION_P = 1
+APPVERSION_P = 2
 APPVERSION = "$(APPVERSION_M).$(APPVERSION_N).$(APPVERSION_P)"
 
 DEFINES += SPEC_VERSION='"3.3.1"'

clients/wui/pnpm-workspace.yaml

@@ -5,3 +5,6 @@
 # needs to install its platform binary; the others are harmless to allow.
 allowBuilds:
   esbuild: true
+
+overrides:
+  undici: ">=7.29.0"

src/gpg_data.c

@@ -261,6 +261,7 @@ int gpg_apdu_put_data(unsigned int ref) {
     unsigned int t, l, sw;
     unsigned int *ptr_l = NULL;
     unsigned char *ptr_v = NULL;
+    gpg_key_t *ptr_key = NULL;
     void *pkey = NULL;
     cx_aes_key_t aes_key = {0};
     cx_err_t error = CX_INTERNAL_ERROR;
@@ -555,7 +556,7 @@ int gpg_apdu_put_data(unsigned int ref) {
             /* ----------------- User -----------------*/
             /* Name */
         case 0x5B:
-            if (G_gpg_vstate.io_length > sizeof(N_gpg_pstate->name.value)) {
+            if (G_gpg_vstate.io_length > GPG_NAME_LENGTH) {
                 sw = SWO_WRONG_LENGTH;
                 break;
             }
@@ -569,7 +570,7 @@ int gpg_apdu_put_data(unsigned int ref) {
             break;
             /* Login data */
         case 0x5E:
-            if (G_gpg_vstate.io_length > sizeof(N_gpg_pstate->login.value)) {
+            if (G_gpg_vstate.io_length > GPG_EXT_PRIVATE_DO_LENGTH) {
                 sw = SWO_WRONG_LENGTH;
                 break;
             }
@@ -583,7 +584,7 @@ int gpg_apdu_put_data(unsigned int ref) {
             break;
             /* Language preferences */
         case 0x5F2D:
-            if (G_gpg_vstate.io_length > sizeof(N_gpg_pstate->lang.value)) {
+            if (G_gpg_vstate.io_length > GPG_LANG_LENGTH) {
                 sw = SWO_WRONG_LENGTH;
                 break;
             }
@@ -597,7 +598,7 @@ int gpg_apdu_put_data(unsigned int ref) {
             break;
             /* salutation */
         case 0x5F35:
-            if (G_gpg_vstate.io_length != sizeof(N_gpg_pstate->salutation)) {
+            if (G_gpg_vstate.io_length != GPG_SALUTATION_LENGTH) {
                 sw = SWO_WRONG_LENGTH;
                 break;
             }
@@ -608,7 +609,7 @@ int gpg_apdu_put_data(unsigned int ref) {
             break;
             /* Uniform resource locator */
         case 0x5F50:
-            if (G_gpg_vstate.io_length > sizeof(N_gpg_pstate->keys[G_gpg_vstate.slot].url.value)) {
+            if (G_gpg_vstate.io_length > GPG_EXT_PRIVATE_DO_LENGTH) {
                 sw = SWO_WRONG_LENGTH;
                 break;
             }
@@ -657,17 +658,20 @@ int gpg_apdu_put_data(unsigned int ref) {
         case 0xC1:
             ptr_l = &G_gpg_vstate.kslot->sig.attributes.length;
             ptr_v = G_gpg_vstate.kslot->sig.attributes.value;
+            ptr_key = &G_gpg_vstate.kslot->sig;
             goto WRITE_ATTRIBUTES;
         case 0xC2:
             ptr_l = &G_gpg_vstate.kslot->dec.attributes.length;
             ptr_v = G_gpg_vstate.kslot->dec.attributes.value;
+            ptr_key = &G_gpg_vstate.kslot->dec;
 #ifdef NO_DECRYPT_cv25519
             decKey = true;
 #endif
             goto WRITE_ATTRIBUTES;
         case 0xC3:
             ptr_l = &G_gpg_vstate.kslot->aut.attributes.length;
             ptr_v = G_gpg_vstate.kslot->aut.attributes.value;
+            ptr_key = &G_gpg_vstate.kslot->aut;
             goto WRITE_ATTRIBUTES;
         WRITE_ATTRIBUTES:
             if (G_gpg_vstate.io_length > 12) {
@@ -706,6 +710,7 @@ int gpg_apdu_put_data(unsigned int ref) {
             }
 
             if (sw == SWO_SUCCESS) {
+                nvm_write(ptr_key, NULL, sizeof(gpg_key_t));
                 nvm_write(ptr_v, G_gpg_vstate.work.io_buffer, G_gpg_vstate.io_length);
                 nvm_write(ptr_l, &G_gpg_vstate.io_length, sizeof(unsigned int));
             }

src/gpg_dispatch.c

@@ -261,6 +261,10 @@ int gpg_dispatch() {
     unsigned int tag, t, l;
     int sw = SWO_UNKNOWN;
 
+    if (G_gpg_vstate.ui_pending) {
+        return SWO_CONDITIONS_NOT_SATISFIED;
+    }
+
     if ((G_gpg_vstate.io_cla != CLA_APP_DEF) && (G_gpg_vstate.io_cla != CLA_APP_CHAIN) &&
         (G_gpg_vstate.io_cla != CLA_APP_APDU_PIN)) {
         return SWO_INVALID_CLA;

src/gpg_gen.c

@@ -187,19 +187,19 @@ static int gpg_read_rsa_kyey(gpg_key_t *keygpg) {
     gpg_io_mark();
     switch (ksz) {
         case 2048 / 8:
-            if (keygpg->priv_key.rsa2048.size == 0) {
+            if (keygpg->priv_key.rsa2048.size != ksz) {
                 return SWO_REFERENCED_DATA_NOT_FOUND;
             }
             gpg_io_insert_tlv(0x81, ksz, (unsigned char *) &keygpg->priv_key.rsa2048.n);
             break;
         case 3072 / 8:
-            if (keygpg->priv_key.rsa3072.size == 0) {
+            if (keygpg->priv_key.rsa3072.size != ksz) {
                 return SWO_REFERENCED_DATA_NOT_FOUND;
             }
             gpg_io_insert_tlv(0x81, ksz, (unsigned char *) &keygpg->priv_key.rsa3072.n);
             break;
         case 4096 / 8:
-            if (keygpg->priv_key.rsa4096.size == 0) {
+            if (keygpg->priv_key.rsa4096.size != ksz) {
                 return SWO_REFERENCED_DATA_NOT_FOUND;
             }
             gpg_io_insert_tlv(0x81, ksz, (unsigned char *) &keygpg->priv_key.rsa4096.n);

src/gpg_pin.c

@@ -222,12 +222,14 @@ int gpg_apdu_verify() {
             case PIN_MODE_SCREEN:
                 // Delegate pin check to ui
                 gpg_io_discard(1);
+                G_gpg_vstate.ux_pinconfirm_p2 = G_gpg_vstate.io_p2;
                 ui_menu_pinentry_display(0);
                 sw = 0;
                 break;
             case PIN_MODE_CONFIRM:
                 // Delegate pin check to ui
                 gpg_io_discard(1);
+                G_gpg_vstate.ux_pinconfirm_p2 = G_gpg_vstate.io_p2;
                 ui_menu_pinconfirm_display(G_gpg_vstate.io_p2);
                 sw = 0;
                 break;
@@ -301,6 +303,7 @@ int gpg_apdu_change_ref_data() {
     if (G_gpg_vstate.io_length == 0) {
         // Delegate pin change to ui
         gpg_io_discard(1);
+        G_gpg_vstate.ux_pinconfirm_p2 = G_gpg_vstate.io_p2;
         ui_menu_pinentry_display(0);
         return 0;
     }

src/gpg_types.h

@@ -47,6 +47,10 @@
 
 #define GPG_KEY_ATTRIBUTES_LENGTH 12
 
+#define GPG_NAME_LENGTH       39
+#define GPG_LANG_LENGTH       8
+#define GPG_SALUTATION_LENGTH 1
+
 #define GPG_RSA_DEFAULT_PUB 0x00010001U
 
 #ifndef CX_AES_128_KEY_LEN
@@ -150,11 +154,11 @@ struct gpg_nv_state_s {
 
     /* -- Cardholder Related Data -- */
     /*  5B */
-    LV(name, 39);
+    LV(name, GPG_NAME_LENGTH);
     /*  5F2D */
-    LV(lang, 8);
+    LV(lang, GPG_LANG_LENGTH);
     /*  5F35 */
-    unsigned char salutation[1];
+    unsigned char salutation[GPG_SALUTATION_LENGTH];
 
     /* -- Application  Related Data -- */
     /*  4F */
@@ -266,6 +270,8 @@ struct gpg_v_state_s {
     char menu[112];
     unsigned char ux_pinentry[GPG_MAX_PW_LENGTH];
     unsigned char ux_pinLen;
+    unsigned char ux_pinconfirm_p2;
+    unsigned char ui_pending;
     unsigned int ux_key;
     unsigned int ux_type;
 

src/gpg_ux_nbgl.c

@@ -1140,7 +1140,8 @@ static void ui_reset(void) {
  *
  */
 void pin_confirm_cb(bool confirm) {
-    gpg_pin_set_verified(G_gpg_vstate.io_p2, confirm);
+    G_gpg_vstate.ui_pending = 0;
+    gpg_pin_set_verified(G_gpg_vstate.ux_pinconfirm_p2, confirm);
 
     gpg_io_discard(0);
     gpg_io_insert_u16(confirm ? SWO_SUCCESS : SWO_CONDITIONS_NOT_SATISFIED);
@@ -1155,6 +1156,7 @@ void pin_confirm_cb(bool confirm) {
  *
  */
 void ui_menu_pinconfirm_display(unsigned int value) {
+    G_gpg_vstate.ui_pending = 1;
     snprintf(G_gpg_vstate.menu,
              sizeof(G_gpg_vstate.menu),
              "%s %x",
@@ -1179,10 +1181,11 @@ static void pinentry_validate_cb(const uint8_t *pinentry, uint8_t length) {
     unsigned char *pin1 = NULL;
     gpg_pin_t *pin = NULL;
 
+    G_gpg_vstate.ui_pending = 0;
     switch (G_gpg_vstate.io_ins) {
         case INS_VERIFY:
-            pin = gpg_pin_get_pin(G_gpg_vstate.io_p2);
-            sw = gpg_pin_check(pin, G_gpg_vstate.io_p2, pinentry, length);
+            pin = gpg_pin_get_pin(G_gpg_vstate.ux_pinconfirm_p2);
+            sw = gpg_pin_check(pin, G_gpg_vstate.ux_pinconfirm_p2, pinentry, length);
             gpg_io_discard(1);
             if (sw == SWO_AUTH_METHOD_BLOCKED) {
                 gpg_io_insert_u16(sw);
@@ -1202,16 +1205,16 @@ static void pinentry_validate_cb(const uint8_t *pinentry, uint8_t length) {
             snprintf(G_gpg_vstate.line,
                      sizeof(G_gpg_vstate.line),
                      "%s PIN",
-                     (G_gpg_vstate.io_p2 == PIN_ID_PW3) ? "ADMIN" : "USER");
+                     (G_gpg_vstate.ux_pinconfirm_p2 == PIN_ID_PW3) ? "ADMIN" : "USER");
             ui_info(G_gpg_vstate.line, "VERIFIED", ui_init, true);
             break;
 
         case INS_CHANGE_REFERENCE_DATA:
             switch (G_gpg_vstate.ux_step) {
                 case 0:
                     // Check Current pin code
-                    pin = gpg_pin_get_pin(G_gpg_vstate.io_p2);
-                    sw = gpg_pin_check(pin, G_gpg_vstate.io_p2, pinentry, length);
+                    pin = gpg_pin_get_pin(G_gpg_vstate.ux_pinconfirm_p2);
+                    sw = gpg_pin_check(pin, G_gpg_vstate.ux_pinconfirm_p2, pinentry, length);
                     gpg_io_discard(1);
                     if (sw == SWO_AUTH_METHOD_BLOCKED) {
                         gpg_io_insert_u16(sw);
@@ -1242,18 +1245,19 @@ static void pinentry_validate_cb(const uint8_t *pinentry, uint8_t length) {
                         gpg_io_discard(1);
                         ui_info(PIN_DIFFERS, EMPTY, ui_menu_pinentry_cb, false);
                     } else {
-                        pin = gpg_pin_get_pin(G_gpg_vstate.io_p2);
+                        pin = gpg_pin_get_pin(G_gpg_vstate.ux_pinconfirm_p2);
                         sw = gpg_pin_set(pin, G_gpg_vstate.work.io_buffer + 1, length);
                         gpg_io_discard(1);
                         gpg_io_insert_u16(sw);
                         gpg_io_do(IO_RETURN_AFTER_TX);
                         if (sw != SWO_SUCCESS) {
                             ui_info("Process Error", EMPTY, ui_init, false);
                         } else {
-                            snprintf(G_gpg_vstate.line,
-                                     sizeof(G_gpg_vstate.line),
-                                     "%s PIN",
-                                     (G_gpg_vstate.io_p2 == PIN_ID_PW3) ? "ADMIN" : "USER");
+                            snprintf(
+                                G_gpg_vstate.line,
+                                sizeof(G_gpg_vstate.line),
+                                "%s PIN",
+                                (G_gpg_vstate.ux_pinconfirm_p2 == PIN_ID_PW3) ? "ADMIN" : "USER");
                             ui_info(G_gpg_vstate.line, "CHANGED", ui_init, true);
                         }
                     }
@@ -1273,6 +1277,7 @@ static void pinentry_validate_cb(const uint8_t *pinentry, uint8_t length) {
  *
  */
 static void pinback_cb(void) {
+    G_gpg_vstate.ui_pending = 0;
     gpg_io_discard(0);
     gpg_io_insert_u16(SWO_CONDITIONS_NOT_SATISFIED);
     gpg_io_do(IO_RETURN_AFTER_TX);
@@ -1289,6 +1294,7 @@ void ui_menu_pinentry_display(unsigned int step) {
     uint8_t minLen;
     char line[10];
 
+    G_gpg_vstate.ui_pending = 1;
     // Init the page title
     explicit_bzero(G_gpg_vstate.line, sizeof(G_gpg_vstate.line));
     if (G_gpg_vstate.io_ins == INS_CHANGE_REFERENCE_DATA) {
@@ -1314,9 +1320,10 @@ void ui_menu_pinentry_display(unsigned int step) {
              sizeof(G_gpg_vstate.menu),
              "%s %s PIN",
              line,
-             (G_gpg_vstate.io_p2 == PIN_ID_PW3) ? "Admin" : "User");
+             (G_gpg_vstate.ux_pinconfirm_p2 == PIN_ID_PW3) ? "Admin" : "User");
 
-    minLen = (G_gpg_vstate.io_p2 == PIN_ID_PW3) ? GPG_MIN_PW3_LENGTH : GPG_MIN_PW1_LENGTH;
+    minLen =
+        (G_gpg_vstate.ux_pinconfirm_p2 == PIN_ID_PW3) ? GPG_MIN_PW3_LENGTH : GPG_MIN_PW1_LENGTH;
     // Draw the keypad
     nbgl_useCaseKeypad(G_gpg_vstate.menu,
                        minLen,