|
1 | | -# ADR 0003: Structured sanitization of `VortexException` messages |
| 1 | +# ADR 0003: `VortexException` contract — message sanitization and bounds typing |
2 | 2 |
|
3 | 3 | - **Status:** Accepted — implementation pending (see Phases below) |
4 | | -- **Date:** 2026-06-13 |
| 4 | +- **Date:** 2026-06-13 (bounds-typing scope added 2026-06-20) |
5 | 5 | - **Deciders:** project maintainer |
6 | 6 | - **Related:** [ADR 0001 — Split read and write runtimes](0001-split-read-and-write-runtimes.md), |
| 7 | + [ADR 0004 — Resource caps and `ReadOptions`](0004-resource-caps-read-options.md), |
7 | 8 | [SECURITY.md](../../SECURITY.md) |
8 | 9 |
|
9 | 10 | ## Context |
@@ -40,6 +41,37 @@ typed `EncodingId` for the attribution field but accepts a free-form `String` |
40 | 41 | for the message body, which callers build via `+` or `.formatted()`. There is |
41 | 42 | no sanitization contract. |
42 | 43 |
|
| 44 | +### Second axis — exception *type*, not just message |
| 45 | + |
| 46 | +Message sanitization governs *what a `VortexException` says*. A separate, |
| 47 | +orthogonal gap governs *whether a `VortexException` is thrown at all*. The |
| 48 | +reader's contract (SECURITY.md) is: any malformed input throws |
| 49 | +`VortexException`, never a raw JDK exception. But ~21 `MemorySegment.asSlice` |
| 50 | +call sites take offsets/lengths straight from untrusted layout/footer |
| 51 | +metadata and pass them to the JDK unguarded: |
| 52 | + |
| 53 | +```java |
| 54 | +// ScanIterator — fbStart/fbLen decoded from an attacker FlatBuffer |
| 55 | +ByteBuffer fbBuf = seg.asSlice(fbStart, fbLen).asByteBuffer()... // raw IndexOutOfBoundsException on overflow |
| 56 | +``` |
| 57 | + |
| 58 | +Only `PostscriptParser` guards its slices (a private `slice()` + |
| 59 | +`checkBlobBounds()`); its own comment warns that every *other* scan-time |
| 60 | +`asSlice` would throw `IndexOutOfBoundsException` and break the contract. The |
| 61 | +same leak appears in three more shapes: |
| 62 | + |
| 63 | +- `Math.toIntExact(storage.length())` (4 extension decoders) → raw |
| 64 | + `ArithmeticException` on a > 2 GB declared length. |
| 65 | +- `new byte[(int)(end - start)]` / `new long[(int) rowCount]` (VarBin, AlpRd, |
| 66 | + Delta) → `NegativeArraySizeException` / `OutOfMemoryError` on crafted |
| 67 | + non-monotonic offsets or huge counts. |
| 68 | +- `ByteBuffer` is `int`-indexed (2 GB cap); a slice fed to `asByteBuffer()` |
| 69 | + past that throws raw too. |
| 70 | + |
| 71 | +These are the same `VortexException`-contract violation as a leaked ANSI |
| 72 | +escape — just on the type axis instead of the content axis — so they belong |
| 73 | +in the same ADR. |
| 74 | + |
43 | 75 | ## Decision |
44 | 76 |
|
45 | 77 | **Pick Option A (enum error catalog) as the structural shape.** Add a |
@@ -203,6 +235,73 @@ throw new VortexException(VortexError.UNKNOWN_LAYOUT_ENCODING, layout.encodingId |
203 | 235 | The message format `[UNKNOWN_LAYOUT_ENCODING] vortex.flat\x0a<injected>` is |
204 | 236 | machine-parseable, log-friendly, and injection-safe. |
205 | 237 |
|
| 238 | +### Bounds typing: the `IoBounds` helper |
| 239 | + |
| 240 | +A public static utility in `io.github.dfa1.vortex.core` (must be reachable by |
| 241 | +core itself — `ProtoReader` has a site — plus reader, `reader.array`, and |
| 242 | +`reader.decode`; `reader → core`, so core is the only home that covers all |
| 243 | +layers). It wraps the untrusted-offset operations and throws `VortexException` |
| 244 | +(via the `VortexError` catalog above) instead of the raw JDK exception: |
| 245 | + |
| 246 | +```java |
| 247 | +public final class IoBounds { |
| 248 | + private IoBounds() {} |
| 249 | + |
| 250 | + /// off/len must lie within [0, size]. Throws VortexException otherwise. |
| 251 | + public static void checkRange(long off, long len, long size) { |
| 252 | + if (off < 0 || len < 0 || len > size - off) { |
| 253 | + throw new VortexException(VortexError.SEGMENT_INDEX_OUT_OF_RANGE, off, len, size); |
| 254 | + } |
| 255 | + } |
| 256 | + |
| 257 | + /// Bounds-checked asSlice — the canonical replacement for raw seg.asSlice. |
| 258 | + public static MemorySegment slice(MemorySegment seg, long off, long len) { |
| 259 | + checkRange(off, len, seg.byteSize()); |
| 260 | + return seg.asSlice(off, len); |
| 261 | + } |
| 262 | + |
| 263 | + /// long → int for sizes/counts that index a ByteBuffer or back a Java array. |
| 264 | + /// Replaces Math.toIntExact (ArithmeticException) and guards the 2 GB cap. |
| 265 | + public static int toIntSize(long n) { |
| 266 | + if (n < 0 || n > Integer.MAX_VALUE) { |
| 267 | + throw new VortexException(VortexError.SEGMENT_INDEX_OUT_OF_RANGE, n); |
| 268 | + } |
| 269 | + return (int) n; |
| 270 | + } |
| 271 | + |
| 272 | + /// Element count for a `new T[n]` decode buffer; same guard as toIntSize, |
| 273 | + /// named for the alloc-count call sites (the per-encoding cap from ADR 0004 |
| 274 | + /// plugs in here later). |
| 275 | + public static int checkCount(long n) { |
| 276 | + return toIntSize(n); |
| 277 | + } |
| 278 | +} |
| 279 | +``` |
| 280 | + |
| 281 | +Why a static helper, not a `BoundedSegment` wrapper (the approach explored in |
| 282 | +[PR #27](https://github.com/dfa1/vortex-java/pull/27)): |
| 283 | + |
| 284 | +- The hot path is `MemorySegment` zero-copy slices; a wrapper type would have |
| 285 | + to be unwrapped at every typed accessor or it taxes per-element reads. A |
| 286 | + static call slices once, off the per-element path, and returns a plain |
| 287 | + `MemorySegment` — no new type crosses module boundaries. |
| 288 | +- It mirrors the `Sanitize` decision: one small pure primitive in `core`, not |
| 289 | + a new abstraction. `Sanitize` cleans the message; `IoBounds` types the |
| 290 | + throw. Symmetric. |
| 291 | + |
| 292 | +#### The consumer-access carve-out |
| 293 | + |
| 294 | +The ~14 per-element guards in `Lazy*` / `Materialized*` / `Generic` accessors |
| 295 | +(`getInt(i)` etc.) throw `IndexOutOfBoundsException` and **stay that way** — |
| 296 | +they are *consumer* random-access (`array.getInt(5)`), where IOOBE is the |
| 297 | +correct JDK-idiomatic signal (cf. `List.get`), not a malformed-file event. |
| 298 | +These must **not** be routed through `IoBounds`. They are instead collapsed |
| 299 | +onto the JDK built-in `Objects.checkIndex(i, length)` (Java 16+) — stdlib, no |
| 300 | +custom helper. The dividing line: |
| 301 | + |
| 302 | +- offset/length/count from **parsed file bytes** → `IoBounds` → `VortexException` |
| 303 | +- index from a **caller's accessor argument** → `Objects.checkIndex` → `IndexOutOfBoundsException` |
| 304 | + |
206 | 305 | ## Migration phases |
207 | 306 |
|
208 | 307 | ### Phase A — Foundation (~1.5 h) |
@@ -244,6 +343,35 @@ approximate-fit existing ones. |
244 | 343 | prevent regression. Also flag `+` inside the VortexException args to |
245 | 344 | catch interpolation-before-sanitization. |
246 | 345 |
|
| 346 | +### Phase E — Bounds typing via `IoBounds` (0.8.0) |
| 347 | + |
| 348 | +Independent of A–D. The `VortexError` catalog (Phase A) is not built yet, so |
| 349 | +`IoBounds` ships using the current `VortexException(String)` constructor with a |
| 350 | +fixed, non-interpolated message (no attacker strings in the bounds messages — |
| 351 | +only numeric offsets/lengths, which need no sanitization). When Phase A lands, |
| 352 | +`IoBounds` migrates to `VortexError.SEGMENT_INDEX_OUT_OF_RANGE` mechanically |
| 353 | +with every other site. Lands in 0.8.0 before the release, since variant decode |
| 354 | +widens the parse surface. |
| 355 | + |
| 356 | +1. Add `IoBounds` (`slice` / `checkRange` / `toIntSize` / `checkCount`) in |
| 357 | + `core` with unit tests: negative offset, length overflow, off+len past end, |
| 358 | + > 2 GB size, exact-boundary pass. |
| 359 | +2. Route the ~21 raw `asSlice` sites through `IoBounds.slice`; fold |
| 360 | + `PostscriptParser`'s private `slice()`/`checkBlobBounds` and `ProtoReader`'s |
| 361 | + hand-rolled guard into it. |
| 362 | +3. Replace `Math.toIntExact(...length())` (4 extension decoders) with |
| 363 | + `IoBounds.toIntSize`; guard the `new T[(int) n]` alloc sites with |
| 364 | + `IoBounds.checkCount`. |
| 365 | +4. Collapse the ~14 consumer-access `getX(i)` guards onto |
| 366 | + `Objects.checkIndex(i, length)` (separate commit — different error class, |
| 367 | + no `IoBounds`). |
| 368 | +5. Checkstyle `RegexpSingleline` rejecting raw `.asSlice(` in |
| 369 | + `reader` / `reader.array` / `reader.decode` / `core.proto` packages |
| 370 | + (mirrors the existing `<p>`-blocking rule), so new raw slices can't regress. |
| 371 | +6. `BoundsTypingSecurityTest`: crafted file with out-of-range slice offset, |
| 372 | + oversize declared length, and non-monotonic VarBin offsets each produce a |
| 373 | + `VortexException`, never a raw JDK exception. |
| 374 | + |
247 | 375 | ## Alternative considered |
248 | 376 |
|
249 | 377 | **Option B — Sealed `VortexException` hierarchy:** Make `VortexException` |
|
0 commit comments