S2ShapeIndexCell::Decode: Bound untrusted counts

[sushant-me]

Split out of #675 at jmr's request, so the parts that were agreed on can be reviewed and submitted separately. This is the src/s2/s2shape_index.cc half only: the loop_starts validation is #681, and the "return an empty cell instead of nullptr" change is not included here.

What the malformed input does before this change

S2ShapeIndexCell::Decode() uses three counts taken straight from the input, none of which is checked against the index or against the type it is stored in. All three are reachable from a cell of a few bytes.

1. Single-shape path: int num_edges = header >> 3; overflows int for a large header. For num_edges == 0xFFFFFFFF the value becomes -1, which S2ClippedShape::Init() receives as uint32_t num_edges and turns into new int32_t[0xFFFFFFFF]. Under AddressSanitizer:

ERROR: AddressSanitizer: requested allocation size 0x3fffffffc (0x400001000 after
adjustments for alignment, red zones etc.) exceeds maximum supported size of 0x40000000
    #1 S2ClippedShape::Init(int, unsigned int) src/s2/s2shape_index.h:557
    #2 S2ShapeIndexCell::Decode(int, Decoder*) src/s2/s2shape_index.cc:229

That is a 16 GiB allocation requested by a 5-byte cell header; where it cannot be satisfied the std::bad_alloc escapes Decode(). For num_edges == 2**31 the count truncates to 0 in the 31-bit num_edges_ field instead, and Decode() returns true with a clipped shape whose edge count is wrong.

2. Multi-shape path: num_clipped = header >> 3; is passed to add_shapes(), which resizes the cell's clipped-shape array to that attacker-controlled count before any further input is read.

3. Multi-shape path: an accumulated shape_id is compared against INT_MAX but never against num_shape_ids, so a cell can encode a clipped shape with shape_id >= num_shape_ids, and later code indexes the shapes with it.

The change

Reject num_edges > INT32_MAX, num_clipped > num_shape_ids, and shape_id >= num_shape_ids (in each of the three branches that accumulate a shape id).

There is deliberately no bound on num_edges against the remaining input, and the comment in the code records why: edges are run-length encoded, so a count of 8 or more is followed by a varint holding the rest of the run length, and a few bytes can legitimately describe many edges. DecodeEdges() already fails cleanly when the input is exhausted.

Tests

src/s2/s2shape_index_test.cc (which already carried a TODO asking for S2ShapeIndexCell coverage) gains three tests:

• S2ShapeIndexCell.DecodeRejectsEdgeCountOverflow
• S2ShapeIndexCell.DecodeRejectsTooManyClippedShapes
• S2ShapeIndexCell.DecodeRejectsShapeIdPastNumShapeIds (one input per accumulation branch)

Each builds a malformed cell byte string and calls S2ShapeIndexCell::Decode(num_shape_ids, &decoder) directly. They never construct or iterate an EncodedS2ShapeIndex, so they do not touch the encoded point vector format, which is little-endian only (encoded_s2point_vector.cc logs FATAL: Needs architecture with 64-bit little-endian unaligned loads). That keeps them valid on the cmake-big-endian job.

Verification

$ git checkout origin/master -- src/s2/s2shape_index.cc        # without the fix
$ ninja -C /home/logic/tmp/buildA -j12 s2shape_index_test
$ /home/logic/tmp/buildA/s2shape_index_test --gtest_filter='S2ShapeIndexCell.*'
Value of: DecodeCell(&cell, 1, {(kNumEdges << 3) | 3})
  Actual: true
Expected: false
[  FAILED  ] S2ShapeIndexCell.DecodeRejectsEdgeCountOverflow
[  FAILED  ] S2ShapeIndexCell.DecodeRejectsTooManyClippedShapes
[  FAILED  ] S2ShapeIndexCell.DecodeRejectsShapeIdPastNumShapeIds
[  PASSED  ] 0 tests.
 3 FAILED TESTS

$ git checkout HEAD -- src/s2/s2shape_index.cc                 # with the fix
$ ninja -C /home/logic/tmp/buildA -j12 s2shape_index_test
$ /home/logic/tmp/buildA/s2shape_index_test --gtest_filter='S2ShapeIndexCell.*'
[       OK ] S2ShapeIndexCell.DecodeRejectsEdgeCountOverflow
[       OK ] S2ShapeIndexCell.DecodeRejectsTooManyClippedShapes
[       OK ] S2ShapeIndexCell.DecodeRejectsShapeIdPastNumShapeIds
[  PASSED  ] 3 tests.

The whole binary also passes under ASan (-fsanitize=address -fno-omit-frame-pointer -g), and the new checks do not reject valid cells: s2shape_index_test 5/5, mutable_s2shape_index_test 29/29, encoded_s2shape_index_test 9/9.