subject:"\[patch\] tcpdump segfault on invalid DECnet packet"

Re: [patch] tcpdump segfault on invalid DECnet packet

2015-10-24 Thread Sebastian Benoit

Stuart Henderson(st...@openbsd.org) on 2015.10.20 16:37:58 +0100:
> On 2015/10/14 11:11, Kevin Reay wrote:
> > Thanks for the review and feedback.
> > Updated patch with removed whitespace changes included.
> 
> This is fine with me. Any OKs to commit it?

yes, ok
 
> > Index: print-decnet.c
> > ===
> > RCS file: /cvs/src/usr.sbin/tcpdump/print-decnet.c,v
> > retrieving revision 1.14
> > diff -u -p -r1.14 print-decnet.c
> > --- print-decnet.c  21 Aug 2015 02:07:32 -  1.14
> > +++ print-decnet.c  14 Oct 2015 22:49:03 -
> > @@ -44,13 +44,13 @@ struct rtentry;
> >  #include "addrtoname.h"
> >  
> >  /* Forwards */
> > -static void print_decnet_ctlmsg(const union routehdr *, u_int);
> > +static int print_decnet_ctlmsg(const union routehdr *, u_int, u_int);
> >  static void print_t_info(int);
> > -static void print_l1_routes(const char *, u_int);
> > -static void print_l2_routes(const char *, u_int);
> > +static int print_l1_routes(const char *, u_int);
> > +static int print_l2_routes(const char *, u_int);
> >  static void print_i_info(int);
> > -static void print_elist(const char *, u_int);
> > -static void print_nsp(const u_char *, u_int);
> > +static int print_elist(const char *, u_int);
> > +static int print_nsp(const u_char *, u_int);
> >  static void print_reason(int);
> >  #ifdef PRINT_NSPDATA
> >  static void pdata(u_char *, int);
> > @@ -76,12 +76,23 @@ decnet_print(register const u_char *ap, 
> > return;
> > }
> >  
> > +   TCHECK2(*ap, sizeof(short));
> > pktlen = EXTRACT_LE_16BITS(ap);
> > +   if (pktlen < sizeof(struct shorthdr)) {
> > +   (void)printf("[|decnet]");
> > +   return;
> > +   }
> > +   if (pktlen > length) {
> > +   (void)printf("[|decnet]");
> > +   return;
> > +   }
> > +   length = pktlen;
> >  
> > rhlen = min(length, caplen);
> > rhlen = min(rhlen, sizeof(*rhp));
> > memcpy((char *)rhp, (char *)&(ap[sizeof(short)]), rhlen);
> >  
> > +   TCHECK(rhp->rh_short.sh_flags);
> > mflags = EXTRACT_LE_8BITS(rhp->rh_short.sh_flags);
> >  
> > if (mflags & RMF_PAD) {
> > @@ -89,6 +100,11 @@ decnet_print(register const u_char *ap, 
> > u_int padlen = mflags & RMF_PADMASK;
> > if (vflag)
> > (void) printf("[pad:%d] ", padlen);
> > +   if (length < padlen + 2) {
> > +   (void)printf("[|decnet]");
> > +   return;
> > +   }
> > +   TCHECK2(ap[sizeof(short)], padlen);
> > ap += padlen;
> > length -= padlen;
> > caplen -= padlen;
> > @@ -100,38 +116,43 @@ decnet_print(register const u_char *ap, 
> >  
> > if (mflags & RMF_FVER) {
> > (void) printf("future-version-decnet");
> > -   default_print(ap, length);
> > +   default_print(ap, min(length, caplen));
> > return;
> > }
> >  
> > /* is it a control message? */
> > if (mflags & RMF_CTLMSG) {
> > -   print_decnet_ctlmsg(rhp, min(length, caplen));
> > +   if(!print_decnet_ctlmsg(rhp, length, caplen))
> > +   goto trunc;
> > return;
> > }
> >  
> > switch (mflags & RMF_MASK) {
> > case RMF_LONG:
> > +   if (length < sizeof(struct longhdr)) {
> > +   (void)printf("[|decnet]");
> > +   return;
> > +   }
> > +   TCHECK(rhp->rh_long);
> > dst =
> > EXTRACT_LE_16BITS(rhp->rh_long.lg_dst.dne_remote.dne_nodeaddr);
> > src =
> > EXTRACT_LE_16BITS(rhp->rh_long.lg_src.dne_remote.dne_nodeaddr);
> > hops = EXTRACT_LE_8BITS(rhp->rh_long.lg_visits);
> > nspp = &(ap[sizeof(short) + sizeof(struct longhdr)]);
> > -   nsplen = min((length - sizeof(struct longhdr)),
> > -(caplen - sizeof(struct longhdr)));
> > +   nsplen = length - sizeof(struct longhdr);
> > break;
> > case RMF_SHORT:
> > +   TCHECK(rhp->rh_short);
> > dst = EXTRACT_LE_16BITS(rhp->rh_short.sh_dst);
> > src = EXTRACT_LE_16BITS(rhp->rh_short.sh_src);
> > hops = (EXTRACT_LE_8BITS(rhp->rh_short.sh_visits) & VIS_MASK)+1;
> > nspp = &(ap[sizeof(short) + sizeof(struct shorthdr)]);
> > -   nsplen = min((length - sizeof(struct shorthdr)),
> > -(caplen - sizeof(struct shorthdr)));
> > +   nsplen = length - sizeof(struct shorthdr);
> > break;
> > default:
> > (void) printf("unknown message flags under mask");
> > -   default_print((u_char *)ap, length);
> > +   default_print((u_char *)ap, min(length, caplen));
> > return;
> > }
> >  
> > @@ -147,11 +168,18 @@ decnet_print(register const u_char *ap, 
> > (void)printf("%d hops ", hops);
> > }
> >  
> > -   print_nsp(nspp, nsplen);
> > +   if (!print_nsp(nspp, nsplen))
> > +   goto trunc;
> > +   return;
> > +
> > +trunc:
> > +   (void)printf("[|decnet]");
> > +   return;
> >  }
> >

Re: [patch] tcpdump segfault on invalid DECnet packet

2015-10-14 Thread Kevin Reay

Thanks for the review and feedback.
Updated patch with removed whitespace changes included.

On Wed, Oct 14, 2015 at 11:55:58AM +0100, Stuart Henderson wrote:
> unnecessary whitespace change (new one is wrong)
Index: print-decnet.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/print-decnet.c,v
retrieving revision 1.14
diff -u -p -r1.14 print-decnet.c
--- print-decnet.c  21 Aug 2015 02:07:32 -  1.14
+++ print-decnet.c  14 Oct 2015 22:49:03 -
@@ -44,13 +44,13 @@ struct rtentry;
 #include "addrtoname.h"
 
 /* Forwards */
-static void print_decnet_ctlmsg(const union routehdr *, u_int);
+static int print_decnet_ctlmsg(const union routehdr *, u_int, u_int);
 static void print_t_info(int);
-static void print_l1_routes(const char *, u_int);
-static void print_l2_routes(const char *, u_int);
+static int print_l1_routes(const char *, u_int);
+static int print_l2_routes(const char *, u_int);
 static void print_i_info(int);
-static void print_elist(const char *, u_int);
-static void print_nsp(const u_char *, u_int);
+static int print_elist(const char *, u_int);
+static int print_nsp(const u_char *, u_int);
 static void print_reason(int);
 #ifdef PRINT_NSPDATA
 static void pdata(u_char *, int);
@@ -76,12 +76,23 @@ decnet_print(register const u_char *ap, 
return;
}
 
+   TCHECK2(*ap, sizeof(short));
pktlen = EXTRACT_LE_16BITS(ap);
+   if (pktlen < sizeof(struct shorthdr)) {
+   (void)printf("[|decnet]");
+   return;
+   }
+   if (pktlen > length) {
+   (void)printf("[|decnet]");
+   return;
+   }
+   length = pktlen;
 
rhlen = min(length, caplen);
rhlen = min(rhlen, sizeof(*rhp));
memcpy((char *)rhp, (char *)&(ap[sizeof(short)]), rhlen);
 
+   TCHECK(rhp->rh_short.sh_flags);
mflags = EXTRACT_LE_8BITS(rhp->rh_short.sh_flags);
 
if (mflags & RMF_PAD) {
@@ -89,6 +100,11 @@ decnet_print(register const u_char *ap, 
u_int padlen = mflags & RMF_PADMASK;
if (vflag)
(void) printf("[pad:%d] ", padlen);
+   if (length < padlen + 2) {
+   (void)printf("[|decnet]");
+   return;
+   }
+   TCHECK2(ap[sizeof(short)], padlen);
ap += padlen;
length -= padlen;
caplen -= padlen;
@@ -100,38 +116,43 @@ decnet_print(register const u_char *ap, 
 
if (mflags & RMF_FVER) {
(void) printf("future-version-decnet");
-   default_print(ap, length);
+   default_print(ap, min(length, caplen));
return;
}
 
/* is it a control message? */
if (mflags & RMF_CTLMSG) {
-   print_decnet_ctlmsg(rhp, min(length, caplen));
+   if(!print_decnet_ctlmsg(rhp, length, caplen))
+   goto trunc;
return;
}
 
switch (mflags & RMF_MASK) {
case RMF_LONG:
+   if (length < sizeof(struct longhdr)) {
+   (void)printf("[|decnet]");
+   return;
+   }
+   TCHECK(rhp->rh_long);
dst =
EXTRACT_LE_16BITS(rhp->rh_long.lg_dst.dne_remote.dne_nodeaddr);
src =
EXTRACT_LE_16BITS(rhp->rh_long.lg_src.dne_remote.dne_nodeaddr);
hops = EXTRACT_LE_8BITS(rhp->rh_long.lg_visits);
nspp = &(ap[sizeof(short) + sizeof(struct longhdr)]);
-   nsplen = min((length - sizeof(struct longhdr)),
-(caplen - sizeof(struct longhdr)));
+   nsplen = length - sizeof(struct longhdr);
break;
case RMF_SHORT:
+   TCHECK(rhp->rh_short);
dst = EXTRACT_LE_16BITS(rhp->rh_short.sh_dst);
src = EXTRACT_LE_16BITS(rhp->rh_short.sh_src);
hops = (EXTRACT_LE_8BITS(rhp->rh_short.sh_visits) & VIS_MASK)+1;
nspp = &(ap[sizeof(short) + sizeof(struct shorthdr)]);
-   nsplen = min((length - sizeof(struct shorthdr)),
-(caplen - sizeof(struct shorthdr)));
+   nsplen = length - sizeof(struct shorthdr);
break;
default:
(void) printf("unknown message flags under mask");
-   default_print((u_char *)ap, length);
+   default_print((u_char *)ap, min(length, caplen));
return;
}
 
@@ -147,11 +168,18 @@ decnet_print(register const u_char *ap, 
(void)printf("%d hops ", hops);
}
 
-   print_nsp(nspp, nsplen);
+   if (!print_nsp(nspp, nsplen))
+   goto trunc;
+   return;
+
+trunc:
+   (void)printf("[|decnet]");
+   return;
 }
 
-static void
-print_decnet_ctlmsg(register const union routehdr *rhp, u_int length)
+static int
+print_decnet_ctlmsg(register const union routehdr *rhp, u_int length,
+u_int caplen)
 {
int mflags =

Re: [patch] tcpdump segfault on invalid DECnet packet

2015-10-14 Thread Stuart Henderson

On 2015/10/11 05:02, Kevin Reay wrote:
> Fix a tcpdump segfault when attempting to print an invalid DECnet
> packet.
> 
> DECnet packet printing code could cause a segfault on an impossibly
> large packet from a specifically crafted packet.
> 
> The segfault would occur in tcpdump.c:default_print() called by
> print-decnet.c:decnet_print().
> 
> Patch built using changes from upstream repo; Git commit:
> f61639179282f8e796966b21e45f79db317d7bdb
> https://github.com/the-tcpdump-group/tcpdump/commit/f61639179282f8e796
> 966b21e45f79db317d7bdb
> 
> Changes adapted to older version of tcpdump in tree. Patch adds
> various length/size checks including TCHECK* throughout. Brings
> print-decnet.c length validation in-line with other print-*.c files.
> Additionally adds return values to internal print_* functions to
> indicate failure of new checks.
> 
> Does not include the header no-copy optimization that was in the
> upstream patch.
> 
> Feedback is greatly appreciated. Let me know if there are any
> questions, or if troublesome pcaps and backtrack are wanted.
> 
> Tested with various pcaps. Behavior now matches newer upstream version
> on Linux.

couple of nits:

> @@ -564,7 +678,7 @@ print_nsp(const u_char *nspp, u_int nspl
>   case COS_SEGMENT:
>   (void)printf("seg ");
>   break;
> - case COS_MESSAGE:
> +  case COS_MESSAGE:

unnecessary whitespace change (new one is wrong)

>   (void)printf("msg ");
>   break;
>   case COS_CRYPTSER:
> @@ -573,7 +687,7 @@ print_nsp(const u_char *nspp, u_int nspl
>   }
>   switch (info & COI_MASK) {
>   case COI_32:
> - (void)printf("ver 3.2 ");
> + (void)printf("ver 3.2 ");

same

otherwise OK with me.

[patch] tcpdump segfault on invalid DECnet packet

2015-10-11 Thread Kevin Reay

Fix a tcpdump segfault when attempting to print an invalid DECnet
packet.

DECnet packet printing code could cause a segfault on an impossibly
large packet from a specifically crafted packet.

The segfault would occur in tcpdump.c:default_print() called by
print-decnet.c:decnet_print().

Patch built using changes from upstream repo; Git commit:
f61639179282f8e796966b21e45f79db317d7bdb
https://github.com/the-tcpdump-group/tcpdump/commit/f61639179282f8e796
966b21e45f79db317d7bdb

Changes adapted to older version of tcpdump in tree. Patch adds
various length/size checks including TCHECK* throughout. Brings
print-decnet.c length validation in-line with other print-*.c files.
Additionally adds return values to internal print_* functions to
indicate failure of new checks.

Does not include the header no-copy optimization that was in the
upstream patch.

Feedback is greatly appreciated. Let me know if there are any
questions, or if troublesome pcaps and backtrack are wanted.

Tested with various pcaps. Behavior now matches newer upstream version
on Linux.


Index: print-decnet.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/print-decnet.c,v
retrieving revision 1.14
diff -u -p -u -p -r1.14 print-decnet.c
--- print-decnet.c  21 Aug 2015 02:07:32 -  1.14
+++ print-decnet.c  11 Oct 2015 10:40:22 -
@@ -44,13 +44,13 @@ struct rtentry;
 #include "addrtoname.h"
 
 /* Forwards */
-static void print_decnet_ctlmsg(const union routehdr *, u_int);
+static int print_decnet_ctlmsg(const union routehdr *, u_int, u_int);
 static void print_t_info(int);
-static void print_l1_routes(const char *, u_int);
-static void print_l2_routes(const char *, u_int);
+static int print_l1_routes(const char *, u_int);
+static int print_l2_routes(const char *, u_int);
 static void print_i_info(int);
-static void print_elist(const char *, u_int);
-static void print_nsp(const u_char *, u_int);
+static int print_elist(const char *, u_int);
+static int print_nsp(const u_char *, u_int);
 static void print_reason(int);
 #ifdef PRINT_NSPDATA
 static void pdata(u_char *, int);
@@ -76,12 +76,23 @@ decnet_print(register const u_char *ap, 
return;
}
 
+   TCHECK2(*ap, sizeof(short));
pktlen = EXTRACT_LE_16BITS(ap);
+   if (pktlen < sizeof(struct shorthdr)) {
+   (void)printf("[|decnet]");
+   return;
+   }
+   if (pktlen > length) {
+   (void)printf("[|decnet]");
+   return;
+   }
+   length = pktlen;
 
rhlen = min(length, caplen);
rhlen = min(rhlen, sizeof(*rhp));
memcpy((char *)rhp, (char *)&(ap[sizeof(short)]), rhlen);
 
+   TCHECK(rhp->rh_short.sh_flags);
mflags = EXTRACT_LE_8BITS(rhp->rh_short.sh_flags);
 
if (mflags & RMF_PAD) {
@@ -89,6 +100,11 @@ decnet_print(register const u_char *ap, 
u_int padlen = mflags & RMF_PADMASK;
if (vflag)
(void) printf("[pad:%d] ", padlen);
+   if (length < padlen + 2) {
+   (void)printf("[|decnet]");
+   return;
+   }
+   TCHECK2(ap[sizeof(short)], padlen);
ap += padlen;
length -= padlen;
caplen -= padlen;
@@ -100,38 +116,43 @@ decnet_print(register const u_char *ap, 
 
if (mflags & RMF_FVER) {
(void) printf("future-version-decnet");
-   default_print(ap, length);
+   default_print(ap, min(length, caplen));
return;
}
 
/* is it a control message? */
if (mflags & RMF_CTLMSG) {
-   print_decnet_ctlmsg(rhp, min(length, caplen));
+   if(!print_decnet_ctlmsg(rhp, length, caplen))
+   goto trunc;
return;
}
 
switch (mflags & RMF_MASK) {
case RMF_LONG:
+   if (length < sizeof(struct longhdr)) {
+   (void)printf("[|decnet]");
+   return;
+   }
+   TCHECK(rhp->rh_long);
dst =
EXTRACT_LE_16BITS(rhp->rh_long.lg_dst.dne_remote.dne_nodeaddr);
src =
EXTRACT_LE_16BITS(rhp->rh_long.lg_src.dne_remote.dne_nodeaddr);
hops = EXTRACT_LE_8BITS(rhp->rh_long.lg_visits);
nspp = &(ap[sizeof(short) + sizeof(struct longhdr)]);
-   nsplen = min((length - sizeof(struct longhdr)),
-(caplen - sizeof(struct longhdr)));
+   nsplen = length - sizeof(struct longhdr);
break;
case RMF_SHORT:
+   TCHECK(rhp->rh_short);
dst = EXTRACT_LE_16BITS(rhp->rh_short.sh_dst);
src = EXTRACT_LE_16BITS(rhp->rh_short.sh_src);
hops = (EXTRACT_LE_8BITS(rhp->rh_short.sh_visits) & VIS_MASK)+1;
nspp = &(ap[sizeof(short) + sizeof(struct shorthdr)]);
-   nsplen = min((length - sizeof(struct shorthdr)),
-

Re: [patch] tcpdump segfault on invalid DECnet packet

Re: [patch] tcpdump segfault on invalid DECnet packet

Re: [patch] tcpdump segfault on invalid DECnet packet

[patch] tcpdump segfault on invalid DECnet packet

4 matches

Site Navigation

Mail list logo

Footer information