From cfa36097a3d0c85cebeb7c19b3789b6454589cae Mon Sep 17 00:00:00 2001 From: Ramana Kumar Date: Fri, 2 Dec 2022 12:30:48 +0000 Subject: [PATCH 1/4] Add checks that field elements are canonical --- src/c_kzg_4844.c | 49 ++++++++++++++++++++++++++++++++++-------------- src/c_kzg_4844.h | 2 +- 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/src/c_kzg_4844.c b/src/c_kzg_4844.c index 000dbdd..a6372b3 100644 --- a/src/c_kzg_4844.c +++ b/src/c_kzg_4844.c @@ -840,12 +840,20 @@ static void compute_powers(BLSFieldElement out[], BLSFieldElement *x, uint64_t n } } -void bytes_to_bls_field(BLSFieldElement *out, const uint8_t bytes[32]) { +static void hash_to_bls_field(BLSFieldElement *out, const uint8_t bytes[32]) { blst_scalar tmp; blst_scalar_from_lendian(&tmp, bytes); blst_fr_from_scalar(out, &tmp); } +C_KZG_RET bytes_to_bls_field(BLSFieldElement *out, const uint8_t bytes[32]) { + blst_scalar tmp; + blst_scalar_from_lendian(&tmp, bytes); + if (!blst_scalar_fr_check(&tmp)) return C_KZG_BADARGS; + blst_fr_from_scalar(out, &tmp); + return C_KZG_OK; +} + static void poly_lincomb(Polynomial out, const Polynomial vectors[], const fr_t scalars[], uint64_t n) { fr_t tmp; uint64_t i, j; @@ -923,14 +931,19 @@ static C_KZG_RET poly_to_kzg_commitment(KZGCommitment *out, const Polynomial p, return g1_lincomb(out, s->g1_values, p, FIELD_ELEMENTS_PER_BLOB); } -static void poly_from_blob(Polynomial p, const Blob blob) { - for (size_t i = 0; i < FIELD_ELEMENTS_PER_BLOB; i++) - bytes_to_bls_field(&p[i], &blob[i * BYTES_PER_FIELD_ELEMENT]); +static C_KZG_RET poly_from_blob(Polynomial p, const Blob blob) { + C_KZG_RET ret; + for (size_t i = 0; i < FIELD_ELEMENTS_PER_BLOB; i++) { + ret = bytes_to_bls_field(&p[i], &blob[i * BYTES_PER_FIELD_ELEMENT]); + if (ret != C_KZG_OK) return ret; + } + return C_KZG_OK; } C_KZG_RET blob_to_kzg_commitment(KZGCommitment *out, const Blob blob, const KZGSettings *s) { Polynomial p; - poly_from_blob(p, blob); + C_KZG_RET ret = poly_from_blob(p, blob); + if (ret != C_KZG_OK) return ret; return poly_to_kzg_commitment(out, p, s); } @@ -968,8 +981,9 @@ C_KZG_RET verify_kzg_proof(bool *out, const KZGProof *kzg_proof, const KZGSettings *s) { BLSFieldElement frz, fry; - bytes_to_bls_field(&frz, z); - bytes_to_bls_field(&fry, y); + C_KZG_RET ret; + ret = bytes_to_bls_field(&frz, z); if (ret != C_KZG_OK) return ret; + ret = bytes_to_bls_field(&fry, y); if (ret != C_KZG_OK) return ret; return verify_kzg_proof_impl(out, commitment, &frz, &fry, kzg_proof, s); } @@ -1143,14 +1157,14 @@ static C_KZG_RET compute_challenges(BLSFieldElement *out, BLSFieldElement r_powe /* Compute r_powers */ BLSFieldElement r; - bytes_to_bls_field(&r, r_bytes); + hash_to_bls_field(&r, r_bytes); compute_powers(r_powers, &r, n); /* Compute eval_challenge */ uint8_t eval_challenge[32] = {0}; hash_input[32] = 0x1; hash(eval_challenge, hash_input, 33); - bytes_to_bls_field(out, eval_challenge); + hash_to_bls_field(out, eval_challenge); free(bytes); return C_KZG_OK; @@ -1185,15 +1199,20 @@ C_KZG_RET compute_aggregate_kzg_proof(KZGProof *out, Polynomial* polys = calloc(n, sizeof(Polynomial)); if (0 < n && polys == NULL) { free(commitments); return C_KZG_MALLOC; } + C_KZG_RET ret; for (size_t i = 0; i < n; i++) { - poly_from_blob(polys[i], blobs[i]); + ret = poly_from_blob(polys[i], blobs[i]); + if (ret != C_KZG_OK) { + if (commitments != NULL) free(commitments); + if (polys != NULL) free(polys); + return ret; + } poly_to_kzg_commitment(&commitments[i], polys[i], s); } Polynomial aggregated_poly; KZGCommitment aggregated_poly_commitment; BLSFieldElement evaluation_challenge; - C_KZG_RET ret; ret = compute_aggregated_poly_and_commitment(aggregated_poly, &aggregated_poly_commitment, &evaluation_challenge, polys, commitments, n); if (commitments != NULL) free(commitments); if (polys != NULL) free(polys); @@ -1210,13 +1229,15 @@ C_KZG_RET verify_aggregate_kzg_proof(bool *out, const KZGSettings *s) { Polynomial* polys = calloc(n, sizeof(Polynomial)); if (polys == NULL) return C_KZG_MALLOC; - for (size_t i = 0; i < n; i++) - poly_from_blob(polys[i], blobs[i]); + C_KZG_RET ret; + for (size_t i = 0; i < n; i++) { + ret = poly_from_blob(polys[i], blobs[i]); + if (ret != C_KZG_OK) { free(polys); return ret; } + } Polynomial aggregated_poly; KZGCommitment aggregated_poly_commitment; BLSFieldElement evaluation_challenge; - C_KZG_RET ret; ret = compute_aggregated_poly_and_commitment(aggregated_poly, &aggregated_poly_commitment, &evaluation_challenge, polys, expected_kzg_commitments, n); free(polys); if (ret != C_KZG_OK) return ret; diff --git a/src/c_kzg_4844.h b/src/c_kzg_4844.h index 9618839..a6e53c3 100644 --- a/src/c_kzg_4844.h +++ b/src/c_kzg_4844.h @@ -89,7 +89,7 @@ typedef struct { C_KZG_RET bytes_to_g1(g1_t* out, const uint8_t in[48]); void bytes_from_g1(uint8_t out[48], const g1_t *in); -void bytes_to_bls_field(BLSFieldElement *out, const uint8_t in[BYTES_PER_FIELD_ELEMENT]); +C_KZG_RET bytes_to_bls_field(BLSFieldElement *out, const uint8_t in[BYTES_PER_FIELD_ELEMENT]); C_KZG_RET load_trusted_setup(KZGSettings *out, const uint8_t g1_bytes[], /* n1 * 48 bytes */ From 4101648253163e1fc8362fcb7122b73cdbd915b0 Mon Sep 17 00:00:00 2001 From: Ramana Kumar Date: Fri, 2 Dec 2022 12:49:20 +0000 Subject: [PATCH 2/4] Update python test for encoding checks We ensure we are below the modulus by just using a zero final byte for each field element encoding. In the test, I do not understand why changing the final (zero) byte causes verification to succeed instead of failing. But this is why the change is now to the first byte. --- bindings/python/tests.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/bindings/python/tests.py b/bindings/python/tests.py index 7b2082f..2b4ea3e 100644 --- a/bindings/python/tests.py +++ b/bindings/python/tests.py @@ -7,7 +7,9 @@ BLOB_SIZE = 4096 MAX_BLOBS_PER_BLOCK = 16 blobs = [ - random.randbytes(32 * BLOB_SIZE) for _ in range(3) + # use zero final bytes to easily ensure the encodings are valid + b''.join([b''.join([random.randbytes(31), bytes(1)]) for _ in range(BLOB_SIZE)]) + for _ in range(3) ] ts = ckzg.load_trusted_setup("../../src/trusted_setup.txt") @@ -26,8 +28,8 @@ assert ckzg.verify_aggregate_kzg_proof(blobs_bytes, kzg_commitments, proof, ts), # Verification fails at wrong value -other = b'x' if not blobs_bytes.endswith(b'x') else b'y' -other_bytes = blobs_bytes[:-1] + other +other = b'x' if not blobs_bytes.startswith(b'x') else b'y' +other_bytes = other + blobs_bytes[1:] assert not ckzg.verify_aggregate_kzg_proof(other_bytes, kzg_commitments, proof, ts), 'verify succeeded incorrectly' From 86f9f5d2ecb6553ea94ae0ee51d42135b01fd653 Mon Sep 17 00:00:00 2001 From: Alexey Osipov Date: Mon, 12 Dec 2022 00:01:21 +0300 Subject: [PATCH 3/4] Align tests with the modulus check --- bindings/csharp/Ckzg.Test/E2eTests.cs | 39 +++++++++++++++++---------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/bindings/csharp/Ckzg.Test/E2eTests.cs b/bindings/csharp/Ckzg.Test/E2eTests.cs index 00a4fe8..898b575 100644 --- a/bindings/csharp/Ckzg.Test/E2eTests.cs +++ b/bindings/csharp/Ckzg.Test/E2eTests.cs @@ -5,30 +5,41 @@ namespace Ckzg.Test; [TestFixture] public class BasicKzgTests { - [TestCase] - public unsafe void Test_Computes_And_Verifies() + private IntPtr _ts; + + [SetUp] + public void Setup() { - IntPtr ts = Ckzg.LoadTrustedSetup("trusted_setup.txt"); - Assert.That((ulong)ts, Is.Not.EqualTo((ulong)0)); - byte[] blob = Enumerable.Range(0, 4096 * 32).Select(x => (byte)(x % 256)).ToArray(); + _ts = Ckzg.LoadTrustedSetup("trusted_setup.txt"); + Assert.That(_ts, Is.Not.EqualTo(IntPtr.Zero)); + } + + [TestCase(0xff, -1, -1)] + [TestCase(0x73, -1, -1)] + [TestCase(0x72, 0, 0)] + [TestCase(0x00, 0, 0)] + public unsafe void Test_Computes_And_Verifies(byte highByteValue, int expectedProofComputed, int expectedProofVerified) + { + byte[] blob = Enumerable.Range(0, 4096 * 32).Select(x => x % 32 == 31 ? highByteValue : (byte)(x % 256)).ToArray(); byte[] proof = new byte[48]; byte[] commitment = new byte[48]; fixed (byte* commitmentPtr = commitment, blobPtr = blob, proofPtr = proof) { - Ckzg.ComputeAggregatedKzgProof(proofPtr, blobPtr, 1, ts); - Ckzg.BlobToKzgCommitment(commitmentPtr, blobPtr, ts); - int result = Ckzg.VerifyAggregatedKzgProof(blobPtr, commitmentPtr, 1, proofPtr, ts); - Assert.That(result, Is.EqualTo(0)); - Ckzg.FreeTrustedSetup(ts); + int proofComputed = Ckzg.ComputeAggregatedKzgProof(proofPtr, blobPtr, 1, _ts); + Assert.That(proofComputed, Is.EqualTo(expectedProofComputed)); + + Ckzg.BlobToKzgCommitment(commitmentPtr, blobPtr, _ts); + int proofVerified = Ckzg.VerifyAggregatedKzgProof(blobPtr, commitmentPtr, 1, proofPtr, _ts); + Assert.That(proofVerified, Is.EqualTo(expectedProofVerified)); + + Ckzg.FreeTrustedSetup(_ts); } } [TestCase] public unsafe void Test_PointEvaluationPrecompile_Verifies() { - IntPtr ts = Ckzg.LoadTrustedSetup("trusted_setup.txt"); - Assert.That((ulong)ts, Is.Not.EqualTo((ulong)0)); byte[] commitment = new byte[48]; commitment[0] = 0xc0; byte[] x = new byte[32]; @@ -38,8 +49,8 @@ public class BasicKzgTests fixed (byte* commitmentPtr = commitment, xPtr = x, yPtr = y, proofPtr = proof) { - int result = Ckzg.VerifyKzgProof(commitmentPtr, xPtr, yPtr, proofPtr, ts); - Ckzg.FreeTrustedSetup(ts); + int result = Ckzg.VerifyKzgProof(commitmentPtr, xPtr, yPtr, proofPtr, _ts); + Ckzg.FreeTrustedSetup(_ts); Assert.That(result, Is.EqualTo(0)); } } From e5338b771ff0304f748023b4164bb25a97cff600 Mon Sep 17 00:00:00 2001 From: dancoffman Date: Mon, 12 Dec 2022 14:48:05 -0800 Subject: [PATCH 4/4] Do not allow blob fields to overflow --- bindings/node.js/test.ts | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/bindings/node.js/test.ts b/bindings/node.js/test.ts index 74c07be..b60f302 100644 --- a/bindings/node.js/test.ts +++ b/bindings/node.js/test.ts @@ -20,7 +20,19 @@ const SETUP_FILE_PATH = existsSync(setupFileName) const BLOB_BYTE_COUNT = FIELD_ELEMENTS_PER_BLOB * BYTES_PER_FIELD_ELEMENT; -const generateRandomBlob = () => new Uint8Array(randomBytes(BLOB_BYTE_COUNT)); +const MAX_TOP_BYTE = 114; + +const generateRandomBlob = () => { + return new Uint8Array( + randomBytes(BLOB_BYTE_COUNT).map((x, i) => { + // Set the top byte to be low enough that the field element doesn't overflow the BLS modulus + if (x > MAX_TOP_BYTE && i % BYTES_PER_FIELD_ELEMENT == 31) { + return Math.floor(Math.random() * MAX_TOP_BYTE); + } + return x; + }), + ); +}; describe("C-KZG", () => { beforeAll(async () => { @@ -50,8 +62,7 @@ describe("C-KZG", () => { ); }); - // Just don't call verifyAggregateKzgProof when there are no blobs or commitments - it.skip("verifies the aggregate proof of empty blobs and commitments", () => { + it("verifies the aggregate proof of empty blobs and commitments", () => { expect(verifyAggregateKzgProof([], [], computeAggregateKzgProof([]))).toBe( true, );