From 11b086374b2e4de9322a0935b8253ee1d2e5eee4 Mon Sep 17 00:00:00 2001 From: Francois Beutin Date: Thu, 19 Dec 2024 18:29:25 +0100 Subject: [PATCH] wip --- .github/workflows/codeql-workflow.yml | 2 +- src/apdu_parser.c | 22 ++++++---------------- src/globals.h | 2 -- src/process_transaction.c | 15 +-------------- src/trusted_name_descriptor_handler.c | 3 +++ test/python/test_trusted_name.py | 23 ++++++++++++----------- 6 files changed, 23 insertions(+), 44 deletions(-) diff --git a/.github/workflows/codeql-workflow.yml b/.github/workflows/codeql-workflow.yml index e7eefa6c..b92324ce 100644 --- a/.github/workflows/codeql-workflow.yml +++ b/.github/workflows/codeql-workflow.yml @@ -17,7 +17,7 @@ jobs: name: Analyse strategy: matrix: - sdk: [ "$NANOS_SDK", "$NANOX_SDK", "$NANOSP_SDK" ] + sdk: [ "$NANOX_SDK", "$NANOSP_SDK", "$STAX_SDK", "$FLEX_SDK" ] #'cpp' covers C and C++ language: [ 'cpp' ] runs-on: ubuntu-latest diff --git a/src/apdu_parser.c b/src/apdu_parser.c index e6d49c2e..ed338811 100644 --- a/src/apdu_parser.c +++ b/src/apdu_parser.c @@ -36,13 +36,7 @@ typedef struct apdu_s { uint8_t rate; uint8_t subcommand; uint16_t data_length; - // We could have put the reconstructed apdu buffer here but it would increase the RAM usage by - // 512 bytes which is a lot on NANOS - // Instead the reconstructed apdu buffer is G_swap_ctx.raw_transaction - // It is unionized with the decoded protobuf transaction requests - // Pro: less memory usage - // Cons: use cautiously and only during the command PROCESS_TRANSACTION_RESPONSE_COMMAND - // The split reception is only made for this command anyway + uint8_t raw_transaction[256 * 2]; } apdu_t; apdu_t G_received_apdu; @@ -252,11 +246,7 @@ uint16_t check_apdu_validity(uint8_t *apdu, size_t apdu_length, command_t *comma G_received_apdu.subcommand = subcommand; G_received_apdu.data_length = data_length; if (!is_last_data_chunk) { - // Use the raw_transaction buffer as temporary storage. - // It's unionized with the decoded transaction but we are currently handling - // PROCESS_TRANSACTION_RESPONSE_COMMAND instruction (checked previously). - // After this command is done it will contain the decoded PB data, don't use it anymore - memcpy(G_swap_ctx.raw_transaction, apdu + OFFSET_CDATA, data_length); + memcpy(G_received_apdu.raw_transaction, apdu + OFFSET_CDATA, data_length); G_received_apdu.expecting_more = true; } } else { @@ -277,15 +267,15 @@ uint16_t check_apdu_validity(uint8_t *apdu, size_t apdu_length, command_t *comma subcommand); return INVALID_P2_EXTENSION; } - if (G_received_apdu.data_length + data_length > sizeof(G_swap_ctx.raw_transaction)) { + if (G_received_apdu.data_length + data_length > sizeof(G_received_apdu.raw_transaction)) { PRINTF("Reception buffer size %d is not sufficient to receive more data (%d + %d)\n", - sizeof(G_swap_ctx.raw_transaction), + sizeof(G_received_apdu.raw_transaction), G_received_apdu.data_length, data_length); return INVALID_P2_EXTENSION; } // Extend the already received buffer - memcpy(G_swap_ctx.raw_transaction + G_received_apdu.data_length, + memcpy(G_received_apdu.raw_transaction + G_received_apdu.data_length, apdu + OFFSET_CDATA, data_length); G_received_apdu.data_length += data_length; @@ -311,7 +301,7 @@ uint16_t check_apdu_validity(uint8_t *apdu, size_t apdu_length, command_t *comma } else { // Split has taken place, data is in the split buffer PRINTF("Split APDU successfully recreated, size %d\n", G_received_apdu.data_length); - command->data.bytes = G_swap_ctx.raw_transaction; + command->data.bytes = G_received_apdu.raw_transaction; } return 0; } diff --git a/src/globals.h b/src/globals.h index 4d4579eb..a1bb8fae 100644 --- a/src/globals.h +++ b/src/globals.h @@ -62,8 +62,6 @@ typedef struct swap_app_context_s { // SWAP, SELL, and FUND flows are unionized as they cannot be used in the same context union { - // This is the raw received APDU - uint8_t raw_transaction[256 * 2]; ledger_swap_NewTransactionResponse swap_transaction; struct { ledger_swap_NewSellResponse sell_transaction; diff --git a/src/process_transaction.c b/src/process_transaction.c index 03b60f79..03baf07c 100644 --- a/src/process_transaction.c +++ b/src/process_transaction.c @@ -336,23 +336,10 @@ static void save_fees(buf_t fees) { } int process_transaction(const command_t *cmd) { - uint8_t undecoded_transaction[sizeof(G_swap_ctx.raw_transaction)]; - uint8_t *data; - // For memory optimization, the undecoded protobuf apdu may have been stored in an union - // with the decoded protobuf transaction. - if (cmd->data.bytes == G_swap_ctx.raw_transaction) { - // Copy locally the apdu to avoid problems during protobuf decode and fees extraction - PRINTF("Copying locally, the APDU has been received split\n"); - memcpy(undecoded_transaction, G_swap_ctx.raw_transaction, cmd->data.size); - data = undecoded_transaction; - } else { - data = cmd->data.bytes; - } - buf_t fees; buf_t payload; bool needs_base64_decoding; - if (!parse_transaction(data, + if (!parse_transaction(cmd->data.bytes, cmd->data.size, cmd->subcommand, &payload, diff --git a/src/trusted_name_descriptor_handler.c b/src/trusted_name_descriptor_handler.c index 78ea74e6..89f28d32 100644 --- a/src/trusted_name_descriptor_handler.c +++ b/src/trusted_name_descriptor_handler.c @@ -266,6 +266,9 @@ static int trusted_name_descriptor_handler_internal(const command_t *cmd) { &trusted_name_info.token_account, &trusted_name_info.owner); + PRINTF("Payout address : %.*H\n", sizeof(G_swap_ctx.swap_transaction.payout_address), G_swap_ctx.swap_transaction.payout_address); + PRINTF("Refund address : %.*H\n", sizeof(G_swap_ctx.swap_transaction.refund_address), G_swap_ctx.swap_transaction.refund_address); + return reply_success(); } diff --git a/test/python/test_trusted_name.py b/test/python/test_trusted_name.py index 60375216..1cb928e1 100644 --- a/test/python/test_trusted_name.py +++ b/test/python/test_trusted_name.py @@ -26,8 +26,8 @@ "payout_extra_id": b"", "currency_from": CURRENCY_FROM.ticker, "currency_to": CURRENCY_TO.ticker, - "amount_to_provider": bytes.fromhex("013fc3a717fb5000"), - "amount_to_wallet": b"\x0b\xeb\xc2\x00", + "amount_to_provider": bytes.fromhex("01"), + "amount_to_wallet": bytes.fromhex("01"), } FUND_TX_INFOS = { "user_id": "John Wick", @@ -82,8 +82,10 @@ def test_trusted_name_wrong_challenge(self, backend, subcommand): @pytest.mark.parametrize("subcommand", [SubCommand.SWAP, SubCommand.SWAP_NG]) @pytest.mark.parametrize("field_to_test", ["payout_address", "refund_address"]) - @pytest.mark.parametrize("value", [b"0xcafedeca", b"f" * 62]) + @pytest.mark.parametrize("value", [b"0xcafedeca", b"f" * 150]) def test_trusted_name_valid(self, backend, subcommand, field_to_test, value): + if subcommand == SubCommand.SWAP and len(value) > 50: + pytest.skip("Can't test max address value on legacy flows") ex = ExchangeClient(backend, Rate.FIXED, subcommand) partner = SigningAuthority(curve=get_partner_curve(subcommand), name="Name") transaction_id = ex.init_transaction().data @@ -94,6 +96,7 @@ def test_trusted_name_valid(self, backend, subcommand, field_to_test, value): tx_infos = TX_INFOS[subcommand].copy() actual_address = tx_infos[field_to_test] temp_address = value + print(f"Replacing {actual_address} by {temp_address}") tx_infos[field_to_test] = temp_address tx, tx_signature = craft_and_sign_tx(subcommand, tx_infos, transaction_id, FEES, partner) ex.process_transaction(tx) @@ -178,21 +181,17 @@ def test_trusted_name_wrong_field(self, backend, subcommand): ex.check_transaction_signature(tx_signature) for fake in ["structure_type", "version", "trusted_name_type", "trusted_name_source", "signer_algo"]: + challenge = ex.get_challenge().data with pytest.raises(ExceptionRAPDU) as e: if fake == "structure_type": - challenge = ex.get_challenge().data ex.send_pki_certificate_and_trusted_name_descriptor(challenge=challenge, structure_type=0) elif fake == "version": - challenge = ex.get_challenge().data ex.send_pki_certificate_and_trusted_name_descriptor(challenge=challenge, version=0) elif fake == "trusted_name_type": - challenge = ex.get_challenge().data ex.send_pki_certificate_and_trusted_name_descriptor(challenge=challenge, trusted_name_type=0) elif fake == "trusted_name_source": - challenge = ex.get_challenge().data ex.send_pki_certificate_and_trusted_name_descriptor(challenge=challenge, trusted_name_source=0) elif fake == "signer_algo": - challenge = ex.get_challenge().data ex.send_pki_certificate_and_trusted_name_descriptor(challenge=challenge, signer_algo=0) assert e.value.status == Errors.WRONG_TLV_CONTENT @@ -207,13 +206,15 @@ def test_trusted_name_wrong_field(self, backend, subcommand): assert e.value.status == Errors.WRONG_TLV_SIGNATURE for fake in ["trusted_name", "address"]: - for value in [b"", b"F"*63]: + values = [b""] + if subcommand == SubCommand.SWAP_NG: + values += [b"F"*151] + for value in values: + challenge = ex.get_challenge().data with pytest.raises(ExceptionRAPDU) as e: if fake == "trusted_name": - challenge = ex.get_challenge().data ex.send_pki_certificate_and_trusted_name_descriptor(challenge=challenge, trusted_name=value) elif fake == "address": - challenge = ex.get_challenge().data ex.send_pki_certificate_and_trusted_name_descriptor(challenge=challenge, address=value) assert e.value.status == Errors.WRONG_TLV_FORMAT