Skip to content

fix(compile): validate policy keys and hashes before compiling descriptors #347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Olorunshogo wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(compile): validate policy keys and hashes before compiling descriptors #347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 88 additions & 41 deletions src/handlers/descriptor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ use {
key::{Parity, rand},
secp256k1::{PublicKey, Scalar, Secp256k1, SecretKey},
},
miniscript::{Descriptor, descriptor::TapTree, policy::Concrete},
miniscript::{
Descriptor, FromStrKey, MiniscriptKey,
descriptor::{DescriptorPublicKey, TapTree},
policy::Concrete,
},
},
std::{str::FromStr, sync::Arc},
};
Expand Down Expand Up @@ -73,55 +77,98 @@ pub struct CompileCommand {
#[arg(env = "TYPE", short = 't', long = "type", default_value = "wsh", value_parser = ["sh","wsh", "sh-wsh", "tr"]
)]
script_type: String,
/// Skip key and hash validation, accepting placeholders (e.g. pk(A)). Useful for
/// inspecting a policy's shape without real keys. Descriptors compiled with this
/// flag are not usable for spending.
#[arg(long = "allow-placeholders")]
allow_placeholders: bool,
}

#[cfg(feature = "compiler")]
/// Converts an x-only key into `Self`, so the `tr` branch can build its internal key
/// generically over the policy's key type. `String` can't get a `From<XOnlyPublicKey>`
/// impl here (orphan rules), hence this local trait instead.
trait TrInternalKey: MiniscriptKey {
fn from_xonly(key: XOnlyPublicKey) -> Self;
}

#[cfg(feature = "compiler")]
impl TrInternalKey for String {
fn from_xonly(key: XOnlyPublicKey) -> Self {
key.to_string()
}
}

#[cfg(feature = "compiler")]
impl TrInternalKey for DescriptorPublicKey {
fn from_xonly(key: XOnlyPublicKey) -> Self {
DescriptorPublicKey::from(key)
}
}

#[cfg(feature = "compiler")]
/// Parses `policy_str` as `Pk` and compiles it for `script_type`. Returns the
/// descriptor string and, for `tr`, the random tweak `r` used to derive the
/// unspendable internal key so the caller can verify it.
fn compile_policy<Pk: FromStrKey + TrInternalKey>(
policy_str: &str,
script_type: &str,
) -> Result<(String, Option<String>), Error> {
let policy: Concrete<Pk> = Concrete::from_str(policy_str)
.map_err(|e| Error::Generic(format!("Invalid policy: {e}")))?;

let mut r = None;

// Compile per branch, not once up front: the contexts have different script
// limits, and the narrowest one would reject policies valid for the requested type.
let descriptor = match script_type {
"sh" => Descriptor::new_sh(policy.compile()?),
"wsh" => Descriptor::new_wsh(policy.compile()?),
"sh-wsh" => Descriptor::new_sh_wsh(policy.compile()?),
"tr" => {
// Use a randomized unspendable internal key (H + rG) instead of a fixed NUMS
// point. This improves privacy by preventing observers from determining whether
// key-path spending is disabled. `r` is returned so the user can verify the key
// is derived from the NUMS point. See BIP-341.
let secp = Secp256k1::new();
let r_secret = SecretKey::new(&mut rand::thread_rng());
r = Some(r_secret.display_secret().to_string());

let nums_key = XOnlyPublicKey::from_str(NUMS_UNSPENDABLE_KEY_HEX)
.map_err(|e| Error::Generic(format!("Invalid NUMS key: {e}")))?;
let nums_point = PublicKey::from_x_only_public_key(nums_key, Parity::Even);

let internal_key_point = nums_point
.add_exp_tweak(&secp, &Scalar::from(r_secret))
.map_err(|e| Error::Generic(format!("Failed to tweak NUMS key: {e}")))?;
let (xonly_internal_key, _) = internal_key_point.x_only_public_key();

let tree = TapTree::Leaf(Arc::new(policy.compile()?));
Descriptor::new_tr(Pk::from_xonly(xonly_internal_key), Some(tree))
}
_ => {
return Err(Error::Generic(
"Invalid script type. Supported: sh, wsh, sh-wsh, tr".into(),
));
}
}?;

Ok((descriptor.to_string(), r))
}

