Bug 11943 - Heap overflow in zma_bswap_4861dc (mew.c)
Heap overflow in zma_bswap_4861dc (mew.c)
Status: RESOLVED FIXED
Product: ClamAV
Classification: ClamAV
Component: All
ALL
x86_64 GNU/Linux
: P1 security
: 0.99.3
Assigned To: Steven Morgan
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-26 19:14 EDT by Suleman Ali
Modified: 2018-03-11 18:16 EDT (History)
9 users (show)

See Also:
QA Contact:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Suleman Ali 2017-10-26 19:14:08 EDT
There is a read out of bounds in the line `mew.c:97` inside the function `lzma_bswap_4861dc`:
```c
 97         p->p2 = EC32(CE32(((uint32_t)cli_readint32(old_edx + 1))));
 98         p->p1 = 0xffffffff;
 99         p->p0 = old_edx + 5;
``` 

The problem comes from the caller function `mew_lzma` where no validation is done before calling `lzma_bswap_4861dc`:
```c
425                 var14 = var10 = var24 = 1;
426 
427                 lzma_bswap_4861dc(&var40, var2C);
428                 new_edx = 0;
429         } while (var28 <= loc_esi); /* source = 0 */
```

The condition to exit the loop could be simplified for:
```c
429         } while (var28 <= 0);
```
Where `var28` contains the last 4 bytes of the input file. That means that the loop is going to continue while the input faile has 0's or negative integers even if the buffer has been completely consumed.

`var2C` is the source of the data read inside `lzma_bswap_4861dc`. The variable is set some line before:
```c
407                 source += 5; /* yes, five */
408                 var2C = source;
409                 source += temp;
```

One possible solution is to add an extra validation before calling the function `lzma_bswap_4861dc`:
```c
425                 var14 = var10 = var24 = 1;
426 
427                 if(CLI_ISCONTAINED(orgsource, size_sum, var2C, 5))
428			return -1;
429		    lzma_bswap_4861dc(&var40, var2C);
430                 new_edx = 0;
431         } while (var28 <= loc_esi); /* source = 0 */
```

Backtrace:
```c
   #0 0x824be11 in lzma_bswap_4861dc /tmp/clamav-devel/libclamav/mew.c:97:2
    #1 0x824be11 in mew_lzma /tmp/clamav-devel/libclamav/mew.c:427
    #2 0x824ed66 in unmew11 /tmp/clamav-devel/libclamav/mew.c:897:6
    #3 0x81e5268 in cli_scanpe /tmp/clamav-devel/libclamav/pe.c:3958:13
    #4 0x8171750 in magic_scandesc /tmp/clamav-devel/libclamav/scanners.c:3542:19
    #5 0x8167904 in cli_base_scandesc /tmp/clamav-devel/libclamav/scanners.c:3616:11
    #6 0x8176391 in cli_magic_scandesc /tmp/clamav-devel/libclamav/scanners.c:3625:12
    #7 0x8176391 in scan_common /tmp/clamav-devel/libclamav/scanners.c:3891
    #8 0x8177162 in cl_scandesc_callback /tmp/clamav-devel/libclamav/scanners.c:4032:12
    #9 0x8177162 in cl_scanfile_callback /tmp/clamav-devel/libclamav/scanners.c:4099
    #10 0x8177162 in cl_scanfile /tmp/clamav-devel/libclamav/scanners.c:4082
```

This vulnerability has been found by Offensive Research at Salesforce.com:
Alberto Garcia (@algillera), Francisco Oca (@francisco_oca) & Suleman Ali (@Salbei_)
Comment 1 Joel Esler 2017-10-27 09:51:51 EDT
Thank you very much for reporting these issues.  If possible, we'd like to gather some initial first details from you on these.  

First, can you tell us what version of ClamAV you are reporting against?  I notice that you have our git-repository name in the reports, is this a fresh check out of git?
Comment 2 Steven Morgan 2017-10-27 12:00:23 EDT
Hello,

Do you have a POC for this?
Comment 3 Suleman Ali 2017-10-27 16:02:59 EDT
Created attachment 7312 [details]
poc, pass is virus

Attached poc, sorry about that. This is the latest version from the git repo, the bugs work on some older versions as well depending on whether the functionality in question was implemented.
Comment 4 Steven Morgan 2017-10-27 16:57:38 EDT
Suggested fix applied in commit 38da4800bfb2d6b13579950b6543302d13e3015c.
Comment 6 andrey 2017-11-01 21:29:19 EDT
Is it possible to get more information on how the fix manifests? Perhaps as a comment inside the file?
Comment 7 Steven Morgan 2017-11-13 16:40:51 EST
(In reply to andrey from comment #6)
> Is it possible to get more information on how the fix manifests? Perhaps as
> a comment inside the file?

Andrey, I used valgrind to see the issue.
Comment 8 Steven Morgan 2017-11-13 16:46:19 EST
We are observing false negative in our regression runs using the suggested patch.

I believe the patch should be:

425                 var14 = var10 = var24 = 1;
426 
427                 if(!CLI_ISCONTAINED(orgsource, size_sum, var2C, 5))
428			return -1;
429		    lzma_bswap_4861dc(&var40, var2C);
430                 new_edx = 0;
431         } while (var28 <= loc_esi); /* source = 0 */
```

That is, negating the result of CLI_ISCONTAINED() with the ! operator.
Comment 9 andrey 2017-11-14 10:57:49 EST
Steve, let us know when the fix is in.
Comment 10 Suleman Ali 2017-11-21 16:26:57 EST
Hi Joel/Steven,

Any updates on the CVEs? Do I have to contact PSIRT or would you guys do that?
Comment 11 Steven Morgan 2017-11-21 16:29:09 EST
(In reply to Suleman Ali from comment #10)
> Hi Joel/Steven,
> 
> Any updates on the CVEs? Do I have to contact PSIRT or would you guys do
> that?

PSIRT is working on the CVEs.
Comment 12 Steven Morgan 2017-11-21 16:31:23 EST
(In reply to andrey from comment #9)
> Steve, let us know when the fix is in.

Sorry for the delay, the reworked fix went in last week in commit e887f113242ffcb0ea8735c3f567c6be77f382d6.