From b2ec3eff0a52c3baae86d1eb11e146151829f06c Mon Sep 17 00:00:00 2001 From: Jorge Martins <106586648+jmartins-ledger@users.noreply.github.com> Date: Mon, 8 Aug 2022 13:53:41 +0200 Subject: [PATCH] Security review (#331) * Fix some issues * add typed_data.c changes * Make attribution after the check --- src_common/uint128.c | 5 +++++ src_features/signMessageEIP712/context.c | 3 +++ src_features/signMessageEIP712/context.h | 3 +++ src_features/signMessageEIP712/encode_field.c | 5 +++++ src_features/signMessageEIP712/field_hash.c | 5 +++++ src_features/signMessageEIP712/path.c | 7 +++++-- src_features/signMessageEIP712/typed_data.c | 19 +++++++++++++++++-- 7 files changed, 43 insertions(+), 4 deletions(-) diff --git a/src_common/uint128.c b/src_common/uint128.c index 0b1fe86..0663a24 100644 --- a/src_common/uint128.c +++ b/src_common/uint128.c @@ -242,6 +242,11 @@ bool tostring128(const uint128_t *const number, divmod128(&rDiv, &base, &rDiv, &rMod); out[offset++] = HEXDIGITS[(uint8_t) LOWER(rMod)]; } while (!zero128(&rDiv)); + + if (offset > (outLength - 1)) { + return false; + } + out[offset] = '\0'; reverseString(out, offset); return true; diff --git a/src_features/signMessageEIP712/context.c b/src_features/signMessageEIP712/context.c index 38730cb..2c79644 100644 --- a/src_features/signMessageEIP712/context.c +++ b/src_features/signMessageEIP712/context.c @@ -14,6 +14,7 @@ #include "shared_context.h" // reset_app_context #include "ui_callbacks.h" // ui_idle +e_struct_init struct_state = NOT_INITIALIZED; s_eip712_context *eip712_context = NULL; /** @@ -51,6 +52,8 @@ bool eip712_context_init(void) { return false; } + struct_state = NOT_INITIALIZED; + return true; } diff --git a/src_features/signMessageEIP712/context.h b/src_features/signMessageEIP712/context.h index 97e947f..9c2366b 100644 --- a/src_features/signMessageEIP712/context.h +++ b/src_features/signMessageEIP712/context.h @@ -16,6 +16,9 @@ extern s_eip712_context *eip712_context; bool eip712_context_init(void); void eip712_context_deinit(void); +typedef enum {NOT_INITIALIZED, INITIALIZED} e_struct_init; +extern e_struct_init struct_state; + #endif // HAVE_EIP712_FULL_SUPPORT #endif // EIP712_CTX_H_ diff --git a/src_features/signMessageEIP712/encode_field.c b/src_features/signMessageEIP712/encode_field.c index 596315f..4ea2411 100644 --- a/src_features/signMessageEIP712/encode_field.c +++ b/src_features/signMessageEIP712/encode_field.c @@ -74,6 +74,11 @@ void *encode_uint(const uint8_t *const value, uint8_t length) { void *encode_int(const uint8_t *const value, uint8_t length, uint8_t typesize) { uint8_t padding_value; + if (length < 1) { + apdu_response_code = APDU_RESPONSE_INVALID_DATA; + return NULL; + } + if ((length == typesize) && (value[0] & (1 << 7))) // negative number { padding_value = 0xFF; diff --git a/src_features/signMessageEIP712/field_hash.c b/src_features/signMessageEIP712/field_hash.c index b3799d3..bd079b4 100644 --- a/src_features/signMessageEIP712/field_hash.c +++ b/src_features/signMessageEIP712/field_hash.c @@ -254,6 +254,11 @@ bool field_hash(const uint8_t *data, uint8_t data_length, bool partial) { field_type = struct_field_type(field_ptr); if (fh->state == FHS_IDLE) // first packet for this frame { + if (data_length < 2) { + apdu_response_code = APDU_RESPONSE_INVALID_DATA; + return false; + } + data = field_hash_prepare(field_ptr, data, &data_length); } if (data_length > fh->remaining_size) { diff --git a/src_features/signMessageEIP712/path.c b/src_features/signMessageEIP712/path.c index f3299cc..b4dd32b 100644 --- a/src_features/signMessageEIP712/path.c +++ b/src_features/signMessageEIP712/path.c @@ -21,10 +21,10 @@ static s_path *path_struct = NULL; * * @param[out] fields_count_ptr the number of fields in the last evaluated depth * @param[in] n the number of depths to evaluate - * @return the feld which the first Nth depths points to + * @return the field which the first Nth depths points to */ static const void *get_nth_field(uint8_t *const fields_count_ptr, uint8_t n) { - const void *struct_ptr = path_struct->root_struct; + const void *struct_ptr = NULL; const void *field_ptr = NULL; const char *typename; uint8_t length; @@ -33,6 +33,9 @@ static const void *get_nth_field(uint8_t *const fields_count_ptr, uint8_t n) { if (path_struct == NULL) { return NULL; } + + struct_ptr = path_struct->root_struct; + if (n > path_struct->depth_count) // sanity check { return NULL; diff --git a/src_features/signMessageEIP712/typed_data.c b/src_features/signMessageEIP712/typed_data.c index e24886a..a3536fa 100644 --- a/src_features/signMessageEIP712/typed_data.c +++ b/src_features/signMessageEIP712/typed_data.c @@ -360,7 +360,7 @@ const char *get_struct_name(const uint8_t *const struct_ptr, uint8_t *const leng * Get struct fields from a given struct * * @param[in] struct_ptr given struct - * @param[out] length name length + * @param[out] length number of fields * @return struct name */ const uint8_t *get_struct_fields_array(const uint8_t *const struct_ptr, uint8_t *const length) { @@ -475,6 +475,8 @@ bool set_struct_name(uint8_t length, const uint8_t *const name) { return false; } *(typed_data->current_struct_fields_array) = 0; + + struct_state = INITIALIZED; return true; } @@ -494,7 +496,7 @@ static const typedesc_t *set_struct_field_typedesc(const uint8_t *const data, if ((*data_idx + sizeof(*typedesc_ptr)) > length) // check buffer bound { apdu_response_code = APDU_RESPONSE_INVALID_DATA; - return false; + return NULL; } if ((typedesc_ptr = mem_alloc(sizeof(uint8_t))) == NULL) { apdu_response_code = APDU_RESPONSE_INSUFFICIENT_MEMORY; @@ -687,6 +689,11 @@ bool set_struct_field(uint8_t length, const uint8_t *const data) { apdu_response_code = APDU_RESPONSE_CONDITION_NOT_SATISFIED; return false; } + + if (struct_state == NOT_INITIALIZED) { + apdu_response_code = APDU_RESPONSE_CONDITION_NOT_SATISFIED; + return false; + } // increment number of struct fields *(typed_data->current_struct_fields_array) += 1; @@ -696,9 +703,17 @@ bool set_struct_field(uint8_t length, const uint8_t *const data) { // check TypeSize flag in TypeDesc if (*typedesc_ptr & TYPESIZE_MASK) { + + // TYPESIZE and TYPE_CUSTOM are mutually exclusive + if ((*typedesc_ptr & TYPE_MASK) == TYPE_CUSTOM) { + apdu_response_code = APDU_RESPONSE_CONDITION_NOT_SATISFIED; + return false; + } + if (set_struct_field_typesize(data, &data_idx, length) == false) { return false; } + } else if ((*typedesc_ptr & TYPE_MASK) == TYPE_CUSTOM) { if (set_struct_field_custom_typename(data, &data_idx, length) == false) { return false;