Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date: Thu, 20 Apr 2017 10:41:05 +0200
From: Guillem Jover <guillem@...ian.org>
To: oss-security@...ts.openwall.com
Subject: Directory traversal in dpkg-source via indented patches on non-GNU
 systems

Hi!

Recently, while going through the POSIX standard to check for some
other stuff related to the patch(1) format, I realized that indented
patches are also accepted, which is something the Dpkg::Source::Patch
perl module is not checking, so any of the sanity checks against
directory traveral attacks can be avoided through indenting.

Of course on Debian and other distributions using GNU patch >= 1.7.5,
this is not a concern anymore, as this implementation should be
directory traversal resistant.

But on systems such as the BSDs, with their own patch(1) variant,
this is effective. And while this could (and should in addition be
considered) a problem with those patch implementations, dpkg-source
has always assumed uncooperating underlaying implementations so this
is something it should probably protect against one way or another.

This issue shows up when unpacking a Debian source package for
examination, but then on those non-GNU systems, usage of patch(1)
is unsafe, so I'm not sure how sever this should be considered.

I've got a test case (attached) that fails (the attack is successful) on
at least NetBSD (not tried others), but they share a similar patch(1)
codebase.

And I started adding support for indented patches so thah the checks
would apply, or to just reject them (as a query on codesearch.debian.net)
didn't trigger any instance of such patches in Debian (which would make
them not able to be unpacked if we reject them). But I'm considering the
shorter and more strightforward solution of just requiring GNU patch at
configure time for now. Which is what I'm attaching here as the fix
I'm planning to merge for dpkg 1.18.24.

Given tha above, does this deserve a CVE? At least we have gotten ones
for similar issues in the past.

Thanks,
Guillem

From 08e07ecbef2e1ebdcaaed7a9c90e10eacbc210a8 Mon Sep 17 00:00:00 2001
From: Guillem Jover <guillem@...ian.org>
Date: Sun, 19 Mar 2017 19:40:14 +0100
Subject: [PATCH] Dpkg::Source::Patch: Indented patch test-case

POSIX specifies that a diff hunk can be indented by spaces or tabs
(while the original patch(1) by Larry Wall also accepts 'X'), as long
as the amount of spaces is consisten for all subsequent lines. And as
we are not checking for this condition at all, any such indented hunk
can avoid the sanity checks performed by Dpkg::Source::Patch.

On systems using GNU patch >= 2.7.5, this should, in principle, not be
a problem anymore, as that implementation protects against directory
traversal issue. But on other systems where the patch implementation
does not perform such checks (such as the BSDs) this is an issue, so
check for this in the test-suite.

Those are arguably all security issues in these various patch
implementations, but given that we are performing sanity checks and that
those implementations are currently very lax, it seems prudent to do the
heavy lifting ourselves and also take the possible blame too.

Fixes: CVE-2017-XXXX
---
 scripts/t/Dpkg_Source_Patch.t                   | 6 +++++-
 scripts/t/Dpkg_Source_Patch/indent-header.patch | 9 +++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)
 create mode 100644 scripts/t/Dpkg_Source_Patch/indent-header.patch

diff --git a/scripts/t/Dpkg_Source_Patch.t b/scripts/t/Dpkg_Source_Patch.t
index 8f382f546..938f98ebb 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 => 9;
+use Test::More tests => 10;
 use Test::Dpkg qw(:paths);
 
 use File::Path qw(make_path);
