InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

Stimulate reader: unbounded numDim causes out-of-bounds stack reads of header arrays

Open
#6,815 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.