Merge pull request #57 from obsidiansystems/eac-fix-crash

Fix crash due to null operations parsing output

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) {

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;
 

src/memory.h

@@ -2,8 +2,13 @@
 
 #include <string.h>
 
+#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))

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!
 }

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
+);

src/types.h

@@ -7,6 +7,12 @@
 #include <stdbool.h>
 #include <string.h>
 
+// 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);