Add a self-check test for processing a message which exceeds nanopb f…

…ield size limits

core/CMakeLists.txt

@@ -81,6 +81,7 @@ set(checks_SRC
     "src/checks/bip32.c"
     "src/checks/check_sign_tx.c"
     "src/checks/conv_checks.c"
+    "src/checks/rpc_checks.c"
     "src/checks/self_checks.c"
     "src/checks/validate_fees.c"
     "src/checks/verify_mix_entropy.c"

core/include/checks.h

@@ -33,6 +33,13 @@ int verify_no_rollback(void);
 int verify_check_qrsignature_pub(void);
 int verify_conv_btc_to_satoshi(void);
 
+/**
+ * Verifies that calling handle_incoming_message() with a serialized protobuf
+ * message which exceeds nanopb field size limits (defined in proto .options
+ * files) fails as expected.
+ */
+int verify_rpc_oversized_message_rejected(void);
+
 #define ASSERT_STR_EQUAL(value, expecting, message) \
   do {                                              \
     if (strcmp(expecting, value) != 0) {            \

core/src/checks/rpc_checks.c

@@ -0,0 +1,202 @@
+#include "checks.h"
+#include "config.h"
+#include "log.h"
+#include "nanopb_stream.h"
+#include "print.h"
+#include "rand.h"
+#include "rpc.h"
+
+#include <assert.h>
+#include <pb_decode.h>
+#include <pb_encode.h>
+#include <squareup/subzero/internal.pb.h>
+
+static size_t get_serialized_message_size(const InternalCommandRequest* const cmd) {
+  pb_ostream_t stream = PB_OSTREAM_SIZING;
+  if (!pb_encode_delimited(&stream, InternalCommandRequest_fields, cmd)) {
+    ERROR("%s: pb_encode_delimited() failed: %s", __func__, PB_GET_ERROR(&stream));
+    return 0;
+  }
+  return stream.bytes_written;
+}
+
+int verify_rpc_oversized_message_rejected(void) {
+  int result = 0;
+  uint8_t* serialized_request = NULL;
+  uint8_t* serialized_response = NULL;
+
+  InternalCommandRequest cmd = InternalCommandRequest_init_default;
+  cmd.version = VERSION;
+  cmd.wallet_id = 1; // dummy value
+  cmd.which_command = InternalCommandRequest_InitWallet_tag;
+  static_assert(
+      sizeof(cmd.command.InitWallet.random_bytes.bytes) == MASTER_SEED_SIZE,
+      "MASTER_SEED_SIZE must equal sizeof(cmd.command.InitWallet.random_bytes.bytes)");
+  cmd.command.InitWallet.random_bytes.size = MASTER_SEED_SIZE;
+  random_buffer(cmd.command.InitWallet.random_bytes.bytes, MASTER_SEED_SIZE);
+
+  size_t serialized_size = get_serialized_message_size(&cmd);
+  if (serialized_size == 0) {
+    result = -1;
+    goto out;
+  }
+
+  // Note that we allocate 1 extra byte because we'll be extending the message.
+  serialized_request = (uint8_t*) calloc(1, serialized_size + 1);
+  if (NULL == serialized_request) {
+    ERROR("%s: calloc(1, %zu) failed", __func__, serialized_size + 1);
+    result = -1;
+    goto out;
+  }
+
+  pb_ostream_t ostream = pb_ostream_from_buffer(serialized_request, serialized_size);
+  if (!pb_encode_delimited(&ostream, InternalCommandRequest_fields, &cmd)) {
+    ERROR("%s: pb_encode_delimited() failed: %s", __func__, PB_GET_ERROR(&ostream));
+    result = -1;
+    goto out;
+  }
+
+  // Helper macro used to check our assumptions in the gnarly protobuf mangling code below
+#define ASSERT_BYTE_EQUALS(buf, idx, expected_val)                                       \
+  do {                                                                                   \
+    const uint8_t* buf_ = (buf);                                                         \
+    const size_t idx_ = (idx);                                                           \
+    const uint8_t expected_val_ = (expected_val);                                        \
+    const uint8_t actual_val_ = buf_[idx_];                                              \
+    if (actual_val_ != expected_val_) {                                                  \
+      ERROR(                                                                             \
+          "%s: unexpected byte %hhu in serialized_request at index %zu, expected: %hhu", \
+          __func__,                                                                      \
+          actual_val_,                                                                   \
+          idx_,                                                                          \
+          expected_val_);                                                                \
+      result = -1;                                                                       \
+      goto out;                                                                          \
+    }                                                                                    \
+  } while (0)
+
+  // Corrupt the message by making the random_bytes field 1 byte longer than the max allowed size.
+  // Note that this is a bit fragile and could break if the protobuf definitions inside
+  // internal.proto are changed. But if that happens, hopefully this test breaks immediately
+  // and can be fixed. Understanding of low-level protobuf serialization is recommended, see
+  // https://protobuf.dev/programming-guides/encoding/ for the details (it's not that bad).
+  // Basically:
+  //   serialized_request[0] - varint-encoded leading LEN byte. This is not normally there for binary
+  //                           encoded protobufs, but it's added by nanopb because we are using
+  //                           pb_encode_delimited(). If the message is longer than 127 bytes, this
+  //                           length will actually take more than 1 byte, shifting everything after
+  //                           it by a byte.
+  //                           *** NOTE: WE NEED TO INCREMENT THIS BY 1. ***
+  //   serialized_request[1] - field id (1 << 3) + tag (0) for field 1 (version). Should equal 0x08.
+  //   serialized_request[2..3] - varint-encoded value for field 1. Leave this alone, it's the
+  //                              contents of the 'version' field (210 at the time of writing). If
+  //                              version ever exceeds 16383, this will start taking up an extra byte
+  //                              and shift everything after it by a byte.
+  //   serialized_request[4] - field id (2 << 3) + tag (0) for field 2 (wallet_id). Should equal 0x10.
+  //   serialized_request[5] - varint-encoded value for field 2. Leave this allone, it's the dummy
+  //                           'wallet' field which we set to 1 above. Should equal 0x01.
+  //   serialized_request[6] - field id (5 << 3) + tag (2, for 'LEN') for field 5 (command.InitWallet).
+  //                           Should equal 0x2a.
+  //   serialized_request[7] - varint-encoded LEN of the InitWalletRequest submessage.
+  //                           Should equal 0x42 (decimal 66).
+  //                           *** NOTE: WE NEED TO INCREMENT THIS BY 1. ***
+  //   serialized_request[8] - field id (1 << 3) + tag (2, for 'LEN') for field 1 of sub-message.
+  //                           Should equal 0x0a.
+  //   serialized_request[9] - varint-encoded LEN of field 1 (random_bytes) of sub-message.
+  //                           Should equal 0x40 (decimal 64).
+  //                           *** NOTE: WE NEED TO INCREMENT THIS BY 1. ***
+  //   serialized_request[10..73] - the contents of the random_bytes field. Should be 64 bytes in length.
+  //   serialized_request[74] - doesn't exist in the original message. We add an extra data byte here.
+  //                            It can be any value, we arbitrarily choose 0xaa.
+  //
+  // Let's check the above assumptions to make sure they are correct before proceeding:
+  ASSERT_BYTE_EQUALS(serialized_request, 0, (uint8_t) 73);
+  ASSERT_BYTE_EQUALS(serialized_request, 0, (uint8_t) (serialized_size - 1));
+  ASSERT_BYTE_EQUALS(serialized_request, 1, (uint8_t) ((1 << 3) + 0));
+  // The 'cmd.version' field is varint-encoded into 2 little-endian bytes:
+  // ... First byte contains least-significant 7 bits + highest bit set to indicate that there's more data.
+  ASSERT_BYTE_EQUALS(serialized_request, 2, (uint8_t) ((cmd.version & 0x7f) | 0x80));
+  // ... Second byte contains the next 1-7 bits + highest bit unset to indicate that there's no more data.
+  ASSERT_BYTE_EQUALS(serialized_request, 3, (uint8_t) (cmd.version >> 7));
+  ASSERT_BYTE_EQUALS(serialized_request, 4, (uint8_t) ((2 << 3) + 0));
+  ASSERT_BYTE_EQUALS(serialized_request, 5, (uint8_t) cmd.wallet_id);
+  ASSERT_BYTE_EQUALS(serialized_request, 6, (uint8_t) ((5 << 3) + 2));
+  ASSERT_BYTE_EQUALS(serialized_request, 7, (uint8_t) (MASTER_SEED_SIZE + 2));
+  ASSERT_BYTE_EQUALS(serialized_request, 8, (uint8_t) ((1 << 3) + 2));
+  ASSERT_BYTE_EQUALS(serialized_request, 9, (uint8_t) MASTER_SEED_SIZE);
+#undef ASSERT_BYTE_EQUALS
+
+  serialized_request[0]++;                    // increment leading LEN byte
+  serialized_request[7]++;                    // increment LEN byte for top-level field 5
+  serialized_request[9]++;                    // increment LEN byte for nested field 1
+  serialized_request[serialized_size] = 0xaa; // set the last byte to an arbitrary value
+
+  pb_istream_t istream = pb_istream_from_buffer(serialized_request, serialized_size + 1);
+  const size_t response_buffer_size = 2048; // 2048 bytes should be more than enough
+  serialized_response = (uint8_t*) calloc(1, response_buffer_size);
+  if (NULL == serialized_response) {
+    ERROR("%s: calloc(1, %zu) failed", __func__, response_buffer_size);
+    result = -1;
+    goto out;
+  }
+  pb_ostream_t ostream2 = pb_ostream_from_buffer(serialized_response, response_buffer_size);
+  ERROR("(next line is expected to show red text...)");
+
+  // Now that we have a serialized buffer, try to pass it to handle_incoming_message().
+  // This should fail because the InitWallet.random_bytes field has a length of 65 bytes,
+  // but nanopb options set the limit for this field at 64 bytes.
+  //
+  // NOTE: when building for nCipher, there are command hooks that would reject the command
+  // because it's missing the tickets for key use authorization. But this doesn't matter for
+  // this test case, because the protobuf parsing happens before that and fails first.
+  handle_incoming_message(&istream, &ostream2);
+  const size_t actual_response_size = ostream2.bytes_written;
+  if (actual_response_size == 0) {
+    ERROR("%s: no response received from handle_incoming_message(): %s", __func__, PB_GET_ERROR(&ostream2));
+    result = -1;
+    goto out;
+  }
+  pb_istream_t istream2 = pb_istream_from_buffer(serialized_response, actual_response_size);
+  InternalCommandResponse response; // note: no need to init, pb_decode_delimited() does it
+  if (!pb_decode_delimited(&istream2, InternalCommandResponse_fields, &response)) {
+    ERROR(
+        "%s: pb_decode_delimited(..., InternalCommandResponse_fields, ...) failed: %s",
+        __func__,
+        PB_GET_ERROR(&istream2));
+    result = -1;
+    goto out;
+  }
+  if (response.which_response != InternalCommandResponse_Error_tag) {
+    ERROR(
+        "%s: wrong response tag: %d, expected: %d",
+        __func__,
+        (int) response.which_response,
+        (int) InternalCommandResponse_Error_tag);
+    result = -1;
+    goto out;
+  }
+  if (response.response.Error.code != Result_COMMAND_DECODE_FAILED) {
+    ERROR(
+        "%s: wrong response error code: %d, expected: %d",
+        __func__,
+        (int) response.response.Error.code,
+        (int) Result_COMMAND_DECODE_FAILED);
+    result = -1;
+    goto out;
+  }
+  if (!response.response.Error.has_message) {
+    ERROR("%s: error response does not contain a 'message' field", __func__);
+    result = -1;
+    goto out;
+  }
+  if (0 != strcmp("Decode Input failed: bytes overflow", response.response.Error.message)) {
+    ERROR("%s: error response contains unexpected message: %s", __func__, response.response.Error.message);
+    result = -1;
+    goto out;
+  }
+
+out:
+  free(serialized_request);
+  free(serialized_response);
+  return result;
+}

core/src/checks/self_checks.c

@@ -68,6 +68,12 @@ int run_self_checks(void) {
     ERROR("self check failure: verify_conv_btc_to_satoshi failed.");
   }
 
+  t = verify_rpc_oversized_message_rejected();
+  if (t != 0) {
+    r = -1;
+    ERROR("self check failure: verify_rpc_oversized_message_rejected failed.");
+  }
+
   // environment specific additional checks + cleanup
   t = post_run_self_checks();
   if (t != 0) {