InsightSoftwareConsortium / InsightSoftwareConsortium/ITK
Stimulate reader: unbounded numDim causes out-of-bounds stack reads of header arrays
- Dominant language
- C++
- Stars
- 1.7k
- Forks
- 748
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 64
Description
## Description
`StimulateImageIO::InternalReadImageInformation` takes the `numDim` field straight from the `.spr` header, passes it to `SetNumberOfDimensions` without any bound, and then uses it to index four **fixed-size, uninitialized stack arrays** of length 4. A header declaring more than four dimensions reads off the end of all of them, and adjacent stack bytes are stored as the image's dimensions, origin, and spacing.
https://github.com/InsightSoftwareConsortium/ITK/blob/main/Modules/IO/Stimulate/src/itkStimulateImageIO.cxx#L214-L215
```cpp
float fov[4];
unsigned int dims[4];
```
Neither is initialized. The count that drives every subsequent loop is read like this:
https://github.com/InsightSoftwareConsortium/ITK/blob/main/Modules/IO/Stimulate/src/itkStimulateImageIO.cxx#L221-L225
```cpp
unsigned int dim = 0;
sscanf(line, "%*s %u", &dim);
this->SetNumberOfDimensions(dim);
```
`sscanf`'s return value is discarded, so a malformed `numDim` line leaves `dim` at its initial value; and no upper bound is applied to a well-formed one.
## Defects
**1. Out-of-bounds stack reads.** Four loops run to `m_NumberOfDimensions` while indexing length-4 arrays:
| Line | Statement | Array |
|---|---|---|
| [L240](https://github.com/InsightSoftwareConsortium/ITK/blob/main/Modules/IO/Stimulate/src/itkStimulateImageIO.cxx#L240) | `m_Dimensions[i] = dims[i];` | `dims[4]` |
| [L259](https://github.com/InsightSoftwareConsortium/ITK/blob/main/Modules/IO/Stimulate/src/itkStimulateImageIO.cxx#L259) | `m_Origin[i] = origin[i];` | `origin[4]` |
| [L300](https://github.com/InsightSoftwareConsortium/ITK/blob/main/Modules/IO/Stimulate/src/itkStimulateImageIO.cxx#L300) | `m_Spacing[i] = spacing[i];` | `spacing[4]` |
| [L442](https://github.com/InsightSoftwareConsortium/ITK/blob/main/Modules/IO/Stimulate/src/itkStimulateImageIO.cxx#L442) | `m_Spacing[i] = fov[i] / dims[i];` | `fov[4]`, `dims[4]` |
`m_Dimensions`/`m_Origin`/`m_Spacing` are resized by `SetNumberOfDimensions`, so the *destination* is fine; the source arrays are not. With `numDim: 9`, indices 4–8 read adjacent stack memory, which is then reported as image geometry.
**2. Uninitialized reads even with `numDim` ≤ 4.** `dims` and `fov` are declared outside the parse loop and are only assigned if the corresponding header line is present. A `.spr` with a `fov` line but no `dim` line reaches L442 and evaluates `fov[i] / dims[i]` against uninitialized `dims` — indeterminate values, and a division by zero whenever the garbage happens to be 0.
**3. Unbounded allocation.** `ImageIOBase::SetNumberOfDimensions` resizes `m_Dimensions`, `m_Spacing`, and `m_Origin` with no upper bound (`Modules/IO/ImageBase/src/itkImageIOBase.cxx:264`). `numDim: 4294967295` requests multi-gigabyte resizes, escaping as a non-ITK `std::bad_alloc`/`std::length_error` rather than an `itk::ExceptionObject`.
## Steps to reproduce
A `.spr` header of three lines is sufficient:
```
numDim: 9
dim: 4 4 2 2
dataType: WORD
```
`dims[2]` and `dims[3]` are both > 1, so neither of the reducing branches at L225–L235 fires and `m_NumberOfDimensions` stays 9. The loop at L240 then reads `dims[4]` through `dims[8]`.
Best observed under AddressSanitizer (an ITK build configured with `-fsanitize=address`), which reports the stack-buffer-overflow directly; without it the failure is silent and produces an image with fabricated spacing and origin.
## Expected behavior
A header declaring more dimensions than the reader supports should raise an `itk::ExceptionObject`, not read out of bounds. Silently emitting an image whose geometry came from adjacent stack memory is the worst outcome for a medical image reader.
## Suggested fix
Check `sscanf`'s return value, bound the count against the array length, and initialize the arrays:
```cpp
float fov[4]{};
unsigned int dims[4]{};
…
unsigned int dim = 0;
if (sscanf(line, "%*s %u", &dim) != 1 || dim < 1 || dim > 4)
{
itkExceptionMacro("Stimulate numDim must be between 1 and 4, got: " << line);
}
this->SetNumberOfDimensions(dim);
```
The `> 4` bound is the honest limit here: every array this reader parses a header field into is declared `[4]`, and the `sscanf` format strings read at most four values. Guarding the division at L442 against a zero `dims[i]` would also be worth doing.
## Versions
- ITK `main` (`af2fbc8`); the file is unchanged for many years, so the 5.x series is affected identically.
- Platform-independent; no allocator-specific behavior involved.
## Context
Found during a survey of header-field validation across ITK's IO modules, prompted by the review of #6761. Companion issue #6813 covers the same defect class in `ITKIOBruker`. Of the ITK-parsed readers surveyed, `ITKIOStimulate` was the only one with no validation of header-derived counts at all — `ITKIOMRC`, `ITKIOHDF5`, `ITKIOBMP` and the VTI reader all check theirs.
Contributor guide
Research direction
Start in Modules/IO/Stimulate/src/itkStimulateImageIO.cxx at itk::StimulateImageIO::InternalReadImageInformation and inspect the numDim parsing and geometry-copy loops. Reproduce the three-line header under AddressSanitizer, then verify malformed, oversized, and incomplete dimensions produce itk::ExceptionObject failures without out-of-bounds or uninitialized reads.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- computer-vision
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100