antirez / antirez/ds4

Follow-up hardening / design items from a security review

Abierto
#549 0 comentarios 0 reacciones 0 asignados Ver en GitHub
Lenguaje dominante
C
Estrellas
22.3k
Forks
2.1k
Merge medio
1 d 3 h
PR fusionados (30 d)
4

Descripción

Hi Antirez — thanks again for the go-ahead to open things here.

I emailed you a coordinated security report on ds4. The memory-corruption and DoS items from it are now individual PRs (#539–#548, one per fix, each verified under ASan/UBSan or compile-checked). This issue tracks the **remaining lower-severity items**, where the right fix is a judgment call I'd rather leave to you than force through a PR. Happy to open a focused PR for any of these once you've picked a direction.

### Hardening (behavior/robustness)

1. **HTTP request framing leniency** (`ds4_server.c`, `header_end` / `content_length`). A bare `\n\n` terminator is accepted alongside `\r\n\r\n`; of duplicate `Content-Length`, the first is honoured; `Transfer-Encoding: chunked` is ignored. Behind a non-normalizing intermediary these are the classic request-smuggling/desync ingredients. Tightening the parser is a behavior change (could reject lenient clients), so it's your call — flagging it rather than patching it unilaterally.

2. **Tokenizer token-count vs model vocab** (`ds4.c`, `vocab_load`). `vocab->n_vocab` is taken from the `tokenizer.ggml.tokens` array length with no check that it matches `DS4_N_VOCAB`. The config path already enforces `deepseek4.vocab_size == DS4_N_VOCAB`, but the token *array* length is independent, so a GGUF can pass that config check yet ship a shorter token array. Two diagnostic paths (`--first-token-test` and the native blk.0 slice printer) then index the token array with logit-derived indices up to `DS4_N_VOCAB` — an out-of-bounds read. Suggested direction: require `tokens.len == DS4_N_VOCAB` in `vocab_load` (mirroring the existing config check), or bound the diagnostic indexing to `n_vocab`. I didn't send a PR because the exact invariant / load-ordering is better confirmed by you.

3. **Unsalted tokenizer hash** (`ds4.c`, `hash_bytes` / `table_*`). FNV-1a, linear probing, no resize and no per-process salt, so a hostile GGUF vocab can be precomputed offline to collide many tokens into one bucket (identical on every deployment). Low severity — it rides the same "attacker already supplies the GGUF vocab" precondition as the model-file findings, and normal loads bound insertions by `n_vocab`. Reported for completeness; a per-process salt would close it if you think it's worth it.

### Design-level (your call on the approach)

These are the design-level findings from the emailed report — each sits behind an operator choice (agent write tools enabled, clustering turned on and reachable), so they're confinement/hardening gaps rather than default-on vulnerabilities. I'm listing them here for visibility but deliberately keeping specifics in the private report:

4. **Agent write/edit tools follow symlinks** (`ds4_agent.c`) — no `realpath`/`O_NOFOLLOW` / workspace-confinement check, so a symlink inside the intended workspace redirects a write outside it. Direction: resolve and reject out-of-workspace targets, or an explicit `--allow-symlinks` opt-in.
5. **Distributed WORK routing dials a peer-supplied host/port** (`ds4_distributed.c`) — structure is validated but the destination isn't, so a worker can be steered to connect to internal addresses (SSRF-shaped pivot). Direction: allow-list/CIDR of known cluster addresses, or authenticate the distributed protocol.
6. **Unbounded per-session KV in distributed workers** (`ds4_distributed.c`, `dist_worker_get_session_locked`) — distinct peer `session_id`s create unbounded context-sized sessions with no cap/eviction. Direction: a session cap + eviction.
7. **CDP browser automation** (`ds4_web.c`) launches Chrome with `--remote-allow-origins=*`, disabling the DevTools-protocol origin check. Direction: scope the allowed origin.

(For completeness: I left the distributed listener's opt-in receive timeout alone — the code comment explains the long-idle-KV-transfer tradeoff, so it reads as deliberate.)

Full detail — exact lines, and PoCs where they exist — is in the emailed report. Just say the word on any of these and I'll send a focused PR.

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.