Repository navigation
Check ElGamal group membership when verifying cast ballots - #495
Merged
Merged
Conversation
Ballot verification is supposed to confirm that the submitted ciphertext elements belong to the order-q subgroup described by the election public key, but the check was only present on a code path that cast ballots do not reach. There are three ElGamal implementations in the tree. crypto/electionalgs.py performs the check, but a cast ballot never goes through it: CastVote wraps a legacy/EncryptedVote, which datatypes.legacy maps onto workflows.homomorphic.EncryptedVote and crypto.elgamal.Ciphertext. Neither of those had the check, and crypto.elgamal.Ciphertext had no check_group_membership method at all. Add check_group_membership to crypto.elgamal.Ciphertext, mirroring the implementation in crypto.algs, and call it from workflows.homomorphic.EncryptedAnswer.verify, mirroring the call in crypto.electionalgs. The JavaScript booth and verifier had the same gap, so add the equivalent check to ElGamal.Ciphertext.verifyDisjunctiveProof. Two related fixes to the same verification code: - crypto.algs.EGZKProof.verify referenced self.pk, which EGZKProof does not carry, so it raised AttributeError on every call and broke trustee decryption proof verification. Use the p and q passed in as arguments. - crypto.elgamal was missing the subgroup check on proof commitments that crypto.algs already performs. Bring the two into line. Tests build a ballot whose alpha lies outside the subgroup but whose proof equations all hold, and assert that verification rejects it. They run against both crypto.algs and crypto.elgamal, since it was the drift between the two implementations that let this through, and they pin down which one a cast ballot actually deserializes into. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bf8CC2rBHmaP1AN1ZTeqAv
…stic Drop the mixin that ran every case against crypto.algs as well. A cast ballot deserializes into crypto.elgamal, and so does a trustee's uploaded decryption proof, so the algs variants were exercising combinations that do not occur; the parity that actually matters is covered by two small cases that check both implementations agree. Drive the forged ballot from a seeded Random rather than the module-level RNG, and construct the simulated proof branches in the test instead of calling simulate_encryption_proof, which draws from the process CSPRNG. The forgery is now byte for byte identical on every run, so a failure reproduces exactly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bf8CC2rBHmaP1AN1ZTeqAv
Contributor
There was a problem hiding this comment.
Pull request overview
This PR closes a verification gap where cast ballots could bypass ElGamal order‑q subgroup membership checks for ciphertext elements, ensuring ballot validation rejects ciphertexts with valid proof equations but invalid group elements. It also aligns drifting ElGamal implementations (Python + JS) and fixes a latent parameter misuse in DH tuple proof verification.
Changes:
- Add subgroup membership checking to the Python
crypto.elgamal.Ciphertextimplementation and enforce it during cast-ballot verification (workflows.homomorphic.EncryptedAnswer.verify). - Add equivalent subgroup membership enforcement to
ElGamal.Ciphertext.verifyDisjunctiveProofin both JavaScript copies (booth + verifier). - Fix
crypto.algs.EGZKProof.verifyto use the(p, q)parameters passed toverify()(instead of non-existentself.pk), and add regression tests that would fail onmaster.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
heliosverifier/js/jscrypto/elgamal.js |
Adds subgroup membership check and enforces it before disjunctive proof verification in the verifier JS crypto. |
heliosbooth/js/jscrypto/elgamal.js |
Mirrors the verifier change so the booth JS crypto rejects out-of-subgroup ciphertext elements as well. |
helios/workflows/homomorphic.py |
Enforces subgroup membership validation on each choice during EncryptedAnswer.verify, covering cast ballot verification paths. |
helios/tests.py |
Adds targeted tests for out-of-subgroup ciphertexts with still-valid proof equations, plus drift/regression tests across implementations. |
helios/crypto/elgamal.py |
Adds check_group_membership and adds subgroup checks on proof commitments (aligning with crypto.algs). |
helios/crypto/algs.py |
Fixes DH tuple proof verification to use the provided (p, q) arguments (eliminating a latent self.pk AttributeError path). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
hengeb
added a commit
to Mind-Hochschul-Netzwerk/helios-server
that referenced
this pull request
Aug 23, 2026
Bringt drei sicherheitsrelevante Upstream-Commits ein: - CSRF-Schutz für bisher ungeschützte zustandsändernde Views (benadida#496) - Fix für verzerrtes Sampling in random_mpz_lt (benadida#494) - ElGamal-Gruppenzugehörigkeitsprüfung beim Verifizieren gecasteter Stimmen (benadida#495) Konfliktauflösung: bei den CSRF-Formularen (jetzt <form method="post"> statt <a href>) wurde jeweils die deutsche MHN-Beschriftung beibehalten, die Formular-/CSRF-Struktur von upstream übernommen. uv.lock bleibt wie in diesem Fork üblich ungetrackt. Nebenbei behoben: one_election_edit griff noch auf die Formularfelder use_voter_aliases/private_p zu, die im "simplified usage"-Commit aus dem ElectionForm entfernt wurden - führte zu einem KeyError beim Bearbeiten einer Wahl, aufgedeckt durch den aktualisierten Upstream-Test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ballot verification is supposed to confirm that the submitted ciphertext elements belong to the order-q subgroup described by the election public key. That check exists in the codebase, but it sits on a code path that cast ballots do not reach.
Why it was missed
There are three ElGamal implementations in the tree, and they have drifted apart.
crypto/electionalgs.pyperforms the check. A cast ballot never goes through it:CastVote.voteis alegacy/EncryptedVote, whichdatatypes.legacymaps ontoworkflows.homomorphic.EncryptedVoteandcrypto.elgamal.Ciphertext. Neither of those carried the check —crypto.elgamal.Ciphertexthad nocheck_group_membershipmethod at all.Changes
check_group_membershiptocrypto.elgamal.Ciphertext, mirroring the existing implementation incrypto.algs.workflows.homomorphic.EncryptedAnswer.verify, mirroring the existing call incrypto.electionalgs.ElGamal.Ciphertext.verifyDisjunctiveProofin the JavaScript booth and verifier, which had the same gap. The two copies ofelgamal.jsare identical before and after.Two tidy-ups to neighbouring verification code, both found while testing the above:
crypto.algs.EGZKProof.verifyreferencedself.pk, whichEGZKProofdoes not carry, so it raisedAttributeErroron every call. Introduced in c2904fb. This is latent rather than live — a trustee's uploaded decryption proofs deserialize intocrypto.elgamal.ZKProof(datatypes/legacy.py), which reads its moduli from its arguments correctly — but the two implementations should not disagree. It now uses thepandqpassed in.crypto.elgamalwas missing the subgroup check on proof commitments thatcrypto.algsalready performs. The two are now in line. This one is redundant with the membership check above rather than load-bearing, and is not separately tested for that reason.Tests
New tests construct a ballot whose
alphalies outside the subgroup but whose proof equations all still hold, and assert that verification rejects it — so the membership check is the only thing that can catch it. Two smaller cases assert that the two ElGamal implementations agree, since the drift between them is what let this through, plus one pinning down which implementation a cast ballot actually deserializes into.The forgery is driven by a seeded RNG and is byte for byte identical on every run, so a failure reproduces exactly.
Each new test fails on
masterand passes here. Full suite: 203 tests, all passing (1 skipped — the LDAP test needs a reachable server).