diff --git a/legacy/firmware/signing.c b/legacy/firmware/signing.c index 76af3fa0e..661e27578 100644 --- a/legacy/firmware/signing.c +++ b/legacy/firmware/signing.c @@ -656,16 +656,13 @@ bool check_change_bip32_path(const TxInfo *tx_info, } static bool fill_input_script_sig(TxInputType *tinput) { - memcpy(&node, &root, sizeof(HDNode)); - if (hdnode_private_ckd_cached(&node, tinput->address_n, - tinput->address_n_count, NULL) == 0) { - // Failed to derive private key - return false; - } if (hdnode_fill_public_key(&node) != 0) { - // Failed to derive public key + fsm_sendFailure(FailureType_Failure_ProcessError, + _("Failed to derive public key.")); + signing_abort(); return false; } + if (tinput->has_multisig) { tinput->script_sig.size = compile_script_multisig(coin, &(tinput->multisig), tinput->script_sig.bytes); @@ -675,33 +672,24 @@ static bool fill_input_script_sig(TxInputType *tinput) { tinput->script_sig.size = compile_script_sig(coin->address_type, hash, tinput->script_sig.bytes); } - return tinput->script_sig.size > 0; -} -bool compile_input_script_sig(TxInputType *tinput) { - if (!info.multisig_fp_mismatch) { - // check that this is still multisig - uint8_t h[32] = {0}; - if (!tinput->has_multisig || - cryptoMultisigFingerprint(&(tinput->multisig), h) == 0 || - memcmp(info.multisig_fp, h, 32) != 0) { - // Transaction has changed during signing - return false; - } - } - if (info.in_address_n_count != BIP32_NOCHANGEALLOWED) { - // check that input address didn't change - size_t count = tinput->address_n_count; - if (count < 2 || count != info.in_address_n_count || - 0 != memcmp(info.in_address_n, tinput->address_n, - (count - 2) * sizeof(uint32_t))) { - return false; - } + if (tinput->script_sig.size == 0) { + fsm_sendFailure(FailureType_Failure_ProcessError, + _("Failed to compile input.")); + signing_abort(); + return false; } + + return true; +} + +static bool derive_node(TxInputType *tinput) { if (!coin_path_check(coin, tinput->script_type, tinput->address_n_count, tinput->address_n, tinput->has_multisig, CoinPathCheckLevel_BASIC)) { if (config_getSafetyCheckLevel() == SafetyCheckLevel_Strict) { + fsm_sendFailure(FailureType_Failure_DataError, _("Forbidden key path")); + signing_abort(); return false; } @@ -711,11 +699,21 @@ bool compile_input_script_sig(TxInputType *tinput) { if (!protectButton(ButtonRequestType_ButtonRequest_UnknownDerivationPath, false)) { fsm_sendFailure(FailureType_Failure_ActionCancelled, NULL); - layoutHome(); + signing_abort(); return false; } } - return fill_input_script_sig(tinput); + + memcpy(&node, &root, sizeof(HDNode)); + if (hdnode_private_ckd_cached(&node, tinput->address_n, + tinput->address_n_count, NULL) == 0) { + fsm_sendFailure(FailureType_Failure_ProcessError, + _("Failed to derive private key.")); + signing_abort(); + return false; + } + + return true; } static bool tx_info_init(TxInfo *tx_info, uint32_t inputs_count, @@ -1190,6 +1188,34 @@ static bool tx_info_add_input(TxInfo *tx_info, const TxInputType *txinput) { return true; } +static bool tx_info_check_input(TxInfo *tx_info, TxInputType *tinput) { + if (!tx_info->multisig_fp_mismatch) { + // check that this is still multisig + uint8_t h[32] = {0}; + if (!tinput->has_multisig || + cryptoMultisigFingerprint(&(tinput->multisig), h) == 0 || + memcmp(tx_info->multisig_fp, h, 32) != 0) { + fsm_sendFailure(FailureType_Failure_ProcessError, + _("Transaction has changed during signing")); + signing_abort(); + return false; + } + } + if (tx_info->in_address_n_count != BIP32_NOCHANGEALLOWED) { + // check that input address didn't change + size_t count = tinput->address_n_count; + if (count < 2 || count != tx_info->in_address_n_count || + 0 != memcmp(tx_info->in_address_n, tinput->address_n, + (count - 2) * sizeof(uint32_t))) { + fsm_sendFailure(FailureType_Failure_ProcessError, + _("Transaction has changed during signing")); + signing_abort(); + return false; + } + } + return true; +} + static bool tx_info_add_output(TxInfo *tx_info, const TxOutputBinType *tx_bin_output) { // Add output to BIP-143/BIP-341 hashOutputs. @@ -1221,7 +1247,7 @@ static void tx_info_finish(TxInfo *tx_info) { } } -static bool signing_check_input(TxInputType *txinput) { +static bool signing_add_input(TxInputType *txinput) { // hash all input data to check it later (relevant for fee computation) tx_input_check_hash(&hasher_check, txinput); @@ -1319,7 +1345,7 @@ static bool is_change_output(const TxInfo *tx_info, return check_change_bip32_path(tx_info, txoutput); } -static bool signing_check_output(TxOutputType *txoutput) { +static bool signing_add_output(TxOutputType *txoutput) { // Phase1: Check outputs // add it to hash_outputs // ask user for permission @@ -1429,7 +1455,7 @@ static bool save_signature(TxInputType *txinput) { return true; } -static bool signing_check_orig_input(TxInputType *orig_input) { +static bool signing_add_orig_input(TxInputType *orig_input) { if (!fill_input_script_pubkey(coin, &root, orig_input)) { fsm_sendFailure(FailureType_Failure_ProcessError, _("Failed to derive scriptPubKey")); @@ -1491,10 +1517,7 @@ static bool signing_check_orig_input(TxInputType *orig_input) { // (aka BIP-143 script code), which is what our code expects here in order // to properly compute the legacy transaction digest or BIP-143 transaction // digest. - if (!fill_input_script_sig(orig_input)) { - fsm_sendFailure(FailureType_Failure_ProcessError, - _("Failed to derive public key.")); - signing_abort(); + if (!derive_node(orig_input) || !fill_input_script_sig(orig_input)) { return false; } @@ -1516,7 +1539,7 @@ static bool signing_check_orig_input(TxInputType *orig_input) { return true; } -static bool signing_check_orig_output(TxOutputType *orig_output) { +static bool signing_add_orig_output(TxOutputType *orig_output) { // Compute scriptPubKey. TxOutputBinType orig_bin_output; if (compile_output(coin, amount_unit, &root, orig_output, &orig_bin_output, @@ -2091,10 +2114,9 @@ static bool signing_sign_segwit_input(TxInputType *txinput) { signing_abort(); return false; } - if (!compile_input_script_sig(txinput)) { - fsm_sendFailure(FailureType_Failure_ProcessError, - _("Failed to compile input")); - signing_abort(); + + if (!tx_info_check_input(&info, txinput) || !derive_node(txinput) || + !fill_input_script_sig(txinput)) { return false; } @@ -2194,7 +2216,7 @@ void signing_txack(TransactionType *tx) { switch (signing_stage) { case STAGE_REQUEST_1_INPUT: if (!signing_validate_input(&tx->inputs[0]) || - !signing_check_input(&tx->inputs[0])) { + !signing_add_input(&tx->inputs[0])) { return; } @@ -2304,7 +2326,7 @@ void signing_txack(TransactionType *tx) { return; case STAGE_REQUEST_1_ORIG_INPUT: if (!signing_validate_input(tx->inputs) || - !signing_check_orig_input(tx->inputs)) { + !signing_add_orig_input(tx->inputs)) { return; } @@ -2313,7 +2335,7 @@ void signing_txack(TransactionType *tx) { return; case STAGE_REQUEST_2_OUTPUT: if (!signing_validate_output(&tx->outputs[0]) || - !signing_check_output(&tx->outputs[0])) { + !signing_add_output(&tx->outputs[0])) { return; } tx_weight += tx_output_weight(coin, &tx->outputs[0]); @@ -2327,7 +2349,7 @@ void signing_txack(TransactionType *tx) { return; case STAGE_REQUEST_2_ORIG_OUTPUT: if (!signing_validate_output(tx->outputs) || - !signing_check_orig_output(tx->outputs)) { + !signing_add_orig_output(tx->outputs)) { return; } @@ -2571,10 +2593,9 @@ void signing_txack(TransactionType *tx) { // check inputs are the same as those in phase 1 tx_input_check_hash(&hasher_check, tx->inputs); if (idx2 == idx1) { - if (!compile_input_script_sig(&tx->inputs[0])) { - fsm_sendFailure(FailureType_Failure_ProcessError, - _("Failed to compile input")); - signing_abort(); + if (!tx_info_check_input(&info, &tx->inputs[0]) || + !derive_node(&tx->inputs[0]) || + !fill_input_script_sig(&tx->inputs[0])) { return; } memcpy(&input, &tx->inputs[0], sizeof(input)); @@ -2671,10 +2692,9 @@ void signing_txack(TransactionType *tx) { signing_abort(); return; } - if (!compile_input_script_sig(&tx->inputs[0])) { - fsm_sendFailure(FailureType_Failure_ProcessError, - _("Failed to compile input")); - signing_abort(); + if (!tx_info_check_input(&info, &tx->inputs[0]) || + !derive_node(&tx->inputs[0]) || + !fill_input_script_sig(&tx->inputs[0])) { return; } if (!tx->inputs[0].has_amount) { @@ -2711,10 +2731,9 @@ void signing_txack(TransactionType *tx) { } else if (tx->inputs[0].script_type == InputScriptType_SPENDP2SHWITNESS && !tx->inputs[0].has_multisig) { - if (!compile_input_script_sig(&tx->inputs[0])) { - fsm_sendFailure(FailureType_Failure_ProcessError, - _("Failed to compile input")); - signing_abort(); + if (!tx_info_check_input(&info, &tx->inputs[0]) || + !derive_node(&tx->inputs[0]) || + !fill_input_script_sig(&tx->inputs[0])) { return; } // fixup normal p2pkh script into witness 0 p2wpkh script for p2sh @@ -2827,10 +2846,9 @@ void signing_txack(TransactionType *tx) { coin->overwintered, info.version_group_id, info.timestamp); ti.version |= (DECRED_SERIALIZE_WITNESS_SIGNING << 16); ti.is_decred = true; - if (!compile_input_script_sig(&tx->inputs[0])) { - fsm_sendFailure(FailureType_Failure_ProcessError, - _("Failed to compile input")); - signing_abort(); + if (!tx_info_check_input(&info, &tx->inputs[0]) || + !derive_node(&tx->inputs[0]) || + !fill_input_script_sig(&tx->inputs[0])) { return; } diff --git a/tests/device_tests/test_msg_signtx.py b/tests/device_tests/test_msg_signtx.py index 561b7b191..ec6de1b16 100644 --- a/tests/device_tests/test_msg_signtx.py +++ b/tests/device_tests/test_msg_signtx.py @@ -965,12 +965,7 @@ class TestMsgSigntx: ) assert exc.value.code == messages.FailureType.ProcessError - if client.features.model == "1": - assert exc.value.message.endswith("Failed to compile input") - else: - assert exc.value.message.endswith( - "Transaction has changed during signing" - ) + assert exc.value.message.endswith("Transaction has changed during signing") def test_spend_coinbase(self, client): inp1 = messages.TxInputType( diff --git a/tests/device_tests/test_msg_signtx_invalid_path.py b/tests/device_tests/test_msg_signtx_invalid_path.py index beef99500..e393e74f5 100644 --- a/tests/device_tests/test_msg_signtx_invalid_path.py +++ b/tests/device_tests/test_msg_signtx_invalid_path.py @@ -61,12 +61,8 @@ class TestMsgSigntxInvalidPath: with pytest.raises(TrezorFailure) as exc: btc.sign_tx(client, "Litecoin", [inp1], [out1], prev_txes=TX_CACHE_MAINNET) - if client.features.model == "1": - assert exc.value.code == proto.FailureType.ProcessError - assert exc.value.message.endswith("Failed to compile input") - else: - assert exc.value.code == proto.FailureType.DataError - assert exc.value.message.endswith("Forbidden key path") + assert exc.value.code == proto.FailureType.DataError + assert exc.value.message.endswith("Forbidden key path") # Adapted from TestMsgSigntx.test_one_one_fee, # only changed the coin from Bitcoin to Litecoin and set safety checks to prompt. diff --git a/tests/device_tests/test_msg_signtx_segwit.py b/tests/device_tests/test_msg_signtx_segwit.py index e434f0d63..af143de8d 100644 --- a/tests/device_tests/test_msg_signtx_segwit.py +++ b/tests/device_tests/test_msg_signtx_segwit.py @@ -340,12 +340,7 @@ class TestMsgSigntxSegwit: with pytest.raises(TrezorFailure) as exc: btc.sign_tx(client, "Testnet", [inp1], [out1, out2], prev_txes=TX_API) assert exc.value.code == proto.FailureType.ProcessError - if client.features.model == "1": - assert exc.value.message.endswith("Failed to compile input") - else: - assert exc.value.message.endswith( - "Transaction has changed during signing" - ) + assert exc.value.message.endswith("Transaction has changed during signing") def test_attack_mixed_inputs(self, client): TRUE_AMOUNT = 123456789 diff --git a/tests/device_tests/test_multisig.py b/tests/device_tests/test_multisig.py index 48d4c567e..42acaad1e 100644 --- a/tests/device_tests/test_multisig.py +++ b/tests/device_tests/test_multisig.py @@ -333,9 +333,4 @@ class TestMultisig: # 01000000000101396e2c107427f9eaece56a37539983adb8efd52b067c3d4567805fc8f3f7bffb01000000171600147a876a07b366f79000b441335f2907f777a0280bffffffff02e8030000000000001976a914e7c1345fc8f87c68170b3aa798a956c2fe6a9eff88ac703a0f000000000017a914a1261837f1b40e84346b1504ffe294e402965f2687024830450221009ff835e861be4e36ca1f2b6224aee2f253dfb9f456b13e4b1724bb4aaff4c9c802205e10679c2ead85743119f468cba5661f68b7da84dd2d477a7215fef98516f1f9012102af12ddd0d55e4fa2fcd084148eaf5b0b641320d0431d63d1e9a90f3cbd0d540700000000 assert exc.value.code == proto.FailureType.ProcessError - if client.features.model == "1": - assert exc.value.message.endswith("Failed to compile input") - else: - assert exc.value.message.endswith( - "Transaction has changed during signing" - ) + assert exc.value.message.endswith("Transaction has changed during signing")