Skip to content

Fix heap-buffer-overflow in _ctmStreamReadSTRING and stop parsing on string read errors - #2

Merged
Fedr merged 3 commits into
masterfrom
fix/ctmStreamReadSTRING-overflow
Sep 8, 2026
Merged

Fix heap-buffer-overflow in _ctmStreamReadSTRING and stop parsing on string read errors#2
Fedr merged 3 commits into
masterfrom
fix/ctmStreamReadSTRING-overflow

Conversation

@Fedr

@Fedr Fedr commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes a heap-buffer-overflow write reachable from the public ctmLoad() on a single crafted .ctm file (CWE-190 → CWE-787), reported privately by Kamal Sentassi of S9S Security Research under coordinated disclosure.

_ctmStreamReadSTRING() read a 32-bit length and called malloc(len + 1). For len = 0xFFFFFFFF the len + 1 wraps in 32-bit unsigned to 0malloc(0), after which up to len attacker-controlled bytes were read into that ~0-byte buffer.

Reachable via the file-comment string read in ctmLoadCustom (right after the header, for RAW/MG1/MG2), and again via UV/attribute-map name fields.

Fix

_ctmStreamReadSTRING (lib/stream.c)

  • Allocate (size_t)len + 1 so the + 1 cannot wrap.
  • Require the read to return exactly len bytes; on a short read (truncated/crafted file) free the buffer, null it, and fail with CTM_BAD_FORMAT.
  • Set CTM_OUT_OF_MEMORY on malloc failure instead of silently ignoring it.
  • Reject len == 0xFFFFFFFF explicitly, so the + 1 cannot wrap on a 32-bit size_t either (wasm32).
  • Return immediately if an earlier read already failed, so a paired mName/mFileName read does not proceed with a garbage length (_ctmStreamReadUINT ignores short reads and its buffer is uninitialized).

Callers (openctm.c, compressRAW.c, compressMG1.c, compressMG2.c)

  • Check mError after every _ctmStreamReadSTRING and stop decoding on failure (MG2 frees its temporary intUVCoords/intAttribs first). Previously the callers ignored the error and kept parsing.
  • ctmLoadCustom resets mError to CTM_NONE up front. This PR adds the library's first reads of mError, and only ctmGetError clears it, so without the reset a context reused after an un-acknowledged failure would abort the next, valid load.
  • ctmLoadCustom clears the mesh before returning on a rejected file comment, so ctmGetInteger does not report file-supplied counts alongside NULL arrays.

Verification

  • Before: ASan build via the public ctmLoad() on the reporter's 44-byte crash_min.ctmheap-buffer-overflow WRITE, allocation at stream.c:138, overflow at stream.c:141, chain ctmLoad → ctmLoadCustom (openctm.c:1236) → _ctmStreamReadSTRING.
  • After: the same file is rejected cleanly with no ASan report.
  • ASan harness against the final branch: crafted len = 0xFFFFFFFF and 0xFFFFFFFE, and a truncated comment, all return CTM_BAD_FORMAT with zero vertex/triangle counts and NULL arrays; a context reused after such a failure without calling ctmGetError then loads a valid file normally.
  • Regression: a valid mesh (vertices, triangles, and a file comment) still round-trips through ctmSave/ctmLoad.

Follow-up (not in this PR)

Secondary, lower-severity items the reporter flagged: file-controlled vertex/triangle/UV/attrib counts drive malloc directly (openctm.c:~1239/1245/1254), enabling large-allocation OOM, plus an error-path leak. To be addressed separately.

…erflow

A crafted .ctm file with a string length of 0xFFFFFFFF made malloc(len + 1)
wrap in 32-bit to malloc(0); the subsequent read then wrote up to len
attacker-controlled bytes past the 1-byte allocation. Reachable from the
public ctmLoad() through the file-comment field (and later UV/attribute name
fields) for all methods.

Allocate with size_t so len + 1 cannot wrap, and require the read to return
exactly len bytes, otherwise reject the file as CTM_BAD_FORMAT. A failed
malloc now sets CTM_OUT_OF_MEMORY instead of being silently ignored.

Reported by Kamal Sentassi, S9S Security Research (coordinated disclosure).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Fedr and others added 2 commits September 7, 2026 23:05
The size_t cast alone does not stop the wrap on a 32-bit size_t target
(e.g. the Emscripten/wasm32 build), where (size_t)len + 1 still wraps to
0 for len == 0xFFFFFFFF. Reject that length outright before allocating.

Callers previously ignored the error _ctmStreamReadSTRING sets: after a
rejected string, ctmLoadCustom and the RAW/MG1/MG2 decoders kept reading,
so mError could be overwritten and the file processed further on garbage.
Return early after each string read (freeing the temp int buffers in MG2).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…g after a failure

ctmLoadCustom now reads mError, so a stale error left by a previous call
(without ctmGetError) would otherwise abort a valid load; reset it up front.
Clear the mesh counts before returning on a rejected file comment so
ctmGetInteger does not report file-supplied counts with NULL arrays.
_ctmStreamReadSTRING returns immediately if an earlier read already failed,
so the paired mName/mFileName reads do not proceed with a garbage length.
@Fedr Fedr changed the title Fix heap-buffer-overflow in _ctmStreamReadSTRING via 32-bit length overflow Fix heap-buffer-overflow in _ctmStreamReadSTRING and stop parsing on string read errors Sep 8, 2026
@Fedr
Fedr merged commit 32135db into master Sep 8, 2026
@Fedr
Fedr deleted the fix/ctmStreamReadSTRING-overflow branch September 8, 2026 08:11
Fedr added a commit that referenced this pull request Sep 8, 2026
#3)

Follow-up to #2 from the same S9S disclosure: file-declared vertex/triangle counts drove malloc directly, so count * stride wrapped to an undersized buffer on 32-bit size_t (wasm32) and in the 32-bit CTMuint products of the float-map and stream.c temp buffers. Counts above UINT_MAX / 16 (largest stride) are now rejected with CTM_BAD_FORMAT; the float-map size is computed in size_t. Also frees the MG1 temporary indices buffer on a failed packed-int read.
Fedr added a commit that referenced this pull request Sep 8, 2026
…#4)

Fork point (upstream v1.0.3), library-only scope, CMake build and export macro, ctmRearrangeTriangles, ctmSaveCustom progress/cancel callback, empty-mesh support, noexcept in the C++ wrapper, the #2/#3 loader hardening, and file-format compatibility notes. README.txt is kept verbatim as the upstream change log.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants