Conversation
This reverts commit ff5886e, restoring the fix from 9d65a21 (PR malaterre#214). The revert was made because the fix regressed Testing/Data/SIEMENS_MAGNETOM-12-MONO2-GDCM12-VRUN.dcm, which left CVE-2026-3650 open on master. The next commit fixes that regression, so the original work can be restored rather than rewritten. Original fix by Matt McCormick; this commit only restores it. Co-authored-by: Matt McCormick <matt@fideus.io>
The fix for CVE-2026-3650 was reverted in ff5886e because it broke Testing/Data/SIEMENS_MAGNETOM-12-MONO2-GDCM12-VRUN.dcm, so the vulnerability is open on master today. Two things were wrong with it, and neither is the idea. 1. It threw Exception, not ParseException. Reader::InternalReadCommon only catches ParseException when it decides whether to retry a file with the CP246 / UN / VR16 / ExplicitImplicit readers. A plain Exception skips that whole chain and fails the read. The VRUN file needs the UN fallback -- its (0009,1214) carries a Value Length of 0x20202020, which is four ASCII spaces, and GDCM recovers by re-reading it as GDCM 1.x wrongly encoded. Throwing ParseException lets that recovery happen exactly as before. 2. It guarded the primary readers only. Explicit, Implicit and Fragment were covered; the four recovery readers were not, so the same unbounded allocation was still reachable through them -- and routing more files to the fallbacks makes that likelier, not less. Fuzzing after fixing (1) reached it immediately through ExplicitImplicitDataElement::ReadValue, 363 bytes in, 4.29GB requested. The recovery readers must not throw, though: they exist to make sense of files whose lengths are already wrong. They cap instead. A value can never be longer than the bytes that remain, so capping costs nothing that was readable, and ExplicitImplicitDataElement already uses that idiom when it reads a trailing element to end of stream. Both behaviours now live in one place, gdcmValueLengthCheck.h, rather than being repeated inline. The undefined-length sentinel 0xffffffff is excluded from both -- clamping it destroys the marker the parsers key off, which is the other way this file fails. Verified: - SIEMENS_MAGNETOM-12-MONO2-GDCM12-VRUN.dcm parses again, 246 elements, byte identical to the tag/VR/VL dump from master - three crafted files that requested 1.1GB, 1.16GB and 4.29GB are all refused - seven unrelated DICOM files parse byte identically to master Original fix by Matt McCormick; this commit is on top of a revert of the revert, so the attribution is preserved.
BasicOffsetTable::Read() sizes a ByteValue from the item length before
reading it, like the element readers, and was not among the sites the
original fix covered:
ERROR: libFuzzer: out-of-memory (malloc(4262453256))
malaterre#13 gdcm::ByteValue::SetLength gdcmByteValue.cxx:55
#14 gdcm::BasicOffsetTable::Read gdcmBasicOffsetTable.h:69
#15 gdcm::SequenceOfFragments::ReadPreValue gdcmSequenceOfFragments.h:129
malaterre#16 gdcm::SequenceOfFragments::Read gdcmSequenceOfFragments.h:89
367 bytes on disk, 4.26GB requested. Found by fuzzing with the rest of
the fix in place.
This reader already throws ParseException a few lines above, so the
checking form of the guard is the one that fits.
SIEMENS_MAGNETOM-12-MONO2-GDCM12-VRUN.dcm still parses to the same 246
elements, and the seven regression files are still byte identical.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The fix for CVE-2026-3650 (9d65a21) was reverted in ff5886e because it broke
so the unbounded allocation is open on master today. I think the idea was right and only the mechanics were wrong, in two places.
1. It threw
Exception, notParseExceptionReader::InternalReadCommoncatchesParseException— and onlyParseException— when deciding whether to retry a file with the CP246 / UN / VR16 / ExplicitImplicit readers (gdcmReader.cxx:496). A plainExceptionfalls through to the outer handler and fails the read outright.That's exactly what the VRUN file needs. Its
(0009,1214)carries a Value Length of538976288=0x20202020— four ASCII spaces — and GDCM recovers by falling back to the UN-explicit reader ("Attempt to read GDCM 1.X wrongly encoded",gdcmReader.cxx:547). The guard threw before that could happen.Throwing
ParseException(withSetLastElement, matching the idiom ~30 lines above inExplicitDataElement::ReadValue) lets the recovery run exactly as before.2. It guarded the primary readers only
Explicit,ImplicitandFragmentwere covered; the four recovery readers were not, so the same unbounded allocation was still reachable through them. Fixing (1) makes that more likely, not less, because more files now reach the fallbacks. Fuzzing after theParseExceptionchange hit it immediately:363 bytes on disk, 4.29 GB requested.
But the recovery readers must not throw — they exist to make sense of files whose lengths are already wrong, and rejecting there ends the chain (I tried it: the VRUN file fails again). They cap instead. A value can never be longer than the bytes that remain, so capping costs nothing that was readable, and
ExplicitImplicitDataElement::ReadValuealready uses that idiom when it reads a trailing element to end of stream.One more trap worth flagging
0xffffffffis the undefined-length sentinel, not a size. Both helpers exclude it explicitly — clamping it silently destroys the marker the parsers key off, and that is the other way this file stops parsing. It cost me a debugging round; leaving it in a comment so it doesn't cost the next person one.Shape of the change
Both behaviours live in one place,
gdcmValueLengthCheck.h, instead of being repeated inline at each reader:CheckValueLengthAgainstStream()— throwsParseException, for the primary readersClampValueLengthToStream()— caps, for the recovery readersCall sites:
ExplicitDataElement,ImplicitDataElement(check) andCP246Explicit,UNExplicit,VR16Explicit,ExplicitImplicit(clamp).Why this matters beyond GDCM
ITK pinned GDCM at
8f581c1b(2026-04-19) in InsightSoftwareConsortium/ITK#6090, which predates the revert, so ITK master and 5.4.x currently carry the version that throws. GDCM master carries the version that allocates. Whichever way you decide this, the two should agree — and this change is the version that lets them agree without either giving up the VRUN file. cc @thewtex @hjmjohnson, who may not have seen the revert.Verified
All four checked against this branch alone, on a clean tree, with
ASAN_OPTIONS=max_allocation_size_mb=64so any allocation over 64 MB is a hard failure — and verified that the same harness does flag the allocation on unpatched master, so the "refused" results aren't a broken test.TestCVE20263650.cxxcomes back with the reapply, so the regression test returns too.Happy to reshape any of this — in particular, if you'd rather the recovery readers throw and the VRUN file be handled some other way, say so and I'll redo it.