xorl %eax, %eax

CVE-2010-2530: NetBSD NetSMB Module Multiple Signedness Issues

with 8 comments

These bugs were discovered and reported by Dan Rosenberg on July 2010. The issue affects NetSMB module of the NetBSD operating system and the vulnerabilities can be found at src/sys/netsmb/smb_subr.c…

char *
smb_strdup(const char *s)
{
	char *p;
	int len;

	len = s ? strlen(s) + 1 : 1;
	p = malloc(len, M_SMBSTR, M_WAITOK);
	if (s)
		memcpy(p, s, len);
	else
		*p = 0;
	return p;
}

This is a simple strdup(3) implementation. It will check the string’s length and if it’s greater than zero it will set ‘len’ to ‘strlen(s) + 1’ otherwise it will initialize it to 1. Next, it will allocate the calculated space using malloc(3) and at last, if malloc(3) returned a valid pointer it will copy the user’s string ‘s’ to this area using memcpy(3) and return the pointer to this location.
The bug here is the use a signed integer to store string’s length which could be negative. To fix this the following patch was applied:

 	char *p;
-	int len;
+	size_t len;
 
 	len = s ? strlen(s) + 1 : 1;

The next one is this:

/*
 * duplicate string from a user space.
 */
char *
smb_strdupin(char *s, int maxlen)
{
	char *p, bt;
	int len = 0;

	for (p = s; ;p++) {
		if (copyin(p, &bt, 1))
			return NULL;
		len++;
		if (maxlen && len > maxlen)
			return NULL;
		if (bt == 0)
			break;
	}
	p = malloc(len, M_SMBSTR, M_WAITOK);
	copyin(s, p, len);
	return p;
}

This code is used to duplicate a string from user space. To do this, it enters a ‘for’ loop and uses copyin(9) kernel API routine to copy 1 Byte to ‘bt’. This loop continues and increments ‘len’ in order to obtain the string’s length. It will then check that ‘maxlen’ argument is non-zero and if it is greater or equal to ‘len’. Finally, it will allocate the required space using malloc(3) and use copyin(9) to copy the string to that location.
The vulnerability here is that both argument ‘maxlen’ and string’s length ‘len’ are signed integers which means that they can contain negative values that can result in heap memory corruption. To fix this the following patch was applied:

 char *
