On 12/08/17 20:40, Sebastian Andrzej Siewior wrote:
On 2017-08-12 00:42:06 [+0100], Stuart Caie wrote:
On 11/08/17 19:07, Sebastian Andrzej Siewior wrote:
[0] https://security-tracker.debian.org/tracker/CVE-2017-6419
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-6419
[1]
https://github.com/vrtadmin/clamav-devel/commit/a83773682e856ad6529ba6db8d1792e6d515d7f1
Stuart, is this enough information or do you need more?
I'm interested in how the fix is to add a check to see if
window_posn+this_run wraps the window, immediately below a comment that
expressly states that won't happen, with the reasoning: this_run has already
been clamped to ensure it does not wrap a frame, and frames don't wrap
windows.
If this is incorrect reasoning, what part of the reasoning is wrong? Is
this_run somehow not being clamped to <=FRAME_SIZE through some code path?
If so, the fix is to clamp it. Is window size not a multiple of frame size?
If so, something is very wrong.
The CVE links the following link:
https://github.com/varsleak/varsleak-vul/blob/master/clamav-vul/heap-overflow/clamav_chm_crash.md
and that folder contains also
https://github.com/varsleak/varsleak-vul/raw/master/clamav-vul/heap-overflow/clamav.crash.chm
and clamav segfaults on that one. Could you please check?
OK, so now I know why.
mspack/chmd.c:1191 (read_reset_table) can't read reset table
mspack/lzxd.c:327 (lzxd_init) output_length=-884758871994
mspack/lzxd.c:336 (lzxd_init) reset_interval=16386
mspack/lzxd.c:467 (lzxd_decompress) length=-884758871994
mspack/lzxd.c:468 (lzxd_decompress) offset=0
mspack/lzxd.c:469 (lzxd_decompress) length-offset=-884758871994
mspack/lzxd.c:470 (lzxd_decompress) frame_size=4390982
mspack/lzxd.c:474 (lzxd_decompress) bytes_todo=4390982
mspack/lzxd.c:539 (lzxd_decompress) this_run=4390982
mspack/lzxd.c:777 (lzxd_decompress) uncompressed: window_posn=0,
this_run=4390982, window_size=65536
The root cause is that chmd.c initialises the lzx stream with an
incorrect output_length.
We decide how large the LZX frame will be. It'll either be 32k, or, if
we know how far we are along in the output stream, and we know there is
less than 32k left to decode, this must be the final frame, which is
allowed to be less than 32k:
frame_size = LZX_FRAME_SIZE;
if (lzx->length && (lzx->length - lzx->offset) < (off_t)frame_size) {
frame_size = lzx->length - lzx->offset;
}
(lzx->length - lzx->offset) should always be a positive number.. but
only if length is positive!
With negative output_length, it fails this test, and sets frame_size to
any value (the bottom 32 bits of output_length).
Complete control of frame_size could cause trouble in lots of places,
not just at the uncompressed block copy.
So, the right thing to do is to guard against negative output_length.
I've done this, and additionally added checking to chmd read_spaninfo()
to place the blame where it lies:
https://github.com/kyz/libmspack/commit/6139a0b9e93fcb7fcf423e56aa825bc869e02229
The example file is now rejected as invalid data:
mspack/chmd.c:1191 (read_reset_table) can't read reset table
mspack/chmd.c:1276 (read_spaninfo) output length is invalid
clamav.crash.chm: extract error on "/#IDXHDR": error in data format
Yeah. The problem is probably that the reporter did not forward the report to
the upstream project (you) but only to ClamAV. And ClamAV itself isn't very
good at handling such things. I noticed this by chance while checking Debian's
bug report on the other CVE and I assumed it would be best to let you know :)
Thanks very much, I appreciate it.
Regards
Stuart
