Merge pull request #2327 from ashnaaseth2325-oss/fix/zip-slip-extraction

Fix zip slip in ODT/IDML template extraction

Author: sydseter

.github/workflows/build-website.yml

@@ -37,7 +37,7 @@
           - name: Install Node.js
             uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6.2.0
             with:
-              node-version: 20.18.2
+              node-version: '22.12'
           - name: Build
             working-directory: cornucopia.owasp.org
             run: |

scripts/convert.py

@@ -87,6 +87,31 @@ def _validate_file_paths(source_filename: str, output_pdf_filename: str) -> Tupl
     return True, source_path, output_dir
 
 
+def _safe_extractall(archive: zipfile.ZipFile, target_dir: str) -> None:
+    """Extract zip members only if their resolved paths stay within target_dir.
+
+    Prevents Zip Slip / path traversal (CWE-22) by resolving symlinks and all
+    '..' components before comparing each member path against the target root.
+    Degenerate root entries ('.', '', './') are skipped rather than extracted,
+    because they carry no file content and resolve to the target directory itself.
+    """
+    abs_target = os.path.realpath(target_dir)
+    for member in archive.infolist():
+        member_path = os.path.realpath(os.path.join(abs_target, member.filename))
+
+        # Root/degenerate entries ('.', '', './') resolve to abs_target itself.
+        # They are directory metadata with no content; skip them safely.
+        if member_path == abs_target:
+            continue
+
+        # Block any member whose resolved path escapes the target directory.
+        # The os.sep suffix prevents prefix collisions (e.g. /tmp/d vs /tmp/d_evil).
+        if not member_path.startswith(abs_target + os.sep):
+            raise ValueError(f"Zip Slip blocked: member '{member.filename}' would extract outside target directory")
+
+        archive.extract(member, target_dir)
+
+
 def _validate_command_args(cmd_args: List[str]) -> bool:
     """Validate command arguments for dangerous characters."""
     dangerous_chars = ["&", "|", ";", "$", "`", "(", ")", "<", ">", "*", "?", "[", "]", "{", "}", "\\"]
@@ -338,10 +363,12 @@ def main() -> None:
     logging.debug(" --- args = %s", str(convert_vars.args))
 
     set_can_convert_to_pdf()
-    if convert_vars.args.pdf and not convert_vars.can_convert_to_pdf and not convert_vars.args.debug:
+    libreoffice_available = bool(shutil.which("libreoffice") or shutil.which("soffice"))
+    can_make_pdf = convert_vars.can_convert_to_pdf or libreoffice_available
+    if convert_vars.args.pdf and not can_make_pdf and not convert_vars.args.debug:
         logging.error(
             "Cannot convert to pdf on this system. "
-            "Pdf conversion is available on Windows and Mac, if MS Word is installed"
+            "Pdf conversion is available on Windows and Mac (with MS Word), or on any system with LibreOffice."
         )
         return
 
@@ -966,7 +993,7 @@ def save_odt_file(template_doc: str, language_dict: Dict[str, str], output_file:
 
     # Unzip source xml files and place in temp output folder
     with zipfile.ZipFile(template_doc) as odt_archive:
-        odt_archive.extractall(temp_output_path)
+        _safe_extractall(odt_archive, temp_output_path)
 
     # ODT text is usually in content.xml and sometimes styles.xml
     targets = ["content.xml", "styles.xml"]
@@ -996,7 +1023,7 @@ def save_idml_file(template_doc: str, language_dict: Dict[str, str], output_file
 
     # Unzip source xml files and place in temp output folder
     with zipfile.ZipFile(template_doc) as idml_archive:
-        idml_archive.extractall(temp_output_path)
+        _safe_extractall(idml_archive, temp_output_path)
         logging.debug(" --- namelist of first few files in archive = %s", str(idml_archive.namelist()[:5]))
 
     xml_files = get_files_from_of_type(temp_output_path, "xml")

tests/scripts/convert_utest.py

@@ -1,9 +1,12 @@
 import unittest
 import unittest.mock as mock
 import argparse
+import io
 import os
 import platform
 import sys
+import tempfile
+import zipfile
 import docx  # type: ignore
 import logging
 import glob
@@ -2156,5 +2159,57 @@ def test_get_paragraphs_from_table_in_doc(self) -> None:
         self.assertGreater(len(paragraphs), want_min_len_paragraphs)
 
 
+class TestSafeExtractAll(unittest.TestCase):
+    """Unit tests for _safe_extractall covering CWE-22 path traversal prevention."""
+
+    def _build_zip(self, members: typing.Dict[str, str]) -> io.BytesIO:
+        buf = io.BytesIO()
+        with zipfile.ZipFile(buf, "w") as zf:
+            for name, content in members.items():
+                zf.writestr(zipfile.ZipInfo(name), content)
+        buf.seek(0)
+        return buf
+
+    def test_blocks_parent_directory_traversal(self) -> None:
+        """Zip Slip via '../' in member filename must raise ValueError."""
+        buf = self._build_zip({"../evil.txt": "pwned"})
+        with tempfile.TemporaryDirectory() as td:
+            with zipfile.ZipFile(buf) as zf:
+                with self.assertRaises(ValueError) as ctx:
+                    c._safe_extractall(zf, td)
+        self.assertIn("Zip Slip blocked", str(ctx.exception))
+
+    def test_blocks_absolute_path_member(self) -> None:
+        """/etc/passwd as a member filename must be blocked."""
+        buf = self._build_zip({"/etc/passwd": "pwned"})
+        with tempfile.TemporaryDirectory() as td:
+            with zipfile.ZipFile(buf) as zf:
+                with self.assertRaises(ValueError):
+                    c._safe_extractall(zf, td)
+
+    def test_allows_legitimate_nested_members(self) -> None:
+        """Normal nested paths must extract correctly."""
+        buf = self._build_zip({"content.xml": "<r/>", "subdir/file.xml": "<s/>"})
+        with tempfile.TemporaryDirectory() as td:
+            with zipfile.ZipFile(buf) as zf:
+                c._safe_extractall(zf, td)
+            self.assertTrue(os.path.isfile(os.path.join(td, "content.xml")))
+            self.assertTrue(os.path.isfile(os.path.join(td, "subdir", "file.xml")))
+
+    def test_skips_root_dot_entry(self) -> None:
+        """A '.' root directory entry must be skipped without error."""
+        buf = io.BytesIO()
+        with zipfile.ZipFile(buf, "w") as zf:
+            dot = zipfile.ZipInfo(".")
+            dot.external_attr = 0o40755 << 16
+            zf.writestr(dot, "")
+            zf.writestr("content.xml", "<r/>")
+        buf.seek(0)
+        with tempfile.TemporaryDirectory() as td:
+            with zipfile.ZipFile(buf) as zf:
+                c._safe_extractall(zf, td)
+            self.assertTrue(os.path.isfile(os.path.join(td, "content.xml")))
+
+
 if __name__ == "__main__":
     unittest.main()