[Archive] Changing handling of Zip Path Traversal Vulnerability

This commit introduces verification of destination of extracted elements.
Before this commit ".." occurrence in archive entry resulted in
exception.

[Verification]
Manual test in developer console with prepared archive file shows
correct exception for ArchiveFile::extractAll and
ArchiveFileEntry::extract methods on TM1 and TW2.
tct-archive-tizen-tests pass rate: 100% on TM1 and TW2.

Change-Id: I823ff3d161bf1897fe98b6746cc0a1f60c874543
Signed-off-by: Arkadiusz Pietraszek <a.pietraszek@samsung.com>

diff --git a/src/archive/archive_file.cc b/src/archive/archive_file.cc
index ac4a95c..1bd22f3 100644 (file)
--- a/src/archive/archive_file.cc
+++ b/src/archive/archive_file.cc
@@ -259,7 +259,8 @@ PlatformResult ArchiveFile::addOperation(OperationCallbackData* callback) {
 
 namespace {
 
-PlatformResult anyFilePathContainsForbiddenSubstrings(UnZip& unzip) {
+PlatformResult anyFilePathTargetsDirectoryOutsideDestination(UnZip& unzip,
+                                                             const std::string& destinationPath) {
   unsigned long irrelevant_info = 0;
   ArchiveFileEntryPtrMapPtr entries_map = std::make_shared<ArchiveFileEntryPtrMap>();
   auto result = unzip.listEntries(&irrelevant_info, &entries_map);
@@ -270,8 +271,9 @@ PlatformResult anyFilePathContainsForbiddenSubstrings(UnZip& unzip) {
 
   for (auto& file_path_and_file_info : *entries_map) {
     const auto& file_path = file_path_and_file_info.first;
-    if (pathContainsProhibitedSubstrings(file_path)) {
-      const std::string error_msg = "File path " + file_path + " conatins a forbidden substring";
+    if (pathTargetsDirectoryOutsideDestination(file_path, destinationPath)) {
+      const std::string error_msg =
+          "Entry path: " + file_path + " targets directory outside destination: " + destinationPath;
       return LogAndCreateResult(ErrorCode::UNKNOWN_ERR, error_msg, ("%s", error_msg.c_str()));
     }
   }
@@ -330,12 +332,14 @@ PlatformResult ArchiveFile::extractAllTask(ExtractAllProgressCallback* callback)
     return result;
   }
 
-  result = anyFilePathContainsForbiddenSubstrings(*unzip);
+  std::string fullPath = directory->getNode()->getPath()->getFullPath();
+
+  result = anyFilePathTargetsDirectoryOutsideDestination(*unzip, fullPath);
   if (!result) {
     return result;
   }
 
-  return unzip->extractAllFilesTo(directory->getNode()->getPath()->getFullPath(), callback);
+  return unzip->extractAllFilesTo(fullPath, callback);
 }
 
 PlatformResult ArchiveFile::getEntries(GetEntriesCallbackData* callback) {

diff --git a/src/archive/archive_instance.cc b/src/archive/archive_instance.cc
index ac5c979..bf38808 100644 (file)
--- a/src/archive/archive_instance.cc
+++ b/src/archive/archive_instance.cc
@@ -534,8 +534,10 @@ void ArchiveInstance::ArchiveFileEntryExtract(const picojson::value& args, picoj
 
   const double callbackId = args.get(JSON_CALLBACK_ID).get<double>();
   const auto entry_path = v_entry_name.get<std::string>();
-  if (pathContainsProhibitedSubstrings(entry_path)) {
-    const std::string error_msg = "Entry path " + entry_path + " contains a prohibited substring";
+  if (pathTargetsDirectoryOutsideDestination(entry_path, v_dest_dir.get<std::string>())) {
+    const std::string error_msg = "Entry path: " + entry_path +
+                                  " targets directory outside destination: " +
+                                  v_dest_dir.get<std::string>();
     LoggerE("%s", error_msg.c_str());
     PostError(PlatformResult{ErrorCode::UNKNOWN_ERR, error_msg}, callbackId);
     return;

diff --git a/src/archive/archive_utils.cc b/src/archive/archive_utils.cc
index f07be1d..ec6b564 100644 (file)
--- a/src/archive/archive_utils.cc
+++ b/src/archive/archive_utils.cc
@@ -224,14 +224,56 @@ std::string getArchiveLogMessage(const int errorCode, const std::string& hint) {
   return std::string(ss.str());
 }
 
-bool pathContainsProhibitedSubstrings(const std::string& path) {
-  ScopeLogger("Path: %s", path.c_str());
+std::string normalizePath(const std::string& destinationPath, const std::string& targetEntryPath) {
+  ScopeLogger("targetEntryPath: %s,  destinationPath: %s", targetEntryPath.c_str(),
+              destinationPath.c_str());
+
+  static constexpr char delimiter = '/';
+  std::string fullPath = destinationPath + delimiter + targetEntryPath;
+
+  // removing "//"
+  std::size_t pos = fullPath.find("//");
+  while (std::string::npos != pos) {
+    fullPath.erase(pos, 1);
+    pos = fullPath.find("//");
+  }
+
+  // removing "/./"
+  pos = fullPath.find("/./");
+  while (std::string::npos != pos) {
+    fullPath.erase(pos, 2);
+    pos = fullPath.find("/./");
+  }
 
-  for (const auto& prohibited_substring : kProhibitedSubstrings) {
-    if (path.find(prohibited_substring) != std::string::npos) {
-      LoggerE("Prohibited substring found: %s", prohibited_substring.c_str());
-      return true;
+  // handling ".."
+  std::size_t pos2 = fullPath.find("..");
+  while (std::string::npos != pos2) {
+    if (2 > pos2) {
+      LoggerE("Too many parent directory references (\"..\") in path: %s", fullPath.c_str());
+      return "";
     }
+    pos = fullPath.find_last_of("/", pos2 - 2);
+    if (std::string::npos == pos) {
+      LoggerE("Too many parent directory references (\"..\") in path: %s", fullPath.c_str());
+      return "";
+    }
+    fullPath.erase(pos, (pos2 - pos + 2));
+    pos2 = fullPath.find("..");
+  }
+
+  return fullPath;
+}
+
+bool pathTargetsDirectoryOutsideDestination(const std::string& targetEntryPath,
+                                            const std::string& destinationPath) {
+  ScopeLogger("targetEntryPath: %s,  destinationPath: %s", targetEntryPath.c_str(),
+              destinationPath.c_str());
+
+  std::string normalizedPath = normalizePath(destinationPath, targetEntryPath);
+
+  if (!normalizedPath.rfind(destinationPath, 0) == 0) {
+    LoggerE("Destination outside of allowed directory: %s", normalizedPath.c_str());
+    return true;
   }
   return false;
 }

diff --git a/src/archive/archive_utils.h b/src/archive/archive_utils.h
index e3a0feb..faddc5f 100644 (file)
--- a/src/archive/archive_utils.h
+++ b/src/archive/archive_utils.h
@@ -111,7 +111,8 @@ std::string getArchiveLogMessage(const int errorCode, const std::string& hint);
  * i.e. extracting files to paths that the application should not access.
  */
 const std::vector<std::string> kProhibitedSubstrings = {".."};
-bool pathContainsProhibitedSubstrings(const std::string& path);
+bool pathTargetsDirectoryOutsideDestination(const std::string& targetEntryPath,
+                                            const std::string& destinationPath);
 
 }  // namespace archive
 }  // namespace extension