diff --git a/legacy/firmware/signing.c b/legacy/firmware/signing.c index c2baec9c2..76af3fa0e 100644 --- a/legacy/firmware/signing.c +++ b/legacy/firmware/signing.c @@ -1034,6 +1034,15 @@ static bool signing_validate_input(const TxInputType *txinput) { } } + if (txinput->has_script_pubkey) { + // scriptPubKey should only be provided for external inputs, which are not + // supported in this firmware + fsm_sendFailure(FailureType_Failure_DataError, + _("Input's script_pubkey provided but not expected.")); + signing_abort(); + return false; + } + return true; } @@ -1212,7 +1221,17 @@ static void tx_info_finish(TxInfo *tx_info) { } } -static bool signing_check_input(const TxInputType *txinput) { +static bool signing_check_input(TxInputType *txinput) { + // hash all input data to check it later (relevant for fee computation) + tx_input_check_hash(&hasher_check, txinput); + + if (!fill_input_script_pubkey(coin, &root, txinput)) { + fsm_sendFailure(FailureType_Failure_ProcessError, + _("Failed to derive scriptPubKey")); + signing_abort(); + return false; + } + // Add input to BIP-143/BIP-341 computation. if (!tx_info_add_input(&info, txinput)) { return false; @@ -1230,8 +1249,6 @@ static bool signing_check_input(const TxInputType *txinput) { tx_serialize_input_hash(&ti, txinput); } #endif - // hash all input data to check it later (relevant for fee computation) - tx_input_check_hash(&hasher_check, txinput); return true; } @@ -1413,17 +1430,29 @@ static bool save_signature(TxInputType *txinput) { } static bool signing_check_orig_input(TxInputType *orig_input) { + if (!fill_input_script_pubkey(coin, &root, orig_input)) { + fsm_sendFailure(FailureType_Failure_ProcessError, + _("Failed to derive scriptPubKey")); + signing_abort(); + return false; + } + // Verify that the original input matches the current input. // An input is characterized by its prev_hash and prev_index. We also // check that the amounts match, so that we don't have to stream the // prevtx twice for the same prevtx output. Verifying that script_type - // matches is just a sanity check. + // matches is just a sanity check. When all inputs are taproot, we don't + // check the prevtxs, so we have to ensure that the claims about the + // script_pubkey values and amounts remain consistent throughout. if (orig_input->prev_hash.size != input.prev_hash.size || memcmp(orig_input->prev_hash.bytes, input.prev_hash.bytes, input.prev_hash.size) != 0 || orig_input->prev_index != input.prev_index || orig_input->amount != input.amount || - orig_input->script_type != input.script_type) { + orig_input->script_type != input.script_type || + orig_input->script_pubkey.size != input.script_pubkey.size || + memcmp(orig_input->script_pubkey.bytes, input.script_pubkey.bytes, + input.script_pubkey.size) != 0) { fsm_sendFailure(FailureType_Failure_ProcessError, _("Original input does not match current input.")); signing_abort(); diff --git a/legacy/firmware/transaction.c b/legacy/firmware/transaction.c index 51612c045..ecfb4f726 100644 --- a/legacy/firmware/transaction.c +++ b/legacy/firmware/transaction.c @@ -216,14 +216,88 @@ bool compute_address(const CoinInfo *coin, InputScriptType script_type, return 1; } +static int address_to_script_pubkey(const CoinInfo *coin, const char *address, + uint8_t *script_pubkey, pb_size_t *size) { + uint8_t addr_raw[MAX_ADDR_RAW_SIZE] = {0}; + size_t addr_raw_len = base58_decode_check(address, coin->curve->hasher_base58, + addr_raw, MAX_ADDR_RAW_SIZE); + + // P2PKH + size_t prefix_len = address_prefix_bytes_len(coin->address_type); + if (addr_raw_len == 20 + prefix_len && + address_check_prefix(addr_raw, coin->address_type)) { + script_pubkey[0] = 0x76; // OP_DUP + script_pubkey[1] = 0xA9; // OP_HASH_160 + script_pubkey[2] = 0x14; // pushing 20 bytes + memcpy(script_pubkey + 3, addr_raw + prefix_len, 20); + script_pubkey[23] = 0x88; // OP_EQUALVERIFY + script_pubkey[24] = 0xAC; // OP_CHECKSIG + *size = 25; + return 1; + } + + // P2SH + prefix_len = address_prefix_bytes_len(coin->address_type_p2sh); + if (addr_raw_len == 20 + prefix_len && + address_check_prefix(addr_raw, coin->address_type_p2sh)) { + script_pubkey[0] = 0xA9; // OP_HASH_160 + script_pubkey[1] = 0x14; // pushing 20 bytes + memcpy(script_pubkey + 2, addr_raw + prefix_len, 20); + script_pubkey[22] = 0x87; // OP_EQUAL + *size = 23; + return 1; + } + + if (coin->cashaddr_prefix && + cash_addr_decode(addr_raw, &addr_raw_len, coin->cashaddr_prefix, + address)) { + if (addr_raw_len == 21 && addr_raw[0] == (CASHADDR_P2KH | CASHADDR_160)) { + script_pubkey[0] = 0x76; // OP_DUP + script_pubkey[1] = 0xA9; // OP_HASH_160 + script_pubkey[2] = 0x14; // pushing 20 bytes + memcpy(script_pubkey + 3, addr_raw + 1, 20); + script_pubkey[23] = 0x88; // OP_EQUALVERIFY + script_pubkey[24] = 0xAC; // OP_CHECKSIG + *size = 25; + return 1; + } else if (addr_raw_len == 21 && + addr_raw[0] == (CASHADDR_P2SH | CASHADDR_160)) { + script_pubkey[0] = 0xA9; // OP_HASH_160 + script_pubkey[1] = 0x14; // pushing 20 bytes + memcpy(script_pubkey + 2, addr_raw + 1, 20); + script_pubkey[22] = 0x87; // OP_EQUAL + *size = 23; + return 1; + } else { + return 0; + } + } + + // SegWit + if (coin->bech32_prefix) { + int witver = 0; + if (!segwit_addr_decode(&witver, addr_raw, &addr_raw_len, + coin->bech32_prefix, address)) { + return 0; + } + // push 1 byte version id (opcode OP_0 = 0, OP_i = 80+i) + // push addr_raw (segwit_addr_decode makes sure addr_raw_len is at most 40) + script_pubkey[0] = witver == 0 ? 0 : 80 + witver; + script_pubkey[1] = addr_raw_len; + memcpy(script_pubkey + 2, addr_raw, addr_raw_len); + *size = addr_raw_len + 2; + return 1; + } + + return 0; +} + int compile_output(const CoinInfo *coin, AmountUnit amount_unit, const HDNode *root, TxOutputType *in, TxOutputBinType *out, bool needs_confirm) { memzero(out, sizeof(TxOutputBinType)); out->amount = in->amount; out->decred_script_version = DECRED_SCRIPT_VERSION; - uint8_t addr_raw[MAX_ADDR_RAW_SIZE] = {0}; - size_t addr_raw_len = 0; if (in->script_type == OutputScriptType_PAYTOOPRETURN) { // only 0 satoshi allowed for OP_RETURN @@ -295,66 +369,8 @@ int compile_output(const CoinInfo *coin, AmountUnit amount_unit, return 0; // failed to compile output } - addr_raw_len = base58_decode_check(in->address, coin->curve->hasher_base58, - addr_raw, MAX_ADDR_RAW_SIZE); - size_t prefix_len = 0; - // p2pkh - if (addr_raw_len == - 20 + (prefix_len = address_prefix_bytes_len(coin->address_type)) && - address_check_prefix(addr_raw, coin->address_type)) { - out->script_pubkey.bytes[0] = 0x76; // OP_DUP - out->script_pubkey.bytes[1] = 0xA9; // OP_HASH_160 - out->script_pubkey.bytes[2] = 0x14; // pushing 20 bytes - memcpy(out->script_pubkey.bytes + 3, addr_raw + prefix_len, 20); - out->script_pubkey.bytes[23] = 0x88; // OP_EQUALVERIFY - out->script_pubkey.bytes[24] = 0xAC; // OP_CHECKSIG - out->script_pubkey.size = 25; - } else - // p2sh - if (addr_raw_len == 20 + (prefix_len = address_prefix_bytes_len( - coin->address_type_p2sh)) && - address_check_prefix(addr_raw, coin->address_type_p2sh)) { - out->script_pubkey.bytes[0] = 0xA9; // OP_HASH_160 - out->script_pubkey.bytes[1] = 0x14; // pushing 20 bytes - memcpy(out->script_pubkey.bytes + 2, addr_raw + prefix_len, 20); - out->script_pubkey.bytes[22] = 0x87; // OP_EQUAL - out->script_pubkey.size = 23; - } else if (coin->cashaddr_prefix && - cash_addr_decode(addr_raw, &addr_raw_len, coin->cashaddr_prefix, - in->address)) { - if (addr_raw_len == 21 && addr_raw[0] == (CASHADDR_P2KH | CASHADDR_160)) { - out->script_pubkey.bytes[0] = 0x76; // OP_DUP - out->script_pubkey.bytes[1] = 0xA9; // OP_HASH_160 - out->script_pubkey.bytes[2] = 0x14; // pushing 20 bytes - memcpy(out->script_pubkey.bytes + 3, addr_raw + 1, 20); - out->script_pubkey.bytes[23] = 0x88; // OP_EQUALVERIFY - out->script_pubkey.bytes[24] = 0xAC; // OP_CHECKSIG - out->script_pubkey.size = 25; - - } else if (addr_raw_len == 21 && - addr_raw[0] == (CASHADDR_P2SH | CASHADDR_160)) { - out->script_pubkey.bytes[0] = 0xA9; // OP_HASH_160 - out->script_pubkey.bytes[1] = 0x14; // pushing 20 bytes - memcpy(out->script_pubkey.bytes + 2, addr_raw + 1, 20); - out->script_pubkey.bytes[22] = 0x87; // OP_EQUAL - out->script_pubkey.size = 23; - } else { - return 0; - } - } else if (coin->bech32_prefix) { - int witver = 0; - if (!segwit_addr_decode(&witver, addr_raw, &addr_raw_len, - coin->bech32_prefix, in->address)) { - return 0; - } - // segwit: - // push 1 byte version id (opcode OP_0 = 0, OP_i = 80+i) - // push addr_raw (segwit_addr_decode makes sure addr_raw_len is at most 40) - out->script_pubkey.bytes[0] = witver == 0 ? 0 : 80 + witver; - out->script_pubkey.bytes[1] = addr_raw_len; - memcpy(out->script_pubkey.bytes + 2, addr_raw, addr_raw_len); - out->script_pubkey.size = addr_raw_len + 2; - } else { + if (!address_to_script_pubkey(coin, in->address, out->script_pubkey.bytes, + &out->script_pubkey.size)) { return 0; } @@ -368,6 +384,24 @@ int compile_output(const CoinInfo *coin, AmountUnit amount_unit, return out->script_pubkey.size; } +int fill_input_script_pubkey(const CoinInfo *coin, const HDNode *root, + TxInputType *in) { + static CONFIDENTIAL HDNode node; + memcpy(&node, root, sizeof(HDNode)); + char address[MAX_ADDR_SIZE] = {0}; + bool res = true; + res = res && hdnode_private_ckd_cached(&node, in->address_n, + in->address_n_count, NULL); + res = res && (hdnode_fill_public_key(&node) == 0); + res = res && compute_address(coin, in->script_type, &node, in->has_multisig, + &in->multisig, address); + memzero(&node, sizeof(node)); + res = res && address_to_script_pubkey(coin, address, in->script_pubkey.bytes, + &in->script_pubkey.size); + in->has_script_pubkey = res; + return res; +} + uint32_t compile_script_sig(uint32_t address_type, const uint8_t *pubkeyhash, uint8_t *out) { if (coinByAddressType(address_type)) { // valid coin type @@ -506,6 +540,7 @@ void tx_input_check_hash(Hasher *hasher, const TxInputType *input) { hasher_Update(hasher, (const uint8_t *)&input->sequence, sizeof(input->sequence)); hasher_Update(hasher, (const uint8_t *)&input->amount, sizeof(input->amount)); + tx_script_hash(hasher, input->script_pubkey.size, input->script_pubkey.bytes); return; } diff --git a/legacy/firmware/transaction.h b/legacy/firmware/transaction.h index 1f9953607..8bc68bb9d 100644 --- a/legacy/firmware/transaction.h +++ b/legacy/firmware/transaction.h @@ -75,6 +75,8 @@ uint32_t serialize_script_multisig(const CoinInfo *coin, int compile_output(const CoinInfo *coin, AmountUnit amount_unit, const HDNode *root, TxOutputType *in, TxOutputBinType *out, bool needs_confirm); +int fill_input_script_pubkey(const CoinInfo *coin, const HDNode *root, + TxInputType *in); void tx_input_check_hash(Hasher *hasher, const TxInputType *input); uint32_t tx_prevout_hash(Hasher *hasher, const TxInputType *input); diff --git a/tests/device_tests/test_multisig.py b/tests/device_tests/test_multisig.py index d194bcefb..48d4c567e 100644 --- a/tests/device_tests/test_multisig.py +++ b/tests/device_tests/test_multisig.py @@ -224,9 +224,14 @@ class TestMultisig: script_type=proto.OutputScriptType.PAYTOADDRESS, ) - with pytest.raises(TrezorFailure, match="Pubkey not found in multisig script"): + with pytest.raises(TrezorFailure) as exc: btc.sign_tx(client, "Bitcoin", [inp1], [out1], prev_txes=TX_API) + if client.features.model == "1": + assert exc.value.message.endswith("Failed to derive scriptPubKey") + else: + assert exc.value.message.endswith("Pubkey not found in multisig script") + def test_attack_change_input(self, client): """ In Phases 1 and 2 the attacker replaces a non-multisig input