This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Directory traversal in `ar`


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
>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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]