Repository navigation
BUG: Bound value allocation by stream size (CVE-2026-3650) (supersedes #233) - #238
Open
hjmjohnson wants to merge 1 commit into
Open
hjmjohnson wants to merge 1 commit into
hjmjohnson wants to merge 1 commit into
Conversation
Data element readers size the value buffer from the Value Length in the file, then read. A file declaring a 4 GB length makes GDCM allocate it before the read fails: a 9.6 KB file whose Pixel Data claims 0xfffffff0 bytes peaks at 8.4 GB resident in gdcminfo (23 MB with this change). The first fix (9d65a21) rejected any length longer than the bytes remaining, before the read. It was reverted (ff5886e) after it broke files GDCM has always recovered, and the reapplication in malaterre#233 still changes ten tests with gdcmData: TestWriter, TestPrinter1, TestReaderUpToTag1, TestFileChangeTransferSyntax1-4 and others. The recovery logic depends on what a failed read leaves behind. Reader picks its fallback parser from the exception type and the last element, and the "Incomplete Pixel Data" and "push the fragment anyway" tolerances keep a value whose length is the declared one. Throwing or clamping before the read changes all three. The Philips file ExplicitVRforPublicElementsImplicitVRforShadowElements.dcm shows it: its explicit misread of "SPI-" as a 760 MB length used to fail and trigger the Explicit/Implicit reader, and a clamped length lets the misparse succeed instead. A file with the last 4000 bytes of Pixel Data missing stopped converting for the same reason. Intervene only when the length exceeds the whole stream, which no read can satisfy. Any length that fits keeps the existing code path unchanged, so allocation stays bounded by the input size. When the length cannot fit, leave the stream as a short read would (at its end, failbit and eofbit set) and take the reader's existing failure branch without allocating. The fallback chain then sees what it always saw. Lengths of 1 MiB or less skip the check, avoiding a seek per element. Pixel Data keeps its "Incomplete Pixel Data" tolerance: a length that cannot fit is shortened to the bytes that remain, so a truncated file still loads with the same warning rather than allocating past its end. A rejected fragment reports a length of 0, because the JPEG 2000 and JPEG-LS decoders size their buffers from the declared fragment lengths. The CP246, UN, and VR16 readers now allocate only when values are read, as the other readers already did. Guarded: Explicit, Implicit (both read paths), ExplicitImplicit (both), CP246, UN, VR16, Fragment (both), the Basic Offset Table, and the File Meta Information reader. The JPEG trailing-byte fixups in SequenceOfFragments now check the fragment length before indexing it, since a rejected fragment arrives empty. Streams that cannot report their size (deflated transfer syntaxes, pipes) are not covered, as before. TestCVE20263650 exercises the Explicit, Implicit, and Fragment readers, truncated Pixel Data, and a length that overruns the remaining bytes but fits the stream. On Linux, outside sanitizers, it caps the address space, so the unfixed code fails it with "Impossible to allocate"; elsewhere it checks behavior only. Co-authored-by: Alan Karl <ahkarl13@gmail.com> Co-authored-by: Matt McCormick <matt@fideus.io>
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
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.
Fixes CVE-2026-3650 (allocation sized by an untrusted Value Length) without the regressions that got the first fix reverted. Supersedes #233; credit to @ahkarl13 and Matt McCormick, whose work this builds on.
A reader now intervenes only when a Value Length exceeds the whole stream, which no read can satisfy. It then fails exactly as a short read would, without allocating. Any length that fits keeps today's code path, so allocation stays bounded by the input size and GDCM's recovery of malformed files is untouched.
TestInvalidDICOMFiles)gdcminfogdcmconv --rawWhy #233 regressed
Reader::InternalReadCommonpicks its fallback parser from the exception type andGetLastElement(), and several readers keep a short Pixel Data value or push an incomplete fragment. All of that keys off what a failed read leaves behind. #233 throws or clamps before the read, so the fallback sees something different:ExplicitVRforPublicElementsImplicitVRforShadowElements.dcm: the explicit reader misreads the bytes"SPI-"after(0009,0012)as a 759,779,411-byte length in a 2,106,698-byte file. On master that read fails and the Explicit/Implicit fallback parses the file. Under CVE-2026-3650: reapply the fix, with the VRUN regression fixed #233 the fallback ends 89 lines into a 338-line dump.JPEGInvalidSecondFrag.dcm: the Fragment check still throws a plainException, which ends the fragment loop.TestWriter,TestPrinter1,TestReaderUpToTag1,TestReaderSelectedTags,TestFileChangeTransferSyntax1-4,TestFileAnonymizer4,TestEquipmentManufacturer. Bisected: all ten arrive with the reapply commit, and the follow-up does not clear them.These tests need gdcmData. A run without
Testing/Datainitialized shows 199 tests rather than 239 and misses all of them.What changed
gdcmValueLengthCheck.haddsValueLengthExceedsStream(is, value, vl). It returns true only for aByteValue(sequences do not allocate onSetLength) whose defined length exceeds the stream's total size. In that case it leaves the stream at its end withfailbit|eofbit, the state a short read leaves. Lengths up to 1 MiB skip the check, so ordinary elements do not pay a seek.Each guarded site then does what its own read-failure branch already does:
ParseExceptionwith*thisfor Explicit, ExplicitImplicit, CP246, UN, VR16, and Fragment;Exception("Should not happen (imp)")for the implicit readers; the existing assert for the Basic Offset Table;return falsefor the File Meta Information reader. Pixel Data keeps its "Incomplete Pixel Data" tolerance: a length that cannot fit is shortened to the bytes that remain, so a truncated file still loads with the same warning. A rejected fragment reports length 0, because the JPEG 2000 and JPEG-LS decoders size their buffers from declared fragment lengths. CP246, UN, and VR16 now allocate only when values are read (SetValueFieldLength), as the other readers already did, which closes theReadSelectedTags(..., false)path through the fallback readers.The three JPEG trailing-byte fixups in
SequenceOfFragmentscheck the fragment length before indexing it (from #233), because a rejected fragment arrives empty.Not covered, as before: streams that cannot report their size (deflated transfer syntaxes, pipes).
The test can fail
TestCVE20263650reads 0xfffffff0-length Explicit, Implicit, and Fragment elements (the fragment must report length 0), truncated Pixel Data (must load with a shortened length), a length that overruns the remaining bytes but fits the stream (old path kept), and a 2 MiB value followed by another element (stream position restored). On Linux, outside sanitizers, it caps the address space at current use plus 1 GiB, so on unfixed code the Explicit and Implicit cases fail withImpossible to allocateand the Fragment case with the wrong exception type. Elsewhere it checks behavior only. Measured: master with only the test added fails it; this branch passes.Verification
TestReaderCanRead, which now passesTestDICOMDIRGenerator1can fail underctest -jin a fresh build tree: it readsTestImageChangeTransferSyntax4's output with no CTest dependency, and adding a test shifts the schedule. The log shows it starting before its input exists; it passes run after its input. That is a pre-existing race, not part of this change.