xorl %eax, %eax

Archive for the ‘bugs’ Category

CVE-2013-3228: Linux kernel IrDA Information Leak

with 2 comments

This is another simple kernel memory information leak fixed by Mathias Krauss. Here is the exact code where this bug is located in net/irda/af_irda.c code.

/*
 * Function irda_recvmsg_dgram (iocb, sock, msg, size, flags)
 *
 *    Try to receive message and copy it to user. The frame is discarded
 *    after being read, regardless of how much the user actually read
 */
static int irda_recvmsg_dgram(struct kiocb *iocb, struct socket *sock,
			      struct msghdr *msg, size_t size, int flags)
{
	struct sock *sk = sock->sk;
	struct irda_sock *self = irda_sk(sk);
	struct sk_buff *skb;
	size_t copied;
	int err;

	IRDA_DEBUG(4, "%s()\n", __func__);

	skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
				flags & MSG_DONTWAIT, &err);
   ...
	skb_copy_datagram_iovec(skb, 0, msg->msg_iov, copied);

	skb_free_datagram(sk, skb);
   ...
	return copied;
}

This is a command which is defined as shown below.

static const struct proto_ops irda_seqpacket_ops = {
  ...
	.recvmsg =	irda_recvmsg_dgram,
  ...
};

static const struct proto_ops irda_dgram_ops = {
  ...
	.recvmsg =	irda_recvmsg_dgram,
  ...
};

#ifdef CONFIG_IRDA_ULTRA
static const struct proto_ops irda_ultra_ops = {
  ...
	.recvmsg =	irda_recvmsg_dgram,
  ...
};
#endif /* CONFIG_IRDA_ULTRA */

As Mathias Krausse pointed out, the ‘msg_namelen’ member of the ‘msghdr’ structure remains uninitialized resulting in kernel information leak. Below is how this structure is defined in include/linux/socket.h header file.

/*
 *      As we do 4.4BSD message passing we use a 4.4BSD message passing
 *      system, not 4.3. Thus msg_accrights(len) are now missing. They
 *      belong in an obscure libc emulation or the bin.
 */
  
struct msghdr {
        void    *       msg_name;       /* Socket name                  */
        int             msg_namelen;    /* Length of name               */
        struct iovec *  msg_iov;        /* Data blocks                  */
        __kernel_size_t msg_iovlen;     /* Number of blocks             */
        void    *       msg_control;    /* Per protocol magic (eg BSD file descriptor passing) */
       __kernel_size_t msg_controllen; /* Length of cmsg list */
       unsigned int    msg_flags;
};

And the fix was to add the missing initialization.

 	IRDA_DEBUG(4, "%s()\n", __func__);
 
+	msg->msg_namelen = 0;
+
 	skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
 				flags & MSG_DONTWAIT, &err);

Written by xorl

May 26, 2013 at 14:18

Posted in bugs, linux

CVE-2013-2007: QEMU Guest Agent Insecure File Permissions

leave a comment »

This vulnerability was reported by Laszlo Ersek of Red Hat and it allows guest privilege escalation when started in daemon mode. As he mentioned, QEMU guest agent creates files with incorrect file permissions. Specifically, at least the files that are affected are the ones created with ‘guest-file-open’ QMP command, shell output redirection, or when invoked by the fsfreeze script.

To overcome this he first updated the umask(2) of become_daemon() routine in qga/main.c as shown below.

     }
 
-    umask(0);
+    umask(S_IRWXG | S_IRWXO);
     sid = setsid();

The find_open_flag() and safe_open_or_create() functions were added in qga/commands-posix.c as you can see here.

