Memory-safety defects reachable from a malformed header (fuzzing + sanitizers)

[hjmjohnson]

Fuzzing MetaImage::Read and MetaScene::Read under AddressSanitizer/UndefinedBehaviorSanitizer found nine memory-safety defects and one unbounded allocation, all reachable from a malformed header. Every case below is confirmed against 11606dc with a reproducer under 300 bytes.

Filing as one issue because the reproduction setup is shared; each finding is independent and can be split out if that is easier to work through. This is parked pending funding — no fix PR is coming imminently, so it is available for anyone who wants it.

Not a duplicate of #66 (static-analysis dead stores) or #114 (% in an output path); #114 is adjacent, since several findings live in the % branch of the read path.

Findings

#	Defect	Site (11606dc)
1	Unbounded copy into an 80-byte word buffer	metaUtils.cxx:1006
2	strncat size argument is bytes-to-append, not buffer size	metaImage.cxx:1452,1453,3078,3079
3	File-controlled array length indexes value[4096]	metaUtils.cxx:1340,1371,1407
4	NDims unvalidated → m_DimSize[-1], divide-by-zero	metaObject.cxx:1520, metaImage.cxx:1410,1418,1423,1462
5	ElementType sizes a buffer the loops read as float	metaLine.cxx:292,435 + Tube/Surface/Blob/Landmark
6	Matrix field consumes length², bounded as length	metaUtils.cxx:1408,1423
7	size_t underflow on an empty line	metaImage.cxx:1358
8	Negative CompressedDataSize reaches new[]	metaImage.cxx:2637
9	MET_StringToWordArray fails with a half-built array; all 9 callers ignore the return	metaUtils.cxx:993
10	Unbounded allocation; the null check cannot fire	metaImage.cxx:565

1, 2 — heap-buffer-overflow WRITE in the word splitter

MET_StringToWordArray allocates a fixed 80-byte buffer per word and copies without a bound:

(*val)[i] = new char[METAIO_MAX_WORD_SIZE];
while (p < l && s[p] != ' ')
{
  (*val)[i][j++] = s[p++];      // no bound on j
}
(*val)[i][j] = '\0';

Any token over 79 characters writes attacker-controlled bytes, of attacker-controlled length, past the allocation.

Separately, the filename-reconstruction path joins words into that same buffer:

std::strncat(wrds[0], " ", METAIO_MAX_WORD_SIZE);
std::strncat(wrds[0], wrds[i], METAIO_MAX_WORD_SIZE);

strncat's third argument is the maximum number of bytes to append, not the size of the destination, so this appends up to 80 bytes onto an already-80-byte buffer, in a loop.

Both need % in ElementDataFile to reach the branch, which is why the existing suite never hit them.

3, 6 — file-controlled length indexes a fixed value array

Three sites take an array length straight from the file:

(*fieldIter)->length = static_cast<int>((*fields)[(*fieldIter)->dependsOn]->value[0]);
for (j = 0; j < static_cast<size_t>((*fieldIter)->length); j++)
{
  readFloatValue(fp, (*fieldIter)->value[j]);   // value is double[4096]
}

The double→int cast is itself UB for a large value, and nothing bounds the result against MET_MAX_NUMBER_OF_FIELD_VALUES. metaUtils.h:403 already clamps against that constant, so the invariant is known — the read path just does not enforce it.

MET_FLOAT_MATRIX is a distinct case: it consumes length * length values from the same array, so its bound is on the square. A length of 4096 means 16,777,216 writes into 4096 slots. The pre-specified-length branches (the else arms) have the same exposure as the dependsOn ones.

4, 7, 8 — index and size arithmetic

NDims is stored from the file with no range check, while the dimension-indexed members are declared [10]. NDims = 0 yields m_DimSize[m_NDims - 1] → m_DimSize[-1], at several sites. A stepV of 0 in the same branch divides by zero.

j is declared size_t, so an empty line underflows:

j = s.length() - 1;                                    // empty line -> SIZE_MAX
while (j > 0 && (isspace(s[j]) || !isprint(s[j])))     // s[SIZE_MAX]

Short strings live in the SSO buffer, so this reads off the stack. The isspace/isprint arguments are also plain char, which is UB for negative values.

CompressedDataSize = -425 from the header reaches new unsigned char[static_cast<size_t>(...)] as 0xfffffffffffffe57.

5 — ElementType sizes a buffer the loops read as float (not yet fixed)

