Skip to content

Commit b21a6b9

Browse files
json: bounds-check \uXXXX escape before MustMove
Reaching `case 'u'` in the JSON string-escape switch only proved the cursor sits on the `u` byte. `StrIterMustMove(&si, 5)` then consumed the `u` plus four hex digits without ever checking that the four digits exist -- crafted input like `"\u"` / `"\u1"` / `"\u12"` / `"\u123"` tripped LOG_FATAL inside the parser, aborting any caller that ingests untrusted JSON. Guard with RemainingLength >= 5 and bail to saved_si on truncation, matching the bool/null arms. Codify the rule: *Must* APIs are only safe when the callsite has statically proven the precondition (peek-then-Must, bounds-then-Must, or a caller-stage validated window). Auditing every Must site in the codebase against this rule turned up no other unsafe call -- all parser Must sites are dominated by elf_range_ok / cmdsize windows, RemainingLength checks, or peek-guarded while bodies.
1 parent 6b8114e commit b21a6b9

2 files changed

Lines changed: 45 additions & 0 deletions

File tree

CODING-CONVENTIONS.md

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -402,6 +402,41 @@ of the codebase to see them in action.
402402
`{<Nr}` (little-endian) and `{>Nr}` (big-endian), with `N` in
403403
`{1, 2, 4, 8}`.
404404
405+
## `*Must*` APIs and runtime input
406+
407+
- The `*Must*` family (`StrIterMustNext`, `StrIterMustMove`,
408+
`IterMustMove`, `IterMustRead`, `StrMustResize`, `VecMustInsert`,
409+
`BufMustRead*`, …) calls `LOG_FATAL` when its precondition fails.
410+
That is a process abort, not a recoverable error — it is the
411+
callsite's job to prove the precondition holds before invoking the
412+
`Must*`.
413+
- **Safe usage:** the precondition is statically dominated by code
414+
immediately above the call. The two canonical shapes:
415+
- **peek-then-Must:** `StrIterPeek(&si, &c)` (or `StrIterRead`)
416+
succeeded in the surrounding control flow, proving ≥1 byte
417+
remains, and then `StrIterMustNext(&si)` consumes that byte.
418+
- **bounds-then-Must:** `if (StrIterRemainingLength(&si) >= N) { ...
419+
StrIterMustMove(&si, N); ... }` (or the same shape with
420+
`IterRemainingLength`). The `if` is the proof; the `Must` is the
421+
payoff.
422+
- **Unsafe usage:** any `*Must*` whose precondition depends on
423+
runtime / attacker-controlled data (parsed file content, network
424+
bytes, `/proc` or `/etc` text, CLI argv, JSON / KvConfig values)
425+
without a preceding bounds check that statically dominates the
426+
call. Crafted truncated input becomes a denial-of-service: the
427+
process aborts on data, not bugs. Use the non-`Must` variant and
428+
bail (`LOG_ERROR` + return `saved_si` / sentinel) instead.
429+
- **Format strings are an exception by policy, not by type.** The
430+
`Fmt` parsers in `Io.c` use `*Must*` over the format string and
431+
rely on every caller passing a C string literal. The day a `Fmt`
432+
API grows a caller that pipes runtime text into the format
433+
argument, those `*Must*` sites become DoS vectors — flag and
434+
convert them.
435+
- **Audit hook:** when adding a `*Must*` callsite, name in a comment
436+
*what* proves the precondition (the peek above, the
437+
`RemainingLength` guard, the `elf_range_ok` / `cmdsize` window).
438+
If you cannot name it, you are introducing an UNSAFE site.
439+
405440
## Documentation
406441
407442
- Every public function and macro gets a doc comment with **`SUCCESS:`**

Source/Misra/Parsers/JSON.c

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,16 @@ StrIter JReadString(StrIter si, Str *str) {
255255

256256
// espaced unicode sequence
257257
case 'u' :
258+
// Peek above proved 1 byte (the 'u'); `\uXXXX`
259+
// needs 4 more. Bail on truncation instead of
260+
// letting StrIterMustMove abort -- attacker
261+
// JSON like `"\u"` / `"\u12"` would DoS the
262+
// process otherwise.
263+
if (StrIterRemainingLength(&si) < 5) {
264+
LOG_ERROR("Truncated \\uXXXX escape in JSON string.");
265+
StrClear(str);
266+
return saved_si;
267+
}
258268
LOG_ERROR(
259269
"No unicode support '{.6}'. Unicode sequence will be skipped.",
260270
LVAL(si.data + si.pos - 1)

0 commit comments

Comments
 (0)