[CVE-2015-8607] XS File::Spec::canonpath loses taint

[p5pRT]

Migrated from rt.perl.org#126862 (status was 'resolved')

Searchable as RT126862$

[p5pRT]

From @xdg

It looks like the XS canonpath shipped with Perl 5.20 (corresponding to
PathTools-3.47) doesn't preserve taint.

I.e.
  Scalar​::Util​::tainted( File​::Spec->canonpath($tainted_value) ) # false

A patch to the PathTools tests to demonstrate this is here​:
https://gist.github.com/xdg/cccc68c668dcaad71b0a

This is a regression from PathTools-3.40. I have spot checked back to 3.12
(shipping with Perl 5.8.8) and canonpath preserves taint there as well.

As most File​::Spec functions wash data through canonpath, this means that
almost any File​::Spec operation on tainted data has untainted results from
perl 5.20 onwards and any earlier Perl that installed PathTools-3.47 or
later from CPAN.

I believe this warrants a CVE report and will need backported fixes for
5.20 and 5.22.

David

--
David Golden <xdg@​xdg.me> Twitter/IRC/Github​: @​xdg

[p5pRT]

From @iabyn

On Wed, Dec 09, 2015 at 12​:06​:10PM -0800, David Golden wrote​:

# New Ticket Created by David Golden
# Please include the string​: [perl #126862]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=126862 >

It looks like the XS canonpath shipped with Perl 5.20 (corresponding to
PathTools-3.47) doesn't preserve taint.

I.e.
Scalar​::Util​::tainted( File​::Spec->canonpath($tainted_value) ) # false

A patch to the PathTools tests to demonstrate this is here​:
https://gist.github.com/xdg/cccc68c668dcaad71b0a

This is a regression from PathTools-3.40. I have spot checked back to 3.12
(shipping with Perl 5.8.8) and canonpath preserves taint there as well.

As most File​::Spec functions wash data through canonpath, this means that
almost any File​::Spec operation on tainted data has untainted results from
perl 5.20 onwards and any earlier Perl that installed PathTools-3.47 or
later from CPAN.

I believe this warrants a CVE report and will need backported fixes for
5.20 and 5.22.

I think its worth backporting to 5.20 and 5.22, but I don't think it needs
a CVE. I regard tainting as being a useful adjunct to security, but not a
primary security mechanism. I.e. your code should be written to be
be secure against malicious data, and tainting is one tool to help you
1) during development, spot and fix such flaws;
2) during production, help catch some things you missed earlier.
If your primary security mechanism is "enable taint", then you've got big
problems already.

Or to put it another way, I see taint as a bit like using clang's address
sanitizer. Its useful, but if it were found that clang failed to spot
a certain type of buffer overrun , I wouldn't regard that as a security
issue requiring all code to be immediately recompiled using a fixed clang,
and then be pushed out to customers as a "hot fix".

--
"You're so sadly neglected, and often ignored.
A poor second to Belgium, When going abroad."
  -- Monty Python, "Finland"

[p5pRT]

The RT System itself - Status changed from 'new' to 'open'

[p5pRT]

From @xdg

On Thu, Dec 10, 2015 at 7​:26 AM, Dave Mitchell via RT <
perl5-security-report@​perl.org> wrote​:

I think its worth backporting to 5.20 and 5.22, but I don't think it needs
a CVE.

Taint is the headline security mechanism in Perl. (See perlsec.) Breaking
that guarantee to users is worthy of a CVE. There is also prior precedent​:
http​://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2011-1487

Even if the risk is small, it needs to be communicated to downstream
vendors.

David

--
David Golden <xdg@​xdg.me> Twitter/IRC/Github​: @​xdg

[p5pRT]

From @rjbs

* David Golden <xdg@​xdg.me> [2015-12-10T08​:31​:18]

Even if the risk is small, it needs to be communicated to downstream
vendors.

I think a CVE is appropriate here, much as I loathe dealing with this process.

Do we have a proposed fix? My guess is that it won't be a huge deal to fix it,
although I haven't even looked. I'm guessing.

--
rjbs

[p5pRT]

From @tonycoz

On Mon Dec 14 14​:57​:07 2015, perl.security@​rjbs.manxome.org wrote​:

