Bug 11946 - Heap overflow in getchecksum(untar.c)
Heap overflow in getchecksum(untar.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:30 EDT by Suleman Ali
Modified: 2021-09-23 16:36 EDT (History)
10 users (show)

See Also:
QA Contact:


Attachments
Improved fix - ensure entire header is read (1.42 KB, text/plain)
2017-11-01 15:59 EDT, Craig Davison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Suleman Ali 2017-10-26 19:30:38 EDT
Created attachment 7311 [details]
poc, pass is virus

There is an Out of bounds read at untar.c:74:
```c
 69 getchecksum(const char *header)
 70 {
 71         char ochecksum[TARCHECKSUMLEN + 1];
 72         int checksum = -1;
 73 
 74         strncpy(ochecksum, header+TARCHECKSUMOFFSET, TARCHECKSUMLEN);
 75         ochecksum[TARCHECKSUMLEN] = '\0';
 76         checksum = octal(ochecksum);
 77         return checksum;
 78 }	
```

The PoC is 10 bytes long. It's identifyed as Tar because of a special magic number `[aliases]` that is used to identify a tar CVE:
`libclamav/filetypes_int.h`
```
"0:0:5b616c69617365735d:TAR-POSIX-CVE-2012-1419:CL_TYPE_ANY:CL_TYPE_POSIX_TAR"
```

The out of bounds read does not happen when analyzing a file because of the cache used while reading a file. It only happens using directly the function to scan a memory map. Specifically when using the cl_scanmap_callback function.

A possible patch:
libclamav/untar.c
```c
182                         if((ret=cli_checklimits("cli_untar", ctx, 0, 0, 0))!=CL_CLEAN)
183                                 return ret;
+184                        if (nread < TARCHECKSUMOFFSET + TARCHECKSUMLEN)
+185                                return ret;
186                         checksum = getchecksum(block);
```

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:52:10 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 16:07:20 EDT
After analysis, suggested patch applied in commit 292d6878fa3e7fd2ab0f7275a78190639ad116d4.
Comment 3 Suleman Ali 2017-10-27 16:20:09 EDT
That was fast Morgan, thanks. Can we please get CVEs assigned for these?
Comment 4 Steven Morgan 2017-10-27 16:41:09 EDT
(In reply to Suleman Ali from comment #3)
> That was fast Morgan, thanks. Can we please get CVEs assigned for these?

You did all the work ;)

Joel is going to get your CVEs from Cisco PSIRT.
Comment 5 Suleman Ali 2017-10-27 16:54:22 EDT
Cool, thanks. This is the first time we're reporting to Cisco proper so I wasn't sure what the process was. And credits to the whole team, Francisco did most of the work for this one hehe.
Comment 7 Craig Davison 2017-10-31 19:08:28 EDT
@jesler and @stevmorg, I looked at the patch. While it will work for the getchecksum() check (, there is code below that reads further into block.

    if(posix) {
        strncpy(magic, block+257, 5);
    .
    .
    .
    }
    
    type = block[TARFILETYPEOFFSET];

TARCHECKSUMOFFSET is 148.

I think a safer thing to do would be to ensure that nread >= 512. BLOCKSIZE is 512, and the tar header is padded out to 512 bytes.
Comment 8 Craig Davison 2017-10-31 19:08:51 EDT
Credit to @antchan2 for finding the above.
Comment 9 Craig Davison 2017-11-01 15:59:04 EDT
Created attachment 7313 [details]
Improved fix - ensure entire header is read
Comment 10 Steven Morgan 2017-11-01 16:08:16 EDT
(In reply to Craig Davison from comment #9)
> Created attachment 7313 [details]
> Improved fix - ensure entire header is read

Thanks Craig, on the list. My link to git is currently broken, will get this committed ASAP.
Comment 11 Steven Morgan 2017-11-01 16:48:42 EDT
(In reply to Craig Davison from comment #9)
> Created attachment 7313 [details]
> Improved fix - ensure entire header is read

Patch applied in commit 0cf813f835e48ab0f94dd54200ceba0dc25fa1c4.
Comment 12 Craig Davison 2017-11-01 18:39:31 EDT
Thanks, Steve