int readSize = m_NPoints * (m_NDims * m_NDims + 4) * elementSize;   // metaLine.cxx:292
char * _data = new char[readSize];
...
for (k = 0; k < sizeof(float); k++) { num[k] = _data[i + k]; }      // always 4 bytes

elementSize comes from the file's ElementType, but every read loop hardcodes sizeof(float). ElementType = MET_UCHAR gives a buffer a quarter of what the loop reads. Clear() sets m_ElementType = MET_FLOAT, so this only misbehaves when a file overrides it. The same shape is in metaTube, metaSurface, metaBlob and metaLandmark, on both read and write paths.

Two ways to resolve, and the choice is a compatibility question rather than a mechanical fix: size by sizeof(float) to match what the code does, or reject a non-float ElementType for point-list types. Doing both seems right, but it changes which existing files remain readable.

9 — a bool return that nine callers ignore

*val = new char *[*n];
for (i = 0; i < *n; i++)
{
  if (p == l)
  {
    return false;      // entries i..*n-1 never assigned; earlier ones leak;
  }                    // *n still claims they exist
  ...
}

Callers then walk to *n and strlen(wrds[i]) on an uninitialized pointer. Every call site discards the return value:

metaBlob.cxx:275   metaDTITube.cxx:407   metaContour.cxx:381,582
metaLine.cxx:277   metaLandmark.cxx:275
metaImage.cxx:1323,1414,2975,3070

Nine callers over many years all ignoring the bool suggests the API shape is the defect. Making the failure path free what it allocated and set *val = nullptr, *n = 0 turns six of the nine into correct no-ops without touching them, and removes the leak; the sites that dereference wrds[0] unconditionally still need a check. [[nodiscard]] would catch the family at compile time.

10 — unbounded allocation with an unreachable error path (not yet fixed)

m_ElementData = new char[static_cast<size_t>(m_Quantity * m_ElementNumberOfChannels * i)];
if (m_ElementData == nullptr)          // never true: new throws
{
  std::cerr << "MetaImage:: M_Allocate:: Insufficient memory" << '\n';
  return false;
}

A ~100-byte header requests 17 GB. The null check cannot fire, so the intended error path is dead and std::bad_alloc escapes to the caller. CodeQL flags this same line as cpp/incorrect-allocation-error-handling.

This is denial-of-service rather than corruption, and there is no defensible constant cap, since large images are legitimate. A consistency check against the bytes actually available, plus a real failure path, seems like the shape of a fix — but it deserves its own discussion.

Reproducing

Reproducers are attached below as a shell script that writes each file; all are confirmed against 11606dc.

Build and run

git clone https://github.com/Kitware/MetaIO.git && cd MetaIO
cmake -G Ninja -S . -B b -DCMAKE_BUILD_TYPE=RelWithDebInfo -DBUILD_TESTING=OFF \
  -DBUILD_SHARED_LIBS=OFF \
  -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ \
  -DCMAKE_C_FLAGS="-fsanitize=address,undefined -g" \
  -DCMAKE_CXX_FLAGS="-fsanitize=address,undefined -g"
ninja -C b

cat > readone.cxx <<'EOF'
#include <iostream>
#include <metaImage.h>
int main(int argc, char * argv[])
{
  if (argc < 2) { std::cerr << "usage: readone <file.mha>\n"; return 2; }
  MetaImage image;
  std::cout << (image.Read(argv[1], true) ? "accepted" : "rejected") << '\n';
  return 0;
}
EOF
clang++ -std=c++17 -g -O1 -fsanitize=address,undefined \
  -I . -I src -I b -I b/src readone.cxx b/src/libMetaIO.a b/itkzlib/libitkzlib.a -o b/readone

