xorl %eax, %eax

Linux kernel airo_get_encode() Buffer Overflow

with 5 comments

This issue affects Linux kernel 2.6.30 prior to 2.6.30-rc6 (just kidding, it affects that one as well) where this was patched (or at least attempted). Credits go to John Linville and Eugene Teo. The bug can be found at drivers/net/wireless/airo.c where the Aironet driver for 4500 and 4800 series cards is located. Here is the vulnerable code from 2.6.30-rc5 release of the Linux kernel:

6460 /*
6461  * Wireless Handler : get Encryption Key
6462  */
6463 static int airo_get_encode(struct net_device *dev,
6464                            struct iw_request_info *info,
6465                            struct iw_point *dwrq,
6466                            char *extra)
6467 {
6468         struct airo_info *local = dev->ml_priv;
6469         int index = (dwrq->flags & IW_ENCODE_INDEX) - 1;
6470         u8 buf[16];
        ...
6477         /* Check encryption mode */
6478         switch(local->config.authType)  {
        ...
6502         /* Copy the key to the user buffer */
6503         dwrq->length = get_wep_key(local, index, &buf[0], sizeof(buf));
6504         memcpy(extra, buf, dwrq->length);
6505
6506         return 0;
6507 }

This function is used to obtain the encryption key (if there is any). At line 6503 it invokes get_wep_key() to retrieve the WEP key and store its retrurn value in dwrq->length. dwrq is a iw_point pointer to structure which is defined in include/linux/wireless.h like this:

679 /*
680  *      For all data larger than 16 octets, we need to use a
681  *      pointer to memory allocated in user space.
682  */
683 struct  iw_point
684 {
685   void __user   *pointer;       /* Pointer to the data  (in user space) */
686   __u16         length;         /* number of fields or size in bytes */
687   __u16         flags;          /* Optional params */
688 };

And get_wep_key() is a utility function located at drivers/net/wireless/airo.c and used to retrieve the WEP key. Here is a snippet:

5198 /* Returns the WEP key at the specified index, or -1 if that key does
5199  * not exist.  The buffer is assumed to be at least 16 bytes in length.
5200  */
5201 static int get_wep_key(struct airo_info *ai, u16 index, char *buf, u16 buflen)
5202 {
       ...
5207         rc = readWepKeyRid(ai, &wkr, 1, 1);
5208         if (rc != SUCCESS)
5209                 return -1;
       ...
5217                 rc = readWepKeyRid(ai, &wkr, 0, 1);
5218                 if (rc != SUCCESS)
5219                         return -1;
       ...
5221         return -1;
5222 }

As you can see, there are at least three cases in which this function will return -1 and all of them are not that hard to trigger. If this function returns -1 for any of the above reasons, airo_get_encode() will attempt to copy 0xffff bytes to buffer extra since length is of type __u16. However, buf is only 16 bytes long (check out line 6470). To fix this buffer overflow, the following patch was applied:

     dwrq->length = get_wep_key(local, index, &buf[0], sizeof(buf));
-    memcpy(extra, buf, dwrq->length);
+    if (dwrq->length != -1)
+        memcpy(extra, buf, dwrq->length);
+    else
+        dwrq->length = 0;

This performs an additional check for -1 in the length parameter… Well, I was just laughing when I saw this. First of all, dwrq->length is an unsigned value, thus it could never contain a negative number, secondly, dwrq->length is just 16 bits long, meaning that even if we assign -1 to it, it will truncate it to 0xffff which is 65535. I believe that this is not an important bug that’s why I’m leaking this information. It was hilarious! Even GCC says it:

sh-3.1$ cat -n malakies_tou_linux.c
     1  #include <stdio.h>
     2  typedef unsigned short __u16;
     3
     4  int
     5  main(void)
     6  {
     7     __u16 length = -1;
     8     if (length != -1)
     9       printf("length (%u) != -1\n", length);
    10     else
    11       printf("length (%u) == -1\n", length);
    12
    13     return 0;
    14  }
    15
sh-3.1$ gcc malakies_tou_linux.c -o malakies_tou_linux
malakies_tou_linux.c: In function ‘main’:
malakies_tou_linux.c:8: warning: comparison is always true due to limited range of data type
sh-3.1$ ./malakies_tou_linux
length (65535) != -1
sh-3.1$

They did the same thing to airo_get_encodeext() as well, it was:

6606 /*
6607  * Wireless Handler : get extended Encryption parameters
6608  */
6609 static int airo_get_encodeext(struct net_device *dev,
6610                             struct iw_request_info *info,
6611                             union iwreq_data *wrqu,
6612                             char *extra)
6613 {
6614         struct airo_info *local = dev->ml_priv;
6615         struct iw_point *encoding = &wrqu->encoding;
6616         struct iw_encode_ext *ext = (struct iw_encode_ext *)extra;
6617         int idx, max_key_len;
6618         u8 buf[16];
         ...
6660         /* Copy the key to the user buffer */
6661         ext->key_len = get_wep_key(local, idx, &buf[0], sizeof(buf));
6662         memcpy(extra, buf, ext->key_len);
6663
6664         return 0;
6665 }

And they patched it to this:

     ext->key_len = get_wep_key(local, idx, &buf[0], sizeof(buf));
-    memcpy(extra, buf, ext->key_len);
+    if (ext->key_len != -1)
+        memcpy(extra, buf, ext->key_len);
+    else
+        ext->key_len = 0;

     return 0;


Again, I’m still a supporter of non-disclosure but this was so funny that it was deserving a post. Many bugs don’t get patched properly and since this was an unimportant one (at least for me), I made this post.

Clarification, this is lul disclosure :P

Written by xorl

May 19, 2009 at 15:55

Posted in bugs, fun, linux

5 Responses

Subscribe to comments with RSS.

  1. lul disclosure FTW!

    thanasisk

    May 19, 2009 at 17:43

  2. This only happens because of the strange way gcc does type promotion; gcc always extends values to 32 bits, so for values smaller than 32 bits, any comparison between a negative and an unsigned will always be false. However, if they’re already 32 bits, gcc just goes ahead and compares them without regard to signedness. If dwrq->length had been an int instead of a short, gcc would emit what the programmer expects with no warnings and it would work (despite the fact that it’s still logically wrong).

    egypt

    May 20, 2009 at 00:43

  3. egypt: You’re right but using -Wextra option GCC checks for that case as well and informs you with a message similar to:
    warning: comparison between signed and unsigned
    My bad for not mentioning this from the first place.

    xorl

    May 20, 2009 at 11:14

  4. Ha! Oh well, stuff happens… Thanks for finding this!

    John W. Linville

    May 20, 2009 at 14:49

  5. You’re welcome Mr. John W. Linville. :)

    xorl

    May 20, 2009 at 15:24


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