Merge pull request #6 from Zondax/feat/sig-format

Feat/sig format
Zondax · Jan 26, 2024 · 732871b · 732871b
2 parents 5df59f0 + 42d85e0
commit 732871b
Show file tree

Hide file tree

Showing 24 changed files with 155 additions and 130 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -41,18 +41,18 @@ if(ENABLE_FUZZING)
         message(FATAL_ERROR "Fuzz logging enabled")
     endif()
 
-    set(CMAKE_CXX_CLANG_TIDY clang-tidy -checks=-*,bugprone-*,cert-*,clang-analyzer-*,-cert-err58-cpp,misc-*)
+    set(CMAKE_CXX_CLANG_TIDY clang-tidy -checks=-*,bugprone-*,cert-*,clang-analyzer-*,-cert-err58-cpp,misc-*,-bugprone-suspicious-include)
 
     if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
         # require at least clang 3.2
-        if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS 10.0)
-            message(FATAL_ERROR "Clang version must be at least 10.0!")
+        if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS 11.0)
+            message(FATAL_ERROR "Clang version must be at least 11.0!")
         endif()
     else()
         message(FATAL_ERROR
-            "You are using an unsupported compiler! Fuzzing only works with Clang 10.\n"
-            "1. Install clang-10 \n"
-            "2. Pass -DCMAKE_C_COMPILER=clang-10 -DCMAKE_CXX_COMPILER=clang++-10")
+            "You are using an unsupported compiler! Fuzzing only works with Clang 11.\n"
+            "1. Install clang-11 \n"
+            "2. Pass -DCMAKE_C_COMPILER=clang-10 -DCMAKE_CXX_COMPILER=clang++-11")
     endif()
 
     string(APPEND CMAKE_C_FLAGS " -fsanitize=fuzzer-no-link")

diff --git a/app/Makefile.version b/app/Makefile.version
@@ -3,4 +3,4 @@ APPVERSION_M=0
 # This is the minor version
 APPVERSION_N=0
 # This is the patch version
-APPVERSION_P=7
+APPVERSION_P=8
diff --git a/app/src/crypto.c b/app/src/crypto.c
@@ -70,6 +70,17 @@ __Z_INLINE zxerr_t compressPubkey(const uint8_t *pubkey, uint16_t pubkeyLen, uin
     return zxerr_ok;
 }
 
