btc: reject raw key hashes in policies
What the developer wrote
Authored by benma's agent
btc: reject raw key hashes in policies
Taproot parsing and sanity_check accept raw public-key-hash leaves,
which are not included in key enumeration. Reject RawPkH in the shared
policy validator before registration or use, including nested fragments.
Correct the stale Taproot parsing comment and cover standalone leaves,
nested expressions, and multi-leaf trees with regression tests.
This also rejects previously registered policies containing this
unsupported fragment when they are used for address derivation or signing.
What changed, and why it matters
This commit fixes a bug in the BitBox02 hardware wallet's Bitcoin policy handling. Previously, the wallet accepted a specific type of unsupported script fragment called a 'raw public-key hash' (RawPkH) inside Taproot wallet policies. Because these fragments were not tracked as keys, they could silently alter how addresses were derived or how transactions were signed, potentially allowing a malicious or malformed policy to behave in unexpected ways. The fix rejects any policy containing RawPkH, including when it is nested inside other expressions, and also rejects already-registered policies that contain it when they are later used.
Treat this as a security-hardening fix with potential exploitability. Review whether any user-registered Taproot policies on shipped devices contain RawPkH fragments and consider migration or rejection at firmware load time. Ensure the updated validate_miniscript() path is also applied to any other descriptor types that may parse leaves independently.
Security signals we found
Unsupported Miniscript fragment accepted by parser
Key enumeration mismatch: RawPkH leaf not counted as a key
Taproot leaf sanity check bypassed during parsing
Policy validation gap affects both registration and later use
Regression tests added for nested and multi-leaf cases
Evidence from the diff
The patch extends the shared Miniscript policy validator in policies.rs to reject miniscript::Terminal::RawPkH alongside the existing hash-fragment rejections. It also updates the Taproot parsing path to explicitly validate every leaf with validate_miniscript(), correcting a stale comment that incorrectly assumed Miniscript::from_str() already enforced the full sanity check. Regression tests are added for standalone RawPkH leaves, nested RawPkH expressions, and multi-leaf Taproot trees. The rejection now applies both at registration time and at use time (address derivation/signing), which catches previously registered policies containing the fragment.
Changed components
src/rust/bitbox02-rust/src/hww/api/bitcoin/policies.rsBitcoin/Taproot wallet policy parserMiniscript policy validatorAddress derivation and signing paths that re-validate registered policiesInspect captured patch +29 / −4
### src/rust/bitbox02-rust/src/hww/api/bitcoin/policies.rs
@@ -40,14 +40,16 @@ where
if miniscript.ext.tree_height >= MAX_MINISCRIPT_ENCODE_DEPTH {
return Err(Error::InvalidInput);
}
- // Hash fragments are not supported. Reject them before a policy can be registered or used.
+ // Hash fragments and raw public-key hashes are not supported. Reject them before a policy can
+ // be registered or used.
if miniscript.iter().any(|fragment| {
matches!(
fragment.node,
miniscript::Terminal::Sha256(_)
| miniscript::Terminal::Hash256(_)
| miniscript::Terminal::Hash160(_)
| miniscript::Terminal::Ripemd160(_)
+ | miniscript::Terminal::RawPkH(_)
)
}) {
return Err(Error::InvalidInput);
@@ -761,9 +763,8 @@ pub async fn parse<'a>(
}
// Match tr(...).
[b't', b'r', b'(', .., b')'] => {
- // During parsing, the leaf scripts are created using `Miniscript::from_str()`, which
- // calls the equivalent of the sanity check. We call it anyway below in case the
- // miniscript library extends/changes the main sanity_check function.
+ // Taproot parsing does not apply all Miniscript sanity checks. Validate every leaf
+ // and check the descriptor explicitly.
let tr = miniscript::descriptor::Tr::from_str(desc).map_err(|_| Error::InvalidInput)?;
for leaf in tr.leaves() {
validate_miniscript(leaf.miniscript())?;
@@ -1087,6 +1088,30 @@ mod tests {
}
}
+ #[async_test::test]
+ async fn test_parse_rejects_raw_pkh() {
+ mock_unlocked();
+ let our_key = make_our_key(KEYPATH_ACCOUNT).await;
+ let raw_pkh = format!("c:expr_raw_pkh({})", "11".repeat(20));
+ for descriptor in [
+ format!("tr(@0/**,{raw_pkh})"),
+ format!("tr(@0/**,and_v(v:pk(@0/<2;3>/*),{raw_pkh}))"),
+ format!("tr(@0/**,{{pk(@0/<2;3>/*),{raw_pkh}}})"),
+ ] {
+ let policy = make_policy(&descriptor, core::slice::from_ref(&our_key));
+ assert_eq!(
+ parse(
+ &mut crate::hal::testing::TestingHal::new(),
+ &policy,
+ BtcCoin::Tbtc,
+ )
+ .await
+ .unwrap_err(),
+ Error::InvalidInput,
+ );
+ }
+ }
+
#[test]
fn test_wallet_policy_pk_translator_rejects_hashes() {
use miniscript::Translator;Why this scored 59/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.