Don't go past the end of a page in a NetScaler file.

Records in a properly formatted NetScaler file shouldn't go past the end
of a page, but nothing guarantees that a NetScaler file will be properly
formatted.

NetScaler 3.x files allow record bodies to go past the end of a page,
but 1.x and 2.x files don't, so treat record headers that go past the
end of a page, and record bodies in 1.x and 2.x files that go past the
end of a page, as errors.

Clean up some stuff while we're at it.

Bug: 13430
Change-Id: I3b1d56086e3bb14b246406f306e3d730df337561
Reviewed-on: https://code.wireshark.org/review/20326
Reviewed-by: Guy Harris <guy@alum.mit.edu>
(cherry picked from commit 19c51d27b9cabf7b811d6c60e42a59330235c2de)
Reviewed-on: https://code.wireshark.org/review/20327

1 files changed, 69 insertions, 16 deletions

diff --git a/wiretap/netscaler.c b/wiretap/netscaler.c
index 789146d7b1..7c4c0b0093 100644
--- a/wiretap/netscaler.c
+++ b/wiretap/netscaler.c
@@ -604,8 +604,8 @@ typedef struct nspr_pktracepart_v26
 typedef struct {
     gchar  *pnstrace_buf;
     gint64  xxx_offset;
-    gint32  nstrace_buf_offset;
-    gint32  nstrace_buflen;
+    guint32 nstrace_buf_offset;
+    guint32 nstrace_buflen;
     /* Performance Monitor Time variables */
     guint32 nspm_curtime;         /* current time since 1970 */
     guint64 nspm_curtimemsec;     /* current time in milliseconds */
@@ -867,8 +867,8 @@ nspm_signature_version(wtap *wth, gchar *nstrace_buf, gint32 len)
     {\
         nstrace_t *nstrace = (nstrace_t *)wth->priv;\
         gchar* nstrace_buf = nstrace->pnstrace_buf;\
-        gint32 nstrace_buf_offset = nstrace->nstrace_buf_offset;\
-        gint32 nstrace_buflen = nstrace->nstrace_buflen;\
+        guint32 nstrace_buf_offset = nstrace->nstrace_buf_offset;\
+        guint32 nstrace_buflen = nstrace->nstrace_buflen;\
         int bytes_read;\
         do\
         {\
@@ -892,7 +892,7 @@ nspm_signature_version(wtap *wth, gchar *nstrace_buf, gint32 len)
             nstrace_buf_offset = 0;\
             nstrace->xxx_offset += nstrace_buflen;\
             nstrace_buflen = GET_READ_PAGE_SIZE((nstrace->file_size - nstrace->xxx_offset));\
-        }while((nstrace_buflen > 0) && (bytes_read = file_read(nstrace_buf, nstrace_buflen, wth->fh)) && bytes_read == nstrace_buflen); \
+        }while((nstrace_buflen > 0) && (bytes_read = file_read(nstrace_buf, nstrace_buflen, wth->fh)) > 0 && (guint32)bytes_read == nstrace_buflen); \
         return FALSE;\
     }
 
