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

Java/node bindings: Insufficient validation of inputs to verify_aggregate_kzg_proof() #63

Closed
asn-d6 opened this issue Jan 13, 2023 · 4 comments
Assignees

Comments

@asn-d6
Copy link
Contributor

asn-d6 commented Jan 13, 2023

The c-kzg public method verify_aggregate_kzg_proof() expects that its blobs and expected_kzg_commitments arguments have length of size n.

This is an undocumented requirement but still real because of the array accessing loop in

c-kzg-4844/src/c_kzg_4844.c

Lines 1290 to 1294 in c72ea8e

for (size_t i = 0; i < n; i++) {
ret = poly_from_blob(&polys[i], &blobs[i]);
if (ret != C_KZG_OK) goto out;
ret = poly_to_kzg_commitment(&commitments[i], &polys[i], s);
if (ret != C_KZG_OK) goto out;

However, this is not respected by all bindings:

  • The Java bindings do not check that there are count commitments present
  • The nodejs bindings assume that there are as many commitments as blobs and then freely copy memory into the short commitments[blob_index] array

I suggest we document this requirement in the core c-kzg library so that bindings know what to expect. We also need to fix the above two cases. We might also need some unittests that check this behavior.

@asn-d6
Copy link
Contributor Author

asn-d6 commented Jan 16, 2023

For additional context here, the caller of verify_aggregate_kzg_proof() makes sure that commitments and blobs have the same size: https://github.com/ethereum/consensus-specs/blob/dev/specs/eip4844/fork-choice.md#validate_blobs_sidecar

It might be worth highlighting that invariant in the bindings by adding an assert.

@xrchz
Copy link
Contributor

xrchz commented Jan 21, 2023

I think this is all done by #68

@xrchz xrchz closed this as completed Jan 21, 2023
@xrchz
Copy link
Contributor

xrchz commented Jan 21, 2023

Oh maybe the node bindings are still not done?

@xrchz xrchz reopened this Jan 21, 2023
@dgcoffman
Copy link
Contributor

I can add a check in the Node bindings

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

No branches or pull requests

3 participants