InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

MetaIO: ten memory-safety defects reachable from a malformed header (Kitware/MetaIO#150)

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

Fuzzing the vendored MetaIO found nine memory-safety defects and one unbounded allocation, all reachable from a malformed `.mha`/`.mhd` header. Full analysis, per-finding line numbers, reproducers, and the harness are in **Kitware/MetaIO#150**; this issue tracks the ITK side.

**Parked pending funding.** Filed so the work is recoverable rather than lost.

## Why this matters for ITK

`ITKIOMeta` is a default-enabled module and `MetaImageIO` is a first-class reader, so every one of these is reachable through `itk::ImageFileReader` on a file a user obtained from a scanner, a collaborator, or a public archive. The smallest reproducer is a **93-byte text header**; none require a crafted binary payload.

Two of the ten are heap-buffer-overflow **writes** with attacker-controlled length and content.

## Status

Eight of the ten have fixes written and verified locally, on an unpushed branch. Two need a decision before they can be fixed:

- **Finding 5** — `ElementType` sizes a buffer that the read loops consume as `float`, in five point-list readers. Fixing it either changes which existing files remain readable, or leaves `ElementType` decorative for those types. Compatibility call.
- **Finding 10** — a ~100-byte header requests 17 GB, and the `if (m_ElementData == nullptr)` guard cannot fire because `new` throws. There is no defensible constant cap, since large images are legitimate.

## Relationship to other issues

- **#6818** — this is the same class as the four hand-audited defects that motivated it, and a direct test of its thesis. Hand-auditing MetaIO for hours found nothing; a fuzzer found the first heap overflow in **90 seconds**, and eight more over five rounds. MetaIO is a reasonable first target for the CI leg proposed there: 28k lines, self-contained, no ITK dependency, builds standalone in under a minute.
- **#6694** — re-vendoring is already tracked. Any MetaIO fixes land upstream first and reach ITK through `Modules/ThirdParty/MetaIO/UpdateFromUpstream.sh`, so that path is the delivery mechanism here too.
- **#5084** — OpenSSF Scorecard *Fuzzing: 0*. The harness in MetaIO#150 is a working starting point.

What the existing safety nets did and did not catch

| Check | Result |
|---|---|
| MetaIO's 15 tests under ASan + UBSan | **clean** — they only feed well-formed files |
| `-Wall -Wextra` | 3 warnings, none related |
| CodeQL `cpp-security-and-quality` (159 results) | **missed findings 1-9** |
| libFuzzer + ASan/UBSan, 5 rounds | **found all 10** |

CodeQL missing them is worth understanding rather than treating as a tooling failure. The unbounded copy loop has no constant to compare a length against, and the `strncat` misuse is syntactically well-formed — the wrong argument is a plausible-looking named constant in the wrong position. CodeQL did independently flag finding 10, plus four genuine `new[]`/scalar-`delete` mismatches in `metaTransform.cxx`. The tools are complementary; neither substitutes for the other.

Re-fuzzing after each fix was necessary, not thorough

Findings arrived over five rounds because fixes changed what was reachable:

- Finding 6 exists *because* finding 3's first fix bounded `length` where the matrix path consumes `length²` — a correct-looking bound that was dimensionally wrong, which the fuzzer walked into within 35,000 executions.
- Finding 9 only surfaced once earlier crashes stopped terminating inputs first.
- Coverage rose 1713 → 2084 blocks across the rounds as fixes unblocked deeper paths.

Round 5 found no new memory corruption in ~115,000 executions per worker. That is the yield beginning to flatten, not evidence the code is clean.

Picking this back up

Local state is a git worktree on branch `harden` off the MetaIO PR #149 branch: eight fixes, a `testMeta16MalformedHeader` regression test covering seven reduced cases (verified to fail without the fixes), libFuzzer harnesses for `MetaImage` and `MetaScene`, an evolved corpus, and a CodeQL database. Unpushed.

Everything needed to rebuild from scratch is in MetaIO#150 — harness source, build flags, and the reproducers as `printf` commands — so none of this depends on that local tree surviving.

Order of work when resumed: settle findings 5 and 10, land one upstream MetaIO PR stacked on Kitware/MetaIO#149, then re-run `UpdateFromUpstream.sh` here. The standing CI leg from #6818 is the thing that keeps it from regressing.

Contributor guide

Open the contributing guide

Research direction

Start with Kitware/MetaIO#150, including its reproducers and libFuzzer harnesses, then review the MetaIO changes and the open decisions for findings 5 and 10. The delivery path is Modules/ThirdParty/MetaIO/UpdateFromUpstream.sh after an upstream fix lands. Done means resolving both decisions, landing the remaining MetaIO fixes upstream, and re-vendoring them into ITK.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
security, testing
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.