# Patch Analysis — CVE-2026-33721 (MapServer SLD Categorize Threshold Overflow)

## Fix Commit

- **Commit**: `fb08dad4afee081b81c57ca0c5d37c149e7755f9` (`rel-8-6-1`)
- **Title**: `msSLDParseRasterSymbolizer(): fix potential heap buffer overflow (#7461)`
- **Files changed**: `src/mapogcsld.cpp` (1 line)

## What the Fix Changes

The patch changes a single condition in `src/mapogcsld.cpp` at line 2897 (rel-8-6-0 numbering):

```cpp
// Vulnerable (rel-8-6-0):
if (nValues == nMaxThreshold) {

// Fixed (rel-8-6-1):
if (nThresholds == nMaxThreshold) {
```

This is inside the `msSLDParseRasterSymbolizer()` function, in the branch that parses `<se:Categorize>` children inside a `<se:RasterSymbolizer>` SLD element.

### Context

- `nValues` counts `<se:Value>` elements in the `Categorize` node.
- `nThresholds` counts `<se:Threshold>` elements in the `Categorize` node.
- `nMaxThreshold` starts at 100 and is increased by 100 on each reallocation.
- `papszThresholds` is a `char**` array allocated with `msSmallMalloc(sizeof(char*) * nMaxThreshold)`.

The original code was a copy-paste error: the reallocation guard for the `papszThresholds` array incorrectly checked `nValues` (the counter for the *other* array) instead of `nThresholds`.

## What Assumptions the Fix Makes

1. **The only bug is the wrong-variable check.** The fix assumes that once the correct counter (`nThresholds`) is compared against `nMaxThreshold`, the dynamic array growth works correctly for any number of threshold elements.
2. **No other counters are similarly confused.** The fix assumes that the adjacent `papszValues` reallocation guard (`nValues == nMaxValues`) was already correct and no similar typos exist elsewhere in the same function or related functions.
3. **The input is well-formed SLD XML.** The fix does not add schema validation or hard limits on the number of elements; it relies on standard dynamic reallocation.

## What Code Paths / Inputs the Fix Does NOT Cover

- **No hard limit on element count.** An attacker could still send an extremely large SLD with millions of `<se:Threshold>` elements. While reallocation now works correctly, excessive memory consumption could still be a concern (DoS via memory exhaustion), though this is a different vulnerability class.
- **No validation of the `nValues == nThresholds + 1` invariant.** The code still assumes the SLD follows the expected schema (one initial Value, then alternating Threshold/Value pairs). Malformed XML with missing Values or extra Thresholds is silently ignored in the post-processing loop, but this is by design.
- **Other SLD parsing functions are unchanged.** The patch does not audit other symbolizer parsers (`PolygonSymbolizer`, `LineSymbolizer`, `PointSymbolizer`, `TextSymbolizer`) for similar copy-paste errors.

## Whether the Fix Is Complete

**Yes, the fix is complete for this specific vulnerability.** The root cause was a single wrong-variable typo, and correcting it closes the overflow path completely.

We verified completeness by:
1. Searching the entire `src/` tree for similar `msSmallMalloc`/`msSmallRealloc` patterns with potentially mismatched counters.
2. Testing the fixed binary with 200, 500, and attempted very-large threshold counts — all handled safely.
3. Testing multiple WMS entry points (`GetMap`, `GetLegendGraphic`, `GetStyles`) on the fixed version — none crashed.

## Target Threat Model Considerations

MapServer's `SECURITY.md` does not explicitly exclude SLD parsing from its security scope. SLD is a standard OGC mechanism intended to be exposed to untrusted callers. The vulnerability crosses a clear trust boundary (unauthenticated HTTP request → server-side memory corruption).

## Comparison: Before vs After

| Scenario | rel-8-6-0 (vulnerable) | rel-8-6-1 (fixed) |
|---|---|---|
| 101 `<se:Threshold>` elements | **Heap-buffer-overflow WRITE** at `papszThresholds[100]` | Reallocates to 200 slots, continues safely |
| 500 `<se:Threshold>` elements | Crashes at index 100 (first overflow) | Reallocates incrementally (100→200→300→400→500), continues safely |
| `GetMap` + `SLD_BODY` | Crash | Returns image or error |
| `GetLegendGraphic` + `SLD_BODY` | Crash | Returns image or error |
| `GetStyles` + `SLD_BODY` | Crash | Returns image or error |

## Conclusion

The patch is minimal, precise, and complete for the reported CVE. No bypasses were found, and no similar bugs were located in the surrounding codebase.