-smb_strdupin(char *s, int maxlen)
+smb_strdupin(char *s, size_t maxlen)
 {
 	char *p, bt;
-	int len = 0;
+	size_t len = 0;
 
 	for (p = s; ;p++) {

The next vulnerable function was smb_memdupin() also located in the same source code file like this:

/*
 * duplicate memory block from a user space.
 */
void *
smb_memdupin(void *umem, int len)
{
	char *p;

	if (len > 8 * 1024)
		return NULL;
	p = malloc(len, M_SMBSTR, M_WAITOK);
	if (copyin(umem, p, len) == 0)
		return p;
	free(p, M_SMBSTR);
	return NULL;
}

It’s a simple routine that duplicates a userland memory block (passed to it through ‘umem’ pointer) using malloc(3) and copyin(9) in a very straightforward manner. Once again, the argument ‘len’ which is used to determine the userland memory block’s size is a signed integer. Because of this it can contain negative values that could lead in kernel heap memory corruption.
The fix was:

 void *
-smb_memdupin(void *umem, int len)
+smb_memdupin(void *umem, size_t len)
 {
 	char *p;

The next one is…

#ifdef SMB_SOCKETDATA_DEBUG
void
m_dumpm(struct mbuf *m) {
	char *p;
	int len;
	printf("d=");
	while(m) {
		p = mtod(m,char *);
		len = m->m_len;
		printf("(%d)",len);
		while(len--){
			printf("%02x ",((int)*(p++)) & 0xff);
		}
		m=m->m_next;
	};
	printf("\n");
}
#endif

So, this is only enabled when ‘SMB_SOCKETDATA_DEBUG’ is defined. The bug here is that ‘len’ integer is a signed and it’s used to store the mbuf’s length which is decremented in order to print a complete dump of the mbuf’s range using printf(3). It will then continue to the next mbuf if until it reaches the final entry (NULL).
To fix this the patch was…

 m_dumpm(struct mbuf *m) {
 	char *p;
-	int len;
+	size_t len;
 	printf("d=");
 	while(m) {
-		p=mtod(m,char *);
-		len=m->m_len;
-		printf("(%d)",len);
+		p = mtod(m,char *);
+		len = m->m_len;
+		printf("(%zu)", len);
 		while(len--){
 			printf("%02x ",((int)*(p++)) & 0xff);
 		}

Which replaces signed integer ‘len’ with a ‘size_t’ one and also replaces the ‘%d’ format string specifier with ‘%zu’ since ‘len’ is now unsigned.
The final bug was present in smb_put_dmem() routine.

int
smb_put_dmem(struct mbchain *mbp, struct smb_vc *vcp, const char *src,
	int size, int caseopt)
{
	struct iconv_drv *dp = vcp->vc_toserver;

	if (size == 0)
		return 0;
	if (dp == NULL) {
		return mb_put_mem(mbp, (const void *)src, size, MB_MSYSTEM);
	}
	mbp->mb_copy = smb_copy_iconv;
	mbp->mb_udata = dp;
	return mb_put_mem(mbp, (const void *)src, size, MB_MCUSTOM);
}

Here, the ‘size’ argument is used in mb_put_mem() to copy (using internally bcopy(3)) ‘size’ number of Bytes from the ‘src’ to ‘mbp’ and if ‘dp’ (pointing to the VCP local charset to server one) isn’t NULL, it’ll update some ‘mbp’ structure’s members (specifically ‘mb_copy’ which is a user defined copy function and ‘mb_udata’ which is a pointer to the user’s data) and use a custom option to copy the specified data again using mb_put_mem().
Of course, the bug is that ‘size’ is a signed integer and it could contain negative values that will lead to kernel memory corruption. The patch to fix this vulnerability was:

 int
 smb_put_dmem(struct mbchain *mbp, struct smb_vc *vcp, const char *src,
-	int size, int caseopt)
+	size_t size, int caseopt)
 {
 	struct iconv_drv *dp = vcp->vc_toserver;

Written by xorl

September 25, 2010 at 06:08

Posted in bugs, netbsd

8 Responses

Subscribe to comments with RSS.

  1. > len = s ? strlen(s) + 1 : 1;

    For len to be negative, strlen(s) have to be greater than 2^31 right ?

    John Creebet

    September 25, 2010 at 07:42

  2. Yeap, that’s why Dan Rosenberg said those were mainly minor issues.

    xorl

    September 25, 2010 at 07:53

  3. Nice blog. In these cases, no memory corruption is possible – the issue is that BSD kernels panic when a request to malloc() with M_WAITOK is made for a large amount of memory that could not possibly be allocated (making it a local DoS). The smb_memdupin() function was easily triggerable via a certain ioctl – I’m not so sure about the other allocation functions relying on strlen() though.

    Dan Rosenberg

    September 27, 2010 at 19:22

  4. These warez are FAKED and TROJANIZED

    troller

    September 29, 2010 at 16:13

  5. great blog.

    in the case of smb_strdupin, wouldn’t maxlen=0 and a non-null terminated p string cause this routine to read past the end of the string, regardless of the fix?

    pvppyk1tten

    October 2, 2010 at 00:09

  6. @ pvppyk1tten: First of all, a string is one or more Bytes terminated by a NULL Byte. So, you cannot have a non-null terminated ‘p’ string.
    Now, from a quick look I had I wasn’t able to find any place where the user controls the ‘maxlen’ value.
    Have a look: http://fxr.watson.org/fxr/source/netsmb/smb_usr.c?v=NETBSD5#L310

    xorl

    October 4, 2010 at 14:48

  7. if maxlen would not be user controlled, I wonder why there would be a problem with it being negative in the first place.

    i don’t see your point about a string always being null terminated though. if i allocate 10 bytes for a string and put 0xff in all of them, i have a non-null terminated string, when i call e.g. strlen() – or this function – on it, it will read past the ten bytes continuing until the first null byte in memory. that’s what i was talking about.

    either way, thanks for the insight.

    pvppyk1tten

    October 4, 2010 at 17:44

  8. That’s exactly what I meant. A string must be NULL terminated. You said “until the first null byte”, so it is NULL terminated. That’s what I meant.
    Those bugs aren’t all exploitable. Check out the above Dan Rosenberg’s comment: https://xorl.wordpress.com/2010/09/25/cve-2010-2530-netbsd-netsmb-module-multiple-signedness-issues/#comment-998

    xorl

    October 4, 2010 at 17:48


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