Skip to content

Commit 6a41ce1

Browse files
ivmaykovivmaykov
committed
Add a self-check test for processing a message which exceeds nanopb field size limits
1 parent 9879eea commit 6a41ce1

4 files changed

Lines changed: 216 additions & 0 deletions

File tree

‎core/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ set(checks_SRC
8181
"src/checks/bip32.c"
8282
"src/checks/check_sign_tx.c"
8383
"src/checks/conv_checks.c"
84+
"src/checks/rpc_checks.c"
8485
"src/checks/self_checks.c"
8586
"src/checks/validate_fees.c"
8687
"src/checks/verify_mix_entropy.c"

‎core/include/checks.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,13 @@ int verify_no_rollback(void);
3333
int verify_check_qrsignature_pub(void);
3434
int verify_conv_btc_to_satoshi(void);
3535

36+
/**
37+
* Verifies that calling handle_incoming_message() with a serialized protobuf
38+
* message which exceeds nanopb field size limits (defined in proto .options
39+
* files) fails as expected.
40+
*/
41+
int verify_rpc_oversized_message_rejected(void);
42+
3643
#define ASSERT_STR_EQUAL(value, expecting, message) \
3744
do { \
3845
if (strcmp(expecting, value) != 0) { \

‎core/src/checks/rpc_checks.c‎

Lines changed: 202 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,202 @@
1+
#include "checks.h"
2+
#include "config.h"
3+
#include "log.h"
4+
#include "nanopb_stream.h"
5+
#include "print.h"
6+
#include "rand.h"
7+
#include "rpc.h"
8+
9+
#include <assert.h>
10+
#include <pb_decode.h>
11+
#include <pb_encode.h>
12+
#include <squareup/subzero/internal.pb.h>
13+
14+
static size_t get_serialized_message_size(const InternalCommandRequest* const cmd) {
15+
pb_ostream_t stream = PB_OSTREAM_SIZING;
16+
if (!pb_encode_delimited(&stream, InternalCommandRequest_fields, cmd)) {
17+
ERROR("%s: pb_encode_delimited() failed: %s", __func__, PB_GET_ERROR(&stream));
18+
return 0;
19+
}
20+
return stream.bytes_written;
21+
}
22+
23+
int verify_rpc_oversized_message_rejected(void) {
24+
int result = 0;
25+
uint8_t* serialized_request = NULL;
26+
uint8_t* serialized_response = NULL;
27+
28+
InternalCommandRequest cmd = InternalCommandRequest_init_default;
29+
cmd.version = VERSION;
30+
cmd.wallet_id = 1; // dummy value
31+
cmd.which_command = InternalCommandRequest_InitWallet_tag;
32+
static_assert(
33+
sizeof(cmd.command.InitWallet.random_bytes.bytes) == MASTER_SEED_SIZE,
34+
"MASTER_SEED_SIZE must equal sizeof(cmd.command.InitWallet.random_bytes.bytes)");
35+
cmd.command.InitWallet.random_bytes.size = MASTER_SEED_SIZE;
36+
random_buffer(cmd.command.InitWallet.random_bytes.bytes, MASTER_SEED_SIZE);
37+
38+
size_t serialized_size = get_serialized_message_size(&cmd);
39+
if (serialized_size == 0) {
40+
result = -1;
41+
goto out;
42+
}
43+
44+
// Note that we allocate 1 extra byte because we'll be extending the message.
45+
serialized_request = (uint8_t*) calloc(1, serialized_size + 1);
46+
if (NULL == serialized_request) {
47+
ERROR("%s: calloc(1, %zu) failed", __func__, serialized_size + 1);
48+
result = -1;
49+
goto out;
50+
}
51+
52+
pb_ostream_t ostream = pb_ostream_from_buffer(serialized_request, serialized_size);
53+
if (!pb_encode_delimited(&ostream, InternalCommandRequest_fields, &cmd)) {
54+
ERROR("%s: pb_encode_delimited() failed: %s", __func__, PB_GET_ERROR(&ostream));
55+
result = -1;
56+
goto out;
57+
}
58+
59+
// Helper macro used to check our assumptions in the gnarly protobuf mangling code below
60+
#define ASSERT_BYTE_EQUALS(buf, idx, expected_val) \
61+
do { \
62+
const uint8_t* buf_ = (buf); \
63+
const size_t idx_ = (idx); \
64+
const uint8_t expected_val_ = (expected_val); \
65+
const uint8_t actual_val_ = buf_[idx_]; \
66+
if (actual_val_ != expected_val_) { \
67+
ERROR( \
68+
"%s: buf[%zu] contains unexpected value: %hhu, expected: %hhu", \
69+
__func__, \
70+
idx_, \
71+
actual_val_, \
72+
expected_val_); \
73+
result = -1; \
74+
goto out; \
75+
} \
76+
} while (0)
77+
78+
// Corrupt the message by making the random_bytes field 1 byte longer than the max allowed size.
79+
// Note that this is a bit fragile and could break if the protobuf definitions inside
80+
// internal.proto are changed. But if that happens, hopefully this test breaks immediately
81+
// and can be fixed. Understanding of low-level protobuf serialization is recommended, see
82+
// https://protobuf.dev/programming-guides/encoding/ for the details (it's not that bad).
83+
// Basically:
84+
// serialized_request[0] - varint-encoded leading LEN byte. This is not normally there for binary
85+
// encoded protobufs, but it's added by nanopb because we are using
86+
// pb_encode_delimited(). If the message is longer than 127 bytes, this
87+
// length will actually take more than 1 byte, shifting everything after
88+
// it by a byte.
89+
// *** NOTE: WE NEED TO INCREMENT THIS BY 1. ***
90+
// serialized_request[1] - field id (1 << 3) + tag (0) for field 1 (version). Should equal 0x08.
91+
// serialized_request[2..3] - varint-encoded value for field 1. Leave this alone, it's the
92+
// contents of the 'version' field (210 at the time of writing). If
93+
// version ever exceeds 16383, this will start taking up an extra byte
94+
// and shift everything after it by a byte.
95+
// serialized_request[4] - field id (2 << 3) + tag (0) for field 2 (wallet_id). Should equal 0x10.
96+
// serialized_request[5] - varint-encoded value for field 2. Leave this allone, it's the dummy
97+
// 'wallet' field which we set to 1 above. Should equal 0x01.
98+
// serialized_request[6] - field id (5 << 3) + tag (2, for 'LEN') for field 5 (command.InitWallet).
99+
// Should equal 0x2a.
100+
// serialized_request[7] - varint-encoded LEN of the InitWalletRequest submessage.
101+
// Should equal 0x42 (decimal 66).
102+
// *** NOTE: WE NEED TO INCREMENT THIS BY 1. ***
103+
// serialized_request[8] - field id (1 << 3) + tag (2, for 'LEN') for field 1 of sub-message.
104+
// Should equal 0x0a.
105+
// serialized_request[9] - varint-encoded LEN of field 1 (random_bytes) of sub-message.
106+
// Should equal 0x40 (decimal 64).
107+
// *** NOTE: WE NEED TO INCREMENT THIS BY 1. ***
108+
// serialized_request[10..73] - the contents of the random_bytes field. Should be 64 bytes in length.
109+
// serialized_request[74] - doesn't exist in the original message. We add an extra data byte here.
110+
// It can be any value, we arbitrarily choose 0xaa.
111+
//
112+
// Let's check the above assumptions to make sure they are correct before proceeding:
113+
ASSERT_BYTE_EQUALS(serialized_request, 0, (uint8_t) 73);
114+
ASSERT_BYTE_EQUALS(serialized_request, 0, (uint8_t) (serialized_size - 1));
115+
ASSERT_BYTE_EQUALS(serialized_request, 1, (uint8_t) ((1 << 3) + 0));
116+
// The 'cmd.version' field is varint-encoded into 2 little-endian bytes:
117+
// ... First byte contains least-significant 7 bits + highest bit set to indicate that there's more data.
118+
ASSERT_BYTE_EQUALS(serialized_request, 2, (uint8_t) ((cmd.version & 0x7f) | 0x80));
119+
// ... Second byte contains the next 1-7 bits + highest bit unset to indicate that there's no more data.
120+
ASSERT_BYTE_EQUALS(serialized_request, 3, (uint8_t) (cmd.version >> 7));
121+
ASSERT_BYTE_EQUALS(serialized_request, 4, (uint8_t) ((2 << 3) + 0));
122+
ASSERT_BYTE_EQUALS(serialized_request, 5, (uint8_t) cmd.wallet_id);
123+
ASSERT_BYTE_EQUALS(serialized_request, 6, (uint8_t) ((5 << 3) + 2));
124+
ASSERT_BYTE_EQUALS(serialized_request, 7, (uint8_t) (MASTER_SEED_SIZE + 2));
125+
ASSERT_BYTE_EQUALS(serialized_request, 8, (uint8_t) ((1 << 3) + 2));
126+
ASSERT_BYTE_EQUALS(serialized_request, 9, (uint8_t) MASTER_SEED_SIZE);
127+
#undef ASSERT_BYTE_EQUALS
128+
129+
serialized_request[0]++; // increment leading LEN byte
130+
serialized_request[7]++; // increment LEN byte for top-level field 5
131+
serialized_request[9]++; // increment LEN byte for nested field 1
132+
serialized_request[serialized_size] = 0xaa; // set the last byte to an arbitrary value
133+
134+
pb_istream_t istream = pb_istream_from_buffer(serialized_request, serialized_size + 1);
135+
const size_t response_buffer_size = 2048; // 2048 bytes should be more than enough
136+
serialized_response = (uint8_t*) calloc(1, response_buffer_size);
137+
if (NULL == serialized_response) {
138+
ERROR("%s: calloc(1, %zu) failed", __func__, response_buffer_size);
139+
result = -1;
140+
goto out;
141+
}
142+
pb_ostream_t ostream2 = pb_ostream_from_buffer(serialized_response, response_buffer_size);
143+
ERROR("(next line is expected to show red text...)");
144+
145+
// Now that we have a serialized buffer, try to pass it to handle_incoming_message().
146+
// This should fail because the InitWallet.random_bytes field has a length of 65 bytes,
147+
// but nanopb options set the limit for this field at 64 bytes.
148+
//
149+
// NOTE: when building for nCipher, there are command hooks that would reject the command
150+
// because it's missing the tickets for key use authorization. But this doesn't matter for
151+
// this test case, because the protobuf parsing happens before that and fails first.
152+
handle_incoming_message(&istream, &ostream2);
153+
const size_t actual_response_size = ostream2.bytes_written;
154+
if (actual_response_size == 0) {
155+
ERROR("%s: no response received from handle_incoming_message(): %s", __func__, PB_GET_ERROR(&ostream2));
156+
result = -1;
157+
goto out;
158+
}
159+
pb_istream_t istream2 = pb_istream_from_buffer(serialized_response, actual_response_size);
160+
InternalCommandResponse response; // note: no need to init, pb_decode_delimited() does it
161+
if (!pb_decode_delimited(&istream2, InternalCommandResponse_fields, &response)) {
162+
ERROR(
163+
"%s: pb_decode_delimited(..., InternalCommandResponse_fields, ...) failed: %s",
164+
__func__,
165+
PB_GET_ERROR(&istream2));
166+
result = -1;
167+
goto out;
168+
}
169+
if (response.which_response != InternalCommandResponse_Error_tag) {
170+
ERROR(
171+
"%s: wrong response tag: %d, expected: %d",
172+
__func__,
173+
(int) response.which_response,
174+
(int) InternalCommandResponse_Error_tag);
175+
result = -1;
176+
goto out;
177+
}
178+
if (response.response.Error.code != Result_COMMAND_DECODE_FAILED) {
179+
ERROR(
180+
"%s: wrong response error code: %d, expected: %d",
181+
__func__,
182+
(int) response.response.Error.code,
183+
(int) Result_COMMAND_DECODE_FAILED);
184+
result = -1;
185+
goto out;
186+
}
187+
if (!response.response.Error.has_message) {
188+
ERROR("%s: error response does not contain a 'message' field", __func__);
189+
result = -1;
190+
goto out;
191+
}
192+
if (0 != strcmp("Decode Input failed: bytes overflow", response.response.Error.message)) {
193+
ERROR("%s: error response contains unexpected message: %s", __func__, response.response.Error.message);
194+
result = -1;
195+
goto out;
196+
}
197+
198+
out:
199+
free(serialized_request);
200+
free(serialized_response);
201+
return result;
202+
}

‎core/src/checks/self_checks.c‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,12 @@ int run_self_checks(void) {
6868
ERROR("self check failure: verify_conv_btc_to_satoshi failed.");
6969
}
7070

71+
t = verify_rpc_oversized_message_rejected();
72+
if (t != 0) {
73+
r = -1;
74+
ERROR("self check failure: verify_rpc_oversized_message_rejected failed.");
75+
}
76+
7177
// environment specific additional checks + cleanup
7278
t = post_run_self_checks();
7379
if (t != 0) {

0 commit comments

Comments
 (0)