for f in cases/*.mha; do echo "== $f"; ASAN_OPTIONS=detect_leaks=0 ./b/readone "$f"; done

The cases

mkdir -p cases
# 1 — heap overflow: a token past the 80-byte word buffer
printf 'ObjectType = Image\nNDims = 2\nDimSize = 4 4\nElementType = MET_UCHAR\nElementDataFile = %%03d%s 1 4 1\n' \
  "$(printf 'A%.0s' $(seq 200))" > cases/b1-long-word.mha

# 2 — heap overflow: strncat joins many words into that same buffer
printf 'ObjectType = Image\nNDims = 2\nDimSize = 4 4\nElementType = MET_UCHAR\nElementDataFile = %%03d seg00000000 seg11111111 seg22222222 seg33333333 seg44444444 seg55555555 seg66666666 seg77777777 seg88888888 1 4 1\n' \
  > cases/b2-strncat-join.mha

# 4 — m_DimSize[-1]
printf 'ObjectType = Image\nNDims = 0\nDimSize = 4 4\nElementType = MET_UCHAR\nElementDataFile = %%03d 1 4 1\n' \
  > cases/b4-ndims-zero.mha

# 6 — matrix field writes past value[4096]
printf 'ObjectType = Image\nNDims = 4096\nTransformMatrix = 1 0 0 1\nDimSize = 4 4\nElementType = MET_UCHAR\nElementDataFile = LOCAL\n' \
  > cases/b6-matrix-oversize.mha

# 7 — size_t underflow on a blank LIST line
printf 'ObjectType = Image\nNDims = 2\nDimSize = 4 4\nElementType = MET_UCHAR\nElementDataFile = LIST\n\n\n\n' \
  > cases/b7-empty-list-line.mha

# 8 — negative CompressedDataSize reaches new[]
printf 'ObjectType = Image\nNDims = 2\nDimSize = 4 4\nElementType = MET_UCHAR\nCompressedData = True\nCompressedDataSize = -425\nElementDataFile = LOCAL\n' \
  > cases/b8-negative-compressed.mha

# 10 — 17 GB allocation request from a 105-byte header
printf 'ObjectType = Image\nNDims = 3\nDimSize = 99999 99999 99999\nElementType = MET_UCHAR\nElementDataFile = LOCAL\n' \
  > cases/b10-oom-dimsize.mha

Expected on 11606dc: 1, 2 report heap-buffer-overflow; 4 reports index -1 out of bounds for type 'int[10]'; 6 reports index 4097 out of bounds for type 'double[4096]'; 7 reports an unsigned-offset error; 8 and 10 report oversized allocations. Finding 5 needs a MetaScene/MetaLine file rather than a MetaImage one; finding 9 was reached by the fuzzer through MetaScene::Read.

Fuzz harness

libFuzzer, writing each input to a scratch file since ReadStream takes an ifstream* rather than a generic istream. Build the library with -fsanitize=fuzzer-no-link,address,undefined and link the harness with -fsanitize=fuzzer,address,undefined.

#include <cstdint>
#include <cstdio>
#include <cstdlib>
#include <string>
#include <metaImage.h>

static const char * kPath = "/tmp/metaio-fuzz-case.mha";

extern "C" int
LLVMFuzzerTestOneInput(const uint8_t * data, size_t size)
{
  if (size == 0 || size > (1u << 20)) { return 0; }
  FILE * f = fopen(kPath, "wb");
  if (f == nullptr) { return 0; }
  fwrite(data, 1, size, f);
  fclose(f);

  MetaImage image;
  if (image.Read(kPath, true))
  {
    const int dims = image.NDims();
    for (int i = 0; i < dims && i < 10; ++i)
    {
      (void)image.DimSize(i);
      (void)image.ElementSpacing(i);
      (void)image.Origin(i);
    }
  }
  return 0;
}

Run with -close_fd_mask=2 to suppress MetaIO's parse chatter without losing libFuzzer's own reports — reopening stderr inside the harness swallows both. Seed from the .mha/.meta files the existing test suite generates.

The same harness against MetaScene::Read reaches every object reader (Tube, Mesh, Blob, Surface, Landmark, Contour, Line, FEMObject) through the dispatcher, which is how finding 5 surfaced; one harness there is worth more than eight separate ones.

Notes on method

Findings arrived over five fuzzing rounds, re-running after each fix. That mattered: finding 6 exists because finding 3's first fix bounded length rather than length², and finding 9 only became reachable once earlier crashes stopped terminating inputs first. Coverage went 1713 → 2084 blocks across the rounds as fixes unblocked deeper paths; round 5 produced no new memory-corruption findings in ~115,000 executions per worker, which is the first sign of the yield flattening rather than a guarantee the code is clean.

The existing 15 tests pass cleanly under ASan/UBSan, and -Wall -Wextra produces only 3 warnings — this class of bug is invisible to both, because every case needs malformed input to reach it.

CodeQL's cpp-security-and-quality suite (159 results) did not find findings 1-9. The unbounded loop has no constant to compare against, and the strncat misuse is syntactically well-formed with a plausible named constant in the wrong position. It did independently flag finding 10, and four genuine new[]/delete mismatches in metaTransform.cxx (lines 57, 84, 411, 487 — parameters is new double[] but freed with scalar delete), which are worth fixing regardless.