Merge pull request #760 from LedgerHQ/security-review-1-16-0

Security Review 1.16.0

Author: apaillier-ledger

.github/workflows/cflite_cron.yml

@@ -35,6 +35,6 @@ jobs:
         uses: google/clusterfuzzlite/actions/run_fuzzers@v1
         with:
           github-token: ${{ secrets.GITHUB_TOKEN }}
-          fuzz-seconds: 300  # 5 minutes
+          fuzz-seconds: 900  # 15 minutes
           mode: ${{ matrix.mode }}
           sanitizer: ${{ matrix.sanitizer }}

src/main.c

@@ -271,7 +271,7 @@ static uint16_t handleApdu(command_t *cmd, uint32_t *flags, uint32_t *tx) {
 
 #ifdef HAVE_WEB3_CHECKS
         case INS_PROVIDE_TX_SIMULATION:
-            sw = handleTxSimulation(cmd->p1, cmd->p2, cmd->data, cmd->lc, flags);
+            sw = handle_tx_simulation(cmd->p1, cmd->p2, cmd->data, cmd->lc, flags);
             break;
 #endif
 

src/mem.c

@@ -43,8 +43,16 @@ void mem_reset(void) {
  * @return Allocated memory pointer; \ref NULL if not enough space left.
  */
 void *mem_alloc(size_t size) {
+    size_t new_idx;
+    size_t free_size;
+
+    if (__builtin_add_overflow((size_t) mem_idx, size, &new_idx) ||
+        __builtin_sub_overflow(sizeof(mem_buffer), (size_t) mem_rev_idx, &free_size)) {
+        PRINTF("Error: overflow detected!\n");
+        return NULL;
+    }
     // Buffer exceeded
-    if ((mem_idx + size) > (sizeof(mem_buffer) - mem_rev_idx)) {
+    if (new_idx > free_size) {
         PRINTF("Error: mem_alloc(%u) failed!\n", size);
         return NULL;
     }
@@ -74,8 +82,16 @@ void mem_dealloc(size_t size) {
  * @return Allocated memory pointer; \ref NULL if not enough space left.
  */
 void *mem_rev_alloc(size_t size) {
+    size_t free_size;
+    size_t new_rev_idx;
+
+    if (__builtin_add_overflow((size_t) mem_rev_idx, size, &new_rev_idx) ||
+        __builtin_sub_overflow(sizeof(mem_buffer), new_rev_idx, &free_size)) {
+        PRINTF("Error: overflow detected!\n");
+        return NULL;
+    }
     // Buffer exceeded
-    if ((sizeof(mem_buffer) - (mem_rev_idx + size)) < mem_idx) {
+    if (free_size < mem_idx) {
         PRINTF("Error: mem_rev_alloc(%u) failed!\n", size);
         return NULL;
     }

src/utils.c

@@ -1,3 +1,4 @@
+#include <ctype.h>
 #include <string.h>
 #include "utils.h"
 
@@ -29,3 +30,20 @@ void str_cpy_explicit_trunc(const char *src, size_t src_size, char *dst, size_t
         memcpy(&dst[off], trunc_marker, sizeof(trunc_marker));
     }
 }
+
+/**
+ * @brief Check the name is printable.
+ *
+ * @param[in] data buffer received
+ * @param[in] name Name to check
+ * @param[in] len Length of the name
+ * @return True/False
+ */
+bool check_name(const uint8_t *name, uint16_t len) {
+    for (uint16_t i = 0; i < len; i++) {
+        if (!isprint(name[i])) {
+            return false;
+        }
+    }
+    return true;
+}

src/utils.h

@@ -2,10 +2,12 @@
 #define UTILS_H_
 
 #include <stdint.h>
+#include <stdbool.h>
 
 #define SET_BIT(a) (1 << a)
 
 void buf_shrink_expand(const uint8_t *src, size_t src_size, uint8_t *dst, size_t dst_size);
 void str_cpy_explicit_trunc(const char *src, size_t src_size, char *dst, size_t dst_size);
+bool check_name(const uint8_t *name, uint16_t len);
 
 #endif  // !UTILS_H_

src_features/generic_tx_parser/calldata.c

@@ -80,7 +80,7 @@ static bool compress_chunk(s_calldata *calldata) {
         start_idx = strip_left;
     } else {
         strip_dir = CHUNK_STRIP_RIGHT;
-        stripped_size = strip_right;
+        stripped_size = CALLDATA_CHUNK_SIZE - strip_right;
         start_idx = 0;
     }
     chunk_info |= strip_dir << CHUNK_INFO_DIR_OFFSET;
@@ -171,10 +171,12 @@ bool calldata_append(const uint8_t *buffer, size_t size) {
 }
 
 void calldata_cleanup(void) {
-    mem_dealloc(g_calldata->chunks_size);
-    mem_dealloc(sizeof(*g_calldata));
-    g_calldata = NULL;
-    mem_dealloc(g_calldata_alignment);
+    if (g_calldata != NULL) {
+        mem_dealloc(g_calldata->chunks_size);
+        mem_dealloc(sizeof(*g_calldata));
+        g_calldata = NULL;
+        mem_dealloc(g_calldata_alignment);
+    }
 }
 
 static bool has_valid_calldata(const s_calldata *calldata) {

src_features/generic_tx_parser/gtp_field_table.c

@@ -61,13 +61,15 @@ void field_table_cleanup(void) {
 bool add_to_field_table(e_param_type type, const char *key, const char *value) {
     int offset = 0;
     uint8_t *ptr;
-    uint8_t key_len = strlen(key) + 1;
-    uint16_t value_len = strlen(value) + 1;
+    uint8_t key_len;
+    uint16_t value_len;
 
     if ((key == NULL) || (value == NULL)) {
         PRINTF("Error: NULL key/value!\n");
         return false;
     }
+    key_len = strlen(key) + 1;
+    value_len = strlen(value) + 1;
     PRINTF(">>> \"%s\": \"%s\"\n", key, value);
     if ((ptr = mem_alloc(sizeof(type) + sizeof(uint8_t) + sizeof(uint16_t) + key_len +
                          value_len)) == NULL) {

src_features/provide_network_info/network_info.c

@@ -1,6 +1,5 @@
 #ifdef HAVE_DYNAMIC_NETWORKS
 
-#include <ctype.h>
 #include "network_info.h"
 #include "utils.h"
 #include "read.h"
@@ -109,23 +108,6 @@ static bool handle_chain_id(const s_tlv_data *data, s_network_info_ctx *context)
     return true;
 }
 
-/**
- * @brief Check the name is printable.
- *
- * @param[in] data buffer received
- * @param[in] name Name to check
- * @param[in] len Length of the name
- * @return True/False
- */
-static bool check_name(const uint8_t *name, uint16_t len) {
-    for (uint16_t i = 0; i < len; i++) {
-        if (!isprint(name[i])) {
-            return false;
-        }
-    }
-    return true;
-}
-
 /**
  * @brief Parse the NETWORK_NAME value.
  *
@@ -237,7 +219,7 @@ bool handle_network_info_struct(const s_tlv_data *data, s_network_info_ctx *cont
             break;
         default:
             PRINTF(TLV_TAG_ERROR_MSG, data->tag);
-            ret = true;  // TODO: shouldn't it be false ?
+            ret = true;
     }
     if (ret && (data->tag != TAG_DER_SIGNATURE)) {
         hash_nbytes(data->raw, data->raw_size, (cx_hash_t *) &context->hash_ctx);

src_features/provide_tx_simulation/cmd_get_tx_simulation.c

@@ -1,6 +1,5 @@
 #ifdef HAVE_WEB3_CHECKS
 
-#include <ctype.h>
 #include "cmd_get_tx_simulation.h"
 #include "apdu_constants.h"
 #include "hash_bytes.h"
@@ -108,23 +107,6 @@ tx_simulation_t TX_SIMULATION = {0};
         memmove((void *) field, data->value, data->length); \
     } while (0)
 
-/**
- * @brief Check the name is printable.
- *
- * @param[in] data buffer received
- * @param[in] name Name to check
- * @param[in] len Length of the name
- * @return True/False
- */
-static bool check_name(const uint8_t *name, uint16_t len) {
-    for (uint16_t i = 0; i < len; i++) {
-        if (!isprint(name[i])) {
-            return false;
-        }
-    }
-    return true;
-}
-
 /**
  * @brief Parse the STRUCTURE_TYPE value.
  *
@@ -379,18 +361,25 @@ static bool verify_signature(s_tx_simu_ctx *context) {
  *
  * Check the mandatory fields are present
  *
- * @param[in] rcv_bit indicates received fields
+ * @param[in] context TX Simu context
  * @return whether it was successful
  */
-static bool verify_fields(uint32_t rcv_bit) {
+static bool verify_fields(s_tx_simu_ctx *context) {
     uint32_t expected_fields;
 
     expected_fields = (1 << BIT_STRUCTURE_TYPE) | (1 << BIT_STRUCTURE_VERSION) |
                       (1 << BIT_TX_HASH) | (1 << BIT_ADDRESS) | (1 << BIT_W3C_NORMALIZED_RISK) |
                       (1 << BIT_W3C_NORMALIZED_CATEGORY) | (1 << BIT_W3C_TINY_URL) |
                       (1 << BIT_W3C_SIMU_TYPE) | (1 << BIT_DER_SIGNATURE);
 
-    return ((rcv_bit & expected_fields) == expected_fields);
+    if (context->simu->type == SIMU_TYPE_TRANSACTION) {
+        expected_fields |= (1 << BIT_CHAIN_ID);
+    }
+    if (context->simu->type == SIMU_TYPE_TYPED_DATA) {
+        expected_fields |= (1 << BIT_DOMAIN_HASH);
+    }
+
+    return ((context->rcv_flags & expected_fields) == expected_fields);
 }
 
 /**
@@ -411,8 +400,10 @@ static void print_simulation_info(s_tx_simu_ctx *context) {
         u64_to_string(context->simu->chain_id, chain_str, sizeof(chain_str));
         PRINTF("[TX SIMU] -    ChainID: %s\n", chain_str);
     }
-    PRINTF("[TX SIMU] -    Risk: %d -> %s\n", context->simu->risk, getTxSimuRiskStr());
-    PRINTF("[TX SIMU] -    Category: %d -> %s\n", context->simu->category, getTxSimuCategoryStr());
+    PRINTF("[TX SIMU] -    Risk: %d -> %s\n", context->simu->risk, get_tx_simulation_risk_str());
+    PRINTF("[TX SIMU] -    Category: %d -> %s\n",
+           context->simu->category,
+           get_tx_simulation_category_str());
     PRINTF("[TX SIMU] -    Provider Msg: %s\n", context->simu->provider_msg);
     PRINTF("[TX SIMU] -    Tiny URL: %s\n", context->simu->tiny_url);
 }
@@ -489,14 +480,14 @@ static bool handle_tlv_payload(const uint8_t *payload, uint16_t size, bool to_fr
 
     ctx.simu = &TX_SIMULATION;
     // Reset the structures
-    explicit_bzero(&TX_SIMULATION, sizeof(tx_simulation_t));
+    explicit_bzero(&TX_SIMULATION, sizeof(TX_SIMULATION));
     // Initialize the hash context
     cx_sha256_init(&ctx.hash_ctx);
 
     parsing_ret = tlv_parse(payload, size, (f_tlv_data_handler) &handle_tx_simu_tlv, &ctx);
     if (to_free) mem_dealloc(size);
-    if (!parsing_ret || !verify_fields(ctx.rcv_flags) || !verify_signature(&ctx)) {
-        explicit_bzero(&TX_SIMULATION, sizeof(tx_simulation_t));
+    if (!parsing_ret || !verify_fields(&ctx) || !verify_signature(&ctx)) {
+        explicit_bzero(&TX_SIMULATION, sizeof(TX_SIMULATION));
         explicit_bzero(&ctx, sizeof(s_tx_simu_ctx));
         return false;
     }
@@ -513,7 +504,7 @@ static bool handle_tlv_payload(const uint8_t *payload, uint16_t size, bool to_fr
  *
  * @param[in] response_expected indicates if a response is expected
  */
-void handleTxSimulationOptIn(bool response_expected) {
+void handle_tx_simulation_opt_in(bool response_expected) {
     if (N_storage.w3c_opt_in) {
         // Web3 Checks already Opt-In
         PRINTF("Web3 Checks already Opt-in!\n");
@@ -530,16 +521,17 @@ void handleTxSimulationOptIn(bool response_expected) {
 /**
  * @brief Handle Tx Simulation APDU.
  *
- * @param[in] p1 APDU parameter 1
+ * @param[in] p1 APDU parameter 1 (indicates Data payload or Opt-In request)
+ * @param[in] p2 APDU parameter 2 (indicates if the payload is the first chunk)
  * @param[in] data buffer received
  * @param[in] length of the buffer
  * @return APDU Response code
  */
-uint16_t handleTxSimulation(uint8_t p1,
-                            uint8_t p2,
-                            const uint8_t *data,
-                            uint8_t length,
-                            unsigned int *flags) {
+uint16_t handle_tx_simulation(uint8_t p1,
+                              uint8_t p2,
+                              const uint8_t *data,
+                              uint8_t length,
+                              unsigned int *flags) {
     uint16_t sw = APDU_RESPONSE_INTERNAL_ERROR;
 
     switch (p1) {
@@ -558,7 +550,7 @@ uint16_t handleTxSimulation(uint8_t p1,
             break;
         case 0x01:
             // TX Simulation Opt-In
-            handleTxSimulationOptIn(true);
+            handle_tx_simulation_opt_in(true);
             *flags |= IO_ASYNCH_REPLY;
             sw = APDU_NO_RESPONSE;
             break;
@@ -574,8 +566,8 @@ uint16_t handleTxSimulation(uint8_t p1,
  * @brief Clear the TX Simulation parameters.
  *
  */
-void clearTxSimulation(void) {
-    explicit_bzero(&TX_SIMULATION, sizeof(tx_simulation_t));
+void clear_tx_simulation(void) {
+    explicit_bzero(&TX_SIMULATION, sizeof(TX_SIMULATION));
 }
 
 /**
@@ -585,7 +577,7 @@ void clearTxSimulation(void) {
  * @param[in] checkFromAddr flag to check the FROM address
  * @return whether it was successful
  */
-bool checkTxSimulationParams(bool checkTxHash, bool checkFromAddr) {
+bool check_tx_simulation_params(bool checkTxHash, bool checkFromAddr) {
     uint8_t msg_sender[ADDRESS_LENGTH] = {0};
     uint64_t chain_id = get_tx_chain_id();
     uint8_t *hash = NULL;
@@ -703,13 +695,13 @@ bool checkTxSimulationParams(bool checkTxHash, bool checkFromAddr) {
  * @param[in] checkTxHash flag to check the TX_HASH
  * @param[in] checkFromAddr flag to check the FROM address
  */
-void setTxSimuWarning(nbgl_warning_t *p_warning, bool checkTxHash, bool checkFromAddr) {
+void set_tx_simulation_warning(nbgl_warning_t *p_warning, bool checkTxHash, bool checkFromAddr) {
     if (!N_storage.w3c_enable) {
         // W3Checks disabled
         return;
     }
     // W3Checks enabled => Verify parameters of the Transaction
-    checkTxSimulationParams(checkTxHash, checkFromAddr);
+    check_tx_simulation_params(checkTxHash, checkFromAddr);
     switch (TX_SIMULATION.risk) {
         case RISK_UNKNOWN:
             p_warning->predefinedSet |= SET_BIT(W3C_ISSUE_WARN);
@@ -727,7 +719,7 @@ void setTxSimuWarning(nbgl_warning_t *p_warning, bool checkTxHash, bool checkFro
             break;
     }
     p_warning->reportProvider = PIC(TX_SIMULATION.partner);
-    p_warning->providerMessage = getTxSimuCategoryStr();
+    p_warning->providerMessage = get_tx_simulation_category_str();
     p_warning->reportUrl = PIC(TX_SIMULATION.tiny_url);
 }
 
@@ -736,7 +728,7 @@ void setTxSimuWarning(nbgl_warning_t *p_warning, bool checkTxHash, bool checkFro
  *
  * @return risk as a string
  */
-const char *getTxSimuRiskStr(void) {
+const char *get_tx_simulation_risk_str(void) {
     switch (TX_SIMULATION.risk) {
         case RISK_UNKNOWN:
             return "UNKNOWN (W3C Issue)";
@@ -757,7 +749,7 @@ const char *getTxSimuRiskStr(void) {
  *
  * @return category string
  */
-const char *getTxSimuCategoryStr(void) {
+const char *get_tx_simulation_category_str(void) {
     // Unknown category string
     switch (TX_SIMULATION.risk) {
         case RISK_UNKNOWN: