Security review

src/stellar.c

@@ -17,6 +17,7 @@
 
 #include "os.h"
 #include "cx.h"
+#include "os_io_seproxyhal.h"
 
 #include "stellar_api.h"
 #include "stellar_types.h"

src/stellar_api.h

@@ -80,7 +80,7 @@ void encode_hash_x_key(const uint8_t *in, char *out);
 void print_public_key(const uint8_t *in, char *out, uint8_t numCharsL, uint8_t numCharsR);
 
 /** output first numCharsL of input + last numCharsR of input separated by ".." */
-void print_summary(char *in, char *out, uint8_t numCharsL, uint8_t numCharsR);
+void print_summary(const char *in, char *out, uint8_t numCharsL, uint8_t numCharsR);
 
 /** raw byte buffer to hexadecimal string representation.
  * len is length of input, provided output must be twice that size */

src/stellar_format.h

@@ -26,7 +26,7 @@
 typedef void (*format_function_t)(tx_context_t *txCtx);
 
 /* 15 formatters in a row ought to be enough for everybody*/
-#define MAX_FORMATTERS_PER_OPERATION 15
+#define MAX_FORMATTERS_PER_OPERATION 16
 
 /* the current formatter */
 extern format_function_t formatter;

src/stellar_format_common.c

@@ -2,6 +2,10 @@
 #include "stellar_vars.h"
 #include "stellar_api.h"
 
+#ifdef TEST
+#include <bsd/string.h>
+#endif
+
 uint8_t current_data_index;
 
 format_function_t get_formatter(tx_context_t *txCtx, bool forward) {
@@ -33,6 +37,7 @@ format_function_t get_formatter(tx_context_t *txCtx, bool forward) {
         default:
             THROW(0x6123);
     }
+    return NULL;
 }
 
 void ui_approve_tx_next_screen(tx_context_t *txCtx) {
@@ -68,9 +73,9 @@ void set_state_data(bool forward) {
         formatter_stack[formatter_index](&ctx.req.tx);
 
         if (opCaption[0] != '\0') {
-            strncpy(detailCaption, opCaption, sizeof(detailCaption));
+            strlcpy(detailCaption, opCaption, sizeof(detailCaption));
             detailValue[0] = ' ';
-            PRINTF("caption: %s %u\n", detailCaption);
+            PRINTF("caption: %s\n", detailCaption);
         } else if (detailCaption[0] != '\0' && detailValue[0] != '\0') {
             PRINTF("caption: %s\n", detailCaption);
             PRINTF("details: %s\n", detailValue);

src/stellar_format_nanos.c

@@ -29,7 +29,7 @@
 
 char opCaption[20];
 char detailCaption[20];
-char detailValue[89];
+char detailValue[DETAIL_VALUE_MAX_SIZE];
 
 format_function_t formatter_stack[MAX_FORMATTERS_PER_OPERATION];
 int8_t formatter_index;
@@ -77,7 +77,9 @@ void format_time_bounds(tx_context_t *txCtx) {
 
 void format_network(tx_context_t *txCtx) {
     strcpy(detailCaption, "Network");
-    strcpy(detailValue, ((char *) PIC(NETWORK_NAMES[txCtx->txDetails.network])));
+    strlcpy(detailValue,
+            (char *) PIC(NETWORK_NAMES[txCtx->txDetails.network]),
+            DETAIL_VALUE_MAX_SIZE);
     push_to_formatter_stack(&format_time_bounds);
 }
 

src/stellar_parser.c

@@ -68,9 +68,9 @@ static bool buffer_read64(buffer_t *buffer, uint64_t *n) {
     }
 
     const uint8_t *ptr = buffer->ptr + buffer->offset;
-    uint64_t i1 = ptr[3] + (ptr[2] << 8u) + (ptr[1] << 16u) + (ptr[0] << 24u);
+    uint32_t i1 = ptr[3] + (ptr[2] << 8u) + (ptr[1] << 16u) + (ptr[0] << 24u);
     uint32_t i2 = ptr[7] + (ptr[6] << 8u) + (ptr[5] << 16u) + (ptr[4] << 24u);
-    *n = i2 | (i1 << 32u);
+    *n = i2 | ((uint64_t) i1 << 32u);
     buffer->offset += 8;
     return true;
 }
@@ -130,17 +130,17 @@ bool parse_account_id(buffer_t *buffer, const uint8_t **account_id) {
 }
 
 static bool parse_network(buffer_t *buffer, tx_details_t *txDetails) {
-    if (!buffer_can_read(buffer, 32)) {
+    if (!buffer_can_read(buffer, HASH_SIZE)) {
         return false;
     }
-    if (memcmp(buffer->ptr, NETWORK_ID_PUBLIC_HASH, 32) == 0) {
+    if (memcmp(buffer->ptr, NETWORK_ID_PUBLIC_HASH, HASH_SIZE) == 0) {
         network_id = txDetails->network = NETWORK_TYPE_PUBLIC;
-    } else if (memcmp(buffer->ptr, NETWORK_ID_TEST_HASH, 32) == 0) {
+    } else if (memcmp(buffer->ptr, NETWORK_ID_TEST_HASH, HASH_SIZE) == 0) {
         network_id = txDetails->network = NETWORK_TYPE_TEST;
     } else {
         network_id = txDetails->network = NETWORK_TYPE_UNKNOWN;
     }
-    buffer_advance(buffer, 32);
+    buffer_advance(buffer, HASH_SIZE);
     return true;
 }
 
@@ -219,11 +219,11 @@ static bool parse_memo(buffer_t *buffer, tx_details_t *txDetails) {
         }
         case MEMO_TYPE_HASH:
         case MEMO_TYPE_RETURN:
-            if (buffer->size - buffer->offset < 32) {
+            if (buffer->size - buffer->offset < HASH_SIZE) {
                 return false;
             }
-            print_binary(buffer->ptr + buffer->offset, txDetails->memo.data, 32);
-            buffer->offset += 32;
+            print_binary(buffer->ptr + buffer->offset, txDetails->memo.data, HASH_SIZE);
+            buffer->offset += HASH_SIZE;
             return true;
         default:
             return false;  // unknown memo type

src/stellar_types.h

@@ -58,6 +58,8 @@
 /* max amount is max int64 scaled down: "922337203685.4775807" */
 #define AMOUNT_MAX_SIZE 21
 
+#define HASH_SIZE 32
+
 // ------------------------------------------------------------------------- //
 //                       TRANSACTION PARSING CONSTANTS                       //
 // ------------------------------------------------------------------------- //
@@ -130,6 +132,8 @@ static const char *NETWORK_NAMES[3] = {"Public", "Test", "Unknown"};
 
 #ifdef TEST
 #include <stdio.h>
+#include <string.h>
+
 #define THROW(code)                \
     do {                           \
         printf("error: %d", code); \
@@ -264,7 +268,8 @@ typedef struct {
 
 typedef struct {
     uint8_t type;
-    char data[65];
+    // Hash in hexa, preceeded by "0x"
+    char data[2 * HASH_SIZE + 2 + 1];
 } memo_t;
 
 typedef struct {
@@ -294,7 +299,7 @@ typedef struct {
     uint32_t bip32[MAX_BIP32_LEN];
     uint8_t raw[MAX_RAW_TX];
     uint32_t rawLength;
-    uint8_t hash[32];
+    uint8_t hash[HASH_SIZE];
     uint16_t offset;
     operation_details_t opDetails;
     tx_details_t txDetails;

src/stellar_utils.c

@@ -158,7 +158,7 @@ void encode_hash_x_key(const uint8_t *in, char *out) {
     encode_key(in, out, 23 << 3);
 }
 
-void print_summary(char *in, char *out, uint8_t numCharsL, uint8_t numCharsR) {
+void print_summary(const char *in, char *out, uint8_t numCharsL, uint8_t numCharsR) {
     uint8_t outLength = numCharsL + numCharsR + 2;
     uint16_t inLength = strlen(in);
     if (inLength > outLength) {

test/CMakeLists.txt

@@ -14,7 +14,7 @@ add_executable(test_printers
         test_utils.c
         ../src/stellar_utils.c)
 
-target_link_libraries(test_printers PUBLIC cmocka)
+target_link_libraries(test_printers PUBLIC cmocka bsd)
 
 add_executable(test_parser
         test_parser.c
@@ -24,7 +24,7 @@ add_executable(test_parser
         ../src/stellar_parser.c
         ../src/stellar_utils.c)
 
-target_link_libraries(test_parser PUBLIC cmocka)
+target_link_libraries(test_parser PUBLIC cmocka bsd)
 
 add_test(test_printers test_printers)
 add_test(test_parser test_parser)