[Pkg-clamav-devel] Bug#773318: Bug#773041: Bug#773318: clamav dies/hangs

[Sebastian Andrzej Siewior]

tags 773318 - moreinfo + patch
thanks

On 2014-12-23 18:15:45 [+0100], Andreas Cadhalpun wrote:
> Hi,
Hi Andreas,

> I think there is a better way than changing the type of frame_end to off_t.
> It is possible to avoid the overflow by reordering the code:

Even better, I like it. The patch at the end of the email is what I
pushed into the wheezy branch for clamav [0]. So after an upload for
stable, we have Wheezy fixed. I have no idea how to close this bug for
unstable since it has to be fixed in a different package. Probably
manually with a comment…
Side question: In case someone has unattended-upgrades running, how does
he get clamd restarted after a libmspack update?

>From a0449d2079c4ba5822e6567ad7094c10108f16cd Mon Sep 17 00:00:00 2001
From: Sebastian Andrzej Siewior <sebastian at breakpoint.cc>
Date: Tue, 23 Dec 2014 21:20:43 +0100
Subject: libmspack: qtmd: fix frame_end overflow

Debian bts #773041, #772891 contains a report of a .cab file which
causes an endless loop.
Eric Sharkey diagnosed the problem as frame_end is 32bit and overflows
and the result the loop makes no progress.
The problem seems that after the overflow, window_posn is larger than
frame_end and therefore we never enter the loop to make progress. But we
still have out_bytes >0 so we don't leave the outer loop either.

Andreas Cadhalpun suggested to instead makeing frame_end 64bit, we could
avoid the overflow by reordering the code the following way:

original, with just out_bytes (without (qtm->o_end - qtm->o_ptr))
| frame_end = window_posn + out_bytes;
| if ((window_posn + frame_todo) < frame_end) {
|         frame_end = window_posn + frame_todo;
| }

replace frame_end in "if" with its content (and move the first frame_end
into the else path)
| if ((window_posn + frame_todo) < (window_posn + out_bytes))
|         frame_end = window_posn + frame_todo;
| else
|         frame_end = window_posn + out_bytes;

remove window_posn from "if" since it is the same both times.
| if (frame_todo <  out_bytes)
|         frame_end = window_posn + frame_todo;
| else
|         frame_end = window_posn + out_bytes;

Andreas added:
|This works, because frame_todo is at most QTM_FRAME_SIZE = 32768.

Suggested-as-patch: Andreas Cadhalpun <andreas.cadhalpun at googlemail.com>
[sebastian at breakpoint: added patch description]
Signed-off-by: Sebastian Andrzej Siewior <sebastian at breakpoint.cc>
---
 libclamav/libmspack-0.4alpha/mspack/qtmd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/libclamav/libmspack-0.4alpha/mspack/qtmd.c b/libclamav/libmspack-0.4alpha/mspack/qtmd.c
index 12b27f5608c4..e584aef8e576 100644
--- a/libclamav/libmspack-0.4alpha/mspack/qtmd.c
+++ b/libclamav/libmspack-0.4alpha/mspack/qtmd.c
@@ -296,9 +296,10 @@ int qtmd_decompress(struct qtmd_stream *qtm, off_t out_bytes) {
 
     /* decode more, up to the number of bytes needed, the frame boundary,
      * or the window boundary, whichever comes first */
-    frame_end = window_posn + (out_bytes - (qtm->o_end - qtm->o_ptr));
-    if ((window_posn + frame_todo) < frame_end) {
+    if (frame_todo < (out_bytes - (qtm->o_end - qtm->o_ptr))) {
       frame_end = window_posn + frame_todo;
+    } else {
+      frame_end = window_posn + (out_bytes - (qtm->o_end - qtm->o_ptr));
     }
     if (frame_end > qtm->window_size) {
       frame_end = qtm->window_size;


[0] https://anonscm.debian.org/cgit/pkg-clamav/clamav.git/tree/debian/patches/0018-libmspack-qtmd-fix-frame_end-overflow.patch?h=wheezy

Sebastian