Mercurial > hg > orthanc

changeset 2352:3ab96768d144

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
Fix issue #52 (DICOM level security association problems)
author Sebastien Jodogne <s.jodogne@gmail.com>
date Tue, 18 Jul 2017 17:33:26 +0200
parents 56504f89d4ac
children 2421c137c304 415450f11cc7
files NEWS OrthancServer/DicomProtocol/DicomUserConnection.cpp OrthancServer/Internals/CommandDispatcher.cpp OrthancServer/OrthancInitialization.cpp OrthancServer/OrthancInitialization.h OrthancServer/ServerEnumerations.cpp OrthancServer/ServerEnumerations.h OrthancServer/main.cpp Resources/Configuration.json UnitTestsSources/UnitTestsMain.cpp
diffstat 10 files changed, 79 insertions(+), 37 deletions(-) [+]
line wrap: on
line diff
--- a/NEWS	Tue Jul 18 08:41:09 2017 +0200
+++ b/NEWS	Tue Jul 18 17:33:26 2017 +0200
@@ -7,12 +7,12 @@
 * Orthanc now anonymizes according to Basic Profile of PS 3.15-2017c Table E.1-1
 * In the "DicomModalities" configuration:
   - Manufacturer type MedInria is now obsolete
-  - Manufacturer types AgfaImpax and SyngoVia are obsolete too,
-    use GenericNoWildcardInDates instead
+  - Manufacturer types AgfaImpax and SyngoVia are obsolete too
+    (use GenericNoWildcardInDates instead)
   - Obsolete manufacturers are still accepted but might disappear in the future
-  - Added new manufacturer: GenericNoWidlcards to replace all '*' by '' in
+  - Added new manufacturer: GenericNoUniversalWildcard to replace all '*' by '' in
     outgoing C-Find requests
-* "Locale" configuration option
+* New security-related options: "DicomAlwaysAllowStore" and "DicomCheckModalityHost"
 
 REST API
 --------
@@ -46,6 +46,7 @@
 * Fix issue 45 (crash when providing a folder to "--config" command-line option)
 * Fix issue 46 (PHI remaining after anonymization)
 * Fix issue 49 (worklists: accentuated characters are removed from C-Find responses)
+* Fix issue 52 (DICOM level security association problems)
 * Fix issue 55 (modification/anonymization of tags that might break the database
   model now requires the "Force" parameter to be set to "true" in the query)
 * Fix issue 56 (case-insensitive matching over accents)
@@ -53,7 +54,8 @@
 * Fix XSS inside DICOM in Orthanc Explorer (as reported by Victor Pasnkel, Morphus Labs)
 * Upgrade to DCMTK 3.6.2 in static builds (released on 2017-07-17)
 * Upgrade to Boost 1.64.0 in static builds
-* Removed configuration option USE_DCMTK_361_PRIVATE_DIC
+* New advanced "Locale" configuration option
+* Removed configuration option "USE_DCMTK_361_PRIVATE_DIC"
 
 
 Version 1.2.0 (2016/12/13)
