Re: Directory traversal in `ar`

Re: Directory traversal in `ar`

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]

On Sun, Dec 28, 2014 at 02:46:15AM +0300, Alexander Cherepanov wrote:
> There is a directory traversal in `ar`:
> 
> # printf '!<arch>\n%-48s%-10s`\n//file/\n%-48s%-10s`\n' // 8 /1 0 > test.a
> # ar -xv test.a
> x - /file
> 
> Patch attached.

Thanks, but I think we need a bit more background.
Unfortunately the ar archive format and long names format are not very
well documented. And there seem to be various different formats.

What our implementation follows is what I believe is the sysv format,
which terminates long names with a '/' and LF. So the current
implementation searches for a '/' and then creates a terminated (NUL)
string, and skips the LF (we don't actually check there is a LF).

You do terminate the string at a '/' but then start searching for the
next long name at the LF (which in your example isn't there).

So if I understand correctly we would still not support directories
in ar files. But maybe that is not the point of your patch?
Is your example something that is actually produced by another ar
implementation? Or is it an example of a bad long file name that
we don't handle properly?

Thanks,

Mark

BTW. For patches we require people to follow the guidelines in the
CONTRIBUTING file (in particular we require a Signed-off-by line):
https://git.fedorahosted.org/cgit/elfutils.git/tree/CONTRIBUTING

Re: Directory traversal in `ar`

[Alexander Cherepanov]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

On 2015-01-05 22:16, Alexander Cherepanov wrote:
> On 2014-12-30 01:17, Alexander Cherepanov wrote:
>> On 2014-12-29 03:00, Mark Wielaard wrote:
>>>>> BTW. For patches we require people to follow the guidelines in the
>>>>> CONTRIBUTING file (in particular we require a Signed-off-by line):
>>>>> https://git.fedorahosted.org/cgit/elfutils.git/tree/CONTRIBUTING
>>>>
>>>> Sorry, a better patch attached.
>>>
>>> The patch looks perfect and I agree with your analysis.
>>> Pushed as is to master.
>>
>> Cook, thanks!
>
> Hm, s/Cook/Cool/
>
>> CVE request is here:
>> http://www.openwall.com/lists/oss-security/2014/12/29/2
>
> CVE-2014-9486 is assigned here:
> http://www.openwall.com/lists/oss-security/2015/01/03/9

Hm, two CVEs were erroneously issued for this issue and CVE-2014-9486 
was REJECTed in the end. The right one is CVE-2014-9447.

http://www.openwall.com/lists/oss-security/2015/01/07/3

-- 
Alexander Cherepanov

Re: Directory traversal in `ar`

[Alexander Cherepanov]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

On 2014-12-30 01:17, Alexander Cherepanov wrote:
> On 2014-12-29 03:00, Mark Wielaard wrote:
>>>> BTW. For patches we require people to follow the guidelines in the
>>>> CONTRIBUTING file (in particular we require a Signed-off-by line):
>>>> https://git.fedorahosted.org/cgit/elfutils.git/tree/CONTRIBUTING
>>>
>>> Sorry, a better patch attached.
>>
>> The patch looks perfect and I agree with your analysis.
>> Pushed as is to master.
>
> Cook, thanks!

Hm, s/Cook/Cool/

> CVE request is here:
> http://www.openwall.com/lists/oss-security/2014/12/29/2

CVE-2014-9486 is assigned here:
http://www.openwall.com/lists/oss-security/2015/01/03/9

-- 
Alexander Cherepanov

Re: Directory traversal in `ar`

[Alexander Cherepanov]

[-- Attachment #1: Type: text/plain, Size: 513 bytes --]

On 2014-12-29 03:00, Mark Wielaard wrote:
>>> BTW. For patches we require people to follow the guidelines in the
>>> CONTRIBUTING file (in particular we require a Signed-off-by line):
>>> https://git.fedorahosted.org/cgit/elfutils.git/tree/CONTRIBUTING
>>
>> Sorry, a better patch attached.
>
> The patch looks perfect and I agree with your analysis.
> Pushed as is to master.

Cook, thanks!

CVE request is here:
http://www.openwall.com/lists/oss-security/2014/12/29/2

-- 
Alexander Cherepanov

Re: Directory traversal in `ar`

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1764 bytes --]

On Sun, Dec 28, 2014 at 11:00:48PM +0300, Alexander Cherepanov wrote:
> >Is your example something that is actually produced by another ar
> >implementation? Or is it an example of a bad long file name that
> >we don't handle properly?
> 
> Yes, this is a constructed example of a malicious file. An attempt to
> extract the contents of the archive will lead to creation of a file in the
> root directory. It's usually agreed that unpackers and similar tools should
> not by default touch files outside the working directory. The danger is in
> overwriting sensitive files by an unconscious user or by an automatic
> process.
> 
> For similar examples please see
> https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2007-4131 (tar),
> https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2010-4651 (patch). And I
> recently reported the same problem in binutils:
> https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-8737 .
> 
> In case of elfutils the danger is mitigated by the fact that AFAICT only one
> '/' is possible in a filename and only in the leading position. Hence only
> files in the root directory can be written with this attack and only when ar
> is executed by root.

Aha. I should have immediately guessed you were looking for something
malicious :) Thanks for the explanation. And yes, the only '/' possible
without your patch is at the start of the long name.

> >BTW. For patches we require people to follow the guidelines in the
> >CONTRIBUTING file (in particular we require a Signed-off-by line):
> >https://git.fedorahosted.org/cgit/elfutils.git/tree/CONTRIBUTING
> 
> Sorry, a better patch attached.

The patch looks perfect and I agree with your analysis.
Pushed as is to master.

Thanks,

Mark

Re: Directory traversal in `ar`

[Alexander Cherepanov]

[-- Attachment #1: Type: text/plain, Size: 3029 bytes --]

On 2014-12-28 14:54, Mark Wielaard wrote:
> On Sun, Dec 28, 2014 at 02:46:15AM +0300, Alexander Cherepanov wrote:
>> There is a directory traversal in `ar`:
>>
>> # printf '!<arch>\n%-48s%-10s`\n//file/\n%-48s%-10s`\n' // 8 /1 0 > test.a
>> # ar -xv test.a
>> x - /file
>>
>> Patch attached.
>
> Thanks, but I think we need a bit more background.

Yes, sorry, I was excessively terse.

> Unfortunately the ar archive format and long names format are not very
> well documented. And there seem to be various different formats.

Sorry, I did not look into this, I only looked at what elfutils implements.

> What our implementation follows is what I believe is the sysv format,
> which terminates long names with a '/' and LF. So the current
> implementation searches for a '/' and then creates a terminated (NUL)
> string, and skips the LF (we don't actually check there is a LF).

Right, and this missing check is the problem.

> You do terminate the string at a '/' but then start searching for the
> next long name at the LF (which in your example isn't there).

More precisely: start searching for the end of the next line name. As 
the start of a long name is determined by an offset stored in a header, 
not by our manipulations with '/', LF and NUL.

Roughly speaking, you can check for LF or you can check for non-'/'. 
Either way is fine. But you cannot just skip it without checking.

> So if I understand correctly we would still not support directories
> in ar files. But maybe that is not the point of your patch?

Yes, it was not my intention to add a missing feature, only to fix a vuln.

> Is your example something that is actually produced by another ar
> implementation? Or is it an example of a bad long file name that
> we don't handle properly?

Yes, this is a constructed example of a malicious file. An attempt to 
extract the contents of the archive will lead to creation of a file in 
the root directory. It's usually agreed that unpackers and similar tools 
should not by default touch files outside the working directory. The 
danger is in overwriting sensitive files by an unconscious user or by an 
automatic process.

For similar examples please see 
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2007-4131 (tar), 
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2010-4651 (patch). 
And I recently reported the same problem in binutils: 
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-8737 .

In case of elfutils the danger is mitigated by the fact that AFAICT only 
one '/' is possible in a filename and only in the leading position. 
Hence only files in the root directory can be written with this attack 
and only when ar is executed by root.

> BTW. For patches we require people to follow the guidelines in the
> CONTRIBUTING file (in particular we require a Signed-off-by line):
> https://git.fedorahosted.org/cgit/elfutils.git/tree/CONTRIBUTING

Sorry, a better patch attached.

-- 
Alexander Cherepanov

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-libelf-Fix-dir-traversal-vuln-in-ar-extraction.patch --]
[-- Type: text/x-patch, Size: 1856 bytes --]

>From d1cfd049f47800b2e1c71f11aef25229c7ea4e51 Mon Sep 17 00:00:00 2001
From: Alexander Cherepanov <cherepan@mccme.ru>
Date: Sun, 28 Dec 2014 19:57:19 +0300
Subject: [PATCH] libelf: Fix dir traversal vuln in ar extraction.

read_long_names terminates names at the first '/' found but then skips
one character without checking (it's supposed to be '\n'). Hence the
next name could start with any character including '/'. This leads to
a directory traversal vulnerability at the time the contents of the
archive is extracted.

The danger is mitigated by the fact that only one '/' is possible in a
resulting filename and only in the leading position. Hence only files
in the root directory can be written via this vuln and only when ar is
executed as root.

The fix for the vuln is to not skip any characters while looking
for '/'.

Signed-off-by: Alexander Cherepanov <cherepan@mccme.ru>
---
 libelf/ChangeLog   | 5 +++++
 libelf/elf_begin.c | 5 +----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 6a1c925..26b63dc 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,8 @@
+2014-12-28  Alexander Cherepanov  <cherepan@mccme.ru>
+
+	* elf_begin.c (read_long_names): Don't miss '/' right after
+	another '/'. Fixes a dir traversal vuln in ar extraction.
+
 2014-12-25  Mark Wielaard  <mjw@redhat.com>
 
 	* elf_begin.c (__libelf_next_arhdr_wrlock): ar_size cannot be
diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c
index 947b0ed..ae1e712 100644
--- a/libelf/elf_begin.c
+++ b/libelf/elf_begin.c
@@ -749,10 +749,7 @@ read_long_names (Elf *elf)
 	    }
 
 	  /* NUL-terminate the string.  */
-	  *runp = '\0';
-
-	  /* Skip the NUL byte and the \012.  */
-	  runp += 2;
+	  *runp++ = '\0';
 
 	  /* A sanity check.  Somebody might have generated invalid
 	     archive.  */
-- 
2.1.3

Directory traversal in `ar`

[Alexander Cherepanov]

[-- Attachment #1: Type: text/plain, Size: 202 bytes --]

Hi!

There is a directory traversal in `ar`:

# printf '!<arch>\n%-48s%-10s`\n//file/\n%-48s%-10s`\n' // 8 /1 0 > test.a
# ar -xv test.a
x - /file

Patch attached.

-- 
Alexander Cherepanov

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-dir-traversal-in-ar.patch --]
[-- Type: text/x-patch, Size: 410 bytes --]

diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c
index 947b0ed..ae1e712 100644
--- a/libelf/elf_begin.c
+++ b/libelf/elf_begin.c
@@ -749,10 +749,7 @@ read_long_names (Elf *elf)
 	    }
 
 	  /* NUL-terminate the string.  */
-	  *runp = '\0';
-
-	  /* Skip the NUL byte and the \012.  */
-	  runp += 2;
+	  *runp++ = '\0';
 
 	  /* A sanity check.  Somebody might have generated invalid
 	     archive.  */