From: Michael Onken Date: Mon, 6 Jul 2026 20:39:03 +0000 (+0200) Subject: Fix extNegList leaks on A-ASSOCIATE parse failure. X-Git-Tag: archive/raspbian/3.7.0+really3.7.0-7+rpi1^2~5 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=9d8172861997e769cdbf7ff3ce30c89bf12d051a;p=dcmtk.git Fix extNegList leaks on A-ASSOCIATE parse failure. Applied-Upstream: 23f181f7a3cb8334056f751a3a0c2ddf01046752 Last-Update: 2026-05-26 Reviewed-By: Étienne Mollier 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 063cf5ab..d7f17c92 100644 --- 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 060361ec..00ee6444 100644 --- 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 020c0fe0..ea173855 100644 --- 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 26b659fd..a6f2ab18 100644 --- 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 9121926f..9e59cd75 100644 --- 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;