[SCM] Samba Shared Repository - branch master updated - 9a3be6f0f8e120797a02fa1be60b51812cfd86f5

Volker Lendecke vlendec at samba.org
Fri Nov 28 08:17:01 GMT 2008


The branch, master has been updated
       via  9a3be6f0f8e120797a02fa1be60b51812cfd86f5 (commit)
       via  738271fc2026b2911b7d20a73496989641714df3 (commit)
       via  9da3101e449649f0614240f13157ac81e17b2e90 (commit)
       via  4a322398c5ffaf238eba1e7bfbe47e2d093c7c4d (commit)
       via  599707c87a739811ba426e44b11189e1ddba078e (commit)
      from  6a627b440e8b3f42db2a8a27047dd3482bad0d28 (commit)

http://gitweb.samba.org/?p=samba.git;a=shortlog;h=master


- Log -----------------------------------------------------------------
commit 9a3be6f0f8e120797a02fa1be60b51812cfd86f5
Author: Volker Lendecke <vl at samba.org>
Date:   Sat Nov 8 16:48:20 2008 +0100

    Move cli_trans_oob to lib/util.c
    
    Rename it to trans_oob, it will be used in the server routines.

commit 738271fc2026b2911b7d20a73496989641714df3
Author: Volker Lendecke <vl at samba.org>
Date:   Sat Nov 8 16:14:12 2008 +0100

    Remove the variable "size" from reply_nttrans
    
    This converts the range checks for the setup[] array to rely on req->wct being
    set correctly in init_smb_request. As that already verifies the vwv array to be
    in the range of the smb_request inbuf, we don't have to do overflow checks here
    anymore.
    
    Jeremy, please check thoroughly! :-)
    
    Thanks,
    
    Volker

commit 9da3101e449649f0614240f13157ac81e17b2e90
Author: Volker Lendecke <vl at samba.org>
Date:   Sat Nov 8 16:14:12 2008 +0100

    Remove the variable "size" from reply_trans
    
    This converts the range checks for the setup[] array to rely on req->wct being
    set correctly in init_smb_request. As that already verifies the vwv array to be
    in the range of the smb_request inbuf, we don't have to do overflow checks here
    anymore.
    
    Jeremy, please check thoroughly! :-)
    
    Thanks,
    
    Volker

commit 4a322398c5ffaf238eba1e7bfbe47e2d093c7c4d
Author: Volker Lendecke <vl at samba.org>
Date:   Sat Nov 8 16:03:07 2008 +0100

    Remove an unused variable

commit 599707c87a739811ba426e44b11189e1ddba078e
Author: Volker Lendecke <vl at samba.org>
Date:   Sat Nov 8 15:44:20 2008 +0100

    Remove two direct inbuf references from reply_sesssetup_and_X_spnego()

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

Summary of changes:
 source3/include/proto.h   |    1 +
 source3/lib/util.c        |   19 +++++++++++++++++++
 source3/libsmb/clitrans.c |   21 ++++-----------------
 source3/smbd/ipc.c        |   28 ++++++++++++++++------------
 source3/smbd/nttrans.c    |   23 +++++++++++++----------
 source3/smbd/sesssetup.c  |    4 ++--
 source3/smbd/trans2.c     |    2 --
 7 files changed, 55 insertions(+), 43 deletions(-)


Changeset truncated at 500 lines:

diff --git a/source3/include/proto.h b/source3/include/proto.h
index 73be87b..71f12a6 100644
--- a/source3/include/proto.h
+++ b/source3/include/proto.h
@@ -1251,6 +1251,7 @@ char *procid_str_static(const struct server_id *pid);
 bool procid_valid(const struct server_id *pid);
 bool procid_is_local(const struct server_id *pid);
 int this_is_smp(void);
+bool trans_oob(uint32_t bufsize, uint32_t offset, uint32_t length);
 bool is_offset_safe(const char *buf_base, size_t buf_len, char *ptr, size_t off);
 char *get_safe_ptr(const char *buf_base, size_t buf_len, char *ptr, size_t off);
 char *get_safe_str_ptr(const char *buf_base, size_t buf_len, char *ptr, size_t off);
