Skip to content

Commit 39f20a9

Browse files
committed
BUG: Backport GDCM CVE-2026-3650 fix
Backport of GDCM PR malaterre/GDCM#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.
1 parent 8505c1a commit 39f20a9

4 files changed

Lines changed: 61 additions & 3 deletions

File tree

Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmExplicitDataElement.txx

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,23 @@ std::istream &ExplicitDataElement::ReadValue(std::istream &is, bool readvalues)
242242
{
243243
//gdcm_assert( TagField != Tag(0x7fe0,0x0010) );
244244
ValueField = new ByteValue;
245+
if( readvalues )
246+
{
247+
const std::streampos cur = is.tellg();
248+
if( cur != std::streampos(-1) )
249+
{
250+
is.seekg(0, std::ios::end);
251+
const std::streampos end = is.tellg();
252+
is.seekg(cur);
253+
if( end != std::streampos(-1) && is.good()
254+
&& static_cast<uint64_t>(end - cur) < static_cast<uint32_t>(ValueLengthField) )
255+
{
256+
gdcmWarningMacro( "Value Length " << ValueLengthField
257+
<< " exceeds remaining stream size for tag " << TagField );
258+
throw Exception( "Value Length exceeds remaining stream size" );
259+
}
260+
}
261+
}
245262
}
246263
// We have the length we should be able to read the value
247264
this->SetValueFieldLength( ValueLengthField, readvalues );

Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmFragment.h

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,18 @@ class GDCM_EXPORT Fragment : public DataElement
9191
{
9292
// Self
9393
SmartPointer<ByteValue> bv = new ByteValue;
94+
const std::streampos cur = is.tellg();
95+
if( cur != std::streampos(-1) )
96+
{
97+
is.seekg(0, std::ios::end);
98+
const std::streampos end = is.tellg();
99+
is.seekg(cur);
100+
if( end != std::streampos(-1) && is.good()
101+
&& static_cast<uint64_t>(end - cur) < static_cast<uint32_t>(ValueLengthField) )
102+
{
103+
throw Exception( "Fragment Value Length exceeds remaining stream size" );
104+
}
105+
}
94106
bv->SetLength(ValueLengthField);
95107
if( !bv->Read<TSwap>(is) )
96108
{
@@ -144,6 +156,18 @@ class GDCM_EXPORT Fragment : public DataElement
144156

145157
// Self
146158
SmartPointer<ByteValue> bv = new ByteValue;
159+
const std::streampos cur2 = is.tellg();
160+
if( cur2 != std::streampos(-1) )
161+
{
162+
is.seekg(0, std::ios::end);
163+
const std::streampos end2 = is.tellg();
164+
is.seekg(cur2);
165+
if( end2 != std::streampos(-1) && is.good()
166+
&& static_cast<uint64_t>(end2 - cur2) < static_cast<uint32_t>(ValueLengthField) )
167+
{
168+
throw Exception( "Fragment Value Length exceeds remaining stream size" );
169+
}
170+
}
147171
bv->SetLength(ValueLengthField);
148172
if( !bv->Read<TSwap>(is) )
149173
{

Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmImplicitDataElement.txx

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,23 @@ std::istream &ImplicitDataElement::ReadValue(std::istream &is, bool readvalues)
215215
ValueLengthField = 202; // 0xca
216216
}
217217
#endif
218+
if( !ValueLengthField.IsUndefined() && readvalues )
219+
{
220+
const std::streampos cur = is.tellg();
221+
if( cur != std::streampos(-1) )
222+
{
223+
is.seekg(0, std::ios::end);
224+
const std::streampos end = is.tellg();
225+
is.seekg(cur);
226+
if( end != std::streampos(-1) && is.good()
227+
&& static_cast<uint64_t>(end - cur) < static_cast<uint32_t>(ValueLengthField) )
228+
{
229+
gdcmWarningMacro( "Value Length " << ValueLengthField
230+
<< " exceeds remaining stream size for tag " << TagField );
231+
throw Exception( "Value Length exceeds remaining stream size" );
232+
}
233+
}
234+
}
218235
// We have the length we should be able to read the value
219236
this->SetValueFieldLength( ValueLengthField, readvalues );
220237
bool failed;

Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmSequenceOfFragments.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ std::istream& ReadValue(std::istream &is, bool /*readvalues*/)
167167
{
168168
gdcm_assert( Fragments.size() == 1 );
169169
const ByteValue *bv = Fragments[0].GetByteValue();
170-
gdcm_assert( (unsigned char)bv->GetPointer()[ bv->GetLength() - 1 ] == 0xfe );
170+
gdcm_assert( bv->GetLength() >= 1 && (unsigned char)bv->GetPointer()[ bv->GetLength() - 1 ] == 0xfe );
171171
// Yes this is an extra copy, this is a bug anyway, go fix YOUR code
172172
Fragments[0].SetByteValue( bv->GetPointer(), bv->GetLength() - 1 );
173173
gdcmWarningMacro( "JPEG Fragment length was declared with an extra byte"
@@ -188,7 +188,7 @@ std::istream& ReadValue(std::istream &is, bool /*readvalues*/)
188188
const size_t lastf = Fragments.size() - 1;
189189
const ByteValue *bv = Fragments[ lastf ].GetByteValue();
190190
const char *a = bv->GetPointer();
191-
gdcmAssertAlwaysMacro( (unsigned char)a[ bv->GetLength() - 1 ] == 0xfe );
191+
gdcmAssertAlwaysMacro( bv->GetLength() >= 1 && (unsigned char)a[ bv->GetLength() - 1 ] == 0xfe );
192192
Fragments[ lastf ].SetByteValue( bv->GetPointer(), bv->GetLength() - 1 );
193193
is.seekg( -9, std::ios::cur );
194194
gdcm_assert( is.good() );
@@ -212,7 +212,7 @@ std::istream& ReadValue(std::istream &is, bool /*readvalues*/)
212212
const size_t lastf = Fragments.size() - 1;
213213
const ByteValue *bv = Fragments[ lastf ].GetByteValue();
214214
const char *a = bv->GetPointer();
215-
gdcmAssertAlwaysMacro( (unsigned char)a[ bv->GetLength() - 2 ] == 0xfe );
215+
gdcmAssertAlwaysMacro( bv->GetLength() >= 2 && (unsigned char)a[ bv->GetLength() - 2 ] == 0xfe );
216216
Fragments[ lastf ].SetByteValue( bv->GetPointer(), bv->GetLength() - 2 );
217217
is.seekg( -10, std::ios::cur );
218218
gdcm_assert( is.good() );

0 commit comments

Comments
 (0)