Merge pull request #3 from vacuumlabs/audit_findings

Audit findings

README.md

@@ -25,7 +25,85 @@ Please:
 _Once the app is approved by Ledger, it will be available in their app store (Ledger Live).
 The builds generated by this repository are for development purposes. THESE ARE UNVETTED DEVELOPMENT RELEASES._
 
-## Quick start guide
+## Quick start guide 
+
+#### Docker
+
+Install and run [Docker](https://www.docker.com/products/docker-desktop/).
+
+#### Compilation
+
+We develop on Ubuntu. On different system slight adjustment of the commands may be necessary (see documentation below).
+
+He recommended way to compile the app is:
+1. Pull the latest docker container.
+```shell
+sudo docker pull ghcr.io/ledgerhq/ledger-app-builder/ledger-app-dev-tools:latest
+```
+2. Start terminal within the container.
+```shell
+sudo docker run --rm -ti --user "$(id -u):$(id -g)" --privileged -v "/dev/bus/usb:/dev/bus/usb" -v "$(realpath .):/app" ghcr.io/ledgerhq/ledger-app-builder/ledger-app-dev-tools:latest
+```
+3. Compile the app
+```shell
+make clean
+make
+make BOLOS_SDK=$NANOX_SDK
+make BOLOS_SDK=$NANOSP_SDK
+make BOLOS_SDK=$STAX_SDK
+```
+
+Stax app can be compiled in DEBUG mode for debugging purposes
+```shell
+make BOLOS_SDK=$STAX_SDK DEBUG=1
+```
+Note, that it is possible (although unlikely) for ledger to make a braking change in ledger-app-dev-tools:latest. 
+
+#### Tests
+
+##### Speculos integration tests
+
+These are the main test. These end to end test cover all app features.
+To run them you should compile the app in ledger-app-dev-tools:latest container. And then (in container) run 
+
+```shell
+pytest tests/ --tb=short -v --device nanos
+pytest tests/ --tb=short -v --device nanox
+pytest tests/ --tb=short -v --device nanosp
+pytest tests/ --tb=short -v --device stax
+```
+
+Note that in case ledger-app-dev-tools:latest is updated there is a chance that slight changes in gui happen. In that case it is necessary to re-generate the snapshots, e.g.
+```shell
+pytest tests/ --tb=short -v --device nanos –golden_run
+```
+and review the changes in test/snapshots directory
+
+##### Unit tests
+
+As we want to test as close as possible to the production environment, the focus is on end to end integration test. However, certain complex parts of code, where test coverage with integration tests may be insufficient, are also tested using unit tests.
+
+```shell
+cd unit-tests/
+cmake -Bbuild -H. && make -C build
+CTEST_OUTPUT_ON_FAILURE=1 make -C build test
+```
+
+#### Scan build
+
+We use scan build in ledger-app-dev-tools container (see Compilation section). 
+
+```shell
+make clean
+make scan-build
+make scan-build BOLOS_SDK=$NANOX_SDK
+make scan-build BOLOS_SDK=$NANOSP_SDK
+make scan-build BOLOS_SDK=$STAX_SDK
+```
+
+## Further information
+
+_Warning_: This is standard documentation for ledger app provided by ledger developers. As we do not use all the options, we do no guarantee that everything is up to date. 
 
 ### General configuration
 
@@ -55,7 +133,7 @@ It will allow you, whether you are developing on macOS, Windows or Linux to quic
 
 ### With a terminal
 
-#### Using the `ledger-app-dev-tools` docker container
+#### Using the `ledger-app-dev-tools` docker container (recommended)
 
 The [ledger-app-dev-tools](https://github.com/LedgerHQ/ledger-app-builder/pkgs/container/ledger-app-builder%2Fledger-app-dev-tools) docker image contains all the required tools and libraries to **build**, **test** and **load** an application.
 
@@ -67,7 +145,7 @@ sudo docker pull ghcr.io/ledgerhq/ledger-app-builder/ledger-app-dev-tools:latest
 
 You can then enter this development environment by executing the following command from the directory of the application `git` repository:
 
-##### Linux (Ubuntu)
+##### Linux (Ubuntu) (recommended)
 
 ```shell
 sudo docker run --rm -ti --user "$(id -u):$(id -g)" --privileged -v "/dev/bus/usb:/dev/bus/usb" -v "$(realpath .):/app" ghcr.io/ledgerhq/ledger-app-builder/ledger-app-dev-tools:latest
@@ -116,7 +194,7 @@ Setup a compilation environment by following the [shell with docker approach](#w
 From inside the container, use the following command to build the app :
 
 ```shell
-make DEBUG=1  # compile optionally with PRINTF
+make 
 ```
 
 You can choose which device to compile and load for by setting the `BOLOS_SDK` environment variable to the following values :
@@ -126,6 +204,11 @@ You can choose which device to compile and load for by setting the `BOLOS_SDK` e
 - `BOLOS_SDK=$NANOSP_SDK`
 - `BOLOS_SDK=$STAX_SDK`
 
+For Stax device you can compile 
+```shell
+make BOLOS_SDK=$STAX_SDK DEBUG=1  # compile optionally with PRINTF
+```
+
 ### Loading on a physical device
 
 This step will vary slightly depending on your platform.
@@ -175,7 +258,7 @@ python3 -m ledgerblue.runScript --scp --fileName bin/app.apdu --elfFile bin/app.
 
 The flow app comes with functional tests implemented with Ledger's [Ragger](https://github.com/LedgerHQ/ragger) test framework.
 
-### Linux (Ubuntu)
+### Linux (Ubuntu) (recommended)
 
 On Linux, you can use [Ledger's VS Code extension](#with-vscode) to run the tests. If you prefer not to, open a terminal and follow the steps below.
 

deps/ledger-zxlib/include/buffering.h

@@ -48,7 +48,7 @@ void buffering_reset();
 /// \param data
 /// \param length
 /// \return the number of appended bytes
-int buffering_append(uint8_t *data, int length);
+uint32_t buffering_append(uint8_t *data, uint32_t length);
 
 /// buffering_get_ram_buffer
 /// \return

deps/ledger-zxlib/src/buffering.c

@@ -46,7 +46,7 @@ void buffering_reset() {
     flash.in_use = 0;
 }
 
-int buffering_append(uint8_t *data, int length) {
+uint32_t buffering_append(uint8_t *data, uint32_t length) {
     if (ram.in_use) {
         if (ram.size - ram.pos >= length) {
             // RAM in use, append to ram if there is enough space

docs/APDUSPEC.md

@@ -216,7 +216,7 @@ Appends payload to transaction / message.
 | ------- | ------- | ---------------- | -------- |
 | Message | bytes.. | RLP data to sign |          |
 
-Signs the message without metadata (arbitrary message signing). This requires expert mode and is able to handle any transaction. The app shows script hash and tries to show transaction arguments and their types, or a message that they are too long to display.
+Signs the message without metadata (arbitrary transaction signing). This requires expert mode and is able to handle any transaction. The app shows script hash and tries to show transaction arguments and their types, or a message that they are too long to display.
 
 ##### Metadata Packet P1 = 0x03
 

src/account.c

@@ -62,8 +62,10 @@ zxerr_t slot_getItem(int8_t displayIdx,
     zemu_log_stack("slot_getItem");
     *pageCount = 1;
 
+    // selects workflow according to what type of slot operation was detected.
     switch (tmp_slotop) {
         case SLOT_OP_SET: {
+            // screens
             switch (displayIdx) {
                 case 0: {
                     snprintf(outKey, outKeyLen, "Set");
@@ -94,6 +96,7 @@ zxerr_t slot_getItem(int8_t displayIdx,
             const account_slot_t *oldSlot =
                 (const account_slot_t *) &N_slot_store.slot[tmp_slotIdx];
             switch (displayIdx) {
+                // screens
                 case 0: {
                     snprintf(outKey, outKeyLen, "Update");
                     snprintf(outVal, outValLen, "Account %d", tmp_slotIdx);
@@ -139,6 +142,7 @@ zxerr_t slot_getItem(int8_t displayIdx,
         case SLOT_UP_DELETE: {
             const account_slot_t *oldSlot =
                 (const account_slot_t *) &N_slot_store.slot[tmp_slotIdx];
+            // screens
             switch (displayIdx) {
                 case 0: {
                     snprintf(outKey, outKeyLen, "Delete");

src/account.h

@@ -38,8 +38,10 @@ typedef struct {
 extern account_slot_t tmp_slot;
 extern uint8_t tmp_slotIdx;
 
+/// Return the number of items in the address view
 zxerr_t slot_getNumItems(uint8_t *num_items);
 
+/// Gets an specific item from the slot view (including paging)
 zxerr_t slot_getItem(int8_t displayIdx,
                      char *outKey,
                      uint16_t outKeyLen,
@@ -48,16 +50,20 @@ zxerr_t slot_getItem(int8_t displayIdx,
                      uint8_t pageIdx,
                      uint8_t *pageCount);
 
+// Updates the slot
+void app_slot_setSlot();
+
+// Gets status of all slots
 zxerr_t slot_status(uint8_t *out, uint16_t outLen);
 
+// Gets data from he slot
 zxerr_t slot_getSlot(uint8_t slotIndex, account_slot_t *out);
 
+// Parses and stores buffer data
 zxerr_t slot_parseSlot(uint8_t *buffer, uint16_t bufferLen);
 
 zxerr_t slot_serializeSlot(const account_slot_t *slot, uint8_t *buffer, uint16_t *bufferLen);
 
-void app_slot_setSlot();
-
 void loadHdPathAndAddressFromSlot();
 
 void loadAddressCompareHdPathFromSlot();

src/apdu_handler.c

@@ -101,7 +101,10 @@ __Z_INLINE void handleSign(volatile uint32_t *flags, volatile uint32_t *tx, uint
             zxerr_t err = message_parse();
             if (err != zxerr_ok) {
                 const char *error_msg = "Invalid message";
-                int error_msg_length = strlen(error_msg);
+                uint16_t error_msg_length = strlen(error_msg);
+                if (error_msg_length > sizeof(G_io_apdu_buffer)) {
+                    THROW(APDU_CODE_UNKNOWN);
+                }
                 MEMCPY(G_io_apdu_buffer, error_msg, error_msg_length);
                 *tx += (error_msg_length);
                 ZEMU_TRACE();
@@ -118,7 +121,10 @@ __Z_INLINE void handleSign(volatile uint32_t *flags, volatile uint32_t *tx, uint
             const char *error_msg = tx_parse(callType);
 
             if (error_msg != NULL) {
-                int error_msg_length = strlen(error_msg);
+                uint16_t error_msg_length = strlen(error_msg);
+                if (error_msg_length >= sizeof(G_io_apdu_buffer)) {
+                    THROW(APDU_CODE_UNKNOWN);
+                }
                 MEMCPY(G_io_apdu_buffer, error_msg, error_msg_length);
                 *tx += (error_msg_length);
                 ZEMU_TRACE();
@@ -200,10 +206,6 @@ __Z_INLINE void handleGetSlot(__Z_UNUSED volatile uint32_t *flags,
 __Z_INLINE void handleSetSlot(volatile uint32_t *flags,
                               __Z_UNUSED volatile uint32_t *tx,
                               uint32_t rx) {
-    if (rx != 5 + 1 + 8 + 20 + 2) {
-        THROW(APDU_CODE_DATA_INVALID);
-    }
-
     zxerr_t err = slot_parseSlot(G_io_apdu_buffer + OFFSET_DATA, rx - OFFSET_DATA);
     if (err != zxerr_ok) {
         THROW(APDU_CODE_DATA_INVALID);

src/coin.h

@@ -45,9 +45,6 @@ typedef struct {
 
 #define SECP256_PK_LEN 65u
 
-#define COIN_AMOUNT_DECIMAL_PLACES 0  // FIXME: Adjust this
-#define COIN_SUPPORTED_TX_VERSION  0
-
 #define MENU_MAIN_APP_LINE1 "Flow"
 #define MENU_MAIN_APP_LINE2 "Ready"
 #define APPVERSION_LINE1    "Version"

src/common/app_helper.c

@@ -31,6 +31,9 @@
 #include "tx_metadata.h"
 
 void extractHDPathAndCryptoOptions(uint32_t rx, uint32_t offset) {
+    if (rx >= sizeof(G_io_apdu_buffer)) {
+        THROW(APDU_CODE_WRONG_LENGTH);
+    }
     if ((rx - offset) < sizeof(hdPath.data) + sizeof(cryptoOptions)) {
         THROW(APDU_CODE_WRONG_LENGTH);
     }

src/common/tx.c

@@ -53,10 +53,7 @@ const uint8_t TX_DOMAIN_TAG[DOMAIN_TAG_LENGTH] = {
 };
 
 void tx_initialize() {
-    buffering_init(ram_buffer,
-                   sizeof(ram_buffer),
-                   (uint8_t *) N_appdata.buffer,
-                   sizeof(N_appdata.buffer));
+    buffering_init(ram_buffer, sizeof(ram_buffer), (uint8_t *) N_appdata.buffer, FLASH_BUFFER_SIZE);
 }
 
 void tx_reset() {

src/crypto.c

@@ -94,8 +94,14 @@ typedef struct {
     uint8_t der_signature[73];
 } __attribute__((packed)) signature_t;
 
-void sha256(const uint8_t *message, uint16_t messageLen, uint8_t message_digest[CX_SHA256_SIZE]) {
-    cx_hash_sha256(message, messageLen, message_digest, CX_SHA256_SIZE);
+zxerr_t sha256(const uint8_t *message,
+               uint16_t messageLen,
+               uint8_t message_digest[CX_SHA256_SIZE]) {
+    size_t digest_len = cx_hash_sha256(message, messageLen, message_digest, CX_SHA256_SIZE);
+    if (digest_len != CX_SHA256_SIZE) {
+        return zxerr_invalid_crypto_settings;
+    }
+    return zxerr_ok;
 }
 
 zxerr_t digest_message(const uint8_t *message,
@@ -132,7 +138,7 @@ zxerr_t digest_message(const uint8_t *message,
             return zxerr_ok;
         }
         case HASH_SHA3_256: {
-            if (digestMax < 32) {
+            if (digestMax < CX_SHA3_256_SIZE) {
                 return zxerr_buffer_too_small;
             }
             zemu_log_stack("sha3_256");
@@ -185,7 +191,17 @@ zxerr_t crypto_sign(const hd_path_t path,
                                sizeof(messageDigest),
                                &messageDigestSize));
 
-    if (messageDigestSize != 32) {
+    if (cx_hash_kind != HASH_SHA2_256 && cx_hash_kind != HASH_SHA3_256) {
+        zemu_log_stack("crypto_sign: zxerr_out_of_bounds");
+        return zxerr_out_of_bounds;
+    }
+
+    if (cx_hash_kind == HASH_SHA2_256 && messageDigestSize != CX_SHA256_SIZE) {
+        zemu_log_stack("crypto_sign: zxerr_out_of_bounds");
+        return zxerr_out_of_bounds;
+    }
+
+    if (cx_hash_kind == HASH_SHA3_256 && messageDigestSize != CX_SHA3_256_SIZE) {
         zemu_log_stack("crypto_sign: zxerr_out_of_bounds");
         return zxerr_out_of_bounds;
     }

src/crypto.h

@@ -30,12 +30,13 @@ typedef enum { HASH_UNKNOWN, HASH_SHA2_256, HASH_SHA3_256 } digest_type_e;
 
 typedef enum { CURVE_UNKNOWN, CURVE_SECP256K1, CURVE_SECP256R1 } curve_e;
 
-#if defined(TARGET_NANOS) || defined(TARGET_NANOX) || defined(TARGET_NANOS2)
+#if defined(TARGET_NANOS) || defined(TARGET_NANOX) || defined(TARGET_NANOS2) || defined(TARGET_STAX)
 #else
-#define CX_SHA256_SIZE 32
+#define CX_SHA256_SIZE   32
+#define CX_SHA3_256_SIZE 32
 #endif
 
-void sha256(const uint8_t *message, uint16_t messageLen, uint8_t message_digest[CX_SHA256_SIZE]);
+zxerr_t sha256(const uint8_t *message, uint16_t messageLen, uint8_t message_digest[CX_SHA256_SIZE]);
 
 zxerr_t crypto_extractPublicKey(const hd_path_t path,
                                 const uint16_t options,