fix(signing): reject BIP86 change output with a non-taproot script type

include/keepkey/firmware/signing.h

@@ -23,8 +23,16 @@
 #include "trezor/crypto/bip32.h"
 #include "keepkey/transport/interface.h"
 
+#include <stddef.h>
 #include <stdint.h>
 #include <stdbool.h>
+/// 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);

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;
 }
 

unittests/firmware/CMakeLists.txt

@@ -1,6 +1,7 @@
 set(sources
     app_confirm.cpp
     recovery.cpp
+    signing.cpp
     storage.cpp
     usb_rx.cpp
     u2f.cpp)

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/<purpose>'/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));
+}