xorl %eax, %eax

CVE-2010-0624: GNU tar(1) & cpio(1) Heap Buffer Overflow

with one comment

I saw this bug today and even though the initial advisory is very detailed I’ll write a blog post too.
The bug was discovered by Jakob Lell and it affects GNU tar(1) prior to 1.23 release and GNU cpio(1) prior to 2.11. You can find the bug at lib/rtapelib.c of GNU tar(1) or GNU cpio(1)’s source code in the function below..

/* Read up to LENGTH bytes into BUFFER from remote tape connection HANDLE.
   Return the number of bytes read on success, SAFE_READ_ERROR on error.  */
rmt_read__ (int handle, char *buffer, size_t length)
  char command_buffer[COMMAND_BUFFER_SIZE];
  size_t status;
  size_t rlen;
  size_t counter;

  sprintf (command_buffer, "R%lu\n", (unsigned long) length);
  if (do_command (handle, command_buffer) == -1
      || (status = get_status (handle)) == SAFE_READ_ERROR)
    return SAFE_READ_ERROR;

  for (counter = 0; counter < status; counter += rlen, buffer += rlen)
      rlen = safe_read (READ_SIDE (handle), buffer, status - counter);
      if (rlen == SAFE_READ_ERROR || rlen == 0)
          _rmt_shutdown (handle, EIO);
          return SAFE_READ_ERROR;

  return status;

This is the reading routine when communicating with a remote tape drive. As you can read, it creates a string stored in ‘command_buffer’ that contains the ‘length’ that represents the number of Bytes to read. It sends that value to the remote tape server using do_command() and checks its return value for errors and retrieves the server’s status using get_status() function.
Next, as you can see it enters a ‘for’ loop that will iterate ‘states’ times. Since there is no check, if the ‘status’ iterates more times than the heap allocated buffer ‘buffer’, it will result in heap memory corruption.
To fix this, the initial ‘if’ clause that was calling get_status() was changed to include a check on the length like this:

   if (do_command (handle, command_buffer) == -1
-      || (status = get_status (handle)) == SAFE_READ_ERROR)
+      || (status = get_status (handle)) == SAFE_READ_ERROR
+      || status > length)
     return SAFE_READ_ERROR;

There was also another patch committed to avoid a similar overflow but as far as I know there is no CVE assigned for it. It’s located in rmt_ioctl__() routine which is shown below…

/* Perform a raw tape operation on remote tape connection HANDLE.
   Return the results of the ioctl, or -1 on error.  */
rmt_ioctl__ (int handle, int operation, char *argument)
  switch (operation)
    case MTIOCGET:
        ssize_t status;
        size_t counter;

        /* Grab the status and read it directly into the structure.  This
           assumes that the status buffer is not padded and that 2 shorts
           fit in a long without any word alignment problems; i.e., the
           whole struct is contiguous.  NOTE - this is probably NOT a good
           assumption.  */

        if (do_command (handle, "S") == -1
            || (status = get_status (handle), status == -1))
          return -1;

        for (; status > 0; status -= counter, argument += counter)
            counter = safe_read (READ_SIDE (handle), argument, status);
            if (counter == SAFE_READ_ERROR || counter == 0)
                _rmt_shutdown (handle, EIO);
                return -1;
#endif /* MTIOCGET */


Where you can easily see that it’s similar to the previous bug and it was fixed by adding this:

+	if (status > sizeof (struct mtop))
+	  {
+	    errno = EOVERFLOW;
+	    return -1;
+	  }
 	for (; status > 0; status -= counter, argument += counter)

That prevents ‘status’ values greater than the size of the expected structure.

Written by xorl

October 27, 2010 at 19:18

Posted in bugs

One Response

Subscribe to comments with RSS.

  1. Oh no! Attack of the malicious tape drives!


    October 27, 2010 at 23:04

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