core/go-crypt
8
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions
go-crypt/FINDINGS.md
Snider 9331fc6eac test(phase0): expand test coverage, security audit, and benchmarks 
Add 29 new tests across auth/, crypt/, and trust/ packages:
- auth: concurrent sessions, token uniqueness, challenge expiry boundary,
  empty password, long/unicode usernames, air-gapped round-trip, expired refresh
- crypt: wrong passphrase, empty/large plaintext, KDF determinism, HKDF info
  separation, checksum edge cases
- trust: concurrent registry operations, tier validation, token expiry boundary,
  empty ScopedRepos behaviour, unknown capabilities

Add benchmark suites:
- crypt: Argon2, ChaCha20, AES-GCM, HMAC (1KB/1MB payloads)
- trust: PolicyEvaluate (100 agents), RegistryGet, RegistryRegister

Security audit documented in FINDINGS.md:
- F1: LTHN hash used for password verification (medium)
- F2: PGP private keys not zeroed after use (low, upstream limitation)
- F3: Empty ScopedRepos bypasses repo scope check (medium)
- F4: go vet clean, no math/rand, no secrets in error messages

All tests pass with -race. go vet clean.

Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2026-02-20 01:14:41 +00:00

151 lines
8.8 KiB
Markdown
Raw Blame History

