AppleTalk: Modernize packet parsing style.

Enable ND_LONGJMP_FROM_TCHECK. Report invalid packets as invalid. Remove
all improvised snapshot end guards as they were redundant. In
print_cstring() have nd_printjn() guard the snapshot end. Use tok2str()
in nbp_print(). Update two tests.

diff --git a/CHANGES b/CHANGES
index 20959f362a8044f39456041d87a2b25a7f07070f..b5611c8064afb7258d2816dce223d09bd29063a3 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -2,7 +2,7 @@ Monthday, Month DD, YYYY by gharris and denis
   Summary for 5.0.0 tcpdump release (so far!)
     Source code:
       Use %zu when printing a sizeof to squelch compiler warnings
-      BOOTP, EGP, EIGRP, Geneve, L2TP, OLSR, PGM, RSVP, UDP: Modernize packet parsing style
+      AppleTalk, BOOTP, EGP, EIGRP, Geneve, L2TP, OLSR, PGM, RSVP, UDP: Modernize packet parsing style
       EGP: Replace custom code with tok2str()
       EIGRP: Get the packet header fields right.
       UDP: Clean up address and port printing.

diff --git a/print-atalk.c b/print-atalk.c
index ae19139700c126a0cd0ecef5028cd19ebcac1a83..9cca576e4fd2aefe509011dd5cfb21dd9614a9d5 100644 (file)
--- a/print-atalk.c
+++ b/print-atalk.c
@@ -30,6 +30,7 @@
 #include <stdio.h>
 #include <string.h>
 
+#define ND_LONGJMP_FROM_TCHECK
 #include "netdissect.h"
 #include "addrtoname.h"
 #include "ethertype.h"
@@ -94,6 +95,12 @@ struct atNBPtuple {
 #define        nbpBrRq         0x10
 #define        nbpLkUp         0x20
 #define        nbpLkUpReply    0x30
+static const struct tok nbp_str[] = {
+       { nbpBrRq,      "brRq"  },
+       { nbpLkUp,      "lkup"  },
+       { nbpLkUpReply, "reply" },
+       { 0, NULL }
+};
 
 #define        ddpRTMP         1       /* RTMP type */
 #define        ddpNBP          2       /* NBP type */
@@ -128,10 +135,8 @@ static void atp_print(netdissect_options *, const struct atATP *, u_int);
 static void atp_bitmap_print(netdissect_options *, u_char);
 static void nbp_print(netdissect_options *, const struct atNBP *, u_int, u_short, u_char, u_char);
 static const struct atNBPtuple *nbp_tuple_print(netdissect_options *ndo, const struct atNBPtuple *,
-                                               const u_char *,
                                                u_short, u_char, u_char);
-static const struct atNBPtuple *nbp_name_print(netdissect_options *, const struct atNBPtuple *,
-                                              const u_char *);
+static const struct atNBPtuple *nbp_name_print(netdissect_options *, const struct atNBPtuple *);
 static const char *ataddr_string(netdissect_options *, u_short, u_char);
 static void ddp_print(netdissect_options *, const u_char *, u_int, u_int, u_short, u_char, u_char);
 static const char *ddpskt_string(netdissect_options *, u_int);
@@ -143,16 +148,8 @@ void
 ltalk_if_print(netdissect_options *ndo,
                const struct pcap_pkthdr *h, const u_char *p)
 {
-       u_int hdrlen;
-
        ndo->ndo_protocol = "ltalk";
-       hdrlen = llap_print(ndo, p, h->len);
-       if (hdrlen == 0) {
-               /* Cut short by the snapshot length. */
-               ndo->ndo_ll_hdr_len += h->caplen;
-               return;
-       }
-       ndo->ndo_ll_hdr_len += hdrlen;
+       ndo->ndo_ll_hdr_len += llap_print(ndo, p, h->len);
 }
 
 /*
@@ -170,12 +167,8 @@ llap_print(netdissect_options *ndo,
 
        ndo->ndo_protocol = "llap";
        if (length < sizeof(*lp)) {
-               ND_PRINT(" [|llap %u]", length);
-               return (length);
-       }
-       if (!ND_TTEST_LEN(bp, sizeof(*lp))) {
-               nd_print_trunc(ndo);
-               return (0);     /* cut short by the snapshot length */
+               ND_PRINT(" (LLAP length %u is too small)", length);
+               goto invalid;
        }
        lp = (const struct LAP *)bp;
        bp += sizeof(*lp);
