From 39f20a913f90e3ef5b167b2dfaa07aec1f5f489a Mon Sep 17 00:00:00 2001 From: Matt McCormick Date: Thu, 16 Apr 2026 16:08:11 -0400 Subject: [PATCH] BUG: Backport GDCM CVE-2026-3650 fix Backport of GDCM PR https://github.com/malaterre/GDCM/pull/214 (commit 23bca9286a7efe8be97d67015aa280138fa8d4b1). A crafted DICOM file could specify an arbitrarily large Value Length field (up to ~4 GB), causing ByteValue::SetLength() to attempt a massive memory allocation before any stream data is read. This enables denial-of-service via memory exhaustion. Add stream-size validation in ExplicitDataElement::ReadValue(), ImplicitDataElement::ReadValue(), Fragment::ReadValue(), and Fragment::ReadBacktrack(). Before allocating a ByteValue, the code now compares the declared VL against the remaining bytes in the stream via tellg()/seekg(). Non-seekable streams skip the check gracefully. Also fix out-of-bounds array accesses in SequenceOfFragments where bv->GetLength() - N was used without verifying minimum length, affecting lines that use gdcmAssertAlwaysMacro (active in release). The TestCVE20263650 test added upstream is not included, as ITK's GDCM subtree does not vendor the Testing/ directory. --- .../gdcmExplicitDataElement.txx | 17 +++++++++++++ .../gdcmFragment.h | 24 +++++++++++++++++++ .../gdcmImplicitDataElement.txx | 17 +++++++++++++ .../gdcmSequenceOfFragments.h | 6 ++--- 4 files changed, 61 insertions(+), 3 deletions(-) diff --git a/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmExplicitDataElement.txx b/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmExplicitDataElement.txx index 2a307a6e47d..b62770516db 100644 --- a/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmExplicitDataElement.txx +++ b/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmExplicitDataElement.txx @@ -242,6 +242,23 @@ std::istream &ExplicitDataElement::ReadValue(std::istream &is, bool readvalues) { //gdcm_assert( TagField != Tag(0x7fe0,0x0010) ); ValueField = new ByteValue; + if( readvalues ) + { + const std::streampos cur = is.tellg(); + if( cur != std::streampos(-1) ) + { + is.seekg(0, std::ios::end); + const std::streampos end = is.tellg(); + is.seekg(cur); + if( end != std::streampos(-1) && is.good() + && static_cast(end - cur) < static_cast(ValueLengthField) ) + { + gdcmWarningMacro( "Value Length " << ValueLengthField + << " exceeds remaining stream size for tag " << TagField ); + throw Exception( "Value Length exceeds remaining stream size" ); + } + } + } } // We have the length we should be able to read the value this->SetValueFieldLength( ValueLengthField, readvalues ); diff --git a/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmFragment.h b/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmFragment.h index d0fc2d22775..5d88c7d9265 100644 --- a/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmFragment.h +++ b/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmFragment.h @@ -91,6 +91,18 @@ class GDCM_EXPORT Fragment : public DataElement { // Self SmartPointer bv = new ByteValue; + const std::streampos cur = is.tellg(); + if( cur != std::streampos(-1) ) + { + is.seekg(0, std::ios::end); + const std::streampos end = is.tellg(); + is.seekg(cur); + if( end != std::streampos(-1) && is.good() + && static_cast(end - cur) < static_cast(ValueLengthField) ) + { + throw Exception( "Fragment Value Length exceeds remaining stream size" ); + } + } bv->SetLength(ValueLengthField); if( !bv->Read(is) ) { @@ -144,6 +156,18 @@ class GDCM_EXPORT Fragment : public DataElement // Self SmartPointer bv = new ByteValue; + const std::streampos cur2 = is.tellg(); + if( cur2 != std::streampos(-1) ) + { + is.seekg(0, std::ios::end); + const std::streampos end2 = is.tellg(); + is.seekg(cur2); + if( end2 != std::streampos(-1) && is.good() + && static_cast(end2 - cur2) < static_cast(ValueLengthField) ) + { + throw Exception( "Fragment Value Length exceeds remaining stream size" ); + } + } bv->SetLength(ValueLengthField); if( !bv->Read(is) ) { diff --git a/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmImplicitDataElement.txx b/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmImplicitDataElement.txx index bca17f0fa43..c2daf57ddd5 100644 --- a/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmImplicitDataElement.txx +++ b/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmImplicitDataElement.txx @@ -215,6 +215,23 @@ std::istream &ImplicitDataElement::ReadValue(std::istream &is, bool readvalues) ValueLengthField = 202; // 0xca } #endif + if( !ValueLengthField.IsUndefined() && readvalues ) + { + const std::streampos cur = is.tellg(); + if( cur != std::streampos(-1) ) + { + is.seekg(0, std::ios::end); + const std::streampos end = is.tellg(); + is.seekg(cur); + if( end != std::streampos(-1) && is.good() + && static_cast(end - cur) < static_cast(ValueLengthField) ) + { + gdcmWarningMacro( "Value Length " << ValueLengthField + << " exceeds remaining stream size for tag " << TagField ); + throw Exception( "Value Length exceeds remaining stream size" ); + } + } + } // We have the length we should be able to read the value this->SetValueFieldLength( ValueLengthField, readvalues ); bool failed; diff --git a/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmSequenceOfFragments.h b/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmSequenceOfFragments.h index 5ee35f2ff76..a90cde1052e 100644 --- a/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmSequenceOfFragments.h +++ b/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmSequenceOfFragments.h @@ -167,7 +167,7 @@ std::istream& ReadValue(std::istream &is, bool /*readvalues*/) { gdcm_assert( Fragments.size() == 1 ); const ByteValue *bv = Fragments[0].GetByteValue(); - gdcm_assert( (unsigned char)bv->GetPointer()[ bv->GetLength() - 1 ] == 0xfe ); + gdcm_assert( bv->GetLength() >= 1 && (unsigned char)bv->GetPointer()[ bv->GetLength() - 1 ] == 0xfe ); // Yes this is an extra copy, this is a bug anyway, go fix YOUR code Fragments[0].SetByteValue( bv->GetPointer(), bv->GetLength() - 1 ); gdcmWarningMacro( "JPEG Fragment length was declared with an extra byte" @@ -188,7 +188,7 @@ std::istream& ReadValue(std::istream &is, bool /*readvalues*/) const size_t lastf = Fragments.size() - 1; const ByteValue *bv = Fragments[ lastf ].GetByteValue(); const char *a = bv->GetPointer(); - gdcmAssertAlwaysMacro( (unsigned char)a[ bv->GetLength() - 1 ] == 0xfe ); + gdcmAssertAlwaysMacro( bv->GetLength() >= 1 && (unsigned char)a[ bv->GetLength() - 1 ] == 0xfe ); Fragments[ lastf ].SetByteValue( bv->GetPointer(), bv->GetLength() - 1 ); is.seekg( -9, std::ios::cur ); gdcm_assert( is.good() ); @@ -212,7 +212,7 @@ std::istream& ReadValue(std::istream &is, bool /*readvalues*/) const size_t lastf = Fragments.size() - 1; const ByteValue *bv = Fragments[ lastf ].GetByteValue(); const char *a = bv->GetPointer(); - gdcmAssertAlwaysMacro( (unsigned char)a[ bv->GetLength() - 2 ] == 0xfe ); + gdcmAssertAlwaysMacro( bv->GetLength() >= 2 && (unsigned char)a[ bv->GetLength() - 2 ] == 0xfe ); Fragments[ lastf ].SetByteValue( bv->GetPointer(), bv->GetLength() - 2 ); is.seekg( -10, std::ios::cur ); gdcm_assert( is.good() );