Skip to content

2/ZK-PROOF-OF-PERSONHOOD: codify audit v2 invariants - #96

Open
cc03668 wants to merge 8 commits into
ethereum:mainfrom
cc03668:specs/2-audit-v2-fixes
Open

2/ZK-PROOF-OF-PERSONHOOD: codify audit v2 invariants#96
cc03668 wants to merge 8 commits into
ethereum:mainfrom
cc03668:specs/2-audit-v2-fixes

Conversation

@cc03668

@cc03668 cc03668 commented May 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Reflects the security-correctness fixes from zkmopro#75 into normative spec text so future implementations inherit the constraints. Editorial only — no protocol changes. Each new MUST/SHOULD maps to a finding in audit_report_v2.md verified closed in audit_report_v3.md.

Changes

  • §4 Nullifier: introduce canonical_message(app_id) with normative MUSTs on fixed length and per-byte binding; specify the 64-byte SHA-256-padded encoding used by the X.509-RSA-PKCS#1v1.5-SHA-256 profile. Closes audit v2 HIGH.
  • CertChain Operations / serial-number extraction: require structural DER-walk binding for sn; SHOULD pin v3 version block as constants. Closes audit v2 CRITICAL.
  • CertChain Circuit / user_pk_limbs extraction: require the modulus tag offset and content offset be bound together via the canonical DER long-form prefix. Closes audit v2 LOW Update README.md #1.
  • Cryptographic Primitives / Poseidon: document the 128-bit security target. Closes audit v2 INFO.
  • Security Considerations: three new subsections generalize the lessons (DER-Offset Structural Binding, Nullifier Payload Canonicalization, Format-Uniqueness Assumptions) so they survive into future profile annexes.
  • References: cite audit reports v2 and v3.

Refs

Review focus

  • Each normative MUST/SHOULD maps to a constraint in PR Fix CICD failure #75's circuits or the v3 audit's verification table — please confirm none overshoots or undershoots what the implementation enforces.
  • New cross-reference anchors (#canonical-signed-message-requirement, #nullifier-payload-canonicalization, #der-offset-structural-binding, #format-uniqueness-assumptions) resolve correctly in the rendered markdown.

Out of scope (separate PRs)

  • DeviceSig → UserSig rename. PR Fix CICD failure #75 commit 72b6cc1 renamed the impl; spec still uses "DeviceSig" in 4 places. Naming drift, not security.
  • Audit v3 "areas for further investigation" (limb canonicality, smtIsOld0 boolean enforcement, outer SEQUENCE-length consistency). Not findings; not yet ready for normative text.

🤖 Generated with Claude Code

Reflects the security-correctness fixes from zkmopro#75 into
normative spec text so future implementations inherit the constraints.
Each change maps to a finding in audit_report_v2.md verified closed in
audit_report_v3.md.

- §4 Nullifier: introduce canonical_message(app_id) with normative
  MUSTs on fixed length and per-byte binding; specify the 64-byte
  SHA-256-padded encoding used by the X.509-RSA-PKCS#1v1.5-SHA-256
  profile. Closes audit v2 HIGH.
- CertChain Operations / serial-number extraction: require structural
  DER-walk binding for sn; SHOULD pin v3 version block as constants.
  Closes audit v2 CRITICAL.
- CertChain Circuit / user_pk_limbs extraction: require the modulus
  tag offset and content offset be bound together via the canonical
  DER long-form prefix. Closes audit v2 LOW ethereum#1.
- Cryptographic Primitives / Poseidon: document the 128-bit security
  target. Closes audit v2 INFO.
- Security Considerations: three new subsections generalize the
  lessons (DER-Offset Structural Binding, Nullifier Payload
  Canonicalization, Format-Uniqueness Assumptions) so they survive
  into future profile annexes.
- References: cite audit reports v2 and v3.

Refs: zkmopro#75, zkmopro#73 (audit v2),
zkmopro#74 (regression tests).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cc03668

cc03668 commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

@moven0831 @vivianjeng I'd love to have your review for this. Thank you!!!

@moven0831

moven0831 commented May 28, 2026

Copy link
Copy Markdown
Collaborator

Hey @cc03668 , should we also document the findings for audit tool here?

update: the findings will be documented here https://github.com/0xvikasrushi/noir-claude-auditor

@cc03668

cc03668 commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

Hey @cc03668 , should we also document the findings for audit tool here?

update: the findings will be documented here https://github.com/0xvikasrushi/noir-claude-auditor

Just noting that we've documented it here: 0xVikasRushi/noir-claude-auditor#1

@moven0831
moven0831 self-requested a review June 2, 2026 20:35

@moven0831 moven0831 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified each normative MUST/SHOULD added by this PR against the current implementation in zkmopro/zkID@b4f151d. All load-bearing invariants are enforced by the circuit code; two inline suggestions below to reconcile spec wording with two known-open audit-v3 follow-ups, otherwise looks good.

Comment thread specs/2-zk-proof-of-personhood/README.md Outdated
Comment thread specs/2-zk-proof-of-personhood/README.md Outdated

@moven0831 moven0831 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up on this PR's "Out of scope: DeviceSig → UserSig rename" note.

The reference impl renamed Circuit B to UserSigRSA256 in zkmopro#75 (commit 72b6cc1) — see userSig.circom and circuits/main/userSigRS2048.circom. The spec at PR HEAD 68a37c8 contains 15 DeviceSig references, not 4 — likely worth a sweep either in this PR or the planned follow-up:

Line Context
L31 Overview intro
L113 "computed in the DeviceSig circuit" — inline suggestion below
L185 Proof-pipeline step 6
L191 Circuit Design intro
L198 DeviceSig responsibilities bullet
L210 Cross-circuit linkage
L214 Cross-circuit linkage detail
L224 RSA-2048 note
L285 ### DeviceSig Circuit section header
L301 #### DeviceSig Operations section header
L303 MUST-enforce intro
L338 Verifier output schema
L427 PK_COMMIT_MISMATCH error description
L459 Verifier protocol bullet
L460 Verifier protocol assertion

Most are straightforward s/DeviceSig/UserSig/g renames. Section headers L285 / L301 change anchor IDs (#devicesig-circuit#usersig-circuit, #devicesig-operations#usersig-operations); I grepped the repo and there are no inbound cross-references to those anchors outside this file, so the anchor change is contained.

Comment thread specs/2-zk-proof-of-personhood/README.md Outdated
cc03668 and others added 3 commits June 3, 2026 16:50
Co-authored-by: Moven T <60170228+moven0831@users.noreply.github.com>
Co-authored-by: Moven T <60170228+moven0831@users.noreply.github.com>
Co-authored-by: Moven T <60170228+moven0831@users.noreply.github.com>
Comment thread specs/2-zk-proof-of-personhood/README.md Outdated
Comment thread specs/2-zk-proof-of-personhood/README.md Outdated
Comment thread specs/2-zk-proof-of-personhood/README.md Outdated
cc03668 and others added 4 commits June 8, 2026 17:57
Co-authored-by: Ya-wen, Jeng <vivi432@yahoo.com.tw>
Co-authored-by: Ya-wen, Jeng <vivi432@yahoo.com.tw>
Co-authored-by: Ya-wen, Jeng <vivi432@yahoo.com.tw>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants