fix(agglayer): enforce canonical zero padding on leaf data#3003
fix(agglayer): enforce canonical zero padding on leaf data#3003Fumuran wants to merge 5 commits into
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mmagician
left a comment
There was a problem hiding this comment.
I'm wondering how easy would it be to add a regression test for what the original issue describes:
a malicious prover can change LEAF_VALUE - and thus the CGI chain hash the AggLayer side must reproduce - without changing any real leaf field.
This would probably require some manipulation of the data loaded in the advice provider, but I haven't thought too much how the exact attack would look like.
I'm not sure that we can assert the CGI chain hash if the leaf data is changed, we never actually read the CGI chain hash value. Or maybe I just misunderstood the idea Edit: I think implicitly we check it during the existing test: we change the padding values and make sure that this change is caught and will not result in the CGI hash chain corruption |
The bridge leaf is a 113-byte keccak preimage stored as 32 felts (29 data felts + 3 trailing padding felts). pack_leaf_data folds padding[0]'s low 3 bytes into the unused tail (byte positions 113..115) of the final packed u32. The keccak precompile truncates the hash to 113 bytes, so padding never changes LEAF_VALUE, but it requires that tail to be zero and only implicitly (a generic InvalidPadding failure). padding[1] and padding[2] are never read by the packer or the hash at all, so nothing checked them. On the bridge-in/CLAIM path the leaf data is piped verbatim from the prover-supplied advice map, so the padding felts were never explicitly validated. Make the canonical-preimage requirement explicit and total in the shared helper: pack_leaf_data now calls a new assert_padding_is_zero helper that asserts all 3 trailing padding felts are zero before packing, panicking with ERR_LEAF_PADDING_NOT_ZERO. The check is shared, so it also covers bridge-out (which already zeros these felts) and any future caller, and it yields a clear bridge-specific error. Docs updated on pack_leaf_data, compute_leaf_value, get_leaf_value, add_leaf_bridge, and SPEC.md. Adds a negative test exercising all three padding offsets. Closes #2985 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses PR review: - rename leaf_utils PADDING_OFFSET -> LEAF_DATA_PADDING_OFFSET, simplify comment - trim get_leaf_value / assert_padding_is_zero docs to the essentials - standardize Panics wording to "...in the leaf data is non-zero" - bridge_out: extract padding-zeroing into a zero_padding_felts helper and roll back the PADDING_OFFSET derivation / extra comments - roll back SPEC padding-row note and trim the CHANGELOG entry Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reuse the base padding pointer across the 3 stores instead of recomputing LEAF_DATA_START_PTR + PADDING_OFFSET each time. Uses `swap` in place of the suggested `movup.1` (the assembler restricts movup immediates to 2..15). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pass the input pointer to assert_padding_is_zero via `dup` before `loc_store`, avoiding the extra `loc_load` to read it back. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5b45944 to
d485152
Compare
|
I think the fix for the stated problem "enforce canonical zero padding on leaf data" is correct, but I wonder whether this is a real issue in the first place, now that I deep dive into this. It's true that a malicious prover could provide non-zero padding and we're not checking these values explicitly. So the CGI hash will never be updated with a malicious |
|
Indeed! It turned out that we actually return an error in case the padding is not zero, we do this check in the keccak precompile (here, we return the |
Summary
Closes #2985.
The bridge leaf is a 113-byte keccak preimage stored as 32 felts: 29 data felts followed by 3 trailing padding felts.
pack_leaf_datafoldspadding[0]'s low 3 bytes into the unused tail (byte positions 113-115) of the final packed u32. The keccak precompile truncates the hash to 113 bytes, so padding never changesLEAF_VALUE, but it requires that tail to be zero - and only enforces it implicitly, via a genericInvalidPaddingfailure.padding[1]andpadding[2]are never read by the packer or the hash at all, so nothing checked them.On the bridge-in/CLAIM path the leaf data is piped verbatim from the prover-supplied advice map, so the padding felts were never explicitly validated.
Fix
Make the requirement explicit and total in the shared helper.
pack_leaf_datanow calls a newassert_padding_is_zerohelper (under aHELPER PROCEDURESsection) that asserts all 3 trailing padding felts are zero before packing, panicking withERR_LEAF_PADDING_NOT_ZEROotherwise. Because the check lives in the shared helper, it also covers bridge-out (which already zeros these felts) and any future caller, and it yields a clear bridge-specific error instead of the generic precompile padding failure.This is defense-in-depth: it does not change
LEAF_VALUEor the claim nullifier (the nullifier is derived from the leaf index and source network, not the leaf preimage).Changes
crates/miden-agglayer/asm/agglayer/bridge/leaf_utils.masmERR_LEAF_PADDING_NOT_ZERO.PADDING_OFFSETderived fromPACKED_DATA_NUM_ELEMENTSso it stays coupled to the packing loop bound.assert_padding_is_zerohelper; called frompack_leaf_databefore the loop. Docs onpack_leaf_data/compute_leaf_valueupdated.crates/miden-agglayer/asm/agglayer/bridge/bridge_in.masmget_leaf_valueadvice-map doc updated to state padding must be zero (and why).crates/miden-agglayer/asm/agglayer/bridge/bridge_out.masmPADDING_OFFSETnow derived fromMETADATA_HASH_OFFSET + 8; comment/doc updated.crates/miden-agglayer/SPEC.mdbridge_in::claimPanics row updated.crates/miden-testing/tests/agglayer/leaf_utils.rspack_leaf_data_rejects_non_zero_paddingtest exercising all three padding offsets (29/30/31).Testing
cargo test -p miden-testing --test lib agglayer::- all 61 agglayer tests pass, including the new negative test.🤖 Generated with Claude Code