patch File::Path with CVE-2017-6512 fix in perl 526 #16144
Comments
From @atoomicCreated by @atoomicThis is a bug report for perl from atoomic@cpan.org, ----------------------------------------------------------------- perl 5.26 RC1 still comes with File::Path version 2.12_01 my concern is about CVE-2017-6512 whis is only fixed by release 2.13 I would like to update File::Path to version 2.13 for perl 5.26 After checking the diff between File::Path 2.13..2.14 which is mainly about ____________ Update File-Path to CPAN version 2.14 Perl Info |
From @csjewellOn Mon, 11 Sep 2017 16:54:47 -0700, atoomic@cpan.org wrote:
(points to the perlpolicy documentation) No, unfortunately, that's not the correct answer. There would have to have a patch created that would be a version 2.12_02 that ONLY brings in the change(s) that fix the CVE. New versions of dual-life modules are not imported into maintenance branches, as a rule. If this wasn't already public, I'd also say to report this to perl5-security-report@perl.org - but it's not as if this needs hidden anymore, especially if the CVE in question is already public. |
|
The RT System itself - Status changed from 'new' to 'open' |
From @csjewellOn Mon, 11 Sep 2017 18:03:32 -0700, csjewell wrote:
And here is said patch, as best as I can do it quickly. |
From @csjewell0001-Remove-time-of-check-to-time-of-use-in-File-Path.patchFrom 49a4b1abb2a6735c86432ced8f5a3a9e1e29c9b4 Mon Sep 17 00:00:00 2001
From: Curtis Jewell <perl@csjewell.fastmail.us>
Date: Mon, 11 Sep 2017 19:37:29 -0600
Subject: [PATCH] Remove time-of-check-to-time-of-use in File-Path
This vulnerability was reported as CVE-2017-6512
Applies as much of git diff 2.12_007..2.12_008 (in order to get the
code that fixes the vulnerability) as applies to maint-5.26.
---
cpan/File-Path/lib/File/Path.pm | 41 ++++++++++++++++++++++++++---------------
cpan/File-Path/t/Path.t | 34 +++++++++++++++++++++++-----------
2 files changed, 49 insertions(+), 26 deletions(-)
diff --git a/cpan/File-Path/lib/File/Path.pm b/cpan/File-Path/lib/File/Path.pm
index 034da1e578..1a1ee4ab39 100644
--- a/cpan/File-Path/lib/File/Path.pm
+++ b/cpan/File-Path/lib/File/Path.pm
@@ -18,7 +18,7 @@ BEGIN {
use Exporter ();
use vars qw($VERSION @ISA @EXPORT @EXPORT_OK);
-$VERSION = '2.12_01';
+$VERSION = '2.12_02';
$VERSION = eval $VERSION;
@ISA = qw(Exporter);
@EXPORT = qw(mkpath rmtree);
@@ -354,21 +354,32 @@ sub _rmtree {
# see if we can escalate privileges to get in
# (e.g. funny protection mask such as -w- instead of rwx)
- $perm &= oct '7777';
- my $nperm = $perm | oct '700';
- if (
- !(
- $arg->{safe}
- or $nperm == $perm
- or chmod( $nperm, $root )
- )
- )
- {
- _error( $arg,
- "cannot make child directory read-write-exec", $canon );
- next ROOT_DIR;
+ # This uses fchmod to avoid traversing outside of the proper
+ # location (CVE-2017-6512)
+ my $root_fh;
+ if (open($root_fh, '<', $root)) {
+ my ($fh_dev, $fh_inode) = (stat $root_fh )[0,1];
+ $perm &= oct '7777';
+ my $nperm = $perm | oct '700';
+ local $@;
+ if (
+ !(
+ $data->{safe}
+ or $nperm == $perm
+ or !-d _
+ or $fh_dev ne $ldev
+ or $fh_inode ne $lino
+ or eval { chmod( $nperm, $root_fh ) }
+ )
+ )
+ {
+ _error( $data,
+ "cannot make child directory read-write-exec", $canon );
+ next ROOT_DIR;
+ }
+ close $root_fh;
}
- elsif ( !chdir($root) ) {
+ if ( !chdir($root) ) {
_error( $arg, "cannot chdir to child", $canon );
next ROOT_DIR;
}
diff --git a/cpan/File-Path/t/Path.t b/cpan/File-Path/t/Path.t
index 5644f57a51..cd6d13d392 100644
--- a/cpan/File-Path/t/Path.t
+++ b/cpan/File-Path/t/Path.t
@@ -3,7 +3,7 @@
use strict;
-use Test::More tests => 127;
+use Test::More tests => 126;
use Config;
use Fcntl ':mode';
use lib 't/';
@@ -17,6 +17,13 @@ BEGIN {
my $Is_VMS = $^O eq 'VMS';
+my $fchmod_supported = 0;
+if (open my $fh, curdir()) {
+ my ($perm) = (stat($fh))[2];
+ $perm &= 07777;
+ eval { $fchmod_supported = chmod( $perm, $fh); };
+}
+
# first check for stupid permissions second for full, so we clean up
# behind ourselves
for my $perm (0111,0777) {
@@ -298,16 +305,19 @@ is($created[0], $dir, "created directory (old style 3 mode undef) cross-check");
is(rmtree($dir, 0, undef), 1, "removed directory 3 verbose undef");
-$dir = catdir($tmp_base,'G');
-$dir = VMS::Filespec::unixify($dir) if $Is_VMS;
+SKIP: {
+ skip "fchmod of directories not supported on this platform", 3 unless $fchmod_supported;
+ $dir = catdir($tmp_base,'G');
+ $dir = VMS::Filespec::unixify($dir) if $Is_VMS;
-@created = mkpath($dir, undef, 0200);
+ @created = mkpath($dir, undef, 0200);
-is(scalar(@created), 1, "created write-only dir");
+ is(scalar(@created), 1, "created write-only dir");
-is($created[0], $dir, "created write-only directory cross-check");
+ is($created[0], $dir, "created write-only directory cross-check");
-is(rmtree($dir), 1, "removed write-only dir");
+ is(rmtree($dir), 1, "removed write-only dir");
+}
# borderline new-style heuristics
if (chdir $tmp_base) {
@@ -449,26 +459,28 @@ SKIP: {
}
SKIP : {
- my $skip_count = 19;
+ my $skip_count = 18;
# this test will fail on Windows, as per:
# http://perldoc.perl.org/perlport.html#chmod
skip "Windows chmod test skipped", $skip_count
if $^O eq 'MSWin32';
+ skip "fchmod() on directories is not supported on this platform", $skip_count
+ unless $fchmod_supported;
my $mode;
my $octal_mode;
my @inputs = (
0777, 0700, 0070, 0007,
0333, 0300, 0030, 0003,
0111, 0100, 0010, 0001,
- 0731, 0713, 0317, 0371, 0173, 0137,
- 00 );
+ 0731, 0713, 0317, 0371,
+ 0173, 0137);
my $input;
my $octal_input;
- $dir = catdir($tmp_base, 'chmod_test');
foreach (@inputs) {
$input = $_;
+ $dir = catdir($tmp_base, sprintf("chmod_test%04o", $input));
# We can skip from here because 0 is last in the list.
skip "Mode of 0 means assume user defaults on VMS", 1
if ($input == 0 && $Is_VMS);
--
2.11.0
|
From @atoomicThanks for your answer and providing the patch On Mon, 11 Sep 2017 18:03:32 -0700, csjewell wrote:
|
From @jkeenanOn 09/11/2017 10:10 PM, Curtis Jewell via RT wrote:
FYI: As this page indicates, Thank you very much. |
From @atoomicmmmm good remark, right now 5.26.1-RC1 is using File::Path 2.12_01, which is not listed in the matrix. As the devel versions released are using 3 digits: 2.12_001, 2.12_002, ..., 2.12_008. I think bumping it to 2.12_02 is fine... but tricky I agree On Mon, 11 Sep 2017 22:42:25 -0700, jkeenan@pobox.com wrote:
|
From @jkeenanOn Tue, 12 Sep 2017 15:18:46 GMT, atoomic wrote:
So this ticket is a proposal for certain changes which have already been imported into blead to be imported into 5.24 and 5.26 maintenance branches. I have no objection to that -- but it's not my decision to make. Pumpkings? Thank you very much. |
From @steve-m-hayOn 14 September 2017 at 19:12, James E Keenan via RT
I have a maint-5.26-based branch smoking here: https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/steveh/filepath Assuming it all goes well then I'll add this to the voting file (and |
From @steve-m-hayOn 14 September 2017 at 21:16, Steve Hay <steve.m.hay@googlemail.com> wrote:
Unfortunately, the smoking hasn't gone well: http://perl.develop-help.com/?s=dc73055141932ae0fb773cf52f6980968b37601c (Are there smoke-me logs anywhere else? I don't know where else to Unless anyone else can help then I don't think this will make it into |
From @jkeenanOn Fri, 15 Sep 2017 07:24:07 GMT, shay wrote:
That's not surprising. To address the security vulnerability cited by Nicolas R. in the first post in this ticket, the behavior of File::Path::rmtree() and remove_tree() had to change. Hence, the tests in t/Path.t had to change as well; those changes have been made in File-Path 2.14 and 2.15 on CPAN. Unless you update cpan/File-Path/t/Path.t (and perhaps some of the other test files as well), you will get test failures in the core distribution.
That also is not surprising. The security vulnerability was found in Unix-specific parts of lib/File/Path.pm which are not exercised on Windows or other platforms. (One of the maintenance problems I face with File-Path is that the code and the principal test file are riddled with "skip if $Win32"-ish blocks.) So you have a number of sub-optimal choices: * You can port the correction for the security vulnerability into 5.26.1 by simply bringing all of CPAN's File-Path-2.14 into core (or, at least, lib/File/Path.pm and relevant test files). You can presumably do this for perl-5.24.* as well; all these changes are already in blead. However, there were a lot of other changes in File-Path between 2.12 and 2.14. That would violate the principle articulated earlier in this thread by Curtis Jewell: "There would have to have a patch created that would be a version 2.12_02 that ONLY brings in the change(s) that fix the CVE. New versions of dual-life modules are not imported into maintenance branches, as a rule." * You can leave lib/File/Path.pm as it currently stands in dc73055 and carefully edit t/Path.t (and perhaps other test files) in that commit to skip the tests that now fail because of the change in rmtree()'s behavior. That would further delay the 5.26.1 maintenance release. * You can say that since we have lived with this security vulnerability since Perl 5.4 in 1997, we can live with it until Perl 5.28 is released next spring. That would permit us to ship 5.26.1 without further delay. Since I'm currently the only active maintainer of File-Path on CPAN, I have to prioritize work on the CPAN version and confine myself to more of an advisory role with respect to the version in core. Thank you very much. -- |
From @steve-m-hayOn 15 September 2017 at 14:22, James E Keenan via RT
Then I'm inclined to leave this for 5.26.2 (if possible) or else If anyone needs the File-Path fix earlier than 5.26.2/5.28.0 then they |
From @jkeenanOn Fri, 15 Sep 2017 13:59:12 GMT, shay wrote:
+1
+1 I'd rather not be making that choice for
-- |
From @atoomicI'm fine with this option, the patch is already in blead and would be there for 5.28 or later nicolas On Fri, 15 Sep 2017 07:05:24 -0700, jkeenan wrote:
|
From @jkeenanRequest withdrawn by original poster. Closing. Thank you very much. |
|
@jkeenan - Status changed from 'open' to 'rejected' |
From @xsawyerx[Top-posted] Sorry for the late response. My position is that we could either: * Import the relevant changes as Curtis Jewel provided them, marking it As Steve is, I am also in favor of the latter option, not rushing the Backporting it to 5.24 is a separate topic and it would be good if we On 09/15/2017 03:58 PM, Steve Hay via perl5-porters wrote:
|
From @jkeenanOn Sun, 17 Sep 2017 20:59:52 GMT, xsawyerx@gmail.com wrote:
sawyer, With each of these you have to take into consideration that the changes in File-Path made between CPAN versions 2.12 and 2.14 are not strictly backwards compatible. The cost of addressing a security vulnerability was a change in the behavior of File::Path::rmtree() (or remove_tree()) such that it no longer removes certain trees with exotic permissions, specifically, cases where the user lacks both read and execute permissions. Because the behavior of the function changed, the unit tests had to change as well. Hence, importing these changes into perl-5.26.1 or later or backporting them into perl-5.24.* is not simply a question of cherry-picking one particular commit to lib/File/Path.pm; it's a question of picking up changes to t/Path.t (and perhaps other t/*.t files as well). That's a fair amount of work. As upstream maintainer I put in many hours of work to get the CPAN distribution in better shape. Backporting these changes into perl-5.26 or perl-5.24 will be less work, but it will still be work, and if it's truly desired someone other than me will have to take that on. Thank you very much.
-- |
From @csjewellOn Tue, 19 Sep 2017 09:40:38 -0700, jkeenan wrote:
Yes, it is. I tried my best to update both the module and the tests with my first patch attempt, and it didn't QUITE work, but at least it shows the type of thing that needs doing. I'm willing to continue to try to fix this and to give you a better patch for 26.2 (it'll have to be on my own time, which is minimal this week) as it'll be a fun learning experience for me. |
From @xsawyerxOn 09/19/2017 07:14 PM, Curtis Jewell via RT wrote:
That will be greatly appreciated, Curtis. |
|
@jmdh - Status changed from 'rejected' to 'open' |
From @jkeenanRe-opened per request of Todd Rinaldo at Core Hackathon. |
From @toddrOn Sun, 15 Oct 2017 05:36:08 -0700, jkeenan wrote:
It appears Debian has already done the backport work via: and |
From @xsawyerxOn 10/15/2017 02:40 PM, Todd Rinaldo via RT wrote:
I think it should be backported. |
Migrated from rt.perl.org#132068 (status was 'open')
Searchable as RT132068$
The text was updated successfully, but these errors were encountered: