From 891bad006072b0444baba66682be1148a310a967 Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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