Audit findings (#6)

* fix legacy get version

* Documentation fixes

* update zxlib version

* update snapshots

* add bling signing

* update snapshots

* bump version

* Python venv for guidelines enforcer

* cache pk to avoid delay on display

---------

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

.github/workflows/guidelines_enforcer.yml

@@ -21,5 +21,5 @@ on:
 jobs:
   guidelines_enforcer:
     if: github.event.repository.private == false
-    name: Call Ledger guidelines_enforcer
+    name: Call Ledger guidelines enforcer
     uses: LedgerHQ/ledger-app-workflows/.github/workflows/reusable_guidelines_enforcer.yml@v1

Makefile

@@ -44,3 +44,7 @@ test_all:
 test_ledger_try:
 	make zemu_install
 	cd tests_zemu && yarn try
+
+test_ledger_try_legacy:
+	make zemu_install
+	cd tests_zemu && yarn try_legacy

app/Makefile

@@ -47,6 +47,7 @@ else
 endif
 
 # Add the PRODUCTION_BUILD definition to the compiler flags
+DEFINES += APP_BLINDSIGN_MODE_ENABLED
 DEFINES += PRODUCTION_BUILD=$(PRODUCTION_BUILD)
 
 include $(CURDIR)/../deps/ledger-zxlib/makefiles/Makefile.app_testing

app/Makefile.version

@@ -3,4 +3,4 @@ APPVERSION_M=0
 # This is the minor version
 APPVERSION_N=0
 # This is the patch version
-APPVERSION_P=4
+APPVERSION_P=5

app/src/apdu_handler.c

@@ -126,12 +126,17 @@ __Z_INLINE void handleSign(volatile uint32_t *flags, volatile uint32_t *tx, uint
         THROW(APDU_CODE_OK);
     }
 
-    const char *error_msg = tx_parse(tx_get_buffer_length(), get_tx_type());
+    uint8_t error_code;
+    const char *error_msg = tx_parse(tx_get_buffer_length(), get_tx_type(), &error_code);
     CHECK_APP_CANARY()
     if (error_msg != NULL) {
         const int error_msg_length = strnlen(error_msg, sizeof(G_io_apdu_buffer));
         memcpy(G_io_apdu_buffer, error_msg, error_msg_length);
         *tx += (error_msg_length);
+        if (error_code == parser_blindsign_mode_required) {
+            *flags |= IO_ASYNCH_REPLY;
+            view_blindsign_error_show();
+        }
         THROW(APDU_CODE_DATA_INVALID);
     }
 

app/src/apdu_handler_legacy.c

@@ -296,16 +296,11 @@ bool legacy_process_transfer_chunk(uint32_t rx) {
 }
 
 void legacy_handleGetVersion(volatile uint32_t *tx) {
-    G_io_apdu_buffer[1] = (LEDGER_MAJOR_VERSION >> 8) & 0xFF;
-    G_io_apdu_buffer[2] = (LEDGER_MAJOR_VERSION >> 0) & 0xFF;
+    G_io_apdu_buffer[0] = (LEDGER_MAJOR_VERSION >> 0) & 0xFF;
+    G_io_apdu_buffer[1] = (LEDGER_MINOR_VERSION >> 0) & 0xFF;
+    G_io_apdu_buffer[2] = (LEDGER_PATCH_VERSION >> 0) & 0xFF;
 
-    G_io_apdu_buffer[3] = (LEDGER_MINOR_VERSION >> 8) & 0xFF;
-    G_io_apdu_buffer[4] = (LEDGER_MINOR_VERSION >> 0) & 0xFF;
-
-    G_io_apdu_buffer[5] = (LEDGER_PATCH_VERSION >> 8) & 0xFF;
-    G_io_apdu_buffer[6] = (LEDGER_PATCH_VERSION >> 0) & 0xFF;
-
-    *tx += 6;
+    *tx += 3;
     THROW(APDU_CODE_OK);
 }
 
@@ -342,7 +337,7 @@ void legacy_handleSignTransaction(volatile uint32_t *flags, volatile uint32_t *t
 
     uint32_t buffer_length = legacy_check_request(tx);
 
-    const char *error_msg = tx_parse(buffer_length, tx_type_json);
+    const char *error_msg = tx_parse(buffer_length, tx_type_json, NULL);
     tx_type = tx_type_json;
     CHECK_APP_CANARY()
     if (error_msg != NULL) {
@@ -366,13 +361,18 @@ void legacy_handleSignHash(volatile uint32_t *flags, volatile uint32_t *tx, uint
 
     uint32_t buffer_length = legacy_check_request(tx);
 
-    const char *error_msg = tx_parse(buffer_length, tx_type_hash);
+    uint8_t error_code;
+    const char *error_msg = tx_parse(buffer_length, tx_type_hash, &error_code);
     tx_type = tx_type_hash;
     CHECK_APP_CANARY()
     if (error_msg != NULL) {
         const int error_msg_length = strnlen(error_msg, sizeof(G_io_apdu_buffer));
         memcpy(G_io_apdu_buffer, error_msg, error_msg_length);
         *tx += (error_msg_length);
+        if (error_code == parser_blindsign_mode_required) {
+            *flags |= IO_ASYNCH_REPLY;
+            view_blindsign_error_show();
+        }
         THROW(APDU_CODE_DATA_INVALID);
     }
 
@@ -393,7 +393,7 @@ void legacy_handleSignTransferTx(volatile uint32_t *flags, volatile uint32_t *tx
 
     uint32_t buffer_length = tx_get_buffer_length();
 
-    const char *error_msg = tx_parse(buffer_length, tx_type_transfer);
+    const char *error_msg = tx_parse(buffer_length, tx_type_transfer, NULL);
     tx_type = tx_type_transfer;
     CHECK_APP_CANARY()
     if (error_msg != NULL) {

app/src/common/parser_common.h

@@ -56,6 +56,7 @@ typedef enum {
     parser_missing_field,
     paser_unknown_transaction,
     parser_tx_obj_empty,
+    parser_blindsign_mode_required,
 
     // Coin Specific
     parser_json_zero_tokens,

app/src/common/tx.c

@@ -84,8 +84,11 @@ uint32_t tx_get_buffer_length() { return buffering_get_buffer()->pos; }
 
 uint8_t *tx_get_buffer() { return buffering_get_buffer()->data; }
 
-const char *tx_parse(uint32_t buffer_length, tx_type_t tx_type_parse) {
+const char *tx_parse(uint32_t buffer_length, tx_type_t tx_type_parse, uint8_t *error_code) {
     uint8_t err = parser_parse(&ctx_parsed_tx, tx_get_buffer(), buffer_length, tx_type_parse);
+    if (error_code != NULL) {
+        *error_code = err;
+    }
 
     CHECK_APP_CANARY()
 

app/src/common/tx.h

@@ -65,7 +65,7 @@ uint8_t *tx_get_buffer();
 /// Parse message stored in transaction buffer
 /// This function should be called as soon as full buffer data is loaded.
 /// \return It returns NULL if data is valid or error message otherwise.
-const char *tx_parse(uint32_t buffer_length, tx_type_t tx_type_parse);
+const char *tx_parse(uint32_t buffer_length, tx_type_t tx_type_parse, uint8_t *error_code);
 
 /// Return the number of items in the transaction
 zxerr_t tx_getNumItems(uint8_t *num_items);

app/src/crypto.c

@@ -103,6 +103,9 @@ zxerr_t crypto_sign(uint8_t *signature, uint16_t signatureMaxlen, const uint8_t
 }
 
 zxerr_t crypto_fillAddress(uint8_t *buffer, uint16_t bufferLen, uint16_t *addrResponseLen) {
+    static uint8_t address[65];
+    static uint8_t is_computed = 0;
+
     if (buffer == NULL || addrResponseLen == NULL) {
         return zxerr_out_of_bounds;
     }
@@ -114,7 +117,14 @@ zxerr_t crypto_fillAddress(uint8_t *buffer, uint16_t bufferLen, uint16_t *addrRe
     }
 
     *addrResponseLen = 0;
-    CHECK_ZXERR(crypto_extractPublicKey(buffer, PUB_KEY_LENGTH))
+
+    if (!is_computed) {
+        CHECK_ZXERR(crypto_extractPublicKey(address, sizeof(address)))
+        is_computed = 1;
+    }
+
+    MEMCPY(buffer, address, PUB_KEY_LENGTH);
+
     *addrResponseLen = PUB_KEY_LENGTH;
 
     return zxerr_ok;

app/src/parser.c

@@ -52,8 +52,8 @@ parser_error_t parser_init_context(parser_context_t *ctx, const uint8_t *buffer,
 }
 
 parser_error_t parser_parse(parser_context_t *ctx, const uint8_t *data, size_t dataLen, tx_type_t tx_type) {
-    if (tx_type == tx_type_hash && !app_mode_expert()) {
-        return parser_expert_mode_required;
+    if (tx_type == tx_type_hash && !app_mode_blindsign()) {
+        return parser_blindsign_mode_required;
     }
 
     CHECK_ERROR(parser_init_context(ctx, data, dataLen))

app/src/parser_impl.c

@@ -485,6 +485,8 @@ const char *parser_getErrorDescription(parser_error_t err) {
             return "display page out of range";
         case parser_tx_obj_empty:
             return "Tx obj empty";
+        case parser_blindsign_mode_required:
+            return "Blind signing mode required";
         case parser_unexpected_value:
             return "Unexpected value";
         case parser_json_too_many_tokens:

docs/APDUSPEC.md

@@ -48,6 +48,33 @@ Some commands contain two different possible INS values.
 Such implementation is to allow for backwards compatibility with the original Kadena App.
 See [Legacy Command definition](#legacy-command-definition) for more details.
 
+### GET_DEVICE_INFO
+
+#### Command
+
+| Field | Type     | Content                | Expected |
+| ----- | -------- | ---------------------- | -------- |
+| CLA   | byte (1) | Application Identifier | 0xE0     |
+| INS   | byte (1) | Instruction ID         | 0x01     |
+| P1    | byte (1) | Parameter 1            | 0x00     |
+| P2    | byte (1) | Parameter 2            | 0x00     |
+| L     | byte (1) | Bytes in payload       | 0x00     |
+
+#### Response
+
+| Field     | Type     | Content            | Note                     |
+| --------- | -------- | ------------------ | ------------------------ |
+| TARGET_ID | byte (4) | Target Id          |                          |
+| OS_LEN    | byte (1) | OS version length  | 0..64                    |
+| OS        | byte (?) | OS version         | Non terminated string    |
+| FLAGS_LEN | byte (1) | Flags length       | 0                        |
+| MCU_LEN   | byte (1) | MCU version length | 0..64                    |
+| MCU       | byte (?) | MCU version        | Non terminated string    |
+| SW1-SW2   | byte (2) | Return code        | see list of return codes |
+
+---
+
+
 ### GET_VERSION
 
 #### Command
@@ -82,9 +109,9 @@ See [Legacy Command definition](#legacy-command-definition) for more details.
 | ------- | -------- | ------------------------- | -------------------------- |
 | CLA     | byte (1) | Application Identifier    | 0x00                       |
 | INS     | byte (1) | Instruction ID            | 0x21                       |
-| P1      | byte (1) | Request User confirmation | No = 0  / Yes = 1          |
+| P1      | byte (1) | Request User confirmation | No = 0  / Yes = Any Other  |
 | P2      | byte (1) | Parameter 2               | ignored                    |
-| L       | byte (1) | Bytes in payload          | 25                         |
+| L       | byte (1) | Bytes in payload          | 20                         |
 | Path[0] | byte (4) | Derivation Path Data      | 0x80000000 \| 44           |
 | Path[1] | byte (4) | Derivation Path Data      | 0x80000000 \| 626          |
 | Path[2] | byte (4) | Derivation Path Data      | ?                          |
@@ -104,13 +131,13 @@ See [Legacy Command definition](#legacy-command-definition) for more details.
 
 #### Command
 
-| Field | Type     | Content                | Expected                   |
-| ----- | -------- | ---------------------- | -------------------------- |
-| CLA   | byte (1) | Application Identifier | 0x00                       |
-| INS   | byte (1) | Instruction ID         | 0x22                       |
-| P1    | byte (1) | ----                   | not used                   |
-| P2    | byte (1) | ----                   | not used                   |
-| L     | byte (1) | Bytes in payload       | (depends)                  |
+| Field | Type     | Content                | Expected                                                              |
+| ----- | -------- | ---------------------- | --------------------------------------------------------------------- |
+| CLA   | byte (1) | Application Identifier | 0x00                                                                  |
+| INS   | byte (1) | Instruction ID         | 0x22                                                                  |
+| P1    | byte (1) | ----                   | First packet = 0x00 / More packets coming = 0x01 / Last packet = 0x02 |
+| P2    | byte (1) | ----                   | not used                                                              |
+| L     | byte (1) | Bytes in payload       | (depends)                                                             |
 
 For the new app, the first packet/chunk includes only the derivation path.
 
@@ -145,13 +172,13 @@ All other packets/chunks contain data chunks that are described below.
 
 #### Command
 
-| Field | Type     | Content                | Expected                   |
-| ----- | -------- | ---------------------- | -------------------------- |
-| CLA   | byte (1) | Application Identifier | 0x00                       |
-| INS   | byte (1) | Instruction ID         | 0x23                       |
-| P1    | byte (1) | ----                   | not used                   |
-| P2    | byte (1) | ----                   | not used                   |
-| L     | byte (1) | Bytes in payload       | (depends)                  |
+| Field | Type     | Content                | Expected                                                              |
+| ----- | -------- | ---------------------- | --------------------------------------------------------------------- |
+| CLA   | byte (1) | Application Identifier | 0x00                                                                  |
+| INS   | byte (1) | Instruction ID         | 0x23                                                                  |
+| P1    | byte (1) | ----                   | First packet = 0x00 / More packets coming = 0x01 / Last packet = 0x02 |
+| P2    | byte (1) | ----                   | not used                                                              |
+| L     | byte (1) | Bytes in payload       | (depends)                                                             |
 
 For the new app, the first packet/chunk includes only the derivation path
 
@@ -182,17 +209,17 @@ All other packets/chunks contain data chunks that are described below
 
 ---
 
-### INS_SING_TRANSFER
+### INS_SIGN_TRANSFER
 
 #### Command
 
-| Field | Type     | Content                | Expected                   |
-| ----- | -------- | ---------------------- | -------------------------- |
-| CLA   | byte (1) | Application Identifier | 0x00                       |
-| INS   | byte (1) | Instruction ID         | 0x24                       |
-| P1    | byte (1) | ----                   | not used                   |
-| P2    | byte (1) | ----                   | not used                   |
-| L     | byte (1) | Bytes in payload       | (depends)                  |
+| Field | Type     | Content                | Expected                                                              |
+| ----- | -------- | ---------------------- | --------------------------------------------------------------------- |
+| CLA   | byte (1) | Application Identifier | 0x00                                                                  |
+| INS   | byte (1) | Instruction ID         | 0x24                                                                  |
+| P1    | byte (1) | ----                   | First packet = 0x00 / More packets coming = 0x01 / Last packet = 0x02 |
+| P2    | byte (1) | ----                   | not used                                                              |
+| L     | byte (1) | Bytes in payload       | (depends)                                                             |
 
 For the new app, the first packet/chunk includes only the derivation path
 
@@ -273,9 +300,9 @@ All other packets/chunks contain data chunks that are described below
 
 | Field      | Type     | Content          | Note                            |
 | ---------- | -------- | ---------------- | ------------------------------- |
-| MAJOR      | byte (2) | Version Major    | 0..65535                        |
-| MINOR      | byte (2) | Version Minor    | 0..65535                        |
-| PATCH      | byte (2) | Version Patch    | 0..65535                        |
+| MAJOR      | byte (1) | Version Major    | 0..255                          |
+| MINOR      | byte (1) | Version Minor    | 0..255                          |
+| PATCH      | byte (1) | Version Patch    | 0..255                          |
 | SW1-SW2    | byte (2) | Return code      | see list of return codes        |
 
 ---
@@ -292,6 +319,7 @@ Same as [BCOMP_GET_PUBKEY](#bcomp_get_pubkey) but requires user confirmation.
 | INS       | byte (1) | Instruction ID             | 0x01                       |
 | P1        | byte (1) | Parameter 1                | ignored                    |
 | P2        | byte (1) | Parameter 2                | ignored                    |
+| L         | byte (1) | Bytes in payload           | (depends)                  |
 | N         | byte (1) | Number of derivation steps | (depends)                  |
 | Path[0]   | byte (4) | Derivation Path Data       | 0x80000000 \| 44           |
 | Path[1]   | byte (4) | Derivation Path Data       | 0x80000000 \| 626          |
@@ -318,6 +346,7 @@ Same as [BCOMP_GET_PUBKEY](#bcomp_get_pubkey) but requires user confirmation.
 | INS       | byte (1) | Instruction ID             | 0x02                       |
 | P1        | byte (1) | Parameter 1                | ignored                    |
 | P2        | byte (1) | Parameter 2                | ignored                    |
+| L         | byte (1) | Bytes in payload           | (depends)                  |
 | N         | byte (1) | Number of derivation steps | (depends)                  |
 | Path[0]   | byte (4) | Derivation Path Data       | 0x80000000 \| 44           |
 | Path[1]   | byte (4) | Derivation Path Data       | 0x80000000 \| 626          |
@@ -382,6 +411,7 @@ Sign a transaction hash using the key for the specified derivation path. Expert
 | INS   | byte (1) | Instruction ID         | 0x04                       |
 | P1    | byte (1) | ----                   | not used                   |
 | P2    | byte (1) | ----                   | not used                   |
+| L     | byte (1) | Bytes in payload       | (depends)                  |
 
 ##### Input data
 
@@ -416,7 +446,7 @@ Builds a transfer transaction using the input data, and provides a signature for
 | INS   | byte (1) | Instruction ID         | 0x10                       |
 | P1    | byte (1) | ----                   | not used                   |
 | P2    | byte (1) | ----                   | not used                   |
-
+| L     | byte (1) | Bytes in payload       | (depends)                  |
 ##### Input data
 
 | Field               | Type                        | Content                     | Expected            |

tests_zemu/package.json

@@ -26,21 +26,21 @@
   },
   "dependencies": {
     "@zondax/ledger-kadena": "v0.0.3",
-    "@zondax/zemu": "^0.50.3"
+    "@zondax/zemu": "^0.51.0"
   },
   "devDependencies": {
     "@ledgerhq/hw-transport-node-hid": "^6.29.5",
     "@noble/curves": "^1.6.0",
     "@types/jest": "^29.5.13",
     "@types/ledgerhq__hw-transport": "^4.21.8",
-    "@typescript-eslint/eslint-plugin": "^8.7.0",
-    "@typescript-eslint/parser": "^8.7.0",
+    "@typescript-eslint/eslint-plugin": "^8.8.1",
+    "@typescript-eslint/parser": "^8.8.1",
     "blakejs": "^1.2.1",
     "crypto-js": "4.2.0",
     "ed25519-supercop": "^2.0.1",
-    "eslint": "^9.11.1",
+    "eslint": "^9.12.0",
     "eslint-config-prettier": "^9.1.0",
-    "eslint-plugin-import": "^2.30.0",
+    "eslint-plugin-import": "^2.31.0",
     "eslint-plugin-jest": "^28.8.3",
     "eslint-plugin-prettier": "^5.2.1",
     "eslint-plugin-promise": "^7.1.0",
@@ -53,6 +53,6 @@
     "sort-package-json": "^2.10.1",
     "ts-jest": "^29.2.5",
     "ts-node": "^10.9.2",
-    "typescript": "^5.6.2"
+    "typescript": "^5.6.3"
   }
 }

tests_zemu/tests/legacy.test.ts

@@ -179,13 +179,15 @@ describe.each(HASH_TEST_CASES)('Hash transactions', function (data) {
 
       const { publicKey } = await app.getPublicKey(data.path)
 
-      await sim.toggleExpertMode()
+      // Enable blind signing mode
+      await sim.toggleBlindSigning()
+
       // do not wait here... we need to navigate
       const signatureRequest = app.signHash(data.path, data.hash)
 
       // Wait until we are not in the main menu
       await sim.waitUntilScreenIsNot(sim.getMainMenuSnapshot())
-      await sim.compareSnapshotsAndApprove('.', `${m.prefix.toLowerCase()}-sign_${data.name}_legacy`)
+      await sim.compareSnapshotsAndApprove('.', `${m.prefix.toLowerCase()}-sign_${data.name}_legacy`, true, 0, 15000, true)
 
       const signatureResponse = await signatureRequest