Merge pull request #276 from LedgerHQ/apr/bugfix/nft_security_review

NFT feature security review

Author: apaillier-ledger

src/apdu_constants.h

@@ -1,3 +1,6 @@
+#ifndef _APDU_CONSTANTS_H_
+#define _APDU_CONSTANTS_H_
+
 #include "shared_context.h"
 
 #define APP_FLAG_DATA_ALLOWED          0x01
@@ -156,3 +159,5 @@ void handleStarkwareUnsafeSign(uint8_t p1,
                                unsigned int *tx);
 
 #endif
+
+#endif  // _APDU_CONSTANTS_H_

src/chainConfig.h

@@ -69,4 +69,4 @@ typedef struct chain_config_s {
 
 #define ETHEREUM_MAINNET_CHAINID 1
 
-#endif /* _CHAIN_CONFIG_H_ */
+#endif  // _CHAIN_CONFIG_H_

src/eth_plugin_handler.c

@@ -48,14 +48,14 @@ void eth_plugin_prepare_query_contract_UI(ethQueryContractUI_t *queryContractUI,
     memset((uint8_t *) queryContractUI, 0, sizeof(ethQueryContractUI_t));
 
     // If no extra information was found, set the pointer to NULL
-    if (allzeroes(&tmpCtx.transactionContext.extraInfo[1], sizeof(union extraInfo_t))) {
+    if (NO_EXTRA_INFO(tmpCtx, 1)) {
         queryContractUI->item1 = NULL;
     } else {
         queryContractUI->item1 = &tmpCtx.transactionContext.extraInfo[1];
     }
 
     // If no extra information was found, set the pointer to NULL
-    if (allzeroes(&tmpCtx.transactionContext.extraInfo[0], sizeof(union extraInfo_t))) {
+    if (NO_EXTRA_INFO(tmpCtx, 0)) {
         queryContractUI->item2 = NULL;
     } else {
         queryContractUI->item2 = &tmpCtx.transactionContext.extraInfo[0];

src/eth_plugin_handler.h

@@ -1,7 +1,11 @@
-#ifndef __ETH_PLUGIN_HANDLER_H__
+#ifndef _ETH_PLUGIN_HANDLER_H_
+#define _ETH_PLUGIN_HANDLER_H_
 
 #include "eth_plugin_interface.h"
 
+#define NO_EXTRA_INFO(ctx, idx) \
+    (allzeroes(&(ctx.transactionContext.extraInfo[idx]), sizeof(extraInfo_t)))
+
 void eth_plugin_prepare_init(ethPluginInitContract_t *init, uint8_t *selector, uint32_t dataSize);
 void eth_plugin_prepare_provide_parameter(ethPluginProvideParameter_t *provideParameter,
                                           uint8_t *parameter,
@@ -27,4 +31,4 @@ eth_plugin_result_t eth_plugin_call(int method, void *parameter);
 
 void plugin_ui_start(void);
 
-#endif
+#endif  // _ETH_PLUGIN_HANDLER_H_

src/eth_plugin_interface.h

@@ -1,6 +1,5 @@
-#ifndef __ETH_PLUGIN_INTERFACE_H__
-
-#define __ETH_PLUGIN_INTERFACE_H__
+#ifndef _ETH_PLUGIN_INTERFACE_H_
+#define _ETH_PLUGIN_INTERFACE_H_
 
 #include "os.h"
 #include "cx.h"
@@ -179,4 +178,4 @@ typedef struct ethQueryContractUI_t {
 
 } ethQueryContractUI_t;
 
-#endif
+#endif  // _ETH_PLUGIN_INTERFACE_H_

src/eth_plugin_internal.c

@@ -1,3 +1,4 @@
+#include <string.h>
 #include "eth_plugin_internal.h"
 
 bool erc20_plugin_available_check(void);

src/eth_plugin_internal.h

@@ -1,5 +1,8 @@
-#ifndef __ETH_PLUGIN_INTERNAL_H__
+#ifndef _ETH_PLUGIN_INTERNAL_H_
+#define _ETH_PLUGIN_INTERNAL_H_
 
+#include <stdint.h>
+#include <stdbool.h>
 #include "eth_plugin_interface.h"
 
 #define SELECTOR_SIZE    4
@@ -45,4 +48,4 @@ extern const uint8_t* const STARKWARE_SELECTORS[NUM_STARKWARE_SELECTORS];
 
 extern internalEthPlugin_t const INTERNAL_ETH_PLUGINS[];
 
-#endif
+#endif  // _ETH_PLUGIN_INTERNAL_H_

src/handle_check_address.c

@@ -6,11 +6,6 @@
 
 #define ZERO(x) memset(&x, 0, sizeof(x))
 
-static int os_strcmp(const char* s1, const char* s2) {
-    size_t size = strlen(s1) + 1;
-    return memcmp(s1, s2, size);
-}
-
 int handle_check_address(check_address_parameters_t* params, chain_config_t* chain_config) {
     PRINTF("Params on the address %d\n", (unsigned int) params);
     PRINTF("Address to check %s\n", params->address_to_check);
@@ -69,7 +64,7 @@ int handle_check_address(check_address_parameters_t* params, chain_config_t* cha
         offset_0x = 2;
     }
 
-    if (os_strcmp(locals_union1.address, params->address_to_check + offset_0x) != 0) {
+    if (strcmp(locals_union1.address, params->address_to_check + offset_0x) != 0) {
         PRINTF("Addresses don't match\n");
         return 0;
     }

src/handle_swap_sign_transaction.c

@@ -1,6 +1,6 @@
-#include "handle_swap_sign_transaction.h"
-#include "usbd_core.h"
+#include "os_io_seproxyhal.h"
 #include "ux.h"
+#include "handle_swap_sign_transaction.h"
 #include "shared_context.h"
 #include "utils.h"
 
@@ -79,4 +79,4 @@ void handle_swap_sign_transaction(chain_config_t* config) {
     BLE_power(1, "Nano X");
 #endif  // HAVE_BLE
     app_main();
-}
+}

src/main.c

@@ -386,7 +386,8 @@ extraInfo_t *getKnownToken(uint8_t *contractAddress) {
         }
     }
 #endif
-    //
+    // Works for ERC-20 & NFT tokens since both structs in the union have the
+    // contract address aligned
     for (uint8_t i = 0; i < MAX_ITEMS; i++) {
         currentItem = (union extraInfo_t *) &tmpCtx.transactionContext.extraInfo[i].token;
         if (tmpCtx.transactionContext.tokenSet[i] &&
@@ -396,15 +397,6 @@ extraInfo_t *getKnownToken(uint8_t *contractAddress) {
         }
     }
 
-    for (uint8_t i = 0; i < MAX_ITEMS; i++) {
-        currentItem = (union extraInfo_t *) &tmpCtx.transactionContext.extraInfo[i].token;
-        if (tmpCtx.transactionContext.tokenSet[i] &&
-            (memcmp(currentItem->nft.contractAddress, contractAddress, ADDRESS_LENGTH) == 0)) {
-            PRINTF("Token found at index %d\n", i);
-            return currentItem;
-        }
-    }
-
     return NULL;
 }