From 70857cdf35d56006eac5124694243e06f83e710b Mon Sep 17 00:00:00 2001 From: highlander Date: Mon, 3 Aug 2026 16:11:11 -0300 Subject: [PATCH 1/2] fix(signing): reject BIP86 change output with a non-taproot script type isCrossAccountSegwitChangeForbidden() enforced purpose/script-type agreement for BIP44, BIP49 and BIP84 change paths, but purpose 86' was never added when taproot support landed. A change output at m/86'/.../1/i declaring PAYTOADDRESS therefore skipped both mixed-mode guards and fell through to the generic path check in check_change_bip32_path(), which accepted it: same length, same account prefix, change chain, in-range index. The output was consequently treated as change, so its confirmation screen was suppressed, while transaction.c serialized it as P2PKH. A malicious host could route the entire change amount into a script that no BIP86 wallet scans for. The funds stay under the device's seed, but they are invisible to normal recovery until the path is derived by hand, and nothing was shown on the display. The reverse direction (44'/49'/84' claiming PAYTOTAPROOT) was already covered by the existing arms; this closes the one-sided hole. Rejecting here does not fail the signing session -- the output simply stops being treated as change and gets confirmed on screen like any other recipient. unittests/firmware/signing.cpp pins both directions. Verified failing without the new arm (make xunit -> Error 2) and passing with it. --- include/keepkey/firmware/signing.h | 8 +++++ lib/firmware/signing.c | 6 +++- unittests/firmware/CMakeLists.txt | 1 + unittests/firmware/signing.cpp | 48 ++++++++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 unittests/firmware/signing.cpp 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..baa0c7c90 100644 --- a/lib/firmware/signing.c +++ b/lib/firmware/signing.c @@ -427,7 +427,7 @@ 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( +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) { @@ -456,6 +456,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)); +} From c55b31971e0c7f2f6d2c43699a674b0d33ca5f93 Mon Sep 17 00:00:00 2001 From: highlander Date: Mon, 3 Aug 2026 17:34:29 -0300 Subject: [PATCH 2/2] style(signing): apply canonical formatter --- lib/firmware/signing.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/firmware/signing.c b/lib/firmware/signing.c index baa0c7c90..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. -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;