Merge lp:~cjwatson/click/audit-missing-dot-slash into lp:click/devel

[Colin Watson]

Forbid installing packages with data tarball members whose names do not start with "./" (LP: #1506467).

Forbid installing packages with data tarball members whose names do not start with "./". These slipped past the dpkg path filtering because that filtering wasn't built for security purposes and assumes that paths start with "./", starting its comparisons from the second character. To fix this, we audit the file names before invoking dpkg.

[Michael Vogt (mvo)]

Looks good, thanks!

review: Approve

=== modified file 'click/install.py'
--- click/install.py	2015-09-17 19:20:07 +0000
+++ click/install.py	2015-10-15 12:56:07 +0000
@@ -276,6 +276,16 @@
                             'with system architecture "%s"' %
                             (architecture, dpkg_architecture))
 
+            # This isn't ideally quick, since it has to decompress the data
+            # part of the package, but dpkg's path filtering code assumes
+            # that all paths start with "./" so we must check it before
+            # passing the package to dpkg.
+            for data_name in package.data:
+                if data_name != "." and not data_name.startswith("./"):
+                    raise ClickInstallerAuditError(
+                        'File name "%s" in package does not start with "./"' %
+                        data_name)
+
             if slow:
                 temp_dir = tempfile.mkdtemp(prefix="click")
                 try:

=== modified file 'click/tests/test_install.py'
--- click/tests/test_install.py	2014-12-03 12:42:21 +0000
+++ click/tests/test_install.py	2015-10-15 12:56:07 +0000
@@ -23,19 +23,24 @@
     ]
 
 
-from contextlib import contextmanager
+from contextlib import (
+    closing,
+    contextmanager,
+    )
 import hashlib
 import json
 import os
 import shutil
 import stat
 import subprocess
+import tarfile
 
 from unittest import skipUnless
 
 from debian.deb822 import Deb822
 from gi.repository import Click
 
+from click.arfile import ArFile
 from click.build import ClickBuilder
 from click.install import (
     ClickInstaller,
@@ -50,6 +55,7 @@
     TestCase,
     touch,
 )
+from click.versions import spec_version
 
 
 @contextmanager
@@ -104,6 +110,7 @@
                 script.write(contents)
         Click.ensuredir(data_dir)
         for name, path in data_files.items():
+            Click.ensuredir(os.path.dirname(os.path.join(data_dir, name)))
             if path is None:
                 touch(os.path.join(data_dir, name))
             elif os.path.isdir(path):
@@ -320,6 +327,46 @@
                     ])
             self.assertEqual(("test-package", "1.0"), installer.audit(path))
 
+    def test_audit_missing_dot_slash(self):
+        # Manually construct a package with data paths that do not start
+        # with "./", which could be used to bypass path filtering.
+        with self.run_in_subprocess(
+                "click_get_frameworks_dir") as (enter, preloads):
+            enter()
+            path = self.make_fake_package(
+                control_fields={"Click-Version": "0.2"},
+                manifest={
+                    "name": "test-package",
+                    "version": "1.0",
+                    "framework": "ubuntu-sdk-13.10",
+                },
+                control_scripts={"preinst": static_preinst},
+                data_files={".click/tmp.ci/manifest": None})
+            # Repack without the leading "./".
+            data_dir = os.path.join(self.temp_dir, "fake-package")
+            data_tar_path = os.path.join(self.temp_dir, "data.tar.gz")
+            control_tar_path = os.path.join(self.temp_dir, "control.tar.gz")
+            package_path = '%s.click' % data_dir
+            with closing(tarfile.TarFile.open(
+                    name=data_tar_path, mode="w:gz", format=tarfile.GNU_FORMAT
+                    )) as data_tar:
+                data_tar.add(
+                    os.path.join(data_dir, ".click"), arcname=".click")
+            with ArFile(name=package_path, mode="w") as package:
+                package.add_magic()
+                package.add_data("debian-binary", b"2.0\n")
+                package.add_data(
+                    "_click-binary", ("%s\n" % spec_version).encode("UTF-8"))
+                package.add_file("control.tar.gz", control_tar_path)
+                package.add_file("data.tar.gz", data_tar_path)
+            self._setup_frameworks(preloads, frameworks=["ubuntu-sdk-13.10"])
+            with mock_quiet_subprocess_call():
+                installer = ClickInstaller(self.db)
+                self.assertRaisesRegex(
+                    ClickInstallerAuditError,
+                    'File name ".click" in package does not start with "./"',
+                    installer.audit, path)
+
     def test_audit_broken_md5sums(self):
         with self.run_in_subprocess(
                 "click_get_frameworks_dir") as (enter, preloads):

=== modified file 'debian/changelog'
--- debian/changelog	2015-10-15 11:47:35 +0000
+++ debian/changelog	2015-10-15 12:56:07 +0000
@@ -2,6 +2,8 @@
 
   * Fix spurious test_sync_without_user_db test failure.
   * Fix test failures under Python 2.
+  * Forbid installing packages with data tarball members whose names do not
+    start with "./" (LP: #1506467).
 
  -- Colin Watson <[e-mail]>  Thu, 15 Oct 2015 12:46:54 +0100