diff options
Diffstat (limited to 'docs/first-review.md')
| -rw-r--r-- | docs/first-review.md | 372 |
1 files changed, 372 insertions, 0 deletions
diff --git a/docs/first-review.md b/docs/first-review.md new file mode 100644 index 0000000..fffac4c --- /dev/null +++ b/docs/first-review.md @@ -0,0 +1,372 @@ +# MeshBay — First Architecture & Security Review + +> Date: 2026-08-10 +> Scope: design-level review of the cryptographic architecture, trust model, and +> security properties as specified in draft v3 (archived in `old-draft.md`) and implemented +> through Phases 1-6 (81 tests, demo-v2 validated). +> +> This review does NOT assess the security of the demo/test deployment. It evaluates +> whether the architecture, as designed, provides a robust foundation for a secure +> decentralized platform. + +--- + +## Executive Summary + +The cryptographic architecture is **strong and well-designed**. The algorithm +choices are modern and correct, the trust model is sound, and the key hierarchy +is properly separated. The six POC spikes were genuinely useful — the jti fix +(Spike 3), the Argon2id recalibration (Spike 1), and the GEK wrapping protocol +confirmation (Spike 6) are exactly the kind of findings that save projects from +shipping real vulnerabilities. + +There are **no fatal design flaws**. The issues found are fixable before Phase 7, +and the most important one (Double Ratchet group model) should be resolved +before writing production chat code. + +Classification: **Critical** (must fix before production), **Significant** (design +gap, fix before Phase 8), **Minor** (improvement, can schedule), **Note** (observation, +no action required). + +--- + +## What Is Solid + +These design decisions are correct and represent genuine security engineering: + +**1. Hub-blind GEK wrapping (ECIES-like)** +The wrapping protocol (ephemeral X25519 + HKDF + ChaCha20-Poly1305 with AAD) is +textbook ECIES done right. The hub stores opaque blobs, the ephemeral keypair +ensures each wrapping produces different ciphertext, and the AAD binding to +`pk_recipient` prevents bundle swapping attacks. This is the most important +crypto decision in the system and it's correct. + +**2. Ed25519 identity verification independent of TLS** +Nodes use self-signed TLS certs for transport confidentiality only. Client +verifies the node's Ed25519 public key (from hub) at the MNP handshake layer. +This decouples transport security from identity — the right design for a system +where nodes can't get CA-signed certificates. + +**3. Mandatory jti in JWT** +The Spike 3 finding was critical. Ed25519 is deterministic — without jti, two +tokens issued in the same second are bit-identical. Adding UUID4 jti to every +token was the correct fix. The architecture now enables per-token revocation. + +**4. On-the-fly encryption model** +Files stored in plaintext on the node, encrypted at read time with per-chunk keys +derived from the GEK via HKDF. This avoids the double-storage problem +(encrypted + plaintext) and makes GEK rotation feasible without re-encrypting +terabytes on disk. + +**5. Domain separation in HKDF** +Every key derivation uses a distinct `info` string (`meshbay:gek_wrap:v1`, +`meshbay:ratchet:root:v1`, etc.). The AES variant adds `:aes` suffix to chunk +key derivation. This is a small detail that prevents cross-protocol key reuse +and shows mature crypto engineering. + +**6. Transport abstraction layer** +The `Transport` interface allowing TCP+TLS v1 → QUIC v2 migration without +protocol changes was a good architectural decision confirmed by the successful +demo-v2 QUIC validation. + +**7. Refresh token stored as blake3 hash** +Server never stores the raw refresh token — only its hash. Correct pattern. +Database breach doesn't leak usable refresh tokens. + +--- + +## Critical Issues + +### C1 — Double Ratchet is not suitable for group chat as described + +**Location:** `meshbay_common/ratchet.py`, draft v3 section 6.6 + +**Problem:** The draft states "all members share the same ratchet state seeded +from the group GEK." The Signal Double Ratchet is designed for **pairwise** +(1:1) communication. It fundamentally cannot work as a shared group state: + +- If all N members share a single ratchet state, each member advancing the + sending chain desynchronizes all other members. Message 5 from Alice and + message 5 from Bob would use the same chain key, producing a nonce/key reuse + — a catastrophic failure for ChaCha20-Poly1305 and AES-GCM. +- The current `RatchetState` class has one `CKs` (sending chain) and one `CKr` + (receiving chain), confirming it's a pairwise protocol. + +**What Signal actually does for groups:** Signal uses a different protocol called +**Sender Keys** (described in their "Group Protocol" specification). Each member +has their own symmetric sending chain key. When a member joins a group, all +existing members send their current sender key to the new member via pairwise +Double Ratchet channels. This gives forward secrecy per member, not per message. + +**Impact:** If implemented as described, group chat will either: +- Silently corrupt messages (if state is truly shared), or +- Require N*(N-1)/2 pairwise ratchet sessions (O(N^2) state, impractical for + groups > 10 members) + +**Recommendation:** Before Phase 7.5 (chat), decide between: +1. **Sender Keys** (Signal Groups approach): each member maintains one symmetric + sending chain. Forward secrecy at member rotation granularity. O(N) state. + Simpler to implement, good enough for most threat models. +2. **Pairwise Double Ratchet**: keep the current implementation but use it for + 1:1 messages only. Group messages would be encrypted N-1 times. O(N^2) cost + per message — only feasible for small groups (<20). +3. **MLS (Message Layer Security, RFC 9420)**: the modern standard for group + messaging. Tree-based ratcheting, O(log N) state and messages. More complex + but future-proof. Python implementations exist (`openmls` bindings, or + `mls-protocol`). + +Recommendation: **Sender Keys** for v1 (pragmatic, Signal-proven), with the +option to migrate to MLS later if group sizes grow. + +### C2 — No group membership verification in MNP handshake + +**Location:** `quic_server.py:125-146`, `server.py:130-151` + +**Problem:** The MNP handshake verifies the JWT signature and expiration, but does +NOT check whether the authenticated user is a member of the group being accessed. +Any valid JWT holder can request any file from any group served by the node. + +The draft says the JWT carries "hub-signed groups membership claim" (section 4.1.4), +but the actual `issue_access_token()` in `auth.py:104-125` does not include any +group membership claims. The JWT contains only `sub`, `pk_user`, `hub_id`, `jti`, +`iat`, `exp`. + +**Impact in Phase 7 (multi-group):** A user authenticated for group A can request +files from group B on the same node. Since all groups share one QUIC port, this +becomes an authorization bypass. + +**Recommendation:** +- Add group membership claims to the JWT: `"groups": ["group_id_1", "group_id_2"]` +- Node verifies the requested group_id is in the JWT's groups claim +- This is a simple change to `issue_access_token()` + handshake verification +- The JWT is already verified offline with the hub's Ed25519 key — adding claims + doesn't change the verification flow + +--- + +## Significant Issues + +### S1 — Admin revocation endpoint has no authorization check + +**Location:** `revocation.py:149-194` + +**Problem:** The `admin_revoke` endpoint requires authentication (`get_current_user`) +but does NOT verify that the current user is a hub admin. Any authenticated user +can revoke any other user or any group. The docstring says "Admin only (user must +be hub admin — user_id in config)" but no such check is implemented. + +**Impact:** Any registered user can revoke any other user or group on the hub. +This is a privilege escalation vulnerability. + +**Recommendation:** Phase 8 plans admin roles (8.1: `hub_admin` flag on User). This +check must be added before the revocation endpoint is used in any non-demo context. +For now, the endpoint exists but is only callable by someone who knows the API — +acceptable for a test deployment, not for production. + +### S2 — Email stored in plaintext in the database + +**Location:** `models.py:42`, draft v3 section 4.1.1 + +**Problem:** The spec says "Email and phone are stored encrypted at rest in the +database." The actual `User` model stores email as `String(256)` — plaintext. +A database breach would expose all user emails. + +**Recommendation:** Encrypt email (and future phone field) with a server-side key +derived from a secret not stored in the database (e.g., from the hub config file). +Use AES-256-GCM with a deterministic IV derived from user_id (for lookups) or +accept that encrypted email cannot be searched by value. + +### S3 — No jti denylist distribution to nodes + +**Location:** draft v3 section 4.1.4, open question #9 + +**Problem:** The architecture describes a jti denylist for immediate token revocation, +but: +- The hub has no `GET /v1/revoke/denylist` endpoint (marked [TBD]) +- Nodes don't check any denylist during JWT verification +- The revocation WebSocket pushes revocation tokens to nodes, but nodes don't + persist or check them during MNP handshake + +**Impact:** A revoked user's JWT remains valid for up to 1 hour (until natural +expiration). The revocation WebSocket can close active connections, but new +connections with the same JWT will succeed. + +**Recommendation:** Two options: +1. **Push + local cache** (recommended): when the node receives a revocation via + WebSocket, it adds the jti to an in-memory set. MNP handshake checks this set. + Simple, real-time, no polling. +2. **Pull**: node periodically fetches the denylist from the hub. Adds latency + between revocation and enforcement. + +Option 1 is simpler and already half-built (the WebSocket channel exists). + +### S4 — AES-GCM keystore uses non-standard 128-bit IV + +**Location:** `crypto.py:148` — `iv = os.urandom(16)` + +**Problem:** AES-GCM is specified for 96-bit (12-byte) nonces (NIST SP 800-38D). +The keystore encryption uses a 128-bit (16-byte) IV. The `cryptography` library +accepts this and processes it through GHASH to derive the internal counter, which +is secure — but it's a deviation from the standard. + +**Impact:** No direct vulnerability. AES-GCM with >96-bit IVs has a slightly +different security proof (birthday bound applies to the GHASH reduction). For a +keystore that's encrypted once and rarely re-encrypted, the practical risk is zero. + +**Recommendation:** Change to `os.urandom(12)` for standard compliance. Simple +one-line fix. The existing keystore files would need re-encryption on next save +(which happens naturally when the user updates their keystore). + +### S5 — Refresh token not rotated on use + +**Location:** `users.py:158-179` + +**Problem:** When a refresh token is used to obtain a new access token, the same +refresh token remains valid. If an attacker intercepts a refresh token, they can +use it repeatedly alongside the legitimate user, and neither party detects the +theft. + +**Recommendation:** Implement refresh token rotation: each use of a refresh token +issues a new refresh token and invalidates the old one. If the old token is used +again (by the attacker), the hub detects the reuse and revokes all tokens for +that user (indicating theft). This is the OAuth 2.0 Security BCP recommendation +(RFC 6819, section 5.2.2.3). + +--- + +## Minor Issues + +### M1 — Username enumeration via registration and pubkeys endpoints + +The registration endpoint returns "Username already taken" (409), and +`GET /v1/users/{username}/pubkeys` returns 404 vs a valid response. Both allow +enumerating valid usernames. For a decentralized platform where users have public +identities, this may be acceptable by design, but it should be a conscious +decision. + +### M2 — TLS self-signed certificate uses RSA-2048 + +**Location:** `tls_cert.py:36` + +The TLS cert uses RSA-2048 while the rest of the system uses Ed25519. Since the +cert is only for transport confidentiality (identity is verified via Ed25519), +this is acceptable. However, using an Ed25519 TLS certificate would be more +consistent and is supported by modern TLS 1.3 stacks. RSA-2048 is ~112-bit +security; Ed25519 is ~128-bit. + +### M3 — No rate limiting on GEK retrieval and pubkeys endpoints + +Only `/register` and `/login` have rate limiting. An attacker could enumerate +pubkeys or attempt to retrieve GEK bundles at high frequency. While GEK bundles +are opaque (no direct attack), rate limiting on all authenticated endpoints is +good hygiene. + +### M4 — Single admin per group with no delegation or recovery + +If the admin's node goes offline, the group becomes inaccessible: no new members +can be added, no GEK rotation, no moderation. There's no mechanism for admin +delegation or recovery. For a personal file-sharing platform this may be +acceptable, but for any group with more than a few members, this is a +single-point-of-failure. + +### M5 — Chunk key derivation uses HKDF salt=None + +**Location:** `crypto.py:46-51` + +The code uses `salt=None` and puts the file context in `info`. This is actually +correct HKDF usage (salt is for randomizing extraction when IKM might be +non-uniform; GEK is from CSPRNG so salt isn't needed; info is for domain +separation). However, the draft v3 spec describes it as using `salt`, which +creates a spec/code discrepancy. Update the spec to match the code, since the +code is correct. + +### M6 — Argon2id production parameters not yet applied + +**Location:** `crypto.py:131-133`, `auth.py:24-26`, `keyderive.py:33-35` + +All three Argon2id usage sites still use the dev parameters (iterations=3, +memory=64MB, ~78ms). Production target is iterations=4, memory=256MB, ~500ms. +Phase 7.7 plans a calibration CLI command. This must be done before any +real-world deployment. The comments document this correctly. + +--- + +## Notes (No Action Required) + +### N1 — Forward secrecy model is appropriate + +File encryption uses GEK-derived symmetric keys — no forward secrecy at the +application layer. If GEK is compromised, past files are decryptable. This is +documented and accepted: the alternative (per-session file encryption keys) +would break seeking, caching, and multi-source download. The transport layer +(TLS 1.3 / QUIC) provides forward secrecy for data in transit. + +### N2 — Error messages in login are correct + +`login()` returns the same "Invalid credentials" for both user-not-found and +wrong-password. This is the correct behavior to prevent user enumeration through +the login flow (even though registration and pubkeys endpoints allow it — see M1). + +### N3 — Hub legal exposure model is well-positioned + +The hub stores no content, no metadata, no node IPs (beyond ephemeral signaling). +GEK bundles are opaque. The hub's legal exposure is analogous to a domain +registrar or email provider — it knows who registered but not what they share. +LCEN/DSA compliance is addressed through IP logging with 1-year retention. + +### N4 — QUIC NAT probe content is fine + +`punch_nat()` sends `b'MESHBAY:NAT:PUNCH'` as a fixed probe. Some NAT +implementations might filter constant payloads, but in practice this works +(demo-v2 confirmed). The content of the probe packet doesn't matter for NAT +entry creation — only the 5-tuple (src_ip, src_port, dst_ip, dst_port, proto) +matters. + +### N5 — Web/CLI key derivation mismatch is by design + +Strategy A (Argon2id) and Strategy B (PBKDF2-SHA512 in browser) produce +different keys from the same password. The code and docs correctly explain this: +users pick one registration path. The web client uses random keypairs stored +encrypted on the hub, not password-derived keys. This avoids the mismatch +entirely. + +--- + +## Prioritized Action Plan + +| # | Issue | Severity | When to fix | +|---|---|---|---| +| C1 | Double Ratchet group model | Critical | Before Phase 7.5 (chat) | +| C2 | No group membership in JWT/handshake | Critical | Phase 7.3 (multi-group) | +| S1 | Admin revocation has no authz check | Significant | Phase 8.1 (admin roles) | +| S2 | Email stored in plaintext | Significant | Phase 8 | +| S3 | No jti denylist on nodes | Significant | Phase 7.2 (signaling) | +| S4 | AES-GCM 128-bit IV | Significant | Any time (1 line) | +| S5 | Refresh token rotation | Significant | Phase 8 | +| M1 | Username enumeration | Minor | Accept or Phase 8 | +| M2 | RSA-2048 TLS cert | Minor | Phase 7 or later | +| M3 | Rate limiting gaps | Minor | Phase 8.6 | +| M4 | Single admin SPOF | Minor | Phase 8+ | +| M5 | Spec/code HKDF discrepancy | Minor | Update spec | +| M6 | Argon2id prod params | Minor | Phase 7.7 | + +--- + +## Conclusion + +MeshBay's security architecture is built on solid foundations. The cryptographic +primitive choices are modern and correct. The trust model (hub-blind, node-hosted, +E2E encrypted) is well-designed and consistently applied. The POC spikes caught +real issues (jti, Argon2id calibration, NAT behavior) that would have been +difficult to fix post-deployment. + +The two critical issues (C1: group ratchet model, C2: group membership +authorization) are both design decisions that need to be made before Phase 7 +produces production chat and multi-group code. They are not retroactive problems +— they are forward-looking decisions that the architecture leaves room for. + +The significant issues (S1-S5) are implementation gaps that should be addressed +during Phases 7-8, in the natural course of hardening the hub and node. + +Overall assessment: **good foundations, ready for Phase 7** after deciding the +group chat encryption model (C1) and adding group claims to the JWT (C2). |