Consolidate the buffer checks for the reply_trans style functions

This is the one where I found the problem that led to 3.2.5. So if there is one
checkin in the last year that I would like others to review and *understand*,
it is this one :-)

Volker

diff --git a/source3/smbd/ipc.c b/source3/smbd/ipc.c
index bf9b1d87c5fd13ad09bbcef0846dda8fc6642333..649ead468281e545656823f068025d053a5abbc9 100644 (file)
--- 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 av_size;
 
        START_PROFILE(SMBtrans);
 
@@ -513,7 +512,6 @@ void reply_trans(struct smb_request *req)
                return;
        }
 
-       av_size = smb_len(req->inbuf);
        dsoff = SVAL(req->vwv+12, 0);
        dscnt = SVAL(req->vwv+11, 0);
        psoff = SVAL(req->vwv+10, 0);
@@ -559,6 +557,12 @@ void reply_trans(struct smb_request *req)
                goto bad_param;
 
        if (state->total_data)  {
+
+               if (trans_oob(state->total_data, 0, dscnt)
+                   || trans_oob(smb_len(req->inbuf), dsoff, dscnt)) {
+                       goto bad_param;
+               }
+
                /* Can't use talloc here, the core routines do realloc on the
                 * params and data. Out of paranoia, 100 bytes too many. */
                state->data = (char *)SMB_MALLOC(state->total_data+100);
@@ -573,21 +577,16 @@ void reply_trans(struct smb_request *req)
                /* null-terminate the slack space */
                memset(&state->data[state->total_data], 0, 100);
 
-               if (dscnt > state->total_data ||
-                               dsoff+dscnt < dsoff) {
-                       goto bad_param;
-               }
-
-               if (dsoff > av_size ||
-                               dscnt > av_size ||
-                               dsoff+dscnt > av_size) {
-                       goto bad_param;
-               }
-
                memcpy(state->data,smb_base(req->inbuf)+dsoff,dscnt);
        }
 
        if (state->total_param) {
+
+               if (trans_oob(state->total_param, 0, pscnt)
+                   || trans_oob(smb_len(req->inbuf), psoff, pscnt)) {
+                       goto bad_param;
+               }
+
                /* Can't use talloc here, the core routines do realloc on the
                 * params and data. Out of paranoia, 100 bytes too many */
                state->param = (char *)SMB_MALLOC(state->total_param+100);
@@ -603,17 +602,6 @@ void reply_trans(struct smb_request *req)
                /* null-terminate the slack space */
                memset(&state->param[state->total_param], 0, 100);
 
-               if (pscnt > state->total_param ||
-                               psoff+pscnt < psoff) {
-                       goto bad_param;
-               }
-
-               if (psoff > av_size ||
-                               pscnt > av_size ||
-                               psoff+pscnt > av_size) {
-                       goto bad_param;
-               }
-
                memcpy(state->param,smb_base(req->inbuf)+psoff,pscnt);
        }
 
@@ -696,7 +684,6 @@ void reply_transs(struct smb_request *req)
        connection_struct *conn = req->conn;
        unsigned int pcnt,poff,dcnt,doff,pdisp,ddisp;
        struct trans_state *state;
-       unsigned int av_size;
 
        START_PROFILE(SMBtranss);
 
@@ -729,8 +716,6 @@ void reply_transs(struct smb_request *req)
        if (SVAL(req->vwv+1, 0) < state->total_data)
                state->total_data = SVAL(req->vwv+1, 0);
 
-       av_size = smb_len(req->inbuf);
-
        pcnt = SVAL(req->vwv+2, 0);
        poff = SVAL(req->vwv+3, 0);
        pdisp = SVAL(req->vwv+4, 0);
