From 3c1ca8dd660f18832a0c99d03082a695a98ff2d9 Mon Sep 17 00:00:00 2001 From: Elliot Cameron Date: Thu, 5 Sep 2019 00:16:43 -0400 Subject: [PATCH 01/11] Don't use bitmask for operation sets since Babylon tags are too large --- src/apdu_sign.c | 30 +++++++++++++++--------------- src/operations.c | 8 ++++---- src/operations.h | 16 ++-------------- 3 files changed, 21 insertions(+), 33 deletions(-) diff --git a/src/apdu_sign.c b/src/apdu_sign.c index 5c7be6a9..0f16c847 100644 --- a/src/apdu_sign.c +++ b/src/apdu_sign.c @@ -88,27 +88,27 @@ static bool sign_reject(void) { return true; // Return to idle } +static bool is_operation_allowed(enum operation_tag tag) { + switch (tag) { + case OPERATION_TAG_DELEGATION: return true; + case OPERATION_TAG_REVEAL: return true; +# ifndef BAKING_APP + case OPERATION_TAG_PROPOSAL: return true; + case OPERATION_TAG_BALLOT: return true; + case OPERATION_TAG_ORIGINATION: return true; + case OPERATION_TAG_TRANSACTION: return true; +# endif + default: return false; + } +} + static bool parse_allowed_operations( struct parsed_operation_group *const out, uint8_t const *const in, size_t const in_size, bip32_path_with_curve_t const *const key ) { - // TODO: Simplify this to just switch on what we got. - allowed_operation_set allowed; - clear_operation_set(&allowed); - - allow_operation(&allowed, OPERATION_TAG_DELEGATION); - allow_operation(&allowed, OPERATION_TAG_REVEAL); -# ifndef BAKING_APP - allow_operation(&allowed, OPERATION_TAG_PROPOSAL); - allow_operation(&allowed, OPERATION_TAG_BALLOT); - allow_operation(&allowed, OPERATION_TAG_ORIGINATION); - allow_operation(&allowed, OPERATION_TAG_TRANSACTION); - // TODO: Add still other operations -# endif - - return parse_operations(out, in, in_size, key->derivation_type, &key->bip32_path, allowed); + return parse_operations(out, in, in_size, key->derivation_type, &key->bip32_path, &is_operation_allowed); } #ifdef BAKING_APP // ---------------------------------------------------------- diff --git a/src/operations.c b/src/operations.c index 5b5781bd..8e8c15f7 100644 --- a/src/operations.c +++ b/src/operations.c @@ -169,7 +169,7 @@ static void parse_operations_throws_parse_error( size_t length, derivation_type_t derivation_type, bip32_path_t const *const bip32_path, - allowed_operation_set ops + is_operation_allowed_t is_operation_allowed ) { check_null(out); check_null(data); @@ -193,7 +193,7 @@ static void parse_operations_throws_parse_error( while (ix < length) { const enum operation_tag tag = NEXT_BYTE(data, &ix, length); // 1 byte is always aligned - if (!is_operation_allowed(ops, tag)) PARSE_ERROR(); + if (!is_operation_allowed(tag)) PARSE_ERROR(); if (tag == OPERATION_TAG_PROPOSAL || tag == OPERATION_TAG_BALLOT) { // These tags don't have the "originated" byte so we have to parse PKH differently. @@ -350,11 +350,11 @@ bool parse_operations( size_t length, derivation_type_t derivation_type, bip32_path_t const *const bip32_path, - allowed_operation_set ops + is_operation_allowed_t is_operation_allowed ) { BEGIN_TRY { TRY { - parse_operations_throws_parse_error(out, data, length, derivation_type, bip32_path, ops); + parse_operations_throws_parse_error(out, data, length, derivation_type, bip32_path, is_operation_allowed); } CATCH(EXC_PARSE_ERROR) { return false; diff --git a/src/operations.h b/src/operations.h index fb7d3665..40552a80 100644 --- a/src/operations.h +++ b/src/operations.h @@ -10,19 +10,7 @@ #include "cx.h" #include "types.h" -typedef uint32_t allowed_operation_set; - -static inline void allow_operation(allowed_operation_set *ops, enum operation_tag tag) { - *ops |= (1 << (uint32_t)tag); -} - -static inline bool is_operation_allowed(allowed_operation_set ops, enum operation_tag tag) { - return (ops & (1 << (uint32_t)tag)) != 0; -} - -static inline void clear_operation_set(allowed_operation_set *ops) { - *ops = 0; -} +typedef bool (*is_operation_allowed_t)(enum operation_tag); // Allows arbitrarily many "REVEAL" operations but only one operation of any other type, // which is the one it puts into the group. @@ -32,5 +20,5 @@ bool parse_operations( size_t length, derivation_type_t curve, bip32_path_t const *const bip32_path, - allowed_operation_set ops + is_operation_allowed_t is_operation_allowed ); From f6d68bbb39b2d1bc68e7c281d642a123ccb6cfd4 Mon Sep 17 00:00:00 2001 From: Elliot Cameron Date: Thu, 5 Sep 2019 00:17:25 -0400 Subject: [PATCH 02/11] Update operation tags for Babylon --- src/types.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/types.h b/src/types.h index d140dc77..917ccfd2 100644 --- a/src/types.h +++ b/src/types.h @@ -213,10 +213,10 @@ enum operation_tag { OPERATION_TAG_NONE = -1, // Sentinal value, as 0 is possibly used for something OPERATION_TAG_PROPOSAL = 5, OPERATION_TAG_BALLOT = 6, - OPERATION_TAG_REVEAL = 7, - OPERATION_TAG_TRANSACTION = 8, - OPERATION_TAG_ORIGINATION = 9, - OPERATION_TAG_DELEGATION = 10, + OPERATION_TAG_REVEAL = 107, + OPERATION_TAG_TRANSACTION = 108, + OPERATION_TAG_ORIGINATION = 109, + OPERATION_TAG_DELEGATION = 110, }; // TODO: Make this an enum. From 058ebcb4d72c251a1221af0f3cc9428df77fa7ad Mon Sep 17 00:00:00 2001 From: Elliot Cameron Date: Thu, 5 Sep 2019 00:17:56 -0400 Subject: [PATCH 03/11] Always parse source field as PKH for Babylon --- src/operations.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/operations.c b/src/operations.c index 8e8c15f7..60fb6196 100644 --- a/src/operations.c +++ b/src/operations.c @@ -195,16 +195,12 @@ static void parse_operations_throws_parse_error( if (!is_operation_allowed(tag)) PARSE_ERROR(); - if (tag == OPERATION_TAG_PROPOSAL || tag == OPERATION_TAG_BALLOT) { - // These tags don't have the "originated" byte so we have to parse PKH differently. - const struct implicit_contract *implicit_source = NEXT_TYPE(struct implicit_contract); - out->operation.source.originated = 0; - out->operation.source.signature_type = parse_raw_tezos_header_signature_type(&implicit_source->signature_type); - memcpy(out->operation.source.hash, implicit_source->pkh, sizeof(out->operation.source.hash)); - } else { - const struct contract *source = NEXT_TYPE(struct contract); - parse_contract(&out->operation.source, source); + const struct implicit_contract *implicit_source = NEXT_TYPE(struct implicit_contract); + out->operation.source.originated = 0; + out->operation.source.signature_type = parse_raw_tezos_header_signature_type(&implicit_source->signature_type); + memcpy(out->operation.source.hash, implicit_source->pkh, sizeof(out->operation.source.hash)); + if (!(tag == OPERATION_TAG_PROPOSAL || tag == OPERATION_TAG_BALLOT)) { out->total_fee += PARSE_Z(data, &ix, length); // fee PARSE_Z(data, &ix, length); // counter PARSE_Z(data, &ix, length); // gas limit From 7b6550a24ab68d3afe13fb5c3247837f5d568f68 Mon Sep 17 00:00:00 2001 From: Elliot Cameron Date: Thu, 12 Sep 2019 23:37:16 -0400 Subject: [PATCH 04/11] Support both Babylon and Athens at once --- src/apdu_sign.c | 21 ++++++++++++++------- src/operations.c | 49 +++++++++++++++++++++++++++++++++++++----------- src/types.h | 12 ++++++++---- 3 files changed, 60 insertions(+), 22 deletions(-) diff --git a/src/apdu_sign.c b/src/apdu_sign.c index 0f16c847..59038529 100644 --- a/src/apdu_sign.c +++ b/src/apdu_sign.c @@ -90,13 +90,17 @@ static bool sign_reject(void) { static bool is_operation_allowed(enum operation_tag tag) { switch (tag) { - case OPERATION_TAG_DELEGATION: return true; - case OPERATION_TAG_REVEAL: return true; + case OPERATION_TAG_ATHENS_DELEGATION: return true; + case OPERATION_TAG_ATHENS_REVEAL: return true; + case OPERATION_TAG_BABYLON_DELEGATION: return true; + case OPERATION_TAG_BABYLON_REVEAL: return true; # ifndef BAKING_APP case OPERATION_TAG_PROPOSAL: return true; case OPERATION_TAG_BALLOT: return true; - case OPERATION_TAG_ORIGINATION: return true; - case OPERATION_TAG_TRANSACTION: return true; + case OPERATION_TAG_ATHENS_ORIGINATION: return true; + case OPERATION_TAG_ATHENS_TRANSACTION: return true; + case OPERATION_TAG_BABYLON_ORIGINATION: return true; + case OPERATION_TAG_BABYLON_TRANSACTION: return true; # endif default: return false; } @@ -250,7 +254,8 @@ bool prompt_transaction( ui_prompt(ballot_prompts, ok, cxl); } - case OPERATION_TAG_ORIGINATION: + case OPERATION_TAG_ATHENS_ORIGINATION: + case OPERATION_TAG_BABYLON_ORIGINATION: { static const uint32_t TYPE_INDEX = 0; static const uint32_t AMOUNT_INDEX = 1; @@ -324,7 +329,8 @@ bool prompt_transaction( ui_prompt(prompts, ok, cxl); } - case OPERATION_TAG_DELEGATION: + case OPERATION_TAG_ATHENS_DELEGATION: + case OPERATION_TAG_BABYLON_DELEGATION: { static const uint32_t TYPE_INDEX = 0; static const uint32_t FEE_INDEX = 1; @@ -365,7 +371,8 @@ bool prompt_transaction( ui_prompt(withdrawal ? withdrawal_prompts : delegation_prompts, ok, cxl); } - case OPERATION_TAG_TRANSACTION: + case OPERATION_TAG_ATHENS_TRANSACTION: + case OPERATION_TAG_BABYLON_TRANSACTION: { static const uint32_t TYPE_INDEX = 0; static const uint32_t AMOUNT_INDEX = 1; diff --git a/src/operations.c b/src/operations.c index 60fb6196..2c70fcf8 100644 --- a/src/operations.c +++ b/src/operations.c @@ -195,21 +195,45 @@ static void parse_operations_throws_parse_error( if (!is_operation_allowed(tag)) PARSE_ERROR(); - const struct implicit_contract *implicit_source = NEXT_TYPE(struct implicit_contract); - out->operation.source.originated = 0; - out->operation.source.signature_type = parse_raw_tezos_header_signature_type(&implicit_source->signature_type); - memcpy(out->operation.source.hash, implicit_source->pkh, sizeof(out->operation.source.hash)); + // Parse 'source' + switch (tag) { + // Tags that don't have "originated" byte only support tz accounts, not KT or tz. + case OPERATION_TAG_PROPOSAL: + case OPERATION_TAG_BALLOT: + case OPERATION_TAG_BABYLON_DELEGATION: + case OPERATION_TAG_BABYLON_ORIGINATION: + case OPERATION_TAG_BABYLON_REVEAL: + case OPERATION_TAG_BABYLON_TRANSACTION: { + struct implicit_contract const *const implicit_source = NEXT_TYPE(struct implicit_contract); + out->operation.source.originated = 0; + out->operation.source.signature_type = parse_raw_tezos_header_signature_type(&implicit_source->signature_type); + memcpy(out->operation.source.hash, implicit_source->pkh, sizeof(out->operation.source.hash)); + break; + } + + case OPERATION_TAG_ATHENS_DELEGATION: + case OPERATION_TAG_ATHENS_ORIGINATION: + case OPERATION_TAG_ATHENS_REVEAL: + case OPERATION_TAG_ATHENS_TRANSACTION: { + struct contract const *const source = NEXT_TYPE(struct contract); + parse_contract(&out->operation.source, source); + break; + } - if (!(tag == OPERATION_TAG_PROPOSAL || tag == OPERATION_TAG_BALLOT)) { + default: PARSE_ERROR(); + } + + // out->operation.source IS NORMALIZED AT THIS POINT + + // Parse common fields for non-governance related operations. + if (tag != OPERATION_TAG_PROPOSAL && tag != OPERATION_TAG_BALLOT) { out->total_fee += PARSE_Z(data, &ix, length); // fee PARSE_Z(data, &ix, length); // counter PARSE_Z(data, &ix, length); // gas limit out->total_storage_limit += PARSE_Z(data, &ix, length); // storage limit } - // out->operation.source IS NORMALIZED AT THIS POINT - - if (tag == OPERATION_TAG_REVEAL) { + if (tag == OPERATION_TAG_ATHENS_REVEAL || tag == OPERATION_TAG_BABYLON_REVEAL) { // Public key up next! Ensure it matches signing key. // Ignore source :-) and do not parse it from hdr. // We don't much care about reveals, they have very little in the way of bad security @@ -282,7 +306,8 @@ static void parse_operations_throws_parse_error( } } break; - case OPERATION_TAG_DELEGATION: + case OPERATION_TAG_ATHENS_DELEGATION: + case OPERATION_TAG_BABYLON_DELEGATION: { uint8_t delegate_present = NEXT_BYTE(data, &ix, length); if (delegate_present) { @@ -295,7 +320,8 @@ static void parse_operations_throws_parse_error( } } break; - case OPERATION_TAG_ORIGINATION: + case OPERATION_TAG_ATHENS_ORIGINATION: + case OPERATION_TAG_BABYLON_ORIGINATION: { struct origination_header { raw_tezos_header_signature_type_t signature_type; @@ -319,7 +345,8 @@ static void parse_operations_throws_parse_error( if (NEXT_BYTE(data, &ix, length) != 0) PARSE_ERROR(); // Has script } break; - case OPERATION_TAG_TRANSACTION: + case OPERATION_TAG_ATHENS_TRANSACTION: + case OPERATION_TAG_BABYLON_TRANSACTION: { out->operation.amount = PARSE_Z(data, &ix, length); diff --git a/src/types.h b/src/types.h index 917ccfd2..2925123b 100644 --- a/src/types.h +++ b/src/types.h @@ -213,10 +213,14 @@ enum operation_tag { OPERATION_TAG_NONE = -1, // Sentinal value, as 0 is possibly used for something OPERATION_TAG_PROPOSAL = 5, OPERATION_TAG_BALLOT = 6, - OPERATION_TAG_REVEAL = 107, - OPERATION_TAG_TRANSACTION = 108, - OPERATION_TAG_ORIGINATION = 109, - OPERATION_TAG_DELEGATION = 110, + OPERATION_TAG_ATHENS_REVEAL = 7, + OPERATION_TAG_ATHENS_TRANSACTION = 8, + OPERATION_TAG_ATHENS_ORIGINATION = 9, + OPERATION_TAG_ATHENS_DELEGATION = 10, + OPERATION_TAG_BABYLON_REVEAL = 107, + OPERATION_TAG_BABYLON_TRANSACTION = 108, + OPERATION_TAG_BABYLON_ORIGINATION = 109, + OPERATION_TAG_BABYLON_DELEGATION = 110, }; // TODO: Make this an enum. From 3698d9e334b207ed994436407dba862fe8f90047 Mon Sep 17 00:00:00 2001 From: Elliot Cameron Date: Thu, 12 Sep 2019 23:37:30 -0400 Subject: [PATCH 05/11] Use PARSE_ERROR consistently --- src/operations.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/operations.c b/src/operations.c index 2c70fcf8..250d1e39 100644 --- a/src/operations.c +++ b/src/operations.c @@ -118,7 +118,7 @@ static inline signature_type_t parse_raw_tezos_header_signature_type( case 0: return SIGNATURE_TYPE_ED25519; case 1: return SIGNATURE_TYPE_SECP256K1; case 2: return SIGNATURE_TYPE_SECP256R1; - default: THROW(EXC_PARSE_ERROR); + default: PARSE_ERROR(); } } From 143ad32a754dcdd6048d2b5bdeef16da0326b725 Mon Sep 17 00:00:00 2001 From: Elliot Cameron Date: Thu, 12 Sep 2019 23:48:25 -0400 Subject: [PATCH 06/11] Use PARSE_ERROR in apdu_sign --- src/apdu_sign.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/apdu_sign.c b/src/apdu_sign.c index 59038529..a27f68c1 100644 --- a/src/apdu_sign.c +++ b/src/apdu_sign.c @@ -18,6 +18,8 @@ #define G global.u.sign +#define PARSE_ERROR() THROW(EXC_PARSE_ERROR) + #define B2B_BLOCKBYTES 128 static inline void conditional_init_hash_state(blake2b_hash_state_t *const state) { @@ -167,11 +169,12 @@ size_t baking_sign_complete(bool const send_hash) { prompt_register_delegate(ok_c, sign_reject); } THROW(EXC_SECURITY); + break; } case MAGIC_BYTE_UNSAFE_OP2: case MAGIC_BYTE_UNSAFE_OP3: default: - THROW(EXC_PARSE_ERROR); + PARSE_ERROR(); } } @@ -194,7 +197,7 @@ bool prompt_transaction( switch (ops->operation.tag) { default: - THROW(EXC_PARSE_ERROR); + PARSE_ERROR(); case OPERATION_TAG_PROPOSAL: { @@ -464,7 +467,7 @@ static size_t wallet_sign_complete(uint8_t instruction) { case MAGIC_BYTE_BLOCK: case MAGIC_BYTE_BAKING_OP: default: - THROW(EXC_PARSE_ERROR); + PARSE_ERROR(); case MAGIC_BYTE_UNSAFE_OP: if (!G.maybe_ops.is_valid || !prompt_transaction(&G.maybe_ops.v, &G.key, ok_c, sign_reject)) { goto unsafe; @@ -545,9 +548,7 @@ static size_t handle_apdu(bool const enable_hashing, bool const enable_parsing, &G.hash_state); } - if (G.message_data_length + buff_size > sizeof(G.message_data)) { - THROW(EXC_PARSE_ERROR); - } + if (G.message_data_length + buff_size > sizeof(G.message_data)) PARSE_ERROR(); memmove(G.message_data + G.message_data_length, buff, buff_size); G.message_data_length += buff_size; From badf0ec9dd2e969acc77ebc09a86458907f42609 Mon Sep 17 00:00:00 2001 From: Elliot Cameron Date: Thu, 12 Sep 2019 23:52:09 -0400 Subject: [PATCH 07/11] Fix streaming logic 1) For Baking app, only parse one data packet 2) For Wallet app, if multiple data packets come in, treat that as unparseable (forcing Sign Hash prompt) 3) Limit the number of overall packets to 255 before failing --- src/apdu_sign.c | 74 +++++++++++++++++++++++++++++++------------------ src/globals.h | 9 +++--- 2 files changed, 51 insertions(+), 32 deletions(-) diff --git a/src/apdu_sign.c b/src/apdu_sign.c index a27f68c1..859b6a03 100644 --- a/src/apdu_sign.c +++ b/src/apdu_sign.c @@ -144,22 +144,19 @@ __attribute__((noreturn)) static void prompt_register_delegate( } size_t baking_sign_complete(bool const send_hash) { - switch (G.magic_number) { + switch (G.magic_byte) { case MAGIC_BYTE_BLOCK: case MAGIC_BYTE_BAKING_OP: - if (G.maybe_parsed_baking_data.is_valid) { - guard_baking_authorized(&G.maybe_parsed_baking_data.v, &G.key); - return perform_signature(true, send_hash); - } else { - THROW(EXC_PARSE_ERROR); - } + guard_baking_authorized(&G.parsed_baking_data, &G.key); + return perform_signature(true, send_hash); break; case MAGIC_BYTE_UNSAFE_OP: { + if (!G.maybe_ops.is_valid) PARSE_ERROR(); + // Must be self-delegation signed by the *authorized* baking key - if (G.maybe_ops.is_valid && - bip32_path_with_curve_eq(&G.key, &N_data.baking_key) && + if (bip32_path_with_curve_eq(&G.key, &N_data.baking_key) && // ops->signing is generated from G.bip32_path and G.curve COMPARE(&G.maybe_ops.v.operation.source, &G.maybe_ops.v.signing) == 0 && @@ -463,7 +460,7 @@ static size_t wallet_sign_complete(uint8_t instruction) { } else { ui_callback_t const ok_c = instruction == INS_SIGN_WITH_HASH ? sign_with_hash_ok : sign_without_hash_ok; - switch (G.magic_number) { + switch (G.magic_byte) { case MAGIC_BYTE_BLOCK: case MAGIC_BYTE_BAKING_OP: default: @@ -494,6 +491,24 @@ static size_t wallet_sign_complete(uint8_t instruction) { #define P1_HASH_ONLY_NEXT 0x03 // You only need it once #define P1_LAST_MARKER 0x80 +static uint8_t get_magic_byte_or_throw(uint8_t const *const buff, size_t const buff_size) { + uint8_t const magic_byte = get_magic_byte(buff, buff_size); + switch (magic_byte) { +# ifdef BAKING_APP + case MAGIC_BYTE_BLOCK: + case MAGIC_BYTE_BAKING_OP: + case MAGIC_BYTE_UNSAFE_OP: // Only for self-delegations +# else + case MAGIC_BYTE_UNSAFE_OP: +# endif + return magic_byte; + + case MAGIC_BYTE_UNSAFE_OP2: + case MAGIC_BYTE_UNSAFE_OP3: + default: PARSE_ERROR(); + } +} + static size_t handle_apdu(bool const enable_hashing, bool const enable_parsing, uint8_t const instruction) { uint8_t *const buff = &G_io_apdu_buffer[OFFSET_CDATA]; uint8_t const p1 = READ_UNALIGNED_BIG_ENDIAN(uint8_t, &G_io_apdu_buffer[OFFSET_P1]); @@ -516,28 +531,35 @@ static size_t handle_apdu(bool const enable_hashing, bool const enable_parsing, case P1_NEXT: if (G.key.bip32_path.length == 0) { THROW(EXC_WRONG_LENGTH_FOR_INS); - } + // Guard against overflow + if (G.packet_index >= 0xFF) PARSE_ERROR(); + G.packet_index++; + break; default: THROW(EXC_WRONG_PARAM); } if (enable_parsing) { - if (G.magic_number == MAGIC_BYTE_INVALID) { - G.magic_number = get_magic_byte(buff, buff_size); - } - # ifdef BAKING_APP - // Keep trying to parse baking data unless we already did so successfully. - G.maybe_parsed_baking_data.is_valid = - G.maybe_parsed_baking_data.is_valid - || parse_baking_data(&G.maybe_parsed_baking_data.v, buff, buff_size); -# endif + if (G.packet_index != 1) PARSE_ERROR(); // Only parse a single packet when baking - // Keep trying to parse operations. - G.maybe_ops.is_valid = - G.maybe_ops.is_valid - || parse_allowed_operations(&G.maybe_ops.v, buff, buff_size, &G.key); + G.magic_byte = get_magic_byte_or_throw(buff, buff_size); + if (G.magic_byte == MAGIC_BYTE_UNSAFE_OP) { + // Parse the operation. It will be verified in `baking_sign_complete`. + G.maybe_ops.is_valid = parse_allowed_operations(&G.maybe_ops.v, buff, buff_size, &G.key); + } else { + // This should be a baking operation so parse it. + if (!parse_baking_data(&G.parsed_baking_data, buff, buff_size)) PARSE_ERROR(); + } +# else + if (G.packet_index == 1) { + G.magic_byte = get_magic_byte_or_throw(buff, buff_size); + G.maybe_ops.is_valid = parse_allowed_operations(&G.maybe_ops.v, buff, buff_size, &G.key); + } else { + G.maybe_ops.is_valid = false; // Force multiple packets to be treated as unparsed + } +# endif } if (enable_hashing) { @@ -592,9 +614,7 @@ size_t handle_apdu_sign_with_hash(uint8_t instruction) { static int perform_signature(bool const on_hash, bool const send_hash) { # ifdef BAKING_APP - if (G.maybe_parsed_baking_data.is_valid) { - write_high_water_mark(&G.maybe_parsed_baking_data.v); - } + write_high_water_mark(&G.parsed_baking_data); # else if (on_hash && G.hash_only) { memcpy(G_io_apdu_buffer, G.final_hash, sizeof(G.final_hash)); diff --git a/src/globals.h b/src/globals.h index 0cd46f5f..9fb6c3bf 100644 --- a/src/globals.h +++ b/src/globals.h @@ -38,11 +38,10 @@ typedef struct { typedef struct { bip32_path_with_curve_t key; + uint8_t packet_index; // 0-index is the initial setup packet, 1 is first packet to hash, etc. + # ifdef BAKING_APP - struct { - bool is_valid; - parsed_baking_data_t v; - } maybe_parsed_baking_data; + parsed_baking_data_t parsed_baking_data; # endif struct { @@ -57,7 +56,7 @@ typedef struct { blake2b_hash_state_t hash_state; uint8_t final_hash[SIGN_HASH_SIZE]; - uint8_t magic_number; + uint8_t magic_byte; bool hash_only; } apdu_sign_state_t; From afd5d24f8c1209bd85e9de54fbfc8cbe6d135fa3 Mon Sep 17 00:00:00 2001 From: Elliot Cameron Date: Thu, 12 Sep 2019 23:54:49 -0400 Subject: [PATCH 08/11] Use consistent styling for exceptions in apdu_sign --- src/apdu_sign.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/apdu_sign.c b/src/apdu_sign.c index 859b6a03..03771646 100644 --- a/src/apdu_sign.c +++ b/src/apdu_sign.c @@ -529,8 +529,8 @@ static size_t handle_apdu(bool const enable_hashing, bool const enable_parsing, // FALL THROUGH #endif case P1_NEXT: - if (G.key.bip32_path.length == 0) { - THROW(EXC_WRONG_LENGTH_FOR_INS); + if (G.key.bip32_path.length == 0) THROW(EXC_WRONG_LENGTH_FOR_INS); + // Guard against overflow if (G.packet_index >= 0xFF) PARSE_ERROR(); G.packet_index++; From ce117df46c520626209f62fb02846bd91f1290f8 Mon Sep 17 00:00:00 2001 From: Elliot Cameron Date: Thu, 12 Sep 2019 21:08:54 -0400 Subject: [PATCH 09/11] Reset APDU state when there is an exception --- src/apdu.c | 2 ++ src/apdu_baking.c | 2 +- src/apdu_hmac.c | 2 +- src/apdu_pubkey.c | 2 +- src/apdu_setup.c | 2 +- src/apdu_sign.c | 2 +- src/globals.c | 4 +++ src/globals.h | 87 +++++++++++++++++++++++++---------------------- src/keys.c | 4 +-- 9 files changed, 60 insertions(+), 47 deletions(-) diff --git a/src/apdu.c b/src/apdu.c index 364ecc6c..0d0b5fd7 100644 --- a/src/apdu.c +++ b/src/apdu.c @@ -76,6 +76,8 @@ void main_loop(apdu_handler const *const handlers, size_t const handlers_size) { THROW(EXCEPTION_IO_RESET); } CATCH_OTHER(e) { + clear_apdu_globals(); // IMPORTANT: Application state must not persist through errors + uint16_t sw = e; switch (sw) { default: diff --git a/src/apdu_baking.c b/src/apdu_baking.c index ad9bbaae..dfe5ae7d 100644 --- a/src/apdu_baking.c +++ b/src/apdu_baking.c @@ -12,7 +12,7 @@ #include -#define G global.u.baking +#define G global.apdu.u.baking static bool reset_ok(void); diff --git a/src/apdu_hmac.c b/src/apdu_hmac.c index 94b7d4bf..0935a5a4 100644 --- a/src/apdu_hmac.c +++ b/src/apdu_hmac.c @@ -8,7 +8,7 @@ #include "protocol.h" -#define G global.u.hmac +#define G global.apdu.u.hmac static inline size_t hmac( uint8_t *const out, size_t const out_size, diff --git a/src/apdu_pubkey.c b/src/apdu_pubkey.c index eda51de8..28b8b59e 100644 --- a/src/apdu_pubkey.c +++ b/src/apdu_pubkey.c @@ -10,7 +10,7 @@ #include -#define G global.u.pubkey +#define G global.apdu.u.pubkey static bool pubkey_ok(void) { delayed_send(provide_pubkey(G_io_apdu_buffer, &G.public_key)); diff --git a/src/apdu_setup.c b/src/apdu_setup.c index a360e458..279761e1 100644 --- a/src/apdu_setup.c +++ b/src/apdu_setup.c @@ -11,7 +11,7 @@ #include -#define G global.u.setup +#define G global.apdu.u.setup struct setup_wire { uint32_t main_chain_id; diff --git a/src/apdu_sign.c b/src/apdu_sign.c index 03771646..ae96a374 100644 --- a/src/apdu_sign.c +++ b/src/apdu_sign.c @@ -16,7 +16,7 @@ #include -#define G global.u.sign +#define G global.apdu.u.sign #define PARSE_ERROR() THROW(EXC_PARSE_ERROR) diff --git a/src/globals.c b/src/globals.c index 7e83095d..8115caef 100644 --- a/src/globals.c +++ b/src/globals.c @@ -30,6 +30,10 @@ globals_t global; unsigned char G_io_seproxyhal_spi_buffer[IO_SEPROXYHAL_BUFFER_SIZE_B]; +void clear_apdu_globals(void) { + memset(&global.apdu, 0, sizeof(global.apdu)); +} + void init_globals(void) { memset(&global, 0, sizeof(global)); diff --git a/src/globals.h b/src/globals.h index 9fb6c3bf..bcf8e7bc 100644 --- a/src/globals.h +++ b/src/globals.h @@ -5,6 +5,11 @@ #include "bolos_target.h" +// Zeros out all globals that can keep track of APDU instruction state. +// Notably this does *not* include UI state. +void clear_apdu_globals(void); + +// Zeros out all application-specific globals and SDK-specific UI/exchange buffers. void init_globals(void); #define MAX_APDU_SIZE 230 // Maximum number of bytes in a single APDU @@ -64,32 +69,6 @@ typedef struct { void *stack_root; apdu_handler handlers[INS_MAX + 1]; - union { - struct { - bip32_path_with_curve_t key; - cx_ecfp_public_key_t public_key; - } pubkey; - - apdu_sign_state_t sign; - -# ifdef BAKING_APP - struct { - level_t reset_level; - } baking; - - struct { - bip32_path_with_curve_t key; - chain_id_t main_chain_id; - struct { - level_t main; - level_t test; - } hwm; - } setup; - - apdu_hmac_state_t hmac; -# endif - } u; - struct { ui_callback_t ok_callback; ui_callback_t cxl_callback; @@ -128,21 +107,49 @@ typedef struct { } prompt; } ui; -#ifdef BAKING_APP struct { - nvram_data new_data; // Staging area for setting N_data + union { + struct { + bip32_path_with_curve_t key; + cx_ecfp_public_key_t public_key; + } pubkey; + + apdu_sign_state_t sign; + +# ifdef BAKING_APP + struct { + level_t reset_level; + } baking; + + struct { + bip32_path_with_curve_t key; + chain_id_t main_chain_id; + struct { + level_t main; + level_t test; + } hwm; + } setup; + + apdu_hmac_state_t hmac; +# endif + } u; + +# ifdef BAKING_APP + struct { + nvram_data new_data; // Staging area for setting N_data - char address_display_data[VALUE_WIDTH + 1]; - } baking_auth; -#endif + char address_display_data[VALUE_WIDTH + 1]; + } baking_auth; +# endif - struct { - struct priv_generate_key_pair generate_key_pair; + struct { + struct priv_generate_key_pair generate_key_pair; - struct { - cx_ecfp_public_key_t compressed; - } public_key_hash; - } priv; + struct { + cx_ecfp_public_key_t compressed; + } public_key_hash; + } priv; + } apdu; } globals_t; extern globals_t global; @@ -182,10 +189,10 @@ high_watermark_t volatile *select_hwm_by_chain(chain_id_t const chain_id, nvram_ // 'out_param' defines the name of a pointer to the nvram_data struct // that 'body' can change to apply updates. #define UPDATE_NVRAM(out_name, body) ({ \ - nvram_data *const out_name = &global.baking_auth.new_data; \ - memcpy(&global.baking_auth.new_data, (nvram_data const *const)&N_data, sizeof(global.baking_auth.new_data)); \ + nvram_data *const out_name = &global.apdu.baking_auth.new_data; \ + memcpy(&global.apdu.baking_auth.new_data, (nvram_data const *const)&N_data, sizeof(global.apdu.baking_auth.new_data)); \ body; \ - nvm_write((void*)&N_data, &global.baking_auth.new_data, sizeof(N_data)); \ + nvm_write((void*)&N_data, &global.apdu.baking_auth.new_data, sizeof(N_data)); \ update_baking_idle_screens(); \ }) #endif diff --git a/src/keys.c b/src/keys.c index b0ab5fe8..db4fd997 100644 --- a/src/keys.c +++ b/src/keys.c @@ -50,7 +50,7 @@ key_pair_t *generate_key_pair_return_global( bip32_path_t const *const bip32_path ) { check_null(bip32_path); - struct priv_generate_key_pair *const priv = &global.priv.generate_key_pair; + struct priv_generate_key_pair *const priv = &global.apdu.priv.generate_key_pair; cx_curve_t const cx_curve = signature_type_to_cx_curve(derivation_type_to_signature_type(derivation_type)); @@ -98,7 +98,7 @@ cx_ecfp_public_key_t const *public_key_hash_return_global( check_null(public_key); if (out_size < HASH_SIZE) THROW(EXC_WRONG_LENGTH); - cx_ecfp_public_key_t *const compressed = &global.priv.public_key_hash.compressed; + cx_ecfp_public_key_t *const compressed = &global.apdu.priv.public_key_hash.compressed; switch (derivation_type_to_signature_type(curve)) { case SIGNATURE_TYPE_ED25519: { From 53cf7ef5dda0f9a8c54eb503332f92091d3c7816 Mon Sep 17 00:00:00 2001 From: Elliot Cameron Date: Thu, 12 Sep 2019 21:22:37 -0400 Subject: [PATCH 10/11] Add some primitive tests for transactions --- test/baking_test.sh | 32 +++++++++++++++++++++++++++++--- test/transaction.sh | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 3 deletions(-) create mode 100755 test/transaction.sh diff --git a/test/baking_test.sh b/test/baking_test.sh index 3ba57831..4e6ba051 100755 --- a/test/baking_test.sh +++ b/test/baking_test.sh @@ -1,11 +1,34 @@ #!/usr/bin/env bash -set -euo pipefail +set -Eeuo pipefail DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" cd "$DIR" { + echo "Authorize baking" + { echo 8001000011048000002c800006c18000000080000000 # Authorize baking + } | ./apdu.sh +} + + +{ + echo "Baking a block with multiple packets should fail" + + { + echo 800681000400000000 # Reset HWM + + echo 8004000011048000002c800006c18000000080000000 + echo 800401000100 # Prefix packet starting with 00 + echo 800481000a017a06a7700000000102 # Bake block at level 1 + } | ./apdu.sh || true + echo "Pass" +} + +{ + echo "Endorsing a previous level should fail" + + { echo 800681000400000000 # Reset HWM echo 8004000011048000002c800006c18000000080000000 @@ -16,8 +39,11 @@ cd "$DIR" echo 8004000011048000002c800006c18000000080000000 echo 800481002a027a06a77000000000000000000000000000000000000000000000000000000000000000000000000002 # Endorse at level 2 + } | ./apdu.sh + { echo 8004000011048000002c800006c18000000080000000 echo 800481002a027a06a77000000000000000000000000000000000000000000000000000000000000000000000000001 # Endorse at level 1 (should fail) - -} | ./apdu.sh + } | ./apdu.sh || true + echo "Pass" +} diff --git a/test/transaction.sh b/test/transaction.sh new file mode 100755 index 00000000..bbbd9e64 --- /dev/null +++ b/test/transaction.sh @@ -0,0 +1,43 @@ +#!/usr/bin/env bash +set -Eeuo pipefail + +DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" +cd "$DIR" + +{ + echo "Exceptions should not allow new packets to be parsed without starting over" + + { + echo 800f000311048000002c800006c18000000080000000 + # Wrong length: should be 0x97 + echo 800f0100960316d8aa98f84a30af5871642e9fab07597a14bf0a9a4f37bb8b734fd28007cee10700006fd9ff5e5aad9738883f9d291dd67f888221ad8fea0902904e000050a7e13e2ce14fd935a4258dbff7f4874421981177e083dd7a3a505d16b1e31d0800006fd9ff5e5aad9738883f9d291dd67f888221ad8fa20903bc5000c0c3930700006fd9ff5e5aad9738883f9d291dd67f888221ad8f00 + } | ./apdu.sh + + echo "After exception" + + { + # Valid second packet, but no first packet. + echo 800f8100970316d8aa98f84a30af5871642e9fab07597a14bf0a9a4f37bb8b734fd28007cee10700006fd9ff5e5aad9738883f9d291dd67f888221ad8fea0902904e000050a7e13e2ce14fd935a4258dbff7f4874421981177e083dd7a3a505d16b1e31d0800006fd9ff5e5aad9738883f9d291dd67f888221ad8fa20903bc5000c0c3930700006fd9ff5e5aad9738883f9d291dd67f888221ad8f00 + } | ./apdu.sh || echo "Pass" +} + + +{ + echo "In-between packets should cause Sign Unverified (ACCEPT THIS)" + + { + echo 800f000311048000002c800006c18000000080000000 + echo 800f01000103 + echo 800f0100020000 + echo 800f8100970316d8aa98f84a30af5871642e9fab07597a14bf0a9a4f37bb8b734fd28007cee10700006fd9ff5e5aad9738883f9d291dd67f888221ad8fea0902904e000050a7e13e2ce14fd935a4258dbff7f4874421981177e083dd7a3a505d16b1e31d0800006fd9ff5e5aad9738883f9d291dd67f888221ad8fa20903bc5000c0c3930700006fd9ff5e5aad9738883f9d291dd67f888221ad8f00 + } | ./apdu.sh +} + +{ + echo "Valid transaction should be parsed (ACCEPT THIS)" + + { + echo 800f000311048000002c800006c18000000080000000 + echo 800f8100970316d8aa98f84a30af5871642e9fab07597a14bf0a9a4f37bb8b734fd28007cee10700006fd9ff5e5aad9738883f9d291dd67f888221ad8fea0902904e000050a7e13e2ce14fd935a4258dbff7f4874421981177e083dd7a3a505d16b1e31d0800006fd9ff5e5aad9738883f9d291dd67f888221ad8fa20903bc5000c0c3930700006fd9ff5e5aad9738883f9d291dd67f888221ad8f00 + } | ./apdu.sh +} From ffb7489a649950b252b021f807329d4f7bf5f60c Mon Sep 17 00:00:00 2001 From: Elliot Cameron Date: Fri, 13 Sep 2019 14:55:12 -0400 Subject: [PATCH 11/11] Version 2.2.0 --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 2cb0546d..34b9da28 100644 --- a/Makefile +++ b/Makefile @@ -18,7 +18,7 @@ GIT_DESCRIBE ?= $(shell git describe --tags --abbrev=8 --always --long --dirty 2 VERSION_TAG ?= $(shell echo "$(GIT_DESCRIBE)" | cut -f1 -d-) APPVERSION_M=2 -APPVERSION_N=1 +APPVERSION_N=2 APPVERSION_P=0 APPVERSION=$(APPVERSION_M).$(APPVERSION_N).$(APPVERSION_P)