Bug 757117

Ok, I just tried the following: Downloaded the lha source rpm from RHEL 4 and built it on my Fedora 14 box.

This is the valgrind output:
$ valgrind lha x AA_Ticket.exe 
==6914== Memcheck, a memory error detector
==6914== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==6914== Using Valgrind-3.6.0 and LibVEX; rerun with -h for copyright info
==6914== Command: lha x AA_Ticket.exe
==6914== 
LHa: Warning: Checksum error (LHarc file?) 
LHa: Error: Unknown method skiped ... 
==6914== Invalid read of size 1
==6914==    at 0x4059D7: get_word (in /usr/bin/lha)
==6914==    by 0x405F89: get_header (in /usr/bin/lha)
==6914==    by 0x4050B4: cmd_extract (in /usr/bin/lha)
==6914==    by 0x401EE2: main (in /usr/bin/lha)
==6914==  Address 0x7ff0042aa is not stack'd, malloc'd or (recently) free'd
==6914== 
==6914== 
==6914== Process terminating with default action of signal 11 (SIGSEGV)
==6914==  Access not within mapped region at address 0x7FF0042AA
==6914==    at 0x4059D7: get_word (in /usr/bin/lha)
==6914==    by 0x405F89: get_header (in /usr/bin/lha)
==6914==    by 0x4050B4: cmd_extract (in /usr/bin/lha)
==6914==    by 0x401EE2: main (in /usr/bin/lha)

