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

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

Closed
ggutoski opened this issue Aug 16, 2024 · 1 comment
Closed
Assignees
Labels

Comments

@ggutoski
Copy link
Contributor

ggutoski commented Aug 16, 2024

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:

jellyfish/vid/src/advz.rs

Lines 536 to 542 in 92714a4

// verify eval proof
// TODO: check all indices that represents the shares
if KzgEvalsMerkleTree::<E, H>::verify(
common.all_evals_digest,
&KzgEvalsMerkleTreeIndex::<E, H>::from(share.index as u64),
&share.evals_proof,
)

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
Copy link
Contributor

Duplicate #659.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants