Extract nested zip files regardless of conform file filter

openaddr/conform.py

@@ -203,26 +203,56 @@ def is_in(path, names):
     return False
 
 class ZipDecompressTask(DecompressionTask):
+    # Recursing into nested zips (see #35/#112) means a maliciously or
+    # accidentally crafted zip bomb could otherwise fill a job's disk before
+    # any per-entry file type filtering applies. Cap the declared
+    # (uncompressed) size of any single entry and how many nested zips deep
+    # we'll recurse, so worst case is bounded and the job fails loudly
+    # instead of exhausting disk.
+    MAX_ZIP_ENTRY_BYTES = 2 * 1024 ** 3  # 2GB
+    MAX_NESTED_ZIP_DEPTH = 10
+
     def decompress(self, source_paths, workdir, filenames):
         output_files = []
         expand_path = os.path.join(workdir, UNZIPPED_DIRNAME)
         mkdirsp(expand_path)
 
         # Extract contents of zip file into expand_path directory.
+        found = set()
         for source_path in source_paths:
-            self._extract_zip(source_path, expand_path, filenames)
+            found |= self._extract_zip(source_path, expand_path, filenames)
+
+        def fully_satisfied():
+            # Only short-circuit when there's an explicit file filter AND
+            # we can prove every name it asked for has actually been
+            # extracted - if filenames is empty (caller wants everything)
+            # or matching is inexact (e.g. a directory-style entry in
+            # `filenames` that `is_in()` matched but doesn't literally
+            # equal), this stays False and we fall back to full recursion,
+            # same as before this optimization existed.
+            return bool(filenames) and set(filenames) <= found
 
         # Recursively extract nested zip files, but fail if more than one zip
-        # appears at the same directory level.
+        # appears at the same directory level. Skip recursing at all once the
+        # requested file(s) are already found - there's no reason to keep
+        # opening nested zips we don't need, which also limits exposure to a
+        # zip bomb hiding deeper in the chain than what was asked for.
         processed = set()
-        pending = list(self._find_single_zips(expand_path))
+        pending = [] if fully_satisfied() else list(self._find_single_zips(expand_path))
 
         while pending:
+            if len(processed) >= self.MAX_NESTED_ZIP_DEPTH:
+                raise DecompressionError(
+                    "Refusing to recurse more than {} nested zip files deep - possible zip bomb"
+                    .format(self.MAX_NESTED_ZIP_DEPTH)
+                )
             zip_path = pending.pop()
             if zip_path in processed:
                 continue
             processed.add(zip_path)
-            self._extract_zip(zip_path, os.path.dirname(zip_path), filenames)
+            found |= self._extract_zip(zip_path, os.path.dirname(zip_path), filenames)
+            if fully_satisfied():
+                break
             pending.extend(self._find_single_zips(os.path.dirname(zip_path)))
 
         # Collect names of directories and files in expand_path directory.
@@ -232,20 +262,54 @@ def decompress(self, source_paths, workdir, filenames):
                     output_files.append(os.path.join(dirpath, dirname))
                     _L.debug("Expanded directory {}".format(output_files[-1]))
             for filename in filenames:
+                if filename.lower().endswith('.zip'):
+                    # A zip left un-recursed-into (e.g. because the request
+                    # was already satisfied without it, or filtered out at
+                    # its own directory level) isn't a usable source file.
+                    continue
                 output_files.append(os.path.join(dirpath, filename))
                 _L.debug("Expanded file {}".format(output_files[-1]))
 
         return output_files
 
     def _extract_zip(self, source_path, expand_path, filenames):
+        ''' Extract matching entries, returning the subset of `filenames`
+            (lower-cased) that were found by exact name - used by
+            decompress() to tell whether it's safe to stop recursing into
+            further nested zips.
+        '''
+        found = set()
         with ZipFile(source_path, 'r') as z:
-            for name in z.namelist():
+            for zinfo in z.infolist():
+                name = zinfo.filename
+
+                # Check the declared (uncompressed) size from the zip's
+                # central directory before extracting anything - a bomb's
+                # compressed size can be tiny, but its declared size isn't.
+                if zinfo.file_size > self.MAX_ZIP_ENTRY_BYTES:
+                    raise DecompressionError(
+                        "Refusing to extract {} - declared size {} bytes exceeds {} byte limit, possible zip bomb"
+                        .format(name, zinfo.file_size, self.MAX_ZIP_ENTRY_BYTES)
+                    )
+
+                # Nested zip files are always extracted regardless of the
+                # filenames filter, since the requested file may be inside
+                # one of them. The filter is re-applied when that nested
+                # zip is itself extracted.
+                if name.lower().endswith('.zip'):
+                    z.extract(zinfo, expand_path)
+                    continue
+
                 if len(filenames) and not is_in(name, filenames):
                     # Download only the named file, if any.
                     _L.debug("Skipped file {}".format(name))
                     continue
 
+                if len(filenames) and name.lower() in filenames:
+                    found.add(name.lower())
+
                 z.extract(name, expand_path)
+        return found
 
     def _find_single_zips(self, root_path):
         zip_paths = []

openaddr/tests/conform.py

@@ -12,6 +12,9 @@
 import tempfile
 import shutil
 
+from unittest import mock
+from zipfile import ZipFile
+
 from .. import SourceConfig
 
 from ..conform import (
@@ -25,7 +28,8 @@
     row_fxn_first_non_empty, row_fxn_constant, row_fxn_map,
     row_canonicalize_unit_and_number, conform_cli,
     convert_regexp_replace, normalize_ogr_filename_case,
-    is_in, geojson_source_to_csv, ogr_source_to_csv, check_source_tests
+    is_in, geojson_source_to_csv, ogr_source_to_csv, check_source_tests,
+    ZipDecompressTask, DecompressionError, elaborate_filenames
     )
 
 " Return an x,y array given a wkt point string"
@@ -2680,3 +2684,171 @@ def test_row_fxn_map_else(self):
 
         d = row_fxn_map(c, d, "accuracy", c.data_source["conform"]["accuracy"])
         self.assertEqual(e, d)
+
+
+class TestZipDecompressTask(unittest.TestCase):
+    ''' Regression tests for GitHub issue #35: "Support zipped shapefiles
+        within source zip" -- an outer source zip contains a nested zip that
+        itself contains the shapefile, plus miscellaneous extra files
+        (license, readme, lookup tables) sitting next to the nested zip.
+    '''
+
+    def setUp(self):
+        self.testdir = tempfile.mkdtemp(prefix='openaddr-TestZipDecompressTask-')
+        self.workdir = os.path.join(self.testdir, 'work')
+        os.mkdir(self.workdir)
+
+        # A real shapefile (.shp/.shx/.dbf/.prj) fixture, already zipped up.
+        self.inner_zip_path = os.path.join(
+            os.path.dirname(__file__), 'conforms', 'lake-man.zip')
+
+    def tearDown(self):
+        shutil.rmtree(self.testdir)
+
+    def _make_outer_zip(self, extra_files={'license.txt': 'be nice', 'readme.txt': 'read me'}):
+        ''' Build an outer.zip containing nested.zip (the shapefile zip)
+            alongside some unrelated non-zip files, matching the structure
+            described in issue #35.
+        '''
+        outer_zip_path = os.path.join(self.testdir, 'outer.zip')
+
+        with ZipFile(outer_zip_path, 'w') as outer_zip:
+            outer_zip.write(self.inner_zip_path, arcname='nested.zip')
+            for name, content in extra_files.items():
+                outer_zip.writestr(name, content)
+
+        return outer_zip_path
+
+    def test_single_nested_zip_is_extracted(self):
+        ''' A zip-of-a-zip-containing-a-shapefile, with no "file" filter
+            (i.e. no conform "file" tag), should be fully extracted so the
+            shapefile members end up available.
+        '''
+        outer_zip_path = self._make_outer_zip()
+
+        task = ZipDecompressTask()
+        output_files = task.decompress([outer_zip_path], self.workdir, [])
+
+        output_names = {os.path.basename(path) for path in output_files}
+        self.assertIn('lake-man.shp', output_names)
+        self.assertIn('lake-man.shx', output_names)
+        self.assertIn('lake-man.dbf', output_names)
+        self.assertIn('lake-man.prj', output_names)
+
+        # The extra sibling files should also have survived extraction.
+        self.assertIn('license.txt', output_names)
+        self.assertIn('readme.txt', output_names)
+
+        shp_path = next(path for path in output_files if path.endswith('lake-man.shp'))
+        self.assertGreater(os.path.getsize(shp_path), 0)
+
+    def test_single_nested_zip_is_extracted_with_file_filter(self):
+        ''' Same structure as above, but exercised the way a real source
+            would use it: with a conform "file" tag naming the shapefile
+            inside the nested zip (e.g. "file": "lake-man.shp"), which is
+            expanded by elaborate_filenames() into the .shp/.shx/.dbf/.prj
+            set and passed through as the `filenames` allow-list.
+        '''
+        outer_zip_path = self._make_outer_zip()
+        filenames = elaborate_filenames('lake-man.shp')
+
+        task = ZipDecompressTask()
+        output_files = task.decompress([outer_zip_path], self.workdir, filenames)
+
+        output_names = {os.path.basename(path) for path in output_files}
+        self.assertIn('lake-man.shp', output_names)
+        self.assertIn('lake-man.shx', output_names)
+        self.assertIn('lake-man.dbf', output_names)
+        self.assertIn('lake-man.prj', output_names)
+
+        # Sibling non-zip files that don't match the filter should still be
+        # skipped, since the caller only asked for the named shapefile.
+        self.assertNotIn('license.txt', output_names)
+        self.assertNotIn('readme.txt', output_names)
+
+    def test_multiple_nested_zips_raise_decompression_error(self):
+        ''' If more than one zip appears at the same directory level inside
+            the outer zip, extraction should fail loudly rather than
+            silently pick one.
+        '''
+        outer_zip_path = os.path.join(self.testdir, 'outer-ambiguous.zip')
+
+        with ZipFile(outer_zip_path, 'w') as outer_zip:
+            outer_zip.write(self.inner_zip_path, arcname='nested-a.zip')
+            outer_zip.write(self.inner_zip_path, arcname='nested-b.zip')
+
+        task = ZipDecompressTask()
+        with self.assertRaises(DecompressionError):
+            task.decompress([outer_zip_path], self.workdir, [])
+
+    def test_skips_nested_zip_once_filtered_file_is_already_found(self):
+        ''' If the outer zip already directly contains everything the
+            "file" filter asked for, an unrelated nested zip sitting next
+            to it should never be opened/extracted at all - there's no
+            reason to recurse once the request is satisfied, and not doing
+            so limits exposure to a zip bomb hiding in that nested zip.
+        '''
+        outer_zip_path = os.path.join(self.testdir, 'outer-already-satisfied.zip')
+        conforms_dir = os.path.join(os.path.dirname(__file__), 'conforms')
+
+        with ZipFile(outer_zip_path, 'w') as outer_zip:
+            for ext in ('.shp', '.shx', '.dbf', '.prj'):
+                outer_zip.write(
+                    os.path.join(conforms_dir, 'lake-man' + ext),
+                    arcname='lake-man' + ext
+                )
+            # An unrelated nested zip that should never get opened.
+            with tempfile.NamedTemporaryFile(suffix='.zip') as decoy_zip_file:
+                with ZipFile(decoy_zip_file.name, 'w') as decoy_zip:
+                    decoy_zip.writestr('decoy.txt', 'should never be extracted')
+                outer_zip.write(decoy_zip_file.name, arcname='decoy.zip')
+
+        filenames = elaborate_filenames('lake-man.shp')
+
+        task = ZipDecompressTask()
+        output_files = task.decompress([outer_zip_path], self.workdir, filenames)
+
+        output_names = {os.path.basename(path) for path in output_files}
+        self.assertIn('lake-man.shp', output_names)
+        self.assertIn('lake-man.shx', output_names)
+        self.assertIn('lake-man.dbf', output_names)
+        self.assertIn('lake-man.prj', output_names)
+
+        # Proves decoy.zip was never opened/extracted: neither its contents
+        # nor the zip file itself should appear anywhere in the output.
+        self.assertNotIn('decoy.txt', output_names)
+        self.assertNotIn('decoy.zip', output_names)
+
+    def test_oversized_entry_raises_decompression_error(self):
+        ''' A zip entry declaring an uncompressed size over the configured
+            cap should be refused before extraction, as a zip bomb guard.
+            Uses a real (small) fixture but lowers the cap far below its
+            actual size, rather than crafting an actual multi-GB payload.
+        '''
+        outer_zip_path = self._make_outer_zip()
+
+        task = ZipDecompressTask()
+        with mock.patch.object(ZipDecompressTask, 'MAX_ZIP_ENTRY_BYTES', 10):
+            with self.assertRaises(DecompressionError):
+                task.decompress([outer_zip_path], self.workdir, [])
+
+    def test_deeply_nested_zips_raise_decompression_error(self):
+        ''' A chain of nested zips deeper than the configured limit should
+            be refused, as a zip bomb guard against chained amplification.
+            Each level's nested zip is placed inside its own subfolder
+            (arcname "levelN/nested.zip") so unwrapping one level doesn't
+            land in the same directory as the next - matching how the
+            existing "one zip per directory" width check expects real
+            nested archives to be laid out.
+        '''
+        current_path = self.inner_zip_path
+        for level in range(4):
+            next_path = os.path.join(self.testdir, 'wrap-{}.zip'.format(level))
+            with ZipFile(next_path, 'w') as z:
+                z.write(current_path, arcname='level{}/nested.zip'.format(level))
+            current_path = next_path
+
+        task = ZipDecompressTask()
+        with mock.patch.object(ZipDecompressTask, 'MAX_NESTED_ZIP_DEPTH', 2):
+            with self.assertRaises(DecompressionError):
+                task.decompress([current_path], self.workdir, [])