diff options
Diffstat (limited to 'docs/first-review.md')
| -rw-r--r-- | docs/first-review.md | 382 |
1 files changed, 0 insertions, 382 deletions
diff --git a/docs/first-review.md b/docs/first-review.md deleted file mode 100644 index 17d5b36..0000000 --- a/docs/first-review.md +++ /dev/null @@ -1,382 +0,0 @@ -# MeshBay — First Architecture & Security Review - -> **Superseded by `MESHBAY_DESIGN.md`.** This was the first security review; its design -> content now lives in §13.1, and the invariants each finding names throughout. -> -> It is kept because code comments, tests and other documents cite its -> sections and its labels, and because it records reasoning a synthesis -> compresses. **Where it disagrees with `MESHBAY_DESIGN.md`, the design -> document is right; where either disagrees with the code, the code is.** -> `MESHBAY_DESIGN.md` §16 maps every section reference here onto its -> replacement, and §13 defines every label. - -> 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). |