diff --git a/source3/lib/util.c b/source3/lib/util.c
index 5007fb7..074b523 100644
--- a/source3/lib/util.c
+++ b/source3/lib/util.c
@@ -2879,6 +2879,25 @@ int this_is_smp(void)
 }
 
 /****************************************************************
+ Check if offset/length fit into bufsize. Should probably be
+ merged with is_offset_safe, but this would require a rewrite
+ of lanman.c. Later :-)
+****************************************************************/
+
+bool trans_oob(uint32_t bufsize, uint32_t offset, uint32_t length)
+{
+	if ((offset + length < offset) || (offset + length < length)) {
+		/* wrap */
+		return true;
+	}
+	if ((offset > bufsize) || (offset + length > bufsize)) {
+		/* overflow */
+		return true;
+	}
+	return false;
+}
+
+/****************************************************************
  Check if an offset into a buffer is safe.
  If this returns True it's safe to indirect into the byte at
  pointer ptr+off.
diff --git a/source3/libsmb/clitrans.c b/source3/libsmb/clitrans.c
index c929f0b..bbdfb75 100644
--- a/source3/libsmb/clitrans.c
+++ b/source3/libsmb/clitrans.c
@@ -978,19 +978,6 @@ static void cli_trans_ship_rest(struct async_req *req,
 	}
 }
 
-static bool cli_trans_oob(uint32_t bufsize, uint32_t offset, uint32_t length)
-{
-	if ((offset + length < offset) || (offset + length < length)) {
-		/* wrap */
-		return true;
-	}
-	if ((offset > bufsize) || (offset + length > bufsize)) {
-		/* overflow */
-		return true;
-	}
-	return false;
-}
-
 static NTSTATUS cli_pull_trans(struct async_req *req,
 			       struct cli_request *cli_req,
 			       uint8_t smb_cmd, bool expect_first_reply,
@@ -1072,10 +1059,10 @@ static NTSTATUS cli_pull_trans(struct async_req *req,
 	 * length. Likewise for param_ofs/param_disp.
 	 */
 
-	if (cli_trans_oob(smb_len(cli_req->inbuf), param_ofs, *pnum_param)
-	    || cli_trans_oob(*ptotal_param, *pparam_disp, *pnum_param)
-	    || cli_trans_oob(smb_len(cli_req->inbuf), data_ofs, *pnum_data)
-	    || cli_trans_oob(*ptotal_data, *pdata_disp, *pnum_data)) {
+	if (trans_oob(smb_len(cli_req->inbuf), param_ofs, *pnum_param)
+	    || trans_oob(*ptotal_param, *pparam_disp, *pnum_param)
+	    || trans_oob(smb_len(cli_req->inbuf), data_ofs, *pnum_data)
+	    || trans_oob(*ptotal_data, *pdata_disp, *pnum_data)) {
 		return NT_STATUS_INVALID_NETWORK_RESPONSE;
 	}
 
diff --git a/source3/smbd/ipc.c b/source3/smbd/ipc.c
index a617756..bf9b1d8 100644
--- a/source3/smbd/ipc.c
+++ b/source3/smbd/ipc.c
@@ -503,7 +503,6 @@ void reply_trans(struct smb_request *req)
 	unsigned int pscnt;
 	struct trans_state *state;
 	NTSTATUS result;
-	unsigned int size;
 	unsigned int av_size;
 
 	START_PROFILE(SMBtrans);
@@ -514,7 +513,6 @@ void reply_trans(struct smb_request *req)
 		return;
 	}
 
-	size = smb_len(req->inbuf) + 4;
 	av_size = smb_len(req->inbuf);
 	dsoff = SVAL(req->vwv+12, 0);
 	dscnt = SVAL(req->vwv+11, 0);
@@ -624,6 +622,19 @@ void reply_trans(struct smb_request *req)
 
 	if (state->setup_count) {
 		unsigned int i;
+
+		/*
+		 * No overflow possible here, state->setup_count is an
+		 * unsigned int, being filled by a single byte from
+		 * CVAL(req->vwv+13, 0) above. The cast in the comparison
+		 * below is not necessary, it's here to clarify things. The
+		 * validity of req->vwv and req->wct has been checked in
+		 * init_smb_request already.
+		 */
+		if (state->setup_count + 14 > (unsigned int)req->wct) {
+			goto bad_param;
+		}
+
 		if((state->setup = TALLOC_ARRAY(
 			    state, uint16, state->setup_count)) == NULL) {
 			DEBUG(0,("reply_trans: setup malloc fail for %u "
@@ -636,17 +647,10 @@ void reply_trans(struct smb_request *req)
 			END_PROFILE(SMBtrans);
 			return;
 		} 
-		if (req->inbuf+smb_vwv14+(state->setup_count*SIZEOFWORD) >
-		    req->inbuf + size)
-			goto bad_param;
-		if ((smb_vwv14+(state->setup_count*SIZEOFWORD) < smb_vwv14) ||
-		    (smb_vwv14+(state->setup_count*SIZEOFWORD) <
-		     (state->setup_count*SIZEOFWORD)))
-			goto bad_param;
 
-		for (i=0;i<state->setup_count;i++)
-			state->setup[i] = SVAL(req->inbuf,
-					       smb_vwv14+i*SIZEOFWORD);
+		for (i=0;i<state->setup_count;i++) {
+			state->setup[i] = SVAL(req->vwv + 14 + i, 0);
+		}
 	}
 
 	state->received_param = pscnt;
diff --git a/source3/smbd/nttrans.c b/source3/smbd/nttrans.c
index 329ba23..b516f02 100644
--- a/source3/smbd/nttrans.c
+++ b/source3/smbd/nttrans.c
@@ -2529,7 +2529,6 @@ void reply_nttrans(struct smb_request *req)
 	uint16 function_code;
 	NTSTATUS result;
 	struct trans_state *state;
-	uint32_t size;
 	uint32_t av_size;
 
 	START_PROFILE(SMBnttrans);
@@ -2540,7 +2539,6 @@ void reply_nttrans(struct smb_request *req)
 		return;
 	}
 
-	size = smb_len(req->inbuf) + 4;
 	av_size = smb_len(req->inbuf);
 	pscnt = IVAL(req->vwv+9, 1);
 	psoff = IVAL(req->vwv+11, 1);
@@ -2676,6 +2674,19 @@ void reply_nttrans(struct smb_request *req)
 	if(state->setup_count > 0) {
 		DEBUG(10,("reply_nttrans: state->setup_count = %d\n",
 			  state->setup_count));
+
+		/*
+		 * No overflow possible here, state->setup_count is an
+		 * unsigned int, being filled by a single byte from
+		 * CVAL(req->vwv+13, 0) above. The cast in the comparison
+		 * below is not necessary, it's here to clarify things. The
+		 * validity of req->vwv and req->wct has been checked in
+		 * init_smb_request already.
+		 */
+		if ((state->setup_count/2) + 19 > (unsigned int)req->wct) {
+			goto bad_param;
+		}
+
 		state->setup = (uint16 *)TALLOC(state, state->setup_count);
 		if (state->setup == NULL) {
 			DEBUG(0,("reply_nttrans : Out of memory\n"));
@@ -2687,14 +2698,6 @@ void reply_nttrans(struct smb_request *req)
 			return;
 		}
 
-		if ((smb_nt_SetupStart + state->setup_count < smb_nt_SetupStart) ||
-		    (smb_nt_SetupStart + state->setup_count < state->setup_count)) {
-			goto bad_param;
-		}
-		if (smb_nt_SetupStart + state->setup_count > size) {
-			goto bad_param;
-		}
-
 		memcpy(state->setup, req->vwv+19, state->setup_count);
 		dump_data(10, (uint8 *)state->setup, state->setup_count);
 	}
diff --git a/source3/smbd/sesssetup.c b/source3/smbd/sesssetup.c
index fde6cdc..24a2010 100644
--- a/source3/smbd/sesssetup.c
+++ b/source3/smbd/sesssetup.c
@@ -1171,7 +1171,7 @@ static void reply_sesssetup_and_X_spnego(struct smb_request *req)
 	const char *p2;
 	uint16 data_blob_len = SVAL(req->vwv+7, 0);
 	enum remote_arch_types ra_type = get_remote_arch();
-	int vuid = SVAL(req->inbuf,smb_uid);
+	int vuid = req->vuid;
 	user_struct *vuser = NULL;
 	NTSTATUS status = NT_STATUS_OK;
 	uint16 smbpid = req->smbpid;
@@ -1203,7 +1203,7 @@ static void reply_sesssetup_and_X_spnego(struct smb_request *req)
 	file_save("negotiate.dat", blob1.data, blob1.length);
 #endif
 
-	p2 = (char *)req->inbuf + smb_vwv13 + data_blob_len;
+	p2 = (char *)req->buf + data_blob_len;
 
 	p2 += srvstr_pull_req_talloc(talloc_tos(), req, &tmp, p2,
 				     STR_TERMINATE);
diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c
index 0c63588..3a28bd4 100644
--- a/source3/smbd/trans2.c
+++ b/source3/smbd/trans2.c
@@ -7533,7 +7533,6 @@ void reply_trans2(struct smb_request *req)
 	unsigned int psoff;
 	unsigned int pscnt;
 	unsigned int tran_call;
-	unsigned int size;
 	unsigned int av_size;
 	struct trans_state *state;
 	NTSTATUS result;
@@ -7551,7 +7550,6 @@ void reply_trans2(struct smb_request *req)
 	psoff = SVAL(req->vwv+10, 0);
 	pscnt = SVAL(req->vwv+9, 0);
 	tran_call = SVAL(req->vwv+14, 0);
-	size = smb_len(req->inbuf) + 4;
 	av_size = smb_len(req->inbuf);
 
 	result = allow_new_trans(conn->pending_trans, req->mid);


-- 
Samba Shared Repository


More information about the samba-cvs mailing list