(In reply to comment #4)

Thanks, Thomas. Reproduced now. I am going to steal this bug to be Security Response product.

Due the 'Additional Info' part from #c0 yet:

>This crash is remote triggerable if lha is used by amavisd-new. amavisd-new
>runs .exe files through lha in the default config to extract self-expanding
>archives.

Can you detail, which impact this has on amavisd? Once the lha process
crashes in attempt to expand the archive, how amavisd behaves? Does it
terminate (as a whole too)? Or just some amavisd worker / thread terminates?

> I've already contacted the amavisd-new author privately
> to discuss changes to the default config.

Is it possible to assume this lha issue is still embargoed or not?
In other words, how many details have been shared with amavisd upstream?

> Additionally there seems to be no upstream maintainer for lha."

Yes, unfortunately. Wasn't even able to find upstream repository for lha
anymore. But hopefully we will find a way how to workaround this.

Due the credit information of the issue -- could you please let us know the form
in which you would like to be credited for discovery of this flaw?

And also, if issue consequences not completely shared with amavisd upstream yet,
would it be possible you not to disclose the flaw details publicly till a embargo
date is reached? (to be clarified later yet depending on patch availability)

Also, would it be possible us to share the details with other Linux distribution
vendors, also shipping the lha package, who might be interested in knowing the
fix *privately* yet prior the embargo date has passed? Could you provide an
approval for us to do so?

Thank you && Regards, Jan.
--
Jan iankko Lieskovsky / Red Hat Security Response Team

An out of stack-based buffer read flaw was found in the way lha, an archiving and compression utility for LHarc/lha/lzh archives, expanded certain LHA archive files. A remote attacker could provide a specially-crafted LHA archive, which once opened by a local, unsuspecting user could lead to lha executable crash. A different vulnerability than CVE-2004-0771, CVE-2004-0769, CVE-2004-0745, CVE-2004-0694, CVE-2004-0235, and CVE-2004-0234.

This issue affects the version of the lha package, as shipped with Red Hat
Enterprise Linux 4.

> Can you detail, which impact this has on amavisd? Once the lha process
> crashes in attempt to expand the archive, how amavisd behaves? Does it
> terminate (as a whole too)? Or just some amavisd worker / thread terminates?

This is the amavisd log output after scanning a bad archive:

Nov 24 01:02:32 server amavis[8336]: (08336-05) (!!)TROUBLE in process_request: TempDir::check: Unexpected file /var/spool/vscan/amavis/amavis-20111124T002639-08336/core at /usr/sbin/amavisd line 5442, <GEN42> line 2362.
Nov 24 01:02:32 server amavis[8336]: (08336-05) (!)Requesting process rundown after fatal error
Nov 24 01:02:32 server amavis[8336]: (08336-05) (!)TempDir removal: tempdir is to be PRESERVED: /var/spool/vscan/amavis/amavis-20111124T002639-08336


So basically amavisd-new doesn't notice the lha crash and then stumbles upon the "core" file from the crash. This results in a process run down and some wasted space in the spool area. Probably the process run down might have the worst performance impact.

We clean our spool area periodically, there might also be a chance of the spool partition filling up. I need to check the default amavisd-new behavior.

> In other words, how many details have been shared with amavisd upstream?

Only Mark Martinec, the maintainer of amavisd-new, was informed privately. He already mentioned the next version of amavisd-new will probably drop support for lha as the code is totally unmaintained and therefore a security hazard.

I think it's a good idea to add Mark to this bug #, so we could coordinate the release of the lha fix + the availability of a workaround on the amavisd-new side. His email address is "Mark Martinec <Mark.Martinec+amavis>".


Please credit this issue to "Thomas Jarosch of Intra2net AG".

Yes, please share the issue with other distributions, Mandriva, Debian and OpenSuSE are affected, too. It's bad that vendor-sec is gone (though I was never part of it).

The core problem of this issue seems to be the following (Michal please correct me if / where necessary):

The lha executable is crashing at line #39 in (BUILD/lha-114i/src/header.c) get_word() routine: 

     33 static unsigned short
     34 get_word()
     35 {
     36         int             b0, b1;
     37 
     38         b0 = get_byte();
     39         b1 = get_byte();
     40         return (b1 << 8) + b0;
     41 }

The get_byte() macro is defined in BUILD/lha-114i/src/lha_macro.h:341 as follows:

    339 /* header.c */
    340 #define setup_get(PTR)  (get_ptr = (PTR))
    341 #define get_byte()              (*get_ptr++ & 0xff)

Note: So first we call 'setup_get' with particular pointer value (we would like to use by next 'get_byte()' call, then call get_byte() to retrieve the byte.

Let's look like setup_get() is used in get_header() routine
(BUILD/lha-114i/src/header.c):

    394 /* ------------------------------------------------------------------------ */
    395 /* build header functions                     */
    396 /* ------------------------------------------------------------------------ */
    397 boolean
    398 get_header(fp, hdr)
    [..]
    424         setup_get(data + I_HEADER_LEVEL);

'I_HEADER_LEVEL' is defined as follows:

lha_macro.h:163:#define I_HEADER_LEVEL			20

and 'data' buffer as follows:

    394 /* ------------------------------------------------------------------------ */
    395 /* build header functions                      */
    396 /* ------------------------------------------------------------------------ */
    397 boolean
    398 get_header(fp, hdr)
    399         FILE           *fp;
    400         register LzHeader *hdr;
    401 {
    402         int             header_size;
    403         int             name_length;
    404         char            data[LZHEADER_STRAGE];

'LZHEADER_STRAGE' as follows:

lha_macro.h:239:#define LZHEADER_STRAGE			4096

The 'data' buffer is being filed in the following get_header() code part:

    415         if (((header_size = getc(fp)) == EOF) || (header_size == 0)) {
    416                 return FALSE;   /* finish */
    417         }
    418 
    419         if (fread(data + I_HEADER_CHECKSUM,
    420                   sizeof(char), header_size - 1, fp) < header_size - 1) {
    421                 fatal_error("Invalid header (LHarc file ?)");
    422                 return FALSE;   /* finish */
    423         }
    424         setup_get(data + I_HEADER_LEVEL);

So we first retrieve 'header_size' from input file, then read 'header_size -1' values from the file to fill in 'data' buffer and subsequently setup pointer in 'data' buffer to be 'data + I_HEADER_LEVEL'.

But if the 'header_size' value in archive is specially-crafted, at line #424 we can end up having get_ptr to be set to something, which is more than "4096 + 20" bytes (or something which would be close to upper border of it), thus subsequent call of 'get_byte' would end up attempting to read value out of that stack allocated buffer 'data' and we would end up with crash (when 'header_size' is close to upper bound of data + I_HEADER_LEVEL it's just question of time, which get_byte() call would seg fault).

My proposal how to fix this issue is to add upper bound check to 'header_size' or 'setup_get()' macro, so we couldn't end up attempting to read byte past the end of the buffer, allocated on the stack.

Hi Thomas,

Thanks again for reporting this issue. I agree with the analysis of my colleague Jan in comment #10, This essentially is a crash due to out-of-bounds read of a stack based buffer. 

It does not look like its exploitable and results in a crash only. I intend to call this as not a security bug and open this up to general public. Are you ok with this?

Thanks.

Hi Huzaifa,

there's still the DoS condition: If you manage to crash lha on an amavisd-new installation, it will leave a "core" file in the temporary directory.

amavisd goes into "preserving evidence" mode and keep the complete email + all extracted parts. If you sent a big message + small crashing lha file, you can use it to fill up the spool area of the mail server. As far as I can tell from the amavisd-new sources, this behavior is hardwired.

So it's not exploitable for code execution but you can trash the mail server.

What do you think?

Cheers,
Thomas

(In reply to comment #12)
> Hi Huzaifa,
> 
> there's still the DoS condition: If you manage to crash lha on an amavisd-new
> installation, it will leave a "core" file in the temporary directory.
> 
> amavisd goes into "preserving evidence" mode and keep the complete email + all
> extracted parts. If you sent a big message + small crashing lha file, you can
> use it to fill up the spool area of the mail server. As far as I can tell from
> the amavisd-new sources, this behavior is hardwired.
> 
> So it's not exploitable for code execution but you can trash the mail server.
> 
> What do you think?

Sure, I think but would'nt this be a security issue with amavisd rather?

amavisd crashes because lha crashes, this creates a core files with each crash, which fills up space in the spool?

In my opinion amavisd should be able to detect an lha crash and report this to the admin. Also you can configure your system to not dump the core, if amavisd crashes.

It really is a bug with lha, but it becomes bad when you use a network enabled application like amavisd to run it.

Statement:

The Red Hat Security Response Team does not consider crash of a client application such as lha as security flaw.

Thank you for submitting this issue for consideration in Red Hat Enterprise Linux. The release for which you requested us to review is now End of Life. 
Please See https://access.redhat.com/support/policy/updates/errata/

If you would like Red Hat to re-consider your feature request for an active release, please re-open the request via appropriate support channels and provide additional supporting details about the importance of this issue.