From 273c0877421409f3d242b3e20edb5e055df5ef84 Mon Sep 17 00:00:00 2001 From: Elliot Cameron Date: Thu, 28 Feb 2019 19:00:16 -0500 Subject: [PATCH 1/2] Fix crash due to null operations parsing output --- src/apdu_sign.c | 19 +++++++++---------- src/globals.h | 8 +------- src/memory.h | 5 +++++ src/operations.c | 18 ++++++++++-------- src/operations.h | 14 ++++++++------ 5 files changed, 33 insertions(+), 31 deletions(-) diff --git a/src/apdu_sign.c b/src/apdu_sign.c index 4f9dc7bc..6ad6d8ce 100644 --- a/src/apdu_sign.c +++ b/src/apdu_sign.c @@ -97,7 +97,7 @@ __attribute__((noreturn)) static void prompt_register_delegate( REGISTER_STATIC_UI_VALUE(TYPE_INDEX, "as delegate?"); register_ui_callback(ADDRESS_INDEX, bip32_path_with_curve_to_pkh_string, &G.key); - register_ui_callback(FEE_INDEX, microtez_to_string_indirect, &G.register_delegate.total_fee); + register_ui_callback(FEE_INDEX, microtez_to_string_indirect, &G.ops.total_fee); ui_prompt(prompts, NULL, ok_cb, cxl_cb); } @@ -120,19 +120,18 @@ uint32_t baking_sign_complete(void) { allow_operation(&allowed, OPERATION_TAG_DELEGATION); allow_operation(&allowed, OPERATION_TAG_REVEAL); - struct parsed_operation_group *ops = - parse_operations( - G.message_data, G.message_data_length, - G.key.curve, &G.key.bip32_path, allowed); + parse_operations( + &G.ops, + G.message_data, G.message_data_length, + G.key.curve, &G.key.bip32_path, allowed); // Must be self-delegation signed by the *authorized* 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(&ops->operation.source, &ops->signing) == 0 && - COMPARE(&ops->operation.destination, &ops->signing) == 0 + COMPARE(&G.ops.operation.source, &G.ops.signing) == 0 && + COMPARE(&G.ops.operation.destination, &G.ops.signing) == 0 ) { - G.register_delegate.total_fee = ops->total_fee; prompt_register_delegate(sign_ok, sign_reject); } THROW(EXC_SECURITY); @@ -162,7 +161,7 @@ static bool prompt_transaction(const void *data, size_t length, cx_curve_t curve ui_callback_t ok, ui_callback_t cxl) { check_null(data); check_null(bip32_path); - struct parsed_operation_group *ops; + struct parsed_operation_group *const ops = &G.ops; #ifndef TEZOS_DEBUG BEGIN_TRY { // TODO: Eventually, "unsafe" operations will be another APDU, @@ -181,7 +180,7 @@ static bool prompt_transaction(const void *data, size_t length, cx_curve_t curve allow_operation(&allowed, OPERATION_TAG_TRANSACTION); // TODO: Add still other operations - ops = parse_operations(data, length, curve, bip32_path, allowed); + parse_operations(ops, data, length, curve, bip32_path, allowed); #ifndef TEZOS_DEBUG } CATCH_OTHER(e) { diff --git a/src/globals.h b/src/globals.h index 977518bf..e7fca542 100644 --- a/src/globals.h +++ b/src/globals.h @@ -46,9 +46,7 @@ typedef struct { uint8_t magic_number; bool hash_only; - struct { - uint64_t total_fee; - } register_delegate; + struct parsed_operation_group ops; } sign; struct { @@ -103,10 +101,6 @@ typedef struct { struct { cx_ecfp_public_key_t compressed; } public_key_hash; - - struct { - struct parsed_operation_group out; - } parse_operations; } priv; } globals_t; diff --git a/src/memory.h b/src/memory.h index dea9ea22..5ae024a9 100644 --- a/src/memory.h +++ b/src/memory.h @@ -2,8 +2,13 @@ #include +#include "exception.h" + + #define COMPARE(a, b) ({ \ _Static_assert(sizeof(*a) == sizeof(*b), "Size mismatch in COMPARE"); \ + check_null(a); \ + check_null(b); \ memcmp(a, b, sizeof(*a)); \ }) #define NUM_ELEMENTS(a) (sizeof(a)/sizeof(*a)) diff --git a/src/operations.c b/src/operations.c index 104eda1d..9799ede4 100644 --- a/src/operations.c +++ b/src/operations.c @@ -143,13 +143,17 @@ static inline void parse_contract(struct parsed_contract *out, const struct cont } } -struct parsed_operation_group *parse_operations(const void *data, size_t length, cx_curve_t curve, - bip32_path_t const *const bip32_path, - allowed_operation_set ops) { +void parse_operations( + struct parsed_operation_group *const out, + void const *const data, + size_t length, + cx_curve_t curve, + bip32_path_t const *const bip32_path, + allowed_operation_set ops +) { + check_null(out); check_null(data); check_null(bip32_path); - - struct parsed_operation_group *const out = &global.priv.parse_operations.out; memset(out, 0, sizeof(*out)); out->operation.tag = OPERATION_TAG_NONE; @@ -216,7 +220,7 @@ struct parsed_operation_group *parse_operations(const void *data, size_t length, // If the source is an implicit contract,... if (out->operation.source.originated == 0) { // ... it had better match our key, otherwise why are we signing it? - if (COMPARE(&out->operation.source, &out->signing)) return false; + if (COMPARE(&out->operation.source, &out->signing) != 0) PARSE_ERROR(); } // OK, it passes muster. @@ -317,6 +321,4 @@ struct parsed_operation_group *parse_operations(const void *data, size_t length, if (out->operation.tag == OPERATION_TAG_NONE && !out->has_reveal) { PARSE_ERROR(); // Must have at least one op } - - return out; // Success! } diff --git a/src/operations.h b/src/operations.h index c3d909dc..ce5f7ab3 100644 --- a/src/operations.h +++ b/src/operations.h @@ -28,9 +28,11 @@ static inline void clear_operation_set(allowed_operation_set *ops) { // Throws upon invalid data. // Allows arbitrarily many "REVEAL" operations but only one operation of any other type, // which is the one it puts into the group. - -// Returns pointer to static data -- non-reentrant as hell. -struct parsed_operation_group * -parse_operations(const void *data, size_t length, cx_curve_t curve, - bip32_path_t const *const bip32_path, - allowed_operation_set ops); +void parse_operations( + struct parsed_operation_group *const out, + void const *const data, + size_t length, + cx_curve_t curve, + bip32_path_t const *const bip32_path, + allowed_operation_set ops +); From 8cef8f8108cecb3d060d8b009d1da2a9b6577ac3 Mon Sep 17 00:00:00 2001 From: Elliot Cameron Date: Fri, 1 Mar 2019 11:46:50 -0500 Subject: [PATCH 2/2] Don't let it happen again: Define type-safe true/false --- src/types.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/types.h b/src/types.h index 7a11cb8a..8409d3ad 100644 --- a/src/types.h +++ b/src/types.h @@ -7,6 +7,12 @@ #include #include +// Type-safe versions of true/false +#undef true +#define true ((bool)1) +#undef false +#define false ((bool)0) + // Return number of bytes to transmit (tx) typedef size_t (*apdu_handler)(uint8_t instruction);