xorl %eax, %eax

Linux kernel SunRPC off-by-two Overwrite (testing kernels only)

with 2 comments

While reading 2.6.32-rc8’s ChangeLog file, I came across this nice little bug. This is an unimportant vulnerability since it didn’t made it to the stable (thanks to argp for that @*)&%@(*%!) kernels.
Now, since there aren’t many kernel level vulnerabilities being disclosed recently (which as I have said before, is a *really* good thing), I blog about this one. To clarify, this affects only testing kernels, there is nothing important for real world exploit developers here.
Now, let’s move to the bug…
On 5 November 2009, Patroklos Argyroudis (aka argp) made a bug report for that issue that was affecting 2.6.32-rc6. If we have a look at net/sunrpc/addr.c we’ll see the following…

/**
 * rpc_uaddr2sockaddr - convert a universal address to a socket address.
 * @uaddr: C string containing universal address to convert
 * @uaddr_len: length of universal address string
 * @sap: buffer into which to plant socket address
 * @salen: size of buffer
 *
 * Returns the size of the socket address if successful; otherwise
 * zero is returned.
 */
size_t rpc_uaddr2sockaddr(const char *uaddr, const size_t uaddr_len,
			  struct sockaddr *sap, const size_t salen)
{
	char *c, buf[RPCBIND_MAXUADDRLEN];
	unsigned long portlo, porthi;
	unsigned short port;

	if (uaddr_len > sizeof(buf))
		return 0;

	memcpy(buf, uaddr, uaddr_len);

	buf[uaddr_len] = '\n';
	buf[uaddr_len + 1] = '\0';
    ...
	return 0;
}
EXPORT_SYMBOL_GPL(rpc_uaddr2sockaddr);

This routine is used to to covert an RPC universal address (uaddr) to its equivalent socket address as it is implied by its name. Its second argument, ‘uaddr_len’ is used to store the length of the universal address and as you can see in the code, if this is greater than the size of ‘buf[]’ it will immediately return. What argp noticed is that in case of a ‘uaddr_len’ equal to ‘RPCBIND_MAXUADDRLEN’, it will bypass the ‘if’ check and lead to an off-by-two overwrite during the ‘buf[]’ termination using “\n and NULL that wordpress removes :P”.
To fix this, argp proposed the following patch:

 	unsigned short port;
 
-	if (uaddr_len > sizeof(buf))
+	if (uaddr_len > sizeof(buf) - 2)
 		return 0;

But the final patch that was used was quite different. Fábio Olivé Leite pointed out that ‘strict_strtoul()’ requires either “\n” or a simple NULL termination. Since this would make things simpler, they updated the above function to perform a simple NULL termination like that:

  * @sap: buffer into which to plant socket address
  * @salen: size of buffer
  *
+ * @uaddr does not have to be '\0'-terminated, but strict_strtoul() and
+ * rpc_pton() require proper string termination to be successful.
+ *
  * Returns the size of the socket address if successful; otherwise
  * zero is returned.
  */
 size_t rpc_uaddr2sockaddr(const char *uaddr, const size_t uaddr_len,
 			  struct sockaddr *sap, const size_t salen)
 {
-	char *c, buf[RPCBIND_MAXUADDRLEN];
+	char *c, buf[RPCBIND_MAXUADDRLEN + sizeof('\0')];
 	unsigned long portlo, porthi;
 	unsigned short port;

-	if (uaddr_len > sizeof(buf))
+	if (uaddr_len > RPCBIND_MAXUADDRLEN)
 		return 0;

 	memcpy(buf, uaddr, uaddr_len);

-	buf[uaddr_len] = '\n';
-	buf[uaddr_len + 1] = '\0';
-
+	buf[uaddr_len] = '\0';
 	c = strrchr(buf, '.');

The buffer allocates the appropriate size, the ‘if’ check uses the buffer’s size not including the additional NULL termination and the write operations are reduced to just a simple NULL termination.
In the same function, the patch continues like this:

@@ -332,9 +333,7 @@ size_t rpc_uaddr2sockaddr(const char *uaddr, const size_t uaddr_len,
 	if (unlikely(portlo > 255))
 		return 0;

-	c[0] = '\n';
-	c[1] = '\0';
-
+	*c = '\0';
 	c = strrchr(buf, '.');
 	if (unlikely(c == NULL))
 		return 0;
@@ -345,8 +344,7 @@ size_t rpc_uaddr2sockaddr(const char *uaddr, const size_t uaddr_len,

 	port = (unsigned short)((porthi << 8) | portlo);

-	c[0] = '\0';
-
+	*c = '\0';
 	if (rpc_pton(buf, strlen(buf), sap, salen) == 0)

Which is more or less, the same procedure. Too bad that this nice little bug was killed before even reaching out the stable releases…

Written by xorl

November 26, 2009 at 23:33

Posted in bugs, linux

2 Responses

Subscribe to comments with RSS.

  1. Honestly, you should revise your ‘about page’, because with a statement like this ‘Too bad that this nice little bug was killed before even reaching out the stable releases…’ you clearly aren’t interested in Security, you’re interested in finding bugs and learning about the various smart ways to exploit/use them. Anyways, your blog post is still cool.

    – MrS

    Mr Snuggles

    November 29, 2009 at 00:50

  2. In my opinion, finding and exploiting bugs is “security”. Also, I think that what I’m doing here is useful since new people to kernel and user space exploitation do not have the opportunity to “see” bugs and bug classes unless then analyze those bugs on their own which isn’t always easy for someone who has just started learning such stuff.
    Unfortunately, most vendors release totally cryptic advisories with no information and a few years ago, when I was starting with these stuff I was wishing that there would be someone to just write a complete advisory (like tk for example which you can see at http://www.trapkit.de/advisories/published.html).
    On the other hand, as I said in my “about” page, I’m not a supporter of full-disclosure but my first statement has nothing to do with this.
    This is why, even though I think that this bug should be “documented” cause it could be useful for some people, I don’t believe that it was wise to kill it.
    That’s also the reason why I’m not going into exploitation unless there is already a public exploit being released.

    xorl

    November 29, 2009 at 13: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