Merge pull request #100 from LedgerHQ/cev/fix_app_secu

Fix App following security Audit

Makefile

@@ -29,7 +29,7 @@ APPNAME = OpenPGP
 
 # Application version
 APPVERSION_M = 2
-APPVERSION_N = 1
+APPVERSION_N = 2
 APPVERSION_P = 0
 APPVERSION = "$(APPVERSION_M).$(APPVERSION_N).$(APPVERSION_P)"
 

doc/user/app-openpgp.rst

@@ -1562,6 +1562,7 @@ The tool usage is the following:
  |  ``--user-pin PIN   User PIN (if pinpad not used)``
  |  ``--restore        Perform a Restore instead of Backup``
  |  ``--file FILE      Backup/Restore file (default is 'gpg_backup')``
+ |  ``--seed-key       After Restore, regenerate all keys, based on seed mode``
  |
  | ``Keys restore is only possible with SEED mode...``
 
@@ -1571,12 +1572,11 @@ To perform a backup, simply use the tool like this:
  | ``Connect to card 'Ledger'...``
  | ``Configuration saved in file 'gpg_backup'.``
 
-Once the configuration is restored, just use the previous tool to re-generate the seeded keys:
+To *restore* a backup, simply use the tool like this:
 
- | ``./gpgcli.py --user-pin 123456 --adm-pin 12345678 --seed-key``
+ | ``$ ./backup.py --restore --adm-pin 12345678 --user-pin 123456 --seed-key``
  | ``Connect to card 'Ledger'...``
- | ``Verify PINs...``
- | ``Get card info...``
+ | ``Configuration saved in file 'gpg_backup'.``
 
 Annexes
 =======

manual-tests/manual.sh

@@ -62,6 +62,16 @@ init() {
     echo enable-pinpad-varlen
     echo card-timeout 1
   } > "${dir}/scdaemon.conf"
+
+  if [[ ${EXPERT} == true ]]; then
+    {
+      echo log-file /tmp/scd.log
+      echo debug-level guru
+      echo debug-all
+    } >> "${dir}/scdaemon.conf"
+  fi
+
+  gpgconf --reload scdaemon
 }
 
 #===============================================================================

pytools/backup.py

@@ -34,7 +34,7 @@ def get_argparser() -> Namespace:
         formatter_class=RawTextHelpFormatter
     )
     parser.add_argument("--reader", type=str, default="Ledger",
-                        help="PCSC reader name (default is '%(default)s')")
+                        help="PCSC reader name (default is '%(default)s') or 'speculos'")
 
     parser.add_argument("--slot", type=int, choices=range(1, 4), help="Select slot (1 to 3)")
 
@@ -51,6 +51,9 @@ def get_argparser() -> Namespace:
     parser.add_argument("--file", type=str, default="gpg_backup",
                         help="Backup/Restore file (default is '%(default)s')")
 
+    parser.add_argument("--seed-key", action="store_true",
+                        help="After Restore, regenerate all keys, based on seed mode")
+
     return parser.parse_args()
 
 
@@ -95,6 +98,10 @@ def entrypoint() -> None:
         if args.restore:
             gpgcard.restore(args.file)
             print(f"Configuration restored from file '{args.file}'.")
+
+            if args.seed_key:
+                gpgcard.seed_key()
+
         else:
             gpgcard.backup(args.file)
             print(f"Configuration saved in file '{args.file}'.")

pytools/gpgapp/gpgcard.py

@@ -24,14 +24,12 @@
 from dataclasses import dataclass
 from Crypto.PublicKey.RSA import construct
 
-# pylint: disable=import-error
-from smartcard.System import readers  # type: ignore
-from smartcard.pcsc import PCSCReader  # type: ignore
-from smartcard import CardConnectionDecorator  # type: ignore
-# pylint: enable=import-error
 from gpgapp.gpgcmd import DataObject, ErrorCodes, KeyTypes, PassWord, PubkeyAlgo  # type: ignore
 from gpgapp.gpgcmd import KEY_OPERATIONS, KEY_TEMPLATES, USER_SALUTATION  # type: ignore
 
