From eed7f310db8155f466e9062513515f84e51727e7 Mon Sep 17 00:00:00 2001 From: Francois Beutin Date: Tue, 1 Aug 2023 17:56:08 +0200 Subject: [PATCH] wip --- src/command_dispatcher.c | 25 +---- src/commands.h | 17 ++-- src/globals.h | 17 +--- src/main.c | 114 ++++++++++++++++++----- src/states.h | 13 ++- src/swap_errors.h | 11 ++- test/python/apps/exchange_test_runner.py | 90 ++++++++++-------- 7 files changed, 171 insertions(+), 116 deletions(-) diff --git a/src/command_dispatcher.c b/src/command_dispatcher.c index c92fdeca..f4960579 100644 --- a/src/command_dispatcher.c +++ b/src/command_dispatcher.c @@ -16,31 +16,10 @@ #include "io.h" + int dispatch_command(command_t *cmd) { PRINTF("command: %d, subcommand: %d, state: %d\n", cmd->ins, cmd->subcommand, G_swap_ctx.state); - if (cmd->rate != FIXED && cmd->rate != FLOATING) { - PRINTF("Unexpected P1 %d\n", cmd->rate); - return reply_error(WRONG_P1); - } - - if (cmd->subcommand != SWAP && cmd->subcommand != SELL && cmd->subcommand != FUND) { - PRINTF("Attempting to recognize NG subcommand\n"); - uint8_t subcommand = 0x0F & cmd->subcommand; - uint8_t extension = 0xF0 & cmd->subcommand; - PRINTF("subcommand = %d\n", subcommand); - PRINTF("extension = %d\n", extension); - if (subcommand != SWAP_NG && subcommand != SELL_NG && subcommand != FUND_NG) { - PRINTF("Unexpected subcommand %d\n", subcommand); - return reply_error(WRONG_P2); - } - if (extension != P2_NONE && extension != P2_MORE && extension != P2_EXTEND && (extension != (P2_MORE | P2_EXTEND))) { - PRINTF("Unexpected extension %d\n", extension); - return reply_error(WRONG_P2); - } - cmd->subcommand = subcommand; - } - int ret = -1; bool valid_command_received = false; @@ -72,7 +51,7 @@ int dispatch_command(command_t *cmd) { } break; case PROCESS_TRANSACTION_RESPONSE_COMMAND: - if (G_swap_ctx.state == TRANSACTION_RECEIVE_IN_PROGRESS && cmd->subcommand == G_swap_ctx.subcommand) { + if (G_swap_ctx.state == PROVIDER_CHECKED && cmd->subcommand == G_swap_ctx.subcommand) { ret = process_transaction(cmd); valid_command_received = true; } diff --git a/src/commands.h b/src/commands.h index 8d951404..797b7350 100644 --- a/src/commands.h +++ b/src/commands.h @@ -1,7 +1,7 @@ -#ifndef _COMMANDS_H_ -#define _COMMANDS_H_ +#pragma once #include "buffer.h" + // commands typedef enum { GET_VERSION_COMMAND = 0x02, @@ -15,12 +15,19 @@ typedef enum { START_SIGNING_TRANSACTION = 0x0A, } command_e; -// subcommands -typedef enum { SWAP = 0x00, SELL = 0x01, FUND = 0x02, SWAP_NG = 0x03, SELL_NG = 0x04, FUND_NG = 0x05 } subcommand_e; // Different rates possible typedef enum { FIXED = 0x00, FLOATING = 0x01 } rate_e; +// subcommands +typedef enum { SWAP = 0x00, SELL = 0x01, FUND = 0x02, SWAP_NG = 0x03, SELL_NG = 0x04, FUND_NG = 0x05 } subcommand_e; +#define SUBCOMMAND_PART 0x0F + +#define P2_NONE (0x00 << 4) +#define P2_EXTEND (0x01 << 4) +#define P2_MORE (0x02 << 4) +#define EXTENSION_PART 0xF0 + /** * Structure with fields of APDU command. */ @@ -30,5 +37,3 @@ typedef struct { subcommand_e subcommand; /// P2 buf_t data; /// Command data } command_t; - -#endif //_COMMANDS_H_ diff --git a/src/globals.h b/src/globals.h index ce0dba3c..759f9cb8 100644 --- a/src/globals.h +++ b/src/globals.h @@ -8,13 +8,6 @@ #include "buffer.h" #include "swap_lib_calls.h" -#define P1_CONFIRM 0x01 -#define P1_NON_CONFIRM 0x00 - -#define P2_NONE (0x00 << 4) -#define P2_EXTEND (0x01 << 4) -#define P2_MORE (0x02 << 4) - #define CURVE_SIZE_BYTES 32U #define UNCOMPRESSED_KEY_LENGTH 65U #define MIN_DER_SIGNATURE_LENGTH 67U @@ -34,7 +27,7 @@ extern uint8_t G_io_seproxyhal_spi_buffer[IO_SEPROXYHAL_BUFFER_SIZE_B]; #pragma pack(push, 1) typedef struct partner_data_s { - unsigned char name_length; + uint8_t name_length; union { // SELL and SWAP flows display nothing // FUND flow displays "To xyz" @@ -50,12 +43,12 @@ typedef struct partner_data_s { typedef struct swap_app_context_s { union { - unsigned char sell_fund[32]; // device_transaction_id (SELL && FUND && NG) + uint8_t sell_fund[32]; // device_transaction_id (SELL && FUND && NG) char swap[10]; // device_transaction_id (SWAP) } device_transaction_id; - unsigned char transaction_fee[16]; - unsigned char transaction_fee_length; + uint8_t transaction_fee[16]; + uint8_t transaction_fee_length; partner_data_t partner; state_e state; @@ -77,7 +70,7 @@ typedef struct swap_app_context_s { }; }; - unsigned char sha256_digest[32]; + uint8_t sha256_digest[32]; cx_ecfp_256_public_key_t ledger_public_key; diff --git a/src/main.c b/src/main.c index 5d8c77c9..21d7a080 100644 --- a/src/main.c +++ b/src/main.c @@ -37,8 +37,8 @@ uint8_t G_io_seproxyhal_spi_buffer[IO_SEPROXYHAL_BUFFER_SIZE_B]; typedef struct apdu_s { uint8_t ins; - uint8_t p1; - uint8_t p2; + uint8_t rate; + uint8_t subcommand; uint16_t data_length; uint8_t data[510]; } apdu_t; @@ -46,6 +46,7 @@ apdu_t G_received_apdu; swap_app_context_t G_swap_ctx; + void app_main(void) { int input_length = 0; @@ -54,35 +55,94 @@ void app_main(void) { for (;;) { input_length = recv_apdu(); PRINTF("New APDU received:\n%.*H\n", input_length, G_io_apdu_buffer); - if (input_length == -1) // there were an error, lets start from the beginning + // there were a fatal error during APDU reception, restart from the beginning + // Don't bother trying to send a status code, IOs are probably out + if (input_length == -1) { return; - if (input_length < OFFSET_CDATA || G_io_apdu_buffer[OFFSET_CLA] != CLA) { - PRINTF("Error: bad APDU\n"); - reply_error(INVALID_INSTRUCTION); + } + + if (input_length < OFFSET_CDATA) { + PRINTF("Error: malformed APDU\n"); + reply_error(MALFORMED_APDU); + return; + } + if (G_io_apdu_buffer[OFFSET_CLA] != CLA) { + PRINTF("Error: malformed APDU\n"); + reply_error(CLASS_NOT_SUPPORTED); return; } - // P2_EXTEND is set to signal that this APDU buffer extends, rather - // than replaces, the current message buffer - bool first_data_chunk = !(G_io_apdu_buffer[OFFSET_P2] & P2_EXTEND); - if (!first_data_chunk && G_swap_ctx.state != TRANSACTION_RECEIVE_IN_PROGRESS) { - PRINTF("Error: Unexpected !first_data_chunk\n"); + uint8_t ins = G_io_apdu_buffer[OFFSET_INS]; + if (ins != GET_VERSION_COMMAND + && ins != START_NEW_TRANSACTION_COMMAND + && ins != SET_PARTNER_KEY_COMMAND + && ins != CHECK_PARTNER_COMMAND + && ins != PROCESS_TRANSACTION_RESPONSE_COMMAND + && ins != CHECK_TRANSACTION_SIGNATURE_COMMAND + && ins != CHECK_PAYOUT_ADDRESS + && ins != CHECK_REFUND_ADDRESS + && ins != START_SIGNING_TRANSACTION) { + PRINTF("Incorrect instruction %d\n", ins); reply_error(INVALID_INSTRUCTION); return; } - bool last_data_chunk = !(G_io_apdu_buffer[OFFSET_P2] & P2_MORE); - if (!last_data_chunk && G_swap_ctx.state != PROVIDER_CHECKED) { - PRINTF("Error: Unexpected !first_data_chunk\n"); - reply_error(INVALID_INSTRUCTION); + uint8_t rate = G_io_apdu_buffer[OFFSET_P1]; + if (rate != FIXED && rate != FLOATING) { + PRINTF("Incorrect P1 %d\n", rate); + reply_error(WRONG_P1); + return; + } + + uint8_t subcommand = G_io_apdu_buffer[OFFSET_P2] & SUBCOMMAND_PART; + if (subcommand != SWAP + && subcommand != SELL + && subcommand != FUND + && subcommand != SWAP_NG + && subcommand != SELL_NG + && subcommand != FUND_NG) { + PRINTF("Incorrect subcommand %d\n", subcommand); + reply_error(WRONG_P2_SUBCOMMAND); return; } + uint8_t extension = G_io_apdu_buffer[OFFSET_P2] & EXTENSION_PART; + if ((extension & ~(P2_NONE|P2_MORE|P2_EXTEND)) != 0) { + PRINTF("Incorrect extension %d\n", extension); + reply_error(WRONG_P2_EXTENSION); + return; + } + + uint8_t data_length = G_io_apdu_buffer[OFFSET_LC]; + if (data_length != input_length - OFFSET_CDATA) { + PRINTF("Incorrect advertized length %d\n", data_length); + reply_error(INVALID_DATA_LENGTH); + return; + } + + // P2_MORE is set to signal that this APDU buffer is not complete + // P2_EXTEND is set to signal that this APDU buffer extends a previous one + bool first_data_chunk = !(extension & P2_EXTEND); + bool last_data_chunk = !(extension & P2_MORE); + + // if (!first_data_chunk && G_swap_ctx.state != PROVIDER_CHECKED) { + // PRINTF("Error: Unexpected !first_data_chunk\n"); + // reply_error(INVALID_INSTRUCTION); + // return; + // } + + // if (!last_data_chunk && G_swap_ctx.state != PROVIDER_CHECKED) { + // PRINTF("Error: Unexpected !first_data_chunk\n"); + // reply_error(INVALID_INSTRUCTION); + // return; + // } + if (first_data_chunk) { - G_received_apdu.ins = G_io_apdu_buffer[OFFSET_INS]; - G_received_apdu.p1 = G_io_apdu_buffer[OFFSET_P1]; - G_received_apdu.p2 = G_io_apdu_buffer[OFFSET_P2]; - G_received_apdu.data_length = input_length - OFFSET_CDATA; + PRINTF("Receiving the first part of an apdu\n"); + G_received_apdu.ins = ins; + G_received_apdu.rate = rate; + G_received_apdu.subcommand = subcommand; + G_received_apdu.data_length = data_length; memcpy(G_received_apdu.data, G_io_apdu_buffer + OFFSET_CDATA, G_received_apdu.data_length); PRINTF("P1 G_received_apdu.data_length = %d\n", G_received_apdu.data_length); for (int i = 0; i < G_received_apdu.data_length; ++i) { @@ -90,7 +150,14 @@ void app_main(void) { } PRINTF("\n"); } else { - PRINTF("Followup data chunk\n"); + PRINTF("Extending an already received partial of an apdu\n"); + if (G_received_apdu.ins != ins + || G_received_apdu.rate != rate + || G_received_apdu.subcommand != subcommand) { + PRINTF("Refusing to extend a different apdu\n"); + reply_error(INVALID_PZ_EXTENSION); + return; + } memcpy(G_received_apdu.data + G_received_apdu.data_length, G_io_apdu_buffer + OFFSET_CDATA, input_length - OFFSET_CDATA); G_received_apdu.data_length += input_length - OFFSET_CDATA; @@ -102,15 +169,16 @@ void app_main(void) { } if (!last_data_chunk) { - G_swap_ctx.state = TRANSACTION_RECEIVE_IN_PROGRESS; + // Reply a blank success to indicate that we await the followup part + // Do NOT update any kind of internal state machine, we have not validated what we have received reply_success(); continue; } command_t cmd = { .ins = (command_e) G_received_apdu.ins, - .rate = G_received_apdu.p1, - .subcommand = G_received_apdu.p2, + .rate = G_received_apdu.rate, + .subcommand = G_received_apdu.subcommand, .data = { .bytes = G_received_apdu.data, diff --git a/src/states.h b/src/states.h index a7633ea6..5a0d48a2 100644 --- a/src/states.h +++ b/src/states.h @@ -5,12 +5,11 @@ typedef enum { WAITING_TRANSACTION = 1, PROVIDER_SET = 2, PROVIDER_CHECKED = 3, - TRANSACTION_RECEIVE_IN_PROGRESS = 4, - TRANSACTION_RECEIVED = 5, - SIGNATURE_CHECKED = 6, - TO_ADDR_CHECKED = 7, - WAITING_USER_VALIDATION = 8, - WAITING_SIGNING = 9, - SIGN_FINISHED = 10, + TRANSACTION_RECEIVED = 4, + SIGNATURE_CHECKED = 5, + TO_ADDR_CHECKED = 6, + WAITING_USER_VALIDATION = 7, + WAITING_SIGNING = 8, + SIGN_FINISHED = 9, STATE_UPPER_BOUND, } state_e; diff --git a/src/swap_errors.h b/src/swap_errors.h index 880986d1..d7f0ff63 100644 --- a/src/swap_errors.h +++ b/src/swap_errors.h @@ -1,5 +1,4 @@ -#ifndef _SWAP_ERRORS_H_ -#define _SWAP_ERRORS_H_ +#pragma once // This value is the part of the host <-> device protocol // they will be reported to host in APDU @@ -15,10 +14,12 @@ typedef enum { USER_REFUSED = 0x6A84, INTERNAL_ERROR = 0x6A85, WRONG_P1 = 0x6A86, - WRONG_P2 = 0x6A87, + WRONG_P2_SUBCOMMAND = 0x6A87, + WRONG_P2_EXTENSION = 0x6A88, + INVALID_PZ_EXTENSION = 0x6A89, CLASS_NOT_SUPPORTED = 0x6E00, + MALFORMED_APDU = 0x6E01, + INVALID_DATA_LENGTH = 0x6E02, INVALID_INSTRUCTION = 0x6D00, SIGN_VERIFICATION_FAIL = 0x9D1A } swap_error_e; - -#endif //_SWAP_ERRORS_H_ \ No newline at end of file diff --git a/test/python/apps/exchange_test_runner.py b/test/python/apps/exchange_test_runner.py index ff5a4e88..633d5fd5 100644 --- a/test/python/apps/exchange_test_runner.py +++ b/test/python/apps/exchange_test_runner.py @@ -165,71 +165,81 @@ def perform_coin_specific_final_tx(self, destination, send_amount, fees, memo): # We test that the currency app returns a fail when checking an incorrect refund address def perform_test_swap_wrong_refund(self): - with pytest.raises(ExceptionRAPDU) as e: - self.perform_valid_swap_from_custom(self.valid_destination_1, - self.valid_send_amount_1, - self.valid_fees_1, - self.valid_destination_memo_1, - refund_address=self.fake_refund, - refund_memo=self.fake_refund_memo, - ui_validation=False) - assert e.value.status == Errors.INVALID_ADDRESS + for ng in [True, False]: + with pytest.raises(ExceptionRAPDU) as e: + self.perform_valid_swap_from_custom(self.valid_destination_1, + self.valid_send_amount_1, + self.valid_fees_1, + self.valid_destination_memo_1, + refund_address=self.fake_refund, + refund_memo=self.fake_refund_memo, + ui_validation=False, + ng=ng) + assert e.value.status == Errors.INVALID_ADDRESS # We test that the currency app returns a fail when checking an incorrect payout address def perform_test_swap_wrong_payout(self): - with pytest.raises(ExceptionRAPDU) as e: - self.perform_valid_swap_to_custom(self.fake_payout, self.valid_send_amount_1, self.valid_fees_1, self.fake_payout_memo, ui_validation=False) - assert e.value.status == Errors.INVALID_ADDRESS + for ng in [True, False]: + with pytest.raises(ExceptionRAPDU) as e: + self.perform_valid_swap_to_custom(self.fake_payout, self.valid_send_amount_1, self.valid_fees_1, self.fake_payout_memo, ui_validation=False, ng=ng) + assert e.value.status == Errors.INVALID_ADDRESS # The absolute standard swap, using default values, user accepts on UI def perform_test_swap_valid_1(self): - self.perform_valid_swap_from_custom(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1, ng=True) - self.perform_coin_specific_final_tx(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1) + for ng in [True, False]: + self.perform_valid_swap_from_custom(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1, ng=ng) + self.perform_coin_specific_final_tx(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1) # The second standard swap, using alternate default values, user accepts on UI def perform_test_swap_valid_2(self): - self.perform_valid_swap_from_custom(self.valid_destination_2, self.valid_send_amount_2, self.valid_fees_2, self.valid_destination_memo_2) - self.perform_coin_specific_final_tx(self.valid_destination_2, self.valid_send_amount_2, self.valid_fees_2, self.valid_destination_memo_2) + for ng in [True, False]: + self.perform_valid_swap_from_custom(self.valid_destination_2, self.valid_send_amount_2, self.valid_fees_2, self.valid_destination_memo_2, ng=ng) + self.perform_coin_specific_final_tx(self.valid_destination_2, self.valid_send_amount_2, self.valid_fees_2, self.valid_destination_memo_2) # Make a valid swap and then ask a second signature def perform_test_swap_refuse_double_sign(self): - self.perform_valid_swap_from_custom(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1) - self.perform_coin_specific_final_tx(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1) - with pytest.raises(ExceptionRAPDU) as e: + for ng in [True, False]: + self.perform_valid_swap_from_custom(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1, ng=ng) self.perform_coin_specific_final_tx(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1) - assert e.value.status == Errors.INVALID_INSTRUCTION or e.value.status == Errors.WRONG_P2 + with pytest.raises(ExceptionRAPDU) as e: + self.perform_coin_specific_final_tx(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1) + assert e.value.status == Errors.INVALID_INSTRUCTION or e.value.status == Errors.WRONG_P2 # Test swap with a malicious TX with tampered fees def perform_test_swap_wrong_fees(self): - assert self.valid_fees_1 != self.valid_fees_2, "This test won't work if the values are the same" - self.perform_valid_swap_from_custom(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1) - with pytest.raises(ExceptionRAPDU) as e: - self.perform_coin_specific_final_tx(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_2, self.valid_destination_memo_1) - assert e.value.status == self.signature_refusal_error_code + for ng in [True, False]: + assert self.valid_fees_1 != self.valid_fees_2, "This test won't work if the values are the same" + self.perform_valid_swap_from_custom(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1, ng=ng) + with pytest.raises(ExceptionRAPDU) as e: + self.perform_coin_specific_final_tx(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_2, self.valid_destination_memo_1) + assert e.value.status == self.signature_refusal_error_code # Test swap with a malicious TX with tampered memo def perform_test_swap_wrong_memo(self): - assert self.valid_destination_memo_1 != self.valid_destination_memo_2, "This test won't work if the values are the same" - self.perform_valid_swap_from_custom(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1) - with pytest.raises(ExceptionRAPDU) as e: - self.perform_coin_specific_final_tx(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_2) - assert e.value.status == self.signature_refusal_error_code + for ng in [True, False]: + assert self.valid_destination_memo_1 != self.valid_destination_memo_2, "This test won't work if the values are the same" + self.perform_valid_swap_from_custom(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1, ng=ng) + with pytest.raises(ExceptionRAPDU) as e: + self.perform_coin_specific_final_tx(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_2) + assert e.value.status == self.signature_refusal_error_code # Test swap with a malicious TX with tampered destination def perform_test_swap_wrong_destination(self): - assert self.valid_destination_1 != self.valid_destination_2, "This test won't work if the values are the same" - self.perform_valid_swap_from_custom(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1) - with pytest.raises(ExceptionRAPDU) as e: - self.perform_coin_specific_final_tx(self.valid_destination_2, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1) - assert e.value.status == self.signature_refusal_error_code + for ng in [True, False]: + assert self.valid_destination_1 != self.valid_destination_2, "This test won't work if the values are the same" + self.perform_valid_swap_from_custom(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1, ng=ng) + with pytest.raises(ExceptionRAPDU) as e: + self.perform_coin_specific_final_tx(self.valid_destination_2, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1) + assert e.value.status == self.signature_refusal_error_code # Test swap with a malicious TX with tampered amount def perform_test_swap_wrong_amount(self): - assert self.valid_send_amount_1 != self.valid_send_amount_2, "This test won't work if the values are the same" - self.perform_valid_swap_from_custom(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1) - with pytest.raises(ExceptionRAPDU) as e: - self.perform_coin_specific_final_tx(self.valid_destination_1, self.valid_send_amount_2, self.valid_fees_1, self.valid_destination_memo_1) - assert e.value.status == self.signature_refusal_error_code + for ng in [True, False]: + assert self.valid_send_amount_1 != self.valid_send_amount_2, "This test won't work if the values are the same" + self.perform_valid_swap_from_custom(self.valid_destination_1, self.valid_send_amount_1, self.valid_fees_1, self.valid_destination_memo_1, ng=ng) + with pytest.raises(ExceptionRAPDU) as e: + self.perform_coin_specific_final_tx(self.valid_destination_1, self.valid_send_amount_2, self.valid_fees_1, self.valid_destination_memo_1) + assert e.value.status == self.signature_refusal_error_code ######################################################### # Generic FUND tests functions, call them in your tests #