From a740955a07362bb4f63087970f2db7daba9ab41b Mon Sep 17 00:00:00 2001 From: Ken Johnson Date: Sun, 19 Apr 2026 07:09:53 -0700 Subject: [PATCH] wc_sdk: cycle cap on IsamDeleteVariableRec + SECURITY.md Audit pass over the vendored Wildcat! 4 SDK (~11 K LoC, circa 2003). Most findings are either defended at the wrapper layer already (mb.fmt.wildcat clamps MsgBytes) or live in DOS-era 16-bit segment math that is unreachable under 32/64-bit FPC. One credible DoS did warrant a direct patch: vrec.pas (IsamDeleteVariableRec): the repeat-loop walks a NextRefNr chain with no cycle guard. A corrupted .MDF that points NextRefNr back at an earlier record caused the deleter to loop forever, deleting the same records repeatedly. Added a 10 M hop cap matching the pattern used in the Squish ReIndex fix. SECURITY.md: documents the threat model for the vendored code, what is defended (MsgBytes clamp, O_NOFOLLOW on lock file, new cycle cap), and what is not (arbitrary-offset Seek via UserConfData, tree-walks below the variable-record layer, FileOpen without O_NOFOLLOW on data files). Consumers get deployment guidance: run Wildcat under an isolated account, mount externally-sourced bases read-only in a sandbox. --- src/wc_sdk/SECURITY.md | 65 ++++++++++++++++++++++++++++++++++++++++++ src/wc_sdk/vrec.pas | 15 ++++++++++ 2 files changed, 80 insertions(+) create mode 100644 src/wc_sdk/SECURITY.md diff --git a/src/wc_sdk/SECURITY.md b/src/wc_sdk/SECURITY.md new file mode 100644 index 0000000..9e7e13e --- /dev/null +++ b/src/wc_sdk/SECURITY.md @@ -0,0 +1,65 @@ +# wc_sdk security posture + +`src/wc_sdk/` is ~11 K lines of Wildcat! 4 SDK code vendored from a +release circa 2003. It predates modern threat models and contains +constructs that would not pass code review today: + +- Fixed-size stack / static buffers filled from `BlockRead` where the + length comes from a field in the record being read. +- Tree / chain walks over in-file pointers with no cycle detection. +- `FileOpen` without `O_NOFOLLOW` on POSIX. +- Turbo Pascal-era `New` / `Dispose` pairs whose error paths leak on + failure. + +## Threat model + +The attacker supplies the bytes on disk for one or more of +`*.MDF`, `*.MNX`, `*.MKY`, the Wildcat user database, and the +`MAKEWILD.DAT` config. The code is invoked read-only through +`src/formats/mb.fmt.wildcat.pas` (our wrapper) from tools that mount +a Wildcat base -- typically a dedicated mailer / tosser account that +should not be root. + +## What is defended + +- `mb.fmt.wildcat.TWildcatBase.ReadBody` clamps the attacker-controlled + `Hdr.MsgBytes` against the fixed-size `TMsgText` so the wrapper can + never hand the SDK a length that would overflow the receive buffer. +- `mb.lock.TMessageLock` uses `O_NOFOLLOW` for the per-base sentinel, + so the lock file cannot be redirected by a symlink swap even if the + Wildcat directory is world-writable. +- `vrec.IsamDeleteVariableRec` has a hop cap so a circular + `NextRefNr` chain in a crafted `.MDF` cannot loop the deleter + forever. + +## What is NOT defended (known residual risk) + +- **Arbitrary-offset Seek** via `UserConfData` in the user DB: a + corrupted `USERS.XDB` can point `UserConfData` anywhere in the + file. The subsequent `BlockRead` is into a fixed-size local, + which contains the record the parser expected to see there. The + caller (`LookupUserId`) treats a bad read as a miss. No known + memory-corruption primitive, but the returned data is + attacker-controlled. Callers that trust user-DB fields for + authentication / capability checks should not rely on Wildcat + alone. +- **Tree walks in `btfileio.pas` / `btisbase.pas`** have no depth or + cycle cap below the variable-record layer. Malformed B-tree files + can theoretically hang reads. We have not seen it in practice. +- **`FileOpen` in `btfileio.pas`** does not pass `O_NOFOLLOW` when + opening data files. Symlink-in-data-dir attacks are therefore + possible if the Wildcat directory is shared with an untrusted + local user. Running Wildcat under a dedicated account with a + non-shared data directory is the recommended mitigation. + +## Guidance for consumers + +- Treat Wildcat bases exactly like any other untrusted input: run + the tosser / reader as an isolated account, with the data + directory owned by that account and mode 700. +- If you are importing a Wildcat base from an external source, + mount it read-only in a sandbox (container, user namespace) the + first time you read it. The hardening above bounds allocation + and CPU use, but a crafted base can still return garbage data -- + that is a correctness decision for the caller, not a safety + guarantee from the library. diff --git a/src/wc_sdk/vrec.pas b/src/wc_sdk/vrec.pas index 569e53c..db3938e 100644 --- a/src/wc_sdk/vrec.pas +++ b/src/wc_sdk/vrec.pas @@ -316,11 +316,20 @@ end; procedure IsamDeleteVariableRec(IFBPtr : IsamFileBlockPtr; {!!.50} RefNr : LongInt); +const + { Cycle guard. A corrupted data file can point NextRefNr back at + an earlier record in the chain; without a cap the repeat-loop + deletes records in a ring forever, corrupting state and burning + CPU. 10 M hops is far beyond any legitimate chain in a Wildcat + base (variable-record chains are typically < 100 entries). } + MAX_CHAIN_HOPS = 10 * 1000 * 1000; var NextRefNr : LongInt; S : Word; + Hops : LongInt; begin S := ILI (IFBPtr^.DIDPtr^[0]^.LenRec).Lo; + Hops := 0; repeat IsamGetBlock(IFBPtr^.DatF, LongInt (S) * RefNr + LongInt (S-4), SizeOf (LongInt), NextRefNr); @@ -328,6 +337,12 @@ begin IsamDeleteRec(IFBPtr, RefNr); {!!.50} if not IsamOK then Exit; RefNr := NextRefNr; + Inc(Hops); + if Hops > MAX_CHAIN_HOPS then begin + IsamOK := False; + IsamError := 10415; { reuse "record too large" error code } + Exit; + end; until NextRefNr = 0; end;