--- a/OrthancServer/DicomProtocol/DicomUserConnection.cpp	Tue Jul 18 08:41:09 2017 +0200
+++ b/OrthancServer/DicomProtocol/DicomUserConnection.cpp	Tue Jul 18 17:33:26 2017 +0200
@@ -496,7 +496,7 @@
     switch (manufacturer)
     {
       case ModalityManufacturer_GenericNoWildcardInDates:
-      case ModalityManufacturer_GenericNoWildcards:
+      case ModalityManufacturer_GenericNoUniversalWildcard:
       {
         std::auto_ptr<DicomMap> fix(fields.Clone());
 
@@ -508,7 +508,7 @@
           // Replace a "*" wildcard query by an empty query ("") for
           // "date" or "all" value representations depending on the
           // type of manufacturer.
-          if (manufacturer == ModalityManufacturer_GenericNoWildcards ||
+          if (manufacturer == ModalityManufacturer_GenericNoUniversalWildcard ||
               (manufacturer == ModalityManufacturer_GenericNoWildcardInDates &&
                FromDcmtkBridge::LookupValueRepresentation(*it) == ValueRepresentation_Date))
           {
--- a/OrthancServer/Internals/CommandDispatcher.cpp	Tue Jul 18 08:41:09 2017 +0200
+++ b/OrthancServer/Internals/CommandDispatcher.cpp	Tue Jul 18 17:33:26 2017 +0200
@@ -806,13 +806,12 @@
 
         // Check whether this request is allowed by the security filter
         if (supported && 
-            request != DicomRequestType_Echo &&  // Always allow incoming ECHO requests
             filter_ != NULL &&
             !filter_->IsAllowedRequest(remoteIp_, remoteAet_, calledAet_, request))
         {
-          LOG(ERROR) << EnumerationToString(request) 
-                     << " requests are disallowed for the AET \"" 
-                     << remoteAet_ << "\"";
+          LOG(WARNING) << "Rejected " << EnumerationToString(request)
+                       << " request from remote DICOM modality with AET \""
+                       << remoteAet_ << "\" and hostname \"" << remoteIp_ << "\"";
           cond = DIMSE_ILLEGALASSOCIATION;
           supported = false;
           finished = true;
@@ -906,7 +905,7 @@
         else
         {
           OFString temp_str;
-          LOG(ERROR) << "DIMSE failure (aborting association): " << cond.text();
+          LOG(INFO) << "DIMSE failure (aborting association): " << cond.text();
           /* some kind of error so abort the association */
           ASC_abortAssociation(assoc_);
         }
--- a/OrthancServer/OrthancInitialization.cpp	Tue Jul 18 08:41:09 2017 +0200
+++ b/OrthancServer/OrthancInitialization.cpp	Tue Jul 18 17:33:26 2017 +0200
@@ -855,10 +855,30 @@
   }
 
 
-  bool Configuration::IsKnownAETitle(const std::string& aet)
+  bool Configuration::IsKnownAETitle(const std::string& aet,
+                                     const std::string& ip)
   {
     RemoteModalityParameters modality;
-    return LookupDicomModalityUsingAETitle(modality, aet);
+    
+    if (!LookupDicomModalityUsingAETitle(modality, aet))
+    {
+      LOG(WARNING) << "Modality \"" << aet
+                   << "\" is not listed in the \"DicomModalities\" configuration option";
+      return false;
+    }
+    else if (!Configuration::GetGlobalBoolParameter("DicomCheckModalityHost", false) ||
+             ip == modality.GetHost())
+    {
+      return true;
+    }
+    else
+    {
+      LOG(WARNING) << "Forbidding access from AET \"" << aet
+                   << "\" given its hostname (" << ip << ") does not match "
+                   << "the \"DicomModalities\" configuration option ("
+                   << modality.GetHost() << " was expected)";
+      return false;
+    }
   }
 
 
--- a/OrthancServer/OrthancInitialization.h	Tue Jul 18 08:41:09 2017 +0200
+++ b/OrthancServer/OrthancInitialization.h	Tue Jul 18 17:33:26 2017 +0200
@@ -106,7 +106,8 @@
     static void GetGlobalListOfStringsParameter(std::list<std::string>& target,
                                                 const std::string& key);
 
-    static bool IsKnownAETitle(const std::string& aet);
+    static bool IsKnownAETitle(const std::string& aet,
+                               const std::string& ip);
 
     static bool IsSameAETitle(const std::string& aet1,
                               const std::string& aet2);
--- a/OrthancServer/ServerEnumerations.cpp	Tue Jul 18 08:41:09 2017 +0200
+++ b/OrthancServer/ServerEnumerations.cpp	Tue Jul 18 17:33:26 2017 +0200
@@ -328,8 +328,8 @@
       case ModalityManufacturer_GenericNoWildcardInDates:
         return "GenericNoWildcardInDates";
 
-      case ModalityManufacturer_GenericNoWildcards:
-        return "GenericNoWildcards";
+      case ModalityManufacturer_GenericNoUniversalWildcard:
+        return "GenericNoUniversalWildcard";
 
       case ModalityManufacturer_StoreScp:
         return "StoreScp";
@@ -393,9 +393,9 @@
     {
       return ModalityManufacturer_GenericNoWildcardInDates;
     }
-    else if (manufacturer == "GenericNoWildcards")
+    else if (manufacturer == "GenericNoUniversalWildcard")
     {
-      return ModalityManufacturer_GenericNoWildcards;
+      return ModalityManufacturer_GenericNoUniversalWildcard;
     }
     else if (manufacturer == "ClearCanvas")
     {
--- a/OrthancServer/ServerEnumerations.h	Tue Jul 18 08:41:09 2017 +0200
+++ b/OrthancServer/ServerEnumerations.h	Tue Jul 18 17:33:26 2017 +0200
@@ -60,7 +60,7 @@
   {
     ModalityManufacturer_Generic,
     ModalityManufacturer_GenericNoWildcardInDates,
-    ModalityManufacturer_GenericNoWildcards,
+    ModalityManufacturer_GenericNoUniversalWildcard,
     ModalityManufacturer_StoreScp,
     ModalityManufacturer_ClearCanvas,
     ModalityManufacturer_Dcm4Chee,
--- a/OrthancServer/main.cpp	Tue Jul 18 08:41:09 2017 +0200
+++ b/OrthancServer/main.cpp	Tue Jul 18 17:33:26 2017 +0200
@@ -158,27 +158,33 @@
   {
   }
 
-  virtual bool IsAllowedConnection(const std::string& /*remoteIp*/,
-                                   const std::string& /*remoteAet*/,
-                                   const std::string& /*calledAet*/)
+  virtual bool IsAllowedConnection(const std::string& remoteIp,
+                                   const std::string& remoteAet,
+                                   const std::string& calledAet)
   {
+    LOG(INFO) << "Incoming connection from AET " << remoteAet
+              << " on IP " << remoteIp << ", calling AET " << calledAet;
+    
     return true;
   }
 
-  virtual bool IsAllowedRequest(const std::string& /*remoteIp*/,
+  virtual bool IsAllowedRequest(const std::string& remoteIp,
                                 const std::string& remoteAet,
-                                const std::string& /*calledAet*/,
+                                const std::string& calledAet,
                                 DicomRequestType type)
   {
-    if (type == DicomRequestType_Store)
+    LOG(INFO) << "Incoming " << Orthanc::EnumerationToString(type) << " request from AET "
+              << remoteAet << " on IP " << remoteIp << ", calling AET " << calledAet;
+    
+    if (type == DicomRequestType_Store &&
+        Configuration::GetGlobalBoolParameter("DicomAlwaysAllowStore", true))
     {
       // Incoming store requests are always accepted, even from unknown AET
       return true;
     }
 
-    if (!Configuration::IsKnownAETitle(remoteAet))
+    if (!Configuration::IsKnownAETitle(remoteAet, remoteIp))
     {
-      LOG(ERROR) << "Unknown remote DICOM modality AET: \"" << remoteAet << "\"";
       return false;
     }
     else
--- a/Resources/Configuration.json	Tue Jul 18 08:41:09 2017 +0200
+++ b/Resources/Configuration.json	Tue Jul 18 17:33:26 2017 +0200
@@ -82,7 +82,8 @@
   // The DICOM Application Entity Title
   "DicomAet" : "ORTHANC",
 
-  // Check whether the called AET corresponds during a DICOM request
+  // Check whether the called AET corresponds to the AET of Orthanc
+  // during an incoming DICOM SCU request
   "DicomCheckCalledAet" : false,
 
   // The DICOM port
@@ -156,18 +157,31 @@
     // "sample" : [ "STORESCP", "127.0.0.1", 2000 ]
 
     /**
-     * A fourth parameter is available to enable patches for a
-     * specific PACS manufacturer. The allowed values are currently
-     * "Generic" (default value),
-     * "GenericNoWildcardInDates" (to replace '*' by '' in date fields in outgoing C-Find)
-     * "GenericNoWildcards" (to replace '*' by '' in all fields in outgoing C-Find)
-     * "StoreScp" (storescp tool from DCMTK),
-     * "ClearCanvas", "Dcm4Chee" and "Vitrea".
+     * A fourth parameter is available to enable patches for
+     * specific PACS manufacturers. The allowed values are currently:
+     * - "Generic" (default value),
+     * - "GenericNoWildcardInDates" (to replace "*" by "" in date fields 
+     *   in outgoing C-Find requests originating from Orthanc)
+     * - "GenericNoUniversalWildcard" (to replace "*" by "" in all fields
+     *   in outgoing C-Find SCU requests originating from Orthanc)
+     * - "StoreScp" (storescp tool from DCMTK),
+     * - "ClearCanvas", "Dcm4Chee" and "Vitrea".
      * This parameter is case-sensitive.
      **/
     // "clearcanvas" : [ "CLEARCANVAS", "192.168.1.1", 104, "ClearCanvas" ]
   },
 
+  // Whether the Orthanc SCP allows incoming C-Store requests, even
+  // from SCU modalities it does not know about (i.e. that are not
+  // listed in the "DicomModalities" option above)
+  "DicomAlwaysAllowStore" : true,
+
+  // Whether Orthanc checks the IP/hostname address of the remote
+  // modality initiating a DICOM connection (as listed in the
+  // "DicomModalities" option above). If this option is set to
+  // "false", Orthanc only checks the AET of the remote modality.
+  "DicomCheckModalityHost" : false,
+
   // The timeout (in seconds) after which the DICOM associations are
   // considered as closed by the Orthanc SCU (client) if the remote
   // DICOM SCP (server) does not answer.
--- a/UnitTestsSources/UnitTestsMain.cpp	Tue Jul 18 08:41:09 2017 +0200
+++ b/UnitTestsSources/UnitTestsMain.cpp	Tue Jul 18 17:33:26 2017 +0200
@@ -528,7 +528,7 @@
 
   ASSERT_STREQ("Generic", EnumerationToString(StringToModalityManufacturer("Generic")));
   ASSERT_STREQ("GenericNoWildcardInDates", EnumerationToString(StringToModalityManufacturer("GenericNoWildcardInDates")));
-  ASSERT_STREQ("GenericNoWildcards", EnumerationToString(StringToModalityManufacturer("GenericNoWildcards")));
+  ASSERT_STREQ("GenericNoUniversalWildcard", EnumerationToString(StringToModalityManufacturer("GenericNoUniversalWildcard")));
   ASSERT_STREQ("StoreScp", EnumerationToString(StringToModalityManufacturer("StoreScp")));
   ASSERT_STREQ("ClearCanvas", EnumerationToString(StringToModalityManufacturer("ClearCanvas")));
   ASSERT_STREQ("Dcm4Chee", EnumerationToString(StringToModalityManufacturer("Dcm4Chee")));