@@ -966,10 +966,28 @@ static gboolean nstrace_set_start_time(wtap *wth)
 #define PACKET_DESCRIBE(phdr,FULLPART,fullpart,ver,type,HEADERVER) \
     do {\
         nspr_pktrace##fullpart##_v##ver##_t *type = (nspr_pktrace##fullpart##_v##ver##_t *) &nstrace_buf[nstrace_buf_offset];\
+        /* Make sure the record header is entirely contained in the page */\
+        if ((nstrace_buflen - nstrace_buf_offset) < sizeof *type) {\
+            *err = WTAP_ERR_BAD_FILE;\
+            *err_info = g_strdup("nstrace: record header crosses page boundary");\
+            return FALSE;\
+        }\
         (phdr)->rec_type = REC_TYPE_PACKET;\
         TIMEDEFV##ver((phdr),fp,type);\
         FULLPART##SIZEDEFV##ver((phdr),type,ver);\
         TRACE_V##ver##_REC_LEN_OFF((phdr),v##ver##_##fullpart,type,pktrace##fullpart##_v##ver);\
+        /* Check sanity of record size */\
+        if ((phdr)->caplen < sizeof *type) {\
+            *err = WTAP_ERR_BAD_FILE;\
+            *err_info = g_strdup("nstrace: record size is less than record header size");\
+            return FALSE;\
+        }\
+        /* Make sure the record is entirely contained in the page */\
+        if ((nstrace_buflen - nstrace_buf_offset) < (phdr)->caplen) {\
+            *err = WTAP_ERR_BAD_FILE;\
+            *err_info = g_strdup("nstrace: record crosses page boundary");\
+            return FALSE;\
+        }\
         ws_buffer_assure_space(wth->frame_buffer, (phdr)->caplen);\
         memcpy(ws_buffer_start_ptr(wth->frame_buffer), type, (phdr)->caplen);\
         *data_offset = nstrace->xxx_offset + nstrace_buf_offset;\
@@ -984,8 +1002,8 @@ static gboolean nstrace_read_v10(wtap *wth, int *err, gchar **err_info, gint64 *
     nstrace_t *nstrace = (nstrace_t *)wth->priv;
     guint64 nsg_creltime = nstrace->nsg_creltime;
     gchar *nstrace_buf = nstrace->pnstrace_buf;
-    gint32 nstrace_buf_offset = nstrace->nstrace_buf_offset;
-    gint32 nstrace_buflen = nstrace->nstrace_buflen;
+    guint32 nstrace_buf_offset = nstrace->nstrace_buf_offset;
+    guint32 nstrace_buflen = nstrace->nstrace_buflen;
     int bytes_read;
 
     *err = 0;
@@ -1048,7 +1066,7 @@ static gboolean nstrace_read_v10(wtap *wth, int *err, gchar **err_info, gint64 *
         nstrace_buf_offset = 0;
         nstrace->xxx_offset += nstrace_buflen;
         nstrace_buflen = GET_READ_PAGE_SIZE((nstrace->file_size - nstrace->xxx_offset));
-    }while((nstrace_buflen > 0) && (bytes_read = file_read(nstrace_buf, nstrace_buflen, wth->fh)) && (bytes_read == nstrace_buflen));
+    }while((nstrace_buflen > 0) && (bytes_read = file_read(nstrace_buf, nstrace_buflen, wth->fh)) > 0 && ((guint32)bytes_read == nstrace_buflen));
 
     return FALSE;
 }
