babelfish-for-postgresql / babelfish_extensions / 28077089988

77%

Build Type

push

github

Committed by web-flow

Commit Message

Fix OOB read and harden allocation-size math in MultiLineString varbinary parsing (#4870) (#4876)

Currently, the CLR-binary → PostGIS conversion path for spatial types (reached via `CAST(0x... AS sys.geometry)`) does not fully validate attacker-controllable fields in the serialized MultiLineString/MultiPoint layout. With this change, two such gaps introduced in the MultiLineString support work (#4831) are closed so that malformed input is rejected with a clean conversion error instead of crashing the backend or corrupting downstream processing.

- **What it used to do:** `parse_figures_and_shapes()` accepted a child shape `figure_offset` of `0xFFFFFFFF` for any shape, because the bounds check explicitly exempted that sentinel. The `0xFFFFFFFF` sentinel is only meaningful for *empty* geometries, which take a separate path (`handle_empty_geometry_bytea`, selected when
  `npoints == 0`) and never reach this code. A crafted *non-empty* MultiLineString/MultiPoint with a child `figure_offset = 0xFFFFFFFF` therefore passed validation, and `get_child_info()` later used that value to index the `figures[]` array — an out-of-bounds read far past the allocated buffer, crashing the backend. Separately, `handle_multi_to_postgis()` accumulated the output WKB size in a `uint32_t` and only checked each child against `MaxAllocSize`, never the running total.
- **What it does now:** non-empty multi-geometry child shapes must reference a valid in-range figure; any out-of-range `figure_offset` (including the `0xFFFFFFFF` sentinel) is rejected with the standard varbinary-conversion error before it can be dereferenced. The output-size accumulator now uses a `uint64_t` and is checked against `MaxAllocSize` on every iteration, consistent with the existing per-child check in `calculate_child_wkb_size()`.
- **Why:** the `figure_offset` gap is an out-of-bounds read reachable by any authenticated user that crashes the backend (denial of service). The accumulator change is defense... (continued)

Coverage Stats

6 of 7 new or added lines in 1 file covered. (85.71%)

4 existing lines in 3 files now uncovered.

55749 of 72112 relevant lines covered (77.31%)

594409.34 hits per line