fix(gabikeys): wipe secret prime factors from private key (#69) - #70
fix(gabikeys): wipe secret prime factors from private key (#69)#70dobby-coder[bot] wants to merge 8 commits into
Conversation
Go's math/big is not constant-time, so a co-located attacker can in principle recover secret material by timing the arithmetic performed on the issuer's secret primes during key generation (issue #8). The one concrete, non-breaking mitigation from that issue is to not retain the secret prime factors longer than necessary: they are only used to derive N and the group order. Add PrivateKey.Destroy, which overwrites and clears P, Q, P', Q', the group order and the ECDSA revocation key. Callers can invoke it once the key has been persisted or is no longer needed to shorten the time the highest-risk secret values linger in memory. The public modulus N is retained. This does not make math/big constant-time (that requires replacing the bignum implementation and is out of scope). The persisted XML format is unchanged, so existing keys keep round-tripping. Regression tests assert the secret factors are cleared and their backing memory overwritten after a key is generated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict is request-changes — posted as COMMENT only because GitHub blocks self-request-changes on a bot's own PR. Treat as blocking.
Rules Dobby 2 gatekeeper review — 1 bug, 1 nit, 1 PR-hygiene issue.
Nice, focused mitigation for the storage half of #8/#69, and thanks for the call-site audit table — that's exactly the analysis the issue asked for. Two code issues and one PR-hygiene point block sign-off:
- Bug —
Destroy()leaves the ECDSA revocation private key fully recoverable. It nilsECDSAand zeroesECDSA.D, but never clearsECDSAString, which holds the base64-encoded marshaled ECDSA private key (set fromsigned.MarshalPrivateKeyatkeys.go:277). AfterDestroy(),RevocationSupported()still returns true andparseRevocationKey()reconstructs the full private scalarD. The doc comment ("the revocation (ECDSA) private key … the key can no longer sign or be serialized") is therefore false. See inline. Fix: also setprivk.ECDSAString = "". - Nit —
ECDSA.D.SetInt64(0)does not scrub the backing words (unlikewipeInt), so the secret scalar bytes linger in memory. See inline. - PR hygiene — no closing keyword for #69. The body uses "Addresses … #69" and "Related: #8, #67", so merging will not auto-close the issue this PR resolves. Add a
Closes #69line (per the closing-keyword rule). Keep #8 as a soft reference since it's only partially addressed.
Sending back to PR Dobby to fix.
- Clear ECDSAString so the base64-marshaled ECDSA private key can no longer be reconstructed via parseRevocationKey after Destroy. - Scrub the ECDSA scalar D's backing words (wipeStdInt) instead of only truncating its length with SetInt64(0). - Extend TestPrivateKeyDestroy to assert the revocation key is unrecoverable after Destroy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Verdict is approve — posted as COMMENT only because GitHub blocks self-approval on a bot's own PR. Treat as a sign-off; no changes requested.
Rules Dobby 2 gatekeeper review — cycle 2: APPROVE.
All cycle-1 findings are resolved and verified on head commit 4e75aa9:
- Revocation key fully wiped —
Destroy()now setsprivk.ECDSAString = ""(soRevocationSupported()returns false andparseRevocationKey()cannot reconstructD) and scrubs the scalar viawipeStdInt(privk.ECDSA.D), which zeroes the backing words (not justSetInt64(0)).TestPrivateKeyDestroyasserts the revocation key is unrecoverable afterDestroy. - Closing keyword added — the body now leads with
Closes #69, so merging auto-closes the issue.
Rule compliance pass (Haiku per-rule check) is clean: conventional-commit title ✓, closing keyword ✓, tests-on-fixes ✓ (TestPrivateKeyDestroy + TestPrivateKeyDestroyIdempotent via the real GenerateKeyPair path), promised-vs-delivered ✓, Go/lint ✓. Two automated flags were verified as false positives: the allZero([]mbig.Word) "type mismatch" is a non-issue because gabi/big.Int.Bits() returns math/big.Word (big/int.go:117), and the "missing audit" flag missed the Exp/Mul/Gcd audit table that is present in the PR body.
Scope note (non-blocking, no change required): this is an opt-in Destroy() the caller must invoke; the XML key format still persists P/Q/PPrime/QPrime on disk. The body reasonably justifies this (interop preservation; wiring Destroy into consumers like irmago is a documented follow-up). This satisfies the storage-mitigation ask of #8/#69; the constant-time bignum work remains tracked in #8.
Mechanics verified correct: big.Int.Go() is a pointer cast so Bits() zeroes live backing memory; N is retained (public); revocation key unrecoverable after Destroy. go test ./... green (10/10 pkgs), gofmt/vet clean.
|
/dobby could this break any production use cases? |
|
On it — I'll dig into PR #70's changes and check for any production use cases that could be affected, then report back here. |
|
Short answer: no — merging this PR as-is can't break anything in production. The diff is purely additive (one new The catch is for later, once someone wires |
|
/dobby fix the merge conflict and reevaluate |
|
On it — I'll resolve the merge conflict on this PR and reevaluate the changes afterward. |
master added gabikeys/keys_test.go (external test package gabikeys_test, the issue #56 file-permission regression tests) where this branch had added a file of the same name in the internal package gabikeys. Both are needed and Go allows both packages in one directory, but not in one file: keep master's keys_test.go verbatim and move this branch's Destroy tests to gabikeys/destroy_test.go. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…thing Two defects in Destroy, found while re-evaluating the change: encoding/xml omits nil pointer fields rather than erroring, so a destroyed key marshalled cleanly into a structurally valid IssuerPrivateKey document containing no key material, with a nil error. Since the natural place to call Destroy is right after writing the key to disk, a later reordering of those two statements would have overwritten a production private key with an empty one and exited 0. Track the wipe in a `destroyed` flag and have Validate, WriteTo and WriteToFile (and so Print) return an error once it is set. The WriteToFile check runs before the file is opened, because the forceOverwrite branch truncates on open. NewPrivateKey stored the caller's prime pointers, so Destroy zeroed the caller's own p and q in place. Copy them in the constructor instead. Also scope the doc comment: wiping helps most right after GenerateKeyPair, and reaches less on the NewPrivateKeyFromFile path, where the primes also live in the heap as the decimal text of the XML file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Merge conflict resolved and the change re-evaluated. Head is now The conflictMaster's #56 fix and this branch both added
Re-evaluation: two defects fixedRe-reading the branch turned up the two problems I flagged when you asked about production impact, both in this PR's own new code, so I fixed them here.
Three new tests cover this and all three fail on the previous head of this branch: I also scoped the doc comment. Wiping reaches furthest right after UnchangedStill opt-in: nothing in gabi or irmago calls @rubenhensen your approval predates these two commits, so it needs another look. One note for whoever runs the lint gate locally: |
…ontract Two doc-only corrections from gatekeeper review, no behaviour change: - ECDSAString is a Go string, so clearing it drops the reference but does not overwrite the base64 bytes the way wipeInt overwrites the big.Int backing words. Name that alongside the GC-copy and XML-decimal-text caveats already in the best-effort paragraph. - Destroy mutates the key without synchronization while Validate/WriteTo/ WriteToFile and the issuance and revocation paths read the same fields, so concurrent use is a data race, not just a nil-dereference. State the quiescence requirement consumers have to meet.
There was a problem hiding this comment.
Verdict is approve — posted as COMMENT only because GitHub blocks self-approval on a bot's own PR. Treat as a sign-off; no changes requested.
Rules Dobby 2 gatekeeper review — cycle 3 (final): APPROVE.
Trigger was /dobby fix the merge conflict and reevaluate. The conflict is resolved and the resolution is provably lossless; the two remaining findings were doc-only and are fixed in this cycle.
Merge-conflict resolution — verified clean
The hand resolution described in the body (master's #56 permission tests kept in gabikeys/keys_test.go, the Destroy tests moved to gabikeys/destroy_test.go to avoid two packages in one file) holds up:
- Nothing dropped from either side. Comparing the branch's contribution before the merge (
git diff $(git merge-base origin/master 56a792f)..56a792f) against its contribution after (git diff origin/master...50524f6), the+/-lines are identical — somaster + branch delta == merged treeexactly, in every file, flagged or not. - Master's
keys_test.gosurvived byte-for-byte —git hash-objectis16132d1bon bothorigin/masterand HEAD. The only file HEAD adds over master's file list isgabikeys/destroy_test.go. - Not a no-op. The two-dot
git diff origin/master HEADis non-empty and identical to the three-dot diff:keys.go +113/-2,destroy_test.go +198. mergeable: MERGEABLE; all four checks green (CodeQL, analyze, lint, test).
Findings this cycle — 0 bugs, 2 nits, both fixed here
Neither warranted another coder loop, so I committed them directly as 85df3fb (doc-only, no behaviour change):
ECDSAStringwipe was overstated.privk.ECDSAString = ""clears the string header, but Go strings are immutable — the base64 bytes are not overwritten the waywipeInt/wipeStdIntzero thebig.Intbacking words. The code already does the most that is possible for a Go string; the problem was the description. TheDestroybest-effort paragraph now names this caveat next to the GC-copy and XML-decimal-text ones, and the PR body's "overwrites the backing memory of ... the base64ECDSAString" claim is corrected to distinguish cleared (unreconstructable) from overwritten.Destroyhas no concurrency contract. It nilsP/Q/PPrime/QPrime/Orderand setsdestroyedwithout synchronization, while those same fields are read byValidate/WriteTo/WriteToFileand by the issuance and revocation paths — confirmed live atissuer.go:93,98,106,clsignature.go:67andrevocation/api.go:196, with no mutex and no existing goroutine-safety contract anywhere ingabikeys. A consumer sharing one*PrivateKeyacross concurrent sessions gets a data race, not just a nil-deref. The doc comment now states the quiescence requirement. This matters for the deferred irmago integration, which the body already flags as a separate decision.
Rule compliance
Per-rule sweep (Haiku fan-out over Go-correctness, PR-hygiene and merge/claim-accuracy rule batches) came back clean apart from the ECDSAString item above, which all three batches independently converged on — corroborating the reviewer's finding rather than adding a new one. Verified: conventional-commit title ✓; Closes #69 confirmed via GraphQL closingIssuesReferences, not just body text ✓; reviewer assigned (@w-ensink) ✓; regression tests exercise the real GenerateKeyPair/NewPrivateKey/WriteToFile path rather than mocks ✓; PR ready, not a stale green draft ✓; no CHANGELOG in this repo, so no stale sibling entry could ride in on the merge ✓. Locally re-run on HEAD: go test ./... 10/10 packages, go vet, gofmt, go fix -diff all clean.
Not an embargoed fix — #8 and #69 are long-standing public issues and there is no GHSA reference, so nothing here needed holding back.
Scope note (unchanged, non-blocking)
Destroy() remains opt-in with no callers in gabi or irmago, and the XML format still persists P/Q/PPrime/QPrime on disk. Both are disclosed in the body with sound reasoning (interop preservation; wiring it into consumers is a follow-up integration decision). This delivers the storage mitigation #8 asked for; the constant-time bignum work stays tracked in #8.
This was cycle 3, the gatekeeper loop limit, so the automated review loop ends here — but it ends on a clean state rather than an exhausted one: there are no unresolved findings. A human approving review is still required to merge (reviewDecision: REVIEW_REQUIRED — @rubenhensen's earlier approval was dismissed by the subsequent pushes, and Dobby never merges PRs). Over to @w-ensink.
|
Done — the merge conflict resolution checked out as lossless (byte-identical Everything else held up: title, One thing worth a maintainer's judgment call: |
2 participants
Closes #69
Summary
Addresses the key-generation timing side-channel tracked in #69 (and the long-standing #8).
Go's
math/bigis documented as not constant-time. As #8 describes, a co-located attacker able to take precise cache/branch-timing measurements of the arithmetic performed on the issuer's secret primes during key generation has a realistic path to recovering secret material. That cannot be fully closed without replacing the bignum implementation, a large separate effort which #8 itself calls out ("implement and possibly upstream const-time exp/gcd arithmetic").The one concrete, non-breaking mitigation named in #8 is its final recommendation: the secret prime factors "are not needed for anything else" beyond deriving
Nand the group order, so they should not be stored. This PR implements that.What changed
PrivateKey.Destroy()ingabikeys/keys.goclears the secret material:P,Q,P',Q', the groupOrder(P'·Q'), and the ECDSA revocation private key (both the parsed key and the base64ECDSAStringit can be reconstructed from). For thebig.Intvalues and the ECDSA scalarDit also overwrites the backing words in place.ECDSAStringis a Go string and therefore immutable, so clearing it drops the reference and makes the key unreconstructable, but its base64 bytes are not overwritten and linger until the GC collects them — the doc comment names that caveat. Callers invoke it once the key is persisted or no longer needed, which shortens the window the highest-risk secret values linger in process memory. It is best-effort, since Go's GC may already have copied values, so it is defense in depth. It is idempotent and nil-safe. The public modulusNis retained; it is not secret.A destroyed key now fails instead of being usable.
Validate,WriteTo,WriteToFileandPrintreturn an error onceDestroyhas run.encoding/xmlomits nil pointer fields rather than erroring, so without this a destroyed key marshalled into a structurally validIssuerPrivateKeydocument holding no key material at all, with a nil error. The natural place to callDestroyis right after writing the key to disk, so a later reordering of those two statements would have overwritten a production key with an empty one and exited 0. TheWriteToFilecheck runs before the file is opened, because theforceOverwritebranch truncates on open.NewPrivateKeynow copiespandq. It previously stored the caller's pointers, soDestroyzeroed the caller's own primes in place.Regression tests in
gabikeys/destroy_test.gogenerate a key through the realGenerateKeyPairpath (toy 256-bit params for speed) and assert that afterDestroythe secret factors are nil and their backing word arrays are zeroed, the revocation key cannot be reconstructed,Nsurvives, serialization fails, an existing key file on disk is left untouched, and the caller's own primes are intact.The persisted XML key format is unchanged, so existing keys keep round-tripping and there is no interop break.
Scope of the mitigation
Wiping reaches furthest right after
GenerateKeyPair, where the primes exist only as thebig.Ints thatDestroyoverwrites. On the load-from-disk path (NewPrivateKeyFromFile) the primes also live in the heap as the decimal text of the XML file, whichDestroydoes not touch. So this is not a general "the primes are gone" guarantee, and the doc comment says so.Audit of
Exp/Mul/Gcdcall sites on secret materialPer the request in #69 to audit remaining sites for unblinded secret material:
gabikeys/keys.gokeygenMul(p,q)→N,Mul(p',q')→Order,Rsh(p,1),LegendreSymbol(s, P/Q)clsignature.go:67,issuer.go:93,revocation/{proof,api}.goModInverse(e, sk.Order)Ordereis a fresh random prime per signature, giving per-invocation noise. This is the "negligible chance" remote case in #8.issuer.go:98,106randomElementMultiplicativeGroup(Order),Mod(·, Order)OrdereCommitper proof.issuer.go:88(Exp(Q, d, pk.N))d = e⁻¹Nis publicdderives from the per-signature randome.clsignature.goeven has a pre-existingTODOabout this exact spot.keyproof/*.goGCDs (squarefree,disjointprimeproduct,primepowerproduct,almostsafeprimeproduct,quasisafeprimeproduct)GCD(·, ·, ·, N)NThe unblinded secret-material arithmetic is concentrated in the one-shot key-generation path; the runtime signing and revocation paths all carry a fresh per-invocation random element, which matches #8's analysis that remote exploitation is negligible. The residual co-located risk is the inherent
math/bignon-constant-time property, out of scope for a code-level change here.Testing
go test ./..., all 10 packages pass.TestPrivateKeyDestroy,TestPrivateKeyDestroyIdempotent,TestPrivateKeyDestroyDoesNotWipeCallerPrimes,TestPrivateKeyDestroyBlocksSerialization,TestPrivateKeyDestroyDoesNotTruncateExistingFile. The last three fail on the previous version of this branch and pass now.gofmt,go vet ./...,go fix -diff ./...,ineffassignandmisspellclean. staticcheck could not be run locally: go.mod requires go1.26 and the available staticcheck build is go1.24, which makes it analyse nothing. CI runs it properly.Notes for reviewers
Destroy()is opt-in. Nothing in gabi or irmago calls it yet, so merging this changes no behaviour on its own. Where consumers should invoke it is a follow-up integration decision, and doing it in the wrong place would break them, because every field it clears is load-bearing somewhere: the irma server issuance path, key validation on every session,irma scheme issuer keyprove,revocation-keypair.A further step would be to stop persisting
pPrime/qPrime(both derivable fromPandQ) to shrink the on-disk secret footprint, but that changes the XML output format and risks interop, so it is deliberately not done here.The merge with
masterneeded a hand resolution. Master's #56 permission tests and this branch's tests both addedgabikeys/keys_test.go, in the external and the internal test package respectively. Go allows both packages in one directory but not in one file, so master'skeys_test.gois kept verbatim and theDestroytests moved togabikeys/destroy_test.go.Since this branch picked up new commits, @rubenhensen's earlier approval predates the two fixes above.
Related: #8, #67 (dependency updates, separate concern).