diff --git a/include/keepkey/firmware/signing.h b/include/keepkey/firmware/signing.h index 55f4c17d1..95b5be425 100644 --- a/include/keepkey/firmware/signing.h +++ b/include/keepkey/firmware/signing.h @@ -23,8 +23,16 @@ #include "trezor/crypto/bip32.h" #include "keepkey/transport/interface.h" +#include #include #include +/// Exposed for unit tests: pure predicate, no signing state involved. +bool isCrossAccountSegwitChangeForbidden(const uint32_t* lhs_address_n, + size_t lhs_address_n_count, + const uint32_t* rhs_address_n, + size_t rhs_address_n_count, + OutputScriptType rhs_script_type); + void signing_init(const SignTx* msg, const CoinType* _coin, const HDNode* _root); void signing_abort(void); diff --git a/lib/firmware/signing.c b/lib/firmware/signing.c index a7b367e0f..311596b90 100644 --- a/lib/firmware/signing.c +++ b/lib/firmware/signing.c @@ -427,10 +427,11 @@ void phase2_request_next_input(void) { /// Compares two BIP32 paths, returning true iff there is something mismatched /// about the mixed-mode change. -static bool isCrossAccountSegwitChangeForbidden( - const uint32_t* lhs_address_n, size_t lhs_address_n_count, - const uint32_t* rhs_address_n, size_t rhs_address_n_count, - OutputScriptType rhs_script_type) { +bool isCrossAccountSegwitChangeForbidden(const uint32_t* lhs_address_n, + size_t lhs_address_n_count, + const uint32_t* rhs_address_n, + size_t rhs_address_n_count, + OutputScriptType rhs_script_type) { (void)lhs_address_n; size_t count = rhs_address_n_count; @@ -456,6 +457,10 @@ static bool isCrossAccountSegwitChangeForbidden( rhs_script_type != OutputScriptType_PAYTOWITNESS) return true; + if (out_purpose == (0x80000000 | 86) && + rhs_script_type != OutputScriptType_PAYTOTAPROOT) + return true; + return false; } diff --git a/unittests/firmware/CMakeLists.txt b/unittests/firmware/CMakeLists.txt index 667911741..b115bde3c 100644 --- a/unittests/firmware/CMakeLists.txt +++ b/unittests/firmware/CMakeLists.txt @@ -1,6 +1,7 @@ set(sources app_confirm.cpp recovery.cpp + signing.cpp storage.cpp usb_rx.cpp u2f.cpp) diff --git a/unittests/firmware/signing.cpp b/unittests/firmware/signing.cpp new file mode 100644 index 000000000..f178ab876 --- /dev/null +++ b/unittests/firmware/signing.cpp @@ -0,0 +1,48 @@ +extern "C" { +#include "keepkey/firmware/signing.h" +} + +#include "gtest/gtest.h" + +namespace { + +constexpr uint32_t H(uint32_t i) { return 0x80000000 | i; } + +// m/'/0'/0'/1/0 -- a first change address in the first account. +struct ChangePath { + uint32_t n[5]; + explicit ChangePath(uint32_t purpose) : n{H(purpose), H(0), H(0), 1, 0} {} +}; + +bool Forbidden(uint32_t in_purpose, uint32_t out_purpose, + OutputScriptType out_script_type) { + ChangePath in(in_purpose), out(out_purpose); + return isCrossAccountSegwitChangeForbidden(in.n, 5, out.n, 5, + out_script_type); +} + +} // namespace + +// Regression: a BIP86 change path paired with any non-taproot script type used +// to fall through to the generic path check, which accepted it as change. That +// suppressed the output confirmation screen while serializing the change to a +// script no BIP86 wallet ever scans for. +TEST(Signing, TaprootChangeMustUseTaprootScriptType) { + EXPECT_TRUE(Forbidden(86, 86, OutputScriptType_PAYTOADDRESS)); + EXPECT_TRUE(Forbidden(86, 86, OutputScriptType_PAYTOWITNESS)); + EXPECT_TRUE(Forbidden(86, 86, OutputScriptType_PAYTOP2SHWITNESS)); +} + +TEST(Signing, MatchedPurposeAndScriptTypeAreAllowed) { + EXPECT_FALSE(Forbidden(86, 86, OutputScriptType_PAYTOTAPROOT)); + EXPECT_FALSE(Forbidden(44, 44, OutputScriptType_PAYTOADDRESS)); + EXPECT_FALSE(Forbidden(49, 49, OutputScriptType_PAYTOP2SHWITNESS)); + EXPECT_FALSE(Forbidden(84, 84, OutputScriptType_PAYTOWITNESS)); +} + +// The pre-taproot direction of the same rule, kept honest by this test. +TEST(Signing, LegacyChangeMayNotClaimTaprootScriptType) { + EXPECT_TRUE(Forbidden(44, 44, OutputScriptType_PAYTOTAPROOT)); + EXPECT_TRUE(Forbidden(49, 49, OutputScriptType_PAYTOTAPROOT)); + EXPECT_TRUE(Forbidden(84, 84, OutputScriptType_PAYTOTAPROOT)); +}