Skip to content

Commit

Permalink
Review
Browse files Browse the repository at this point in the history
  • Loading branch information
fbeutin-ledger committed Dec 19, 2024
1 parent aa6c38e commit 7fdfc6a
Show file tree
Hide file tree
Showing 12 changed files with 71 additions and 38 deletions.
7 changes: 1 addition & 6 deletions src/get_challenge_handler.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@

static uint32_t challenge;

#define LAST_BYTE_MASK 0x000000FF

/**
* Generate a new challenge from the Random Number Generator
*/
Expand All @@ -35,10 +33,7 @@ uint32_t get_challenge(void) {
int get_challenge_handler(void) {
PRINTF("New challenge -> %u\n", challenge);
uint8_t output_buffer[4];
output_buffer[0] = (uint8_t) ((challenge >> 24) & LAST_BYTE_MASK);
output_buffer[1] = (uint8_t) ((challenge >> 16) & LAST_BYTE_MASK);
output_buffer[2] = (uint8_t) ((challenge >> 8) & LAST_BYTE_MASK);
output_buffer[3] = (uint8_t) (challenge & LAST_BYTE_MASK);
U4BE_ENCODE(output_buffer, 0, challenge);

buffer_t output;
output.ptr = output_buffer;
Expand Down
8 changes: 7 additions & 1 deletion src/pki.c
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
swap_error_e check_signature_with_pki(const uint8_t *buffer,
uint8_t buffer_length,
uint8_t expected_key_usage,
cx_curve_t expected_curve,
const cbuf_t *signature) {
uint8_t key_usage = 0;
size_t certificate_name_len = 0;
Expand All @@ -21,7 +22,12 @@ swap_error_e check_signature_with_pki(const uint8_t *buffer,

if (key_usage != expected_key_usage) {
PRINTF("Wrong usage certificate %d, expected %d\n", key_usage, expected_key_usage);
return WRONG_CERTIFICATE_KEY_USAGE;
return WRONG_CERTIFICATE_DATA;
}

if (public_key.curve != expected_curve) {
PRINTF("Wrong curve %d, expected %d\n", public_key.curve, expected_curve);
return WRONG_CERTIFICATE_DATA;
}

PRINTF("Certificate '%s' loaded with success\n", certificate_name);
Expand Down
1 change: 1 addition & 0 deletions src/pki.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,5 @@
swap_error_e check_signature_with_pki(const uint8_t *buffer,
uint8_t buffer_length,
uint8_t expected_key_usage,
cx_curve_t expected_curve,
const cbuf_t *signature);
2 changes: 1 addition & 1 deletion src/swap_errors.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ typedef enum {
WRONG_TLV_KEY_ID = 0x6A92,
WRONG_TLV_SIGNATURE = 0x6A93,
NO_CERTIFICATE_LOADED = 0x6A94,
WRONG_CERTIFICATE_KEY_USAGE = 0x6A95,
WRONG_CERTIFICATE_DATA = 0x6A95,
CLASS_NOT_SUPPORTED = 0x6E00,
MALFORMED_APDU = 0x6E01,
INVALID_DATA_LENGTH = 0x6E02,
Expand Down
56 changes: 35 additions & 21 deletions src/tlv_utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
#define DER_LONG_FORM_FLAG 0x80 // 8th bit set
#define DER_FIRST_BYTE_VALUE_MASK 0x7f

bool get_uint_from_tlv_data(const tlv_data_t *data, uint32_t *value) {
bool get_uint32_from_tlv_data(const tlv_data_t *data, uint32_t *value) {
uint8_t size_diff;
uint8_t buffer[sizeof(uint32_t)];

Expand All @@ -18,20 +18,29 @@ bool get_uint_from_tlv_data(const tlv_data_t *data, uint32_t *value) {
return true;
}

bool get_uint16_t_from_tlv_data(const tlv_data_t *data, uint16_t *value) {
uint32_t tmp_value;
if (!get_uint32_from_tlv_data(data, &tmp_value) || (tmp_value > UINT16_MAX)) {
return false;
}
*value = (uint16_t) tmp_value;
return true;
}

bool get_uint8_t_from_tlv_data(const tlv_data_t *data, uint8_t *value) {
uint32_t tmp_value;
if (!get_uint_from_tlv_data(data, &tmp_value) || (tmp_value > UINT8_MAX)) {
if (!get_uint32_from_tlv_data(data, &tmp_value) || (tmp_value > UINT8_MAX)) {
return false;
}
*value = tmp_value;
*value = (uint8_t) tmp_value;
return true;
}

bool get_cbuf_from_tlv_data(const tlv_data_t *data,
cbuf_t *out,
uint16_t min_size,
uint16_t max_size) {
if (data->length < min_size) {
if (min_size != 0 && data->length < min_size) {
PRINTF("Expected at least %d bytes, found %D\n", min_size, data->length);
return false;
}
Expand All @@ -54,7 +63,7 @@ bool get_cbuf_from_tlv_data(const tlv_data_t *data,
* @param[out] value the parsed value
* @return whether it was successful
*/
bool parse_der_value(const buf_t *payload, size_t *offset, uint32_t *value) {
static bool get_der_value_as_uint32(const buf_t *payload, size_t *offset, uint32_t *value) {
bool ret = false;
uint8_t byte_length;
uint8_t buf[sizeof(*value)];
Expand Down Expand Up @@ -87,31 +96,36 @@ bool parse_der_value(const buf_t *payload, size_t *offset, uint32_t *value) {
return ret;
}

/**
* Get DER-encoded value as an uint8
*
* Parses the value and checks if it fits in the given \ref uint8_t value
*
* @param[in] payload the TLV payload
* @param[in,out] offset
* @param[out] value the parsed value
* @return whether it was successful
*/
bool get_der_value_as_uint8(const buf_t *payload, size_t *offset, uint8_t *value) {
static bool get_der_value_as_uint16(const buf_t *payload, size_t *offset, uint16_t *value) {
uint32_t tmp_value;
if (!parse_der_value(payload, offset, &tmp_value)) {
if (!get_der_value_as_uint32(payload, offset, &tmp_value) || (tmp_value > UINT16_MAX)) {
return false;
}

if (tmp_value > UINT8_MAX) {
PRINTF("TLV DER-encoded value %d does not fit in uint8_t\n", tmp_value);
*value = (uint16_t) tmp_value;
return true;
}

static bool get_der_value_as_uint8(const buf_t *payload, size_t *offset, uint8_t *value) {
uint32_t tmp_value;
if (!get_der_value_as_uint32(payload, offset, &tmp_value) || (tmp_value > UINT8_MAX)) {
return false;
}

*value = (uint8_t) tmp_value;
return true;
}

// clang-format off
// Use _Generic keyword to perform a compile-time switch / case depending on value actual type
#define GET_DER_VALUE_AS_UINTX(payload, offset, value) \
_Generic((value), \
uint8_t *: get_der_value_as_uint8, \
uint16_t *: get_der_value_as_uint16, \
uint32_t *: get_der_value_as_uint32 \
)(payload, offset, value)
// clang-format on

// Some Xmacro dark magic to assign to each tag_FLAG its flag value
// Used by the function below that maps TAGS to FLAGS
typedef enum tlv_rcv_bit_e {
Expand Down Expand Up @@ -202,14 +216,14 @@ bool parse_tlv(const tlv_handler_t handlers[TLV_COUNT],
switch (step) {
case TLV_TAG:
tag_start_offset = offset;
if (!get_der_value_as_uint8(payload, &offset, &data.tag)) {
if (!GET_DER_VALUE_AS_UINTX(payload, &offset, &data.tag)) {
return false;
}
step = TLV_LENGTH;
break;

case TLV_LENGTH:
if (!get_der_value_as_uint8(payload, &offset, &data.length)) {
if (!get_der_value_as_uint16(payload, &offset, &data.length)) {
return false;
}
step = TLV_VALUE;
Expand Down
23 changes: 19 additions & 4 deletions src/tlv_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ typedef enum tlv_tag_count_e {
// The received TLV data that will be fed to each handler
typedef struct tlv_data_s {
tlv_tag_t tag;
uint8_t length;
uint16_t length;
const uint8_t *value;
} tlv_data_t;

Expand All @@ -79,13 +79,28 @@ typedef struct {
* @param[out] value Pointer to a uint32_t where the result will be stored
* @return True if the extraction was successful, false otherwise (invalid length or data)
*/
bool get_uint_from_tlv_data(const tlv_data_t *data, uint32_t *value);
bool get_uint32_from_tlv_data(const tlv_data_t *data, uint32_t *value);

/**
* Get uint16_t from tlv data
*
* This function extracts a uint16_t value from the TLV data by calling `get_uint32_from_tlv_data`.
* The extracted value is then checked to ensure it fits within the uint16_t range.
*
* If the value is too large (greater than `UINT16_MAX`), the extraction fails.
*
* @param[in] data The TLV data containing the value to be extracted
* @param[out] value Pointer to a uint16_t where the result will be stored
* @return True if the extraction was successful, false otherwise (either failed extraction or
* out-of-range value)
*/
bool get_uint16_t_from_tlv_data(const tlv_data_t *data, uint16_t *value);

/**
* Get uint8_t from tlv data
*
* This function extracts a uint8_t value from the TLV data by calling `get_uint_from_tlv_data`.
* The extracted value is then checked to ensure it fits within the uint8_t range (0-255).
* This function extracts a uint8_t value from the TLV data by calling `get_uint32_from_tlv_data`.
* The extracted value is then checked to ensure it fits within the uint8_t range.
*
* If the value is too large (greater than `UINT8_MAX`), the extraction fails.
*
Expand Down
6 changes: 3 additions & 3 deletions src/trusted_name_descriptor_handler.c
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ static bool handle_struct_version(const tlv_data_t *data, tlv_out_t *trusted_nam
}

static bool handle_challenge(const tlv_data_t *data, tlv_out_t *trusted_name_info) {
return get_uint_from_tlv_data(data, &trusted_name_info->challenge);
return get_uint32_from_tlv_data(data, &trusted_name_info->challenge);
}

static bool handle_sign_key_id(const tlv_data_t *data, tlv_out_t *trusted_name_info) {
Expand Down Expand Up @@ -195,8 +195,7 @@ static int trusted_name_descriptor_handler_internal(const command_t *cmd) {
uint32_t received_tags_flags = 0;

// The parser will fill it with the hash of the whole TLV payload (except SIGN tag)
uint8_t tlv_hash[INT256_LENGTH];
memset(&tlv_hash, 0, sizeof(tlv_hash));
uint8_t tlv_hash[INT256_LENGTH] = {0};

// Mapping of tags to handler functions. Given ot the parser
tlv_handler_t handlers[TLV_COUNT] = {
Expand Down Expand Up @@ -236,6 +235,7 @@ static int trusted_name_descriptor_handler_internal(const command_t *cmd) {
ret = check_signature_with_pki(tlv_hash,
INT256_LENGTH,
CERTIFICATE_PUBLIC_KEY_USAGE_TRUSTED_NAME,
CX_CURVE_SECP256K1,
&trusted_name_info.input_sig);
if (ret != SUCCESS) {
PRINTF("Failed to verify signature of trusted name info\n");
Expand Down
Binary file removed test/.DS_Store
Binary file not shown.
Binary file removed test/python/.DS_Store
Binary file not shown.
2 changes: 1 addition & 1 deletion test/python/apps/cal.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def get_conf_for_ticker(self, overload_signer: Optional[SigningAuthority]=None)
USDD_CURRENCY_CONFIGURATION = CurrencyConfiguration(ticker="USDD", conf=TRX_USDD_CONF, packed_derivation_path=TRX_PACKED_DERIVATION_PATH)
ADA_BYRON_CURRENCY_CONFIGURATION = CurrencyConfiguration(ticker="ADA", conf=ADA_CONF, packed_derivation_path=ADA_BYRON_PACKED_DERIVATION_PATH)
ADA_SHELLEY_CURRENCY_CONFIGURATION = CurrencyConfiguration(ticker="ADA", conf=ADA_CONF, packed_derivation_path=ADA_SHELLEY_PACKED_DERIVATION_PATH)
JUP_CURRENCY_CONFIGURATION = CurrencyConfiguration(ticker="JUP", conf=JUP_CONF, packed_derivation_path=JUP_PACKED_DERIVATION_PATH)
# JUP_CURRENCY_CONFIGURATION = CurrencyConfiguration(ticker="JUP", conf=JUP_CONF, packed_derivation_path=JUP_PACKED_DERIVATION_PATH)


# Helper that can be called from outside if we want to generate errors easily
Expand Down
2 changes: 1 addition & 1 deletion test/python/apps/exchange.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ class Errors(IntEnum):
WRONG_TLV_KEY_ID = 0x6A92
WRONG_TLV_SIGNATURE = 0x6A93
NO_CERTIFICATE_LOADED = 0x6A94
WRONG_CERTIFICATE_KEY_USAGE = 0x6A95
WRONG_CERTIFICATE_DATA = 0x6A95
CLASS_NOT_SUPPORTED = 0x6E00
MALFORMED_APDU = 0x6E01
INVALID_DATA_LENGTH = 0x6E02
Expand Down
2 changes: 2 additions & 0 deletions test/python/test_trusted_name.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
}
FEES = 100


class TestTrustedName:

@pytest.mark.parametrize("subcommand", [SubCommand.SWAP, SubCommand.SWAP_NG])
Expand Down Expand Up @@ -216,6 +217,7 @@ def test_trusted_name_wrong_field(self, backend, subcommand):
ex.send_pki_certificate_and_trusted_name_descriptor(challenge=challenge, address=value)
assert e.value.status == Errors.WRONG_TLV_FORMAT


# Needs to be in a separate Speculos start
class TestTrustedNameNoCertificate:

Expand Down

0 comments on commit 7fdfc6a

Please sign in to comment.