cifuzz: reject unsafe tar members in GitHub Actions artifacts

Author: 0xmrma

infra/cifuzz/filestore/github_actions/__init__.py

@@ -14,6 +14,7 @@
 """Implementation of a filestore using Github actions artifacts."""
 import logging
 import os
+from pathlib import PurePosixPath
 import shutil
 import sys
 import tarfile
@@ -127,10 +128,12 @@ def _download_artifact(self, name, dst_directory):
         logging.error('Artifact zip did not contain a tarfile.')
         return False
 
-      # TODO(jonathanmetzman): Replace this with archive.unpack from
-      # libClusterFuzz so we can avoid path traversal issues.
       with tarfile.TarFile(artifact_tarfile_path) as artifact_tarfile:
-        artifact_tarfile.extractall(dst_directory)
+        try:
+          _safe_extract_tar(artifact_tarfile, dst_directory)
+        except tarfile.TarError as error:
+          logging.error('Unsafe artifact tarfile: %s', error)
+          return False
     return True
 
   def _raw_download_artifact(self, name, dst_directory):
@@ -159,6 +162,31 @@ def download_coverage(self, name, dst_directory):
     return self._download_artifact(self.COVERAGE_PREFIX + name, dst_directory)
 
 
+def _is_safe_tar_member(member):
+  """Returns whether |member| is safe to extract."""
+  member_path = PurePosixPath(member.name)
+
+  if member_path.is_absolute():
+    return False
+
+  if '..' in member_path.parts:
+    return False
+
+  if member.issym() or member.islnk():
+    return False
+
+  return True
+
+
+def _safe_extract_tar(artifact_tarfile, dst_directory):
+  """Safely extracts |artifact_tarfile| into |dst_directory|."""
+  for member in artifact_tarfile.getmembers():
+    if not _is_safe_tar_member(member):
+      raise tarfile.TarError(f'Unsafe tar member: {member.name}')
+
+  artifact_tarfile.extractall(dst_directory)
+
+
 def _upload_artifact_with_upload_js(name, artifact_paths, directory):
   """Uploads the artifacts in |artifact_paths| that are located in |directory|
   to |name|, using the upload.js script."""

infra/cifuzz/filestore/github_actions/github_actions_test.py

@@ -226,6 +226,44 @@ def mock_download_and_unpack_zip_impl(url, download_artifact_temp_dir,
               os.path.join(artifact_download_dst_dir,
                            os.path.basename(self.testcase))))
 
+  @mock.patch('filestore.github_actions.GithubActionsFilestore._find_artifact')
+  @mock.patch('http_utils.download_and_unpack_zip')
+  def test_download_build_rejects_traversal_tar_member(
+      self, mock_download_and_unpack_zip, mock_find_artifact):
+    """Tests that traversal tar entries are rejected."""
+    artifact_name = 'cifuzz-build-address-commit'
+
+    def fake_download_and_unpack_zip(download_url, dst_directory, headers=None):
+      del download_url, headers
+      tar_path = os.path.join(dst_directory, artifact_name + '.tar')
+      safe_file = os.path.join(dst_directory, 'demo_fuzzer')
+      traversal_file = os.path.join(dst_directory, 'escape.txt')
+
+      with open(safe_file, 'w', encoding='utf-8') as file_handle:
+        file_handle.write('demo')
+
+      with open(traversal_file, 'w', encoding='utf-8') as file_handle:
+        file_handle.write('escape')
+
+      with tarfile.open(tar_path, 'w') as tar_handle:
+        tar_handle.add(safe_file, arcname='demo_fuzzer')
+        tar_handle.add(traversal_file, arcname='../escape.txt')
+      return True
+
+    mock_download_and_unpack_zip.side_effect = fake_download_and_unpack_zip
+    mock_find_artifact.return_value = {
+        'expired': False,
+        'name': artifact_name,
+        'archive_download_url': 'http://example.com/download'
+    }
+
+    filestore = github_actions.GithubActionsFilestore(self.config)
+    result = filestore.download_build('address-commit', self.local_dir)
+
+    self.assertFalse(result)
+    self.assertFalse(
+        os.path.exists(os.path.join(self.local_dir, 'demo_fuzzer')))
+
   @mock.patch('filestore.github_actions.github_api.list_artifacts')
   def test_find_artifact(self, mock_list_artifacts):
     """Tests that _find_artifact works as intended."""