The branch, v3-5-test has been updated
       via  08bb0fb Fix bug #7698 - Assert causes smbd to panic on invalid 
NetBIOS session request.
      from  f1f260c s3: Fix bug 7470

http://gitweb.samba.org/?p=samba.git;a=shortlog;h=v3-5-test


- Log -----------------------------------------------------------------
commit 08bb0fb61580cf528109ebd061a91e4fa5be5a2b
Author: Jeremy Allison <j...@samba.org>
Date:   Sun Sep 26 04:49:29 2010 -0700

    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.

-----------------------------------------------------------------------

Summary of changes:
 source3/include/proto.h     |    6 +-
 source3/libsmb/cliconnect.c |   15 +++++--
 source3/libsmb/nmblib.c     |   86 ++++++++++++++++++++++++++++--------------
 source3/smbd/process.c      |    2 +-
 source3/smbd/reply.c        |   39 ++++++++++++++-----
 source3/utils/smbfilter.c   |   41 ++++++++++++++++----
 6 files changed, 133 insertions(+), 56 deletions(-)


Changeset truncated at 500 lines:

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..3101e18 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -495,14 +495,11 @@ static bool netbios_session_retarget(const char *name, 
int name_type)
  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);


-- 
Samba Shared Repository

Reply via email to