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

Proposed by Colin Watson on 2015-10-15
Status: Merged
Merged at revision: 587
Proposed branch: lp:~cjwatson/click/audit-missing-dot-slash
Merge into: lp:click/devel
Diff against target: 125 lines (+60/-1)
3 files modified
click/install.py (+10/-0)
click/tests/test_install.py (+48/-1)
debian/changelog (+2/-0)
To merge this branch: bzr merge lp:~cjwatson/click/audit-missing-dot-slash
Reviewer Review Type Date Requested Status
Michael Vogt 2015-10-15 Approve on 2015-10-15
Review via email: mp+274554@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Michael Vogt (mvo) wrote :

Looks good, thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'click/install.py'
2--- click/install.py 2015-09-17 19:20:07 +0000
3+++ click/install.py 2015-10-15 12:56:07 +0000
4@@ -276,6 +276,16 @@
5 'with system architecture "%s"' %
6 (architecture, dpkg_architecture))
7
8+ # This isn't ideally quick, since it has to decompress the data
9+ # part of the package, but dpkg's path filtering code assumes
10+ # that all paths start with "./" so we must check it before
11+ # passing the package to dpkg.
12+ for data_name in package.data:
13+ if data_name != "." and not data_name.startswith("./"):
14+ raise ClickInstallerAuditError(
15+ 'File name "%s" in package does not start with "./"' %
16+ data_name)
17+
18 if slow:
19 temp_dir = tempfile.mkdtemp(prefix="click")
20 try:
21
22=== modified file 'click/tests/test_install.py'
23--- click/tests/test_install.py 2014-12-03 12:42:21 +0000
24+++ click/tests/test_install.py 2015-10-15 12:56:07 +0000
25@@ -23,19 +23,24 @@
26 ]
27
28
29-from contextlib import contextmanager
30+from contextlib import (
31+ closing,
32+ contextmanager,
33+ )
34 import hashlib
35 import json
36 import os
37 import shutil
38 import stat
39 import subprocess
40+import tarfile
41
42 from unittest import skipUnless
43
44 from debian.deb822 import Deb822
45 from gi.repository import Click
46
47+from click.arfile import ArFile
48 from click.build import ClickBuilder
49 from click.install import (
50 ClickInstaller,
51@@ -50,6 +55,7 @@
52 TestCase,
53 touch,
54 )
55+from click.versions import spec_version
56
57
58 @contextmanager
59@@ -104,6 +110,7 @@
60 script.write(contents)
61 Click.ensuredir(data_dir)
62 for name, path in data_files.items():
63+ Click.ensuredir(os.path.dirname(os.path.join(data_dir, name)))
64 if path is None:
65 touch(os.path.join(data_dir, name))
66 elif os.path.isdir(path):
67@@ -320,6 +327,46 @@
68 ])
69 self.assertEqual(("test-package", "1.0"), installer.audit(path))
70
71+ def test_audit_missing_dot_slash(self):
72+ # Manually construct a package with data paths that do not start
73+ # with "./", which could be used to bypass path filtering.
74+ with self.run_in_subprocess(
75+ "click_get_frameworks_dir") as (enter, preloads):
76+ enter()
77+ path = self.make_fake_package(
78+ control_fields={"Click-Version": "0.2"},
79+ manifest={
80+ "name": "test-package",
81+ "version": "1.0",
82+ "framework": "ubuntu-sdk-13.10",
83+ },
84+ control_scripts={"preinst": static_preinst},
85+ data_files={".click/tmp.ci/manifest": None})
86+ # Repack without the leading "./".
87+ data_dir = os.path.join(self.temp_dir, "fake-package")
88+ data_tar_path = os.path.join(self.temp_dir, "data.tar.gz")
89+ control_tar_path = os.path.join(self.temp_dir, "control.tar.gz")
90+ package_path = '%s.click' % data_dir
91+ with closing(tarfile.TarFile.open(
92+ name=data_tar_path, mode="w:gz", format=tarfile.GNU_FORMAT
93+ )) as data_tar:
94+ data_tar.add(
95+ os.path.join(data_dir, ".click"), arcname=".click")
96+ with ArFile(name=package_path, mode="w") as package:
97+ package.add_magic()
98+ package.add_data("debian-binary", b"2.0\n")
99+ package.add_data(
100+ "_click-binary", ("%s\n" % spec_version).encode("UTF-8"))
101+ package.add_file("control.tar.gz", control_tar_path)
102+ package.add_file("data.tar.gz", data_tar_path)
103+ self._setup_frameworks(preloads, frameworks=["ubuntu-sdk-13.10"])
104+ with mock_quiet_subprocess_call():
105+ installer = ClickInstaller(self.db)
106+ self.assertRaisesRegex(
107+ ClickInstallerAuditError,
108+ 'File name ".click" in package does not start with "./"',
109+ installer.audit, path)
110+
111 def test_audit_broken_md5sums(self):
112 with self.run_in_subprocess(
113 "click_get_frameworks_dir") as (enter, preloads):
114
115=== modified file 'debian/changelog'
116--- debian/changelog 2015-10-15 11:47:35 +0000
117+++ debian/changelog 2015-10-15 12:56:07 +0000
118@@ -2,6 +2,8 @@
119
120 * Fix spurious test_sync_without_user_db test failure.
121 * Fix test failures under Python 2.
122+ * Forbid installing packages with data tarball members whose names do not
123+ start with "./" (LP: #1506467).
124
125 -- Colin Watson <cjwatson@ubuntu.com> Thu, 15 Oct 2015 12:46:54 +0100
126

Subscribers

People subscribed via source and target branches