From: Pawel Wasowski <p.wasowski2@samsung.com>
Date: Wed, 8 Jan 2020 10:28:08 +0000 (+0100)
Subject: [Archive] Prevent extracting files with ".." in relative paths
X-Git-Tag: submit/tizen/20200116.071506^2^2^2^2~1
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=refs%2Fchanges%2F66%2F221966%2F1;p=platform%2Fcore%2Fapi%2Fwebapi-plugins.git

[Archive] Prevent extracting files with ".." in relative paths

This change mitigates a potential security issue, which could occur if
a zip archive contained files with ".." in their paths.
ArchiveEntry.extract() and Archive.extractAll() will not extract such
files.

Verification: auto tct-tizen-archive-tests pass rate: 100%
              Manual tests: attempts to extract files with forbidden
              ".." results in an UnknownError.

Change-Id: I563744d834d24e896493f55d15e579e714d539f9
Signed-off-by: Pawel Wasowski <p.wasowski2@samsung.com>
---

diff --git a/src/archive/archive_file.cc b/src/archive/archive_file.cc
index 797f9632..39da1ec2 100644
--- a/src/archive/archive_file.cc
+++ b/src/archive/archive_file.cc
@@ -256,6 +256,30 @@ PlatformResult ArchiveFile::addOperation(OperationCallbackData* callback) {
   return PlatformResult(ErrorCode::NO_ERROR);
 }
 
+namespace {
+
+PlatformResult anyFilePathContainsForbiddenSubstrings(UnZip& unzip) {
+  unsigned long irrelevant_info = 0;
+  ArchiveFileEntryPtrMapPtr entries_map = std::make_shared<ArchiveFileEntryPtrMap>();
+  auto result = unzip.listEntries(&irrelevant_info, &entries_map);
+  if (!result) {
+    LoggerE("Failed to list archive entries");
+    return result;
+  }
+
+  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";
+      return LogAndCreateResult(ErrorCode::UNKNOWN_ERR, error_msg, ("%s", error_msg.c_str()));
+    }
+  }
+
+  return PlatformResult{ErrorCode::NO_ERROR};
+}
+
+}  // namespace
+
 PlatformResult ArchiveFile::extractAllTask(ExtractAllProgressCallback* callback) {
   ScopeLogger();
   filesystem::FilePtr directory = callback->getDirectory();
@@ -305,6 +329,11 @@ PlatformResult ArchiveFile::extractAllTask(ExtractAllProgressCallback* callback)
     return result;
   }
 
+  result = anyFilePathContainsForbiddenSubstrings(*unzip);
+  if (!result) {
+    return result;
+  }
+
   return unzip->extractAllFilesTo(directory->getNode()->getPath()->getFullPath(), callback);
 }
 
diff --git a/src/archive/archive_instance.cc b/src/archive/archive_instance.cc
index 83063e75..092ab22b 100644
--- a/src/archive/archive_instance.cc
+++ b/src/archive/archive_instance.cc
@@ -529,6 +529,14 @@ void ArchiveInstance::Extract(const picojson::value& args, picojson::object& out
   picojson::value v_entry_name = data.at(PARAM_NAME);
 
   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";
+    LoggerE("%s", error_msg.c_str());
+    PostError(PlatformResult{ErrorCode::UNKNOWN_ERR, error_msg}, callbackId);
+    return;
+  }
+
   const long operationId = static_cast<long>(v_op_id.get<double>());
   const long handle = static_cast<long>(v_handle.get<double>());
 
diff --git a/src/archive/archive_utils.cc b/src/archive/archive_utils.cc
index 3c91f2ba..f07be1de 100644
--- a/src/archive/archive_utils.cc
+++ b/src/archive/archive_utils.cc
@@ -224,5 +224,17 @@ 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());
+
+  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;
+    }
+  }
+  return false;
+}
+
 }  // namespace archive
 }  // namespace extension
diff --git a/src/archive/archive_utils.h b/src/archive/archive_utils.h
index 74e7aa5d..e3a0feb7 100644
--- a/src/archive/archive_utils.h
+++ b/src/archive/archive_utils.h
@@ -105,6 +105,14 @@ std::string getBasePathFromPath(const std::string& fullpath);
 
 std::string getArchiveLogMessage(const int errorCode, const std::string& hint);
 
+/**
+ * The API will not allow some operations on files that contain forbidden
+ * substrings in their paths. This is done to prevent path traversal attacks,
+ * i.e. extracting files to paths that the application should not access.
+ */
+const std::vector<std::string> kProhibitedSubstrings = {".."};
+bool pathContainsProhibitedSubstrings(const std::string& path);
+
 }  // namespace archive
 }  // namespace extension
 
diff --git a/src/archive/un_zip_extract_request.cc b/src/archive/un_zip_extract_request.cc
index 69964704..4772c5c2 100644
--- a/src/archive/un_zip_extract_request.cc
+++ b/src/archive/un_zip_extract_request.cc
@@ -94,8 +94,9 @@ void createMissingDirectories(const std::string& path, bool check_first = true)
         // permissions would be changed after extract process would be finished,
         // for now using default 0775
         if (mkdir(left_part.c_str(), S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH) == -1) {
-          LoggerE("Couldn't create new directory: %s errno: %s", left_part.c_str(),
-                  GetErrorString(errno).c_str());
+          int errno_copy = errno;
+          LoggerE("Couldn't create new directory: %s errno: %d (%s)", left_part.c_str(), errno,
+                  GetErrorString(errno_copy).c_str());
         }
       }
     }