From 714f14168ce74d972343767f53f47325cb030063 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@z.cash>
Date: Tue, 18 Jan 2022 21:25:42 +0000
Subject: [PATCH] rust: Add missing checks for transparent UFVK items

---
 Cargo.lock                       | 25 +++++++++++++++++++++++++
 Cargo.toml                       |  1 +
 src/Makefile.am                  | 10 +++++++++-
 src/rust/src/unified_keys_ffi.rs |  7 +++++--
 4 files changed, 40 insertions(+), 3 deletions(-)

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/unified_keys_ffi.rs b/src/rust/src/unified_keys_ffi.rs
index c2d86f5b0..ca0b19708 100644
--- a/src/rust/src/unified_keys_ffi.rs
+++ b/src/rust/src/unified_keys_ffi.rs
@@ -56,11 +56,14 @@ pub extern "C" fn unified_full_viewing_key_parse(
                                 return std::ptr::null_mut();
                             }
                         }
-                        Fvk::P2pkh(_) => {
+                        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.
-                            // TODO: Check secp256k1 encoding.
+                            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 { .. } => (),