Internal review — finding triage
This file tracks the status of every internal review finding through to remediation or accepted-as-is. Findings are addressed in priority order (critical > high > medium > low). Informational findings may be addressed or accepted as-documented.
Status legend:
- Open — finding stands; no fix landed yet.
- Open, Phase 3 prerequisite — must be addressed before the PQ-hybrid default ships in Phase 3.
- In progress — fix PR exists.
- Fixed — fix landed; PR linked.
- Accepted — no fix planned; rationale recorded.
- Document — to be reflected in the spec / public docs only.
Findings
| Finding | Task | Severity | Status | Component | Recommended fix | PR |
|---|---|---|---|---|---|---|
| 1.1 — Spec prose contradicts AAD definition (file_id not in AAD) | 1 | Medium | Fixed | spec | Fix prose + add AAD test vector | crypto#1 |
1.2 — HKDF salt="" ambiguity in nonce-prefix derivation | 1 | Low | Fixed | spec + impl | Tighten spec to salt=zeros(32) + add nonce-prefix test vector. See 1.2 severity recalibration below. | crypto#1 |
1.3 — Spec uses redundant truncateN(...)[0..k] notation | 1 | Low | Fixed | spec | Replace with HKDF(..., L=k+1) directly | crypto#1 |
1.4a — AES-GCM-v1 decryptBlob parses suite_payload before MAC verify | 1 | Low | Fixed | impl | Swap call order in aes-gcm-v1/api.ts:208-212 | crypto#2 |
1.4b — XChaCha-v1 decryptBlob parses suite_payload before MAC verify | 1 | Low | Fixed | impl | Swap call order in xchacha-v1/api.ts:177-178; bundle PR with 1.4a | crypto#2 |
| 1.4c — Spec contradiction: header § MUST is unimplementable for PQ-hybrid | 1 | Medium | Fixed | spec | Scope header § MUST to non-KEM suites; add CCA-hygiene security argument to § "0x03" | crypto#3 |
1.5 — validateHeaderInputs lacks (totalChunks × chunkSize, plaintextSize) cross-field check | 1 | Low | Fixed | impl | Replicate parseHeader's invariant, ideally via shared helper | crypto#2 |
1.6 — Web dispatcher silently defaults cipherVersion to v0 | 1 | Low | Fixed | impl | Remove ?? 1 default; Stage 2c is shipped (PRs #115/#116/#117) | web#123 |
| 1.7 — Worker fileId byte comparison is non-constant-time | 1 | Informational | Accepted | impl | None; value is already public via cleartext header. Optional refactor to constantTimeEqual for code-style consistency | — |
| 1.8 — Spec underspecifies suite_payload zero-fill convention for vault-stored wrapped keys | 1 | Low | Fixed | spec | Spec to state "all 72/96/N bytes are zero when wrapped key is stored out-of-band" | crypto#1 |
| 1.9 — V0 trailing-bytes check rejects exactly-tag-size remainder | 1 | Informational | Accepted | impl | None; defense-in-depth, optional one-liner in v0 spec | — |
| 1.10 — Versioning policy override path not exposed | 1 | Informational | Accepted | impl | None; intentional safer-default. Revisit if/when a minor-version bump ships | — |
| 1.11 — V0/V1 isolation is structural (positive finding) | 1 | Informational | Fixed | spec | Add to format-v1.md § "Compatibility with v0" so future contributors don't add cross-format auto-detection | crypto#1 |
| 2.1 — Web app uses PBKDF2-SHA-256 instead of Argon2id for vault-key wrap | 2 | Medium | Fixed | impl | Migrate vaultKeyClient to Argon2id MODERATE; additive via existing kdf column; new accounts immediately, legacy on next password change | web#126 |
| 2.2 — Web app flat envelope scheme vs spec's HKDF hierarchy; rk used directly in 4 contexts | 2 | Medium | Open | spec + impl | Option A preferred; see doc for Option A vs B decision + spec intent question | — |
| 2.3 — Argon2id INTERACTIVE used on high-entropy inputs in metadata-key derivation | 2 | Low | Open | impl | Split metadata-key derivation by input type: HKDF for folder-keyed paths (high-entropy), keep Argon2id for share-password path (low-entropy). Pairs with 2.2 Option A. | — |
| 2.4 — Spec underspecifies ML-KEM-1024 seed (d, z) split convention | 2 | Low | Fixed | spec | Add one paragraph documenting d = seed[0..32], z = seed[32..64] convention + add test vector | crypto#3 |
| 2.5 — Two HKDF info strings constructed inline, contradicting library's own invariant | 2 | Low | Fixed | impl | Add ML_KEM_1024_SEED and ARGON2ID_SALT_COMPRESSION to HKDF_INFO constants in crypto/src/internal/types.ts; update the two call sites to reference them | crypto#1 |
2.6 — Spec's "Forbidden cross-context usage" list omits shieldfive/v1/argon2id/salt-compression | 2 | Low | Fixed | spec | Add to spec's forbidden-list block; pairs with 2.5 | crypto#1 |
| 2.7 — Spec doesn't document intentional XChaCha info-string reuse in PQ-hybrid | 2 | Informational | Fixed | spec | Replace ambiguous paragraph in spec § "Per-file key derivation" with explicit reuse-is-safe argument (IKM space partitioned) | crypto#1 |
| 2.8 — Salt-absent ambiguity inherits Task 1 Finding 1.2 into key-derivation.md | 2 | Informational | Fixed | spec | Bundle with 1.2 fix: same "salt absent = zeros(32) per RFC 5869" disambiguation, applied to key-derivation.md preamble | crypto#1 |
| 3.1 — A2 splice prose says "file_id AAD" but cross-file binding is HKDF salt | 3 | Medium | Fixed | spec | Replace bullet; cross-link format-v1.md § "Suite payloads" and crypto#1 test vector. Mirrors Task 1 Finding 1.1 in the threat-model doc | crypto#4 |
| 3.2 — A3 documented via suite 0x03 but web client emits classical 0x01 | 3 | Medium | Fixed | spec | Add "Current deployment status" subsection under A3 + comparison-table footnote. Web wire-up of suite 0x03 tracked separately as Phase 3 work | crypto#4 |
| 3.3 — Threat model presents v1 AEAD invariants without v0 carve-out | 3 | Medium | Fixed | spec | Add "Legacy v0 data" subsection under A2 referencing format-v0.md § "Limitations of v0" | crypto#4 |
| 3.4 — Recovery-key compromise not in adversary model | 3 | Low | Fixed | spec | Add "Recovery-key compromise" subsection under "Out of scope" | crypto#4 |
| 3.5 — Share-link recipients not enumerated as a partial-access party | 3 | Low | Fixed | spec | New "Trust principals" section with "Share-link recipient" subsection | crypto#4 |
| 3.6 — Metadata leakage list understates what the deployed schema exposes | 3 | Low | Fixed | spec | Replace paragraph with non-exhaustive list (name_hash, per-share viewer geo, hashed IP, lockout state, download counts, last_active_at) | crypto#4 |
| 3.7 — TLS termination boundary implicit in A2 "Cannot break TLS" | 3 | Informational | Fixed | spec | Append edge-termination paragraph to A2; AEAD layer is end-to-end client-to-blob, so an attacker controlling TLS termination is effectively A1 | crypto#4 |
| 5.1 — initDecryptV1 accepts non-AES-GCM-v1 suites without explicit refusal | 5 | Low | Fixed | impl | Add explicit parsed.suite !== SUITE.AES_256_GCM_V1 reject between verifyHeaderMac and createAesGcmV1DecryptStream | web#133 |
| 5.4 — processEncryptV1/processDecryptV1 lack index bound check | 5 | Low | Fixed | impl | Reject state.totalChunks === 0 || index >= state.totalChunks at the top of each process function | web#133 |
| 6.2 — verifyAesGcmV1Proof accepts chunk_0 length in structurally-invalid [1, 16] range | 6 | Low | Fixed | impl | Reject chunkZeroLength < 1 + AES_GCM_TAG_BYTES between the zero-reject and upper-bound check | web#133 |
Notes
1.2 severity recalibration
Severity recalibrated Medium→Low in PR #1: the original finding claimed
salt=Uint8Array(0) and salt=zeros(32) produce different HKDF-Extract
outputs for HMAC-SHA-256. This is false — HMAC zero-pads keys shorter
than the 64-byte block size, so the two are computationally equivalent.
Spec edit retained for clarity; canonical form is zeros(32) per RFC
5869, but the two are equivalent for this construction.