+typedef const char * const ccpc;
+
+/* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html */
+static const struct {
+    ccpc *forms;
+    int oflag_base;
+} guest_file_open_modes[] = {
+    { (ccpc[]){ "r",  "rb",         NULL }, O_RDONLY                      },
+    { (ccpc[]){ "w",  "wb",         NULL }, O_WRONLY | O_CREAT | O_TRUNC  },
+    { (ccpc[]){ "a",  "ab",         NULL }, O_WRONLY | O_CREAT | O_APPEND },
+    { (ccpc[]){ "r+", "rb+", "r+b", NULL }, O_RDWR                        },
+    { (ccpc[]){ "w+", "wb+", "w+b", NULL }, O_RDWR   | O_CREAT | O_TRUNC  },
+    { (ccpc[]){ "a+", "ab+", "a+b", NULL }, O_RDWR   | O_CREAT | O_APPEND }
+};
+
+static int
+find_open_flag(const char *mode_str, Error **err)
+{
+    unsigned mode;
+
+    for (mode = 0; mode < ARRAY_SIZE(guest_file_open_modes); ++mode) {
+        ccpc *form;
+
+        form = guest_file_open_modes[mode].forms;
+        while (*form != NULL && strcmp(*form, mode_str) != 0) {
+            ++form;
+        }
+        if (*form != NULL) {
+            break;
+        }
+    }
+
+    if (mode == ARRAY_SIZE(guest_file_open_modes)) {
+        error_setg(err, "invalid file open mode '%s'", mode_str);
+        return -1;
+    }
+    return guest_file_open_modes[mode].oflag_base | O_NOCTTY | O_NONBLOCK;
+}
+
+#define DEFAULT_NEW_FILE_MODE (S_IRUSR | S_IWUSR | \
+                               S_IRGRP | S_IWGRP | \
+                               S_IROTH | S_IWOTH)
+
+static FILE *
+safe_open_or_create(const char *path, const char *mode, Error **err)
+{
+    Error *local_err = NULL;
+    int oflag;
+
+    oflag = find_open_flag(mode, &local_err);
+    if (local_err == NULL) {
+        int fd;
+
+        /* If the caller wants / allows creation of a new file, we implement it
+         * with a two step process: open() + (open() / fchmod()).
+         *
+         * First we insist on creating the file exclusively as a new file. If
+         * that succeeds, we're free to set any file-mode bits on it. (The
+         * motivation is that we want to set those file-mode bits independently
+         * of the current umask.)
+         *
+         * If the exclusive creation fails because the file already exists
+         * (EEXIST is not possible for any other reason), we just attempt to
+         * open the file, but in this case we won't be allowed to change the
+         * file-mode bits on the preexistent file.
+         *
+         * The pathname should never disappear between the two open()s in
+         * practice. If it happens, then someone very likely tried to race us.
+         * In this case just go ahead and report the ENOENT from the second
+         * open() to the caller.
+         *
+         * If the caller wants to open a preexistent file, then the first
+         * open() is decisive and its third argument is ignored, and the second
+         * open() and the fchmod() are never called.
+         */
+        fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
+        if (fd == -1 && errno == EEXIST) {
+            oflag &= ~(unsigned)O_CREAT;
+            fd = open(path, oflag);
+        }
+
+        if (fd == -1) {
+            error_setg_errno(&local_err, errno, "failed to open file '%s' "
+                             "(mode: '%s')", path, mode);
+        } else {
+            qemu_set_cloexec(fd);
+
+            if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
+                error_setg_errno(&local_err, errno, "failed to set permission "
+                                 "0%03o on new file '%s' (mode: '%s')",
+                                 (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
+            } else {
+                FILE *f;
+
+                f = fdopen(fd, mode);
+                if (f == NULL) {
+                    error_setg_errno(&local_err, errno, "failed to associate "
+                                     "stdio stream with file descriptor %d, "
+                                     "file '%s' (mode: '%s')", fd, path, mode);
+                } else {
+                    return f;
+                }
+            }
+
+            close(fd);
+        }
+    }
+
+    error_propagate(err, local_err);
+    return NULL;
+}
+
 int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
 {
     FILE *fh;
+    Error *local_err = NULL;
     int fd;
     int64_t ret = -1, handle;

And qmp_guest_file_open() was updated to use the newly added safe_open_or_create() routine shown above.

     slog("guest-file-open called, filepath: %s, mode: %s", path, mode);
-    fh = fopen(path, mode);
-    if (!fh) {
-        error_setg_errno(err, errno, "failed to open file '%s' (mode: '%s')",
-                         path, mode);
+    fh = safe_open_or_create(path, mode, &local_err);
+    if (local_err != NULL) {
+        error_propagate(err, local_err);
         return -1;
     }

Written by xorl

May 26, 2013 at 14:14

Posted in bugs

CVE-2013-1798: Linux kernel KVM IOAPIC_REG_SELECT Invalid Memory Access

leave a comment »

This was very nice vulnerability reported by Andrew Honig of Google. The bug is triggered when a user specifies an invalid IOAPIC_REG_SELECT value which is reachable via read KVM I/O device operation as you can see below.

static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
			    void *val)
{
	struct kvm_ioapic *ioapic = to_ioapic(this);
	u32 result;
   ...
	switch (addr) {
	case IOAPIC_REG_SELECT:
		result = ioapic->ioregsel;
		break;

	case IOAPIC_REG_WINDOW:
		result = ioapic_read_indirect(ioapic, addr, len);
		break;
   ...
	return 0;
}
   ...
static const struct kvm_io_device_ops ioapic_mmio_ops = {
	.read     = ioapic_mmio_read,
	.write    = ioapic_mmio_write,
};

Additionally, if a user makes a read by invoking IOAPIC_REG_WINDOW it will result in calling ioapic_read_indirect(). Here is what this function does.

static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
					  unsigned long addr,
					  unsigned long length)
{
	unsigned long result = 0;

	switch (ioapic->ioregsel) {
  ...
	default:
		{
			u32 redir_index = (ioapic->ioregsel - 0x10) >> 1;
			u64 redir_content;

			ASSERT(redir_index < IOAPIC_NUM_PINS);

			redir_content = ioapic->redirtbl[redir_index].bits;
			result = (ioapic->ioregsel & 0x1) ?
			    (redir_content >> 32) & 0xffffffff :
			    redir_content & 0xffffffff;
			break;
		}
	}

	return result;
}

It calculates and initializes the value of ‘redir_index’ from the user controlled ‘ioapic->ioregsel’ variable and then uses it as an index to ‘ioapic->redirtbl[]’ array. If this value is larger than IOAPIC_NUM_PINS it will result in invalid memory access. Here is how IOAPIC_NUM_PINS is defined in virt/kvm/ioapic.h header file.

#define IOAPIC_NUM_PINS  KVM_IOAPIC_NUM_PINS

And this is because it is architecture specific. For IA64 is defined in include/uapi/asm/kvm.h as 48 and for x86 in arch/x86/include/uapi/asm/kvm.h as 24. As you might have noticed there is an ASSERT() call to make this check but of course, this will only take effect in the debug builds.
The fix was to replace that ASSERT() call with a range check like this.

 			u32 redir_index = (ioapic->ioregsel - 0x10) >> 1;
 			u64 redir_content;
-			ASSERT(redir_index < IOAPIC_NUM_PINS);
+			if (redir_index < IOAPIC_NUM_PINS)
+				redir_content =
+					ioapic->redirtbl[redir_index].bits;
+			else
+				redir_content = ~0ULL;
-			redir_content = ioapic->redirtbl[redir_index].bits;
 			result = (ioapic->ioregsel & 0x1) ?
 			    (redir_content >> 32) & 0xffffffff :

Written by xorl

May 23, 2013 at 22:44

Posted in bugs, linux

CVE-2013-2074: KDE kdelibs Password Exposure

leave a comment »

This was a low impact vulnerability reported by m.wege. The issue occurs on “internal server error” and triggers the below code located at kioslave/http/http.cpp.

/**
 * This function will read in the return header from the server.  It will
 * not read in the body of the return message.  It will also not transmit
 * the header to our client as the client doesn't need to know the gory
 * details of HTTP headers.
 */
bool HTTPProtocol::readResponseHeader()
{
   ...
    if (m_request.responseCode >= 500 && m_request.responseCode <= 599) {
        // Server side errors

        if (m_request.method == HTTP_HEAD) {
   ...
            if (!sendErrorPageNotification()) {
                error(ERR_INTERNAL_SERVER, m_request.url.url());
                return false;
   ...
    } else if (!isAuthenticationRequired(m_request.responseCode) && m_request.responseCode >= 400 && m_request.responseCode <= 499) {
        // Any other client errors
        // Tell that we will only get an error page here.
        if (!sendErrorPageNotification()) {
            if (m_request.responseCode == 403)
                error(ERR_ACCESS_DENIED, m_request.url.url());
            else
                error(ERR_DOES_NOT_EXIST, m_request.url.url());
            return false;
        }
   ...
}

Clearly, error() could be called passing the error number along with a string which in all of the above cases is the result of m_request.url.url().

void HTTPProtocol::error( int _err, const QString &_text )
{
  // Close the connection only on connection errors. Otherwise, honor the
  // keep alive flag.
  if (_err == ERR_CONNECTION_BROKEN || _err == ERR_COULD_NOT_CONNECT)
      httpClose(false);
  else
      httpClose(m_request.isKeepAlive);

  if (!m_request.id.isEmpty())
  {
    forwardHttpResponseHeader();
    sendMetaData();
  }

  // It's over, we don't need it anymore
  clearPostDataBuffer();

  SlaveBase::error( _err, _text );
  m_iError = _err;
}

The url() routine is part of kdecore/io/kurl.cpp file and it is being to used to dump the URL.

QString KUrl::url( AdjustPathOption trailing ) const
{
  if (QString::compare(scheme(), QLatin1String("mailto"), Qt::CaseInsensitive) == 0) {
      // mailto urls should be prettified, see the url183433 testcase.
      return prettyUrl(trailing);
  }
  if ( trailing == AddTrailingSlash && !path().endsWith( QLatin1Char('/') ) ) {
      // -1 and 0 are provided by QUrl, but not +1, so that one is a bit tricky.
      // To avoid reimplementing toEncoded() all over again, I just use another QUrl
      // Let's hope this is fast, or not called often...
      QUrl newUrl( *this );
      newUrl.setPath( path() + QLatin1Char('/') );
      return QString::fromLatin1(newUrl.toEncoded());
  }
  else if ( trailing == RemoveTrailingSlash) {
      const QString cleanedPath = trailingSlash(trailing, path());
      if (cleanedPath == QLatin1String("/")) {
          if (path() != QLatin1String("/")) {
              QUrl fixedUrl = *this;
              fixedUrl.setPath(cleanedPath);
              return QLatin1String(fixedUrl.toEncoded(None));
          }
          return QLatin1String(toEncoded(None));
      }
  }
  return QString::fromLatin1(toEncoded(trailing == RemoveTrailingSlash ? StripTrailingSlash : None));
}

Of course, this means that the same also applies when the URL includes username and password information such as http://username:password@example.com. To fix this the calls to url() were replaced by prettyUrl() with the following patch.

diff --git a/kioslave/http/http.cpp b/kioslave/http/http.cpp
index 2d139a9..129fc7b 100644
--- a/kioslave/http/http.cpp
+++ b/kioslave/http/http.cpp
@@ -3056,7 +3056,7 @@ try_again:
             ; // Ignore error
         } else {
             if (!sendErrorPageNotification()) {
-                error(ERR_INTERNAL_SERVER, m_request.url.url());
+                error(ERR_INTERNAL_SERVER, m_request.url.prettyUrl());
                 return false;
             }
         }
@@ -3072,9 +3072,9 @@ try_again:
         // Tell that we will only get an error page here.
         if (!sendErrorPageNotification()) {
             if (m_request.responseCode == 403)
-                error(ERR_ACCESS_DENIED, m_request.url.url());
+                error(ERR_ACCESS_DENIED, m_request.url.prettyUrl());
             else
-                error(ERR_DOES_NOT_EXIST, m_request.url.url());
+                error(ERR_DOES_NOT_EXIST, m_request.url.prettyUrl());
             return false;
         }
     } else if (m_request.responseCode >= 301 && m_request.responseCode<= 303) {

This routine among other things removes the password from the URL.

QString KUrl::prettyUrl( AdjustPathOption trailing ) const
{
  // reconstruct the URL in a "pretty" form
  // a "pretty" URL is NOT suitable for data transfer. It's only for showing data to the user.
  // however, it must be parseable back to its original state, since
  // notably Konqueror displays it in the Location address.

  // A pretty URL is the same as a normal URL, except that:
  // - the password is removed
  // - the hostname is shown in Unicode (as opposed to ACE/Punycode)
  // - the pathname and fragment parts are shown in Unicode (as opposed to %-encoding)
  QString result = scheme();
    ...
}

Written by xorl

May 22, 2013 at 23:47

Posted in bugs

CVE-2013-1796: Linux kernel KVM MSR_KVM_SYSTEM_TIME Buffer Overflow

leave a comment »

This is a really nice vulnerability killed by Andy Honig. It is particularly interesting because it allows host kernel memory corruption through guest GPA (Guest Physical Address) manipulation. If we have a look in arch/x86/kvm/x86.c we can see the following code.

int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
{
	bool pr = false;
	u32 msr = msr_info->index;
	u64 data = msr_info->data;

	switch (msr) {   
   ...
	case MSR_KVM_SYSTEM_TIME: {
		kvmclock_reset(vcpu);

		vcpu->arch.time = data;
		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);

		/* we verify if the enable bit is set... */
		if (!(data & 1))
			break;

		/* ...but clean it before doing the actual write */
		vcpu->arch.time_offset = data & ~(PAGE_MASK | 1);

		vcpu->arch.time_page =
				gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT);

		if (is_error_page(vcpu->arch.time_page))
			vcpu->arch.time_page = NULL;

		break;
	}
   ...
	return 0;
}
EXPORT_SYMBOL_GPL(kvm_set_msr_common);

So by utilizing the ‘MSR_KVM_SYSTEM_TIME’ kvmclock MSR a user can set ‘vcpu->arch.time_page’ through gfn_to_page() call that uses the user derived ‘data’ information. As Andy Honig mentioned in his commit, the arbitrary write occurs when kmap atomic attempts to obtain a pointer to the time structure page and performing a memcpy() to it starting at the user controlled offset. The fix was to add a check that verifies that the provided value does not exceed the structure’s boundaries.

 		/* ...but clean it before doing the actual write */
 		vcpu->arch.time_offset = data & ~(PAGE_MASK | 1);
+		/* Check that the address is 32-byte aligned. */
+		if (vcpu->arch.time_offset &
+				(sizeof(struct pvclock_vcpu_time_info) - 1))
+			break;
+
 		vcpu->arch.time_page =
 				gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT);

Written by xorl

May 22, 2013 at 22:15

Posted in bugs, linux

Follow

Get every new post delivered to your Inbox.

Join 59 other followers