Skip to content
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.

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

Fully-validate contents of UAs and UFVKs on parsing #5466

Closed
str4d opened this issue Jan 12, 2022 · 4 comments
Closed

Fully-validate contents of UAs and UFVKs on parsing #5466

str4d opened this issue Jan 12, 2022 · 4 comments
Assignees
Labels
A-rust-ffi Area: The Rust FFI in the librustzcash library. NU5 Network upgrade: NU5-specific tasks

Comments

@str4d
Copy link
Contributor

str4d commented Jan 12, 2022

The zcash_address crate intentionally treats the internals of items as opaque bytes, in order to not impose any particular protocol-specific dependency on the downstream user (for maximum usability). However, this means the user needs to do that verification themselves for known item types. This will normally happen when they get the preferred receiver to send funds to, but in the case of zcashd we need to handle a bunch more types (e.g. UFVK items).

For simplicity, we should just check all known item types at parse time. Then once we have the C++ representation (wrapping a pointer to the Rust version in most cases), we can assume correctness of the internals and .expect("We already checked this") whenever we need to perform an operation on the Unified container.

@str4d str4d added A-rust-ffi Area: The Rust FFI in the librustzcash library. NU5 Network upgrade: NU5-specific tasks labels Jan 12, 2022
@therealyingtong
Copy link
Contributor

Should we do this in librustzcash/zcash_primitives instead?

@str4d
Copy link
Contributor Author

str4d commented Jan 14, 2022

Right now, we should do this here because we parse UAs into a C++ type. However, there's a separate issue for refactoring the C++ UA type, after which it might become a wrapper around a Rust type. If that happens, then the simplest approach would be to have the zcashd wrappers wrap types from zcash_primitives that include the extra type checks, rather than wrapping the zcash_address types that don't.

@str4d str4d self-assigned this Jan 18, 2022
@str4d
Copy link
Contributor Author

str4d commented Jan 18, 2022

Oh, I already did this for UAs! The transparent ones require no extra validation (they are opaque hashes), and for Sapling I already added the necessary check:

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.
if sapling::PaymentAddress::from_bytes(data).is_none() {
tracing::error!("Unified Address contains invalid Sapling receiver");
false
} else {
unsafe { (sapling_cb.unwrap())(ua_obj, data.as_ptr()) }
}
}

The Orchard component is not yet checked because an API was missing at the time (but now exists):

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 {

So I'll open an PR to add the now-possible Orchard receiver check, and implement the missing checks in the UFVK logic.

@str4d
Copy link
Contributor Author

str4d commented Jan 19, 2022

Closed by #5484, merged into #5419.

@str4d str4d closed this as completed Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust-ffi Area: The Rust FFI in the librustzcash library. NU5 Network upgrade: NU5-specific tasks
Projects
None yet
Development

No branches or pull requests

2 participants