[Reproducible-commits] [dpkg] 03/05: Dpkg::Source::Patch: Fix patch header parsing to avoid directory traversals

Tue May 3 08:43:18 UTC 2016

This is an automated email from the git hooks/post-receive script.

holger pushed a commit to annotated tag 1.15.11
in repository dpkg.

commit b1719917979ac339a610a27aef8de7f4dcf93887
Author: Guillem Jover <guillem at debian.org>
Date:   Fri May 2 01:41:18 2014 +0200

    Dpkg::Source::Patch: Fix patch header parsing to avoid directory traversals
    
    Cherry picked from commit 5348cbc981a65c3c9b05bb4d13553bda930c2d78.
    
    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 at 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
---
 debian/changelog                              |  7 +++++++
 scripts/Dpkg/Source/Patch.pm                  |  2 +-
 scripts/Makefile.am                           |  4 ++++
 scripts/t/Dpkg_Source_Patch.t                 | 16 +++++++++++++++-
 scripts/t/Dpkg_Source_Patch/index-+++.patch   |  4 ++++
 scripts/t/Dpkg_Source_Patch/index-alone.patch |  3 +++
 scripts/t/Dpkg_Source_Patch/index-inert.patch |  8 ++++++++
 scripts/t/Dpkg_Source_Patch/partial.patch     |  3 +++
 8 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/debian/changelog b/debian/changelog
index 613b988..621875c 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -3,6 +3,13 @@ dpkg (1.15.11) UNRELEASED; urgency=low
   [ Guillem Jover ]
   * Test suite:
     - Add test cases for Dpkg::Source::Patch CVE-2014-0471 and CVE-2014-3127.
+  * 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 at 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
 
  -- Guillem Jover <guillem at debian.org>  Fri, 02 May 2014 00:09:43 +0200
 
diff --git a/scripts/Dpkg/Source/Patch.pm b/scripts/Dpkg/Source/Patch.pm
index db10ac4..12dba83 100644
--- a/scripts/Dpkg/Source/Patch.pm
+++ b/scripts/Dpkg/Source/Patch.pm
@@ -355,7 +355,7 @@ sub analyze {
     while (defined($_) || not eof($self)) {
 	my (%path, %fn);
 	# skip comments leading up to patch (if any)
-	until (/^--- /) {
+	until (/^(?:--- |\+\+\+ |@@ -)/) {
 	    last HUNK if not defined($_ = getline($self));
 	}
 	$diff_count++;
diff --git a/scripts/Makefile.am b/scripts/Makefile.am
index d6bef47..43bd51a 100644
--- a/scripts/Makefile.am
+++ b/scripts/Makefile.am
@@ -218,6 +218,10 @@ test_data = \
 	t/200_Dpkg_Shlibs/objdump.dbd-pg \
 	t/200_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/600_Dpkg_Changelog/countme \
 	t/600_Dpkg_Changelog/fields \
 	t/600_Dpkg_Changelog/misplaced-tz \
diff --git a/scripts/t/Dpkg_Source_Patch.t b/scripts/t/Dpkg_Source_Patch.t
index 0634c8d..2d067df 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 0000000..4ebc23e
--- /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 0000000..904d3d1
--- /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 0000000..5d16c7d
--- /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 0000000..0878858
--- /dev/null
+++ b/scripts/t/Dpkg_Source_Patch/partial.patch
@@ -0,0 +1,3 @@
++++ b/symlink/partial-file
+@@ -0,0 +1,1 @@
++Escaped

-- 
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/reproducible/dpkg.git

