diff --git a/Cargo.lock b/Cargo.lock index 9ad1950c4..6f0342048 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -269,6 +269,12 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c4872d67bab6358e59559027aa3b9157c53d9358c51423c17554809a8858e0f8" +[[package]] +name = "cc" +version = "1.0.72" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "22a9137b95ea06864e018375b72adfb7db6e6f68cfc8df5a04d00288050485ee" + [[package]] name = "cfg-if" version = "1.0.0" @@ -803,6 +809,7 @@ dependencies = [ "nonempty", "orchard", "rand_core 0.6.3", + "secp256k1", "subtle", "thiserror", "tokio", @@ -1403,6 +1410,24 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d29ab0c6d3fc0ee92fe66e2d99f700eab17a8d57d1c1d3b748380fb20baa78cd" +[[package]] +name = "secp256k1" +version = "0.20.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "97d03ceae636d0fed5bae6a7f4f664354c5f4fcedf6eef053fef17e49f837d0a" +dependencies = [ + "secp256k1-sys", +] + +[[package]] +name = "secp256k1-sys" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "957da2573cde917463ece3570eab4a0b3f19de6f1646cde62e6fd3868f566036" +dependencies = [ + "cc", +] + [[package]] name = "serde" version = "1.0.131" diff --git a/Cargo.toml b/Cargo.toml index 36d98c961..e067901e9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -38,6 +38,7 @@ jubjub = "0.8" memuse = "0.2" nonempty = "0.7" orchard = "=0.1.0-beta.1" +secp256k1 = "0.20" subtle = "2.2" rand_core = "0.6" tracing = "0.1" diff --git a/src/Makefile.am b/src/Makefile.am index bca93e6ce..5662bd5bf 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -39,7 +39,14 @@ if ENABLE_WALLET LIBBITCOIN_WALLET=libbitcoin_wallet.a endif -RUST_ENV_VARS = RUSTC="$(RUSTC)" TERM=dumb +# We depend on the secp256k1 crate for some logic on the Rust side of the FFI. This crate +# is a wrapper around libsecp256k1, which we already vendor in our code; the crate vendors +# its own copy with non-colliding symbols. To ensure that we only use a single version of +# libsecp256k1, we disable symbol renaming in the secp256k1-sys crate so it links to the +# same library as the C++ code. +# - Note that this does not prevent the secp256k1-sys vendored code from being built; this +# requires https://github.com/rust-bitcoin/rust-secp256k1/issues/380 to be addressed. +RUST_ENV_VARS = RUSTC="$(RUSTC)" TERM=dumb RUSTFLAGS="--cfg=rust_secp_no_symbol_renaming" RUST_BUILD_OPTS = --lib --release --target $(RUST_TARGET) rust_verbose = $(rust_verbose_@AM_V@) @@ -507,6 +514,7 @@ zcash_cli_LDADD = \ $(LIBZCASH) \ $(LIBRUSTZCASH) \ $(LIBBITCOIN_CRYPTO) \ + $(LIBSECP256K1) \ $(LIBZCASH_LIBS) # diff --git a/src/rust/src/address_ffi.rs b/src/rust/src/address_ffi.rs index e56e38923..5b0b7ba26 100644 --- a/src/rust/src/address_ffi.rs +++ b/src/rust/src/address_ffi.rs @@ -76,15 +76,16 @@ impl UnifiedAddressHelper { .into_iter() .map(|receiver| match receiver { unified::Receiver::Orchard(data) => { - // ZIP 316: Senders MUST reject Unified Addresses in which any - // constituent address does not meet the validation requirements of - // its Receiver Encoding. - // TODO: Add this API to the orchard crate. - // if let Err(e) = orchard::Address::from_bytes(data) { - // tracing::error!("{}", e); - // false - // } else { + // ZIP 316: Consumers MUST reject Unified Addresses/Viewing Keys in + // which any constituent Item does not meet the validation + // requirements of its encoding. + if orchard::Address::from_raw_address_bytes(&data) + .is_none() + .into() { + tracing::error!("Unified Address contains invalid Orchard receiver"); + false + } else { unsafe { // TODO: Replace with Orchard support. (unknown_cb.unwrap())(ua_obj, 0x03, data.as_ptr(), data.len()) @@ -92,9 +93,9 @@ impl UnifiedAddressHelper { } } unified::Receiver::Sapling(data) => { - // ZIP 316: Senders MUST reject Unified Addresses in which any - // constituent address does not meet the validation requirements of - // its Receiver Encoding. + // ZIP 316: Consumers MUST reject Unified Addresses/Viewing Keys in + // which any constituent Item does not meet the validation + // requirements of its encoding. if sapling::PaymentAddress::from_bytes(&data).is_none() { tracing::error!("Unified Address contains invalid Sapling receiver"); false diff --git a/src/rust/src/unified_keys_ffi.rs b/src/rust/src/unified_keys_ffi.rs index a2adabd82..ca0b19708 100644 --- a/src/rust/src/unified_keys_ffi.rs +++ b/src/rust/src/unified_keys_ffi.rs @@ -34,7 +34,43 @@ pub extern "C" fn unified_full_viewing_key_parse( match unsafe { CStr::from_ptr(encoded) }.to_str() { Ok(encoded) => match Ufvk::decode(encoded) { - Ok((parsed_network, fvk)) if parsed_network == network => Box::into_raw(Box::new(fvk)), + Ok((parsed_network, fvk)) if parsed_network == network => { + // ZIP 316: Consumers MUST reject Unified Addresses/Viewing Keys in which + // any constituent Item does not meet the validation requirements of its + // encoding. + for item in fvk.items() { + match item { + Fvk::Orchard(data) => { + if orchard::keys::FullViewingKey::from_bytes(&data).is_none() { + error!("Unified FVK contains invalid Orchard FVK"); + return std::ptr::null_mut(); + } + } + Fvk::Sapling(data) => { + // The last 32 bytes is the diversifier key, which is opaque. + // The remaining 96 bytes should be a valid Sapling FVK. + if zcash_primitives::sapling::keys::FullViewingKey::read(&data[..96]) + .is_err() + { + error!("Unified FVK contains invalid Sapling FVK"); + return std::ptr::null_mut(); + } + } + Fvk::P2pkh(data) => { + // The first 32 bytes is the chaincode, which is opaque. + // The remaining 33 bytes should be the compressed encoding of + // a secp256k1 point. + if let Err(e) = secp256k1::PublicKey::from_slice(&data[32..]) { + error!("Unified FVK contains invalid transparent FVK: {}", e); + return std::ptr::null_mut(); + } + } + // Can't check anything for unknown typecodes. + Fvk::Unknown { .. } => (), + } + } + Box::into_raw(Box::new(fvk)) + } Ok((parsed_network, _)) => { error!( "Key was encoded for a different network ({:?}) than what was requested ({:?})",