fix: handle empty or malformed logs during message processing by nchamo · Pull Request #21192 · AztecProtocol/aztec-packages

nchamo · 2026-03-05T23:28:35Z

Problem

AES128::decrypt in aes128.nr currently panics at 5 points when given malformed ciphertext or wrong-key data. During message discovery (do_sync_state), a panic in decrypt crashes the entire sync process rather than gracefully skipping the unprocessable log.

Additionally, do_sync_state itself panics when it encounters empty logs, since it unconditionally indexes into the log without checking its length first.

Panic points in `decrypt`

Empty ciphertext -- ciphertext.get(0) panics when the BoundedVec is empty.
Short header plaintext -- header_plaintext.get(0) / .get(1) panic when the AES decrypt oracle returns fewer than 2 bytes (e.g. wrong-key PKCS#7 stripping produces an empty result).
Invalid ciphertext length -- BoundedVec::from_parts(ciphertext_with_padding, ciphertext_length) panics when the 2-byte header decodes to a length exceeding MESSAGE_PLAINTEXT_SIZE_IN_BYTES (e.g. 65535 from corrupted data).
Invalid plaintext length -- fields_from_bytes asserts bytes.len() % 32 == 0, panicking when the decrypted body has a non-aligned length (e.g. 33 bytes).
Field overflow -- fields_from_bytes asserts each 32-byte chunk fits within the BN254 field modulus, panicking when decrypted bytes exceed it (e.g. 0xFF repeated 32 times).

Panic in `do_sync_state`

do_sync_state processes every pending tagged log without validating its size. It would panic when pending_tagged_log.log.get(0) was called on empty logs

Important for reviewers

I recommend using "hide whitespace" config when reviewing file changes, since it would make the changes easier to understand

Fixes F-356
Fixes F-191

nchamo · 2026-03-05T23:30:31Z

noir-projects/aztec-nr/aztec/src/messages/processing/message_context.nr

 /// order to perform validation, store results, etc. For example, messages containing notes require knowledge of note
 /// hashes and the first nullifier in order to find the note's nonce.
-#[derive(Deserialize, Eq)]
+#[derive(Deserialize, Eq, Serialize)]


Needed for the do_sync_state test

nchamo · 2026-03-05T23:30:34Z

noir-projects/aztec-nr/aztec/src/messages/processing/pending_tagged_log.nr

-#[derive(Deserialize, Eq)]
+#[derive(Deserialize, Eq, Serialize)]
 pub(crate) struct PendingTaggedLog {
    pub log: BoundedVec<Field, PRIVATE_LOG_SIZE_IN_FIELDS>,


Needed for the do_sync_state test

…rmed input

…ability

nchamo · 2026-03-06T11:50:43Z

noir-projects/aztec-nr/aztec/src/messages/encryption/aes128.nr

        recipient: AztecAddress,
    ) -> Option<BoundedVec<Field, MESSAGE_PLAINTEXT_LEN>> {
-        let eph_pk_x = ciphertext.get(0);
+        // Extract the ephemeral public key x-coordinate and masked fields, returning None for empty ciphertext.


Since Noir lacks early return, I though the best way to make this clear as possible was to fully embrace Option. Open to other suggestions though

~~embrace monads~~

It's a bit annoying that we end up in nested-hell, but it is the cleanest way of handling this I think.

nchamo · 2026-03-06T11:51:15Z

noir-projects/aztec-nr/aztec/src/messages/encryption/aes128.nr

    }
+
+    #[test]
+    unconstrained fn decrypt_returns_none_on_empty_ciphertext() {


All these scenarios used to cause a panic

nchamo · 2026-03-06T11:53:24Z

noir-projects/aztec-nr/aztec/src/utils/conversion/fields_to_bytes.nr

-        for j in 0..32 {
-            let next_byte = bytes.get(i * 32 + j);
-            field = field * 256 + next_byte as Field;
+    try_fields_from_bytes(bytes).expect(f"Value does not fit in field")


I'm now using try_fields_from_bytes to avoid the repeated logic, but let me know if you think the two should coexist (maybe as a form of optimization?)

But expect can fail, no? So if the bytes cannot be put into fields we'll panic. Is this not an issue?

Sorry I wasn't clear. Both functions can co-exists. I didn't delete fields_from_bytes because it was public and maybe someone users are using it

My comment tried (and failed) to imply that having fields_from_bytes use try_fields_from_bytes might not be the most optimized solution, since we do things a little differently to handle the Option instead of panicking. It should be negligible, but I wanted to double check

noir-projects/aztec-nr/aztec/src/utils/conversion/fields_to_bytes.nr

nventuro · 2026-03-07T00:24:34Z

noir-projects/aztec-nr/aztec/src/messages/discovery/mod.nr

+            capsules::store(contract_address, base_slot, 1 as Field);
+            capsules::store(
+                contract_address,
+                base_slot + 1,
+                PendingTaggedLog { log: BoundedVec::new(), context: std::mem::zeroed() },
+            );
+
+            let logs: CapsuleArray<PendingTaggedLog> = CapsuleArray::at(contract_address, base_slot);
+            assert_eq(logs.len(), 1);


Instead of doing things this way, just create the capsule array and push into it.

nventuro · 2026-03-07T00:28:34Z

noir-projects/aztec-nr/aztec/src/messages/discovery/mod.nr

+            let no_handler: Option<CustomMessageHandler<()>> = Option::none();
+            do_sync_state(contract_address, dummy_compute_nhnn, no_handler);


Suggested change

let no_handler: Option<CustomMessageHandler<()>> = Option::none();

do_sync_state(contract_address, dummy_compute_nhnn, no_handler);

do_sync_state(contract_address, dummy_compute_nhnn, Option::none());

This way you don't need to write the type.

That didn't work for me. I think Noir needs me to specify what the type for CustomMessageHandler is, even if Option is empty. It can infer it

nventuro · 2026-03-07T00:28:58Z

noir-projects/aztec-nr/aztec/src/messages/discovery/mod.nr

+    use crate::protocol::address::AztecAddress;
+
+    #[test]
+    unconstrained fn do_sync_state_can_handle_empty_logs() {


Suggested change

unconstrained fn do_sync_state_can_handle_empty_logs() {

unconstrained fn do_sync_state_does_not_panic_on_empty_logs() {

nventuro · 2026-03-07T00:32:10Z

noir-projects/aztec-nr/aztec/src/messages/encryption/aes128.nr

        recipient: AztecAddress,
    ) -> Option<BoundedVec<Field, MESSAGE_PLAINTEXT_LEN>> {
-        let eph_pk_x = ciphertext.get(0);
+        // Extract the ephemeral public key x-coordinate and masked fields, returning None for empty ciphertext.


It's a bit annoying that we end up in nested-hell, but it is the cleanest way of handling this I think.

nventuro · 2026-03-07T00:34:02Z

noir-projects/aztec-nr/aztec/src/messages/encryption/aes128.nr

    ) -> Option<BoundedVec<Field, MESSAGE_PLAINTEXT_LEN>> {
-        let eph_pk_x = ciphertext.get(0);
+        // Extract the ephemeral public key x-coordinate and masked fields, returning None for empty ciphertext.
+        split_ciphertext(ciphertext).and_then(|(eph_pk_x, masked_fields)| {


What if this were a lambda?

|ciphertext| { if ciphertext.len() > 0 { Option::some( (ciphertext.get(0), array::subbvec(ciphertext, EPH_PK_X_SIZE_IN_FIELDS)), ) } else { Option::none() } }.and_then(|(eph_pk_x, masked_fields)| {

nventuro · 2026-03-07T01:03:47Z

noir-projects/aztec-nr/aztec/src/messages/encryption/aes128.nr

+            header_bytes[0] = 0xFF;
+            header_bytes[1] = 0xFF;


Better to have the len be MESSAGE_PLAINTEXT_SIZE_IN_BYTES+1, so that we make sure we're handling the edge case correctly.

nventuro · 2026-03-07T01:05:05Z

noir-projects/aztec-nr/aztec/src/utils/conversion/fields_to_bytes.nr

-        for j in 0..32 {
-            let next_byte = bytes.get(i * 32 + j);
-            field = field * 256 + next_byte as Field;
+    try_fields_from_bytes(bytes).expect(f"Value does not fit in field")


But expect can fail, no? So if the bytes cannot be put into fields we'll panic. Is this not an issue?

nventuro · 2026-03-07T01:05:59Z

noir-projects/aztec-nr/aztec/src/utils/conversion/fields_to_bytes.nr

-                    ok = true;
+/// Non-panicking version of `fields_from_bytes`. Returns `Option::none()` if the input
+/// length is not a multiple of 32 or if any 32-byte chunk exceeds the field modulus.
+pub fn try_fields_from_bytes<let N: u32>(bytes: BoundedVec<u8, N>) -> Option<BoundedVec<Field, N / 32>> {


Suggested change

pub fn try_fields_from_bytes<let N: u32>(bytes: BoundedVec<u8, N>) -> Option<BoundedVec<Field, N / 32>> {

fn try_fields_from_bytes<let N: u32>(bytes: BoundedVec<u8, N>) -> Option<BoundedVec<Field, N / 32>> {

Let's keep API surface small for now.

nventuro · 2026-03-07T01:10:22Z

noir-projects/aztec-nr/aztec/src/utils/conversion/fields_to_bytes.nr

+    if bytes.len() % 32 == 0 {
+        let mut fields = BoundedVec::new();
+        let p = std::field::modulus_be_bytes();
+        let mut valid = true;


Might read easier if extracting this body into a fn. Typically helps with nested options etc.

nventuro · 2026-03-07T01:10:50Z

noir-projects/aztec-nr/aztec/src/utils/conversion/fields_to_bytes.nr

+        // 0xFF repeated 32 times is larger than the BN254 field modulus.
+        let input = BoundedVec::<_, 32>::from_array([0xFF as u8; 32]);


Better to test with p directly to check the edge case.

benesjan · 2026-03-08T09:54:55Z

Since Nico went through this and the feedback was not yet addressed here I don't think my review is necessary at this point.

@nchamo please request review again from me if you would like me to chime in.

nchamo commented Mar 5, 2026

View reviewed changes

nchamo self-assigned this Mar 6, 2026

fix: make decrypt return Option::none() instead of panicking on malfo…

ba3b667

…rmed input

nchamo force-pushed the fix/decrypt-returns-none-instead-of-panicking branch from 8d1b7b8 to ba3b667 Compare March 6, 2026 01:13

nchamo added 2 commits March 5, 2026 22:23

fix: simplify do_sync_state guard, improve try_fields_from_bytes read…

4876b19

…ability

fix: rename test to do_sync_state_can_handle_empty_logs

1260523

nchamo commented Mar 6, 2026

View reviewed changes

nchamo changed the title ~~fix: make decrypt return Option::none() instead of panicking on malformed input~~ fix: handle empty or malformed logs during message processing Mar 6, 2026

nchamo marked this pull request as ready for review March 6, 2026 13:53

nchamo requested a review from nventuro as a code owner March 6, 2026 13:53

nchamo requested a review from benesjan March 6, 2026 13:53

nventuro reviewed Mar 7, 2026

View reviewed changes

benesjan removed their request for review March 8, 2026 09:55

nchamo added 2 commits March 9, 2026 11:14

test: add AES128 decrypt failure mode tests and improve comment clarity

79cd08b

fix: clean up AES128 decrypt failure mode test assertions

e25fb08

nchamo requested a review from nventuro March 9, 2026 16:33

		let no_handler: Option<CustomMessageHandler<()>> = Option::none();
		do_sync_state(contract_address, dummy_compute_nhnn, no_handler);

	unconstrained fn do_sync_state_can_handle_empty_logs() {
	unconstrained fn do_sync_state_does_not_panic_on_empty_logs() {

	pub fn try_fields_from_bytes<let N: u32>(bytes: BoundedVec<u8, N>) -> Option<BoundedVec<Field, N / 32>> {
	fn try_fields_from_bytes<let N: u32>(bytes: BoundedVec<u8, N>) -> Option<BoundedVec<Field, N / 32>> {

		// 0xFF repeated 32 times is larger than the BN254 field modulus.
		let input = BoundedVec::<_, 32>::from_array([0xFF as u8; 32]);

Conversation

nchamo commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Panic points in decrypt

Panic in do_sync_state

Important for reviewers

Uh oh!

nchamo Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchamo Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchamo Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchamo Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benesjan commented Mar 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nchamo commented Mar 5, 2026 •

edited

Loading

Panic points in `decrypt`

Panic in `do_sync_state`

nchamo Mar 5, 2026 •

edited

Loading

nchamo Mar 5, 2026 •

edited

Loading

nchamo Mar 6, 2026 •

edited

Loading

nchamo Mar 9, 2026 •

edited

Loading