fix: note hash collision #11869

benesjan · 2025-02-10T12:59:23Z

Fixes #11862
+ fixes generator collision vulnerability spotted by Nico. The issue was that G_slot value was sometimes accidentally generated by the macros an used for other non-storage-slot note field. I fixed that in this PR by nuking separate generators.nr file and making the auto-generated generators start at index 2, while G_slot and G_len have indices 0 and 1 (talking about indices in derive_generators("aztec_nr_generators".as_bytes(), INDEX).

benesjan · 2025-02-10T12:59:39Z

refactor!: Notes implementing Packable<N> #12004
refactor!: nuking NoteHeader #11942
refactor: moving storage slot out of NoteHeader #11904
fix: note hash collision #11869 👈 (View in Graphite)
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

github-actions · 2025-02-10T15:48:16Z

Changes to public function bytecode sizes

Generated at commit: f9f3de2708f0ecba6c2b6d931cc588d6d97c66d9, compared to commit: c6cc3d381253f79d3532c2940f73a9304665f4d4

🧾 Summary (100% most significant diffs)

Program	Bytecode size in bytes (+/-)	%
TokenBlacklist::mint_private	+131 ❌	+3.86%
TokenBlacklist::shield	+141 ❌	+2.59%
TokenBlacklist::public_dispatch	+106 ❌	+0.45%

Full diff report 👇

Program	Bytecode size in bytes (+/-)	%
TokenBlacklist::mint_private	3,525 (+131)	+3.86%
TokenBlacklist::shield	5,586 (+141)	+2.59%
TokenBlacklist::public_dispatch	23,831 (+106)	+0.45%

noir-projects/aztec-nr/aztec/src/generators.nr

benesjan · 2025-02-10T15:53:45Z

noir-projects/aztec-nr/aztec/src/generators.nr

-        let generators: [Point; 6] = derive_generators("aztec_nr_generators".as_bytes(), 0);
+        // We use "aztec_nr_generators_fixed" instead of "aztec_nr_generators" to avoid collisions with generators
+        // that are generated dynamically by `generate_multi_scalar_mul(...)`
+        let generators: [Point; 7] = derive_generators("aztec_nr_generators_fixed".as_bytes(), 0);


This was a vulnerability flagged by @nventuro

Since I was already touching the file I decided to fix it here by changing the input bytes.

This was the issue.

I think a more future-proof solution might be to put G_slot and G_len at the beginning (indexes 0 and 1) instead of at the end. Then if we ever grow the number of fields in a partial note, the "slot" and "len" generators won't change. What do you think?

I wonder if the G_len should be the exact same G_len that is used by the pedersen_hash bberg function. I'm not actually sure how we could reliably get hold of that value and use it here, though.

Are the generators_fixed reserved for other things? I actually don't know about how we derive generators at the mo.

Edit: I took a look at aztec-packages/noir/noir-repo/noir_stdlib/src/hash/mod.nr: notice the function

Should we be using this pedersen_commitment_with_separator function? Should we be using the DEFAULT_DOMAIN_SEPARATOR and pedersen_hash_length, or would that collide with other pedersen hashes in the stack?

I decided to do a few things here. First I nuked the generators.nr file as it was now only used for the import o G_slot and G_len and by custom implemenation of 2 note types: MockNote (used only for testing) and the EcdsaPublicKeyNote. Both of these are not partial notes so I just modified the note hash func to use poseidon2.

Regarding the G_slot and G_len I know generate them inside the macros and with this change they are next to the function that generates the other generators for the MSM used in note hashes. This I think makes it safer as we have 1 place where we generate the generators and they are only used by macros.

If devs for whatever reason will want their own generators they will need to generate them on their own.

Also as you proposed the G_slot and G_len now are at the beginning and the rest of the generators start at start_index 2 for this reason (derive_generators("aztec_nr_generators".as_bytes(), **2**)).

Should we be using this pedersen_commitment_with_separator function?

I don't think it works because we don't want the length to be included when finalizing the partial note (computing the commitment of the publicly added values and then adding that to the original partial_note_commitment).

Should we be using the DEFAULT_DOMAIN_SEPARATOR and pedersen_hash_length, or would that collide with other pedersen hashes in the stack?

I don't really see the value in this and would keep that separate just for the peace of mind.

noir-projects/aztec-nr/aztec/src/generators.nr

noir-projects/noir-protocol-circuits/crates/types/src/constants.nr

iAmMichaelConnor

Thanks Jan! I've written some comments. I'm not sure which can actually easily be solved now, versus deferred to the dedicated "rigorous domain separation" epic in github. Perhaps if you don't think you'll be able to implement one of my suggested changes in this PR, please can you write a TODO in the code, and paraphrase my concern, and then point to the "domain separation" github epic.

iAmMichaelConnor · 2025-02-10T16:18:56Z

noir-projects/aztec-nr/aztec/src/generators.nr

-        let generators: [Point; 6] = derive_generators("aztec_nr_generators".as_bytes(), 0);
+        // We use "aztec_nr_generators_fixed" instead of "aztec_nr_generators" to avoid collisions with generators
+        // that are generated dynamically by `generate_multi_scalar_mul(...)`
+        let generators: [Point; 7] = derive_generators("aztec_nr_generators_fixed".as_bytes(), 0);


I think a more future-proof solution might be to put G_slot and G_len at the beginning (indexes 0 and 1) instead of at the end. Then if we ever grow the number of fields in a partial note, the "slot" and "len" generators won't change. What do you think?

iAmMichaelConnor · 2025-02-10T16:22:29Z

noir-projects/aztec-nr/aztec/src/generators.nr

-        let generators: [Point; 6] = derive_generators("aztec_nr_generators".as_bytes(), 0);
+        // We use "aztec_nr_generators_fixed" instead of "aztec_nr_generators" to avoid collisions with generators
+        // that are generated dynamically by `generate_multi_scalar_mul(...)`
+        let generators: [Point; 7] = derive_generators("aztec_nr_generators_fixed".as_bytes(), 0);


I wonder if the G_len should be the exact same G_len that is used by the pedersen_hash bberg function. I'm not actually sure how we could reliably get hold of that value and use it here, though.

noir-projects/aztec-nr/aztec/src/macros/notes/mod.nr

iAmMichaelConnor · 2025-02-10T16:33:36Z

noir-projects/aztec-nr/aztec/src/generators.nr

-        let generators: [Point; 6] = derive_generators("aztec_nr_generators".as_bytes(), 0);
+        // We use "aztec_nr_generators_fixed" instead of "aztec_nr_generators" to avoid collisions with generators
+        // that are generated dynamically by `generate_multi_scalar_mul(...)`
+        let generators: [Point; 7] = derive_generators("aztec_nr_generators_fixed".as_bytes(), 0);


Are the generators_fixed reserved for other things? I actually don't know about how we derive generators at the mo.

Edit: I took a look at aztec-packages/noir/noir-repo/noir_stdlib/src/hash/mod.nr: notice the function

Should we be using this pedersen_commitment_with_separator function? Should we be using the DEFAULT_DOMAIN_SEPARATOR and pedersen_hash_length, or would that collide with other pedersen hashes in the stack?

noir-projects/noir-contracts/contracts/ecdsa_public_key_note/src/lib.nr

benesjan · 2025-02-11T10:09:51Z

noir-projects/aztec-nr/aztec/src/encrypted_logs/log_assembly_strategies/default_aes128/note.nr

+            0x00af91db798fa320746831a59b74362dfd0cf9e7c239f6aad11a4b47d0d870ee,
+            0x00d25a054613a83be7be8512f2c09664bc4f7ab60a127b06584f476918581b8a,
+            0x003840d100d8c1d78d4b68b787ed353ebfb8cd2987503d3b472f614f25799a18,
+            0x003f38322629d401010101010101010101010101010101010101010101010101,


This change got triggered by me moving the generator indices by 1:

benesjan · 2025-02-11T10:17:15Z

yarn-project/circuit-types/src/logs/l1_payload/encrypted_log_payload.test.ts

@@ -84,7 +84,7 @@ describe('EncryptedLogPayload', () => {
    const payload = await log.generatePayload(ephSk, recipientCompleteAddress.address, fixedRand);

    expect(payload.toBuffer().toString('hex')).toMatchInlineSnapshot(
-      `"0e9cffc3ddd746affb02410d8f0a823e89939785bcc8e88ee4f3cae05e737c360d460c0e434d846ec1ea286e4090eb56376ff27bddc1aacae1d856549f701fa700010577790aeabcc2d81ec8d0c99e7f5d2bf2f1452025dc777a178404f851d9003de81cde78411f27a921e16ebbfba71a5570d3f62f1134c90daced33663ba000856cb19c7d563da183a40a6f8bd4988d1696ad6bf0c717c8fb8f6294bd0366001ed04e4f77a111c7090fcd34c61cfae744e8589a42defba4d0d927dd4679fe00ec09b49d8d4cf548ea62d44c8839b2fd14664e9d1439b199a8d5166e362348004a69de2d410e010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101"`,
+      `"0e9cffc3ddd746affb02410d8f0a823e89939785bcc8e88ee4f3cae05e737c360d460c0e434d846ec1ea286e4090eb56376ff27bddc1aacae1d856549f701fa7000194e6d7872db8f61e8e59f23580f4db45d13677f873ec473a409cf61fd04d00334e5fb6083721f3eb4eef500876af3c9acfab0a1cb1804b930606fdb0b28300af91db798fa320746831a59b74362dfd0cf9e7c239f6aad11a4b47d0d870ee00d25a054613a83be7be8512f2c09664bc4f7ab60a127b06584f476918581b8a003840d100d8c1d78d4b68b787ed353ebfb8cd2987503d3b472f614f25799a18003f38322629d4010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101"`,


This change got triggered by me moving the generator indices by 1:

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.76.2</summary> ## [0.76.2](aztec-package-v0.76.1...aztec-package-v0.76.2) (2025-02-11) ### Miscellaneous * **logging:** Support explicit FORCE_COLOR parameter ([#11902](#11902)) ([3b3f859](3b3f859)) </details> <details><summary>barretenberg.js: 0.76.2</summary> ## [0.76.2](barretenberg.js-v0.76.1...barretenberg.js-v0.76.2) (2025-02-11) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.76.2</summary> ## [0.76.2](aztec-packages-v0.76.1...aztec-packages-v0.76.2) (2025-02-11) ### Features * Batch writes to the proving broker database ([#11900](#11900)) ([608f887](608f887)) ### Bug Fixes * Cleanup also post test_kind.sh ([#11886](#11886)) ([50cdb15](50cdb15)) * Dont skip wasm civc tests ([#11909](#11909)) ([0395e0b](0395e0b)) * Note hash collision ([#11869](#11869)) ([f289b7c](f289b7c)) * Orchestrator test ([#11901](#11901)) ([f1bb51c](f1bb51c)) * Smt_verification: negative bitvecs, changed gates indicies. acir_formal_proofs: noir-style signed division ([#11649](#11649)) ([4146496](4146496)) * Update path of stern logs ([#11906](#11906)) ([05afb5b](05afb5b)) ### Miscellaneous * Arm runner start fix ([#11903](#11903)) ([6c83c40](6c83c40)) * Fixing the sizes of VMs in CIVC ([#11793](#11793)) ([1afddbd](1afddbd)) * **logging:** Support explicit FORCE_COLOR parameter ([#11902](#11902)) ([3b3f859](3b3f859)) * Misc fixes to devnet deploy flow ([#11738](#11738)) ([bc4cca7](bc4cca7)) * Remove warnings from noir protocol circuits ([#11803](#11803)) ([c6cc3d3](c6cc3d3)) * Replace relative paths to noir-protocol-circuits ([74d6e6a](74d6e6a)) * Replacing use of capsules 1.0 with pxe_db + nuking capsules 1.0 ([#11885](#11885)) ([72be678](72be678)) </details> <details><summary>barretenberg: 0.76.2</summary> ## [0.76.2](barretenberg-v0.76.1...barretenberg-v0.76.2) (2025-02-11) ### Bug Fixes * Note hash collision ([#11869](#11869)) ([f289b7c](f289b7c)) * Smt_verification: negative bitvecs, changed gates indicies. acir_formal_proofs: noir-style signed division ([#11649](#11649)) ([4146496](4146496)) ### Miscellaneous * Fixing the sizes of VMs in CIVC ([#11793](#11793)) ([1afddbd](1afddbd)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.76.2</summary> ## [0.76.2](AztecProtocol/aztec-packages@aztec-package-v0.76.1...aztec-package-v0.76.2) (2025-02-11) ### Miscellaneous * **logging:** Support explicit FORCE_COLOR parameter ([#11902](AztecProtocol/aztec-packages#11902)) ([3b3f859](AztecProtocol/aztec-packages@3b3f859)) </details> <details><summary>barretenberg.js: 0.76.2</summary> ## [0.76.2](AztecProtocol/aztec-packages@barretenberg.js-v0.76.1...barretenberg.js-v0.76.2) (2025-02-11) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.76.2</summary> ## [0.76.2](AztecProtocol/aztec-packages@aztec-packages-v0.76.1...aztec-packages-v0.76.2) (2025-02-11) ### Features * Batch writes to the proving broker database ([#11900](AztecProtocol/aztec-packages#11900)) ([608f887](AztecProtocol/aztec-packages@608f887)) ### Bug Fixes * Cleanup also post test_kind.sh ([#11886](AztecProtocol/aztec-packages#11886)) ([50cdb15](AztecProtocol/aztec-packages@50cdb15)) * Dont skip wasm civc tests ([#11909](AztecProtocol/aztec-packages#11909)) ([0395e0b](AztecProtocol/aztec-packages@0395e0b)) * Note hash collision ([#11869](AztecProtocol/aztec-packages#11869)) ([f289b7c](AztecProtocol/aztec-packages@f289b7c)) * Orchestrator test ([#11901](AztecProtocol/aztec-packages#11901)) ([f1bb51c](AztecProtocol/aztec-packages@f1bb51c)) * Smt_verification: negative bitvecs, changed gates indicies. acir_formal_proofs: noir-style signed division ([#11649](AztecProtocol/aztec-packages#11649)) ([4146496](AztecProtocol/aztec-packages@4146496)) * Update path of stern logs ([#11906](AztecProtocol/aztec-packages#11906)) ([05afb5b](AztecProtocol/aztec-packages@05afb5b)) ### Miscellaneous * Arm runner start fix ([#11903](AztecProtocol/aztec-packages#11903)) ([6c83c40](AztecProtocol/aztec-packages@6c83c40)) * Fixing the sizes of VMs in CIVC ([#11793](AztecProtocol/aztec-packages#11793)) ([1afddbd](AztecProtocol/aztec-packages@1afddbd)) * **logging:** Support explicit FORCE_COLOR parameter ([#11902](AztecProtocol/aztec-packages#11902)) ([3b3f859](AztecProtocol/aztec-packages@3b3f859)) * Misc fixes to devnet deploy flow ([#11738](AztecProtocol/aztec-packages#11738)) ([bc4cca7](AztecProtocol/aztec-packages@bc4cca7)) * Remove warnings from noir protocol circuits ([#11803](AztecProtocol/aztec-packages#11803)) ([c6cc3d3](AztecProtocol/aztec-packages@c6cc3d3)) * Replace relative paths to noir-protocol-circuits ([74d6e6a](AztecProtocol/aztec-packages@74d6e6a)) * Replacing use of capsules 1.0 with pxe_db + nuking capsules 1.0 ([#11885](AztecProtocol/aztec-packages#11885)) ([72be678](AztecProtocol/aztec-packages@72be678)) </details> <details><summary>barretenberg: 0.76.2</summary> ## [0.76.2](AztecProtocol/aztec-packages@barretenberg-v0.76.1...barretenberg-v0.76.2) (2025-02-11) ### Bug Fixes * Note hash collision ([#11869](AztecProtocol/aztec-packages#11869)) ([f289b7c](AztecProtocol/aztec-packages@f289b7c)) * Smt_verification: negative bitvecs, changed gates indicies. acir_formal_proofs: noir-style signed division ([#11649](AztecProtocol/aztec-packages#11649)) ([4146496](AztecProtocol/aztec-packages@4146496)) ### Miscellaneous * Fixing the sizes of VMs in CIVC ([#11793](AztecProtocol/aztec-packages#11793)) ([1afddbd](AztecProtocol/aztec-packages@1afddbd)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

benesjan force-pushed the 02-10-fix_note_hash_collision branch from 5de5cce to 9fc6507 Compare February 10, 2025 15:06

benesjan added the e2e-all CI: Enables this CI job. label Feb 10, 2025

benesjan marked this pull request as ready for review February 10, 2025 15:06

benesjan requested a review from iAmMichaelConnor February 10, 2025 15:51

benesjan commented Feb 10, 2025

View reviewed changes

noir-projects/aztec-nr/aztec/src/generators.nr Outdated Show resolved Hide resolved

benesjan commented Feb 10, 2025

View reviewed changes

noir-projects/aztec-nr/aztec/src/generators.nr Show resolved Hide resolved

benesjan commented Feb 10, 2025

View reviewed changes

noir-projects/noir-protocol-circuits/crates/types/src/constants.nr Show resolved Hide resolved

iAmMichaelConnor reviewed Feb 10, 2025

View reviewed changes

benesjan added 6 commits February 11, 2025 07:56

fix: note hash collision

2dc69e6

WIP

351d383

WIP

3b67ff6

WIP

927c1be

WIP

ca5ba54

WIP

eaf340d

benesjan force-pushed the 02-10-fix_note_hash_collision branch from 9fc6507 to eaf340d Compare February 11, 2025 09:24

test fix

3ba8801

benesjan removed the e2e-all CI: Enables this CI job. label Feb 11, 2025

benesjan commented Feb 11, 2025

View reviewed changes

benesjan added 2 commits February 11, 2025 10:19

docs

9000e08

WIP

f66084d

benesjan requested a review from iAmMichaelConnor February 11, 2025 11:02

benesjan mentioned this pull request Feb 11, 2025

refactor: moving storage slot out of NoteHeader #11904

Merged

iAmMichaelConnor approved these changes Feb 11, 2025

View reviewed changes

benesjan merged commit f289b7c into master Feb 11, 2025
52 of 53 checks passed

benesjan deleted the 02-10-fix_note_hash_collision branch February 11, 2025 17:08

AztecBot mentioned this pull request Feb 11, 2025

chore(master): Release 0.76.2 #11899

Merged

This was referenced Feb 12, 2025

refactor!: nuking NoteHeader #11942

Open

refactor!: Notes implementing Packable<N> #12004

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: note hash collision #11869

fix: note hash collision #11869

benesjan commented Feb 10, 2025 •

edited

Loading

benesjan commented Feb 10, 2025 •

edited

Loading

github-actions bot commented Feb 10, 2025 •

edited

Loading

benesjan Feb 10, 2025

iAmMichaelConnor Feb 10, 2025

iAmMichaelConnor Feb 10, 2025

iAmMichaelConnor Feb 10, 2025

benesjan Feb 11, 2025 •

edited

Loading

iAmMichaelConnor left a comment

iAmMichaelConnor Feb 10, 2025

iAmMichaelConnor Feb 10, 2025

iAmMichaelConnor Feb 10, 2025

benesjan Feb 11, 2025

benesjan Feb 11, 2025

fix: note hash collision #11869

fix: note hash collision #11869

Conversation

benesjan commented Feb 10, 2025 • edited Loading

benesjan commented Feb 10, 2025 • edited Loading

github-actions bot commented Feb 10, 2025 • edited Loading

Changes to public function bytecode sizes

🧾 Summary (100% most significant diffs)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benesjan Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

iAmMichaelConnor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benesjan commented Feb 10, 2025 •

edited

Loading

benesjan commented Feb 10, 2025 •

edited

Loading

github-actions bot commented Feb 10, 2025 •

edited

Loading

benesjan Feb 11, 2025 •

edited

Loading