Fix extNegList leaks on A-ASSOCIATE parse failure.

Applied-Upstream: 23f181f7a3cb8334056f751a3a0c2ddf01046752
Last-Update: 2026-05-26
Reviewed-By: Étienne Mollier <emollier@debian.org>
Bug-Debian: https://bugs.debian.org/1141411

Thanks for the report and analysis to Abhinav Agarwal.

This closes DCMTK feature #126.

Gbp-Pq: Name CVE-2026-50254.patch

diff --git a/dcmnet/libsrc/dulconst.cc b/dcmnet/libsrc/dulconst.cc
index 063cf5aba37550c642dcb6a92c5c53a2ed45e60e..d7f17c923678ea1e3de79c9f39c6ef63574fbff2 100644 (file)
--- a/dcmnet/libsrc/dulconst.cc
+++ b/dcmnet/libsrc/dulconst.cc
@@ -1,6 +1,6 @@
 /*
  *
- *  Copyright (C) 1994-2024, OFFIS e.V.
+ *  Copyright (C) 1994-2026, OFFIS e.V.
  *  All rights reserved.  See COPYRIGHT file for details.
  *
  *  This software and supporting documentation were partly developed by
@@ -1087,6 +1087,14 @@ constructExtNeg(unsigned char type,
     unsigned long length;
     *rtnLength = 0;
 
+    /* The transient list built here ends up as userInfo->extNegList of the
+     * outgoing PDU; via the shallow appendList() it borrows the sub-item
+     * pointers from the params-owned list. destroyUserInformationLists()
+     * (called once the PDU has been streamed) releases only the container,
+     * leaving the items in place for DUL_ClearServiceParameters() to free
+     * through the params side. See the ownership note in
+     * destroyUserInformationLists() (helpers.cc).
+     */
     if (type == DUL_TYPEASSOCIATERQ && params->requestedExtNegList != NULL) {
         *lst = new SOPClassExtendedNegotiationSubItemList;
         if (*lst == NULL) return EC_MemoryExhausted;

diff --git a/dcmnet/libsrc/dulfsm.cc b/dcmnet/libsrc/dulfsm.cc
index 060361eccfb7478521402b3ea4bcaf93afa399b6..00ee6444e7c482ceff543738773f9f194cc2295e 100644 (file)
--- a/dcmnet/libsrc/dulfsm.cc
+++ b/dcmnet/libsrc/dulfsm.cc
@@ -1,6 +1,6 @@
 /*
  *
- *  Copyright (C) 1994-2025, OFFIS e.V.
+ *  Copyright (C) 1994-2026, OFFIS e.V.
  *  All rights reserved.  See COPYRIGHT file for details.
  *
  *  This software and supporting documentation were partly developed by
@@ -1027,7 +1027,14 @@ AE_3_AssociateConfirmationAccept(PRIVATE_NETWORKKEY ** /*network*/,
 
         }
 
