EspressoSystems / jellyfish

A Rust Implementation of the PLONK ZKP System and Extensions
https://jellyfish.docs.espressosys.com
MIT License
408 stars 106 forks source link

security: ADVZ verify_share should ensure merkle leaf matches `Share` data #660

Closed ggutoski closed 2 months ago

ggutoski commented 3 months ago

As observed in #659 , Share struct contains two copies of payload data: one in Share::evals and another in Share::evals_proof. The copy in Share::evals_proof is checked against its merkle path here:

https://github.com/EspressoSystems/jellyfish/blob/92714a4cc509fac07b8e8fc321fc0271c5dbe6b6/vid/src/advz.rs#L536-L542

All subsequent verification is done against Share::evals. Currently we do not check that these two copies are equal, so there's a potential security vulnerability. This issue could be fixed by checking that these two copies are equal. But a far better solution is to eliminate the duplicate copy as suggested in #659. Presumably, any fix for #659 will also fix this issue.

[EDIT: Kudos to @akonring for noticing this potential vulnerability.]

philippecamacho commented 2 months ago

Duplicate #659.