[Clamav-devel] libclamunrar and several year-old critical vulnerability in unrar code

[Andreas Weigel]

Hello,

a critical vulnerability in the unrar Code (version 5.5.3) has been 
rediscovered (has already been noticed in Sophos-specific unrar code in 
2012 but the fix has never made it upstream):

https://bugs.chromium.org/p/project-zero/issues/detail?id=1286&desc=6#maincol

checking the unrar code in libclamunrar, I found that exactly the same 
vulnerability exists:

>     [...]
>     int op_type, cur_channel, byte_count, start_pos, pa, pb, pc;
>     [...]
>     case VMSF_DELTA:
>         data_size = rarvm_data->R[4];
>         channels = rarvm_data->R[0];
>         src_pos = 0;
>         border = data_size*2;
>
>         SET_VALUE(FALSE, &rarvm_data->mem[VM_GLOBALMEMADDR+0x20], 
> data_size);
>         if ((unsigned int)data_size >= VM_GLOBALMEMADDR/2) {
>             break;
>         }
>         for (cur_channel=0 ; cur_channel < channels ; cur_channel++) {
>             unsigned char prev_byte = 0;
>             for (dest_pos=data_size+cur_channel ; dest_pos<border ; 
> dest_pos+=channels) {
>                 rarvm_data->mem[dest_pos] = (prev_byte -= 
> rarvm_data->mem[src_pos++]);
>             }
>         }
>         break;
>     [...]
The test file given on the page linked above does not trigger the 
problem with clamscan, but only due to the code in question not being 
executed, because of other issues at other places. I was able to 
reproduce the invalid memory write taking the following steps (against 
tag clamav-0.99.2, corresponding to the latest stable release):

- increase the size of the memory region allocated for vmcode in 
unrar.c::read_vm_code() and initialize it properly (inspired by the way 
the current "upstream" implementation of unrar allocates memory)

    --> without this "fix", the test file causes libclamunrar to read 
from unitialized memory, resulting in a vmcode array that doesn't pass 
the test in unrarvm.c:rarvm_prepare xor_sum (and thereby preventing the 
vulnerable code to be executed)

    --> this issue has been fixed by a more clean approach in the 
current master branch by introducing bounds checking in getbits; 
thereby, the correct vmcode array is created 
(https://github.com/vrtadmin/clamav-devel/commit/7cdd2b551244d83add7d3f7062ea11475a51762a) 
and the code advances past the xor_sum check.

- I (by somewhat by qualified guessing) commented 
libclamunrar/unrar.c:894 (master) to make the unpack routine continue, 
which otherwise fails after some iterations in unrar.c:110. Thereby, the 
invalid memory access can be reproduced. If the cancellation of 
processing before is a bug or a feature, I currently can not tell.

I do currently neither know enough about the RAR format nor about the 
purpose of the used code, but as it seems there are a number of (quite 
pointless?) read system calls with length=0 in rar_unp_read_buf, which 
finally fails due to unpack_data->in_addr advancing beyond 
unpack_data->read_top. To me, it seems that the code fails to continue 
processing by chance more than by purpose. Be that as it may, the 
vulnerability should be fixed in any case. I would not be surprised if a 
slightly modified test file could cause the issue to surface with the 
unmodified code.

Apart from that, are there any plans to use the current upstream version 
of unrar? While this would have been counterproductive in the case at 
hand, at least it could possibly help to reduce synchronization issues 
in the future. The current version of libclamunrar seems to be a quite 
messy piece of code and there been at least some improvements in the 
"upstream" version.

Kind Regards,

Andreas

-- 
Mit freundlichem Gruß / Best regards,

Andreas Weigel
UTM Backend Developer

Securepoint GmbH
Salzstrasse 1
D-21335 Lüneburg

https://www.securepoint.de

Tel.: +49(0)413124010
Fax: +49(0)4131240118

Geschäftsführer: Lutz Hausmann, Claudia Hausmann
Amtsgericht Lüneburg HRB 1776
USt.-ID-Nr.: DE 188 528 597