Bug #19577 for Archive-Tar: extract_file() drops volume part of extraction path

Mon May 29 13:26:35 2006 JDB [...] cpan.org - Ticket created

Subject:

extract_file() drops volume part of extraction path

extract_file() uses File::Spec->splitpath() to split the extraction path into volume, directory and filename, but it ignores the volume part when it determines that the path is an absolute local path. This has no effect on Unix, which doesn't use volumes, but e.g. on Windows this creates a bug whenever the extraction path is absolute, but not on the current drive. Here is a patch: --- Archive/Tar.pm.~1~ Tue May 23 22:20:01 2006 +++ Archive/Tar.pm Tue May 23 22:20:01 2006 @@ -534,7 +534,7 @@ my $dir; ### is $name an absolute path? ### if( File::Spec->file_name_is_absolute( $dirs ) ) { - $dir = $dirs; + $dir = File::Spec->catpath( $vol, $dirs, "" ); ### it's a relative path ### } else { End of Patch.

Mon May 29 13:32:23 2006 JDB [...] cpan.org - Status changed from 'new' to 'open'

Mon May 29 13:43:27 2006 jand [...] ActiveState.com - Correspondence added

Subject:	RE: [rt.cpan.org #19577] AutoReply: extract_file() drops volume part of extraction path
Date:	Mon, 29 May 2006 10:42:49 -0700
To:	<bug-Archive-Tar [...] rt.cpan.org>, <JDB [...] cpan.org>
From:	"Jan Dubois" <jand [...] activestate.com>

Show quoted text

> This has no effect on Unix, which doesn't use volumes, but e.g. on > Windows this creates a bug whenever the extraction path is absolute, > but not on the current drive. > > Here is a patch:

[...] Even with the patch the code doesn't work correctly for relative paths on other than the current volume (e.g. "h:relative/path/file"). I'm not sure if this concept of having a current working directory for each volume exists anywhere besides Windows. On Windows this could be fixed by calling Cwd::getdcwd(), but it requires Cwd version 2.18 or later. Cheers, -Jan

Mon May 29 13:48:39 2006 jand [...] ActiveState.com - Correspondence added

Subject:	RE: [rt.cpan.org #19577] AutoReply: extract_file() drops volume part of extraction path
Date:	Mon, 29 May 2006 10:48:04 -0700
To:	<bug-Archive-Tar [...] rt.cpan.org>, <JDB [...] cpan.org>
From:	"Jan Dubois" <jand [...] activestate.com>

BTW, this behavior is broken in 1.29, not fixed in it. I must have used the wrong listbox when selecting it, but RT doesn't allow me to correct this. :( Cheers, -Jan

Tue Jul 04 09:01:09 2006 kane [...] cpan.org - Fixed in 1.29 deleted

Tue Jul 04 09:01:10 2006 kane [...] cpan.org - Broken in 1.29 added

Wed Jul 05 08:41:48 2006 kane [...] cpan.org - Correspondence added

Hi Jan, On Mon May 29 13:43:27 2006, jand@ActiveState.com wrote: Show quoted text

> > This has no effect on Unix, which doesn't use volumes, but e.g. on > > Windows this creates a bug whenever the extraction path is absolute, > > but not on the current drive. > > > > Here is a patch:

> [...] > > Even with the patch the code doesn't work correctly for relative paths > on other than the current volume (e.g. "h:relative/path/file"). I'm > not sure if this concept of having a current working directory for > each volume exists anywhere besides Windows.

I'm not sure what hte 'best' approach is to this. If a tarfile is packed by using the full path "h:\a\b", should the extraction then happen to h:\a\b or to [current volume]\a\b? Same for the relative paths of course. If not going for [current volume], there's a good chance that extraction will fail, as not everyone has a h: nor do they intend to extract there. Also, if you would have a test case that demonstrates this behaviour (or even better, an addition to our .t file), that'd make verifying the desired behaviour a lot easier. Show quoted text

> On Windows this could be fixed by calling Cwd::getdcwd(), but it requires > Cwd version 2.18 or later.

That shouldn't be too much of a problem i think: $ corelist -a Cwd [...] 5.008004 2.17 5.008005 2.19 So most people should have access to it. And if we go for the [current volume] solution, getdcwd shouldn't even be needed. Awaiting your thoughts, -- Jos P.S. sorry for the late reply, but i've first secured access to a win32 machine so i can test any code/ideas you send along on a real environment, rather than just mentally :)

Wed Jul 05 08:41:50 2006 kane [...] cpan.org - Status changed from 'open' to 'stalled'

Sun Aug 31 04:06:18 2008 GAAS [...] cpan.org - Correspondence added

The reason ActivePerl has this patch is that ppm will extract files with code that basically does: $tar->extract_file("foo.pm", "$ENV{TEMPDIR}/dir/foo.pm"); This code fails if $ENV{TEMPDIR} does not point to a directory on the current volume, since the volume part was chopped off the extraction path but never put back again. Attached is a refresh of the patch relative to Archive-Tar-1.39_03 as it currently appears in ActivePerl. There is now also an additonal $vol test in the is_abs() test; I presume so you can call $tar->extract_file("foo.pm", "h:foo.pm") and have it actually copy the file to the "h:" drive.

diff --git a/lib/Archive/Tar.pm b/lib/Archive/Tar.pm index 8f425a4..862e1de 100644 --- a/lib/Archive/Tar.pm +++ b/lib/Archive/Tar.pm @@ -610,7 +610,7 @@ sub _extract_file { my $dir; ### is $name an absolute path? ### - if( File::Spec->file_name_is_absolute( $dirs ) ) { + if( $vol || File::Spec->file_name_is_absolute( $dirs ) ) { ### absolute names are not allowed to be in tarballs under ### strict mode, so only allow it if a user tells us to do it @@ -623,7 +623,7 @@ sub _extract_file { } ### user asked us to, it's fine. - $dir = $dirs; + $dir = File::Spec->catpath( $vol, $dirs, "" ); ### it's a relative path ### } else {

Sun Aug 31 04:06:24 2008 The RT System itself - Status changed from 'stalled' to 'open'

Sat Sep 06 10:02:17 2008 kane [...] cpan.org - Correspondence added

On Sun Aug 31 04:06:18 2008, GAAS wrote: Show quoted text

> The reason ActivePerl has this patch is that ppm will extract files with > code that basically does: > > $tar->extract_file("foo.pm", "$ENV{TEMPDIR}/dir/foo.pm"); > > This code fails if $ENV{TEMPDIR} does not point to a directory on the > current volume, since the volume part was chopped off the extraction > path but never put back again. > > Attached is a refresh of the patch relative to Archive-Tar-1.39_03 as it > currently appears in ActivePerl. There is now also an additonal $vol > test in the is_abs() test; I presume so you can call > $tar->extract_file("foo.pm", "h:foo.pm") and have it actually copy the > file to the "h:" drive.

Thanks for the explenation Gisle. However, this goes against the 'rules' of tar, and is a potential vulnerability. The problem is quite well explained here: http://rt.cpan.org/Ticket/Display.html?id=30380 However, if you could modify your patch so that it takes into account the setting of $INSECURE_EXTRACT_MODE (and use that in your own calling code), then there shouldn't be a problem applying it. Would that be acceptable? Cheers,

Mon Sep 08 00:45:08 2008 jand [...] ActiveState.com - Correspondence added

Subject:	RE: [rt.cpan.org #19577] extract_file() drops volume part of extraction path
Date:	Sun, 7 Sep 2008 21:44:21 -0700
To:	<bug-Archive-Tar [...] rt.cpan.org>
From:	"Jan Dubois" <jand [...] activestate.com>

On Sat, 06 Sep 2008, Jos Boumans via RT wrote: Show quoted text

> > <URL: http://rt.cpan.org/Ticket/Display.html?id=19577 > > > On Sun Aug 31 04:06:18 2008, GAAS wrote:

> > The reason ActivePerl has this patch is that ppm will extract files with > > code that basically does: > > > > $tar->extract_file("foo.pm", "$ENV{TEMPDIR}/dir/foo.pm"); > > > > This code fails if $ENV{TEMPDIR} does not point to a directory on the > > current volume, since the volume part was chopped off the extraction > > path but never put back again. > > > > Attached is a refresh of the patch relative to Archive-Tar-1.39_03 as it > > currently appears in ActivePerl. There is now also an additonal $vol > > test in the is_abs() test; I presume so you can call > > $tar->extract_file("foo.pm", "h:foo.pm") and have it actually copy the > > file to the "h:" drive.

> > Thanks for the explenation Gisle. > > However, this goes against the 'rules' of tar, and is a potential > vulnerability. The problem is quite well explained here: > > http://rt.cpan.org/Ticket/Display.html?id=30380

I think that is a totally different issue. Those "rules of tar" try to prevent paths *stored inside the tarball* to extract to a directory not under the current directory. This bug is about the behavior of the 2-argument extract_file() method. It already allows you to specify an extraction path that is not under the current directory. In fact, you can specify any path you want to. The problem is just that you are dropping the volume part of this extraction path. Show quoted text

> However, if you could modify your patch so that it takes into account > the setting of $INSECURE_EXTRACT_MODE (and use that in your own > calling code), then there shouldn't be a problem applying it. > > Would that be acceptable?

I think this is exactly backwards. Right now you may end up extracting to a different directory than the one specified as the second argument in extract_file(). Fixing this is not less secure, it is more secure. Cheers, -Jan

Mon Sep 08 08:09:12 2008 kane [...] cpan.org - Correspondence added

Show quoted text

> I think that is a totally different issue. Those "rules of tar" try to > prevent paths *stored inside the tarball* to extract to a directory not > under the current directory. > > This bug is about the behavior of the 2-argument extract_file() method. > It already allows you to specify an extraction path that is not under > the current directory. In fact, you can specify any path you want to. > The problem is just that you are dropping the volume part of this extraction > path.

Apparently I've misunderstood the nature of the issue; i assumed it was to let tarballs specify volume names and have those be respected when extracting. Thanks for explaining, the patch is now in Archive::Tar.

Mon Sep 08 08:09:15 2008 kane [...] cpan.org - Status changed from 'open' to 'resolved'