+typedef struct {
+    uint8_t r[32];
+    uint8_t s[32];
+    uint8_t v;
+
+    // DER signature max size should be 73
+    // https://bitcoin.stackexchange.com/questions/77191/what-is-the-maximum-size-of-a-der-encoded-ecdsa-signature#77192
+    uint8_t der_signature[73];
+
+} __attribute__((packed)) signature_t;
+
 zxerr_t crypto_sign(uint8_t *signature, uint16_t signatureMaxlen, uint16_t *sigSize, bool hash) {
     if (signature == NULL || sigSize == NULL) {
         return zxerr_invalid_crypto_settings;
@@ -89,7 +100,10 @@ zxerr_t crypto_sign(uint8_t *signature, uint16_t signatureMaxlen, uint16_t *sigS
     cx_ecfp_private_key_t cx_privateKey = {0};
     uint8_t privateKeyData[64] = {0};
     unsigned int info = 0;
-    size_t signatureLength = MAX_DER_SIGNATURE_LEN;
+    uint32_t signatureLength = sizeof_field(signature_t, der_signature);
+    signature_t *const signature_object = (signature_t *)(signature);
+    *sigSize = 0;
+
     zxerr_t error = zxerr_unknown;
 
     // Generate keys
@@ -99,33 +113,27 @@ zxerr_t crypto_sign(uint8_t *signature, uint16_t signatureMaxlen, uint16_t *sigS
 
     // Sign
     CATCH_CXERROR(cx_ecdsa_sign_no_throw(&cx_privateKey, CX_RND_RFC6979 | CX_LAST, CX_SHA256, messageDigest, CX_SHA256_SIZE,
-                                         signature, &signatureLength, &info));
+                                         signature_object->der_signature, &signatureLength, &info));
 
-    *sigSize = signatureLength;
-    error = zxerr_ok;
+    const err_convert_e err_c = convertDERtoRSV(signature_object->der_signature, info, signature_object->r,
+                                                signature_object->s, &signature_object->v);
+    if (err_c != no_error) {
+        error = zxerr_unknown;
+    } else {
+        *sigSize =
+            sizeof_field(signature_t, r) + sizeof_field(signature_t, s) + sizeof_field(signature_t, v) + signatureLength;
+        error = zxerr_ok;
+    }
 
 catch_cx_error:
     MEMZERO(&cx_privateKey, sizeof(cx_privateKey));
     MEMZERO(privateKeyData, sizeof(privateKeyData));
-
     if (error != zxerr_ok) {
         MEMZERO(signature, signatureMaxlen);
     }
-
     return error;
 }
 
-typedef struct {
-    uint8_t r[32];
-    uint8_t s[32];
-    uint8_t v;
-
-    // DER signature max size should be 73
-    // https://bitcoin.stackexchange.com/questions/77191/what-is-the-maximum-size-of-a-der-encoded-ecdsa-signature#77192
-    uint8_t der_signature[73];
-
-} __attribute__((packed)) signature_t;
-
 zxerr_t _sign(uint8_t *output, uint16_t outputLen, const uint8_t *message, uint16_t messageLen, uint16_t *sigSize,
               unsigned int *info) {
     if (output == NULL || message == NULL || sigSize == NULL || outputLen < sizeof(signature_t) ||

diff --git a/app/src/eth_erc20.c b/app/src/eth_erc20.c
@@ -97,7 +97,7 @@ parser_error_t printERC20Value(const rlp_t *data, char *outVal, uint16_t outValL
 
 bool validateERC20(rlp_t data) {
     // Check that data start with ERC20 prefix
-    if (data.rlpLen != ERC20_DATA_LENGTH || memcmp(data.ptr, ERC20_TRANSFER_PREFIX, 4) != 0) {
+    if (data.ptr == NULL || data.rlpLen != ERC20_DATA_LENGTH || memcmp(data.ptr, ERC20_TRANSFER_PREFIX, 4) != 0) {
         return false;
     }
 

diff --git a/app/src/eth_utils.c b/app/src/eth_utils.c
@@ -139,7 +139,7 @@ parser_error_t printRLPNumber(const rlp_t *num, char *outVal, uint16_t outValLen
 }
 
 parser_error_t printEVMAddress(const rlp_t *address, char *outVal, uint16_t outValLen, uint8_t pageIdx, uint8_t *pageCount) {
-    if (address == NULL || outVal == NULL || pageCount == NULL || address->rlpLen != ETH_ADDR_LEN) {
+    if (address == NULL || outVal == NULL || address->ptr == NULL || pageCount == NULL || address->rlpLen != ETH_ADDR_LEN) {
         return parser_unexpected_error;
     }
 

diff --git a/app/src/parser_impl_common.c b/app/src/parser_impl_common.c
@@ -197,6 +197,7 @@ bool parser_output_contains_change_address(parser_context_t *c) {
     bool contains = false;
     // verify address is renderable  compare with CHANGE ADDRESS
 #if defined(TARGET_NANOS) || defined(TARGET_NANOS2) || defined(TARGET_NANOX) || defined(TARGET_STAX)
+    CTX_CHECK_AVAIL(c, ADDRESS_LEN)
     if (MEMCMP(c->buffer + c->offset, change_address, ADDRESS_LEN) == 0) {
         contains = false;
     } else {
@@ -205,6 +206,8 @@ bool parser_output_contains_change_address(parser_context_t *c) {
 #else
     uint8_t test_change_address[ADDRESS_LEN] = {0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa,
                                                 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa};
+
+    CTX_CHECK_AVAIL(c, ADDRESS_LEN)
     if (MEMCMP(c->buffer + c->offset, test_change_address, ADDRESS_LEN) == 0) {
         contains = false;
     } else {

diff --git a/app/src/parser_impl_eth.c b/app/src/parser_impl_eth.c
@@ -41,9 +41,11 @@ static parser_error_t readChainID(parser_context_t *ctx, rlp_t *chainId) {
     uint64_t tmpChainId = 0;
     if (chainId->rlpLen > 0) {
         CHECK_ERROR(be_bytes_to_u64(chainId->ptr, chainId->rlpLen, &tmpChainId))
-    } else {
+    } else if (chainId->kind == RLP_KIND_BYTE) {
         // case were the prefix is the byte itself
         tmpChainId = chainId->ptr[0];
+    } else {
+        return parser_unexpected_error;
     }
 
     // Check allowed values for chain id
@@ -136,6 +138,7 @@ static parser_error_t readTxnType(parser_context_t *ctx, eth_tx_type_e *type) {
 }
 
 parser_error_t _readEth(parser_context_t *ctx, eth_tx_t *tx_obj) {
+    MEMZERO(&eth_tx_obj, sizeof(eth_tx_obj));
     CHECK_ERROR(readTxnType(ctx, &tx_obj->tx_type))
     // We expect a list with all the fields from the transaction
     rlp_t list = {0};

diff --git a/app/src/parser_impl_eth.h b/app/src/parser_impl_eth.h
@@ -73,7 +73,6 @@ parser_error_t _getNumItemsEth(uint8_t *numItems);
 
 parser_error_t _validateTxEth();
 
-// parser_error_t _computeV(unsigned int info, uint8_t *v);
 parser_error_t _computeV(parser_context_t *ctx, eth_tx_t *tx_obj, unsigned int info, uint8_t *v);
 
 #ifdef __cplusplus

diff --git a/app/src/uint256.c b/app/src/uint256.c
@@ -1,28 +1,27 @@
 /*******************************************************************************
-*   Ledger Ethereum App
-*   (c) 2016-2019 Ledger
-*
-*  Licensed under the Apache License, Version 2.0 (the "License");
-*  you may not use this file except in compliance with the License.
-*  You may obtain a copy of the License at
-*
-*      http://www.apache.org/licenses/LICENSE-2.0
-*
-*  Unless required by applicable law or agreed to in writing, software
-*  distributed under the License is distributed on an "AS IS" BASIS,
-*  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-*  See the License for the specific language governing permissions and
-*  limitations under the License.
-********************************************************************************/
+ *   Ledger Ethereum App
+ *   (c) 2016-2019 Ledger
+ *
+ *  Licensed under the Apache License, Version 2.0 (the "License");
+ *  you may not use this file except in compliance with the License.
+ *  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ ********************************************************************************/
 
 // Adapted from https://github.com/calccrypto/uint256_t
 
-#include <stdio.h>
-#include <stdlib.h>
-#include <stdint.h>
-
 #include "uint256.h"
 
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
 
 static const char HEXDIGITS[] = "0123456789abcdef";
 
@@ -61,13 +60,9 @@ parser_error_t readu256BE(parser_context_t *ctx, uint256_t *bigInt) {
     return parser_ok;
 }
 
-bool zero128(uint128_t *number) {
-    return ((LOWER_P(number) == 0) && (UPPER_P(number) == 0));
-}
+bool zero128(uint128_t *number) { return ((LOWER_P(number) == 0) && (UPPER_P(number) == 0)); }
 
-bool zero256(uint256_t *number) {
-    return (zero128(&LOWER_P(number)) && zero128(&UPPER_P(number)));
-}
+bool zero256(uint256_t *number) { return (zero128(&LOWER_P(number)) && zero128(&UPPER_P(number))); }
 
 void copy128(uint128_t *target, uint128_t *number) {
     UPPER_P(target) = UPPER_P(number);
@@ -98,8 +93,7 @@ void shiftl128(uint128_t *number, uint32_t value, uint128_t *target) {
     } else if (value == 0) {
         copy128(target, number);
     } else if (value < 64) {
-        UPPER_P(target) =
-            (UPPER_P(number) << value) + (LOWER_P(number) >> (64 - value));
+        UPPER_P(target) = (UPPER_P(number) << value) + (LOWER_P(number) >> (64 - value));
         LOWER_P(target) = (LOWER_P(number) << value);
     } else if (value > 64) {
         UPPER_P(target) = LOWER_P(number) << (value - 64);
@@ -143,8 +137,7 @@ void shiftr128(uint128_t *number, uint32_t value, uint128_t *target) {
     } else if (value < 64) {
         uint128_t result;
         UPPER(result) = UPPER_P(number) >> value;
-        LOWER(result) =
-            (UPPER_P(number) << (64 - value)) + (LOWER_P(number) >> value);
+        LOWER(result) = (UPPER_P(number) << (64 - value)) + (LOWER_P(number) >> value);
         copy128(target, &result);
     } else if (value > 64) {
         LOWER_P(target) = UPPER_P(number) >> (value - 64);
@@ -220,13 +213,11 @@ uint32_t bits256(uint256_t *number) {
 }
 
 bool equal128(uint128_t *number1, uint128_t *number2) {
-    return (UPPER_P(number1) == UPPER_P(number2)) &&
-           (LOWER_P(number1) == LOWER_P(number2));
+    return (UPPER_P(number1) == UPPER_P(number2)) && (LOWER_P(number1) == LOWER_P(number2));
 }
 
 bool equal256(uint256_t *number1, uint256_t *number2) {
-    return (equal128(&UPPER_P(number1), &UPPER_P(number2)) &&
-            equal128(&LOWER_P(number1), &LOWER_P(number2)));
+    return (equal128(&UPPER_P(number1), &UPPER_P(number2)) && equal128(&LOWER_P(number1), &LOWER_P(number2)));
 }
 
 bool gt128(uint128_t *number1, uint128_t *number2) {
@@ -243,18 +234,12 @@ bool gt256(uint256_t *number1, uint256_t *number2) {
     return gt128(&UPPER_P(number1), &UPPER_P(number2));
 }
 
-bool gte128(uint128_t *number1, uint128_t *number2) {
-    return gt128(number1, number2) || equal128(number1, number2);
-}
+bool gte128(uint128_t *number1, uint128_t *number2) { return gt128(number1, number2) || equal128(number1, number2); }
 
-bool gte256(uint256_t *number1, uint256_t *number2) {
-    return gt256(number1, number2) || equal256(number1, number2);
-}
+bool gte256(uint256_t *number1, uint256_t *number2) { return gt256(number1, number2) || equal256(number1, number2); }
 
 void add128(uint128_t *number1, uint128_t *number2, uint128_t *target) {
-    UPPER_P(target) =
-        UPPER_P(number1) + UPPER_P(number2) +
-        ((LOWER_P(number1) + LOWER_P(number2)) < LOWER_P(number1));
+    UPPER_P(target) = UPPER_P(number1) + UPPER_P(number2) + ((LOWER_P(number1) + LOWER_P(number2)) < LOWER_P(number1));
     LOWER_P(target) = LOWER_P(number1) + LOWER_P(number2);
 }
 
@@ -272,9 +257,7 @@ void add256(uint256_t *number1, uint256_t *number2, uint256_t *target) {
 }
 
 void minus128(uint128_t *number1, uint128_t *number2, uint128_t *target) {
-    UPPER_P(target) =
-        UPPER_P(number1) - UPPER_P(number2) -
-        ((LOWER_P(number1) - LOWER_P(number2)) > LOWER_P(number1));
+    UPPER_P(target) = UPPER_P(number1) - UPPER_P(number2) - ((LOWER_P(number1) - LOWER_P(number2)) > LOWER_P(number1));
     LOWER_P(target) = LOWER_P(number1) - LOWER_P(number2);
 }
 
@@ -302,10 +285,9 @@ void or256(uint256_t *number1, uint256_t *number2, uint256_t *target) {
 }
 
 void mul128(uint128_t *number1, uint128_t *number2, uint128_t *target) {
-    uint64_t top[4] = {UPPER_P(number1) >> 32, UPPER_P(number1) & 0xffffffff,
-                       LOWER_P(number1) >> 32, LOWER_P(number1) & 0xffffffff};
-    uint64_t bottom[4] = {UPPER_P(number2) >> 32, UPPER_P(number2) & 0xffffffff,
-                          LOWER_P(number2) >> 32,
+    uint64_t top[4] = {UPPER_P(number1) >> 32, UPPER_P(number1) & 0xffffffff, LOWER_P(number1) >> 32,
+                       LOWER_P(number1) & 0xffffffff};
+    uint64_t bottom[4] = {UPPER_P(number2) >> 32, UPPER_P(number2) & 0xffffffff, LOWER_P(number2) >> 32,
                           LOWER_P(number2) & 0xffffffff};
     uint64_t products[4][4];
     uint128_t tmp, tmp2;
@@ -437,8 +419,7 @@ void mul256(uint256_t *number1, uint256_t *number2, uint256_t *target) {
     add256(&target1, &target2, target);
 }
 
-void divmod128(uint128_t *l, uint128_t *r, uint128_t *retDiv,
-               uint128_t *retMod) {
+void divmod128(uint128_t *l, uint128_t *r, uint128_t *retDiv, uint128_t *retMod) {
     uint128_t copyd, adder, resDiv, resMod;
     uint128_t one;
     UPPER(one) = 0;
@@ -469,8 +450,7 @@ void divmod128(uint128_t *l, uint128_t *r, uint128_t *retDiv,
     }
 }
 
-void divmod256(uint256_t *l, uint256_t *r, uint256_t *retDiv,
-               uint256_t *retMod) {
+void divmod256(uint256_t *l, uint256_t *r, uint256_t *retDiv, uint256_t *retMod) {
     uint256_t copyd, adder, resDiv, resMod;
     uint256_t one;
     clear256(&one);
@@ -512,8 +492,11 @@ static void reverseString(char *str, uint32_t length) {
     }
 }
 
-bool tostring128(uint128_t *number, uint32_t baseParam, char *out,
-                 uint32_t outLength) {
+bool tostring128(uint128_t *number, uint32_t baseParam, char *out, uint32_t outLength) {
+    if (number == NULL || out == NULL || outLength == 0) {
+        return false;
+    }
+
     uint128_t rDiv;
     uint128_t rMod;
     uint128_t base;
@@ -532,13 +515,18 @@ bool tostring128(uint128_t *number, uint32_t baseParam, char *out,
         divmod128(&rDiv, &base, &rDiv, &rMod);
         out[offset++] = HEXDIGITS[(uint8_t)LOWER(rMod)];
     } while (!zero128(&rDiv));
+    if (offset >= outLength) {
+        return false;
+    }
     out[offset] = '\0';
     reverseString(out, offset);
     return true;
 }
 
-bool tostring256(uint256_t *number, uint32_t baseParam, char *out,
-                 uint32_t outLength) {
+bool tostring256(uint256_t *number, uint32_t baseParam, char *out, uint32_t outLength) {
+    if (number == NULL || out == NULL || outLength <= 1) {
+        return false;
+    }
     uint256_t rDiv;
     uint256_t rMod;
     uint256_t base;
@@ -552,7 +540,7 @@ bool tostring256(uint256_t *number, uint32_t baseParam, char *out,
         return false;
     }
 
-    outLength--;    // Keep a byte for termination
+    outLength--;  // Keep a byte for termination
 
     do {
         if (offset > (outLength - 1)) {