InsightSoftwareConsortium / InsightSoftwareConsortium/ITK
VTK legacy reader: COLOR_SCALARS component count is used unvalidated
- 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
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