Merge pull request #15 from qtumproject/time/audit_report_fix

Add fixes after audit report

Makefile

@@ -21,6 +21,8 @@ endif
 
 include $(BOLOS_SDK)/Makefile.defines
 
+ENABLE_PENDING_REVIEW_SCREEN = 1
+
 # TODO: compile with the right path restrictions
 
 # Application allowed derivation curves.
@@ -33,7 +35,7 @@ APP_LOAD_PARAMS += --path_slip21 "LEDGER-Wallet policy"
 # Application version
 APPVERSION_M = 2
 APPVERSION_N = 1
-APPVERSION_P = 5
+APPVERSION_P = 6
 APPVERSION   = "$(APPVERSION_M).$(APPVERSION_N).$(APPVERSION_P)"
 
 APP_STACK_SIZE = 3072

doc/qtum.md

@@ -20,7 +20,7 @@ The main commands use `CLA = 0xE1`, unlike the legacy Qtum application that used
 |  E1 |  04 | SIGN_PSBT              | Sign a PSBT with a registered or default wallet |
 |  E1 |  05 | GET_MASTER_FINGERPRINT | Return the fingerprint of the master public key |
 |  E1 |  10 | SIGN_MESSAGE           | Sign a message with a key from a BIP32 path (Qtum Message Signing) |
-|  E1 |  81 | SIGN_SENDER_PSBT       | Sign an op_sender output |
+|  E1 |  81 | SIGN_SENDER_PSBT       | Sign a contract sender output (op_sender) |
 
 The `CLA = 0xF8` is used for framework-specific (rather than app-specific) APDUs; at this time, only one command is present.
 
@@ -330,6 +330,67 @@ The digest being signed is the double-SHA256 of the message, after prefixing the
 
 The client must respond to the `GET_PREIMAGE`, `GET_MERKLE_LEAF_PROOF` and `GET_MERKLE_LEAF_INDEX` queries for the Merkle tree of the list of chunks in the message.
 
+### SIGN_SENDER_PSBT
+
+Given a PSBTv2 and a registered wallet (or a standard one), sign a contract sender output using a wallet address.
+
+#### Encoding
+
+**Command**
+
+| *CLA* | *INS* |
+|-------|-------|
+| E1    | 81    |
+
+**Input data**
+
+| Length  | Name                   | Description |
+|---------|------------------------|-------------|
+| `1`     | `n`                    | Number of derivation steps (maximum 8) |
+| `4`     | `bip32_path[0]`        | First derivation step (big endian) |
+| `4`     | `bip32_path[1]`        | Second derivation step (big endian) |
+|         | ...                    |             |
+| `4`     | `bip32_path[n-1]`      | `n`-th derivation step (big endian) |
+| `<var>` | `global_map_size`      | The number of key/value pairs of the global map of the psbt |
+| `32`    | `global_map_keys_root` | The Merkle root of the keys of the global map |
+| `32`    | `global_map_vals_root` | The Merkle root of the values of the global map |
+| `<var>` | `n_inputs`             | The number of inputs of the psbt | 
+| `32`    | `inputs_maps_root`     | The Merkle root of the vector of Merkleized map commitments for the input maps |
+| `<var>` | `n_outputs`            | The number of outputs of the psbt | 
+| `32`    | `outputs_maps_root`    | The Merkle root of the vector of Merkleized map commitments for the output maps |
+| `32`    | `wallet_id`            | The id of the wallet |
+| `32`    | `wallet_hmac`          | The hmac of a registered wallet, or exactly 32 0 bytes |
+
+**Output data**
+
+The signature is returned using the YIELD client command.
+
+#### Description
+
+Using the information in the PSBT and the wallet description, this command verifies what inputs are internal and what outputs match the pattern for a change address. After validating all the external outputs and the transaction fee with the user, it sign the contract sender output with the `bip32_path` address; the signature is sent to the client using the YIELD command, in the format described below.
+
+The results yielded via the YIELD command respect the following format: `<output_index> <pubkey_augm_len> <pubkey_augm> <signature>`, where:
+- `output_index` is a Qtum style varint, the index of the output being signed (starting from 0);
+- `pubkey_augm_len` is an unsigned byte equal to the length of `pubkey_augm`;
+- `pubkey_augm` is the legacy `pubkey` used for signing;
+- `signature` is the returned signature, possibly concatenated with the sighash byte (as it would be pushed on the stack).
+
+If `P2` is `0` (version `0` of the protocol), `pubkey_augm_len` and `pubkey_augm` are omitted in the YIELD messages.
+
+For a registered wallet, the hmac must be correct.
+
+For a default wallet, `hmac` must be equal to 32 bytes `0`.
+
+#### Client commands
+
+`GET_PREIMAGE` must know and respond for the full serialized wallet policy whose sha256 hash is `wallet_id`; moreover, it must know and respond for the sha256 hash of its descriptor template.
+
+The client must respond to the `GET_PREIMAGE`, `GET_MERKLE_LEAF_PROOF` and `GET_MERKLE_LEAF_INDEX` queries for all the Merkle trees in the input, including each of the Merkle trees for keys and values of the Merkleized map commitments of each of the inputs/outputs maps of the psbt.
+
+The `GET_MORE_ELEMENTS` command must be handled.
+
+The `YIELD` command must be processed in order to receive the signature.
+
 ## Client commands reference
 
 This section documents the commands that the Hardware Wallet can request to the client when returning with a `SW_INTERRUPTED_EXECUTION` status word.
