From ead85a0aaa45755e2068b1ea33363a517020c345 Mon Sep 17 00:00:00 2001 From: Jorge Martins Date: Wed, 2 Nov 2022 13:34:26 +0100 Subject: [PATCH 1/6] Add funcs to avoid tricking user when using plugin Usually the length of an array is sent in a parameter. Most of the times the developer simply uses U2BE/U4BE to get this length. It is possible to forge a tx with a `length > sizeof(uint16_t/uint32_t)` and trick the user into signing something different from what is shown. For instance consider the following parameter: 00 ... 01 00 00 00 01 if the developer uses U2BE/U4BE, it is possible that this length is shown to the user and if it is, the user will see the length as 1. --- doc/ethapp_plugins.adoc | 11 +++++++++++ src/eth_plugin_internal.c | 19 +++++++++++++++++++ src/eth_plugin_internal.h | 4 ++++ tools/build_sdk.py | 2 +- 4 files changed, 35 insertions(+), 1 deletion(-) diff --git a/doc/ethapp_plugins.adoc b/doc/ethapp_plugins.adoc index af89087..61f6e0d 100644 --- a/doc/ethapp_plugins.adoc +++ b/doc/ethapp_plugins.adoc @@ -134,6 +134,17 @@ The following return codes are expected, any other will abort the signing proces * ETH_PLUGIN_RESULT_OK : if the plugin can be successfully initialized * ETH_PLUGIN_RESULT_FALLBACK : if the signing logic should fallback to the generic one +There are already defined functions to extract data from a parameter: +[source,C] +---- +void copy_address(uint8_t* dst, const uint8_t* parameter, uint8_t dst_size); +void copy_parameter(uint8_t* dst, const uint8_t* parameter, uint8_t dst_size); + +// Get the value from the beginning of the parameter (right to left) and check if the rest of it is zero +bool U2BE_from_parameter(uint8_t* parameter, uint16_t* value); +bool U4BE_from_parameter(uint8_t* parameter, uint32_t* value); +---- + ### ETH_PLUGIN_FINALIZE [source,C] diff --git a/src/eth_plugin_internal.c b/src/eth_plugin_internal.c index b266969..b608177 100644 --- a/src/eth_plugin_internal.c +++ b/src/eth_plugin_internal.c @@ -1,5 +1,6 @@ #include #include "eth_plugin_internal.h" +#include "ethUtils.h" // allzeroes bool erc20_plugin_available_check(void); @@ -15,6 +16,24 @@ void copy_parameter(uint8_t* dst, const uint8_t* parameter, uint8_t dst_size) { memmove(dst, parameter, copy_size); } +bool U2BE_from_parameter(uint8_t* parameter, uint16_t* value) { + if (allzeroes(parameter, PARAMETER_LENGTH - sizeof(uint16_t))) { + *value = U2BE(parameter, PARAMETER_LENGTH - sizeof(uint16_t)); + return true; + } + + return false; +} + +bool U4BE_from_parameter(uint8_t* parameter, uint32_t* value) { + if (allzeroes(parameter, PARAMETER_LENGTH - sizeof(uint32_t))) { + *value = U4BE(parameter, PARAMETER_LENGTH - sizeof(uint32_t)); + return true; + } + + return false; +} + #ifdef HAVE_STARKWARE void starkware_plugin_call(int message, void* parameters); #endif diff --git a/src/eth_plugin_internal.h b/src/eth_plugin_internal.h index 1ad8382..3d7ee55 100644 --- a/src/eth_plugin_internal.h +++ b/src/eth_plugin_internal.h @@ -16,6 +16,10 @@ void copy_parameter(uint8_t* dst, const uint8_t* parameter, uint8_t dst_size); void erc721_plugin_call(int message, void* parameters); void erc1155_plugin_call(int message, void* parameters); +// Get the value from the beginning of the parameter (right to left) and check if the rest of it is zero +bool U2BE_from_parameter(uint8_t* parameter, uint16_t* value); +bool U4BE_from_parameter(uint8_t* parameter, uint32_t* value); + typedef bool (*PluginAvailableCheck)(void); typedef struct internalEthPlugin_t { diff --git a/tools/build_sdk.py b/tools/build_sdk.py index aab3a6e..b68b368 100755 --- a/tools/build_sdk.py +++ b/tools/build_sdk.py @@ -164,7 +164,7 @@ if __name__ == "__main__": "typedef union": ["extraInfo_t"], "__attribute__((no_instrument_function)) inline": ["int allzeroes"], "const": ["HEXDIGITS"], - "fn": ["void getEthAddressStringFromBinary", "void getEthAddressFromKey", "void getEthDisplayableAddress", "bool adjustDecimals", "bool uint256_to_decimal", "void amountToString", "void u64_to_string", "void copy_address", "void copy_parameter"] + "fn": ["void getEthAddressStringFromBinary", "void getEthAddressFromKey", "void getEthDisplayableAddress", "bool adjustDecimals", "bool uint256_to_decimal", "void amountToString", "void u64_to_string", "void copy_address", "void copy_parameter", "bool U2BE_from_parameter", "U4BE_from_parameter"] } merge_headers(headers_to_merge, nodes_to_extract) From b120fc6565e4e7263db6e47fcf98c94bda8eb2bd Mon Sep 17 00:00:00 2001 From: Jorge Martins Date: Tue, 8 Nov 2022 09:56:00 +0100 Subject: [PATCH 2/6] fix code style --- src/eth_plugin_internal.c | 2 +- src/eth_plugin_internal.h | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/eth_plugin_internal.c b/src/eth_plugin_internal.c index b608177..40a6f70 100644 --- a/src/eth_plugin_internal.c +++ b/src/eth_plugin_internal.c @@ -1,6 +1,6 @@ #include #include "eth_plugin_internal.h" -#include "ethUtils.h" // allzeroes +#include "ethUtils.h" // allzeroes bool erc20_plugin_available_check(void); diff --git a/src/eth_plugin_internal.h b/src/eth_plugin_internal.h index 3d7ee55..cb5521f 100644 --- a/src/eth_plugin_internal.h +++ b/src/eth_plugin_internal.h @@ -16,7 +16,8 @@ void copy_parameter(uint8_t* dst, const uint8_t* parameter, uint8_t dst_size); void erc721_plugin_call(int message, void* parameters); void erc1155_plugin_call(int message, void* parameters); -// Get the value from the beginning of the parameter (right to left) and check if the rest of it is zero +// Get the value from the beginning of the parameter (right to left) and check if the rest of it is +// zero bool U2BE_from_parameter(uint8_t* parameter, uint16_t* value); bool U4BE_from_parameter(uint8_t* parameter, uint32_t* value); From 51db776de6c3f8611c6b17152497e9ad4b6e8087 Mon Sep 17 00:00:00 2001 From: Jorge Martins Date: Tue, 8 Nov 2022 09:57:24 +0100 Subject: [PATCH 3/6] add const to parameters --- src/eth_plugin_internal.c | 4 ++-- src/eth_plugin_internal.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/eth_plugin_internal.c b/src/eth_plugin_internal.c index 40a6f70..075f2ec 100644 --- a/src/eth_plugin_internal.c +++ b/src/eth_plugin_internal.c @@ -16,7 +16,7 @@ void copy_parameter(uint8_t* dst, const uint8_t* parameter, uint8_t dst_size) { memmove(dst, parameter, copy_size); } -bool U2BE_from_parameter(uint8_t* parameter, uint16_t* value) { +bool U2BE_from_parameter(const uint8_t* parameter, uint16_t* value) { if (allzeroes(parameter, PARAMETER_LENGTH - sizeof(uint16_t))) { *value = U2BE(parameter, PARAMETER_LENGTH - sizeof(uint16_t)); return true; @@ -25,7 +25,7 @@ bool U2BE_from_parameter(uint8_t* parameter, uint16_t* value) { return false; } -bool U4BE_from_parameter(uint8_t* parameter, uint32_t* value) { +bool U4BE_from_parameter(const uint8_t* parameter, uint32_t* value) { if (allzeroes(parameter, PARAMETER_LENGTH - sizeof(uint32_t))) { *value = U4BE(parameter, PARAMETER_LENGTH - sizeof(uint32_t)); return true; diff --git a/src/eth_plugin_internal.h b/src/eth_plugin_internal.h index cb5521f..692c9b6 100644 --- a/src/eth_plugin_internal.h +++ b/src/eth_plugin_internal.h @@ -18,8 +18,8 @@ void erc1155_plugin_call(int message, void* parameters); // Get the value from the beginning of the parameter (right to left) and check if the rest of it is // zero -bool U2BE_from_parameter(uint8_t* parameter, uint16_t* value); -bool U4BE_from_parameter(uint8_t* parameter, uint32_t* value); +bool U2BE_from_parameter(const uint8_t* parameter, uint16_t* value); +bool U4BE_from_parameter(const uint8_t* parameter, uint32_t* value); typedef bool (*PluginAvailableCheck)(void); From 65d2c88f2da174935c4ac1333fe395ee969d78d7 Mon Sep 17 00:00:00 2001 From: Jorge Martins Date: Tue, 8 Nov 2022 10:22:26 +0100 Subject: [PATCH 4/6] update docs --- doc/ethapp_plugins.adoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/ethapp_plugins.adoc b/doc/ethapp_plugins.adoc index 61f6e0d..a3bdbbe 100644 --- a/doc/ethapp_plugins.adoc +++ b/doc/ethapp_plugins.adoc @@ -141,8 +141,8 @@ void copy_address(uint8_t* dst, const uint8_t* parameter, uint8_t dst_size); void copy_parameter(uint8_t* dst, const uint8_t* parameter, uint8_t dst_size); // Get the value from the beginning of the parameter (right to left) and check if the rest of it is zero -bool U2BE_from_parameter(uint8_t* parameter, uint16_t* value); -bool U4BE_from_parameter(uint8_t* parameter, uint32_t* value); +bool U2BE_from_parameter(const uint8_t* parameter, uint16_t* value); +bool U4BE_from_parameter(const uint8_t* parameter, uint32_t* value); ---- ### ETH_PLUGIN_FINALIZE From a49752fe9b76fba51f75afd937d08d6963a3988d Mon Sep 17 00:00:00 2001 From: Jorge Martins Date: Tue, 8 Nov 2022 11:23:14 +0100 Subject: [PATCH 5/6] allzeroes const void* buf --- src_common/ethUtils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src_common/ethUtils.h b/src_common/ethUtils.h index 0755b13..9722e2c 100644 --- a/src_common/ethUtils.h +++ b/src_common/ethUtils.h @@ -66,7 +66,7 @@ bool adjustDecimals(const char *src, size_t targetLength, uint8_t decimals); -static __attribute__((no_instrument_function)) inline int allzeroes(void *buf, size_t n) { +static __attribute__((no_instrument_function)) inline int allzeroes(const void *buf, size_t n) { uint8_t *p = (uint8_t *) buf; for (size_t i = 0; i < n; ++i) { if (p[i]) { From 60fc9c141784976a8f935da4071562f1943ae3ab Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Tue, 15 Nov 2022 18:11:03 +0100 Subject: [PATCH 6/6] Add missing function return type to SDK generation script for consistency --- tools/build_sdk.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/build_sdk.py b/tools/build_sdk.py index b68b368..a808e4a 100755 --- a/tools/build_sdk.py +++ b/tools/build_sdk.py @@ -164,7 +164,7 @@ if __name__ == "__main__": "typedef union": ["extraInfo_t"], "__attribute__((no_instrument_function)) inline": ["int allzeroes"], "const": ["HEXDIGITS"], - "fn": ["void getEthAddressStringFromBinary", "void getEthAddressFromKey", "void getEthDisplayableAddress", "bool adjustDecimals", "bool uint256_to_decimal", "void amountToString", "void u64_to_string", "void copy_address", "void copy_parameter", "bool U2BE_from_parameter", "U4BE_from_parameter"] + "fn": ["void getEthAddressStringFromBinary", "void getEthAddressFromKey", "void getEthDisplayableAddress", "bool adjustDecimals", "bool uint256_to_decimal", "void amountToString", "void u64_to_string", "void copy_address", "void copy_parameter", "bool U2BE_from_parameter", "bool U4BE_from_parameter"] } merge_headers(headers_to_merge, nodes_to_extract)