Skip to content

Commit 4d26f34

Browse files
abensoabenso
committed
fix APDU dispatch and metadata parsing edge cases
1 parent ba7e00d commit 4d26f34

13 files changed

Lines changed: 163 additions & 55 deletions

‎app/src/addr.c‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,10 +55,12 @@ zxerr_t addr_getItem(int8_t displayIdx,
5555
pageString(outVal, outValLen, (char *)(G_io_apdu_buffer + PK_LEN_25519), pageIdx, pageCount);
5656
}
5757
return zxerr_ok;
58-
case 1:
58+
case 1: {
5959
snprintf(outKey, outKeyLen, "Public key");
60-
pageStringHex(outVal, outValLen, (char *)G_io_apdu_buffer, PK_LEN_25519, pageIdx, pageCount);
60+
const uint16_t pkLen = (scheme == secp256k1) ? SECP256K1_PK_LEN : PK_LEN_25519;
61+
pageStringHex(outVal, outValLen, (char *)G_io_apdu_buffer, pkLen, pageIdx, pageCount);
6162
return zxerr_ok;
63+
}
6264
case 2: {
6365
if (!app_mode_expert()) {
6466
return zxerr_no_data;

‎app/src/apdu_handler.c‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ static bool tx_initialized = false;
3838
uint16_t blobLen = 0;
3939
scheme_type_e scheme = ed25519;
4040

41+
bool review_pending = false;
42+
4143
static void extractHDPath(uint32_t rx, uint32_t offset) {
4244
tx_initialized = false;
4345

@@ -143,6 +145,9 @@ __Z_INLINE void handleGetAddr(volatile uint32_t *flags, volatile uint32_t *tx, u
143145

144146
// Get address type from P2
145147
scheme = G_io_apdu_buffer[OFFSET_P2];
148+
if (scheme != ed25519 && scheme != secp256k1) {
149+
THROW(APDU_CODE_INVALIDP1P2);
150+
}
146151

147152
if (scheme == ed25519) {
148153
// check if we have ss58prefix available
@@ -169,6 +174,7 @@ __Z_INLINE void handleGetAddr(volatile uint32_t *flags, volatile uint32_t *tx, u
169174
}
170175
if (requireConfirmation) {
171176
view_review_init(addr_getItem, addr_getNumItems, app_reply_address);
177+
set_review_pending(true);
172178
view_review_show(REVIEW_ADDRESS);
173179
*flags |= IO_ASYNCH_REPLY;
174180
return;
@@ -212,6 +218,7 @@ __Z_INLINE void handleSign(volatile uint32_t *flags, volatile uint32_t *tx, uint
212218
} else {
213219
view_review_init(tx_getItem, tx_getNumItems, app_sign_secp256k1);
214220
}
221+
set_review_pending(true);
215222
view_review_show(REVIEW_TXN);
216223
*flags |= IO_ASYNCH_REPLY;
217224
}
@@ -240,6 +247,7 @@ __Z_INLINE void handleSignRaw(volatile uint32_t *flags, volatile uint32_t *tx, u
240247
} else {
241248
view_review_init(tx_raw_getItem, tx_raw_getNumItems, app_sign_secp256k1);
242249
}
250+
set_review_pending(true);
243251
view_review_show(REVIEW_MSG);
244252
*flags |= IO_ASYNCH_REPLY;
245253
}
@@ -257,6 +265,10 @@ void handleApdu(volatile uint32_t *flags, volatile uint32_t *tx, uint32_t rx) {
257265
THROW(APDU_CODE_WRONG_LENGTH);
258266
}
259267

268+
if (is_review_pending()) {
269+
THROW(APDU_CODE_COMMAND_NOT_ALLOWED);
270+
}
271+
260272
switch (G_io_apdu_buffer[OFFSET_INS]) {
261273
case INS_GET_VERSION: {
262274
handle_getversion(flags, tx);
@@ -285,6 +297,7 @@ void handleApdu(volatile uint32_t *flags, volatile uint32_t *tx, uint32_t rx) {
285297
}
286298
}
287299
CATCH(EXCEPTION_IO_RESET) {
300+
set_review_pending(false);
288301
THROW(EXCEPTION_IO_RESET);
289302
}
290303
// NOLINTNEXTLINE(readability-identifier-length): `e` is descriptive

‎app/src/common/actions.h‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#pragma once
1717

1818
#include <os_io_seproxyhal.h>
19+
#include <stdbool.h>
1920
#include <stdint.h>
2021

2122
#include "addr.h"
@@ -28,11 +29,23 @@
2829
extern uint16_t action_addrResponseLen;
2930
extern uint16_t blobLen;
3031

32+
extern bool review_pending;
33+
34+
__Z_INLINE void set_review_pending(bool val) {
35+
review_pending = val;
36+
}
37+
38+
__Z_INLINE bool is_review_pending(void) {
39+
return review_pending;
40+
}
41+
3142
__Z_INLINE void app_sign_ed25519() {
3243
const uint8_t *message = tx_get_buffer();
3344

3445
zxerr_t err = crypto_sign_ed25519(G_io_apdu_buffer, IO_APDU_BUFFER_SIZE - 3, message, blobLen);
3546

47+
set_review_pending(false);
48+
3649
if (err != zxerr_ok) {
3750
set_code(G_io_apdu_buffer, 0, APDU_CODE_SIGN_VERIFY_ERROR);
3851
io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, 2);
@@ -47,6 +60,9 @@ __Z_INLINE void app_sign_secp256k1() {
4760

4861
uint16_t sigLen = 0;
4962
zxerr_t err = crypto_sign_secp256k1(G_io_apdu_buffer, IO_APDU_BUFFER_SIZE - 3, message, blobLen, &sigLen);
63+
64+
set_review_pending(false);
65+
5066
if (err != zxerr_ok) {
5167
set_code(G_io_apdu_buffer, 0, APDU_CODE_SIGN_VERIFY_ERROR);
5268
io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, 2);
@@ -57,6 +73,7 @@ __Z_INLINE void app_sign_secp256k1() {
5773
}
5874

5975
__Z_INLINE void app_reject() {
76+
set_review_pending(false);
6077
set_code(G_io_apdu_buffer, 0, APDU_CODE_COMMAND_NOT_ALLOWED);
6178
io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, 2);
6279
}
@@ -68,11 +85,13 @@ __Z_INLINE zxerr_t app_fill_address(const uint16_t ss58prefix, const scheme_type
6885
}
6986

7087
__Z_INLINE void app_reply_error() {
88+
set_review_pending(false);
7189
set_code(G_io_apdu_buffer, 0, APDU_CODE_DATA_INVALID);
7290
io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, 2);
7391
}
7492

7593
__Z_INLINE void app_reply_address() {
94+
set_review_pending(false);
7695
if (action_addrResponseLen == 0) {
7796
app_reply_error();
7897
return;

‎app/src/common/tx.c‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,11 @@ const char *tx_raw_parse() {
8484
return parser_getErrorDescription(parser_no_data);
8585
}
8686

87+
// Check the actual buffered payload
88+
if (dataLen != blobLen) {
89+
return parser_getErrorDescription(parser_unexpected_value);
90+
}
91+
8792
// we need to have, at least, prefix and postfix
8893
if (dataLen < prefixLen + postfixLen) {
8994
return parser_getErrorDescription(parser_unexpected_value);

‎app/src/crypto.c‎

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ zxerr_t crypto_sign_ed25519(uint8_t *signature, uint16_t signatureMaxlen, const
139139
catch_cx_error:
140140
MEMZERO(&cx_privateKey, sizeof(cx_privateKey));
141141
MEMZERO(privateKeyData, SK_LEN_25519);
142+
MEMZERO(messageDigest, BLAKE2B_DIGEST_SIZE);
142143

143144
if (error != zxerr_ok) {
144145
MEMZERO(signature, signatureMaxlen);
@@ -155,12 +156,12 @@ zxerr_t crypto_sign_secp256k1(
155156

156157
// Hash the message
157158
uint8_t messageDigest[CX_KECCAK_256_SIZE] = {0};
159+
uint8_t intermediate_digest[BLAKE2B_DIGEST_SIZE] = {0};
158160
zxerr_t error = zxerr_unknown;
159161

160162
// When the payload is bigger than MAX_SIGN_SIZE, it needs to be hashed with blake2b_256 before hashing with keccak_256.
161163
// https://github.com/paritytech/polkadot-sdk/blob/1a512570552119a49a8ecb2abfb7021954c4422d/substrate/primitives/runtime/src/generic/unchecked_extrinsic.rs#L567
162164
if (messageLen > MAX_SIGN_SIZE) {
163-
uint8_t intermediate_digest[BLAKE2B_DIGEST_SIZE];
164165
// Hash it with blake2b
165166
cx_blake2b_t ctx;
166167
CATCH_CXERROR(cx_blake2b_init_no_throw(&ctx, 256));
@@ -201,6 +202,8 @@ zxerr_t crypto_sign_secp256k1(
201202
catch_cx_error:
202203
MEMZERO(&cx_privateKey, sizeof(cx_privateKey));
203204
MEMZERO(privateKeyData, sizeof(privateKeyData));
205+
MEMZERO(messageDigest, sizeof(messageDigest));
206+
MEMZERO(intermediate_digest, sizeof(intermediate_digest));
204207

205208
if (error != zxerr_ok) {
206209
MEMZERO(output, outputLen);
@@ -251,24 +254,36 @@ static zxerr_t crypto_fillAddress_secp256k1(uint8_t *buffer,
251254
// Clear the buffer
252255
MEMZERO(buffer, bufferLen);
253256

254-
// Extract the uncompressed public key
255257
uint8_t uncompressedPubkey[SECP256K1_PK_LEN_UNCOMPRESSED] = {0};
256-
CHECK_ZXERR(crypto_extractPublicKey_secp256k1(uncompressedPubkey, sizeof(uncompressedPubkey), hdPath_to_use))
258+
uint8_t hashed1_pk[CX_KECCAK_256_SIZE] = {0};
259+
char *const addr = (char *)(buffer + SECP256K1_PK_LEN);
260+
zxerr_t err = zxerr_unknown;
261+
262+
// Extract the uncompressed public key
263+
if (crypto_extractPublicKey_secp256k1(uncompressedPubkey, sizeof(uncompressedPubkey), hdPath_to_use) != zxerr_ok) {
264+
goto fill_secp256k1_cleanup;
265+
}
257266

258267
// Compress the public key
259-
CHECK_ZXERR(compressPubkey(uncompressedPubkey, sizeof(uncompressedPubkey), buffer, bufferLen))
260-
char *addr = (char *)(buffer + SECP256K1_PK_LEN);
268+
if (compressPubkey(uncompressedPubkey, sizeof(uncompressedPubkey), buffer, bufferLen) != zxerr_ok) {
269+
goto fill_secp256k1_cleanup;
270+
}
261271

262272
// Encode the address - skip the first byte (0x04) that indicates the uncompressed format
263-
uint8_t hashed1_pk[CX_KECCAK_256_SIZE] = {0};
264-
CHECK_CX_OK(cx_keccak_256_hash(&uncompressedPubkey[1], sizeof(uncompressedPubkey) - 1, hashed1_pk));
273+
if (cx_keccak_256_hash(&uncompressedPubkey[1], sizeof(uncompressedPubkey) - 1, hashed1_pk) != CX_OK) {
274+
goto fill_secp256k1_cleanup;
275+
}
265276

266277
// Take the last 20 bytes of the Keccak hash as the address
267278
MEMCPY(addr, hashed1_pk + CX_KECCAK_256_SIZE - SECP256K1_ADDRESS_LEN, SECP256K1_ADDRESS_LEN);
268279

269280
*addrResponseLen = SECP256K1_PK_LEN + SECP256K1_ADDRESS_LEN;
281+
err = zxerr_ok;
270282

271-
return zxerr_ok;
283+
fill_secp256k1_cleanup:
284+
MEMZERO(uncompressedPubkey, sizeof(uncompressedPubkey));
285+
MEMZERO(hashed1_pk, sizeof(hashed1_pk));
286+
return err;
272287
}
273288

274289
// fill a crypto address using the global hdpath

‎app/src/metadata_parser.c‎

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ parser_error_t parseTypeRef(parser_context_t *blob,
132132
}
133133

134134
if (type->index != Void && type->index != ById) {
135-
printItem->itemCount++;
135+
CHECK_ERROR(addItemCount(printItem, 1));
136136
}
137137

138138
if (printItem->printing && printItem->target == printItem->itemCount) {
@@ -336,7 +336,7 @@ static parser_error_t parseEnumerationVariantType(parser_context_t *blob,
336336
}
337337
} else {
338338
if (!isOption && !isSome) {
339-
printItem->itemCount++;
339+
CHECK_ERROR(addItemCount(printItem, 1));
340340
if (printItem->printing && printItem->target == printItem->itemCount) {
341341
printItem->item.val = tmpEntry->enumeration.name;
342342
printItem->item.valEnc = EncString;
@@ -398,15 +398,15 @@ static parser_error_t parseSequenceType(parser_context_t *blob,
398398
CHECK_ERROR(readCompactU32(blob, &sequenceLen));
399399
TypeRef_t sequenceType = tmpEntry->sequence;
400400
if (sequenceLen == 0) {
401-
printItem->itemCount++;
401+
CHECK_ERROR(addItemCount(printItem, 1));
402402
if (printItem->printing && printItem->target == printItem->itemCount) {
403403
printItem->item.valEnc = EncEmptyVec;
404404
}
405405
return parser_ok;
406406
}
407407

408408
if (sequenceType.index == U8) {
409-
printItem->itemCount++;
409+
CHECK_ERROR(addItemCount(printItem, 1));
410410
if (printItem->printing && printItem->target == printItem->itemCount) {
411411
printItem->item.valEnc = EncHexString;
412412
printItem->item.val.ptr = blob->buffer + blob->offset;
@@ -454,7 +454,7 @@ static parser_error_t parseArrayType(parser_context_t *blob,
454454
const uint32_t arrLen = tmpEntry->array.len;
455455
const TypeRef_t arrType = tmpEntry->array.typeParam;
456456
if (arrType.index == U8) {
457-
printItem->itemCount++;
457+
CHECK_ERROR(addItemCount(printItem, 1));
458458
if (printItem->printing && printItem->target == printItem->itemCount) {
459459
printItem->item.valEnc = EncHexString;
460460
printItem->item.val.ptr = blob->buffer + blob->offset;
@@ -523,13 +523,8 @@ static parser_error_t parseTupleType(parser_context_t *blob,
523523
* @param printItem Pointer to the print item to update.
524524
* @return parser_error_t Error code indicating the result of the operation.
525525
*/
526-
parser_error_t parseMetadataEntry(
526+
static parser_error_t parseMetadataEntryInner(
527527
parser_context_t *blob, parser_context_t *metadata, uint32_t typeId, RegistryEntry_t *tmpEntry, PrintItem_t *printItem) {
528-
if (metadata == NULL || blob == NULL || tmpEntry == NULL || printItem == NULL) {
529-
return parser_no_data;
530-
}
531-
532-
CHECK_ERROR(checkStack());
533528
MEMZERO(tmpEntry, sizeof(*tmpEntry));
534529

535530
// Get the requested entry corresponding to typeId
@@ -553,11 +548,23 @@ parser_error_t parseMetadataEntry(
553548
break;
554549
case BitSequenceType:
555550
// TODO: implement BitSequence
556-
return parser_value_out_of_range;
557-
558551
default:
559552
return parser_value_out_of_range;
560553
}
561554

562-
return freeStack();
555+
return parser_ok;
556+
}
557+
558+
parser_error_t parseMetadataEntry(
559+
parser_context_t *blob, parser_context_t *metadata, uint32_t typeId, RegistryEntry_t *tmpEntry, PrintItem_t *printItem) {
560+
if (metadata == NULL || blob == NULL || tmpEntry == NULL || printItem == NULL) {
561+
return parser_no_data;
562+
}
563+
564+
// Acquire a recursion slot. On failure no slot was taken, so don't free.
565+
CHECK_ERROR(checkStack());
566+
567+
const parser_error_t err = parseMetadataEntryInner(blob, metadata, typeId, tmpEntry, printItem);
568+
(void)freeStack();
569+
return err;
563570
}

‎app/src/metadata_proof.c‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,13 @@ parser_error_t getMetadataDigest(Metadata_t *metadata, uint8_t metadataDigest[BL
243243
// get rootHash of registry
244244
CHECK_ERROR(getHash(ROOT_IDX, &proof));
245245

246+
// Require the proof to fully consume the supplied registry, indices and lemmas.
247+
if (proof.registry.registry.offset != proof.registry.registry.bufferLen ||
248+
proof.indices.indices.offset != proof.indices.indices.bufferLen ||
249+
proof.lemmas.lemmas.offset != proof.lemmas.lemmas.bufferLen) {
250+
return parser_unexpected_unparsed_bytes;
251+
}
252+
246253
// get hash of descriptor
247254
uint8_t extrinsicMetadataHash[BLAKE3_OUT_LEN] = {0};
248255
CHECK_ERROR(zxblake3_hash(metadata->shortMetadata.extrinsic.ctx.buffer, metadata->shortMetadata.extrinsic.ctx.bufferLen,

‎app/src/parser.c‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ parser_error_t parser_getItem(parser_tx_t *txObj, ui_field_t *uiFields) {
167167
CHECK_ERROR(readSignedExtension(signedExtensions, &tmpExtension));
168168

169169
// we have custom logic for Era
170-
if (strncmp((const char *)tmpExtension.identifier.ptr, STR_ERA, tmpExtension.identifier.len) == 0) {
170+
if (identifier_matches(&tmpExtension.identifier, STR_ERA)) {
171171
pd_ExtrinsicEra_t tmpEra = {0};
172172
CHECK_ERROR(_readEra(blobBuf, &tmpEra));
173173

@@ -191,17 +191,16 @@ parser_error_t parser_getItem(parser_tx_t *txObj, ui_field_t *uiFields) {
191191
}
192192
}
193193

194-
printItem.itemCount += tmpEra.isMortal ? 3 : 1;
194+
CHECK_ERROR(addItemCount(&printItem, tmpEra.isMortal ? 3 : 1));
195195
continue;
196196
}
197197

198198
CHECK_ERROR(parseTypeRef(blobBuf, metadataBuf, &tmpEntry, &tmpExtension.includedInExtrinsic, &printItem));
199199

200200
if (printItem.itemCount == printItem.target && printItem.item.valEnc != EncNoEncoding) {
201-
if (strncmp((const char *)tmpExtension.identifier.ptr, STR_TIP, tmpExtension.identifier.len) == 0) {
201+
if (identifier_matches(&tmpExtension.identifier, STR_TIP)) {
202202
snprintf(uiFields->outKey, uiFields->outKeyLen, "Tip");
203-
} else if (strncmp((const char *)tmpExtension.identifier.ptr, STR_TIP_WITH_ASSETID,
204-
tmpExtension.identifier.len) == 0) {
203+
} else if (identifier_matches(&tmpExtension.identifier, STR_TIP_WITH_ASSETID)) {
205204
snprintf(uiFields->outKey, uiFields->outKeyLen, "Tip");
206205

207206
if (printItem.item.valEnc == EncBalance) {
@@ -231,7 +230,7 @@ parser_error_t parser_getItem(parser_tx_t *txObj, ui_field_t *uiFields) {
231230

232231
if (printItem.itemCount == printItem.target && printItem.item.valEnc != EncNoEncoding) {
233232
// we have custom title in case of era and tip
234-
if (strncmp((const char *)tmpExtension.identifier.ptr, STR_ERA, tmpExtension.identifier.len) == 0) {
233+
if (identifier_matches(&tmpExtension.identifier, STR_ERA)) {
235234
snprintf(uiFields->outKey, uiFields->outKeyLen, "Mortality Block");
236235
} else {
237236
snprintf(uiFields->outKey, uiFields->outKeyLen, "%.*s", (int)tmpExtension.identifier.len,
@@ -247,8 +246,8 @@ parser_error_t parser_getItem(parser_tx_t *txObj, ui_field_t *uiFields) {
247246
printItem.printing = false;
248247
CHECK_ERROR(readSignedExtension(signedExtensions, &tmpExtension));
249248

250-
if (strncmp((const char *)tmpExtension.identifier.ptr, STR_TIP, tmpExtension.identifier.len) == 0 ||
251-
strncmp((const char *)tmpExtension.identifier.ptr, STR_TIP_WITH_ASSETID, tmpExtension.identifier.len) == 0) {
249+
if (identifier_matches(&tmpExtension.identifier, STR_TIP) ||
250+
identifier_matches(&tmpExtension.identifier, STR_TIP_WITH_ASSETID)) {
252251
printItem.printing = true;
253252
printItem.itemCount = 0;
254253
}
@@ -258,8 +257,7 @@ parser_error_t parser_getItem(parser_tx_t *txObj, ui_field_t *uiFields) {
258257
if (printItem.printing && printItem.itemCount == printItem.target && printItem.item.valEnc != EncNoEncoding) {
259258
snprintf(uiFields->outKey, uiFields->outKeyLen, "Tip");
260259

261-
if (strncmp((const char *)tmpExtension.identifier.ptr, STR_TIP_WITH_ASSETID, tmpExtension.identifier.len) ==
262-
0) {
260+
if (identifier_matches(&tmpExtension.identifier, STR_TIP_WITH_ASSETID)) {
263261
if (printItem.item.valEnc == EncBalance) {
264262
printItem.item.valEnc = EncUnsigned;
265263
} else if (printItem.item.valEnc == EncCompactBalance) {

0 commit comments

Comments
 (0)