+# pylint: disable=import-error
+from ledgercomm import Transport  # type: ignore
+# pylint: enable=import-error
 
 APDU_MAX_SIZE: int = 0xFE
 APDU_CHAINING_MODE: int = 0x10
@@ -143,7 +141,7 @@ def reset(self):
 class GPGCard() :
     def __init__(self) -> None:
         self.log: bool = False
-        self.connection: CardConnectionDecorator = None
+        self.transport: Transport = None
         self.slot_current: bytes = b"\x00"
         self.slot_config: bytes = bytes(3)
         self.data: CardInfo = CardInfo()
@@ -156,21 +154,17 @@ def connect(self, device: str) -> None:
             device (str): Reader device name
         """
 
-        allreaders: list = readers()
-        for elt in allreaders:
-            if str(elt).startswith(device):
-                reader: PCSCReader.PCSCReader = elt
-                self.connection = reader.createConnection()
-                self.connection.connect()
-                return
+        if device == "speculos":
+            self.transport = Transport("tcp", server="127.0.0.1", port=9999, debug=False)
+        else:
+            self.transport = Transport("hid")
         print("")
-        raise GPGCardExcpetion(ErrorCodes.ERR_INTERNAL, "No Reader detected!")
 
 
     def disconnect(self):
         """Connect from the selected Reader"""
 
-        return self.connection.disconnect()
+        self.transport.close()
 
 
     ############### LOG interface ###############
@@ -1236,8 +1230,9 @@ def _transmit(self, data: bytes, long_resp: bool = False) -> Tuple[bytes, int, i
         """
 
         self.add_log("send", data)
-        resp, sw1, sw2 = self.connection.transmit(list(data))
-        sw = (sw1 << 8) | sw2
+        sw, resp = self.transport.exchange_raw(data)
+        sw1 = (sw >> 8) & 0xFF
+        sw2 = sw & 0xFF
         self.add_log("recv", resp, sw)
         if sw != ErrorCodes.ERR_SUCCESS and not long_resp:
             raise GPGCardExcpetion(sw, "")

pytools/gpgcli.py

@@ -38,7 +38,7 @@ def get_argparser() -> Namespace:
     parser.add_argument("--info", action="store_true",
                         help="Get and display card information")
     parser.add_argument("--reader", type=str, default="Ledger",
-                        help="PCSC reader name (default is '%(default)s')")
+                        help="PCSC reader name (default is '%(default)s') or 'speculos'")
 
     parser.add_argument("--apdu", action="store_true", help="Log APDU exchange")
     parser.add_argument("--slot", type=int, choices=range(1, 4), help="Select slot (1 to 3)")

pytools/requirements.txt

@@ -1,2 +1,2 @@
-pyscard
 pycryptodome
+ledgercomm

src/gpg_challenge.c

