From f06a867513524664a1b03dfcf812d8b60fdd02cc Mon Sep 17 00:00:00 2001
From: Marco Eichelberg <dicom@offis.de>
Date: Fri, 6 May 2022 17:30:02 +0200
Subject: [PATCH] Fixed path traversal vulnerability.
Thanks to Sharon Brizinov >sharon.b@claroty.com> and Noam Moshe from
Claroty Research for the bug report and sample files.
This closes DCMTK issue #1021.
---
dcmnet/apps/movescu.cc | 3 ++-
dcmnet/apps/storescp.cc | 8 +++++--
dcmnet/libsrc/dstorscp.cc | 5 ++++-
dcmnet/libsrc/scu.cc | 1 +
ofstd/include/dcmtk/ofstd/ofstd.h | 18 +++++++++++++++-
ofstd/libsrc/offname.cc | 19 ++++++++++-------
ofstd/libsrc/ofstd.cc | 35 ++++++++++++++++++++++++++++++-
7 files changed, 76 insertions(+), 13 deletions(-)
diff --git a/dcmnet/apps/movescu.cc b/dcmnet/apps/movescu.cc
index 40f41674c..7e444d46b 100644
--- a/dcmnet/apps/movescu.cc
+++ b/dcmnet/apps/movescu.cc
@@ -1,6 +1,6 @@
/*
*
- * Copyright (C) 1994-2021, OFFIS e.V.
+ * Copyright (C) 1994-2022, OFFIS e.V.
* All rights reserved. See COPYRIGHT file for details.
*
* This software and supporting documentation were developed by
@@ -1425,6 +1425,7 @@ static OFCondition storeSCP(
sprintf(imageFileName, "%s.%s",
dcmSOPClassUIDToModality(req->AffectedSOPClassUID),
req->AffectedSOPInstanceUID);
+ OFStandard::sanitizeFilename(imageFileName);
}
OFString temp_str;
diff --git a/dcmnet/apps/storescp.cc b/dcmnet/apps/storescp.cc
index ee53f2887..68b7b4e97 100644
--- a/dcmnet/apps/storescp.cc
+++ b/dcmnet/apps/storescp.cc
@@ -1853,12 +1853,14 @@ storeSCPCallback(
if (!subdirectoryName.empty())
subdirectoryName += '_';
subdirectoryName += currentStudyInstanceUID;
+ OFStandard::sanitizeFilename(subdirectoryName);
break;
case ESM_PatientName:
// pattern: "[Patient's Name]_[YYYYMMDD]_[HHMMSSMMM]"
subdirectoryName = currentPatientName;
subdirectoryName += '_';
subdirectoryName += timestamp;
+ OFStandard::sanitizeFilename(subdirectoryName);
break;
case ESM_None:
break;
@@ -2065,9 +2067,11 @@ static OFCondition storeSCP(
}
else
{
- // don't create new UID, use the study instance UID as found in object
+ // Use the SOP instance UID as found in the C-STORE request message as part of the filename
+ OFString uid = req->AffectedSOPInstanceUID;
+ OFStandard::sanitizeFilename(uid);
sprintf(imageFileName, "%s%c%s.%s%s", opt_outputDirectory.c_str(), PATH_SEPARATOR, dcmSOPClassUIDToModality(req->AffectedSOPClassUID, "UNKNOWN"),
- req->AffectedSOPInstanceUID, opt_fileNameExtension.c_str());
+ uid.c_str(), opt_fileNameExtension.c_str());
}
}
diff --git a/dcmnet/libsrc/dstorscp.cc b/dcmnet/libsrc/dstorscp.cc
index e491ae5ea..1811846a2 100644
--- a/dcmnet/libsrc/dstorscp.cc
+++ b/dcmnet/libsrc/dstorscp.cc
@@ -1,6 +1,6 @@
/*
*
- * Copyright (C) 2013-2021, OFFIS e.V.
+ * Copyright (C) 2013-2022, OFFIS e.V.
* All rights reserved. See COPYRIGHT file for details.
*
* This software and supporting documentation were developed by
@@ -425,6 +425,7 @@ OFCondition DcmStorageSCP::generateDirAndFilename(OFString &filename,
generatedFileName = tmpString;
OFSTRINGSTREAM_FREESTR(tmpString);
// combine the generated file name with the directory name
+ OFStandard::sanitizeFilename(generatedFileName);
OFStandard::combineDirAndFilename(filename, directoryName, generatedFileName);
}
break;
@@ -441,6 +442,7 @@ OFCondition DcmStorageSCP::generateDirAndFilename(OFString &filename,
generatedFileName = tmpString;
OFSTRINGSTREAM_FREESTR(tmpString);
// combine the generated file name with the directory name
+ OFStandard::sanitizeFilename(generatedFileName);
OFStandard::combineDirAndFilename(filename, directoryName, generatedFileName);
break;
}
@@ -469,6 +471,7 @@ OFCondition DcmStorageSCP::generateDirAndFilename(OFString &filename,
generatedFileName = tmpString;
OFSTRINGSTREAM_FREESTR(tmpString);
// combine the generated file name
+ OFStandard::sanitizeFilename(generatedFileName);
OFStandard::combineDirAndFilename(filename, directoryName, generatedFileName);
} else
status = EC_CouldNotGenerateFilename;
diff --git a/dcmnet/libsrc/scu.cc b/dcmnet/libsrc/scu.cc
index 34b252553..1cc9c3a46 100644
--- a/dcmnet/libsrc/scu.cc
+++ b/dcmnet/libsrc/scu.cc
@@ -1418,6 +1418,7 @@ OFString DcmSCU::createStorageFilename(DcmDataset* dataset)
OFString name = dcmSOPClassUIDToModality(sopClassUID.c_str(), "UNKNOWN");
name += ".";
name += sopInstanceUID;
+ OFStandard::sanitizeFilename(name);
OFString returnStr;
OFStandard::combineDirAndFilename(returnStr, m_storageDir, name, OFTrue);
return returnStr;
diff --git a/ofstd/include/dcmtk/ofstd/ofstd.h b/ofstd/include/dcmtk/ofstd/ofstd.h
index 1548e26d0..56054ccb0 100644
--- a/ofstd/include/dcmtk/ofstd/ofstd.h
+++ b/ofstd/include/dcmtk/ofstd/ofstd.h
@@ -1,6 +1,6 @@
/*
*
- * Copyright (C) 2000-2021, OFFIS e.V.
+ * Copyright (C) 2000-2022, OFFIS e.V.
* All rights reserved. See COPYRIGHT file for details.
*
* This software and supporting documentation were developed by
@@ -1165,6 +1165,22 @@ class DCMTK_OFSTD_EXPORT OFStandard
*/
static void forceSleep(Uint32 seconds);
+ /** sanitize a filename (NOT a path name!) by replacing all path
+ * separators with underscores. This avoids possible path traversal
+ * vulnerabilities if malformed data read from file or received over
+ * a network is used as part of a filename.
+ * @param fname filename to be sanitized
+ */
+ static void sanitizeFilename(OFString& fname);
+
+ /** sanitize a filename (NOT a path name!) by replacing all path
+ * separators with underscores. This avoids possible path traversal
+ * vulnerabilities if malformed data read from file or received over
+ * a network is used as part of a filename.
+ * @param fname filename to be sanitized
+ */
+ static void sanitizeFilename(char *fname);
+
private:
/** private implementation of strlcpy. Called when strlcpy
diff --git a/ofstd/libsrc/offname.cc b/ofstd/libsrc/offname.cc
index 832376189..a56a7e1b5 100644
--- a/ofstd/libsrc/offname.cc
+++ b/ofstd/libsrc/offname.cc
@@ -1,6 +1,6 @@
/*
*
- * Copyright (C) 1997-2021, OFFIS e.V.
+ * Copyright (C) 1997-2022, OFFIS e.V.
* All rights reserved. See COPYRIGHT file for details.
*
* This software and supporting documentation were developed by
@@ -73,18 +73,23 @@ OFBool OFFilenameCreator::makeFilename(unsigned int &seed, const char *dir, cons
{
// create filename
filename.clear();
- if (dir)
- {
- filename = dir;
- filename += PATH_SEPARATOR;
- }
- if (prefix) filename += prefix;
+ if (prefix) filename = prefix;
addLongToString(creation_time, filename);
// on some systems OFrand_r may produce only 16-bit random numbers.
// To be on the safe side, we use two random numbers for the upper and the lower 16 bits.
addLongToString((((OFrand_r(seed) & 0xFFFF) << 16) | (OFrand_r(seed) & 0xFFFF)), filename);
if (postfix) filename += postfix;
+ OFStandard::sanitizeFilename(filename);
+
+ if (dir)
+ {
+ OFString dirname = dir;
+ dirname += PATH_SEPARATOR;
+ dirname += filename;
+ filename = dirname;
+ }
+
// check if filename exists
stat_result = stat(filename.c_str(), &stat_buf);
if (stat_result == 0)
diff --git a/ofstd/libsrc/ofstd.cc b/ofstd/libsrc/ofstd.cc
index ae1466a9f..33ecd7954 100644
--- a/ofstd/libsrc/ofstd.cc
+++ b/ofstd/libsrc/ofstd.cc
@@ -1,6 +1,6 @@
/*
*
- * Copyright (C) 2001-2021, OFFIS e.V.
+ * Copyright (C) 2001-2022, OFFIS e.V.
* All rights reserved. See COPYRIGHT file for details.
*
* This software and supporting documentation were developed by
@@ -3245,6 +3245,39 @@ void OFStandard::forceSleep(Uint32 seconds)
}
}
+
+void OFStandard::sanitizeFilename(OFString& fname)
+{
+ size_t len = fname.length();
+ for (size_t i=0; i<len; ++i)
+ {
+#ifdef _WIN32
+ if ((fname[i] == PATH_SEPARATOR)||(fname[i] == '/')) fname[i] = '_';
+#else
+ if (fname[i] == PATH_SEPARATOR) fname[i] = '_';
+#endif
+ }
+}
+
+
+void OFStandard::sanitizeFilename(char *fname)
+{
+ if (fname)
+ {
+ char *c = fname;
+ while (*c)
+ {
+#ifdef _WIN32
+ if ((*c == PATH_SEPARATOR)||(*c == '/')) *c = '_';
+#else
+ if (*c == PATH_SEPARATOR) *c = '_';
+#endif
+ ++c;
+ }
+ }
+}
+
+
#include DCMTK_DIAGNOSTIC_IGNORE_STRICT_ALIASING_WARNING
// black magic:
--
2.30.2