* David Golden <xdg@​xdg.me> [2015-12-10T08​:31​:18]

Even if the risk is small, it needs to be communicated to downstream
vendors.

I think a CVE is appropriate here, much as I loathe dealing with this
process.

Do we have a proposed fix? My guess is that it won't be a huge deal
to fix it,
although I haven't even looked. I'm guessing.

Yes, it's simple, attached.

Tony

[p5pRT]

From @tonycoz

0001-perl-126862-ensure-File-Spec-canonpath-preserves-tai.patch

From b6307f728a4f842a54ea96959e386c7daa92ece1 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 15 Dec 2015 10:56:54 +1100
Subject: [perl #126862] ensure File::Spec::canonpath() preserves taint

Previously the unix specific XS implementation of canonpath() would
return an untainted path when supplied a tainted path.

For the empty string case, newSVpvs() already sets taint as needed on
its result.
---
 dist/PathTools/Cwd.xs    |  1 +
 dist/PathTools/t/taint.t | 19 ++++++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/dist/PathTools/Cwd.xs b/dist/PathTools/Cwd.xs
index 9d4dcf0..3d018dc 100644
--- a/dist/PathTools/Cwd.xs
+++ b/dist/PathTools/Cwd.xs
@@ -535,6 +535,7 @@ THX_unix_canonpath(pTHX_ SV *path)
     *o = 0;
     SvPOK_on(retval);
     SvCUR_set(retval, o - SvPVX(retval));
+    SvTAINT(retval);
     return retval;
 }
 
diff --git a/dist/PathTools/t/taint.t b/dist/PathTools/t/taint.t
index 309b3e5..48f8c5b 100644
--- a/dist/PathTools/t/taint.t
+++ b/dist/PathTools/t/taint.t
@@ -12,7 +12,7 @@ use Test::More;
 BEGIN {
     plan(
         ${^TAINT}
-        ? (tests => 17)
+        ? (tests => 21)
         : (skip_all => "A perl without taint support")
     );
 }
