From 6c821525ea48a6e8c04ee09cf59adebdcab1d02a Mon Sep 17 00:00:00 2001 From: Vasyl Horbatenko Date: Thu, 7 Sep 2023 13:07:09 +0300 Subject: [PATCH 1/2] Fix read access violation when length variable is overflowed It was possible to overflow length in "length -= 4 + pdvLength;" when pdvLength is set to a value greater then LONG_MAX + pduLength. By doing that the length overflowed and became positive, even if it was initially less than 4 + pdvLength. --- dcmnet/libsrc/dulfsm.cc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/dcmnet/libsrc/dulfsm.cc b/dcmnet/libsrc/dulfsm.cc index b932fbef7..c05516b50 100644 --- a/dcmnet/libsrc/dulfsm.cc +++ b/dcmnet/libsrc/dulfsm.cc @@ -1413,19 +1413,22 @@ DT_2_IndicatePData(PRIVATE_NETWORKKEY ** /*network*/, p = (*association)->fragmentBuffer; //set p to the buffer which contains the PDU's PDVs while (length >= 4) { //as long as length is at least 4 (= a length field can be read) EXTRACT_LONG_BIG(p, pdvLength); //determine the length of the current PDV (the PDV p points to) - p += 4 + pdvLength; //move p so that it points to the next PDV (move p 4 bytes over the length field plus the amount of bytes which is captured in the PDV's length field (over presentation context.Id, message information header and data fragment)) - length -= 4 + pdvLength; //update length (i.e. determine the length of the buffer which has not been evaluated yet.) - pdvCount++; //increase counter by one, since we've found another PDV // There must be at least a presentation context ID and a message // control header (see below), else the calculation pdvLength - 2 below // will underflow. - if (pdvLength < 2) + // Check that pdvLength will not overflow ULONG_MAX with additional 4 bytes. + // Check that pdvLength + additional 4 bytes is less than remaining length. + if (pdvLength < 2 || ULONG_MAX - pdvLength < 4 || length < 4 + pdvLength) { char buf[256]; sprintf(buf, "PDV with invalid length %lu encountered. This probably indicates a malformed P DATA PDU.", pdvLength); return makeDcmnetCondition(DULC_ILLEGALPDULENGTH, OF_error, buf); } + + p += 4 + pdvLength; //move p so that it points to the next PDV (move p 4 bytes over the length field plus the amount of bytes which is captured in the PDV's length field (over presentation context.Id, message information header and data fragment)) + length -= 4 + pdvLength; //update length (i.e. determine the length of the buffer which has not been evaluated yet.) + pdvCount++; //increase counter by one, since we've found another PDV } /* if after having counted the PDVs the length variable does not equal */ From 3fe859450562671f2c0ef559c2e749e084741df4 Mon Sep 17 00:00:00 2001 From: Vasyl Horbatenko Date: Thu, 7 Sep 2023 13:23:25 +0300 Subject: [PATCH 2/2] Make length unsigned long instead of long. By doing this we avoid warning '<': signed/unsigned mismatch. This can also prevent problems when pduLength is greater than long. --- dcmnet/libsrc/dulfsm.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dcmnet/libsrc/dulfsm.cc b/dcmnet/libsrc/dulfsm.cc index c05516b50..2fedb9b1f 100644 --- a/dcmnet/libsrc/dulfsm.cc +++ b/dcmnet/libsrc/dulfsm.cc @@ -1387,8 +1387,7 @@ DT_2_IndicatePData(PRIVATE_NETWORKKEY ** /*network*/, unsigned long pduLength, pdvLength, - pdvCount; - long + pdvCount, length; unsigned char *p;