95 lines
4.2 KiB
Markdown
95 lines
4.2 KiB
Markdown
# arena-battle — findings & patches
|
|
|
|
TCP service, port **1337** (container 31337). Line protocol, one command per packet.
|
|
Flag store: **fighter notes** (`NOTE_CREATE` / `NOTE_GET`).
|
|
|
|
## 1. Predictable auth tokens — CRITICAL, flag leak
|
|
|
|
`create_fighter()`:
|
|
|
|
```cpp
|
|
f.auth_token = "token_" + std::to_string(f.id) + "_" + std::to_string(rand());
|
|
```
|
|
|
|
`srand()` is never called, so glibc `rand()` emits its default seed-1 sequence
|
|
(1804289383, 846930886, 1681692777, …). Fighter ids are sequential and `rand()`
|
|
is called exactly once per fighter, so **the Nth fighter's token is fully
|
|
computable offline**.
|
|
|
|
`NOTE_GET` authenticates on the token alone — no session, no ownership check —
|
|
so any token dumps that fighter's notes:
|
|
|
|
```
|
|
REGISTER x warrior -> tells us the current fighter id (= fighter count)
|
|
NOTE_GET token_<id>_<rand[id]> -> note list
|
|
NOTE_GET token_<id>_<rand[id]> <n> -> title + content (flag)
|
|
```
|
|
|
|
Verified: `sploits/arena_battle.py` pulls 15/15 planted flags in 0.23 s against
|
|
a box with 1600 fighters.
|
|
|
|
**Patch:** tokens now come from `/dev/urandom` (64-bit, `gen_auth_token()`),
|
|
keeping the exact `token_<id>_<digits>` shape so clients see no change.
|
|
A CSPRNG is required here, not `mt19937` — an attacker who registers a few
|
|
hundred fighters can recover a Mersenne Twister state from its outputs.
|
|
|
|
Also added: a connection is dropped after 16 invalid tokens (`MAX_BAD_TOKENS`),
|
|
so token guessing cannot be scaled up.
|
|
|
|
## 2. `BUY` integer overflow — free gold
|
|
|
|
`int total_price = item.price * quantity;` overflows, and the code then *rewards*
|
|
it: `f.gold += abs(total_price)` returning `"OK: Overflow exploited!"`.
|
|
**Patch:** 64-bit arithmetic plus an explicit cap; the free-gold branch is gone.
|
|
|
|
## 3. No locking on shared state — crash risk (SLA)
|
|
|
|
`fighters`, `battles`, `client_fighter` and `fighter_battle` were mutated from
|
|
every client thread with **no mutex at all** — `state_mutex` was only ever taken
|
|
by `cleanup_thread`. Concurrent `std::map` inserts are undefined behaviour, and
|
|
`cleanup_thread` erasing a fighter while a handler holds a `Fighter&` to it is a
|
|
use-after-free.
|
|
**Patch:** every command and the per-connection teardown now run under
|
|
`state_mutex`. (I could not force a crash in testing — this is hardening against
|
|
a real race, not a demonstrated exploit.)
|
|
|
|
## 4. `cleanup_thread` closed file descriptors owned by live threads
|
|
|
|
It called `close(cf->first)` on a socket whose handler thread was still in
|
|
`read()`. Once closed, the fd number is recycled onto the *next* accepted
|
|
connection, and the stale handler then reads/writes another client's socket.
|
|
**Patch:** the mapping is erased, the fd is left to its owning thread.
|
|
|
|
## 5. `operator[]` on missing fighters
|
|
|
|
`cmd_attack` / `cmd_status` did `fighters[defender_id]`, inserting a blank
|
|
fighter when the id was gone (e.g. cleaned up mid-battle).
|
|
**Patch:** existence checked first.
|
|
|
|
## 6. `listen(server_fd, 10)` — trivial connection-storm DoS
|
|
|
|
A backlog of 10 lets a burst of connections lock the checker out; this actually
|
|
happened during load testing. **Patch:** backlog 256.
|
|
|
|
## 7. Docker
|
|
|
|
- `docker-compose.yml` had no `image:` line — the CI job does
|
|
`docker compose config --images | grep "^git.itqdev.xyz/4x10m/"` and
|
|
`exit 1`s when empty, so **every push failed**. Added.
|
|
- Healthcheck used `nc`, which is not installed in the image, so the container
|
|
was permanently `unhealthy`. Switched to a bash `/dev/tcp` probe.
|
|
|
|
## Not fixed — watch these
|
|
|
|
- **Flags are written to `logs/server.log`**: `LOG(client_str + " " + line)`
|
|
logs the full command line, so every `NOTE_CREATE <title> <flag>` lands on
|
|
disk in cleartext. Harmless on its own, but it turns any file-read or RCE
|
|
anywhere on the box into a full flag dump. Consider redacting.
|
|
- `split(line, ' ')` means a note title/content can never contain a space, and
|
|
two commands in one TCP packet are parsed as one. Left as-is: the checker
|
|
depends on this behaviour.
|
|
- Other teams may "fix" the token bug with `srand(time(NULL))`, which is still
|
|
breakable (brute-force the seed, or recover the generator state from ~62
|
|
tokens we register ourselves). The sploit prints a warning on token mismatch;
|
|
building that fallback is the next step if flags dry up.
|