-        /* extended negotiation */
+        /* Extended negotiation: ownership of the sub-item objects transfers
+         * from the transient assoc.userInfo.extNegList to service->acceptedExtNegList
+         * via the shallow appendList(). The destroyUserInformationLists() call
+         * a few lines below deliberately frees only the list container; the
+         * items themselves will be released by DUL_ClearServiceParameters()
+         * through the service parameter list. See the ownership note in
+         * destroyUserInformationLists() (helpers.cc).
+         */
         if (assoc.userInfo.extNegList != NULL) {
             service->acceptedExtNegList = new SOPClassExtendedNegotiationSubItemList;
             if (service->acceptedExtNegList == NULL)  return EC_MemoryExhausted;
@@ -1231,7 +1238,14 @@ AE_6_ExamineAssociateRequest(PRIVATE_NETWORKKEY ** /*network*/,
             return DUL_PCTRANSLATIONFAILURE;
         }
 
-        /* extended negotiation */
+        /* Extended negotiation: ownership of the sub-item objects transfers
+         * from the transient assoc.userInfo.extNegList to service->requestedExtNegList
+         * via the shallow appendList(). The destroyUserInformationLists() call
+         * a few lines below frees only the list container; the items themselves
+         * are released later by DUL_ClearServiceParameters() through the service
+         * parameter list. See the ownership note in destroyUserInformationLists()
+         * (helpers.cc).
+         */
         if (assoc.userInfo.extNegList != NULL) {
             service->requestedExtNegList = new SOPClassExtendedNegotiationSubItemList;
             if (service->requestedExtNegList == NULL) return EC_MemoryExhausted;

diff --git a/dcmnet/libsrc/dulparse.cc b/dcmnet/libsrc/dulparse.cc
index 020c0fe0e772548eab098589acf6b7613a09b593..ea173855766d3d9fbb27b9467ac21dcb26c8fe36 100644 (file)
--- a/dcmnet/libsrc/dulparse.cc
+++ b/dcmnet/libsrc/dulparse.cc
@@ -1,6 +1,6 @@
 /*
  *
- *  Copyright (C) 1994-2024, OFFIS e.V.
+ *  Copyright (C) 1994-2026, OFFIS e.V.
  *  All rights reserved.  See COPYRIGHT file for details.
  *
  *  This software and supporting documentation were partly developed by
@@ -292,6 +292,15 @@ parseAssociate(unsigned char *buf, unsigned long pduLength,
     if (cond.bad())
     {
       destroyAssociatePDUPresentationContextList(&assoc->presentationContextList);
+      /* On a parse error the parsed extended negotiation sub-items are only
+       * held by assoc->userInfo.extNegList; no service parameter list has
+       * adopted them yet. destroyUserInformationLists() releases the list
+       * container but not its members (see the ownership note there), so
+       * the SOPClassExtendedNegotiationSubItem objects must be released
+       * explicitly here.
+       */
+      if (assoc->userInfo.extNegList != NULL)
+        deleteListMembers(*assoc->userInfo.extNegList);
       destroyUserInformationLists(&assoc->userInfo);
     }
     return cond;
@@ -590,7 +599,17 @@ parseUserInfo(DUL_USERINFO * userInfo,
             extNeg = new SOPClassExtendedNegotiationSubItem;
             if (extNeg == NULL)  return EC_MemoryExhausted;
             cond = parseExtNeg(extNeg, buf, &length, userLength);
-            if (cond.bad()) return cond;
+            if (cond.bad())
+            {
+                /* extNeg has not yet been pushed to extNegList, so the
+                 * outer cleanup in parseAssociate would not see it. Release
+                 * it here. parseExtNeg only returns errors before allocating
+                 * extNeg->serviceClassAppInfo, so there is no inner buffer
+                 * to free.
+                 */
+                delete extNeg;
+                return cond;
+            }
             if (userInfo->extNegList == NULL)
             {
                 userInfo->extNegList = new SOPClassExtendedNegotiationSubItemList;

diff --git a/dcmnet/libsrc/extneg.cc b/dcmnet/libsrc/extneg.cc
index 26b659fd0183e5982de60d0fc3df8ee3295e01cc..a6f2ab18a004909bcb12089ba86d28d71ed184a8 100644 (file)
--- a/dcmnet/libsrc/extneg.cc
+++ b/dcmnet/libsrc/extneg.cc
@@ -14,6 +14,12 @@
 #include "dcmtk/dcmnet/extneg.h"
 
 
+/* Shallow copy: 'to' receives the same SOPClassExtendedNegotiationSubItem*
+ * pointers held by 'from'. Both lists end up sharing the items; exactly one
+ * side may eventually call deleteListMembers() on them. See the ownership
+ * note in destroyUserInformationLists() (helpers.cc) for which side that is
+ * on each call path.
+ */
 void appendList(const SOPClassExtendedNegotiationSubItemList& from, SOPClassExtendedNegotiationSubItemList& to)
 {
     OFListConstIterator(SOPClassExtendedNegotiationSubItem*) i = from.begin();

diff --git a/dcmnet/libsrc/helpers.cc b/dcmnet/libsrc/helpers.cc
index 9121926f7812f5b7e57c6e9c0ae7dd00ab259b79..9e59cd750a405a5161f452bc28397e7afa2cfdfe 100644 (file)
--- a/dcmnet/libsrc/helpers.cc
+++ b/dcmnet/libsrc/helpers.cc
@@ -1,6 +1,6 @@
 /*
  *
- *  Copyright (C) 2021, OFFIS e.V.
+ *  Copyright (C) 2021-2026, OFFIS e.V.
  *  All rights reserved.  See COPYRIGHT file for details.
  *
  *  This software and supporting documentation were partly developed by
@@ -63,7 +63,15 @@ destroyUserInformationLists(DUL_USERINFO * userInfo)
     }
     LST_Destroy(&userInfo->SCUSCPRoleList);
 
-    /* extended negotiation */
+    /* Extended negotiation: the list contents (SOPClassExtendedNegotiationSubItem*)
+     * are intentionally NOT deleted here. On all normal paths the items are
+     * shared (via the shallow appendList()) with the owning service parameter
+     * list (params->{requested,accepted}ExtNegList in DUL_ASSOCIATESERVICEPARAMETERS),
+     * which releases them through DUL_ClearServiceParameters() ->
+     * deleteListMembers(). Callers with no service-side owner (e.g. the
+     * parse-error cleanup in parseAssociate()) must call deleteListMembers()
+     * themselves before invoking this function.
+     */
     delete userInfo->extNegList;
     userInfo->extNegList = NULL;