Bug #125523 for Archive-Tar: CVE-2018-12015 directory traversal vulnerability

Thu Jun 07 17:31:29 2018 dom [...] cpan.org - Ticket created

Subject:

CVE-2018-12015 directory traversal vulnerability

As reported to the Debian BTS[1] Archive-Tar has a symlink-related directory traversal vulnerability: ----- By default, the Archive::Tar module doesn't allow extracting files outside the current working directory. However, you can bypass this secure extraction mode easily by putting a symlink and a regular file with the same name into the tarball. I've attached proof of concept tarball, which makes Archive::Tar create /tmp/moo, regardless of what the current working directory is: $ tar -tvvf traversal.tar.gz lrwxrwxrwx root/root 0 2018-06-05 18:55 moo -> /tmp/moo -rw-r--r-- root/root 4 2018-06-05 18:55 moo $ pwd /home/jwilk $ ls /tmp/moo ls: cannot access '/tmp/moo': No such file or directory $ perl -MArchive::Tar -e 'Archive::Tar->extract_archive("traversal.tar.gz")' $ ls /tmp/moo /tmp/moo ----- The attachment is here: https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=900834;filename=traversal.tar.gz;msg=3 [1] <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=900834>

Fri Jun 08 02:24:35 2018 ppisar [...] redhat.com - Correspondence added

Dne Čt 07.čen.2018 17:31:29, DOM napsal(a): Show quoted text

> $ tar -tvvf traversal.tar.gz > lrwxrwxrwx root/root 0 2018-06-05 18:55 moo -> /tmp/moo > -rw-r--r-- root/root 4 2018-06-05 18:55 moo >

Tar archive can contain multiple file entries with the same name. GNU tar info page reads: [...] files are extracted from an archive in the order in which they were archived. Thus, when the archive is extracted, a file archived later in time will replace a file of the same name which was archived earlier [...] When you extract the archive, the older version of the file will be extracted first, and then replaced by the newer version when it is extracted. It looks like first Archive::Tar creates the symlink (first entry) and then writes the regular file content into the symlink (second entry). GNU tar indeed replaces the symlink with a regular file.

Fri Jun 08 02:24:36 2018 The RT System itself - Status changed from 'new' to 'open'

Fri Jun 08 04:02:52 2018 ppisar [...] redhat.com - Correspondence added

Dne Čt 07.čen.2018 17:31:29, DOM napsal(a): Show quoted text

> As reported to the Debian BTS[1] Archive-Tar has a symlink-related > directory traversal vulnerability: >

Attached patch simply removes every existing (non-directory) file that's going to be extracted. Even in non-secure mode. Is it fine, or too strict, or even dangerous?

Subject:

0001-Remove-existing-files-before-overwriting-them.patch

From d23726d0d3d30ce451c6eadda41a2df5446ead27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= <ppisar@redhat.com> Date: Fri, 8 Jun 2018 09:53:16 +0200 Subject: [PATCH] Remove existing files before overwriting them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Archive should extract only the latest same-named entry. Extracted regular file should not be writtent into existing block device (or any other one). https://rt.cpan.org/Ticket/Display.html?id=125523 Signed-off-by: Petr PÃsaÅ <ppisar@redhat.com> --- lib/Archive/Tar.pm | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/Archive/Tar.pm b/lib/Archive/Tar.pm index 9ac2763..adfb433 100644 --- a/lib/Archive/Tar.pm +++ b/lib/Archive/Tar.pm @@ -845,6 +845,20 @@ sub _extract_file { return; } + ### If a file system already contains a block device with the same name as + ### the being extracted regular file, we would write the file's content + ### to the block device. So remove the existing file (block device) now. + ### If an archive contains multiple same-named entries, the last one + ### should replace the previous ones. So remove the old file now. + ### If the old entry is a symlink to a file outside of the CWD, the new + ### entry would create a file there. This is CVE-2018-12015 + ### <https://rt.cpan.org/Ticket/Display.html?id=125523>. + if (-l $full || -e _) { + if (!unlink $full) { + $self->_error( qq[Could not remove old file '$full': $!] ); + return; + } + } if( length $entry->type && $entry->is_file ) { my $fh = IO::File->new; $fh->open( $full, '>' ) or ( -- 2.14.4

Fri Jun 08 10:22:48 2018 dom [...] earth.li - Correspondence added

