Attachment 5984 Details for Bug 7698 – git-am fix for 3.5.next

[patch] git-am fix for 3.5.next

0001-Fix-bug-7698-Assert-causes-smbd-to-panic-on-invalid-.patch (text/plain), 12.65 KB, created by Jeremy Allison on 2010-09-26 06:51:33 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2010-09-26 06:51:33 UTC

Size: 12.65 KB

patch

obsolete

>From 891bad006072b0444baba66682be1148a310a967 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Sun, 26 Sep 2010 04:49:29 -0700
>Subject: [PATCH] Fix bug #7698 - Assert causes smbd to panic on invalid NetBIOS session request.
>
>Found by the CodeNomicon test suites at the SNIA plugfest.
>
>http://www.codenomicon.com/
>
>If an invalid NetBIOS session request is received the code in name_len() in
>libsmb/nmblib.c can hit an assert.
>
>Re-write name_len() and name_extract() to use "buf/len" pairs and
>always limit reads.
>
>(Modified for 3.5.x)
>
>Jeremy.
>---
> source3/include/proto.h     |    6 +-
> source3/libsmb/cliconnect.c |   15 +++++--
> source3/libsmb/nmblib.c     |   86 ++++++++++++++++++++++++++++--------------
> source3/smbd/process.c      |    2 +-
> source3/smbd/reply.c        |   41 +++++++++++++++-----
> source3/utils/smbfilter.c   |   41 ++++++++++++++++----
> 6 files changed, 134 insertions(+), 57 deletions(-)
>
>diff --git a/source3/include/proto.h b/source3/include/proto.h
>index 483fd84..5064fdb 100644
>--- a/source3/include/proto.h
>+++ b/source3/include/proto.h
>@@ -3191,8 +3191,8 @@ bool match_mailslot_name(struct packet_struct *p, const char *mailslot_name);
> int matching_len_bits(unsigned char *p1, unsigned char *p2, size_t len);
> void sort_query_replies(char *data, int n, struct in_addr ip);
> char *name_mangle(TALLOC_CTX *mem_ctx, char *In, char name_type);
>-int name_extract(char *buf,int ofs, fstring name);
>-int name_len(char *s1);
>+int name_extract(unsigned char *buf,size_t buf_len, unsigned int ofs, fstring name);
>+int name_len(unsigned char *s1, size_t buf_len);
> 
> /* The following definitions come from libsmb/nterr.c  */
> 
>@@ -6859,7 +6859,7 @@ bool check_fsp_ntquota_handle(connection_struct *conn, struct smb_request *req,
> 			      files_struct *fsp);
> bool fsp_belongs_conn(connection_struct *conn, struct smb_request *req,
> 		      files_struct *fsp);
>-void reply_special(char *inbuf);
>+void reply_special(char *inbuf, size_t inbuf_len);
> void reply_tcon(struct smb_request *req);
> void reply_tcon_and_X(struct smb_request *req);
> void reply_unknown_new(struct smb_request *req, uint8 type);
>diff --git a/source3/libsmb/cliconnect.c b/source3/libsmb/cliconnect.c
>index 22be999..a3febde 100644
>--- a/source3/libsmb/cliconnect.c
>+++ b/source3/libsmb/cliconnect.c
>@@ -1872,6 +1872,7 @@ bool cli_session_request(struct cli_state *cli,
> {
> 	char *p;
> 	int len = 4;
>+	int namelen = 0;
> 	char *tmp;
> 
> 	/* 445 doesn't have session request */
>@@ -1890,8 +1891,11 @@ bool cli_session_request(struct cli_state *cli,
> 	}
> 
> 	p = cli->outbuf+len;
>-	memcpy(p, tmp, name_len(tmp));
>-	len += name_len(tmp);
>+	namelen = name_len((unsigned char *)tmp, talloc_get_size(tmp));
>+	if (namelen > 0) {
>+		memcpy(p, tmp, namelen);
>+		len += namelen;
>+	}
> 	TALLOC_FREE(tmp);
> 
> 	/* and my name */
>@@ -1903,8 +1907,11 @@ bool cli_session_request(struct cli_state *cli,
> 	}
> 
> 	p = cli->outbuf+len;
>-	memcpy(p, tmp, name_len(tmp));
>-	len += name_len(tmp);
>+	namelen = name_len((unsigned char *)tmp, talloc_get_size(tmp));
>+	if (namelen > 0) {
>+		memcpy(p, tmp, namelen);
>+		len += namelen;
>+	}
> 	TALLOC_FREE(tmp);
> 
> 	/* send a session request (RFC 1002) */
>diff --git a/source3/libsmb/nmblib.c b/source3/libsmb/nmblib.c
>index 1a21066..cf7023a 100644
>--- a/source3/libsmb/nmblib.c
>+++ b/source3/libsmb/nmblib.c
>@@ -1237,21 +1237,33 @@ void sort_query_replies(char *data, int n, struct in_addr ip)
> 
> /****************************************************************************
>  Interpret the weird netbios "name" into a unix fstring. Return the name type.
>+ Returns -1 on error.
> ****************************************************************************/
> 
>-static int name_interpret(char *in, fstring name)
>+static int name_interpret(unsigned char *buf, size_t buf_len,
>+		unsigned char *in, fstring name)
> {
>+	unsigned char *end_ptr = buf + buf_len;
> 	int ret;
>-	int len = (*in++) / 2;
>+	unsigned int len;
> 	fstring out_string;
>-	char *out = out_string;
>+	unsigned char *out = (unsigned char *)out_string;
> 
> 	*out=0;
> 
>-	if (len > 30 || len<1)
>-		return(0);
>+	if (in >= end_ptr) {
>+		return -1;
>+	}
>+	len = (*in++) / 2;
>+
>+	if (len<1) {
>+		return -1;
>+	}
> 
> 	while (len--) {
>+		if (&in[1] >= end_ptr) {
>+			return -1;
>+		}
> 		if (in[0] < 'A' || in[0] > 'P' || in[1] < 'A' || in[1] > 'P') {
> 			*out = 0;
> 			return(0);
>@@ -1259,21 +1271,13 @@ static int name_interpret(char *in, fstring name)
> 		*out = ((in[0]-'A')<<4) + (in[1]-'A');
> 		in += 2;
> 		out++;
>+		if (PTR_DIFF(out,out_string) >= sizeof(fstring)) {
>+			return -1;
>+		}
> 	}
> 	ret = out[-1];
> 	out[-1] = 0;
> 
>-#ifdef NETBIOS_SCOPE
>-	/* Handle any scope names */
>-	while(*in) {
>-		*out++ = '.'; /* Scope names are separated by periods */
>-		len = *(unsigned char *)in++;
>-		StrnCpy(out, in, len);
>-		out += len;
>-		*out=0;
>-		in += len;
>-	}
>-#endif
> 	pull_ascii_fstring(name, out_string);
> 
> 	return(ret);
>@@ -1352,12 +1356,25 @@ char *name_mangle(TALLOC_CTX *mem_ctx, char *In, char name_type)
>  Find a pointer to a netbios name.
> ****************************************************************************/
> 
>-static char *name_ptr(char *buf,int ofs)
>+static unsigned char *name_ptr(unsigned char *buf, size_t buf_len, unsigned int ofs)
> {
>-	unsigned char c = *(unsigned char *)(buf+ofs);
>+	unsigned char c = 0;
>+
>+	if (ofs > buf_len || buf_len < 1) {
>+		return NULL;
>+	}
> 
>+	c = *(unsigned char *)(buf+ofs);
> 	if ((c & 0xC0) == 0xC0) {
>-		uint16 l = RSVAL(buf, ofs) & 0x3FFF;
>+		uint16 l = 0;
>+
>+		if (ofs > buf_len - 1) {
>+			return NULL;
>+		}
>+		l = RSVAL(buf, ofs) & 0x3FFF;
>+		if (l > buf_len) {
>+			return NULL;
>+		}
> 		DEBUG(5,("name ptr to pos %d from %d is %s\n",l,ofs,buf+l));
> 		return(buf + l);
> 	} else {
>@@ -1367,37 +1384,48 @@ static char *name_ptr(char *buf,int ofs)
> 
> /****************************************************************************
>  Extract a netbios name from a buf (into a unix string) return name type.
>+ Returns -1 on error.
> ****************************************************************************/
> 
>-int name_extract(char *buf,int ofs, fstring name)
>+int name_extract(unsigned char *buf, size_t buf_len, unsigned int ofs, fstring name)
> {
>-	char *p = name_ptr(buf,ofs);
>-	int d = PTR_DIFF(p,buf+ofs);
>+	unsigned char *p = name_ptr(buf,buf_len,ofs);
> 
> 	name[0] = '\0';
>-	if (d < -50 || d > 50)
>-		return(0);
>-	return(name_interpret(p,name));
>+	if (p == NULL) {
>+		return -1;
>+	}
>+	return(name_interpret(buf,buf_len,p,name));
> }
> 
> /****************************************************************************
>  Return the total storage length of a mangled name.
>+ Returns -1 on error.
> ****************************************************************************/
> 
>-int name_len(char *s1)
>+int name_len(unsigned char *s1, size_t buf_len)
> {
> 	/* NOTE: this argument _must_ be unsigned */
> 	unsigned char *s = (unsigned char *)s1;
>-	int len;
>+	int len = 0;
> 
>+	if (buf_len < 1) {
>+		return -1;
>+	}
> 	/* If the two high bits of the byte are set, return 2. */
>-	if (0xC0 == (*s & 0xC0))
>+	if (0xC0 == (*s & 0xC0)) {
>+		if (buf_len < 2) {
>+			return -1;
>+		}
> 		return(2);
>+	}
> 
> 	/* Add up the length bytes. */
> 	for (len = 1; (*s); s += (*s) + 1) {
> 		len += *s + 1;
>-		SMB_ASSERT(len < 80);
>+		if (len > buf_len) {
>+			return -1;
>+		}
> 	}
> 
> 	return(len);
>diff --git a/source3/smbd/process.c b/source3/smbd/process.c
>index 3367d70..1596c0d 100644
>--- a/source3/smbd/process.c
>+++ b/source3/smbd/process.c
>@@ -1488,7 +1488,7 @@ static void process_smb(struct smbd_server_connection *conn,
> 		/*
> 		 * NetBIOS session request, keepalive, etc.
> 		 */
>-		reply_special((char *)inbuf);
>+		reply_special((char *)inbuf, nread);
> 		goto done;
> 	}
> 
>diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
>index e0ee8d2..9cb8492 100644
>--- a/source3/smbd/reply.c
>+++ b/source3/smbd/reply.c
>@@ -492,17 +492,14 @@ static bool netbios_session_retarget(const char *name, int name_type)
> }
> 
> /****************************************************************************
>- Reply to a (netbios-level) special message.
>+ Reply to a (netbios-level) special message. 
> ****************************************************************************/
> 
>-void reply_special(char *inbuf)
>+void reply_special(char *inbuf, size_t inbuf_size)
> {
> 	int msg_type = CVAL(inbuf,0);
> 	int msg_flags = CVAL(inbuf,1);
>-	fstring name1,name2;
>-	char name_type1, name_type2;
> 	struct smbd_server_connection *sconn = smbd_server_conn;
>-
> 	/*
> 	 * We only really use 4 bytes of the outbuf, but for the smb_setlen
> 	 * calculation & friends (srv_send_smb uses that) we need the full smb
>@@ -510,14 +507,19 @@ void reply_special(char *inbuf)
> 	 */
> 	char outbuf[smb_size];
> 
>-	*name1 = *name2 = 0;
>-
> 	memset(outbuf, '\0', sizeof(outbuf));
> 
> 	smb_setlen(outbuf,0);
> 
> 	switch (msg_type) {
> 	case 0x81: /* session request */
>+	{
>+		/* inbuf_size is guarenteed to be at least 4. */
>+		fstring name1,name2;
>+		int name_type1, name_type2;
>+		int name_len1, name_len2;
>+
>+		*name1 = *name2 = 0;
> 
> 		if (sconn->nbt.got_session) {
> 			exit_server_cleanly("multiple session request not permitted");
>@@ -525,13 +527,29 @@ void reply_special(char *inbuf)
> 
> 		SCVAL(outbuf,0,0x82);
> 		SCVAL(outbuf,3,0);
>-		if (name_len(inbuf+4) > 50 || 
>-		    name_len(inbuf+4 + name_len(inbuf + 4)) > 50) {
>+
>+		/* inbuf_size is guaranteed to be at least 4. */
>+		name_len1 = name_len((unsigned char *)(inbuf+4),inbuf_size - 4);
>+		if (name_len1 <= 0 || name_len1 > inbuf_size - 4) {
>+			DEBUG(0,("Invalid name length in session request\n"));
>+			return;
>+		}
>+		name_len2 = name_len((unsigned char *)(inbuf+4+name_len1),inbuf_size - 4 - name_len1);
>+		if (name_len2 <= 0 || name_len2 > inbuf_size - 4 - name_len1) {
> 			DEBUG(0,("Invalid name length in session request\n"));
> 			return;
> 		}
>-		name_type1 = name_extract(inbuf,4,name1);
>-		name_type2 = name_extract(inbuf,4 + name_len(inbuf + 4),name2);
>+
>+		name_type1 = name_extract((unsigned char *)inbuf,
>+				inbuf_size,(unsigned int)4,name1);
>+		name_type2 = name_extract((unsigned char *)inbuf,
>+				inbuf_size,(unsigned int)(4 + name_len1),name2);
>+
>+		if (name_type1 == -1 || name_type2 == -1) {
>+			DEBUG(0,("Invalid name type in session request\n"));
>+			return;
>+		}
>+
> 		DEBUG(2,("netbios connect: name1=%s0x%x name2=%s0x%x\n",
> 			 name1, name_type1, name2, name_type2));
> 
>@@ -565,6 +583,7 @@ void reply_special(char *inbuf)
> 
> 		sconn->nbt.got_session = true;
> 		break;
>+	}
> 
> 	case 0x89: /* session keepalive request 
> 		      (some old clients produce this?) */
>diff --git a/source3/utils/smbfilter.c b/source3/utils/smbfilter.c
>index 83de0c4..65d8461 100644
>--- a/source3/utils/smbfilter.c
>+++ b/source3/utils/smbfilter.c
>@@ -74,20 +74,44 @@ static void filter_reply(char *buf)
> 	}
> }
> 
>-static void filter_request(char *buf)
>+static void filter_request(char *buf, size_t buf_len)
> {
> 	int msg_type = CVAL(buf,0);
> 	int type = CVAL(buf,smb_com);
>-	fstring name1,name2;
> 	unsigned x;
>+	fstring name1,name2;
>+	int name_len1, name_len2;
>+	int name_type1, name_type2;
> 
> 	if (msg_type) {
> 		/* it's a netbios special */
>-		switch (msg_type) {
>+		switch (msg_type)
> 		case 0x81:
> 			/* session request */
>-			name_extract(buf,4,name1);
>-			name_extract(buf,4 + name_len(buf + 4),name2);
>+			/* inbuf_size is guaranteed to be at least 4. */
>+			name_len1 = name_len((unsigned char *)(buf+4),
>+					buf_len - 4);
>+			if (name_len1 <= 0 || name_len1 > buf_len - 4) {
>+				DEBUG(0,("Invalid name length in session request\n"));
>+				return;
>+			}
>+			name_len2 = name_len((unsigned char *)(buf+4+name_len1),
>+					buf_len - 4 - name_len1);
>+			if (name_len2 <= 0 || name_len2 > buf_len - 4 - name_len1) {
>+				DEBUG(0,("Invalid name length in session request\n"));
>+				return;
>+			}
>+
>+			name_type1 = name_extract((unsigned char *)buf,
>+					buf_len,(unsigned int)4,name1);
>+			name_type2 = name_extract((unsigned char *)buf,
>+					buf_len,(unsigned int)(4 + name_len1),name2);
>+
>+			if (name_type1 == -1 || name_type2 == -1) {
>+				DEBUG(0,("Invalid name type in session request\n"));
>+				return;
>+			}
>+
> 			d_printf("sesion_request: %s -> %s\n",
> 				 name1, name2);
> 			if (netbiosname) {
>@@ -97,11 +121,11 @@ static void filter_request(char *buf)
> 					/* replace the destination netbios
> 					 * name */
> 					memcpy(buf+4, mangled,
>-					       name_len(mangled));
>+					       name_len((unsigned char *)mangled,
>+							talloc_get_size(mangled)));
> 					TALLOC_FREE(mangled);
> 				}
> 			}
>-		}
> 		return;
> 	}
> 
>@@ -118,7 +142,6 @@ static void filter_request(char *buf)
> 		SIVAL(buf, smb_vwv11, x);
> 		break;
> 	}
>-
> }
> 
> /****************************************************************************
>@@ -184,7 +207,7 @@ static void filter_child(int c, struct sockaddr_storage *dest_ss)
> 				d_printf("client closed connection\n");
> 				exit(0);
> 			}
>-			filter_request(packet);
>+			filter_request(packet, len);
> 			if (!send_smb(s, packet)) {
> 				d_printf("server is dead\n");
> 				exit(1);
>-- 
>1.7.0.4
>

Flags:

vl: review+

Actions: View

Attachments on bug 7698: 5984