Skip to content

docs: add SECURITY.md and threat model - #54

Open
DibranMulder wants to merge 2 commits into
masterfrom
docs/security-policy
Open

docs: add SECURITY.md and threat model#54
DibranMulder wants to merge 2 commits into
masterfrom
docs/security-policy

Conversation

@DibranMulder

Copy link
Copy Markdown

Summary

  • Adds SECURITY.md with a private disclosure address, supported-versions statement, and explicit in-scope / out-of-scope lists.
  • Adds docs/threat-model.md describing assets, actors, in-scope guarantees, and operational guidance for issuer and wallet deployments.
  • Provides a citable position on issue Sensitive information is not protected #5: memory residency of secrets (key material in math/big.Int, no mlock, no deterministic zeroing) is documented as an out-of-scope risk that operators mitigate at the platform layer (swap off, coredumps off, encrypted key storage, ptrace restrictions). A memguard-style fix is rejected with rationale, because math/big allocates fresh backing arrays per operation and a top-level locked buffer would not protect the intermediate values.

Notes for reviewers

  • The reporting address used is security@privacybydesign.foundation. Please confirm this is the intended mailbox (or switch to GitHub's private vulnerability reporting) before merging.
  • No code changes; documentation only.

Document the disclosure process and define what gabi does and does not
defend against, with explicit operational guidance for issuer and wallet
deployments. Addresses the memory-residency concern raised in #5 by
scoping it as an out-of-scope, operator-mitigated risk.

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed against gabi's internals. The asset list maps correctly onto the real structs — gabikeys.PrivateKey does carry P, Q, PPrime, QPrime, ECDSA(String) plus internal N/Order, and CredentialBuilder does hold secret, vPrime, mUser etc. The keyproof, rangeproof, and revocation packages all exist. The rationale on why memguard isn't a useful fix for math/big-based arithmetic is correct and worth having on the record for issue #5.

A couple of small things inline. The main open question is the disclosure mailbox, which the author already flagged.

The text reads cleanly — no test or build impact since this is documentation only.

Comment thread SECURITY.md

## Reporting a vulnerability

Please report suspected vulnerabilities privately to

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As called out in the PR description: please get explicit confirmation that security@privacybydesign.foundation is monitored before merging, or switch to GitHub's private vulnerability reporting (Settings → Security → Private vulnerability reporting). A documented but unmonitored mailbox is worse than no policy, since reporters trust the address and walk away.

Comment thread SECURITY.md
issue for cryptographic soundness bugs, key-recovery attacks, or anything
you believe gives an advantage to a malicious issuer, prover, or verifier.

We aim to acknowledge reports within 5 working days and to publish a fix

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor: gabi itself doesn't really have a release cadence beyond what irmago pins. Worth being clear that "latest tagged release" in practice means "whatever irmago currently consumes" — otherwise downstream operators may pin an old gabi tag thinking it's still supported.

Comment thread docs/threat-model.md

| # | Asset | Where it lives | Impact if disclosed |
| --- | ------------------------------------------------------------------------------------------------------ | ----------------------------------------------------------- | -------------------------------------------------------------------------------------------- |
| A1 | Issuer private key (`PrivateKey.P`, `Q`, `PPrime`, `QPrime`, `Order`, `ECDSA`) | Issuer server process memory; on disk as XML | Attacker can mint arbitrary credentials — total scheme break for that issuer. |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small accuracy nit on A1: Order is an internal derived field (xml:"-") computed from PPrime/QPrime, not an independent stored secret. Worth either dropping it from the asset list or noting it as "derived from P/Q" — same for the implicit modulus N, which is public. Doesn't change the conclusion, just keeps the field list honest.

Comment thread docs/threat-model.md
not deterministically zeroed after use. A kernel info-leak (e.g.
Spectre-class side channels, `/proc/<pid>/mem`, swap file analysis,
hibernation image, coredump) can recover these values.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider naming crypto/rand here too (it's mentioned in SECURITY.md but not in the threat model itself). A reader skimming only this file shouldn't have to guess where randomness comes from.

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.

2 participants