Subject:	Re: [rt.cpan.org #125523] CVE-2018-12015 directory traversal vulnerability
Date:	Fri, 8 Jun 2018 14:51:30 +0100
To:	Petr Pisar via RT <bug-Archive-Tar [...] rt.cpan.org>
From:	Dominic Hargreaves <dom [...] earth.li>

On Fri, Jun 08, 2018 at 04:02:58AM -0400, Petr Pisar via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=125523 > > > Dne Čt 07.čen.2018 17:31:29, DOM napsal(a):

> > As reported to the Debian BTS[1] Archive-Tar has a symlink-related > > directory traversal vulnerability: > >

> Attached patch simply removes every existing (non-directory) file that's going to be extracted. Even in non-secure mode. > > Is it fine, or too strict, or even dangerous?

This looks good to me, but I'd like a second opinion from someone more familiar with the code before applying to Debian. Thanks! Dominic.

Fri Jun 08 11:31:22 2018 chris [...] bingosnet.co.uk - Correspondence added

Subject:	Re: [rt.cpan.org #125523] CVE-2018-12015 directory traversal vulnerability
Date:	Fri, 8 Jun 2018 16:29:47 +0100
To:	"dom [...] earth.li via RT" <bug-Archive-Tar [...] rt.cpan.org>
From:	Chris 'BinGOs' Williams <chris [...] bingosnet.co.uk>

On Fri, Jun 08, 2018 at 10:22:48AM -0400, dom@earth.li via RT wrote: Show quoted text

> > This looks good to me, but I'd like a second opinion from someone more > familiar with the code before applying to Debian. > > Thanks! > Dominic.

I have applied it as https://github.com/jib/archive-tar-new/commit/ae65651eab053fc6dc4590dbb863a268215c1fc5 and released 2.28 to CPAN: https://metacpan.org/release/BINGOS/Archive-Tar-2.28 Cheers, -- Chris Williams aka BinGOs PGP ID 0x4658671F http://www.gumbynet.org.uk ==========================

Download signature.asc
application/pgp-signature 181b

Message body not shown because it is not plain text.

Sat Jun 09 19:22:32 2018 ether [...] cpan.org - Correspondence added

On 2018-06-08 08:31:22, chris@bingosnet.co.uk wrote: Show quoted text

> On Fri, Jun 08, 2018 at 10:22:48AM -0400, dom@earth.li via RT wrote:

> > > > This looks good to me, but I'd like a second opinion from someone > > more > > familiar with the code before applying to Debian. > > > > Thanks! > > Dominic.

> > I have applied it as https://github.com/jib/archive-tar- > new/commit/ae65651eab053fc6dc4590dbb863a268215c1fc5 > > and released 2.28 to CPAN: > https://metacpan.org/release/BINGOS/Archive-Tar-2.28 > > Cheers,

Can this ticket be closed now?

Sat Jun 09 19:29:52 2018 dom [...] earth.li - Correspondence added

Subject:	Re: [rt.cpan.org #125523] CVE-2018-12015 directory traversal vulnerability
Date:	Sun, 10 Jun 2018 00:29:43 +0100
To:	"chris [...] bingosnet.co.uk via RT" <bug-Archive-Tar [...] rt.cpan.org>
From:	Dominic Hargreaves <dom [...] earth.li>

On Fri, Jun 08, 2018 at 11:31:23AM -0400, chris@bingosnet.co.uk via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=125523 > > > On Fri, Jun 08, 2018 at 10:22:48AM -0400, dom@earth.li via RT wrote:

> > > > This looks good to me, but I'd like a second opinion from someone more > > familiar with the code before applying to Debian. > > > > Thanks! > > Dominic.

> > I have applied it as https://github.com/jib/archive-tar-new/commit/ae65651eab053fc6dc4590dbb863a268215c1fc5 > > and released 2.28 to CPAN: https://metacpan.org/release/BINGOS/Archive-Tar-2.28

Thanks! We're rolling this out in Debian now. Best, Dominic.

Wed Jun 13 23:17:50 2018 andrew [...] cpan.org - Correspondence added

On Fri Jun 08 04:02:52 2018, ppisar wrote: Show quoted text

> Dne Čt 07.čen.2018 17:31:29, DOM napsal(a):

> > As reported to the Debian BTS[1] Archive-Tar has a symlink-related > > directory traversal vulnerability: > >

> Attached patch simply removes every existing (non-directory) file > that's going to be extracted. Even in non-secure mode. > > Is it fine, or too strict, or even dangerous? >

Doesn't this just shorten the race and yet still have the same problem? If the attacker does something like: perl -e 'symlink "attack_vector" "to.extract" while 1' Then I would think there's a reasonable chance that the symlink will get created between the unlink and the open while Archive::Tar is busy checking the length, if it is a file, and instantiating an IO::File object. I don't think the unlink is a negative, in that it avoids a case where you might run out of space to extract two copies of the file, but shouldn't this use a tempfile with a reasonably random name to avoid the race and then atomically rename it to the correct name?

Thu Jun 14 22:58:45 2018 andrew [...] cpan.org - Correspondence added

On Wed Jun 13 23:17:50 2018, ANDREW wrote: Show quoted text

> On Fri Jun 08 04:02:52 2018, ppisar wrote:

> > Dne Čt 07.čen.2018 17:31:29, DOM napsal(a):

> > > As reported to the Debian BTS[1] Archive-Tar has a symlink-related > > > directory traversal vulnerability: > > >

> > Attached patch simply removes every existing (non-directory) file > > that's going to be extracted. Even in non-secure mode. > > > > Is it fine, or too strict, or even dangerous? > >

Talking to ewilhelm at pdx.pm, the solution is probably to use the O_EXCL flag for the open. This is not guaranteed to work on NFS, so you would also need a lockfile for that. No idea what guarantees you're looking for with this. https://man.openbsd.org/open#O_EXCL

Fri Jun 15 18:57:54 2018 andrew [...] cpan.org - Correspondence added

RT-Send-CC:

dom [...] earth.li, chris [...] bingosnet.co.uk

On Thu Jun 14 22:58:45 2018, ANDREW wrote: Show quoted text

> On Wed Jun 13 23:17:50 2018, ANDREW wrote:

> > On Fri Jun 08 04:02:52 2018, ppisar wrote:

> > > Dne Čt 07.čen.2018 17:31:29, DOM napsal(a):

> > > > As reported to the Debian BTS[1] Archive-Tar has a symlink- > > > > related > > > > directory traversal vulnerability: > > > >

> > > Attached patch simply removes every existing (non-directory) file > > > that's going to be extracted. Even in non-secure mode. > > > > > > Is it fine, or too strict, or even dangerous? > > >

> > Talking to ewilhelm at pdx.pm, the solution is probably to use the > O_EXCL flag for the open. This is not guaranteed to work on NFS, so > you would also need a lockfile for that. No idea what guarantees > you're looking for with this. > > https://man.openbsd.org/open#O_EXCL

Here is a patch that adds the O_EXCL flag to the open call. Removing the unlink patch with just this one shows that it works, as the test suite extracts over files.

Subject:

0001-Extract-files-with-O_EXCL.patch

From 0766551dd0298b5216699abf3f805c59b92c7840 Mon Sep 17 00:00:00 2001 From: Andrew Hewus Fresh <andrew@afresh1.com> Date: Fri, 15 Jun 2018 15:47:49 -0700 Subject: [PATCH] Extract files with O_EXCL This will cause the open to die if the file exists, avoiding a race condition and security risk. Relates to commit ae65651eab053fc6dc4590dbb863a268215c1fc5 and https://rt.cpan.org/Ticket/Display.html?id=125523 --- lib/Archive/Tar.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Archive/Tar.pm b/lib/Archive/Tar.pm index 5950b3e..a3aa577 100644 --- a/lib/Archive/Tar.pm +++ b/lib/Archive/Tar.pm @@ -861,7 +861,7 @@ sub _extract_file { } if( length $entry->type && $entry->is_file ) { my $fh = IO::File->new; - $fh->open( $full, '>' ) or ( + $fh->open( $full, O_WRONLY|O_CREAT|O_EXCL ) or ( $self->_error( qq[Could not open file '$full': $!] ), return ); -- 2.16.4

Fri Jun 15 20:15:57 2018 dom [...] earth.li - Correspondence added

Subject:	Re: [rt.cpan.org #125523] CVE-2018-12015 directory traversal vulnerability
Date:	Sat, 16 Jun 2018 01:15:28 +0100
To:	Andrew Fresh via RT <bug-Archive-Tar [...] rt.cpan.org>
From:	Dominic Hargreaves <dom [...] earth.li>

On Wed, Jun 13, 2018 at 11:17:51PM -0400, Andrew Fresh via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=125523 > > > On Fri Jun 08 04:02:52 2018, ppisar wrote:

> > Dne Čt 07.čen.2018 17:31:29, DOM napsal(a):

> > > As reported to the Debian BTS[1] Archive-Tar has a symlink-related > > > directory traversal vulnerability: > > >

> > Attached patch simply removes every existing (non-directory) file > > that's going to be extracted. Even in non-secure mode. > > > > Is it fine, or too strict, or even dangerous? > >

> > > Doesn't this just shorten the race and yet still have the same problem? > > If the attacker does something like: > perl -e 'symlink "attack_vector" "to.extract" while 1'

The attack referred to in this ticket is not to do with people extracting into an attacker controlled directory, but people extracting an attacker-controlled archive. So there is no change for the attacker to run this code. Show quoted text

> Then I would think there's a reasonable chance that the symlink will get created between the unlink and the open while Archive::Tar is busy checking the length, if it is a file, and instantiating an IO::File object. > > I don't think the unlink is a negative, in that it avoids a case where you might run out of space to extract two copies of the file, but shouldn't this use a tempfile with a reasonably random name to avoid the race and then atomically rename it to the correct name?

This isn't to say that the attack you describe isn't valid, but it would be much, much harder to pull off, since it would require the victim to be unpacking an archive - and this isn't a bug in Archive-Tar so much as a user behaviour problem, right? Or am I misunderstanding completely? Dominic.

Fri Jun 15 21:53:34 2018 andrew [...] cpan.org - Correspondence added

RT-Send-CC:

dom [...] earth.li

On Fri Jun 15 20:15:57 2018, dom@earth.li wrote: Show quoted text

> On Wed, Jun 13, 2018 at 11:17:51PM -0400, Andrew Fresh via RT wrote: > This isn't to say that the attack you describe isn't valid, but it > would > be much, much harder to pull off, since it would require the victim to > be > unpacking an archive - and this isn't a bug in Archive-Tar so much as > a > user behaviour problem, right? > > Or am I misunderstanding completely? > > Dominic.

I think you are correct, I must have I misunderstood the problem when first reading the ticket. The existing patch should solve the problem of a malicious tar file extracting outside of the current working directory with a symlink. Thank you for explaining again.