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 <[email protected]>
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 <[email protected]>
---
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 <[email protected]>
+
+ * 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 <[email protected]>
* 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
