xorl %eax, %eax

CVE-2009-1374: Pidgin QQ Protocol Buffer Overflow

leave a comment »

This bug was discovered by Ka-Hing Cheung and affects pidgin prior to 2.5.6 release. QQ is a protocol for instant messaging in China. Anyway, here is the vulnerable function from 2.5.5 release of pidgin:

25  * QQ encryption algorithm
26  * Convert from ASM code provided by PerlOICQ
27  *
28  * Puzzlebird, Nov-Dec 2002
       ...
247 static inline gint decrypt_out(guint8 *dest, gint crypted_len, const guint8* const key)
248 {
249         gint plain_len;
250         guint32 key32[4];
251         guint32 crypted32[2];
252         guint32 c32_prev[2];
253         guint32 plain32[2];
254         guint32 p32_prev[2];
255         gint count64;
256         gint padding;
257         guint8 *crypted_ptr = dest;
       ...
277         count64 = crypted_len / 8;
278         while (count64-- > 0){
279                 c32_prev[0] = crypted32[0]; c32_prev[1] = crypted32[1];
280                 crypted_ptr += 8;
281
282                 memcpy(crypted32, crypted_ptr, sizeof(crypted32));
283                 p32_prev[0] ^= crypted32[0]; p32_prev[1] ^= crypted32[1];
       ...
291         return plain_len;
292 }


As you can see in the above routine, the encrypted length passed to it as an argument (crypted_len) is divided by 8 at line 277 and stored in count64. Now, count64 is used as a counter in a while loop (line 278) to iterate count64 times and copy 8 bytes at each iteration (lines 279-283). The problem with the above loop is that it will iterate one additional time since it is post-incremented. To fix this the following patch was applied:

     count64 = crypted_len / 8;
-    while (count64-- > 0){
+    while (--count64 > 0){
         c32_prev[0] = crypted32[0]; c32_prev[1] = crypted32[1];

Pidgin developers believe that this cannot lead to anything more than a DoS. In my humble opinion, ANY memory corruption bug must be considered as  high risk vulnerability that can lead to code execution (even if you are not really sure how).
In addition to this, another patch was committed to include a missing counter check to the equivalent encoding routine at libpurple/protocols/qq/qq_crypt.c which was similar to this:

128 /* 64-bit blocks and some kind of feedback mode of operation */
129 static inline void encrypt_out(guint8 *crypted, const gint crypted_len, const guint8 *key)
130 {
131         /* ships in encipher */
132         guint32 plain32[2];
133         guint32 p32_prev[2];
134         guint32 key32[4];
135         guint32 crypted32[2];
136         guint32 c32_prev[2];
137
138         guint8 *crypted_ptr;
139         gint count64;
       ...
151         count64 = crypted_len / 8;
152         while (count64-- > 0){
153                 /* encrypt it */
       ...
165                 /* set next 64 bits want to crypt*/
166                 crypted_ptr += 8;
167                 memcpy(crypted32, crypted_ptr, sizeof(crypted32));
168                 plain32[0] = crypted32[0] ^ c32_prev[0]; plain32[1] = crypted32[1] ^ c32_prev[1];
169         }
170 }

And since 2.5.6 release the previous copy operation is changed to this:

         /* set next 64 bits want to crypt*/
-        crypted_ptr += 8;
-        memcpy(crypted32, crypted_ptr, sizeof(crypted32));
-        plain32[0] = crypted32[0] ^ c32_prev[0]; plain32[1] = crypted32[1] ^ c32_prev[1];
+        if (count64 > 0) {
+            crypted_ptr += 8;
+            memcpy(crypted32, crypted_ptr, sizeof(crypted32));
+            plain32[0] = crypted32[0] ^ c32_prev[0]; plain32[1] = crypted32[1] ^ c32_prev[1];
+        }
     }

Written by xorl

May 27, 2009 at 18:38

Posted in bugs

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