check for tarballs containing sketchy symlinks

It's acceptable for a tarball to have a symlink at `a/b/c/foo.txt` that
points to `../../../foo.txt` (see `legal_symlink_dots.tar`), because
that symlink "stays within" the archive. However, it should be illegal
for the same symlink to point to `../../../../foo.txt` (see
`illegal_symlink_dots.tar`), because that symlink "reaches outside" the
archive. Similarly, it should always be illegal for a tarball to hold a
symlink pointing to an absolute path.

Add validation and tests cases for these behaviors.

peru/resources/plugins/curl/curl_plugin.py

@@ -126,13 +126,17 @@ def plugin_sync(url, sha1):
 
 def extract_tar(archive_path, dest):
  with tarfile.open(archive_path) as t:
- validate_filenames(info.path for info in t.getmembers())
+ for info in t.getmembers():
+ validate_filename(info.path)
+ if info.issym():
+ validate_symlink(info.path, info.linkname)
  t.extractall(dest)
 
 
 def extract_zip(archive_path, dest):
  with zipfile.ZipFile(archive_path) as z:
- validate_filenames(z.namelist())
+ for name in z.namelist():
+ validate_filename(name)
  z.extractall(dest)
  # Set file permissions. Tar does this by default, but with zip we need
  # to do it ourselves.
@@ -153,11 +157,30 @@ def extract_zip(archive_path, dest):
  os.chmod(os.path.join(dest, info.filename), 0o755)
 
 
-def validate_filenames(names):
- for name in names:
- path = pathlib.PurePosixPath(name)
- if path.is_absolute() or '..' in path.parts:
- raise EvilArchiveError('Illegal path in archive: ' + name)
+def validate_filename(name):
+ path = pathlib.PurePosixPath(name)
+ if path.is_absolute() or ".." in path.parts:
+ raise EvilArchiveError("Illegal path in archive: " + name)
+
+
+def validate_symlink(name, target):
+ # We might do this twice but that's fine.
+ validate_filename(name)
+
+ allowed_parent_parts = len(pathlib.PurePosixPath(name).parts) - 1
+
+ target_path = pathlib.PurePosixPath(target)
+ if target_path.is_absolute():
+ raise EvilArchiveError("Illegal symlink target in archive: " + target)
+ leading_parent_parts = 0
+ for part in target_path.parts:
+ if part != "..":
+ break
+ leading_parent_parts += 1
+ if leading_parent_parts > allowed_parent_parts:
+ raise EvilArchiveError("Illegal symlink target in archive: " + target)
+ if ".." in target_path.parts[leading_parent_parts:]:
+ raise EvilArchiveError("Illegal symlink target in archive: " + target)
 
 
 class EvilArchiveError(RuntimeError):

tests/test_curl_plugin.py

@@ -104,6 +104,21 @@ def test_evil_archives(self):
  with self.assertRaises(curl_plugin.EvilArchiveError):
  curl_plugin.extract_tar(str(tar_archive), dest)
 
+ def test_evil_symlink_archives(self):
+ """Even worse than archives containing bad paths, an archive could
+ contain a *symlink* pointing to a bad path. Then a subsequent entry in
+ the *same* archive could write through the symlink."""
+ dest = shared.create_dir()
+ for case in ["illegal_symlink_dots", "illegal_symlink_absolute"]:
+ tar_archive = shared.test_resources / (case + ".tar")
+ with self.assertRaises(curl_plugin.EvilArchiveError):
+ curl_plugin.extract_tar(str(tar_archive), dest)
+ # But leading dots should be allowed in symlinks, as long as they don't
+ # escape the root of the archive.
+ for case in ["legal_symlink_dots"]:
+ tar_archive = shared.test_resources / (case + ".tar")
+ curl_plugin.extract_tar(str(tar_archive), dest)
+
  def test_request_has_user_agent_header(self):
  actual = curl_plugin.build_request("http://example.test")
  self.assertTrue(actual.has_header("User-agent"))