#[cfg(feature = "compiler")]
impl AppCommand<AppContext<Init>> for CompileCommand {
type Output = DescriptorResult;

fn execute(&self, _ctx: &mut AppContext<Init>) -> Result<Self::Output, Error> {
let policy: Concrete<String> = Concrete::from_str(&self.policy)
.map_err(|e| Error::Generic(format!("Invalid policy: {e}")))?;

let mut r = None;

// Compile per branch, not once up front: the contexts have different script
// limits, and the narrowest one would reject policies valid for the requested type.
let descriptor = match self.script_type.as_str() {
"sh" => Descriptor::new_sh(policy.compile()?),
"wsh" => Descriptor::new_wsh(policy.compile()?),
"sh-wsh" => Descriptor::new_sh_wsh(policy.compile()?),
"tr" => {
// Use a randomized unspendable internal key (H + rG) instead of a fixed NUMS
// point. This improves privacy by preventing observers from determining whether
// key-path spending is disabled. `r` is returned so the user can verify the key
// is derived from the NUMS point. See BIP-341.
let secp = Secp256k1::new();
let r_secret = SecretKey::new(&mut rand::thread_rng());
r = Some(r_secret.display_secret().to_string());

let nums_key = XOnlyPublicKey::from_str(NUMS_UNSPENDABLE_KEY_HEX)
.map_err(|e| Error::Generic(format!("Invalid NUMS key: {e}")))?;
let nums_point = PublicKey::from_x_only_public_key(nums_key, Parity::Even);

let internal_key_point =
nums_point
.add_exp_tweak(&secp, &Scalar::from(r_secret))
.map_err(|e| Error::Generic(format!("Failed to tweak NUMS key: {e}")))?;
let (xonly_internal_key, _) = internal_key_point.x_only_public_key();

let tree = TapTree::Leaf(Arc::new(policy.compile()?));
Descriptor::new_tr(xonly_internal_key.to_string(), Some(tree))
}
_ => {
return Err(Error::Generic(
"Invalid script type. Supported: sh, wsh, sh-wsh, tr".into(),
));
}
}?;
let (descriptor, r) = if self.allow_placeholders {
compile_policy::<String>(&self.policy, &self.script_type)?
} else {
compile_policy::<DescriptorPublicKey>(&self.policy, &self.script_type)?
};

Ok(DescriptorResult {
descriptor: Some(descriptor.to_string()),
descriptor: Some(descriptor),
mnemonic: None,
multipath_descriptor: None,
public_descriptors: None,
Expand Down
78 changes: 70 additions & 8 deletions tests/integration/init.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,10 @@ mod test_compile {

/// A policy can be valid for tr or wsh type and still exceed the limits of the
/// legacy context, whose 520-byte redeemScript cap does not apply to it.
///
/// Uses placeholder keys (K01, K02, ...) since only the policy's shape, not its
/// keys, matters for this test - `--allow-placeholders` keeps that shape-only
/// intent even though real key validation is on by default.
#[test]
fn test_compile_policy_beyond_legacy_limits() {
let temp_dir = TempDir::new().unwrap();
Expand All @@ -211,16 +215,22 @@ mod test_compile {
let policy = format!("thresh(2,{keys})");

// compile tr
cli.cmd("compile", &[&policy, "--type", "tr"])
.assert()
.success()
.stdout(predicate::str::contains("tr("));
cli.cmd(
"compile",
&[&policy, "--type", "tr", "--allow-placeholders"],
)
.assert()
.success()
.stdout(predicate::str::contains("tr("));

// compile wsh
cli.cmd("compile", &[&policy, "--type", "wsh"])
.assert()
.success()
.stdout(predicate::str::contains("wsh("));
cli.cmd(
"compile",
&[&policy, "--type", "wsh", "--allow-placeholders"],
)
.assert()
.success()
.stdout(predicate::str::contains("wsh("));
}

#[test]
Expand All @@ -233,6 +243,58 @@ mod test_compile {
.failure()
.stderr(predicate::str::contains("Invalid policy"));
}

#[test]
fn test_compile_rejects_placeholder_key_by_default() {
let temp_dir = TempDir::new().unwrap();
let cli = BdkCli::new("testnet", Some(temp_dir.path().to_path_buf()));

cli.cmd("compile", &["pk(ABC)", "--type", "wsh"])
.assert()
.failure()
.stderr(predicate::str::contains("Invalid policy"));
}

#[test]
fn test_compile_rejects_placeholder_hash_by_default() {
let temp_dir = TempDir::new().unwrap();
let cli = BdkCli::new("testnet", Some(temp_dir.path().to_path_buf()));

let policy =
"and(pk(02e5b88fdb71c696e1a473f309a47535b7190e21a22bd25e7fc8bd055db3bba12f),sha256(H))";

cli.cmd("compile", &[policy, "--type", "wsh"])
.assert()
.failure()
.stderr(predicate::str::contains("Invalid policy"));
}

#[test]
fn test_compile_allows_placeholder_key_with_flag() {
let temp_dir = TempDir::new().unwrap();
let cli = BdkCli::new("testnet", Some(temp_dir.path().to_path_buf()));

cli.cmd(
"compile",
&["pk(ABC)", "--type", "wsh", "--allow-placeholders"],
)
.assert()
.success()
.stdout(predicate::str::contains("wsh(pk(ABC))"));
}

#[test]
fn test_compile_accepts_xpub_key() {
let temp_dir = TempDir::new().unwrap();
let cli = BdkCli::new("testnet", Some(temp_dir.path().to_path_buf()));

let policy = "pk(xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8/0/*)";

cli.cmd("compile", &[policy, "--type", "wsh"])
.assert()
.success()
.stdout(predicate::str::contains("wsh("));
}
}

// --- CONFIG COMMAND TESTS ---
Expand Down