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.

Jeremy.

6 files changed, 134 insertions, 57 deletions

diff --git a/source3/include/proto.h b/source3/include/proto.h
index c252b0e19c..1687186268 100644
--- a/source3/include/proto.h
+++ b/source3/include/proto.h
@@ -2806,8 +2806,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  */
 
@@ -5268,7 +5268,7 @@ bool check_fsp(connection_struct *conn, struct smb_request *req,
 	       files_struct *fsp);
 bool check_fsp_ntquota_handle(connection_struct *conn, struct smb_request *req,
 			      files_struct *fsp);
-void reply_special(struct smbd_server_connection *sconn, char *inbuf);
+void reply_special(struct smbd_server_connection *sconn, 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 f76f17c1bd..1e11e158f7 100644
--- a/source3/libsmb/cliconnect.c
+++ b/source3/libsmb/cliconnect.c
@@ -2230,6 +2230,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 */
@@ -2248,8 +2249,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 */
@@ -2261,8 +2265,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 150e1f6af9..1c95d0e99f 100644
--- a/source3/libsmb/nmblib.c
+++ b/source3/libsmb/nmblib.c
@@ -1241,21 +1241,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);
@@ -1263,21 +1275,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);
@@ -1356,12 +1360,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 {
@@ -1371,37 +1388,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 b890687425..1ce4ca7263 100644
--- a/source3/smbd/process.c
+++ b/source3/smbd/process.c
@@ -1656,7 +1656,7 @@ static void process_smb(struct smbd_server_connection *conn,
 		/*
 		 * NetBIOS session request, keepalive, etc.
 		 */
-		reply_special(conn, (char *)inbuf);
+		reply_special(conn, (char *)inbuf, nread);
 		goto done;
 	}
 
diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index 43ca479e28..abff317d52 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -483,16 +483,13 @@ static bool netbios_session_retarget(struct smbd_server_connection *sconn,
 }
 
 /****************************************************************************
- Reply to a (netbios-level) special message.
+ Reply to a (netbios-level) special message. 
 ****************************************************************************/
 
-void reply_special(struct smbd_server_connection *sconn, char *inbuf)
+void reply_special(struct smbd_server_connection *sconn, 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;
-
 	/*
 	 * 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
@@ -500,14 +497,19 @@ void reply_special(struct smbd_server_connection *sconn, 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");
@@ -515,13 +517,29 @@ void reply_special(struct smbd_server_connection *sconn, 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));
 
@@ -564,6 +582,7 @@ void reply_special(struct smbd_server_connection *sconn, 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 83de0c4c37..65d846124b 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);