InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

Bruker 2dseq reader does not validate file-controlled sizes before indexing and allocating

Open
#6,813 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

`itkBruker2dseqImageIO.cxx` reads geometry, frame-count, and frame-group values out of a Bruker `visu_pars` header and uses them to size allocations and index arrays without validating any of them. `GetParameter` verifies only that a key is **present**, never that the value has the cardinality or range the call site assumes:

https://github.com/InsightSoftwareConsortium/ITK/blob/main/Modules/IO/Bruker/src/itkBruker2dseqImageIO.cxx#L45-L53

`visu_pars` is untrusted input in the ordinary case — a user opens a dataset obtained from a scanner, a collaborator, or a public archive. A malformed or hostile header currently reaches out-of-bounds reads, a potential heap write overflow, and silently wrong image geometry.

All five items below are present on `main` today (`af2fbc8`) and are long-standing; none were introduced by a recent change. They were found while reviewing #6761, which does not create any of them, but does raise their reachability considerably: that PR takes the readable-dataset count from 677 to 1631 out of 1636 sampled public ParaVision datasets, so roughly 950 files that previously threw during header parsing will reach this geometry code once it lands.

## Items

### S1 — Array cardinality never validated before indexing

`VisuCoreOrientation` is consumed as nine doubles regardless of how many the file supplied:

https://github.com/InsightSoftwareConsortium/ITK/blob/main/Modules/IO/Bruker/src/itkBruker2dseqImageIO.cxx#L901

`##$VisuCoreOrientation=( 1 ) 1` yields a one-element vector, and `vnl_matrix(&orient[0], 3, 3)` reads 64 bytes past the allocation. `( 0 )` indexes an empty vector.

Same pattern, same cause:

- `VisuCorePosition` — `vnl_vector corner(&position[0], 3)` (L908) needs at least 3
- `VisuCoreSize` / `VisuCoreExtent` — indexed to `[2]` when `VisuCoreDim == 3`
- `VisuCoreFrameThickness` — `[0]` on a possibly-empty vector (L818)

A checked accessor keeps each call site to one line:

```cpp
// Cardinality comes from the file; verify before indexing.
std::vector
GetParameterArray(const MetaDataDictionary & dict, const std::string & name, const SizeType minimumSize)
{
auto values = GetParameter>(dict, name);
if (values.size() < minimumSize)
{
itkGenericExceptionMacro("Bruker parameter " << name << " has " << values.size() << " values, need "
<< minimumSize);
}
return values;
}
```

### S2 — `VisuFGOrderDesc` tuple indexed at `[1]` without a size check

https://github.com/InsightSoftwareConsortium/ITK/blob/main/Modules/IO/Bruker/src/itkBruker2dseqImageIO.cxx#L592-L601

`##$VisuFGOrderDesc=( 1 ) (2)` produces a single-field tuple, so `i[1]` is out of bounds.

```cpp
if (i.size() < 2)
{
itkGenericExceptionMacro("Bruker 2dseq VisuFGOrderDesc tuple has " << i.size() << " fields, need at least 2");
}
```

### S3 — Signed frame-group length multiplied into an unsigned accumulator

`itk::StringToInt32` correctly throws an ITK exception on a non-numeric field, but its result is **signed** and is multiplied into a `size_t` (L600). `( 1 ) (-4, , ...)` wraps `sizeToSwap` to ~1.8e19, which passes the `sizeToSwap > 1` guard and reaches:

https://github.com/InsightSoftwareConsortium/ITK/blob/main/Modules/IO/Bruker/src/itkBruker2dseqImageIO.cxx#L86

That either throws a non-ITK `std::bad_alloc`/`std::length_error` out of `Read()`, or — when the product wraps to something small — lets the copy loop walk `fromPixel` far past the end of `buffer`, a **heap write overflow**. `ReadImageInformation` has the same hazard via its `sizeT *= length` accumulation.

```cpp
const auto rawLength = itk::StringToInt32(i[0], "Bruker 2dseq VisuFGOrderDesc size");
if (rawLength <= 0)
{
itkGenericExceptionMacro("Bruker 2dseq VisuFGOrderDesc size must be positive, got " << rawLength);
}
```

### S4 — `VisuCoreFrameCount` never reconciled with the allocated buffer

`buffer` is sized from `GetImageSizeInComponents()`, but `Rescale`'s outer loop is driven by a dictionary value:

https://github.com/InsightSoftwareConsortium/ITK/blob/main/Modules/IO/Bruker/src/itkBruker2dseqImageIO.cxx#L548

Nothing checks `frameCount * frameSize` against the allocation, so a large `VisuCoreFrameCount` combined with a small `VisuCoreSize` writes past the buffer. A negative value is worse: `static_cast` of a negative `double` is undefined behavior.

```cpp
if (frameCount == 0 || frameSize == 0 || frameCount != numberOfComponents / frameSize ||
frameCount * frameSize != numberOfComponents)
{
itkExceptionMacro("Bruker VisuCoreFrameCount " << frameCount << " is inconsistent with " << numberOfComponents
<< " components in " << frameSize << "-voxel frames");
}
```

### S5 — Zero or non-finite extent yields `inf` spacing with no diagnostic

https://github.com/InsightSoftwareConsortium/ITK/blob/main/Modules/IO/Bruker/src/itkBruker2dseqImageIO.cxx#L796

`##$VisuCoreSize=( 2 ) 0 0` sets a zero-length dimension and an infinite spacing, and the image is returned with no error. `halfStep[0] = FoV[0] / (2 * size[0])` then propagates `inf`/`NaN` into the origin. For a medical image reader, silently emitting geometrically meaningless data is worse than refusing the file.

```cpp
for (SizeType i = 0; i < brukerDim; ++i)
{
if (!(size[i] >= 1.0) || !std::isfinite(FoV[i]))
{
itkGenericExceptionMacro("Bruker VisuCoreSize/VisuCoreExtent invalid on axis " << i);
}
}
```

The `!(x >= 1.0)` form is deliberate — it rejects `NaN`, which `x < 1.0` would let through.

## Suggested approach

S1 and S5 share a fix (a checked accessor plus a range check where extents are consumed); S2 and S3 share one (validating the `VisuFGOrderDesc` tuples once, at parse time, instead of at each use). S4 is independent.

Regression coverage can be synthetic — malformed `visu_pars` files written by the test, with no new external data required. #6761 adds a GoogleTest driver to this module that would be a natural home for them.

## Versions

- ITK `main` (`af2fbc8`); the same code is present in the 5.4 series.
- Platform-independent — no allocator or compiler-specific behavior involved.

Contributor guide

Open the contributing guide

Research direction

Start with Modules/IO/Bruker/src/itkBruker2dseqImageIO.cxx, especially GetParameter, ReadImageInformation, Read, and Rescale, and inspect the GoogleTest driver added by #6761. Create synthetic malformed visu_pars cases covering S1-S5 and run the Bruker tests. Done means invalid cardinalities, sizes, frame counts, and extents are rejected with ITK diagnostics rather than unsafe indexing, allocation, or geometry.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
security, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 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.