Merge remote-tracking branch 'agent/benma-agent/reject-oversized-policy-keys'
What the developer wrote
Authored by Marko Bencun
Merge remote-tracking branch 'agent/benma-agent/reject-oversized-policy-keys'
What changed, and why it matters
This commit adds a length check before showing a Bitcoin policy key on the BitBox02 device screen. Previously, an extremely long key string could be displayed or processed without a size limit. The fix rejects keys whose on-screen text exceeds the maximum body size the confirmation UI can handle, preventing potential display truncation, UI confusion, or memory-related issues during policy registration.
No immediate user action required. Ensure firmware is updated to a version containing this commit. Developers should verify that other confirmation screens enforce similar size limits consistently.
Security signals we found
Input size limit added before UI confirmation
New unit test for boundary condition (MAX_CONFIRM_BODY_SIZE and MAX_CONFIRM_BODY_SIZE + 1)
Potential UI truncation or buffer issue mitigated for policy key display
Evidence from the diff
In src/rust/bitbox02-rust/src/hww/api/bitcoin/policies.rs, the ParsedPolicy::confirm() method now checks if the formatted key_str exceeds confirm::MAX_CONFIRM_BODY_SIZE before calling hal.ui().confirm(). If it does, it returns Error::InvalidInput. A unit test verifies that a key string exactly at the limit is accepted and one byte over is rejected. This is a defensive input-validation fix for oversized policy keys during wallet policy registration.
Changed components
BitBox02 firmwareBitcoin policy registration UI flowsrc/rust/bitbox02-rust/src/hww/api/bitcoin/policies.rsInspect captured patch +43 / −0
### src/rust/bitbox02-rust/src/hww/api/bitcoin/policies.rs
@@ -490,6 +490,9 @@ impl ParsedPolicy<'_> {
} else if Some(i) == taproot_unspendable_internal_key_index {
key_str = format!("Provably unspendable: {}", key_str)
}
+ if key_str.len() > confirm::MAX_CONFIRM_BODY_SIZE {
+ return Err(Error::InvalidInput);
+ }
hal.ui()
.confirm(&ConfirmParams {
title: &format!("Key {}/{}", i + 1, num_keys),
@@ -1362,6 +1365,46 @@ mod tests {
);
}
+ #[async_test::test]
+ async fn test_confirm_rejects_oversized_key() {
+ mock_unlocked();
+
+ let coin = BtcCoin::Tbtc;
+ let params = super::super::params::get(coin);
+ let our_key = make_our_key(KEYPATH_ACCOUNT).await;
+
+ for (last_path_element, expected_len, expected_ok) in [
+ (HARDENED, confirm::MAX_CONFIRM_BODY_SIZE, true),
+ (HARDENED + 10, confirm::MAX_CONFIRM_BODY_SIZE + 1, false),
+ ] {
+ let mut external_key = make_key(SOME_XPUB_1);
+ external_key.root_fingerprint = vec![0x12, 0x34, 0x56, 0x78];
+ external_key.keypath = vec![u32::MAX; 43];
+ external_key.keypath.push(last_path_element);
+ let key_str = format!(
+ "[{}/{}]{}",
+ hex::encode(&external_key.root_fingerprint),
+ util::bip32::to_string_no_prefix(&external_key.keypath),
+ bip32::Xpub::from(external_key.xpub.as_ref().unwrap())
+ .serialize_str(bip32::XPubType::Tpub)
+ .unwrap(),
+ );
+ assert_eq!(key_str.len(), expected_len);
+ let policy = make_policy(
+ "wsh(or_b(pk(@0/**),s:pk(@1/**)))",
+ &[external_key, our_key.clone()],
+ );
+ let mut hal = crate::hal::testing::TestingHal::new();
+ let parsed = parse(&mut hal, &policy, coin).await.unwrap();
+
+ let result = parsed
+ .confirm(&mut hal, "Register", params, "Test", Mode::Advanced)
+ .await;
+ assert_eq!(result.is_ok(), expected_ok);
+ assert_eq!(hal.ui.screens.len(), if expected_ok { 5 } else { 3 });
+ }
+ }
+
#[async_test::test]
async fn test_parse_check_dups_in_policy_tr() {
mock_unlocked();Why this scored 44/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.