diff --git a/core/CMakeLists.txt b/core/CMakeLists.txt index f9088c4f..b7c52fe3 100644 --- a/core/CMakeLists.txt +++ b/core/CMakeLists.txt @@ -150,6 +150,7 @@ set(checks_SRC "src/checks/check_sign_tx.c" "src/checks/conv_checks.c" "src/checks/misc_checks.c" + "src/checks/rpc_checks.c" "src/checks/self_checks.c" "src/checks/validate_fees.c" "src/checks/verify_mix_entropy.c" diff --git a/core/include/checks.h b/core/include/checks.h index 0b58a90e..fbe11ea9 100644 --- a/core/include/checks.h +++ b/core/include/checks.h @@ -39,6 +39,15 @@ 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. + * + * Note that as this function uses statically-allocated buffers, it is not thread-safe. + */ +int verify_rpc_oversized_message_rejected(void); + #define ASSERT_STR_EQUAL(value, expecting, message) \ do { \ if (strcmp(expecting, value) != 0) { \ diff --git a/core/src/checks/rpc_checks.c b/core/src/checks/rpc_checks.c new file mode 100644 index 00000000..d147952b --- /dev/null +++ b/core/src/checks/rpc_checks.c @@ -0,0 +1,281 @@ +#include "checks.h" +#include "config.h" +#include "log.h" +#include "memzero.h" +#include "nanopb_stream.h" +#include "print.h" +#include "rand.h" +#include "rpc.h" + +#include +#include +#include +#include + +// Helper which returns the size of a buffer that would be needed to hold the serialized version of the given +// protobuf structure, assuming that pb_encode_delimited() serialization will be used. +static size_t get_serialized_proto_struct_size(const pb_field_t fields[], const void* const proto_struct) { + pb_ostream_t stream = PB_OSTREAM_SIZING; + if (!pb_encode_delimited(&stream, fields, proto_struct)) { + ERROR("%s: pb_encode_delimited() failed: %s", __func__, PB_GET_ERROR(&stream)); + return 0; + } + return stream.bytes_written; +} + +// Serializes the given protobuf structure to the given buffer of the given size, using pb_encode_delimited(). +// Returns true on success or false on failure. +// Caller brings their own buffer memory, this function does not allocate. +static bool serialize_proto_struct_to_buffer( + pb_byte_t* const buffer, + const size_t buffer_size, + const pb_field_t fields[], + const void* const proto_struct) { + pb_ostream_t ostream = pb_ostream_from_buffer(buffer, buffer_size); + if (!pb_encode_delimited(&ostream, fields, proto_struct)) { + ERROR("%s: pb_encode_delimited() failed: %s", __func__, PB_GET_ERROR(&ostream)); + return false; + } + return true; +} + +// Deserializes the given buffer into the given protobuf structure, using pb_decode_delimited(). +// Returns true on success or false on failure. +// Caller brings their own protobuf structure, this function does not allocate. +static bool deserialize_proto_struct_from_buffer( + const pb_byte_t* const buffer, + const size_t buffer_size, + const pb_field_t fields[], + void* const proto_struct) { + pb_istream_t istream = pb_istream_from_buffer(buffer, buffer_size); + if (!pb_decode_delimited(&istream, fields, proto_struct)) { + ERROR("%s: pb_decode_delimited() failed: %s", __func__, PB_GET_ERROR(&istream)); + return false; + } + return true; +} + +/** + * Helper for checking assumptions in the gnarly protobuf mangling code in verify_rpc_oversized_message_rejected(). + */ +static bool check_byte_equals(const char* parent_func, const pb_byte_t* const buf, size_t idx, pb_byte_t expected_val) { + const pb_byte_t actual_val = buf[idx]; + if (actual_val != expected_val) { + ERROR( + "%s: buf[%zu] contains an unexpected value: %hhu, expected: %hhu", parent_func, idx, actual_val, expected_val); + return false; + } + return true; +} + +static pb_byte_t request_buffer[256] = { 0 }; +static pb_byte_t response_buffer[256] = { 0 }; + +int verify_rpc_oversized_message_rejected(void) { + int result = 0; + + // Construct an initial InternalCommandRequest which holds an InitWallet command + // with a maximum-allowed-length random_bytes field. + 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); + + // Compute the size of the serialized struct. + size_t serialized_size = get_serialized_proto_struct_size(InternalCommandRequest_fields, &cmd); + if (serialized_size == 0) { + ERROR("%s: error computing serialized request size", __func__); + result = -1; + goto out; + } + + if (serialized_size + 1 > sizeof(request_buffer)) { + ERROR( + "%s: sizeof(request_buffer) == %zu but needs to be at least %zu. Modify the code and rebuild.", + __func__, + sizeof(request_buffer), + serialized_size + 1); + result = -1; + goto out; + } + + // Serialize the request struct into request_buffer. + if (!serialize_proto_struct_to_buffer(request_buffer, sizeof(request_buffer), InternalCommandRequest_fields, &cmd)) { + ERROR("%s: serialize_proto_struct_to_buffer() failed", __func__); + result = -1; + goto out; + } + + // 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 8. + // 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 16. + // serialized_request[5] - varint-encoded value for field 2. Leave this alone, it's the dummy + // 'wallet' field which we set to 1 above. Should equal 1. + // serialized_request[6] - field id (5 << 3) + tag (2, for 'LEN') for field 5 (command.InitWallet). + // Should equal 42. + // serialized_request[7] - varint-encoded LEN of the InitWalletRequest submessage. + // Should equal 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 10. + // serialized_request[9] - varint-encoded LEN of field 1 (random_bytes) of sub-message. + // Should equal 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: + if (!check_byte_equals(__func__, request_buffer, 0, (pb_byte_t) 73)) { + result = -1; + goto out; + } + if (!check_byte_equals(__func__, request_buffer, 0, (pb_byte_t) (serialized_size - 1))) { + result = -1; + goto out; + } + if (!check_byte_equals(__func__, request_buffer, 1, (pb_byte_t) ((1 << 3) + 0))) { + result = -1; + goto out; + } + // 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. + if (!check_byte_equals(__func__, request_buffer, 2, (pb_byte_t) ((cmd.version & 0x7f) | 0x80))) { + result = -1; + goto out; + } + // ... Second byte contains the next 1-7 bits + highest bit unset to indicate that there's no more data. + if (!check_byte_equals(__func__, request_buffer, 3, (pb_byte_t) (cmd.version >> 7))) { + result = -1; + goto out; + } + if (!check_byte_equals(__func__, request_buffer, 4, (pb_byte_t) ((2 << 3) + 0))) { + result = -1; + goto out; + } + if (!check_byte_equals(__func__, request_buffer, 5, (pb_byte_t) cmd.wallet_id)) { + result = -1; + goto out; + } + if (!check_byte_equals(__func__, request_buffer, 6, (pb_byte_t) ((5 << 3) + 2))) { + result = -1; + goto out; + } + if (!check_byte_equals(__func__, request_buffer, 7, (pb_byte_t) (MASTER_SEED_SIZE + 2))) { + result = -1; + goto out; + } + if (!check_byte_equals(__func__, request_buffer, 8, (pb_byte_t) ((1 << 3) + 2))) { + result = -1; + goto out; + } + if (!check_byte_equals(__func__, request_buffer, 9, (pb_byte_t) MASTER_SEED_SIZE)) { + result = -1; + goto out; + } + + request_buffer[0]++; // increment leading LEN byte + request_buffer[7]++; // increment LEN byte for top-level field 5 + request_buffer[9]++; // increment LEN byte for nested field 1 + request_buffer[serialized_size] = 0xaa; // set the last byte to an arbitrary value + serialized_size++; // increment serialized_size since we added a byte of data + + // Create a stream which will read from the corrupted serialized buffer. + pb_istream_t istream = pb_istream_from_buffer(request_buffer, serialized_size); + // Create a stream which will write to the response buffer. + pb_ostream_t ostream = pb_ostream_from_buffer(response_buffer, sizeof(response_buffer)); + + // 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. + ERROR("(next line is expected to show red text...)"); + + handle_incoming_message(&istream, &ostream); // <---- this is the actual function under test + + // Extract the response structure from the serialized_response buffer. It should be an error. + const size_t response_size = ostream.bytes_written; + if (response_size == 0) { + ERROR("%s: no response received from handle_incoming_message(): %s", __func__, PB_GET_ERROR(&ostream)); + result = -1; + goto out; + } + + // note: no need to initialize the response, static bool deserialize_proto_struct_from_buffer() does it via + // pb_decode_delimited(). + InternalCommandResponse response; + if (!deserialize_proto_struct_from_buffer( + response_buffer, response_size, InternalCommandResponse_fields, &response)) { + ERROR("%s: deserialize_proto_struct_from_buffer() failed", __func__); + result = -1; + goto out; + } + + // Check that the response contains an error. + 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; + } + + // Check that the error response contains the expected error code. + 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; + } + + // Check that the error response contains some message. + if (!response.response.Error.has_message) { + ERROR("%s: error response does not contain a 'message' field", __func__); + result = -1; + goto out; + } + + // Check that the error response contains the expected message. + 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: + memzero(request_buffer, sizeof(request_buffer)); + memzero(response_buffer, sizeof(response_buffer)); + + if (result == 0) { + INFO("%s: ok", __func__); + } + return result; +} diff --git a/core/src/checks/self_checks.c b/core/src/checks/self_checks.c index 113b7795..97c0b8a3 100644 --- a/core/src/checks/self_checks.c +++ b/core/src/checks/self_checks.c @@ -74,6 +74,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) {