@@ -67,4 +67,8 @@ test_patch_escape('partial', 'symlink', 'partial.patch',
 test_patch_escape('ghost-hunk', 'symlink', 'ghost-hunk.patch',
                   'Patch cannot escape using a disabling hunk');
 
+# This is CVE-2017-XXXX
+test_patch_escape('indent-header', 'symlink', 'indent-header.patch',
+                  'Patch cannot escape indented hunks');
+
 1;
diff --git a/scripts/t/Dpkg_Source_Patch/indent-header.patch b/scripts/t/Dpkg_Source_Patch/indent-header.patch
new file mode 100644
index 000000000..4bef00829
--- /dev/null
+++ b/scripts/t/Dpkg_Source_Patch/indent-header.patch
@@ -0,0 +1,9 @@
+  --- /dev/null
+  +++ b/symlink/index-file
+  @@ -0,0 +1,1 @@
+  +Escaped
+
+--- /dev/null
++++ b/dummy-file
+@@ -0,0 +1,1 @@
++Dummy to make the code see a valid hunk
-- 
2.12.2.762.g0e3151a226


From e1dcc2f0c02a31d12d2455e4ce457ff426b6c6fe Mon Sep 17 00:00:00 2001
From: Guillem Jover <guillem@...ian.org>
Date: Tue, 28 Mar 2017 22:44:36 +0200
Subject: [PATCH] build: Detect the required GNU patch

This makes sure the perl module is using a directory traversal resistant
patch implementation, currently that's only GNU patch.
---
 configure.ac                 |  1 +
 m4/dpkg-progs.m4             | 21 +++++++++++++++++++++
 scripts/Dpkg.pm              | 13 ++++++++++++-
 scripts/Dpkg/Source/Patch.pm |  9 +++++----
 scripts/Makefile.am          |  2 ++
 5 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/configure.ac b/configure.ac
index d5075509c..87c1e9d09 100644
--- a/configure.ac
+++ b/configure.ac
@@ -66,6 +66,7 @@ AC_PROG_CC
 DPKG_C_C99
 AC_PROG_CXX
 DPKG_CXX_CXX11
+DPKG_PROG_PATCH
 AC_CHECK_PROGS([DOXYGEN], [doxygen])
 AC_CHECK_PROG([HAVE_DOT], [dot], [YES], [NO])
 DPKG_PROG_PO4A
diff --git a/m4/dpkg-progs.m4 b/m4/dpkg-progs.m4
index f4a0b5b3a..3a0a8478f 100644
--- a/m4/dpkg-progs.m4
+++ b/m4/dpkg-progs.m4
@@ -70,3 +70,24 @@ AC_DEFUN([DPKG_DEB_PROG_TAR], [
   AC_SUBST([TAR], [$ac_cv_path_TAR])
   AC_DEFINE_UNQUOTED([TAR], ["$TAR"], [GNU tar program])
 ])# DPKG_DEB_PROG_TAR
+
+# DPKG_PROG_PATCH
+# ---------------
+# Specify GNU patch program name to use by dpkg-source. On GNU systems this
+# is usually simply patch, on BSD systems this is usually gpatch.
+# Even though most invocations would work with other patch implementations,
+# currently only GNU patch is directory traversal resitant.
+AC_DEFUN([DPKG_PROG_PATCH], [
+  AC_ARG_VAR([PATCH], [GNU patch program])
+  AC_CACHE_CHECK([for GNU patch], [ac_cv_path_PATCH], [
+    AC_PATH_PROGS_FEATURE_CHECK([PATCH], [gpatch patch], [
+      AS_IF([$ac_path_PATCH --version 2>/dev/null | grep -q '^GNU patch'], [
+        ac_cv_path_PATCH=$ac_path_PATCH ac_path_PATCH_found=:
+      ])
+    ], [
+      AC_MSG_ERROR([cannot find a GNU patch program])
+    ])
+  ])
+  AC_SUBST([PATCH], [$ac_cv_path_PATCH])
+  AC_DEFINE_UNQUOTED([PATCH], ["$PATCH"], [GNU patch program])
+])# DPKG_PROG_PATCH
diff --git a/scripts/Dpkg.pm b/scripts/Dpkg.pm
index 1b9624c4d..3deb933f9 100644
--- a/scripts/Dpkg.pm
+++ b/scripts/Dpkg.pm
@@ -29,12 +29,13 @@ this system installation.
 use strict;
 use warnings;
 
-our $VERSION = '1.02';
+our $VERSION = '1.03';
 our @EXPORT_OK = qw(
     $PROGNAME
     $PROGVERSION
     $PROGMAKE
     $PROGTAR
+    $PROGPATCH
     $CONFDIR
     $ADMINDIR
     $LIBDIR
@@ -70,6 +71,11 @@ Contains the name of the system GNU make program.
 
 Contains the name of the system GNU tar program.
 
+=item $Dpkg::PROGPATCH
+
+Contains the name of the system GNU patch program (or another implementation
+that is directory traversal resistant).
+
 =item $Dpkg::CONFDIR
 
 Contains the path to the dpkg system configuration directory.
@@ -96,6 +102,7 @@ our ($PROGNAME) = $0 =~ m{(?:.*/)?([^/]*)};
 our $PROGVERSION = '1.18.x';
 our $PROGMAKE = $ENV{DPKG_PROGMAKE} // 'make';
 our $PROGTAR = $ENV{DPKG_PROGTAR} // 'tar';
+our $PROGPATCH = $ENV{DPKG_PROGPATCH} // 'patch';
 
 our $CONFDIR = '/etc/dpkg';
 our $ADMINDIR = '/var/lib/dpkg';
@@ -114,6 +121,10 @@ our $pkgdatadir = $DATADIR;
 
 =head1 CHANGES
 
+=head2 Version 1.03 (dpkg 1.18.24)
+
+New variable: $PROGPATCH.
+
 =head2 Version 1.02 (dpkg 1.18.11)
 
 New variable: $PROGTAR, $PROGMAKE.
diff --git a/scripts/Dpkg/Source/Patch.pm b/scripts/Dpkg/Source/Patch.pm
index ee5e114f8..22e9d213d 100644
--- a/scripts/Dpkg/Source/Patch.pm
+++ b/scripts/Dpkg/Source/Patch.pm
@@ -30,6 +30,7 @@ use File::Compare;
 use Fcntl ':mode';
 use Time::HiRes qw(stat);
 
+use Dpkg;
 use Dpkg::Gettext;
 use Dpkg::ErrorHandling;
 use Dpkg::IPC;
@@ -582,7 +583,7 @@ sub apply {
     $self->ensure_open('r');
     my ($stdout, $stderr) = ('', '');
     spawn(
-	exec => [ 'patch', @{$opts{options}} ],
+	exec => [ $Dpkg::PROGPATCH, @{$opts{options}} ],
 	chdir => $destdir,
 	env => { LC_ALL => 'C', LANG => 'C', PATCH_GET => '0' },
 	delete_env => [ 'POSIXLY_CORRECT' ], # ensure expected patch behaviour
@@ -595,7 +596,7 @@ sub apply {
     if ($?) {
 	print { *STDOUT } $stdout;
 	print { *STDERR } $stderr;
-	subprocerr('LC_ALL=C patch ' . join(' ', @{$opts{options}}) .
+	subprocerr("LC_ALL=C $Dpkg::PROGPATCH " . join(' ', @{$opts{options}}) .
 	           ' < ' . $self->get_filename());
     }
     $self->close();
@@ -632,7 +633,7 @@ sub check_apply {
     # Apply the patch
     $self->ensure_open('r');
     my $patch_pid = spawn(
-	exec => [ 'patch', @{$opts{options}} ],
+	exec => [ $Dpkg::PROGPATCH, @{$opts{options}} ],
 	chdir => $destdir,
 	env => { LC_ALL => 'C', LANG => 'C', PATCH_GET => '0' },
 	delete_env => [ 'POSIXLY_CORRECT' ], # ensure expected patch behaviour
@@ -642,7 +643,7 @@ sub check_apply {
     );
     wait_child($patch_pid, nocheck => 1);
     my $exit = WEXITSTATUS($?);
-    subprocerr('patch --dry-run') unless WIFEXITED($?);
+    subprocerr("$Dpkg::PROGPATCH --dry-run") unless WIFEXITED($?);
     $self->close();
     return ($exit == 0);
 }
diff --git a/scripts/Makefile.am b/scripts/Makefile.am
index 5583fa960..54a83c044 100644
--- a/scripts/Makefile.am
+++ b/scripts/Makefile.am
@@ -132,6 +132,7 @@ do_perl_subst = $(AM_V_GEN) sed \
 	-e "s:our \$$DATADIR = .*;:our \$$DATADIR = '$(pkgdatadir)';:" \
 	-e "s:our \$$PROGMAKE = .*;:our \$$PROGMAKE = '$(MAKE)';:" \
 	-e "s:our \$$PROGTAR = .*;:our \$$PROGTAR = '$(TAR)';:" \
+	-e "s:our \$$PROGPATCH = .*;:our \$$PROGPATCH = '$(PATCH)';:" \
 	-e "s:our \$$PROGVERSION = .*;:our \$$PROGVERSION = '$(PACKAGE_VERSION)';:"
 
 do_shell_subst = $(AM_V_GEN) sed \
@@ -193,6 +194,7 @@ coverage-clean:
 
 TEST_ENV_VARS = \
 	DPKG_PROGTAR=$(TAR) \
+	DPKG_PROGPATCH=$(PATCH) \
 	DPKG_PROGMAKE=$(MAKE) \
 	DPKG_DATADIR=$(top_srcdir)/data \
 	DPKG_ORIGINS_DIR=$(srcdir)/t/origins
-- 
2.12.2.762.g0e3151a226


Powered by blists - more mailing lists

Your e-mail address:

Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.

Powered by Openwall GNU/*/Linux - Powered by OpenVZ