Skip Menu |
Report information
Id: 126862
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: xdg [at] xdg.me
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: (no value)
Type: (no value)
Perl Version: (no value)
Fixed In: (no value)



Date: Wed, 9 Dec 2015 15:04:46 -0500
Subject: XS File::Spec::canonpath loses taint
From: David Golden <xdg [...] xdg.me>
To: "perl5-security-report [...] perl.org" <perl5-security-report [...] perl.org>
Download (untitled) / with headers
text/plain 843b
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
Subject: Re: [perl #126862] XS File::Spec::canonpath loses taint
To: perl5-security-report [...] perl.org
Date: Thu, 10 Dec 2015 12:25:35 +0000
From: Dave Mitchell <davem [...] iabyn.com>
On Wed, Dec 09, 2015 at 12:06:10PM -0800, David Golden wrote: Show quoted text
> # 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.perl.org/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"
Subject: Re: [perl #126862] XS File::Spec::canonpath loses taint
Date: Thu, 10 Dec 2015 08:31:18 -0500
To: "perl5-security-report [...] perl.org" <perl5-security-report [...] perl.org>
From: David Golden <xdg [...] xdg.me>
Download (untitled) / with headers
text/plain 541b
On Thu, Dec 10, 2015 at 7:26 AM, Dave Mitchell via RT <perl5-security-report@perl.org> wrote:
Show quoted text
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
Date: Mon, 14 Dec 2015 17:56:19 -0500
Subject: Re: [perl #126862] XS File::Spec::canonpath loses taint
From: Ricardo Signes <perl.security [...] rjbs.manxome.org>
To: David Golden <xdg [...] xdg.me>
CC: "perl5-security-report [...] perl.org" <perl5-security-report [...] perl.org>
Download (untitled) / with headers
text/plain 350b
* David Golden <xdg@xdg.me> [2015-12-10T08:31:18] Show quoted text
> 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
Download signature.asc
application/pgp-signature 473b

Message body not shown because it is not plain text.

RT-Send-CC: rt-deliver-to-perl5-security-report [...] rt.perl.org
Download (untitled) / with headers
text/plain 462b
On Mon Dec 14 14:57:07 2015, perl.security@rjbs.manxome.org wrote: Show quoted text
> * 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
Subject: 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
Subject: 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
To: Tony Cook via RT <perl5-security-report [...] perl.org>
From: Ricardo Signes <perl.security [...] rjbs.manxome.org>
CC: ;, rt-deliver-to-perl5-security-report [...] rt.perl.org
Date: Tue, 15 Dec 2015 23:21:12 -0500
Subject: Re: [perl #126862] XS File::Spec::canonpath loses taint
Download (untitled) / with headers
text/plain 212b
* Tony Cook via RT <perl5-security-report@perl.org> [2015-12-14T19:09:48] Show quoted text
> Yes, it's simple, attached.
Thanks, I'll begin the CVE and vendor notification process tomorrow, with a four week lead time. -- rjbs
Download signature.asc
application/pgp-signature 473b

Message body not shown because it is not plain text.

Date: Thu, 17 Dec 2015 19:58:35 -0500
Subject: Re: [perl #126862] XS File::Spec::canonpath loses taint
To: Tony Cook via RT <perl5-security-report [...] perl.org>
From: Ricardo Signes <perl.security [...] rjbs.manxome.org>
CC: ;, rt-deliver-to-perl5-security-report [...] rt.perl.org
Download (untitled) / with headers
text/plain 871b
* Tony Cook via RT <perl5-security-report@perl.org> [2015-12-14T19:09:48] Show quoted text
> 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
Download signature.asc
application/pgp-signature 473b

Message body not shown because it is not plain text.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 312b
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


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org