Re: [SECURITY] [DSA 515-1] New lha packages fix several vulnerabilities; Re:

[GOTO Masanori <gotom () debian or jp>]

At Thu, 10 Jun 2004 10:11:17 +0900,
GOTO Masanori wrote:
> At Tue, 8 Jun 2004 10:32:48 -0700,
> Matt Zimmerman wrote:
> > On Sun, Jun 06, 2004 at 04:28:56PM -0000, lw () wszia edu pl wrote:
> > > In-Reply-To: <20040605203922.GW19402 () alcor net>
> > >
> > > i didn't bother to check deb package, but this patch:
> > > >    http://security.debian.org/pool/updates/non-free/l/lha/lha_1.14i-2woody1.diff.gz
> > > applied to this package:
> > > >      Size/MD5 checksum:    21414 0f990fd920ea4770dd088a97c1c87f18
> > > >    http://security.debian.org/pool/updates/non-free/l/lha/lha_1.14i.orig.tar.gz
> > >
> > > has not fixed this problem:
> > > http://www.securityfocus.com/archive/1/363418
> > > POC is at:
> > > http://lw.ftw.zamosc.pl/lha-exploit.txt
>
> Thanks for your checking, you're right and actually sample program
> (lha-exploit.txt) causes the problem.
>
> > > http://bugs.gentoo.org/show_bug.cgi?id=51285
> > > is some dull discussion about that bug.
>
> This patch seems being appropriate.  I'll check again, and I'll
> prepare the next version for this problem.

When I replied the above mail, I did not change three address in
lha-exploit.txt .  I changed the perl program described at
http://lw.ftw.zamosc.pl/lha-exploit.txt for my environment.  Then I
found that the proposed patch at
http://bugs.gentoo.org/show_bug.cgi?id=51285 fixes partially for this
problem.  When I used lha with "l" or "v" option:

        gotom@celesta> ./lha l ../../exploit-test/archive.lhz
         PERMSSN    UID  GID      SIZE  RATIO     STAMP           NAME
        ---------- ----------- ------- ------ ------------ --------------------
        /
        ---------- ----------- ------- ------ ------------ --------------------
         Total  v: command not found 0 ****** Jun 10 12:40
        drwxr-xr-x     0/0           0 ****** May 19 04:47 F/
        drwxr-xr-x     0/0           0 ****** May 19 04:47 
F/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/
        drwxr-xr-x     0/0           0 ****** May 19 04:47 
F/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/
        drwxr-xr-x     0/0           0 ****** May 19 04:47 
F/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/
        [unknown]                    0 ****** Sep 10 14:53 
F/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/
        [unknown]                    0 ****** Sep 10 14:53 
F/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/AAAA@@/
        ---------- ----------- ------- ------ ------------ --------------------
         Total  v: command not found 0 ****** Jun 10 12:40

"command not found" is the result of execution system().  So if
malicious local user creates /tmp/something (which is written in the
exploit binary), and then root user or root process executes
archive.tgz, the local user can do something possibly.

This is because the "LzHeader hdr" in get_header(fp, hdr) is allocated
on the stack.  But directory is allowed by 1024 (for the first fix of
this bug) in get_header() with type==2.  But hdr->name has 256 bytes.
So strcat() and strcpy() in get_header can make this problem.

        The patch from Ulf for fixing the original probelm, at header.c:

                        case 2:
                                /*
                                 * directory
                                 */
                                if (header_size >= FILENAME_LENGTH) {
                                  fprintf(stderr, "Possible buffer overflow hack attack, type
 #2\n");
                                  exit(110);
                                }

        lha_macro.h:

                #define FILENAME_LENGTH     1024

        At the end of get_header in header.c:

                if (dir_length) {
                        strcat(dirname, hdr->name);     // potential buffer overflow!
                        strcpy(hdr->name, dirname);     // potential buffer overflow!
                        name_length += dir_length;
                }

The size of the maximum path name should be taken from
pathconf(_PC_PATH_MAX), but SUSv3 says _POSIX_PATH_MAX=256 and
_XOPEN_PATH_MAX=1024.  In this case hdr->name has 256, so at least
it's allowed for this problem.  

Looking at the LHA header, LHarc format had path length limitation up
to 233 byte, but the newer format level 2 header has no limitation
theoretically.  "Possbile buffer overflow hack" is just _possible_, so
the long pathname can be included in an lzh archive.  However,
generally speaking, the path length limitation up to 1024 or even 256
byte is allowable, I think.

Of course, it's sure that the correct fix is to use variable length
buffer for hdr->name using file_length and dir_length.

So, from the above analysis, the proposed patch is safe for "x", but
not for "l".  The following patch should fix this problem for
potential malicious archive with lha option "l" "v" "x".


--- src/header.c        2002-07-19 17:23:58.000000000 +0900
+++ src/header.c        2004-06-16 09:49:23.000000000 +0900
@@ -648,8 +648,17 @@
        }
 
        if (dir_length) {
+               if ((dir_length + name_length) > sizeof(dirname)) {
+                       fprintf(stderr, "Insufficient buffer size\n");
+                       exit(112);
+               }
                strcat(dirname, hdr->name);
-               strcpy(hdr->name, dirname);
+
+               if ((dir_length + name_length) > sizeof(hdr->name)) {
+                       fprintf(stderr, "Insufficient buffer size\n");
+                       exit(112);
+               }
+               strncpy(hdr->name, dirname, sizeof(hdr->name));
                name_length += dir_length;
        }
 


Comments are welcome.

Regards,
-- gotom