PVE-AC verification is self-referential in aggregate_to_restore_row, allowing forged bundles to pass

[abster333]

Summary:

aggregate_to_restore_row calls verify with the ciphertext's own Q instead of a caller-supplied commitments ( Xs/Q_expected), so an internally consistent forged bundle passes verification.

Affected components:

• Core: ec_pve_ac_t::aggregate_to_restore_row, ec_pve_ac_t::verify
• Files:
  • src/cbmpc/protocol/pve_ac.cpp
  • src/cdmp/protocol/pve_ac.h

Evidence:

• verify compares provided Q and derived L to internal fields, making checks tautological if Q is internal:
  • Lines 131-147: pve_ac.cpp
• aggregate_to_restore_row passes the bundle's own Q into verify only when all_ac_pks is non-empty:
  • lines 225-235: pve_ac.cpp
• Defualt parameters imply verifcation is expected to be active but are effectively bypassed when all_ac_pks is empty:
  • Lines 71-74: pve_ac.h

Expected vs actual

• Expeted: With skip_verify=fale, recovery should validate against external commitments (Xs/Q_expected) or a signed manifest hash. If absent, fail closed.
• Actual: Recovery acepts fresh cipertext over attacker-chosen scalers if internally consistent; verification is self-referential and can be skipped when all_ac_pks is empty.

What this means

• An atacker who can replace the stored PVE-AC bundle can cause recovery to restore attacker-chosen private scalers.
• Downstream systms that prompote recovered keys (e.g., signing, withdrawals, API auth) can then be driven by attacker post-recovery.

Fix Direction

• Pass caller‑supplied expected Xs (or manifest hash) into verify.
• Fail closed when skip_verify=false but commitments are absent.
• Update tests to expect rejection of forged bundles and to cover the “missing all_ac_pks while skip_verify=false” path.

[Arash-Afshar]

Thanks for the report. You’re right that aggregate_to_restore_row currently calls verify with the bundle’s internal Q, and that skip_verify=false may effectively do nothing, which is confusing and easy to misuse.
Our intended flow is that callers verify the bundle against external expected commitments (public shares / Q_expected, or an equivalent trusted manifest) via verify/AcVerify before recovery; aggregate_to_restore_row’s checks are consistency checks, not a standalone authenticity guarantee.
That said, we agree the API should be safer-by-default and clearer. We’ll update it soon.