# FINDINGS.md — go-crypt Research & Discovery
## 2026-02-20: Initial Analysis (Virgil)
### Origin
Extracted from `core/go` on 16 Feb 2026 (commit `8498ecf`). Single extraction commit — fresh repo with no prior history.
### Package Inventory
| Package | Source LOC | Test LOC | Test Count | Notes |
|---------|-----------|----------|-----------|-------|
| `auth/` | 455 | 581 | 25+ | OpenPGP challenge-response + LTHN password |
| `crypt/` | 90 | 45 | 4 | High-level encrypt/decrypt convenience |
| `crypt/kdf.go` | 60 | 56 | — | Argon2id, scrypt, HKDF |
| `crypt/symmetric.go` | 100 | 55 | — | ChaCha20-Poly1305, AES-256-GCM |
| `crypt/hash.go` | 89 | 50 | — | Argon2id password hashing, Bcrypt |
| `crypt/hmac.go` | 30 | 40 | — | HMAC-SHA256/512 |
| `crypt/checksum.go` | 55 | 23 | — | SHA-256/512 file and data checksums |
| `crypt/chachapoly/` | 50 | 114 | 9 | Standalone ChaCha20-Poly1305 wrapper |
| `crypt/lthn/` | 94 | 66 | 6 | RFC-0004 quasi-salted hash |
| `crypt/pgp/` | 230 | 164 | 11 | OpenPGP via ProtonMail go-crypto |
| `crypt/rsa/` | 91 | 101 | 3 | RSA OAEP-SHA256 |
| `crypt/openpgp/` | 191 | 43 | — | Service wrapper, core.Crypt interface |
| `trust/` | 165 | 164 | — | Agent registry, tier management |
| `trust/policy.go` | 238 | 268 | 40+ | Policy engine, 9 capabilities |
**Total**: ~1,938 source LOC, ~1,770 test LOC (47.7% test ratio)
### Dependencies
- `forge.lthn.ai/core/go` — core.E error handling, core.Crypt interface, io.Medium storage
- `github.com/ProtonMail/go-crypto` v1.3.0 — OpenPGP (replaces deprecated golang.org/x/crypto/openpgp)
- `golang.org/x/crypto` v0.48.0 — Argon2, ChaCha20-Poly1305, scrypt, HKDF, bcrypt
- `github.com/cloudflare/circl` v1.6.3 — indirect (elliptic curve via ProtonMail)
### Key Observations
1. **Dual ChaCha20 wrappers** — `crypt/symmetric.go` and `crypt/chachapoly/chachapoly.go` implement nearly identical ChaCha20-Poly1305. The chachapoly sub-package pre-allocates nonce+plaintext capacity (minor optimisation). Consider consolidating.
2. **LTHN hash is NOT constant-time** — `lthn.Verify()` uses direct string comparison (`==`), not `subtle.ConstantTimeCompare`. This is acceptable since LTHN is for content IDs, not passwords — but should be documented clearly.
3. **OpenPGP service has IPC handler** — `openpgp.Service.HandleIPCEvents()` dispatches `"openpgp.create_key_pair"`. This is the only IPC-aware component in go-crypt.
4. **Trust policy decisions are advisory** — `PolicyEngine.Evaluate()` returns `NeedsApproval` but there's no approval queue or workflow. The enforcement layer is expected to live in a higher-level package (go-agentic or go-scm).
5. **Session tokens are in-memory** — No persistence. Suitable for development and single-process deployments, but not distributed systems or crash recovery.
6. **Test naming follows `_Good`/`_Bad`/`_Ugly` pattern** — Consistent with core/go conventions.
### Integration Points
- **go-p2p** → UEPS layer will need crypt/ for consent-gated encryption
- **go-scm** → AgentCI trusts agents via trust/ policy engine
- **go-agentic** → Agent session management via auth/
- **core/go** → OpenPGP service registered via core.Crypt interface
### Security Review Flags
- Argon2id parameters (64MB/3/4) are within OWASP recommended range
- RSA minimum 2048-bit enforced at key generation
- ChaCha20 nonces are 24-byte (XChaCha20-Poly1305), not 12-byte — good, avoids nonce reuse risk
- PGP uses ProtonMail fork (actively maintained, post-quantum research)
- No detected use of `math/rand` — all randomness from `crypto/rand`
---
## Security Audit (Phase 0)
Conducted 2026-02-20. All source files reviewed for cryptographic hygiene.
### 1. Constant-Time Comparisons
| Location | Comparison | Verdict |
|----------|-----------|---------|
| `crypt/hash.go:66` | `subtle.ConstantTimeCompare(computedHash, expectedHash)` | PASS — Argon2id password verification uses constant-time compare |
| `crypt/hmac.go:29` | `hmac.Equal(mac, expected.Sum(nil))` | PASS — HMAC verification uses constant-time compare |
| `crypt/lthn/lthn.go:93` | `Hash(input) == hash` | ACCEPTABLE — LTHN is for content IDs, not passwords. Documented in CLAUDE.md. |
| `auth/auth.go:282` | `a.sessions[token]` | ACCEPTABLE — Map lookup by token as key. 64-hex-char token (256-bit entropy) makes brute-force timing attacks infeasible. |
| `auth/auth.go:387` | `lthn.Verify(password, storedHash)` | **FINDING** — Password verification uses LTHN hash with non-constant-time `==`. See Finding F1 below. |
### 2. Nonce/Randomness Generation
All nonce and random value generation uses `crypto/rand`:
| Location | Purpose | Entropy |
|----------|---------|---------|
| `auth/auth.go:218` | Challenge nonce | 32 bytes (256-bit) via `crypto/rand.Read` |
| `auth/auth.go:439` | Session token | 32 bytes (256-bit) via `crypto/rand.Read` |
| `crypt/kdf.go:55` | Salt generation | 16 bytes (128-bit) via `crypto/rand.Read` |
| `crypt/symmetric.go:22` | ChaCha20 nonce | 24 bytes via `crypto/rand.Read` |
| `crypt/symmetric.go:67` | AES-GCM nonce | 12 bytes via `crypto/rand.Read` |
| `crypt/rsa/rsa.go:25` | RSA key generation | `crypto/rand.Reader` |
**No usage of `math/rand` detected anywhere in the codebase.** PASS.
### 3. PGP Private Key Handling
**FINDING F2**: PGP private key material is NOT zeroed after use. In `pgp.Decrypt()` and `pgp.Sign()`, the private key is decrypted into memory (via `entity.PrivateKey.Decrypt()`) but the decrypted key material remains in memory until garbage collected. The ProtonMail go-crypto library does not provide a `Wipe()` or `Zero()` method on `packet.PrivateKey`, so this is currently a limitation of the upstream library rather than a code defect. Mitigating this would require forking or patching go-crypto.
**Severity**: Low. The Go runtime does not guarantee memory zeroing, and GC-managed languages inherently have this limitation. In practice, an attacker who can read process memory already has full access.
### 4. Error Message Review
No secrets (passwords, tokens, private keys, nonces) leak in error messages. All error strings are generic:
- `"user not found"`, `"invalid password"`, `"session not found"`, `"session expired"`
- `"failed to decrypt"`, `"failed to encrypt"`, `"challenge expired"`
- `"ciphertext too short"`, `"failed to generate nonce"`
The `trust.Register` error includes the agent name (`"invalid tier %d for agent %q"`) which is acceptable — agent names are not secrets.
PASS.
### 5. Session Token Security
- **Entropy**: 32 bytes from `crypto/rand` → 256-bit. Well above the 128-bit minimum.
- **Format**: Hex-encoded → 64-character string. No structural information leaked.
- **Storage**: In-memory `map[string]*Session` behind `sync.RWMutex`.
- **Expiry**: Checked on every `ValidateSession` and `RefreshSession` call. Expired sessions are deleted on access.
PASS.
### Findings
#### F1: LTHN Hash Used for Password Verification (Medium Severity)
`auth.Login()` verifies passwords via `lthn.Verify()` which uses the LTHN quasi-salted hash (RFC-0004) with a non-constant-time string comparison (`==`). LTHN was designed for content identifiers, NOT passwords.
**Impact**: The LTHN hash is deterministic (same input always produces same output) with no random salt. While the quasi-salt derivation adds entropy, it provides weaker protection than Argon2id (`crypt.HashPassword`/`crypt.VerifyPassword` which is available but unused here).
**Timing risk**: The `==` comparison in `lthn.Verify` could theoretically leak information through timing side-channels, though the practical impact is limited because:
1. The comparison is on SHA-256 hex digests (fixed 64 chars)
2. An attacker would need to hash candidate passwords through the LTHN algorithm first
**Recommendation**: Consider migrating password storage from LTHN to Argon2id (`crypt.HashPassword`/`crypt.VerifyPassword`) in a future phase. This would add random salting and constant-time comparison.
#### F2: PGP Private Keys Not Zeroed After Use (Low Severity)
See Section 3 above. Upstream limitation of ProtonMail go-crypto.
#### F3: Trust Policy — Empty ScopedRepos Bypasses Scope Check (Medium Severity)
In `policy.go:122`, the repo scope check is: `if isRepoScoped(cap) && len(agent.ScopedRepos) > 0`. This means a Tier 2 agent with empty `ScopedRepos` (either `nil` or `[]string{}`) is treated as "unrestricted" rather than "no access".
**Impact**: If an admin creates a Tier 2 agent without explicitly setting `ScopedRepos`, the agent gets access to ALL repositories for repo-scoped capabilities (`repo.push`, `pr.create`, `pr.merge`, `secrets.read`).
**Recommendation**: Consider treating empty `ScopedRepos` as "no access" for Tier 2 agents, or requiring explicit `ScopedRepos: []string{"*"}` for unrestricted access. This is a design decision for Phase 3.
#### F4: `go vet` Clean
`go vet ./...` produces no warnings. PASS.
Reference in a new issue View git blame Copy permalink