@@ -1112,11 +1130,29 @@ static gboolean nstrace_read_v10(wtap *wth, int *err, gchar **err_info, gint64 *
 #define PACKET_DESCRIBE(phdr,FULLPART,ver,enumprefix,type,structname,HEADERVER)\
     do {\
         nspr_##structname##_t *fp= (nspr_##structname##_t*)&nstrace_buf[nstrace_buf_offset];\
+        /* Make sure the record header is entirely contained in the page */\
+        if ((nstrace_buflen - nstrace_buf_offset) < sizeof *fp) {\
+            *err = WTAP_ERR_BAD_FILE;\
+            *err_info = g_strdup("nstrace: record header crosses page boundary");\
+            return FALSE;\
+        }\
         (phdr)->rec_type = REC_TYPE_PACKET;\
         TIMEDEFV##ver((phdr),fp,type);\
         FULLPART##SIZEDEFV##ver((phdr),fp,ver);\
         TRACE_V##ver##_REC_LEN_OFF((phdr),enumprefix,type,structname);\
         (phdr)->pseudo_header.nstr.rec_type = NSPR_HEADER_VERSION##HEADERVER;\
+        /* Check sanity of record size */\
+        if ((phdr)->caplen < sizeof *fp) {\
+            *err = WTAP_ERR_BAD_FILE;\
+            *err_info = g_strdup("nstrace: record size is less than record header size");\
+            return FALSE;\
+        }\
+        /* Make sure the record is entirely contained in the page */\
+        if ((nstrace_buflen - nstrace_buf_offset) < (phdr)->caplen) {\
+            *err = WTAP_ERR_BAD_FILE;\
+            *err_info = g_strdup("nstrace: record crosses page boundary");\
+            return FALSE;\
+        }\
         ws_buffer_assure_space(wth->frame_buffer, (phdr)->caplen);\
         memcpy(ws_buffer_start_ptr(wth->frame_buffer), fp, (phdr)->caplen);\
         *data_offset = nstrace->xxx_offset + nstrace_buf_offset;\
@@ -1131,8 +1167,8 @@ static gboolean nstrace_read_v20(wtap *wth, int *err, gchar **err_info, gint64 *
     nstrace_t *nstrace = (nstrace_t *)wth->priv;
     guint64 nsg_creltime = nstrace->nsg_creltime;
     gchar *nstrace_buf = nstrace->pnstrace_buf;
-    gint32 nstrace_buf_offset = nstrace->nstrace_buf_offset;
-    gint32 nstrace_buflen = nstrace->nstrace_buflen;
+    guint32 nstrace_buf_offset = nstrace->nstrace_buf_offset;
+    guint32 nstrace_buflen = nstrace->nstrace_buflen;
     int bytes_read;
 
     *err = 0;
@@ -1228,7 +1264,7 @@ static gboolean nstrace_read_v20(wtap *wth, int *err, gchar **err_info, gint64 *
         nstrace_buf_offset = 0;
         nstrace->xxx_offset += nstrace_buflen;
         nstrace_buflen = GET_READ_PAGE_SIZE((nstrace->file_size - nstrace->xxx_offset));
-    }while((nstrace_buflen > 0) && (bytes_read = file_read(nstrace_buf, nstrace_buflen, wth->fh)) && (bytes_read == nstrace_buflen));
+    }while((nstrace_buflen > 0) && (bytes_read = file_read(nstrace_buf, nstrace_buflen, wth->fh)) > 0 && ((guint32)bytes_read == nstrace_buflen));
 
     return FALSE;
 }
@@ -1272,14 +1308,27 @@ static gboolean nstrace_read_v20(wtap *wth, int *err, gchar **err_info, gint64 *
 #define PACKET_DESCRIBE(phdr,FULLPART,ver,enumprefix,type,structname,HEADERVER)\
     do {\
         nspr_##structname##_t *fp = (nspr_##structname##_t *) &nstrace_buf[nstrace_buf_offset];\
+        /* Make sure the record header is entirely contained in the page */\
+        if ((nstrace->nstrace_buflen - nstrace_buf_offset) < sizeof *fp) {\
+            *err = WTAP_ERR_BAD_FILE;\
+            *err_info = g_strdup("nstrace: record header crosses page boundary");\
+            return FALSE;\
+        }\
         (phdr)->rec_type = REC_TYPE_PACKET;\
         TIMEDEFV##ver((phdr),fp,type);\
         FULLPART##SIZEDEFV##ver((phdr),fp,ver);\
         TRACE_V##ver##_REC_LEN_OFF((phdr),enumprefix,type,structname);\
         SETETHOFFSET_##ver(phdr)\
         (phdr)->pseudo_header.nstr.rec_type = NSPR_HEADER_VERSION##HEADERVER;\
+        /* Check sanity of record size */\
+        if ((phdr)->caplen < sizeof *fp) {\
+            *err = WTAP_ERR_BAD_FILE;\
+            *err_info = g_strdup("nstrace: record size is less than record header size");\
+            return FALSE;\
+        }\
         ws_buffer_assure_space(wth->frame_buffer, (phdr)->caplen);\
         *data_offset = nstrace->xxx_offset + nstrace_buf_offset;\
+        /* Copy record header */\
         while (nstrace_tmpbuff_off < nspr_##structname##_s) {\
             nstrace_tmpbuff[nstrace_tmpbuff_off++] = nstrace_buf[nstrace_buf_offset++];\
         }\
@@ -1287,12 +1336,15 @@ static gboolean nstrace_read_v20(wtap *wth, int *err, gchar **err_info, gint64 *
         rec_size = nst_dataSize - nstrace_tmpbuff_off;\
         nsg_nextPageOffset = ((nstrace_buf_offset + rec_size) >= (guint)nstrace->nstrace_buflen) ?\
         ((nstrace_buf_offset + rec_size) - (NSPR_PAGESIZE_TRACE - 1)) : 0;\
+        /* Copy record data */\
         while (nsg_nextPageOffset) {\
+            /* Copy everything from this page */\
             while (nstrace_buf_offset < nstrace->nstrace_buflen) {\
                 nstrace_tmpbuff[nstrace_tmpbuff_off++] = nstrace_buf[nstrace_buf_offset++];\
             }\
             nstrace->xxx_offset += nstrace_buflen;\
             nstrace_buflen = NSPR_PAGESIZE_TRACE;\
+            /* Read the next page */\
             bytes_read = file_read(nstrace_buf, NSPR_PAGESIZE_TRACE, wth->fh);\
             if ( !file_eof(wth->fh) && bytes_read != NSPR_PAGESIZE_TRACE) {\
                 return FALSE;\
@@ -1304,6 +1356,7 @@ static gboolean nstrace_read_v20(wtap *wth, int *err, gchar **err_info, gint64 *
             nsg_nextPageOffset = ((nstrace_buf_offset + rec_size) >= (guint)nstrace->nstrace_buflen) ?\
             ((nstrace_buf_offset + rec_size) - (NSPR_PAGESIZE_TRACE- 1)): 0;\
         } \
+        /* Copy the rest of the record */\
         while (nstrace_tmpbuff_off < nst_dataSize) {\
             nstrace_tmpbuff[nstrace_tmpbuff_off++] = nstrace_buf[nstrace_buf_offset++];\
         }\
@@ -1319,8 +1372,8 @@ static gboolean nstrace_read_v30(wtap *wth, int *err, gchar **err_info, gint64 *
     nstrace_t *nstrace = (nstrace_t *)wth->priv;
     guint64 nsg_creltime;
     gchar *nstrace_buf = nstrace->pnstrace_buf;
-    gint32 nstrace_buf_offset = nstrace->nstrace_buf_offset;
-    gint32 nstrace_buflen = nstrace->nstrace_buflen;
+    guint32 nstrace_buf_offset = nstrace->nstrace_buf_offset;
+    guint32 nstrace_buflen = nstrace->nstrace_buflen;
     guint8 nstrace_tmpbuff[65536];
     guint32 nstrace_tmpbuff_off=0,nst_dataSize=0,rec_size=0,nsg_nextPageOffset=0;
     nspr_hd_v20_t *hdp;
@@ -1336,7 +1389,7 @@ static gboolean nstrace_read_v30(wtap *wth, int *err, gchar **err_info, gint64 *
         if (!nstrace_buf[nstrace_buf_offset] && nstrace_buf_offset <= NSPR_PAGESIZE_TRACE){
             nstrace_buf_offset = NSPR_PAGESIZE_TRACE;
         }
-        if(file_eof(wth->fh) && bytes_read>0 ){
+        if (file_eof(wth->fh) && bytes_read > 0 && bytes_read < NSPR_PAGESIZE_TRACE){
             memset(&nstrace_buf[bytes_read], 0, NSPR_PAGESIZE_TRACE-bytes_read);
         }
         while ((nstrace_buf_offset < NSPR_PAGESIZE_TRACE) &&
@@ -1345,7 +1398,7 @@ static gboolean nstrace_read_v30(wtap *wth, int *err, gchar **err_info, gint64 *
             hdp = (nspr_hd_v20_t *) &nstrace_buf[nstrace_buf_offset];
             if(nspr_getv20recordsize(hdp) == 0){
               *err=WTAP_ERR_BAD_FILE;
-              *err_info = g_strdup("Zero size record found");
+              *err_info = g_strdup("nstrace: zero size record found");
               return FALSE;
             }
             switch (hdp->phd_RecordType)
@@ -1396,7 +1449,7 @@ static gboolean nstrace_read_v30(wtap *wth, int *err, gchar **err_info, gint64 *
         nstrace_buf_offset = 0;
         nstrace->xxx_offset += nstrace_buflen;
         nstrace_buflen = NSPR_PAGESIZE_TRACE;
-    } while((nstrace_buflen > 0) && (bytes_read = file_read(nstrace_buf, nstrace_buflen, wth->fh)) && (file_eof(wth->fh) || bytes_read == nstrace_buflen));
+    } while((nstrace_buflen > 0) && (bytes_read = file_read(nstrace_buf, nstrace_buflen, wth->fh)) > 0 && (file_eof(wth->fh) || (guint32)bytes_read == nstrace_buflen));
 
     return FALSE;
 }