@@ -43,7 +43,7 @@ int gpg_apdu_get_challenge() {
         unsigned int path[2];
         unsigned char chain[32] = {0};
 
-        memset(chain, 0, 32);
+        explicit_bzero(chain, 32);
         path[0] = 0x80475047;
         path[1] = 0x0F0F0F0F;
         CX_CHECK(os_derive_bip32_no_throw(CX_CURVE_SECP256K1, path, 2, Sr, chain));

src/gpg_data.c

@@ -95,8 +95,8 @@ int gpg_apdu_get_data(unsigned int ref) {
             break;
         case 0x5F50:
             /* Uniform resource locator */
-            gpg_io_insert((const unsigned char *) N_gpg_pstate->url.value,
-                          N_gpg_pstate->url.length);
+            gpg_io_insert((const unsigned char *) N_gpg_pstate->keys[G_gpg_vstate.slot].url.value,
+                          N_gpg_pstate->keys[G_gpg_vstate.slot].url.length);
             break;
         case 0x65:
             /* Name, Language, salutation */
@@ -111,7 +111,7 @@ int gpg_apdu_get_data(unsigned int ref) {
 
             /* ----------------- aid, histo, ext_length, ... ----------------- */
         case 0x6E:
-            gpg_io_insert_tlv(0x4F, 16, (const unsigned char *) N_gpg_pstate->AID);
+            gpg_io_insert_tlv(0x4F, AID_LENGTH, (const unsigned char *) N_gpg_pstate->AID);
             memmove(G_gpg_vstate.work.io_buffer + G_gpg_vstate.io_offset - 6,
                     G_gpg_vstate.kslot->serial,
                     4);
@@ -379,7 +379,6 @@ int gpg_apdu_put_data(unsigned int ref) {
                     case 0x93:
                         len_q = l;
                         break;
-                        break;
                     case 0x94:
                     case 0x95:
                     case 0x96:
@@ -460,9 +459,9 @@ int gpg_apdu_put_data(unsigned int ref) {
                 p = G_gpg_vstate.work.io_buffer + G_gpg_vstate.io_offset;
                 q = p + len_p;
                 memmove(pq + ksz - len_p, p, len_p);
-                memset(pq, 0, ksz - len_p);
+                explicit_bzero(pq, ksz - len_p);
                 memmove(pq + 2 * ksz - len_q, q, len_q);
-                memset(pq + ksz, 0, ksz - len_q);
+                explicit_bzero(pq + ksz, ksz - len_q);
 
                 // regenerate RSA private key
                 unsigned char _e[4];
@@ -572,14 +571,14 @@ int gpg_apdu_put_data(unsigned int ref) {
             break;
             /* Uniform resource locator */
         case 0x5F50:
-            if (G_gpg_vstate.io_length > sizeof(N_gpg_pstate->url.value)) {
+            if (G_gpg_vstate.io_length > sizeof(N_gpg_pstate->keys[G_gpg_vstate.slot].url.value)) {
                 sw = SW_WRONG_LENGTH;
                 break;
             }
-            nvm_write((void *) N_gpg_pstate->url.value,
+            nvm_write((void *) N_gpg_pstate->keys[G_gpg_vstate.slot].url.value,
                       G_gpg_vstate.work.io_buffer,
                       G_gpg_vstate.io_length);
-            nvm_write((void *) &N_gpg_pstate->url.length,
+            nvm_write((void *) &N_gpg_pstate->keys[G_gpg_vstate.slot].url.length,
                       &G_gpg_vstate.io_length,
                       sizeof(unsigned int));
             sw = SW_OK;
@@ -743,7 +742,7 @@ int gpg_apdu_put_data(unsigned int ref) {
                                               G_gpg_vstate.io_length,
                                               &aes_key));
             nvm_write((void *) &N_gpg_pstate->SM_enc, &aes_key, sizeof(cx_aes_key_t));
-            CX_CHECK(cx_aes_init_key_no_throw(G_gpg_vstate.work.io_buffer + 16,
+            CX_CHECK(cx_aes_init_key_no_throw(G_gpg_vstate.work.io_buffer + CX_AES_128_KEY_LEN,
                                               G_gpg_vstate.io_length,
                                               &aes_key));
             nvm_write((void *) &N_gpg_pstate->SM_mac, &aes_key, sizeof(cx_aes_key_t));
@@ -809,11 +808,11 @@ static int gpg_init_keyenc(cx_aes_key_t *keyenc) {
     if (sw != SW_OK) {
         return sw;
     }
-    sw = gpg_pso_derive_key_seed(seed, (unsigned char *) PIC("key "), 1, seed, 16);
+    sw = gpg_pso_derive_key_seed(seed, (unsigned char *) PIC("key "), 1, seed, CX_AES_BLOCK_SIZE);
     if (sw != SW_OK) {
         return sw;
     }
-    CX_CHECK(cx_aes_init_key_no_throw(seed, 16, keyenc));
+    CX_CHECK(cx_aes_init_key_no_throw(seed, CX_AES_BLOCK_SIZE, keyenc));
 
 end:
     if (error != CX_OK) {
@@ -1007,12 +1006,7 @@ int gpg_apdu_put_key_data(unsigned int ref) {
                 sw = SW_CONDITIONS_NOT_SATISFIED;
                 break;
             }
-            if (len != GPG_IO_BUFFER_LENGTH) {
-                sw = SW_CONDITIONS_NOT_SATISFIED;
-                break;
-            }
 
-            PRINTF("[DATA] - put_key_data: key len: %d\n", len);
             gpg_io_discard(0);
             CX_CHECK(cx_aes_no_throw(&keyenc,
                                      CX_DECRYPT | CX_CHAIN_CBC | CX_PAD_ISO9797M2 | CX_LAST,
@@ -1021,7 +1015,6 @@ int gpg_apdu_put_key_data(unsigned int ref) {
                                      G_gpg_vstate.work.io_buffer,
                                      &ksz));
             if (len != ksz) {
-                PRINTF("[DATA] - put_key_data: Wrong aes output len: %d / %d\n", len, ksz);
                 sw = SW_WRONG_DATA;
                 break;
             }

src/gpg_dispatch.c

@@ -242,14 +242,13 @@ int gpg_dispatch() {
 #ifdef GPG_LOG
         case INS_GET_LOG:
             gpg_io_discard(1);
-            gpg_io_insert(G_gpg_vstate.log_buffer, 32);
+            gpg_io_insert(G_gpg_vstate.log_buffer, sizeof(G_gpg_vstate.log_buffer));
             return SW_OK;
 #endif
 
             /* --- SELECT --- */
         case INS_SELECT:
             return gpg_apdu_select();
-            break;
 
             /* --- ACTIVATE/TERMINATE FILE --- */
         case INS_ACTIVATE_FILE:
@@ -258,17 +257,14 @@ int gpg_dispatch() {
                 gpg_install(STATE_ACTIVATE);
             }
             return SW_OK;
-            break;
 
         case INS_TERMINATE_DF:
             gpg_io_discard(0);
             if (gpg_pin_is_verified(PIN_ID_PW3) || (N_gpg_pstate->PW3.counter == 0)) {
                 gpg_install(STATE_TERMINATE);
                 return SW_OK;
-                break;
             }
             return SW_CONDITIONS_NOT_SATISFIED;
-            break;
     }
 
     /* Other commands allowed if not terminated */
@@ -283,12 +279,6 @@ int gpg_dispatch() {
     }
 
     switch (G_gpg_vstate.io_ins) {
-#ifdef GPG_DEBUG_APDU
-        case 0x42:
-            sw = debug_apdu();
-            break;
-#endif
-
         case INS_EXIT:
             os_sched_exit(0);
             sw = SW_OK;
@@ -308,13 +298,11 @@ int gpg_dispatch() {
             }
             gpg_io_fetch_tl(&t, &l);
             if (t != 0x60) {
-                // TODO add l check
                 sw = SW_WRONG_DATA;
                 break;
             }
             gpg_io_fetch_tl(&t, &l);
             if (t != 0x5C) {
-                // TODO add l check
                 sw = SW_WRONG_DATA;
                 break;
             }

src/gpg_gen.c

@@ -27,7 +27,7 @@ int gpg_pso_derive_slot_seed(int slot, unsigned char *seed) {
     unsigned char chain[32];
     cx_err_t error = CX_INTERNAL_ERROR;
 
-    memset(chain, 0, 32);
+    explicit_bzero(chain, 32);
     path[0] = 0x80475047;
     path[1] = slot + 1;
     CX_CHECK(os_derive_bip32_no_throw(CX_CURVE_SECP256K1, path, 2, seed, chain));
@@ -219,7 +219,7 @@ static int gpg_gen_ecc_kyey(gpg_key_t *keygpg, uint8_t *name) {
 
     nvm_write(&G_gpg_vstate.kslot->sig_count, &reset_cnt, sizeof(unsigned int));
     gpg_io_clear();
-    return SW_OK;
+    error = SW_OK;
 
 end:
     return error;
@@ -256,7 +256,7 @@ static int gpg_read_ecc_kyey(gpg_key_t *keygpg) {
                           keygpg->pub_key.ecfp.W_len,
                           (unsigned char *) &keygpg->pub_key.ecfp.W);
     }
-    return SW_OK;
+    error = SW_OK;
 
 end:
     return error;