@@ -34,3 +34,20 @@ foreach my $func (@Functions) {
 
 # Previous versions of Cwd tainted $^O
 is !tainted($^O), 1, "\$^O should not be tainted";
+
+{
+    # [perl #126862] canonpath() loses taint
+    my $tainted = substr($ENV{PATH}, 0, 0);
+    # yes, getcwd()'s result should be tainted, and is tested above
+    # but be sure
+    ok tainted(File::Spec->canonpath($tainted . Cwd::getcwd)),
+        "canonpath() keeps taint on non-empty string";
+    ok tainted(File::Spec->canonpath($tainted)),
+        "canonpath() keeps taint on empty string";
+
+    (Cwd::getcwd() =~ /^(.*)/);
+    my $untainted = $1;
+    ok !tainted($untainted), "make sure our untainted value is untainted";
+    ok !tainted(File::Spec->canonpath($untainted)),
+        "canonpath() doesn't add taint to untainted string";
+}
-- 
2.1.4

[p5pRT]

From @tonycoz

0002-bump-the-PathTools-version.patch

From ca65c886d2b1c0c8557b9ba72cc443a510d25ec5 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 15 Dec 2015 11:08:32 +1100
Subject: bump the PathTools version

which is duplicated in many files
---
 dist/PathTools/Cwd.pm                     | 2 +-
 dist/PathTools/lib/File/Spec.pm           | 2 +-
 dist/PathTools/lib/File/Spec/AmigaOS.pm   | 2 +-
 dist/PathTools/lib/File/Spec/Cygwin.pm    | 2 +-
 dist/PathTools/lib/File/Spec/Epoc.pm      | 2 +-
 dist/PathTools/lib/File/Spec/Functions.pm | 2 +-
 dist/PathTools/lib/File/Spec/Mac.pm       | 2 +-
 dist/PathTools/lib/File/Spec/OS2.pm       | 2 +-
 dist/PathTools/lib/File/Spec/Unix.pm      | 2 +-
 dist/PathTools/lib/File/Spec/VMS.pm       | 2 +-
 dist/PathTools/lib/File/Spec/Win32.pm     | 2 +-
 11 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/dist/PathTools/Cwd.pm b/dist/PathTools/Cwd.pm
index 64618f9..0d35806 100644
--- a/dist/PathTools/Cwd.pm
+++ b/dist/PathTools/Cwd.pm
@@ -3,7 +3,7 @@ use strict;
 use Exporter;
 use vars qw(@ISA @EXPORT @EXPORT_OK $VERSION);
 
-$VERSION = '3.60';
+$VERSION = '3.61';
 my $xs_version = $VERSION;
 $VERSION =~ tr/_//d;
 
diff --git a/dist/PathTools/lib/File/Spec.pm b/dist/PathTools/lib/File/Spec.pm
index f416908..bcbec2d 100644
--- a/dist/PathTools/lib/File/Spec.pm
+++ b/dist/PathTools/lib/File/Spec.pm
@@ -3,7 +3,7 @@ package File::Spec;
 use strict;
 use vars qw(@ISA $VERSION);
 
-$VERSION = '3.60';
+$VERSION = '3.61';
 $VERSION =~ tr/_//d;
 
 my %module = (MacOS   => 'Mac',
diff --git a/dist/PathTools/lib/File/Spec/AmigaOS.pm b/dist/PathTools/lib/File/Spec/AmigaOS.pm
index f979f2f..7d02ad5 100644
--- a/dist/PathTools/lib/File/Spec/AmigaOS.pm
+++ b/dist/PathTools/lib/File/Spec/AmigaOS.pm
@@ -4,7 +4,7 @@ use strict;
 use vars qw(@ISA $VERSION);
 require File::Spec::Unix;
 
-$VERSION = '3.60';
+$VERSION = '3.61';
 $VERSION =~ tr/_//d;
 
 @ISA = qw(File::Spec::Unix);
diff --git a/dist/PathTools/lib/File/Spec/Cygwin.pm b/dist/PathTools/lib/File/Spec/Cygwin.pm
index 558a742..9dd176b 100644
--- a/dist/PathTools/lib/File/Spec/Cygwin.pm
+++ b/dist/PathTools/lib/File/Spec/Cygwin.pm
@@ -4,7 +4,7 @@ use strict;
 use vars qw(@ISA $VERSION);
 require File::Spec::Unix;
 
-$VERSION = '3.60';
+$VERSION = '3.61';
 $VERSION =~ tr/_//d;
 
 @ISA = qw(File::Spec::Unix);
diff --git a/dist/PathTools/lib/File/Spec/Epoc.pm b/dist/PathTools/lib/File/Spec/Epoc.pm
index afca637..0c640ed 100644
--- a/dist/PathTools/lib/File/Spec/Epoc.pm
+++ b/dist/PathTools/lib/File/Spec/Epoc.pm
@@ -3,7 +3,7 @@ package File::Spec::Epoc;
 use strict;
 use vars qw($VERSION @ISA);
 
-$VERSION = '3.60';
+$VERSION = '3.61';
 $VERSION =~ tr/_//d;
 
 require File::Spec::Unix;
diff --git a/dist/PathTools/lib/File/Spec/Functions.pm b/dist/PathTools/lib/File/Spec/Functions.pm
index 276ddcf..9badcf2 100644
--- a/dist/PathTools/lib/File/Spec/Functions.pm
+++ b/dist/PathTools/lib/File/Spec/Functions.pm
@@ -5,7 +5,7 @@ use strict;
 
 use vars qw(@ISA @EXPORT @EXPORT_OK %EXPORT_TAGS $VERSION);
 
-$VERSION = '3.60';
+$VERSION = '3.61';
 $VERSION =~ tr/_//d;
 
 require Exporter;
diff --git a/dist/PathTools/lib/File/Spec/Mac.pm b/dist/PathTools/lib/File/Spec/Mac.pm
index 4da700c..06e73c8 100644
--- a/dist/PathTools/lib/File/Spec/Mac.pm
+++ b/dist/PathTools/lib/File/Spec/Mac.pm
@@ -4,7 +4,7 @@ use strict;
 use vars qw(@ISA $VERSION);
 require File::Spec::Unix;
 
-$VERSION = '3.60';
+$VERSION = '3.61';
 $VERSION =~ tr/_//d;
 
 @ISA = qw(File::Spec::Unix);
diff --git a/dist/PathTools/lib/File/Spec/OS2.pm b/dist/PathTools/lib/File/Spec/OS2.pm
index fad1198..d6d2f48 100644
--- a/dist/PathTools/lib/File/Spec/OS2.pm
+++ b/dist/PathTools/lib/File/Spec/OS2.pm
@@ -4,7 +4,7 @@ use strict;
 use vars qw(@ISA $VERSION);
 require File::Spec::Unix;
 
-$VERSION = '3.60';
+$VERSION = '3.61';
 $VERSION =~ tr/_//d;
 
 @ISA = qw(File::Spec::Unix);
diff --git a/dist/PathTools/lib/File/Spec/Unix.pm b/dist/PathTools/lib/File/Spec/Unix.pm
index 94e4351..5f92112 100644
--- a/dist/PathTools/lib/File/Spec/Unix.pm
+++ b/dist/PathTools/lib/File/Spec/Unix.pm
@@ -3,7 +3,7 @@ package File::Spec::Unix;
 use strict;
 use vars qw($VERSION);
 
-$VERSION = '3.60';
+$VERSION = '3.61';
 my $xs_version = $VERSION;
 $VERSION =~ tr/_//d;
 
diff --git a/dist/PathTools/lib/File/Spec/VMS.pm b/dist/PathTools/lib/File/Spec/VMS.pm
index b050bf2..ecb239d 100644
--- a/dist/PathTools/lib/File/Spec/VMS.pm
+++ b/dist/PathTools/lib/File/Spec/VMS.pm
@@ -4,7 +4,7 @@ use strict;
 use vars qw(@ISA $VERSION);
 require File::Spec::Unix;
 
-$VERSION = '3.60';
+$VERSION = '3.61';
 $VERSION =~ tr/_//d;
 
 @ISA = qw(File::Spec::Unix);
diff --git a/dist/PathTools/lib/File/Spec/Win32.pm b/dist/PathTools/lib/File/Spec/Win32.pm
index 8839800..447cbf5 100644
--- a/dist/PathTools/lib/File/Spec/Win32.pm
+++ b/dist/PathTools/lib/File/Spec/Win32.pm
@@ -5,7 +5,7 @@ use strict;
 use vars qw(@ISA $VERSION);
 require File::Spec::Unix;
 
-$VERSION = '3.60';
+$VERSION = '3.61';
 $VERSION =~ tr/_//d;
 
 @ISA = qw(File::Spec::Unix);
-- 
2.1.4

[p5pRT]

From @rjbs

* Tony Cook via RT <perl5-security-report@​perl.org> [2015-12-14T19​:09​:48]

Yes, it's simple, attached.

Thanks, I'll begin the CVE and vendor notification process tomorrow, with a
four week lead time.

--
rjbs

[p5pRT]

From @rjbs

* Tony Cook via RT <perl5-security-report@​perl.org> [2015-12-14T19​:09​:48]

Yes, it's simple, attached.

I have assigned this CVE-2015-8607.

I will email vendors tomorrow at lunchtime, unless there are objections between
now and then. I will request an embargo until Monday, January 11 (three
weeks).

  Beginning in PathTools 3.47 and/or perl 5.20.0, the File​::Spec​::canonpath()
  routine returned untained strings even if passed tainted input. This defect
  undermines the guarantee of taint propagation, which is sometimes used to
  ensure that unvalidated user input does not reach sensitive code.

  This defect was found and reported by David Golden of MongoDB, and the
  attached patch was provided by Tony Cook. Please embargo this report and
  patch until Monday, January 11, when it will be made available via the perl 5
  source code repository.

--
rjbs

[p5pRT]

From @rjbs

This ticket has been moved to the public queue, the embargo has expired, and patches have been pushed to the main repository.

I have made a new release of PathTools, available any minute now. As PathTools can be updated outside of perl, I am marking this ticket resolved, rather than pending release.

--
rjbs

[p5pRT]

From [Unknown Contact. See original ticket]

This ticket has been moved to the public queue, the embargo has expired, and patches have been pushed to the main repository.

I have made a new release of PathTools, available any minute now. As PathTools can be updated outside of perl, I am marking this ticket resolved, rather than pending release.

--
rjbs

[p5pRT]

@rjbs - Status changed from 'open' to 'resolved'