@@ -346,7 +407,7 @@ This section documents the commands that the Hardware Wallet can request to the
 
 **Command code**: 0x10
 
-The `YIELD` client command is sent to the client to communicate some result during the execution of a command. Currently only used during `SIGN_PSBT` in order to communicate each of the signatures. The format of the attached message is documented for each command that uses `YIELD`.
+The `YIELD` client command is sent to the client to communicate some result during the execution of a command. Currently used during `SIGN_PSBT` and `SIGN_SENDER_PSBT` in order to communicate each of the signatures. The format of the attached message is documented for each command that uses `YIELD`.
 
 The client must respond with an empty message.
 

src/boilerplate/dispatcher.c

@@ -58,17 +58,15 @@ static void set_ui_dirty() {
 // TODO: refactor code in common with the main apdu loop
 static int process_interruption(dispatcher_context_t *dc) {
     command_t cmd;
-    int input_len;
+    size_t input_len = 0;
 
     // Reset structured APDU command
     memset(&cmd, 0, sizeof(cmd));
 
     io_start_interruption_timeout();
 
     // Receive command bytes in G_io_apdu_buffer
-    if ((input_len = io_exchange(CHANNEL_APDU, G_output_len)) < 0) {
-        return -1;
-    }
+    input_len = io_exchange(CHANNEL_APDU, G_output_len);
 
     io_clear_interruption_timeout();
 
@@ -138,7 +136,7 @@ void apdu_dispatcher(command_descriptor_t const cmd_descriptors[],
         return;
     } else {
         bool cla_found = false, ins_found = false;
-        command_handler_t handler;
+        command_handler_t handler = 0;
         for (int i = 0; i < n_descriptors; i++) {
             if (cmd_descriptors[i].cla != cmd->cla) continue;
             cla_found = true;
@@ -149,7 +147,10 @@ void apdu_dispatcher(command_descriptor_t const cmd_descriptors[],
             break;
         }
 
-        if (!cla_found) {
+        if (!handler) {
+            io_send_sw(SW_CLA_NOT_SUPPORTED);
+            return;
+        } else if (!cla_found) {
             io_send_sw(SW_CLA_NOT_SUPPORTED);
             return;
         } else if (!ins_found) {

src/common/base58.c

@@ -91,19 +91,6 @@ int base58_decode(const char *in, size_t in_len, uint8_t *out, size_t out_len) {
         }
     }
 
-    // // original code for reference
-    // for (uint8_t i = 0; i < in_len; i++) {
-    //     if (in[i] >= sizeof(BASE58_TABLE)) {
-    //         return -1;
-    //     }
-
-    //     tmp[i] = BASE58_TABLE[(int) in[i]];
-
-    //     if (tmp[i] == 0xFF) {
-    //         return -1;
-    //     }
-    // }
-
     while ((zero_count < in_len) && (tmp[zero_count] == 0)) {
         ++zero_count;
     }

src/common/bip32.c

@@ -36,7 +36,7 @@ bool bip32_path_read(const uint8_t *in, size_t in_len, uint32_t *out, size_t out
     size_t offset = 0;
 
     for (size_t i = 0; i < out_len; i++) {
-        if (offset > in_len) {
+        if (offset + 4 > in_len) {
             return false;
         }
         out[i] = read_u32_be(in, offset);
@@ -195,4 +195,4 @@ int get_bip44_purpose(int address_type) {
         default:
             return -1;
     }
-}
+}

src/common/buffer.c

@@ -133,6 +133,10 @@ bool buffer_read_u64(buffer_t *buffer, uint64_t *value, endianness_t endianness)
 }
 
 bool buffer_read_varint(buffer_t *buffer, uint64_t *value) {
+    if (buffer->ptr == NULL) {
+        return false;
+    }
+
     int length = varint_read(buffer->ptr + buffer->offset, buffer->size - buffer->offset, value);
 
     if (length < 0) {

src/common/merkle.c

@@ -38,22 +38,6 @@ void merkle_compute_element_hash(const uint8_t *in, size_t in_len, uint8_t out[s
     crypto_hash_digest(&hash.header, out, 32);
 }
 
-// void merkle_combine_hashes(const uint8_t left[static 32],
-//                            const uint8_t right[static 32],
-//                            uint8_t out[static 32]) {
-//     PRINT_STACK_POINTER();
-
-//     cx_sha256_t hash;
-//     cx_sha256_init(&hash);
-
-//     // H(0x01 | left | right)
-//     crypto_hash_update_u8(&hash.header, 0x01);
-//     crypto_hash_update(&hash.header, left, 32);
-//     crypto_hash_update(&hash.header, right, 32);
-
-//     crypto_hash_digest(&hash.header, out, 32);
-// }
-
 // implementation using the cxram section, in order to save ram
 void merkle_combine_hashes(const uint8_t left[static 32],
                            const uint8_t right[static 32],
@@ -103,4 +87,4 @@ int merkle_get_ith_direction(size_t size, size_t index, size_t i) {
     }
 
     return -1;
-}
+}

src/common/script.c

@@ -84,7 +84,8 @@ int get_script_type(const uint8_t script[], size_t script_len) {
 
 #ifndef SKIP_FOR_CMOCKA
 
-// TODO: add unit tests
+// crypto.c is disabled in unit tests by Bitcoin, which is needed for get_script_address
+// unit tests should be added for script address when it is enabled
 int get_script_address(const uint8_t script[], size_t script_len, char *out, size_t out_len) {
     int script_type = get_script_type(script, script_len);
     int addr_len;

src/common/script.h

@@ -158,10 +158,10 @@ typedef enum {
     SCRIPT_TYPE_P2WSH = 0x03,
     SCRIPT_TYPE_P2TR = 0x04,
     SCRIPT_TYPE_UNKNOWN_SEGWIT = 0xFF,  // a valid but undefined segwit script
-    SCRIPT_TYPE_CREATE_SENDER,
-    SCRIPT_TYPE_CALL_SENDER,
-    SCRIPT_TYPE_CREATE,
-    SCRIPT_TYPE_CALL,
+    SCRIPT_TYPE_CREATE_SENDER = 0x100,
+    SCRIPT_TYPE_CALL_SENDER = 0x101,
+    SCRIPT_TYPE_CREATE = 0x102,
+    SCRIPT_TYPE_CALL = 0x103,
 } script_type_e;
 
 static inline bool is_p2wpkh(const uint8_t script[], size_t script_len) {

src/handler/get_extended_pubkey.c

@@ -63,8 +63,12 @@ static bool is_path_safe_for_pubkey_export(const uint32_t bip32_path[],
             break;
         case 45:
             // BIP-45 prescribes simply length 1, but we instead support existing deployed
-            // use cases with path "m/45'/coin_type'/account'
-            hardened_der_len = 3;
+            // use cases with path "m/45'/coin_type'/account' or "m/45'/coin_type'/account
+            if (bip32_path[2] < 0x80000000) {
+                hardened_der_len = 2;
+            } else {
+                hardened_der_len = 3;
+            }
             break;
         case 48:
             hardened_der_len = 4;

src/handler/lib/check_merkle_tree_sorted.c

@@ -16,8 +16,6 @@ int call_check_merkle_tree_sorted_with_callback(dispatcher_context_t *dispatcher
                                                 size_t size,
                                                 merkle_tree_elements_callback_t callback,
                                                 const merkleized_map_commitment_t *map_commitment) {
-    // LOG_PROCESSOR(__FILE__, __LINE__, __func__);
-
     int prev_el_len = 0;
     uint8_t prev_el[MAX_CHECK_MERKLE_TREE_SORTED_PREIMAGE_SIZE];
 
@@ -76,4 +74,4 @@ static int compare_byte_arrays(const uint8_t array1[],
     }
 
     return memcmp_result;
-}
+}

src/handler/lib/get_merkle_leaf_element.c

@@ -9,8 +9,6 @@ int call_get_merkle_leaf_element(dispatcher_context_t *dispatcher_context,
                                  uint32_t leaf_index,
                                  uint8_t *out_ptr,
                                  size_t out_ptr_len) {
-    // LOG_PROCESSOR(__FILE__, __LINE__, __func__);
-
     uint8_t leaf_hash[32];
 
     int res = call_get_merkle_leaf_hash(dispatcher_context,

src/handler/lib/get_merkle_leaf_hash.c

@@ -17,8 +17,6 @@ int call_get_merkle_leaf_hash(dispatcher_context_t *dc,
                               uint32_t tree_size,
                               uint32_t leaf_index,
                               uint8_t out[static 32]) {
-    // LOG_PROCESSOR(__FILE__, __LINE__, __func__);
-
     PRINT_STACK_POINTER();
 
     {  // make sure memory is deallocated as soon as possible

src/handler/lib/get_merkle_leaf_index.c

@@ -9,8 +9,6 @@ int call_get_merkle_leaf_index(dispatcher_context_t *dispatcher_context,
                                size_t size,
                                const uint8_t root[static 32],
                                const uint8_t leaf_hash[static 32]) {
-    // LOG_PROCESSOR(__FILE__, __LINE__, __func__);
-
     {  // free memory as soon as possible
         uint8_t request[1 + 32 + 32];
         request[0] = CCMD_GET_MERKLE_LEAF_INDEX;

src/handler/lib/get_merkle_preimage.c

@@ -17,8 +17,6 @@ int call_get_merkle_preimage(dispatcher_context_t *dispatcher_context,
                              const uint8_t hash[static 32],
                              uint8_t *out_ptr,
                              size_t out_ptr_len) {
-    // LOG_PROCESSOR(__FILE__, __LINE__, __func__);
-
     PRINT_STACK_POINTER();
 
     uint8_t cmd = CCMD_GET_PREIMAGE;

src/handler/lib/get_merkleized_map.c

@@ -14,8 +14,6 @@ int call_get_merkleized_map_with_callback(dispatcher_context_t *dispatcher_conte
                                           int index,
                                           merkle_tree_elements_callback_t callback,
                                           merkleized_map_commitment_t *out_ptr) {
-    // LOG_PROCESSOR(__FILE__, __LINE__, __func__);
-
     uint8_t raw_output[9 + 2 * 32];  // maximum size of serialized result (9 bytes for the varint,
                                      // and the 2 Merkle roots)
 
@@ -42,4 +40,4 @@ int call_get_merkleized_map_with_callback(dispatcher_context_t *dispatcher_conte
                                                        out_ptr->size,
                                                        callback,
                                                        out_ptr);
-}
+}

src/handler/lib/get_merkleized_map_value.c

@@ -11,8 +11,6 @@ int call_get_merkleized_map_value(dispatcher_context_t *dispatcher_context,
                                   int key_len,
                                   uint8_t *out,
                                   int out_len) {
-    // LOG_PROCESSOR(__FILE__, __LINE__, __func__);
-
     uint8_t key_merkle_hash[32];
     merkle_compute_element_hash(key, key_len, key_merkle_hash);
 
@@ -30,4 +28,4 @@ int call_get_merkleized_map_value(dispatcher_context_t *dispatcher_context,
                                         index,
                                         out,
                                         out_len);
-}
+}

src/handler/lib/get_merkleized_map_value_hash.c

@@ -10,8 +10,6 @@ int call_get_merkleized_map_value_hash(dispatcher_context_t *dispatcher_context,
                                        const uint8_t *key,
                                        int key_len,
                                        uint8_t out[static 32]) {
-    // LOG_PROCESSOR(__FILE__, __LINE__, __func__);
-
     uint8_t key_merkle_hash[32];
     merkle_compute_element_hash(key, key_len, key_merkle_hash);
 

src/handler/lib/get_preimage.c

@@ -10,8 +10,6 @@ int call_get_preimage(dispatcher_context_t *dispatcher_context,
                       const uint8_t hash[static 32],
                       uint8_t *out,
                       size_t out_len) {
-    // LOG_PROCESSOR(__FILE__, __LINE__, __func__);
-
     uint8_t cmd = CCMD_GET_PREIMAGE;
     dispatcher_context->add_to_response(&cmd, 1);
     uint8_t zero = 0;