CVE-2016-7983,7984/Don't use strchr() to scan packet data.

It can't be told to stop at the end of the packet data.  Add a
fn_printztn() that prints null-terminated strings, with a length check,
and which returns the number of bytes processed, or 0 if we ran out of
data.  That means it does the scanning we need, but safely.

Use it in the TFTP and BOOTP printers.

Fixes a heap overflow found with American Fuzzy Lop by Hanno Böck.

diff --git a/netdissect.h b/netdissect.h
index ea77e74d4632acfc992fdae46e482702e162605f..b04de0060de778fab3c42d9bc88f7bf8a3e6ec48 100644 (file)
--- a/netdissect.h
+++ b/netdissect.h
@@ -332,6 +332,7 @@ extern void relts_print(netdissect_options *, int);
 
 extern void fn_print_char(netdissect_options *, u_char);
 extern int fn_print(netdissect_options *, const u_char *, const u_char *);
+extern u_int fn_printztn(netdissect_options *ndo, const u_char *, u_int, const u_char *);
 extern int fn_printn(netdissect_options *, const u_char *, u_int, const u_char *);
 extern int fn_printzp(netdissect_options *, const u_char *, u_int, const u_char *);
 

diff --git a/print-bootp.c b/print-bootp.c
index d6be1d5b631a509f6067adce944713850d076b4e..26ac0de92c67c23296f4747cb0501875689db3c3 100644 (file)
--- a/print-bootp.c
+++ b/print-bootp.c
@@ -355,7 +355,8 @@ bootp_print(netdissect_options *ndo,
        ND_TCHECK2(bp->bp_sname[0], 1);         /* check first char only */
        if (*bp->bp_sname) {
                ND_PRINT((ndo, "\n\t  sname \""));
-               if (fn_print(ndo, bp->bp_sname, ndo->ndo_snapend)) {
+               if (fn_printztn(ndo, bp->bp_sname, (u_int)sizeof bp->bp_sname,
+                   ndo->ndo_snapend)) {
                        ND_PRINT((ndo, "\""));
                        ND_PRINT((ndo, "%s", tstr + 1));
                        return;
@@ -365,7 +366,8 @@ bootp_print(netdissect_options *ndo,
        ND_TCHECK2(bp->bp_file[0], 1);          /* check first char only */
        if (*bp->bp_file) {
                ND_PRINT((ndo, "\n\t  file \""));
-               if (fn_print(ndo, bp->bp_file, ndo->ndo_snapend)) {
+               if (fn_printztn(ndo, bp->bp_file, (u_int)sizeof bp->bp_file,
+                   ndo->ndo_snapend)) {
                        ND_PRINT((ndo, "\""));
                        ND_PRINT((ndo, "%s", tstr + 1));
                        return;

diff --git a/print-tftp.c b/print-tftp.c
index 5519c686a416daf3bbc94243443cd8f3f74b01d8..69bc601fdda4ed8be3ac4f5e2bb9e24098be8ce8 100644 (file)
--- a/print-tftp.c
+++ b/print-tftp.c
@@ -109,7 +109,8 @@ tftp_print(netdissect_options *ndo,
        register const struct tftphdr *tp;
        register const char *cp;
        register const u_char *p;
-       register int opcode, i;
+       register int opcode;
+       u_int ui;
 
        tp = (const struct tftphdr *)bp;
 
@@ -117,9 +118,12 @@ tftp_print(netdissect_options *ndo,
        ND_PRINT((ndo, " %d", length));
 
        /* Print tftp request type */
+       if (length < 2)
+               goto trunc;
        ND_TCHECK(tp->th_opcode);
        opcode = EXTRACT_16BITS(&tp->th_opcode);
        cp = tok2str(op2str, "tftp-#%d", opcode);
+       length -= 2;
        ND_PRINT((ndo, " %s", cp));
        /* Bail if bogus opcode */
        if (*cp == 't')
@@ -129,46 +133,80 @@ tftp_print(netdissect_options *ndo,
 
        case RRQ:
        case WRQ:
-       case OACK:
                p = (const u_char *)tp->th_stuff;
+               if (length == 0)
+                       goto trunc;
+               ND_PRINT((ndo, " "));
+               /* Print filename */
+               ND_PRINT((ndo, "\""));
+               ui = fn_printztn(ndo, p, length, ndo->ndo_snapend);
+               ND_PRINT((ndo, "\""));
+               if (ui == 0)
+                       goto trunc;
+               p += ui;
+               length -= ui;
+
+               /* Print the mode - RRQ and WRQ only */
+               if (length == 0)
+                       goto trunc;     /* no mode */
                ND_PRINT((ndo, " "));
-               /* Print filename or first option */
-               if (opcode != OACK)
-                       ND_PRINT((ndo, "\""));
-               i = fn_print(ndo, p, ndo->ndo_snapend);
-               if (opcode != OACK)
-                       ND_PRINT((ndo, "\""));
-
-               /* Print the mode (RRQ and WRQ only) and any options */
-               while ((p = (const u_char *)strchr((const char *)p, '\0')) != NULL) {
-                       if (length <= (u_int)(p - (const u_char *)&tp->th_block))
-                               break;
-                       p++;
-                       if (*p != '\0') {
+               ui = fn_printztn(ndo, p, length, ndo->ndo_snapend);
+               if (ui == 0)
+                       goto trunc;
+               p += ui;
+               length -= ui;
+
+               /* Print options, if any */
+               while (length != 0) {
+                       ND_TCHECK(*p);
+                       if (*p != '\0')
                                ND_PRINT((ndo, " "));
-                               fn_print(ndo, p, ndo->ndo_snapend);
-                       }
+                       ui = fn_printztn(ndo, p, length, ndo->ndo_snapend);
+                       if (ui == 0)
+                               goto trunc;
+                       p += ui;
+                       length -= ui;
                }
+               break;
 
-               if (i)
-                       goto trunc;
+       case OACK:
+               p = (const u_char *)tp->th_stuff;
+               /* Print options */
+               while (length != 0) {
+                       ND_TCHECK(*p);
+                       if (*p != '\0')
+                               ND_PRINT((ndo, " "));
+                       ui = fn_printztn(ndo, p, length, ndo->ndo_snapend);
+                       if (ui == 0)
+                               goto trunc;
+                       p += ui;
+                       length -= ui;
+               }
                break;
 
        case ACK:
        case DATA:
+               if (length < 2)
+                       goto trunc;     /* no block number */
                ND_TCHECK(tp->th_block);
                ND_PRINT((ndo, " block %d", EXTRACT_16BITS(&tp->th_block)));
                break;
 
        case TFTP_ERROR:
                /* Print error code string */
+               if (length < 2)
+                       goto trunc;     /* no error code */
                ND_TCHECK(tp->th_code);
-               ND_PRINT((ndo, " %s \"", tok2str(err2str, "tftp-err-#%d \"",
+               ND_PRINT((ndo, " %s", tok2str(err2str, "tftp-err-#%d \"",
                                       EXTRACT_16BITS(&tp->th_code))));
+               length -= 2;
                /* Print error message string */
-               i = fn_print(ndo, (const u_char *)tp->th_data, ndo->ndo_snapend);
+               if (length == 0)
+                       goto trunc;     /* no error message */
+               ND_PRINT((ndo, " \""));
+               ui = fn_printztn(ndo, (const u_char *)tp->th_data, length, ndo->ndo_snapend);
                ND_PRINT((ndo, "\""));
-               if (i)
+               if (ui == 0)
                        goto trunc;
                break;
 

diff --git a/tests/TESTLIST b/tests/TESTLIST
index eda358a69fb8043da253fd095c95a5d23808e6e8..1beb4a55e269ae41720a53762abf470e4aa6190a 100644 (file)
--- a/tests/TESTLIST
+++ b/tests/TESTLIST
@@ -400,3 +400,4 @@ stp-heapoverflow-4  stp-heapoverflow-4.pcap stp-heapoverflow-4.out  -t -v -n
 stp-heapoverflow-5     stp-heapoverflow-5.pcap stp-heapoverflow-5.out  -t -v -n
 arp-too-long-tha       arp-too-long-tha.pcap   arp-too-long-tha.out    -t -v -n
 juniper_header-heapoverflow    juniper_header-heapoverflow.pcap        juniper_header-heapoverflow.out -t -v -n
+tftp-heapoverflow      tftp-heapoverflow.pcap  tftp-heapoverflow.out   -t -v -n

diff --git a/tests/tftp-heapoverflow.out b/tests/tftp-heapoverflow.out
new file mode 100644 (file)
index 0000000..0d68d45
--- /dev/null
+++ b/tests/tftp-heapoverflow.out
@@ -0,0 +1,2 @@
+IP (tos 0x30, ttl 48, id 12336, offset 0, flags [DF], proto UDP (17), length 12336, bad cksum 3030 (->299d)!)
+    48.48.48.48.69 > 48.48.48.48.12336:  12308 RRQ "00" [|tftp]

diff --git a/tests/tftp-heapoverflow.pcap b/tests/tftp-heapoverflow.pcap
new file mode 100644 (file)
index 0000000..f1ecc73
Binary files /dev/null and b/tests/tftp-heapoverflow.pcap differ

diff --git a/util-print.c b/util-print.c
index 113f784660c8652d8da5bcddc8a347d617904a8a..7071ee8a41e401e7c840eb27ad5db91b1ed02a29 100644 (file)
--- a/util-print.c
+++ b/util-print.c
@@ -118,6 +118,56 @@ fn_print(netdissect_options *ndo,
        return(ret);
 }
 
+/*
+ * Print out a null-terminated filename (or other ascii string) from
+ * a fixed-length buffer.
+ * If ep is NULL, assume no truncation check is needed.
+ * Return the number of bytes of string processed, including the
+ * terminating null, if not truncated.  Return 0 if truncated.
+ */
+u_int
+fn_printztn(netdissect_options *ndo,
+         register const u_char *s, register u_int n, register const u_char *ep)
+{
+       register u_int bytes;
+       register u_char c;
+
+       bytes = 0;
+       for (;;) {
+               if (n == 0 || (ep != NULL && s >= ep)) {
+                       /*
+                        * Truncated.  This includes "no null before we
+                        * got to the end of the fixed-length buffer".
+                        *
+                        * XXX - BOOTP says "null-terminated", which
+                        * means the maximum length of the string, in
+                        * bytes, is 1 less than the size of the buffer,
+                        * as there must always be a terminating null.
+                        */
+                       bytes = 0;
+                       break;
+               }
+
+               c = *s++;
+               bytes++;
+               n--;
+               if (c == '\0') {
+                       /* End of string */
+                       break;
+               }
+               if (!ND_ISASCII(c)) {
+                       c = ND_TOASCII(c);
+                       ND_PRINT((ndo, "M-"));
+               }
+               if (!ND_ISPRINT(c)) {
+                       c ^= 0x40;      /* DEL to ?, others to alpha */
+                       ND_PRINT((ndo, "^"));
+               }
+               ND_PRINT((ndo, "%c", c));
+       }
+       return(bytes);
+}
+
 /*
  * Print out a counted filename (or other ascii string).
  * If ep is NULL, assume no truncation check is needed.