@@ -747,41 +732,19 @@ void reply_transs(struct smb_request *req)
                goto bad_param;
 
        if (pcnt) {
-               if (pdisp > state->total_param ||
-                               pcnt > state->total_param ||
-                               pdisp+pcnt > state->total_param ||
-                               pdisp+pcnt < pdisp) {
-                       goto bad_param;
-               }
-
-               if (poff > av_size ||
-                               pcnt > av_size ||
-                               poff+pcnt > av_size ||
-                               poff+pcnt < poff) {
+               if (trans_oob(state->total_param, pdisp, pcnt)
+                   || trans_oob(smb_len(req->inbuf), poff, pcnt)) {
                        goto bad_param;
                }
-
-               memcpy(state->param+pdisp,smb_base(req->inbuf)+poff,
-                      pcnt);
+               memcpy(state->param+pdisp,smb_base(req->inbuf)+poff,pcnt);
        }
 
        if (dcnt) {
-               if (ddisp > state->total_data ||
-                               dcnt > state->total_data ||
-                               ddisp+dcnt > state->total_data ||
-                               ddisp+dcnt < ddisp) {
+               if (trans_oob(state->total_data, ddisp, dcnt)
+                   || trans_oob(smb_len(req->inbuf), doff, dcnt)) {
                        goto bad_param;
                }
-
-               if (doff > av_size ||
-                               dcnt > av_size ||
-                               doff+dcnt > av_size ||
-                               doff+dcnt < doff) {
-                       goto bad_param;
-               }
-
-               memcpy(state->data+ddisp, smb_base(req->inbuf)+doff,
-                      dcnt);
+               memcpy(state->data+ddisp, smb_base(req->inbuf)+doff,dcnt);
        }
 
        if ((state->received_param < state->total_param) ||

diff --git a/source3/smbd/nttrans.c b/source3/smbd/nttrans.c
index b516f02c2198de1d0621b03b0211fb2fa2469a3b..fe2029eeed9e08c3a1cdf09ab9112a494c485531 100644 (file)
--- 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 av_size;
 
        START_PROFILE(SMBnttrans);
 
@@ -2539,7 +2538,6 @@ void reply_nttrans(struct smb_request *req)
                return;
        }
 
-       av_size = smb_len(req->inbuf);
        pscnt = IVAL(req->vwv+9, 1);
        psoff = IVAL(req->vwv+11, 1);
        dscnt = IVAL(req->vwv+13, 1);
@@ -2616,6 +2614,12 @@ void reply_nttrans(struct smb_request *req)
                goto bad_param;
 
        if (state->total_data)  {
+
+               if (trans_oob(state->total_data, 0, dscnt)
+                   || trans_oob(smb_len(req->inbuf), dsoff, dscnt)) {
+                       goto bad_param;
+               }
+
                /* Can't use talloc here, the core routines do realloc on the
                 * params and data. */
                if ((state->data = (char *)SMB_MALLOC(state->total_data)) == NULL) {
@@ -2627,21 +2631,16 @@ void reply_nttrans(struct smb_request *req)
                        return;
                }
 
-               if (dscnt > state->total_data ||
-                               dsoff+dscnt < dsoff) {
-                       goto bad_param;
-               }
-
-               if (dsoff > av_size ||
-                               dscnt > av_size ||
-                               dsoff+dscnt > av_size) {
-                       goto bad_param;
-               }
-
                memcpy(state->data,smb_base(req->inbuf)+dsoff,dscnt);
        }
 
        if (state->total_param) {
+
+               if (trans_oob(state->total_param, 0, pscnt)
+                   || trans_oob(smb_len(req->inbuf), psoff, pscnt)) {
+                       goto bad_param;
+               }
+
                /* Can't use talloc here, the core routines do realloc on the
                 * params and data. */
                if ((state->param = (char *)SMB_MALLOC(state->total_param)) == NULL) {
@@ -2654,17 +2653,6 @@ void reply_nttrans(struct smb_request *req)
                        return;
                }
 
-               if (pscnt > state->total_param ||
-                               psoff+pscnt < psoff) {
-                       goto bad_param;
-               }
-
-               if (psoff > av_size ||
-                               pscnt > av_size ||
-                               psoff+pscnt > av_size) {
-                       goto bad_param;
-               }
-
                memcpy(state->param,smb_base(req->inbuf)+psoff,pscnt);
        }
 
@@ -2741,8 +2729,6 @@ void reply_nttranss(struct smb_request *req)
        connection_struct *conn = req->conn;
        uint32_t pcnt,poff,dcnt,doff,pdisp,ddisp;
        struct trans_state *state;
-       uint32_t av_size;
-       uint32_t size;
 
        START_PROFILE(SMBnttranss);
 
@@ -2776,9 +2762,6 @@ void reply_nttranss(struct smb_request *req)
                state->total_data = IVAL(req->vwv+3, 1);
        }
 
-       size = smb_len(req->inbuf) + 4;
-       av_size = smb_len(req->inbuf);
-
        pcnt = IVAL(req->vwv+5, 1);
        poff = IVAL(req->vwv+7, 1);
        pdisp = IVAL(req->vwv+9, 1);
@@ -2795,41 +2778,19 @@ void reply_nttranss(struct smb_request *req)
                goto bad_param;
 
        if (pcnt) {
-               if (pdisp > state->total_param ||
-                               pcnt > state->total_param ||
-                               pdisp+pcnt > state->total_param ||
-                               pdisp+pcnt < pdisp) {
-                       goto bad_param;
-               }
-
-               if (poff > av_size ||
-                               pcnt > av_size ||
-                               poff+pcnt > av_size ||
-                               poff+pcnt < poff) {
+               if (trans_oob(state->total_param, pdisp, pcnt)
+                   || trans_oob(smb_len(req->inbuf), poff, pcnt)) {
                        goto bad_param;
                }
-
-               memcpy(state->param+pdisp, smb_base(req->inbuf)+poff,
-                      pcnt);
+               memcpy(state->param+pdisp, smb_base(req->inbuf)+poff,pcnt);
        }
 
        if (dcnt) {
-               if (ddisp > state->total_data ||
-                               dcnt > state->total_data ||
-                               ddisp+dcnt > state->total_data ||
-                               ddisp+dcnt < ddisp) {
+               if (trans_oob(state->total_data, ddisp, dcnt)
+                   || trans_oob(smb_len(req->inbuf), doff, dcnt)) {
                        goto bad_param;
                }
-
-               if (doff > av_size ||
-                               dcnt > av_size ||
-                               doff+dcnt > av_size ||
-                               doff+dcnt < doff) {
-                       goto bad_param;
-               }
-
-               memcpy(state->data+ddisp, smb_base(req->inbuf)+doff,
-                      dcnt);
+               memcpy(state->data+ddisp, smb_base(req->inbuf)+doff,dcnt);
        }
 
        if ((state->received_param < state->total_param) ||

diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c
index 3a28bd424f167497085c6918567620dd727ce226..cc8c61175bb0656d00b56439ac1dc8a9466f7d1d 100644 (file)
--- 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 av_size;
        struct trans_state *state;
        NTSTATUS result;
 
@@ -7550,7 +7549,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);
-       av_size = smb_len(req->inbuf);
 
        result = allow_new_trans(conn->pending_trans, req->mid);
        if (!NT_STATUS_IS_OK(result)) {
@@ -7632,6 +7630,12 @@ void reply_trans2(struct smb_request *req)
                goto bad_param;
 
        if (state->total_data) {
+
+               if (trans_oob(state->total_data, 0, dscnt)
+                   || trans_oob(smb_len(req->inbuf), dsoff, dscnt)) {
+                       goto bad_param;
+               }
+
                /* Can't use talloc here, the core routines do realloc on the
                 * params and data. */
                state->data = (char *)SMB_MALLOC(state->total_data);
@@ -7644,21 +7648,16 @@ void reply_trans2(struct smb_request *req)
                        return;
                }
 
-               if (dscnt > state->total_data ||
-                               dsoff+dscnt < dsoff) {
-                       goto bad_param;
-               }
-
-               if (dsoff > av_size ||
-                               dscnt > av_size ||
-                               dsoff+dscnt > av_size) {
-                       goto bad_param;
-               }
-
                memcpy(state->data,smb_base(req->inbuf)+dsoff,dscnt);
        }
 
        if (state->total_param) {
+
+               if (trans_oob(state->total_param, 0, pscnt)
+                   || trans_oob(smb_len(req->inbuf), psoff, pscnt)) {
+                       goto bad_param;
+               }
+
                /* Can't use talloc here, the core routines do realloc on the
                 * params and data. */
                state->param = (char *)SMB_MALLOC(state->total_param);
@@ -7672,17 +7671,6 @@ void reply_trans2(struct smb_request *req)
                        return;
                } 
 
-               if (pscnt > state->total_param ||
-                               psoff+pscnt < psoff) {
-                       goto bad_param;
-               }
-
-               if (psoff > av_size ||
-                               pscnt > av_size ||
-                               psoff+pscnt > av_size) {
-                       goto bad_param;
-               }
-
                memcpy(state->param,smb_base(req->inbuf)+psoff,pscnt);
        }
 
@@ -7730,8 +7718,6 @@ void reply_transs2(struct smb_request *req)
        connection_struct *conn = req->conn;
        unsigned int pcnt,poff,dcnt,doff,pdisp,ddisp;
        struct trans_state *state;
-       unsigned int size;
-       unsigned int av_size;
 
        START_PROFILE(SMBtranss2);
 
@@ -7743,9 +7729,6 @@ void reply_transs2(struct smb_request *req)
                return;
        }
 
-       size = smb_len(req->inbuf)+4;
-       av_size = smb_len(req->inbuf);
-
        for (state = conn->pending_trans; state != NULL;
             state = state->next) {
                if (state->mid == req->mid) {
@@ -7783,41 +7766,19 @@ void reply_transs2(struct smb_request *req)
                goto bad_param;
 
        if (pcnt) {
-               if (pdisp > state->total_param ||
-                               pcnt > state->total_param ||
-                               pdisp+pcnt > state->total_param ||
-                               pdisp+pcnt < pdisp) {
-                       goto bad_param;
-               }
-
-               if (poff > av_size ||
-                               pcnt > av_size ||
-                               poff+pcnt > av_size ||
-                               poff+pcnt < poff) {
+               if (trans_oob(state->total_param, pdisp, pcnt)
+                   || trans_oob(smb_len(req->inbuf), poff, pcnt)) {
                        goto bad_param;
                }
-
-               memcpy(state->param+pdisp,smb_base(req->inbuf)+poff,
-                      pcnt);
+               memcpy(state->param+pdisp,smb_base(req->inbuf)+poff,pcnt);
        }
 
        if (dcnt) {
-               if (ddisp > state->total_data ||
-                               dcnt > state->total_data ||
-                               ddisp+dcnt > state->total_data ||
-                               ddisp+dcnt < ddisp) {
+               if (trans_oob(state->total_data, ddisp, dcnt)
+                   || trans_oob(smb_len(req->inbuf), doff, dcnt)) {
                        goto bad_param;
                }
-
-               if (doff > av_size ||
-                               dcnt > av_size ||
-                               doff+dcnt > av_size ||
-                               doff+dcnt < doff) {
-                       goto bad_param;
-               }
-
-               memcpy(state->data+ddisp, smb_base(req->inbuf)+doff,
-                      dcnt);      
+               memcpy(state->data+ddisp, smb_base(req->inbuf)+doff,dcnt);
        }
 
        if ((state->received_param < state->total_param) ||