Dpkg::Source::Patch: Fix patch header parsing to avoid directory traversals

The code parsing the patches was not taking into account that patches
w/ partial or no pathname headers are still valid patches, and that
they can specify the pathname in the Index: pseudo-header or in a
single «+++ » pathname header, which allows doing directory traversal
when unpacking source packages.

The first vector is due to how the Index: pseudo-header is handled by
patch. Its value gets used (on non-POSIX mode) only when both «+++ »
and «--- » pathname headers do not provide a pathname, by either having
an empty pathname or by the header being completely absent. The minimal
fix for this is to just consider that we've parsed the header when we
see a hunk header marker «@@ -». This is CVE-2014-3865 and #749183.

The other vector is due to patches with only a «+++ » pathname header,
which get skipped by the parser as it only checks for «--- » pathname
header lines. The minimal fix for this is to also check for «+++ » when
parsing the patch header. This is CVE-2014-3864 and #746498.

The first issue is a superset of the second, and its fix is sufficient
and covers and fixes too the second vector, as the «@@ -» marker is
mandatory for a patch to be valid.

An unspecified directory traversal vulnerability was initially reported
in #746498 by Javier Serrano Polo <javier@jasp.net>, and while no
information had been provided, I independently found #749183 and what
was supposed to be #746498, which was later on published.

Fixes: CVE-2014-3864, CVE-2014-3865
Closes: #746498, #749183

8 files changed, 45 insertions, 2 deletions

diff --git a/debian/changelog b/debian/changelog
index ba0c19f3d..80990a7c5 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -66,6 +66,13 @@ dpkg (1.17.10) UNRELEASED; urgency=low
     Thanks to Hleb Valoshka <375gnu@gmail.com>.
   * Add support for DragonFlyBSD to start-stop-daemon. Closes: #734452
     Based on a patch by Hleb Valoshka <375gnu@gmail.com>.
+  * Correctly parse patch headers in Dpkg::Source::Patch, to avoid directory
+    traversal attempts from hostile source packages when unpacking them.
+    Reported by Javier Serrano Polo <javier@jasp.net> as an unspecified
+    directory traversal; meanwhile also independently found by me both
+    #749183 and what was supposed to be #746498, which was later on published
+    and ended up being just a subset of the other non-reported issue.
+    Fixes CVE-2014-3864 and CVE-2014-3865. Closes: #746498, #749183
 
   [ Updated programs translations ]
   * Catalan (Guillem Jover).
diff --git a/scripts/Dpkg/Source/Patch.pm b/scripts/Dpkg/Source/Patch.pm
index c45d5e768..edb2db140 100644
--- a/scripts/Dpkg/Source/Patch.pm
+++ b/scripts/Dpkg/Source/Patch.pm
@@ -405,7 +405,7 @@ sub analyze {
 	my (%path, %fn);
 	# skip comments leading up to patch (if any)
 	while (1) {
-	    if (/^--- /) {
+	    if (/^(?:--- |\+\+\+ |@@ -)/) {
 		last;
 	    } else {
 		$patch_header .= "$_\n";
diff --git a/scripts/Makefile.am b/scripts/Makefile.am
index 61c736336..8830106ac 100644
--- a/scripts/Makefile.am
+++ b/scripts/Makefile.am
@@ -257,6 +257,10 @@ test_data = \
 	t/Dpkg_Shlibs/objdump.dbd-pg \
 	t/Dpkg_Shlibs/objdump.ls \
 	t/Dpkg_Source_Patch/c-style.patch \
+	t/Dpkg_Source_Patch/index-+++.patch \
+	t/Dpkg_Source_Patch/index-alone.patch \
+	t/Dpkg_Source_Patch/index-inert.patch \
+	t/Dpkg_Source_Patch/partial.patch \
 	t/Dpkg_Changelog/countme \
 	t/Dpkg_Changelog/fields \
 	t/Dpkg_Changelog/misplaced-tz \
diff --git a/scripts/t/Dpkg_Source_Patch.t b/scripts/t/Dpkg_Source_Patch.t
index 0634c8d9c..2d067df95 100644
--- a/scripts/t/Dpkg_Source_Patch.t
+++ b/scripts/t/Dpkg_Source_Patch.t
@@ -16,7 +16,7 @@
 use strict;
 use warnings;
 
-use Test::More tests => 3;
+use Test::More tests => 8;
 
 use File::Path qw(make_path);
 
@@ -50,4 +50,18 @@ test_patch_escape('c-style-parsed', "\tmp", 'c-style.patch',
 test_patch_escape('c-style-unknown', '\\tmp', 'c-style.patch',
                   'Patch cannot escape using unknown c-style encoded filename');
 
+# This is CVE-2014-3865
+test_patch_escape('index-alone', 'symlink', 'index-alone.patch',
+                  'Patch cannot escape using Index: w/o ---/+++ header');
+test_patch_escape('index-+++', 'symlink', 'index-+++.patch',
+                  'Patch cannot escape using Index: w/ only +++ header');
+test_patch_escape('index-inert', 'symlink', 'index-inert.patch',
+                  'Patch should not fail to apply using an inert Index:');
+ok(-e "$tmpdir/index-inert-tree/inert-file",
+   'Patch with inert Index: applies correrctly');
+
+# This is CVE-2014-3864
+test_patch_escape('partial', 'symlink', 'partial.patch',
+                  'Patch cannot escape using partial +++ header');
+
 1;
diff --git a/scripts/t/Dpkg_Source_Patch/index-+++.patch b/scripts/t/Dpkg_Source_Patch/index-+++.patch
new file mode 100644
index 000000000..4ebc23e18
--- /dev/null
+++ b/scripts/t/Dpkg_Source_Patch/index-+++.patch
@@ -0,0 +1,4 @@
+Index: index/symlink/index-file
++++ 
+@@ -0,0 +1,1 @@
++Escaped
diff --git a/scripts/t/Dpkg_Source_Patch/index-alone.patch b/scripts/t/Dpkg_Source_Patch/index-alone.patch
new file mode 100644
index 000000000..904d3d17c
--- /dev/null
+++ b/scripts/t/Dpkg_Source_Patch/index-alone.patch
@@ -0,0 +1,3 @@
+Index: index/symlink/index-file
+@@ -0,0 +1,1 @@
++Escaped
diff --git a/scripts/t/Dpkg_Source_Patch/index-inert.patch b/scripts/t/Dpkg_Source_Patch/index-inert.patch
new file mode 100644
index 000000000..5d16c7d22
--- /dev/null
+++ b/scripts/t/Dpkg_Source_Patch/index-inert.patch
@@ -0,0 +1,8 @@
+This could be a comment about the patch itself, where we could use an
+Index: a/ header with /../ inside that could be interpreted as a	
+malicious pseudo-header, so we should not validate it,
+
+--- /dev/null
++++ b/inert-file
+@@ -0,0 +1,1 @@
++Inert
diff --git a/scripts/t/Dpkg_Source_Patch/partial.patch b/scripts/t/Dpkg_Source_Patch/partial.patch
new file mode 100644
index 000000000..087885877
--- /dev/null
+++ b/scripts/t/Dpkg_Source_Patch/partial.patch
@@ -0,0 +1,3 @@
++++ b/symlink/partial-file
+@@ -0,0 +1,1 @@
++Escaped