From: Debian Med Packaging Team Date: Mon, 6 Jul 2026 20:39:03 +0000 (+0200) Subject: CVE-2026-52868 X-Git-Tag: archive/raspbian/3.7.0+really3.7.0-7+rpi1^2~3 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=4d1a8a91c01d5596ccc7c1d42c2d3776134772ef;p=dcmtk.git CVE-2026-52868 commit e3878daf870cd2db50eadfde38615f0afae8a584 Author: Michael Onken Date: Tue May 19 17:16:08 2026 +0200 Fix path traversal in wlmscpfs through Called AET. The wlmscpfs SCP appended the Called Application Entity Title received in the A-ASSOCIATE-RQ directly onto the configured worklist data file path and used the existence of the resulting directory as an access control decision. Because DICOM VR AE permits the characters "/", "\" and ".", a peer could send a 16-byte AE title such as "../secret/VICTIM" and have wlmscpfs serve worklist records from a sibling directory of the configured root. With option --request-file-path enabled, the same AE title (and the Patient ID) was substituted into the output filename template without sanitization, producing an arbitrary-location write primitive outside the configured request file directory. This commit closes both holes: - wlmscpfs now rejects any A-ASSOCIATE-RQ whose Called AE title is not safe to use as a single filesystem path component, refusing the association with WLM_BAD_AE_SERVICE. The validation is implemented in the new static method WlmFileSystemInteractionManager::IsValidAETitle\ ForFilesystem, which rejects empty or over-long titles, any title containing a dot, and delegates the remaining character check to OFStandard::sanitizeAETitle (sanitize-and-compare). - The placeholder substitution in storeRequestToFile now passes each substituted value (#a, #c, #p) through OFStandard::sanitizeAETitle before insertion, and additionally sanitizes the final filename, so that any path separator surviving the substitution is defanged. Supporting changes: - Promote storescp's private sanitizeAETitle helper into the public OFStandard::sanitizeAETitle, with documentation noting that the function is also used by wlmscpfs to validate filesystem path components (so widening the allow list has downstream effects). - Replace storescp's local copy of the helper with the new public one. - Add a forward declaration of DcmSequenceOfItems in wlfsim.h that was previously missing (existing callers happened to include dctk.h first). - Document the new behaviour in the wlmscpfs man page. Tests: - New ofstd_OFStandard_sanitizeAETitle test covers the lifted helper (path separators, NUL, control bytes, high-range bytes, shell metacharacters, the quotation-mark preservation behaviour, and empty/length-1 edge cases). - New dcmwlm_aetitle_validation test covers the validator directly with every path-traversal payload from the bug report ("../secret/\ VICTIM", "../CARDIOLOGY", "..", ".", "..\secret"), dotted variants ("MY.AE", "foo..bar", ".foo", "foo."), structural rejections (empty, 17 bytes, embedded NUL, tab, 0xFF), shell metacharacters, and a row of legitimate AE titles that must still be accepted. - dcmwlm previously had no OFTEST scaffolding; tests.cc has been added along with the corresponding CMakeLists.txt and Makefile.in entries. Thanks to Abhinav Agarwal for the report. Gbp-Pq: Name CVE-2026-52868.patch --- diff --git a/dcmnet/apps/storescp.cc b/dcmnet/apps/storescp.cc index 183c92f1..f303aedc 100644 --- a/dcmnet/apps/storescp.cc +++ b/dcmnet/apps/storescp.cc @@ -95,7 +95,6 @@ static void renameOnEndOfStudy(); static OFString replaceChars( const OFString &srcstr, const OFString &pattern, const OFString &substitute ); static void executeCommand( const OFString &cmd ); static void cleanChildren(pid_t pid, OFBool synch); -static void sanitizeAETitle(OFString& aet); static OFCondition acceptUnknownContextsWithPreferredTransferSyntaxes( T_ASC_Parameters * params, const char* transferSyntaxes[], @@ -2338,34 +2337,6 @@ static void executeEndOfStudyEvents() lastStudySubdirectoryPathAndName.clear(); } -/* replace all characters that might be interpreted by the shell with underscores - */ -static void sanitizeAETitle(OFString& aet) -{ - static const char sanitized_aetitle_charset[] = - { - ' ', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '-', '.', '_', - '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', ':', '_', '_', '_', '_', '_', - '@', 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', - 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z', '_', '_', '_', '_', '_', - '_', 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', - 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', '_', '_', '_', '_', '_' - }; - - // the aet string starts and ends with quotation marks. We ignore these. - size_t len = aet.length(); - if (len < 3) return; - - char c; - --len; - for (size_t i=1; i < len; ++i) - { - c = aet[i]; - if (c != 0 && (c < 32 || c >= 127)) c = '_'; else c = sanitized_aetitle_charset[c-32]; - aet[i] = c; - } -} - static void executeOnReception() /* * This function deals with the execution of the command line which was passed @@ -2401,7 +2372,7 @@ static void executeOnReception() // perform substitution for placeholder #a. // Note that this string is already enclosed in double quotes at this point s = callingAETitle; - sanitizeAETitle(s); + OFStandard::sanitizeAETitle(s); if (s != callingAETitle) { OFLOG_WARN(storescpLogger, "Sanitized unusual characters in calling aetitle, converted from " << callingAETitle << " to " << s << "."); @@ -2411,7 +2382,7 @@ static void executeOnReception() // perform substitution for placeholder #c. // Note that this string is already enclosed in double quotes at this point s = calledAETitle; - sanitizeAETitle(s); + OFStandard::sanitizeAETitle(s); if (s != calledAETitle) { OFLOG_WARN(storescpLogger, "Sanitized unusual characters in called aetitle, converted from " << calledAETitle << " to " << s << "."); @@ -2421,7 +2392,7 @@ static void executeOnReception() // perform substitution for placeholder #r. // Note that this string is already enclosed in double quotes at this point s = callingPresentationAddress; - sanitizeAETitle(s); + OFStandard::sanitizeAETitle(s); if (s != callingPresentationAddress) { OFLOG_WARN(storescpLogger, "Sanitized unusual characters in calling presentation address, converted from " << callingPresentationAddress << " to " << s << "."); @@ -2537,7 +2508,7 @@ static void executeOnEndOfStudy() // perform substitution for placeholder #a. // Note that this string is already enclosed in double quotes at this point s = callingAETitle; - sanitizeAETitle(s); + OFStandard::sanitizeAETitle(s); if (s != callingAETitle) { OFLOG_WARN(storescpLogger, "Sanitized unusual characters in calling aetitle, converted from " << callingAETitle << " to " << s << "."); @@ -2547,7 +2518,7 @@ static void executeOnEndOfStudy() // perform substitution for placeholder #c. // Note that this string is already enclosed in double quotes at this point s = calledAETitle; - sanitizeAETitle(s); + OFStandard::sanitizeAETitle(s); if (s != calledAETitle) { OFLOG_WARN(storescpLogger, "Sanitized unusual characters in called aetitle, converted from " << calledAETitle << " to " << s << "."); @@ -2557,7 +2528,7 @@ static void executeOnEndOfStudy() // perform substitution for placeholder #r. // Note that this string is already enclosed in double quotes at this point s = callingPresentationAddress; - sanitizeAETitle(s); + OFStandard::sanitizeAETitle(s); if (s != callingPresentationAddress) { OFLOG_WARN(storescpLogger, "Sanitized unusual characters in calling presentation address, converted from " << callingPresentationAddress << " to " << s << "."); diff --git a/dcmwlm/docs/wlmscpfs.man b/dcmwlm/docs/wlmscpfs.man index 02d87b07..e695392e 100644 --- a/dcmwlm/docs/wlmscpfs.man +++ b/dcmwlm/docs/wlmscpfs.man @@ -242,6 +242,23 @@ which the SCP might have to return to an SCU in a C-FIND response message. Table K.6-1 in part 4 annex K of the DICOM standard lists all corresponding type 1 attributes (see column "Return Key Type"). +The called Application Entity Title from an incoming A-ASSOCIATE-RQ is used +as a subdirectory name below the \e --data-file-path directory in order to +look up the worklist files that this association may access. The DICOM VR +AE (see DICOM PS3.5 Section 6.1.3) permits the characters "/" (forward slash) +and "." (full stop), so a conformant peer can send an AE title such as +"../OTHER" that would resolve to a sibling directory of the configured +worklist root. To prevent this, \b wlmscpfs rejects any A-ASSOCIATE-RQ +whose called AE title is not safe to use as a single filesystem path +component. The set of accepted characters is letters, digits, space and +the characters "-", ":", "@" and "_"; any other byte (path separators +"/" and "\\", NUL, control bytes, bytes outside the printable ASCII range, +shell metacharacters as well as the dot character) causes the association +to be rejected. Dots are rejected outright (rather than only the +standalone components "." and "..") because real-world AE titles never +contain dots and rejecting the entire character avoids any need to reason +about platform-specific path normalization corner cases. + \subsection wlmscpfs_request_files Writing Request Files Providing option \e --request-file-path enables writing of the incoming C-FIND @@ -300,6 +317,13 @@ characters, the file name computed by \b wlmscpfs might be broken and thus cannot be written successfully or will look broken once written. Also, an empty Patient ID is used as such, i.e. the \#p will be replaced with an empty string. +For security reasons, the values substituted for the placeholders \#a, \#c and +\#p are sanitized before they are inserted into the resulting file name: path +separators ("/", "\\") and other characters that are not safe in a file name +component are replaced with the underscore character ("_"). This prevents +attacker-controlled values transmitted over the network (called/calling AE +title, Patient ID) from escaping the configured request file directory. + \subsection wlmscpfs_dicom_conformance DICOM Conformance The \b wlmscpfs application supports the following SOP Classes as an SCP: diff --git a/dcmwlm/include/dcmtk/dcmwlm/wlfsim.h b/dcmwlm/include/dcmtk/dcmwlm/wlfsim.h index 27d2d5ba..6c85dfac 100644 --- a/dcmwlm/include/dcmtk/dcmwlm/wlfsim.h +++ b/dcmwlm/include/dcmtk/dcmwlm/wlfsim.h @@ -34,6 +34,7 @@ class DcmDataset; class DcmTagKey; class OFCondition; class DcmItem; +class DcmSequenceOfItems; class OFdirectory_iterator; /** This class encapsulates data structures and operations for managing @@ -199,13 +200,53 @@ class DCMTK_DCMWLM_EXPORT WlmFileSystemInteractionManager /** Checks if the given called application entity title is supported. If this is the case, * OFTrue will be returned, else OFFalse. + * + * The check is performed in two stages. First, the AE title is validated against the + * rules of IsValidAETitleForFilesystem(): titles that contain path separators, embedded + * NUL or other control characters, or any dot character are rejected outright (returning + * OFFalse). This prevents the wire-side AE title from escaping the worklist data file + * path through path traversal. Second, the AE title is appended to the configured + * worklist root path and the resulting directory must exist. + * * @param calledApplicationEntityTitlev The application entity title which shall be checked - * for support. Valid pointer expected. + * for support. * @return OFTrue, if the called application entity title is supported, - * OFFalse, if the called application entity title is not supported or it is not given. + * OFFalse, if the called application entity title is not supported, not given, + * or rejected because it would not be safe to use as a filesystem path component. */ OFBool IsCalledApplicationEntityTitleSupported( const OFString& calledApplicationEntityTitlev ); + /** Determines whether the given DICOM Application Entity Title is safe to use as a single + * directory or filename component below the worklist data file path. + * + * An AE title is considered safe if and only if all of the following hold: + * - it is not empty and not longer than 16 bytes (the DICOM AE VR length limit), + * - it contains no dot ('.') at all, + * - OFStandard::sanitizeAETitle() would not change any byte of it. + * + * The third rule delegates the character-set check to OFStandard::sanitizeAETitle(), + * which is the same allow list used by \b storescp for filename and shell-substitution + * contexts: letters, digits, space and the characters '-', '.', ':', '@', '_' are kept, + * every other byte (path separators '/' and '\\', NUL, control characters, bytes + * outside the printable ASCII range and shell metacharacters) is replaced. If the + * sanitized string differs from the input, the AE title contains at least one such + * unsafe byte and is rejected. + * + * Dots are rejected by the explicit second rule even though sanitizeAETitle() keeps + * them. Real-world AE titles never contain dots, and rejecting the entire character + * avoids any need to reason about path normalization corner cases (".", "..", trailing + * dots on Windows etc.). + * + * Note that this delegation couples the rule applied here to the allow list inside + * OFStandard::sanitizeAETitle(). If that allow list is widened in the future, the set + * of AE titles accepted as safe filesystem path components widens too, so any change + * to it must consider downstream effects on \b wlmscpfs. + * + * @param aeTitle Application Entity Title to validate. + * @return OFTrue if the AE title is safe to use as a path component, OFFalse otherwise. + */ + static OFBool IsValidAETitleForFilesystem( const OFString& aeTitle ); + /** This function determines the records from the Worklist files that match * the given search mask and returns the number of matching records. Also, * this function will store the matching records inside the member variable diff --git a/dcmwlm/libsrc/wlfsim.cc b/dcmwlm/libsrc/wlfsim.cc index aa2789ff..40a93c4e 100644 --- a/dcmwlm/libsrc/wlfsim.cc +++ b/dcmwlm/libsrc/wlfsim.cc @@ -1,6 +1,6 @@ /* * - * Copyright (C) 1996-2024, OFFIS e.V. + * Copyright (C) 1996-2026, OFFIS e.V. * All rights reserved. See COPYRIGHT file for details. * * This software and supporting documentation were developed by @@ -155,6 +155,32 @@ OFCondition WlmFileSystemInteractionManager::DisconnectFromFileSystem() // ---------------------------------------------------------------------------- +OFBool WlmFileSystemInteractionManager::IsValidAETitleForFilesystem( const OFString& aeTitle ) +{ + // Length must be 1..16 (DICOM AE VR limit). + if( aeTitle.empty() || aeTitle.length() > 16 ) + return OFFalse; + + // OFStandard::sanitizeAETitle() keeps '.' in its allow list (the function + // is also used in shell-substitution contexts where dots are common in + // AE titles), so reject dots explicitly here. Rejecting the entire + // character avoids any need to reason about platform-specific path + // normalization corner cases (".", "..", trailing dots on Windows etc.). + if( aeTitle.find('.') != OFString_npos ) + return OFFalse; + + // Delegate the remaining character check to OFStandard::sanitizeAETitle(). + // If the sanitizer would change any byte, the AE title contains characters + // that are not safe to use as a single filesystem path component (path + // separators, NUL, control bytes, bytes outside the printable ASCII range + // and shell metacharacters). + OFString sanitized = aeTitle; + OFStandard::sanitizeAETitle( sanitized ); + return ( sanitized == aeTitle ) ? OFTrue : OFFalse; +} + +// ---------------------------------------------------------------------------- + OFBool WlmFileSystemInteractionManager::IsCalledApplicationEntityTitleSupported( const OFString& calledApplicationEntityTitlev ) // Date : July 11, 2002 // Author : Thomas Wilkens @@ -168,6 +194,15 @@ OFBool WlmFileSystemInteractionManager::IsCalledApplicationEntityTitleSupported( // copy value calledApplicationEntityTitle = calledApplicationEntityTitlev; + // Reject AE titles that are not safe to use as a filesystem path component. + // This guards against path traversal via path separators or "../" segments + // smuggled in through the wire-side AE title. + if( !IsValidAETitleForFilesystem( calledApplicationEntityTitle ) ) + { + DCMWLM_WARN( "Refusing called AE title because it is not safe to use as a directory name (contains path separators, control characters or is \".\"/\"..\"); rejecting association" ); + return( OFFalse ); + } + // Determine complete path to the files that make up the data source. OFString fullPath( dfPath ); if( !fullPath.empty() && fullPath[fullPath.length()-1] != PATH_SEPARATOR ) diff --git a/dcmwlm/libsrc/wlmactmg.cc b/dcmwlm/libsrc/wlmactmg.cc index 02c8425e..19a13b0b 100644 --- a/dcmwlm/libsrc/wlmactmg.cc +++ b/dcmwlm/libsrc/wlmactmg.cc @@ -1174,10 +1174,19 @@ static void FindCallback( void *callbackData, OFBool cancelled, T_DIMSE_C_FindRQ static void storeRequestToFile(DcmDataset& request, const OFString& callingAE, const OFString& calledAE, const OFString& reqFilePath, const OFString& reqFileFormat) { OFString fileName = reqFileFormat; - // Called Application Entity Title - OFStringUtil::replace_all(fileName, WLM_CALLED_AETITLE_PLACEHOLDER, calledAE); - // Calling Application Entity Title - OFStringUtil::replace_all(fileName, WLM_CALLING_AETITLE_PLACEHOLDER, callingAE); + + // Each substituted value is sanitized before being inserted into the + // filename template. This prevents the calling/called AE title and the + // Patient ID (all originating on the wire) from injecting path + // separators or other unsafe characters into the resulting filename + // and thereby escaping the configured request file directory. + OFString safeCalledAE = calledAE; + OFStandard::sanitizeAETitle(safeCalledAE); + OFStringUtil::replace_all(fileName, WLM_CALLED_AETITLE_PLACEHOLDER, safeCalledAE); + + OFString safeCallingAE = callingAE; + OFStandard::sanitizeAETitle(safeCallingAE); + OFStringUtil::replace_all(fileName, WLM_CALLING_AETITLE_PLACEHOLDER, safeCallingAE); // Process ID int processID = dcmtk::log4cplus::internal::get_process_id(); @@ -1199,8 +1208,15 @@ static void storeRequestToFile(DcmDataset& request, const OFString& callingAE, c // Patient ID goes last since it might contain placeholders again (".#x...)" OFString patientID; request.findAndGetOFStringArray(DCM_PatientID, patientID); + OFStandard::sanitizeAETitle(patientID); OFStringUtil::replace_all(fileName, WLM_PATIENT_ID_PLACEHOLDER, patientID); + // Defense in depth: also sanitize the fully assembled filename so that + // any path separator that survived (for example through interaction + // between placeholder substitutions or via the user-supplied format + // string itself) is replaced before the file is opened. + OFStandard::sanitizeAETitle(fileName); + // Finally store file STD_NAMESPACE ofstream outputStream; OFString fullPath; diff --git a/dcmwlm/tests/CMakeLists.txt b/dcmwlm/tests/CMakeLists.txt index 458840d6..ead3fda9 100644 --- a/dcmwlm/tests/CMakeLists.txt +++ b/dcmwlm/tests/CMakeLists.txt @@ -1,5 +1,10 @@ # declare executables DCMTK_ADD_TEST_EXECUTABLE(wltest wltest.cc) +DCMTK_ADD_TEST_EXECUTABLE(dcmwlm_tests tests.cc twlaetval.cc) # make sure executables are linked to the corresponding libraries DCMTK_TARGET_LINK_MODULES(wltest dcmwlm dcmtls) +DCMTK_TARGET_LINK_MODULES(dcmwlm_tests dcmwlm) + +# This macro parses tests.cc and registers all tests +DCMTK_ADD_TESTS(dcmwlm) diff --git a/dcmwlm/tests/Makefile.in b/dcmwlm/tests/Makefile.in index 78049d74..c58c57f1 100644 --- a/dcmwlm/tests/Makefile.in +++ b/dcmwlm/tests/Makefile.in @@ -25,19 +25,25 @@ LIBDIRS = -L$(top_srcdir)/libsrc -L$(dcmnetdir)/libsrc -L$(dcmdatadir)/libsrc \ LOCALLIBS = -ldcmwlm -ldcmnet -ldcmdata -loflog -lofstd -loficonv $(ZLIBLIBS) \ $(TCPWRAPPERLIBS) $(CHARCONVLIBS) $(MATHLIBS) -objs = wltest.o -progs = wltest +test_objs = tests.o twlaetval.o +objs = wltest.o $(test_objs) +progs = wltest dcmwlm_tests all: $(progs) -wltest: $(objs) - $(CXX) $(CXXFLAGS) $(LIBDIRS) $(LDFLAGS) -o $@ $(objs) $(LOCALLIBS) $(LIBS) +wltest: wltest.o + $(CXX) $(CXXFLAGS) $(LIBDIRS) $(LDFLAGS) -o $@ wltest.o $(LOCALLIBS) $(LIBS) +dcmwlm_tests: $(test_objs) + $(CXX) $(CXXFLAGS) $(LIBDIRS) $(LDFLAGS) -o $@ $(test_objs) $(LOCALLIBS) $(LIBS) -check: -check-exhaustive: +check: dcmwlm_tests + ./dcmwlm_tests + +check-exhaustive: dcmwlm_tests + ./dcmwlm_tests -x install: all diff --git a/dcmwlm/tests/tests.cc b/dcmwlm/tests/tests.cc new file mode 100644 index 00000000..1ea82f4c --- /dev/null +++ b/dcmwlm/tests/tests.cc @@ -0,0 +1,27 @@ +/* + * + * Copyright (C) 2026, OFFIS e.V. + * All rights reserved. See COPYRIGHT file for details. + * + * This software and supporting documentation were developed by + * + * OFFIS e.V. + * R&D Division Health + * Escherweg 2 + * D-26121 Oldenburg, Germany + * + * + * Module: dcmwlm + * + * Author: Michael Onken + * + * Purpose: main test program + * + */ + +#include "dcmtk/config/osconfig.h" +#include "dcmtk/ofstd/oftest.h" + +OFTEST_REGISTER(dcmwlm_aetitle_validation); + +OFTEST_MAIN("dcmwlm") diff --git a/dcmwlm/tests/twlaetval.cc b/dcmwlm/tests/twlaetval.cc new file mode 100644 index 00000000..7f7dac44 --- /dev/null +++ b/dcmwlm/tests/twlaetval.cc @@ -0,0 +1,116 @@ +/* + * + * Copyright (C) 2026, OFFIS e.V. + * All rights reserved. See COPYRIGHT file for details. + * + * This software and supporting documentation were developed by + * + * OFFIS e.V. + * R&D Division Health + * Escherweg 2 + * D-26121 Oldenburg, Germany + * + * + * Module: dcmwlm + * + * Author: Michael Onken + * + * Purpose: Tests for the AE title validation used by the wlmscpfs + * filesystem worklist SCP to guard against path traversal + * through the Called Application Entity Title. + * + */ + +#include "dcmtk/config/osconfig.h" + +#include "dcmtk/ofstd/oftest.h" +#include "dcmtk/ofstd/ofstring.h" +#include "dcmtk/dcmwlm/wlfsim.h" + + +// Helper: build an OFString that contains a single NUL byte in the middle +// of an otherwise printable AE title. OFString tolerates embedded NUL bytes +// when constructed from a (pointer, length) pair, which is how a NUL byte +// could enter wlmscpfs from the wire. +static OFString aeTitleWithEmbeddedNul() +{ + const char raw[] = { 'A', 'B', '\0', 'C', 'D' }; + return OFString(raw, sizeof(raw)); +} + + +OFTEST(dcmwlm_aetitle_validation) +{ + // ----- Path traversal payloads from the original bug report. ----- + // All of these must be rejected; if any of them is accepted, the + // CVE-2026-... path traversal hole is open again. + + // 16-byte payload from the proof of concept ("../secret/VICTIM"). + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("../secret/VICTIM")); + + // Multi-AET demo ("../CARDIOLOGY"). + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("../CARDIOLOGY")); + + // Plain parent reference and self reference. + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("..")); + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem(".")); + + // Windows-style separator. + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("..\\secret")); + + // Leading-dot variants and dots in the middle. None of these are + // path-traversal payloads on their own, but we reject any AE title + // that contains a dot, see the documentation of + // IsValidAETitleForFilesystem() for the rationale. + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem(".foo")); + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("foo.")); + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("foo..bar")); + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("MY.AE")); + + // ----- Other unsafe input. ----- + + // Empty AE title. + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("")); + + // Too long (17 bytes; the DICOM AE VR allows at most 16). + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("ABCDEFGHIJKLMNOPQ")); + + // Embedded NUL byte. + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem(aeTitleWithEmbeddedNul())); + + // Control character (tab). + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("AB\tCD")); + + // High-range byte. + { + OFString withHigh; + withHigh.append("AE", 2); + withHigh.append(1, OFstatic_cast(char, 0xFF)); + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem(withHigh)); + } + + // Shell metacharacters: rejected because + // OFStandard::sanitizeAETitle() would replace them. + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("AE;rm")); + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("AE|cat")); + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("AE`id`")); + OFCHECK(!WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("AE$VAR")); + + // ----- Legitimate AE titles must still be accepted. ----- + + // Plain alphanumeric, mixed case. + OFCHECK(WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("WORKLIST")); + OFCHECK(WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("CARDIOLOGY")); + OFCHECK(WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("MyAE")); + + // Single character (minimum length). + OFCHECK(WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("A")); + + // Maximum length (16 bytes). + OFCHECK(WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("ABCDEFGHIJKLMNOP")); + + // Conformant punctuation that is also safe as a path component. + OFCHECK(WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("MY-AE_1")); + OFCHECK(WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("AE@HOST")); + OFCHECK(WlmFileSystemInteractionManager::IsValidAETitleForFilesystem("AE:1")); +} diff --git a/ofstd/include/dcmtk/ofstd/ofstd.h b/ofstd/include/dcmtk/ofstd/ofstd.h index 265f51d9..9d70d27f 100644 --- a/ofstd/include/dcmtk/ofstd/ofstd.h +++ b/ofstd/include/dcmtk/ofstd/ofstd.h @@ -1181,6 +1181,44 @@ class DCMTK_OFSTD_EXPORT OFStandard */ static void sanitizeFilename(char *fname); + /** sanitize a DICOM Application Entity Title for safe use as a + * substitution value in a filename or in a command line that will + * be passed to a shell. + * + * This method maps each byte through an allow list: ASCII letters, + * digits, space and the characters '-', '.', ':', '@', '_' are kept + * unchanged; every other byte (including path separators '/' and + * '\\', NUL, control characters, shell metacharacters and bytes + * outside the printable ASCII range) is replaced by '_'. + * + * The aetitle string is expected to be already trimmed (i.e. without + * leading or trailing whitespace). Surrounding quotation marks, if + * present, are preserved unchanged (the first and last byte are not + * rewritten) so that quoted AE titles intended for shell command + * substitution remain quoted. + * + * This sanitization is sufficient for preventing shell injection in + * contexts where the AE title is substituted into an already quoted + * command-line argument, and for preventing path separators from + * appearing in filenames. It is NOT sufficient to prevent path + * traversal on its own, because the character '.' is kept and the + * sequence "..", which is a path-relative parent reference on most + * operating systems, is therefore not removed. Callers that + * substitute the result into a filesystem path must reject or + * collapse such sequences explicitly. + * + * Note: this function is also used by the wlmscpfs application as + * the basis of its filesystem-path-component validation (an AE title + * is accepted only if sanitizeAETitle() would not change it, in + * addition to an explicit dot rejection). Widening the allow list + * below therefore widens the set of AE titles accepted as worklist + * directory names; any change to it must consider that downstream + * effect. + * + * @param aetitle Application Entity Title to be sanitized in place. + */ + static void sanitizeAETitle(OFString& aetitle); + /** retrieve the name of the default directory for support data. * On Windows, this method resolves environment variables such as * \%PROGRAMDATA% in the path, on Posix platforms it just returns diff --git a/ofstd/libsrc/ofstd.cc b/ofstd/libsrc/ofstd.cc index 78284b04..eb9ee549 100644 --- a/ofstd/libsrc/ofstd.cc +++ b/ofstd/libsrc/ofstd.cc @@ -3440,6 +3440,42 @@ void OFStandard::sanitizeFilename(char *fname) } +// Allow list used by sanitizeAETitle(). Index is (byte - 32), so the +// table covers the printable ASCII range 0x20..0x7E. Every entry either +// repeats the input byte (kept) or is '_' (replaced). Kept characters: +// space, '-', '.', ':', '@', '_' and ASCII letters/digits. All shell +// metacharacters and path separators map to '_'. +static const char sanitized_aetitle_charset[] = +{ + ' ', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '-', '.', '_', + '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', ':', '_', '_', '_', '_', '_', + '@', 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', + 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z', '_', '_', '_', '_', '_', + '_', 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', + 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', '_', '_', '_', '_', '_' +}; + + +void OFStandard::sanitizeAETitle(OFString& aetitle) +{ + // Preserve a surrounding pair of quotation marks (used by callers + // that substitute the AE title into an already quoted shell argument). + size_t len = aetitle.length(); + size_t start = 0; + if (len >= 2 && aetitle[0] == '"' && aetitle[len - 1] == '"') + { + start = 1; + --len; + } + for (size_t i = start; i < len; ++i) + { + unsigned char c = OFstatic_cast(unsigned char, aetitle[i]); + if (c < 32 || c >= 127) aetitle[i] = '_'; + else aetitle[i] = sanitized_aetitle_charset[c - 32]; + } +} + + OFString OFStandard::getDefaultSupportDataDir() { #ifdef HAVE_WINDOWS_H diff --git a/ofstd/tests/tests.cc b/ofstd/tests/tests.cc index 33398b21..9fdb6b78 100644 --- a/ofstd/tests/tests.cc +++ b/ofstd/tests/tests.cc @@ -1,6 +1,6 @@ /* * - * Copyright (C) 2011-2025, OFFIS e.V. + * Copyright (C) 2011-2026, OFFIS e.V. * All rights reserved. See COPYRIGHT file for details. * * This software and supporting documentation were developed by @@ -48,6 +48,7 @@ OFTEST_REGISTER(ofstd_OFStack); OFTEST_REGISTER(ofstd_OFStandard_isReadWriteable); OFTEST_REGISTER(ofstd_OFStandard_appendFilenameExtension); OFTEST_REGISTER(ofstd_OFStandard_removeRootDirFromPathname); +OFTEST_REGISTER(ofstd_OFStandard_sanitizeAETitle); OFTEST_REGISTER(ofstd_OFFile); OFTEST_REGISTER(ofstd_OFString_compare); OFTEST_REGISTER(ofstd_OFString_concatenate); diff --git a/ofstd/tests/tofstd.cc b/ofstd/tests/tofstd.cc index 4238be36..40b063a0 100644 --- a/ofstd/tests/tofstd.cc +++ b/ofstd/tests/tofstd.cc @@ -1,6 +1,6 @@ /* * - * Copyright (C) 2002-2022, OFFIS e.V. + * Copyright (C) 2002-2026, OFFIS e.V. * All rights reserved. See COPYRIGHT file for details. * * This software and supporting documentation were developed by @@ -373,6 +373,62 @@ OFTEST(ofstd_OFStandard_removeRootDirFromPathname) OFCHECK(OFStandard::removeRootDirFromPathname(result, nullPtr, nullPtr).good()); } +OFTEST(ofstd_OFStandard_sanitizeAETitle) +{ + // Pure-ASCII alphanumeric is preserved. + OFString s = "MY_AE_42"; + OFStandard::sanitizeAETitle(s); + OFCHECK_EQUAL(s, "MY_AE_42"); + + // Dots, colons, dashes, '@' are part of the allow list. + s = "AE.TITLE:1@host-1"; + OFStandard::sanitizeAETitle(s); + OFCHECK_EQUAL(s, "AE.TITLE:1@host-1"); + + // Path separators map to underscores. + s = "../etc/passwd"; + OFStandard::sanitizeAETitle(s); + OFCHECK_EQUAL(s, ".._etc_passwd"); + + // Backslash is replaced (Windows path separator). + s = "..\\secret"; + OFStandard::sanitizeAETitle(s); + OFCHECK_EQUAL(s, ".._secret"); + + // Shell metacharacters are replaced. + s = "AE;rm -rf /"; + OFStandard::sanitizeAETitle(s); + OFCHECK_EQUAL(s, "AE_rm -rf _"); + + // Control characters (including NUL inside the string) are replaced. + OFString withNul; + withNul.append("AB", 2); + withNul.append(1, '\0'); + withNul.append("CD", 2); + OFStandard::sanitizeAETitle(withNul); + OFCHECK_EQUAL(withNul, OFString("AB_CD")); + + // High-range bytes (>= 0x7F) are replaced. + OFString withHigh; + withHigh.append("AE", 2); + withHigh.append(1, OFstatic_cast(char, 0xFF)); + OFStandard::sanitizeAETitle(withHigh); + OFCHECK_EQUAL(withHigh, OFString("AE_")); + + // A surrounding quotation-mark pair is preserved (storescp behaviour). + s = "\"../bad\""; + OFStandard::sanitizeAETitle(s); + OFCHECK_EQUAL(s, "\".._bad\""); + + // Empty string and length-1 string are handled without crashing. + s = ""; + OFStandard::sanitizeAETitle(s); + OFCHECK_EQUAL(s, ""); + s = "/"; + OFStandard::sanitizeAETitle(s); + OFCHECK_EQUAL(s, "_"); +} + OFTEST(ofstd_safeSubtractAddMult) { // --------------- Subtraction ----------------