@@ -184,13 +177,10 @@ llap_print(netdissect_options *ndo,
        switch (GET_U_1(lp->type)) {
 
        case lapShortDDP:
+               ndo->ndo_protocol = "sddp";
                if (length < ddpSSize) {
-                       ND_PRINT(" [|sddp %u]", length);
-                       return (length);
-               }
-               if (!ND_TTEST_LEN(bp, ddpSSize)) {
-                       ND_PRINT(" [|sddp]");
-                       return (0);     /* cut short by the snapshot length */
+                       ND_PRINT(" (SDDP length %u is too small)", length);
+                       goto invalid;
                }
                sdp = (const struct atShortDDP *)bp;
                ND_PRINT("%s.%s",
@@ -207,13 +197,10 @@ llap_print(netdissect_options *ndo,
                break;
 
        case lapDDP:
+               ndo->ndo_protocol = "ddp";
                if (length < ddpSize) {
-                       ND_PRINT(" [|ddp %u]", length);
-                       return (length);
-               }
-               if (!ND_TTEST_LEN(bp, ddpSize)) {
-                       ND_PRINT(" [|ddp]");
-                       return (0);     /* cut short by the snapshot length */
+                       ND_PRINT(" (DDP length %u is too small)", length);
+                       goto invalid;
                }
                dp = (const struct atDDP *)bp;
                snet = GET_BE_U_2(dp->srcNet);
@@ -237,6 +224,9 @@ llap_print(netdissect_options *ndo,
                break;
        }
        return (hdrlen);
+invalid:
+       nd_print_invalid(ndo);
+       return length;
 }
 
 /*
@@ -256,12 +246,8 @@ atalk_print(netdissect_options *ndo,
             ND_PRINT("AT ");
 
        if (length < ddpSize) {
-               ND_PRINT(" [|ddp %u]", length);
-               return;
-       }
-       if (!ND_TTEST_LEN(bp, ddpSize)) {
-               ND_PRINT(" [|ddp]");
-               return;
+               ND_PRINT(" (length %u is too small)", length);
+               goto invalid;
        }
        dp = (const struct atDDP *)bp;
        snet = GET_BE_U_2(dp->srcNet);
@@ -274,6 +260,9 @@ atalk_print(netdissect_options *ndo,
        length -= ddpSize;
        ddp_print(ndo, bp, length, GET_U_1(dp->type), snet,
                  GET_U_1(dp->srcNode), GET_U_1(dp->srcSkt));
+       return;
+invalid:
+       nd_print_invalid(ndo);
 }
 
 /* XXX should probably pass in the snap header and do checks like arp_print() */
@@ -283,20 +272,18 @@ aarp_print(netdissect_options *ndo,
 {
        const struct aarp *ap;
 
-#define AT(member) ataddr_string(ndo, (ap->member[1]<<8)|ap->member[2],ap->member[3])
+#define AT(member) ataddr_string(ndo, \
+       (GET_U_1(&ap->member[1])<<8)|GET_U_1(&ap->member[2]), \
+       GET_U_1(&ap->member[3]))
 
        ndo->ndo_protocol = "aarp";
        ND_PRINT("aarp ");
        ap = (const struct aarp *)bp;
-       if (!ND_TTEST_SIZE(ap)) {
-               /* Just bail if we don't have the whole chunk. */
-               nd_print_trunc(ndo);
-               return;
-       }
        if (length < sizeof(*ap)) {
-               ND_PRINT(" [|aarp %u]", length);
-               return;
+               ND_PRINT(" (length %u is too small)", length);
+               goto invalid;
        }
+       ND_TCHECK_SIZE(ap);
        if (GET_BE_U_2(ap->htype) == 1 &&
            GET_BE_U_2(ap->ptype) == ETHERTYPE_ATALK &&
            GET_U_1(ap->halen) == MAC_ADDR_LEN && GET_U_1(ap->palen) == 4)
@@ -317,6 +304,9 @@ aarp_print(netdissect_options *ndo,
        ND_PRINT("len %u op %u htype %u ptype %#x halen %u palen %u",
            length, GET_BE_U_2(ap->op), GET_BE_U_2(ap->htype),
            GET_BE_U_2(ap->ptype), GET_U_1(ap->halen), GET_U_1(ap->palen));
+       return;
+invalid:
+       nd_print_invalid(ndo);
 }
 
 /*
@@ -355,14 +345,10 @@ atp_print(netdissect_options *ndo,
        uint8_t control;
        uint32_t data;
 
-       if ((const u_char *)(ap + 1) > ndo->ndo_snapend) {
-               /* Just bail if we don't have the whole chunk. */
-               nd_print_trunc(ndo);
-               return;
-       }
+       ndo->ndo_protocol = "atp";
        if (length < sizeof(*ap)) {
-               ND_PRINT(" [|atp %u]", length);
-               return;
+               ND_PRINT(" (ATP length %u is too small)", length);
+               goto invalid;
        }
        length -= sizeof(*ap);
        control = GET_U_1(ap->control);
@@ -444,6 +430,9 @@ atp_print(netdissect_options *ndo,
        data = GET_BE_U_4(ap->userData);
        if (data != 0)
                ND_PRINT(" 0x%x", data);
+       return;
+invalid:
+       nd_print_invalid(ndo);
 }
 
 static void
@@ -486,37 +475,22 @@ nbp_print(netdissect_options *ndo,
                (const struct atNBPtuple *)((const u_char *)np + nbpHeaderSize);
        uint8_t control;
        u_int i;
-       const u_char *ep;
-
-       if (length < nbpHeaderSize) {
-               ND_PRINT(" truncated-nbp %u", length);
-               return;
-       }
 
-       length -= nbpHeaderSize;
-       if (length < 8) {
+       if (length < nbpHeaderSize + 8) {
                /* must be room for at least one tuple */
-               ND_PRINT(" truncated-nbp %u", length + nbpHeaderSize);
-               return;
-       }
-       /* ep points to end of available data */
-       ep = ndo->ndo_snapend;
-       if ((const u_char *)tp > ep) {
-               nd_print_trunc(ndo);
-               return;
+               ND_PRINT(" undersized-nbp %u", length);
+               goto invalid;
        }
+       length -= nbpHeaderSize;
        control = GET_U_1(np->control);
-       switch (i = (control & 0xf0)) {
+       ND_PRINT(" nbp-%s", tok2str(nbp_str, "0x%x", control & 0xf0));
+       ND_PRINT(" %u", GET_U_1(np->id));
+       switch (control & 0xf0) {
 
        case nbpBrRq:
        case nbpLkUp:
-               ND_PRINT(i == nbpLkUp? " nbp-lkup %u:":" nbp-brRq %u:",
-                        GET_U_1(np->id));
-               if ((const u_char *)(tp + 1) > ep) {
-                       nd_print_trunc(ndo);
-                       return;
-               }
-               (void)nbp_name_print(ndo, tp, ep);
+               ND_PRINT(":");
+               (void)nbp_name_print(ndo, tp);
                /*
                 * look for anomalies: the spec says there can only
                 * be one tuple, the address must match the source
@@ -536,63 +510,50 @@ nbp_print(netdissect_options *ndo,
                break;
 
        case nbpLkUpReply:
-               ND_PRINT(" nbp-reply %u:", GET_U_1(np->id));
+               ND_PRINT(":");
 
                /* print each of the tuples in the reply */
                for (i = control & 0xf; i != 0 && tp; i--)
-                       tp = nbp_tuple_print(ndo, tp, ep, snet, snode, skt);
+                       tp = nbp_tuple_print(ndo, tp, snet, snode, skt);
                break;
 
        default:
-               ND_PRINT(" nbp-0x%x  %u (%u)", control, GET_U_1(np->id),
-                        length);
+               ND_PRINT("  (%u)", length);
                break;
        }
+       return;
+invalid:
+       nd_print_invalid(ndo);
 }
 
 /* print a counted string */
 static const u_char *
 print_cstring(netdissect_options *ndo,
-              const u_char *cp, const u_char *ep)
+              const u_char *cp)
 {
        u_int length;
 
-       if (cp >= ep) {
-               nd_print_trunc(ndo);
-               return (0);
-       }
        length = GET_U_1(cp);
        cp++;
 
        /* Spec says string can be at most 32 bytes long */
        if (length > 32) {
                ND_PRINT("[len=%u]", length);
-               return (0);
+               ND_TCHECK_LEN(cp, length);
+               return NULL;
        }
-       while (length != 0) {
-               if (cp >= ep) {
-                       nd_print_trunc(ndo);
-                       return (0);
-               }
-               fn_print_char(ndo, GET_U_1(cp));
-               cp++;
-               length--;
-       }
-       return (cp);
+       nd_printjn(ndo, cp, length);
+       return cp + length;
 }
 
 static const struct atNBPtuple *
 nbp_tuple_print(netdissect_options *ndo,
-                const struct atNBPtuple *tp, const u_char *ep,
+                const struct atNBPtuple *tp,
                 u_short snet, u_char snode, u_char skt)
 {
        const struct atNBPtuple *tpn;
 
-       if ((const u_char *)(tp + 1) > ep) {
-               nd_print_trunc(ndo);
-               return 0;
-       }
-       tpn = nbp_name_print(ndo, tp, ep);
+       tpn = nbp_name_print(ndo, tp);
 
        /* if the enumerator isn't 1, print it */
        if (GET_U_1(tp->enumerator) != 1)
@@ -613,7 +574,7 @@ nbp_tuple_print(netdissect_options *ndo,
 
 static const struct atNBPtuple *
 nbp_name_print(netdissect_options *ndo,
-               const struct atNBPtuple *tp, const u_char *ep)
+               const struct atNBPtuple *tp)
 {
        const u_char *cp = (const u_char *)tp + nbpTupleSize;
 
@@ -621,13 +582,13 @@ nbp_name_print(netdissect_options *ndo,
 
        /* Object */
        ND_PRINT("\"");
-       if ((cp = print_cstring(ndo, cp, ep)) != NULL) {
+       if ((cp = print_cstring(ndo, cp)) != NULL) {
                /* Type */
                ND_PRINT(":");
-               if ((cp = print_cstring(ndo, cp, ep)) != NULL) {
+               if ((cp = print_cstring(ndo, cp)) != NULL) {
                        /* Zone */
                        ND_PRINT("@");
-                       if ((cp = print_cstring(ndo, cp, ep)) != NULL)
+                       if ((cp = print_cstring(ndo, cp)) != NULL)
                                ND_PRINT("\"");
                }
        }

diff --git a/tests/heapoverflow-EXTRACT_16BITS.out b/tests/heapoverflow-EXTRACT_16BITS.out
index 33e973b5681f339837cb9bccea17f85b6b89ee5d..982db42a31b282df875de5a45e5d43d81e7a8ce2 100644 (file)
--- a/tests/heapoverflow-EXTRACT_16BITS.out
+++ b/tests/heapoverflow-EXTRACT_16BITS.out
@@ -1 +1 @@
-    1  05:27:12.808464432 et1 AT  [|ddp]
+    1  05:27:12.808464432 et1 AT  [|atalk]

diff --git a/tests/heapoverflow-atalk_print.out b/tests/heapoverflow-atalk_print.out
index 33e973b5681f339837cb9bccea17f85b6b89ee5d..982db42a31b282df875de5a45e5d43d81e7a8ce2 100644 (file)
--- a/tests/heapoverflow-atalk_print.out
+++ b/tests/heapoverflow-atalk_print.out
@@ -1 +1 @@
-    1  05:27:12.808464432 et1 AT  [|ddp]
+    1  05:27:12.808464432 et1 AT  [|atalk]