diff --git a/legacy/firmware/signing.c b/legacy/firmware/signing.c index 4db1918c7..7fb241726 100644 --- a/legacy/firmware/signing.c +++ b/legacy/firmware/signing.c @@ -66,6 +66,7 @@ enum { STAGE_REQUEST_DECRED_WITNESS, #endif } signing_stage; +static bool foreign_address_confirmed; // indicates that user approved warning static bool taproot_only; // indicates whether all internal inputs are Taproot static uint32_t idx1; // The index of the input or output in the current tx // which is being processed, signed or serialized. @@ -747,25 +748,24 @@ static bool fill_input_script_sig(TxInputType *tinput) { static bool derive_node(TxInputType *tinput) { if (!coin_path_check(coin, tinput->script_type, tinput->address_n_count, - tinput->address_n, tinput->has_multisig, false)) { - if (is_replacement) { - fsm_sendFailure( - FailureType_Failure_ProcessError, - _("Non-standard paths not allowed in replacement transactions.")); - signing_abort(); - return false; - } - - if (config_getSafetyCheckLevel() == SafetyCheckLevel_Strict) { - fsm_sendFailure(FailureType_Failure_DataError, _("Forbidden key path")); - signing_abort(); - return false; - } + tinput->address_n, tinput->has_multisig, false) && + config_getSafetyCheckLevel() == SafetyCheckLevel_Strict) { + fsm_sendFailure(FailureType_Failure_DataError, _("Forbidden key path")); + signing_abort(); + return false; + } - if (!fsm_layoutPathWarning()) { - signing_abort(); - return false; - } + // Sanity check not critical for security. The main reason for this is that we + // are not comfortable with using the same private key in multiple signature + // schemes (ECDSA and Schnorr) and we want to be sure that the user went + // through a warning screen before we sign the input. + if (!foreign_address_confirmed && + !coin_path_check(coin, tinput->script_type, tinput->address_n_count, + tinput->address_n, tinput->has_multisig, true)) { + fsm_sendFailure(FailureType_Failure_ProcessError, + _("Transaction has changed during signing")); + signing_abort(); + return false; } memcpy(&node, &root, sizeof(HDNode)); @@ -953,6 +953,7 @@ void signing_init(const SignTx *msg, const CoinInfo *_coin, tx_weight = 4 * size; + foreign_address_confirmed = false; taproot_only = true; signatures = 0; idx1 = 0; @@ -1377,10 +1378,22 @@ static bool signing_add_input(TxInputType *txinput) { return false; } - if (!fsm_checkCoinPath(coin, txinput->script_type, txinput->address_n_count, - txinput->address_n, txinput->has_multisig, true)) { - signing_abort(); - return false; + if (txinput->script_type != InputScriptType_EXTERNAL && + !coin_path_check(coin, txinput->script_type, txinput->address_n_count, + txinput->address_n, txinput->has_multisig, true)) { + if (config_getSafetyCheckLevel() == SafetyCheckLevel_Strict && + !coin_path_check(coin, txinput->script_type, txinput->address_n_count, + txinput->address_n, txinput->has_multisig, false)) { + fsm_sendFailure(FailureType_Failure_DataError, _("Forbidden key path")); + signing_abort(); + return false; + } + + if (!fsm_layoutPathWarning()) { + signing_abort(); + return false; + } + foreign_address_confirmed = true; } if (!fill_input_script_pubkey(coin, &root, txinput)) { diff --git a/tests/ui_tests/fixtures.json b/tests/ui_tests/fixtures.json index 726dcdeb7..e06b72b3e 100644 --- a/tests/ui_tests/fixtures.json +++ b/tests/ui_tests/fixtures.json @@ -137,7 +137,7 @@ "T1_bitcoin-test_komodo.py::test_one_one_rewards_claim": "d6ba8441ad5e4bbdb191eef24fd679637e468cf1e7317b290bf89d2588bea214", "T1_bitcoin-test_multisig.py::test_15_of_15": "ba0a7e6d6a6f37bf553130acc7aa2471d9d7d804f1564837f149b3b5ecec0461", "T1_bitcoin-test_multisig.py::test_2_of_3": "4ece08d5a19590a521e22aae038a492796922f26bb8ba70614211a6ade5bce18", -"T1_bitcoin-test_multisig.py::test_attack_change_input": "f4f75000bfc3af0006a767e9afbc7f3930bc6bad864ca0d31656735d4103bddc", +"T1_bitcoin-test_multisig.py::test_attack_change_input": "b5beb346018c89e34a219b5466e3f2f257865b8623c356e2b249742d89e04da3", "T1_bitcoin-test_multisig.py::test_missing_pubkey": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", "T1_bitcoin-test_multisig_change.py::test_external_external": "b31515c4102fe602ed1e951bf67db483ffb8874dacc4b0359e9ef67d16999178", "T1_bitcoin-test_multisig_change.py::test_external_internal": "ceef2445ab95ec10543e320224b13febef2428d240cb01c58dd7ce878b121997", @@ -255,7 +255,7 @@ "T1_bitcoin-test_signtx_invalid_path.py::test_invalid_path_fail": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", "T1_bitcoin-test_signtx_invalid_path.py::test_invalid_path_fail_asap": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", "T1_bitcoin-test_signtx_invalid_path.py::test_invalid_path_pass_forkid": "c83cf4f8324514dba0831e96fa2c20c30cef4121dcd594cf3232af19e4721a07", -"T1_bitcoin-test_signtx_invalid_path.py::test_invalid_path_prompt": "0603601df63d976ca01ee3428d3f18f908695f500bb0c81e33483edd164b60f7", +"T1_bitcoin-test_signtx_invalid_path.py::test_invalid_path_prompt": "cbf1b6f66151a120689757e6dcd7af9483ceb19e5aa1dff3e1fb5c6f4be8d292", "T1_bitcoin-test_signtx_mixed_inputs.py::test_non_segwit_segwit_inputs": "5b980d57e7d1707d8802bff0665e0460fe5c594c91d758b05ee59020b72e3941", "T1_bitcoin-test_signtx_mixed_inputs.py::test_non_segwit_segwit_non_segwit_inputs": "704181fd5aa2ff33a9e5a27874187585ef946f7cdf6e690a7316a33921f25602", "T1_bitcoin-test_signtx_mixed_inputs.py::test_segwit_non_segwit_inputs": "5b980d57e7d1707d8802bff0665e0460fe5c594c91d758b05ee59020b72e3941",