xorl %eax, %eax

ClamAV CLI_ISCONTAINED Invalid Calculation

with 2 comments

This was reported by Martin Olsen of LightSpeedSystems and disclosed on 7 April 2009. Unfortunately for us, ClamAV seems to have a “nothing is critical security bug to us” policy and if you don’t keep an eye on their ChangeLog you’ll miss any of those (of course regarding public vulnerabilities :-P). Anyway, this affects ClamAV prior to 0.95.1 release. Here I’ll be using 0.95 to examine the bug. The problem can be found at libclamav/others.h where CLI_ISCONTAINED() and CLI_ISCONTAINED2() macros are defined. These macros were written by Tomasz Kojm and are widely used in various packers on ClamAV to find data inside a buffer in a wrap-free manner. As the comment says:

52 /*
53  * CLI_ISCONTAINED(buf1, size1, buf2, size2) checks if buf2 is contained     
54  * within buf1.
55  *
56  * buf1 and buf2 are pointers (or offsets) for the main buffer and the
57  * sub-buffer respectively, and size1/2 are their sizes
58  *
59  * The macro can be used to protect against wraps.
60  */
61 #define CLI_ISCONTAINED(bb, bb_size, sb, sb_size)       \
62     (bb_size > 0 && sb_size > 0 && (size_t)sb_size <= (size_t)bb_size \
63      && sb >= bb && sb + sb_size <= bb + bb_size && sb + sb_size > bb)
64
65 #define CLI_ISCONTAINED2(bb, bb_size, sb, sb_size)      \
66     (bb_size > 0 && sb_size >= 0 && (size_t)sb_size <= (size_t)bb_size \
67      && sb >= bb && sb + sb_size <= bb + bb_size && sb + sb_size >= bb)
68


For better understanding, here is a more readable form of CLI_ISCONTAINED():

if ( buffer1_size > 0 &&
     buffer2_size > 0 &&
     buffer2_size <= buffer1_size &&
     buffer2 >= buffer1 &&
     (buffer2 + buffer2_size) <= (buffer1 + buffer1_size) &&
     (buffer2 + buffer2_size) > buffer1 )
  return 1;
else
  return 0;

I know its the same but occasionally I re-write snippets with more descriptive names to make them simpler and easier to understand and remember. Anyway, so what’s wrong with this? It’s simple but I think not so easy to spot. The problem is on how it passes the macro parameters. It doesn’t use parenthesis  on them! This results in miscalculations that can lead to invalid results. Especially, calls that pass in parameters like this:

libclamav/upack.c:

85       if (upack_version == UPACK_399)
86       {
87               /* jmp 1 */
88               loc_edi = dest + (cli_readint32(loc_esi) -  vma);
89               if (!CLI_ISCONTAINED(dest, dsize, dest+ep+0xa, 2) || dest[ep+0xa] != '\xeb')


Can easily return invalid results. These results can lead to processing a corrupted buffers which I’m not aware of the consequences since those two macros are widely used in ClamAV library. Probably on some code this might lead to an exploitable condition. Oh.. and by the way, did I mention that these macros are used on numerous locations. For example, just from libclamav/upx.c I got these:

static char *checkpe():

102   char *sections;
103   if (!CLI_ISCONTAINED(dst, dsize,  pehdr, 0xf8)) return NULL;
     ...
112   if (!CLI_ISCONTAINED(dst, dsize, sections, *sectcnt*0x28)) return NULL;
113
114   return sections;


static int pefromupx():

149   if (valign && CLI_ISCONTAINED(src, ssize, src + ep - upx1 + valign, 4)) {
150     imports = dst + cli_readint32(src + ep - upx1 + valign);
     ...
159       while (CLI_ISCONTAINED(dst, *dsize,  pehdr, 8) && cli_readint32(pehdr)) {
160         pehdr+=8;
161         while(CLI_ISCONTAINED(dst, *dsize,  pehdr, 2) && *pehdr) {
162           pehdr++;
163           while (CLI_ISCONTAINED(dst, *dsize,  pehdr, 2) && *pehdr)
164             pehdr++;
     ...
212     /* Within bounds ? */
213     if (!CLI_ISCONTAINED(upx0, realstuffsz, urva, vsize)) {


static int doubleebx():

268     if (! CLI_ISCONTAINED(src, ssize, src+*scur, 4))
269       return -1;


int upx_inflate2b():

344     if (!CLI_ISCONTAINED(dst, *dsize, dst+dcur+unp_offset, backsize) 
344 || !CLI_ISCONTAINED(dst, *dsize, dst+dcur, backsize) || unp_offset >=0)


int upx_inflate2d():

426     if (!CLI_ISCONTAINED(dst, *dsize, dst+dcur+unp_offset, backsize) 
426 || !CLI_ISCONTAINED(dst, *dsize, dst+dcur, backsize) || unp_offset >=0 )


int upx_inflate2e():

515     if (!CLI_ISCONTAINED(dst, *dsize, dst+dcur+unp_offset, backsize) 
515 || !CLI_ISCONTAINED(dst, *dsize, dst+dcur, backsize) || unp_offset >=0 )


The original report was for UPack but these are used in so many places that definitely you could get something out of them if you spend some time. The patch was of course to add the missing parenthesis and provide valid results from now on like this:

 #define CLI_ISCONTAINED(bb, bb_size, sb, sb_size)      \
-    (bb_size > 0 && sb_size > 0 && (size_t)sb_size <= (size_t)bb_size  \
-     && sb >= bb && sb + sb_size <= bb + bb_size && sb + sb_size > bb)
+  ((bb_size) > 0 && (sb_size) > 0 && (size_t)(sb_size) <= (size_t)(bb_size) \
+   && (sb) >= (bb) && ((sb) + (sb_size)) <= ((bb) + (bb_size)) && ((sb) + (sb_size)) > (bb) && (sb) < ((bb) + (bb_size)))

 #define CLI_ISCONTAINED2(bb, bb_size, sb, sb_size)     \
-    (bb_size > 0 && sb_size >= 0 && (size_t)sb_size <= (size_t)bb_size \
-     && sb >= bb && sb + sb_size <= bb + bb_size && sb + sb_size >= bb)
+  ((bb_size) > 0 && (sb_size) >= 0 && (size_t)(sb_size) <= (size_t)(bb_size) \
+   && (sb) >= (bb) && ((sb) + (sb_size)) <= ((bb) + (bb_size)) && ((sb) + (sb_size)) >= (bb) && (sb) < ((bb) + (bb_size)))


Interesting that ClamAV did not publish some official security advisory for this bug. Of course, I might be wrong and missed this report. If anyone is aware of a public advisory provided by ClamAV please let me know.

Written by xorl

April 11, 2009 at 15:06

Posted in bugs

2 Responses

Subscribe to comments with RSS.

  1. The analysis is flawed in that “It doesn’t use parenthesis on them!” is not the actual problem here.
    Check the diff again…

    aCaB

    April 12, 2009 at 01:03

  2. Oh.. you are right. Thanks for pointing this out. They added this:
    (sb) < ((bb) + (bb_size))
    Which checks that the buffer2 is not larger than buffer1 + buffer1_size. My apologies to everyone for missing this. I usually do these posts quickly and it seems I skipped something really important on this one. :(

    xorl

    April 12, 2009 at 01:49


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s