InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

VTK legacy reader: COLOR_SCALARS component count is used unvalidated

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

The `COLOR_SCALARS` branch of the legacy VTK ASCII header reader discards `sscanf`'s return value and applies no upper bound to the component count before using it to configure the image:

https://github.com/InsightSoftwareConsortium/ITK/blob/main/Modules/IO/VTK/src/itkVTKImageIO.cxx#L315-L316

```cpp
unsigned int numComp = 1;
sscanf(text.c_str(), "%*s %*s %u", &numComp);
```

`numComp` then reaches `SetNumberOfComponents(numComp)` on both the ASCII and binary paths (L333 and L339). Because `GetImageSizeInBytes()` multiplies the component count into the buffer size, a header line such as

```
COLOR_SCALARS colors 4294967295
```

sizes the read allocation from an unvalidated file-controlled value.

Initializing `numComp = 1` means a *failed* parse is benign — the defect is the absence of a range check on a *successful* one. That is narrower than it first appears, but it is still the only field in this reader with no bound.

## The same file already shows the correct pattern

The `DIMENSIONS` handler thirty lines earlier does exactly what is missing here — checks the conversion count *and* the value range before use:

https://github.com/InsightSoftwareConsortium/ITK/blob/main/Modules/IO/VTK/src/itkVTKImageIO.cxx#L220-L226

```cpp
long long signedDims[3]{};
if (sscanf(text.c_str(), "%*s %lld %lld %lld", signedDims, signedDims + 1, signedDims + 2) != 3 ||
signedDims[0] < 1 || signedDims[1] < 1 || signedDims[2] < 1)
{
itkExceptionMacro("Malformed DIMENSIONS line: " << text);
}
```

`SPACING` (L264), `ORIGIN` (L282), `VECTORS` (L302) and `TENSORS` (L384) are all guarded the same way. `COLOR_SCALARS` is the outlier.

## Suggested fix

Match the surrounding idiom. The VTK legacy format defines `COLOR_SCALARS` with `nValues` per scalar, and this reader maps only 1/3/4 onto SCALAR/RGB/RGBA with everything else falling through to VECTOR, so a modest ceiling is defensible:

```cpp
unsigned int numComp = 1;
if (sscanf(text.c_str(), "%*s %*s %u", &numComp) != 1 || numComp < 1)
{
itkExceptionMacro("Malformed COLOR_SCALARS line: " << text);
}
```

Whether to add an explicit upper bound, and what it should be, is a judgment call for the maintainers — `SCALARS` immediately below has the same open-ended behavior but defaults more defensively.

## Versions

ITK `main` (`af2fbc8`). Long-standing; not introduced by any recent change.

## Context

Found during a survey of header-field validation across ITK's IO modules, prompted by the review of #6761. Related: #6813 (`ITKIOBruker`), #6815 (`ITKIOStimulate`).

Contributor guide

Open the contributing guide

Research direction

Start in Modules/IO/VTK/src/itkVTKImageIO.cxx at the COLOR_SCALARS parsing around lines 315-316, then compare it with the validated DIMENSIONS handler around lines 220-226. Trace the value to SetNumberOfComponents on the ASCII and binary paths, and inspect the existing VTK IO tests. Done means malformed or excessive component counts are rejected before image sizing, with tests covering the behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.