o Overhaul filtering, adding facilities to jump over rules and to

  negate the sense of rules.
o Remove the redundant (and undocumented) ``host'' and ``port''
  words (README.changes updated).
o Don't permit (and ignore) garbage instead of the protocol.

Mostly submitted by:  Peter Jeremy <jeremyp@gsmx07.alcatel.com.au>

7 files changed, 431 insertions, 297 deletions

diff --git a/usr.sbin/ppp/README.changes b/usr.sbin/ppp/README.changes
index d70c053..a66f90f 100644
--- a/usr.sbin/ppp/README.changes
+++ b/usr.sbin/ppp/README.changes
@@ -85,3 +85,5 @@ o The ``set device'' command now expects each device to be specified as an
   on commas and spaces.
 o The ``show modem'' command is depricated and has been changed to
   ``show physical''.
+o The words ``host'' and ``port'' are no longer accepted by the ``set filter''
+  command.  Removing them should yield the same results as before.
diff --git a/usr.sbin/ppp/bundle.c b/usr.sbin/ppp/bundle.c
index 27e51d6..3f871fb 100644
--- a/usr.sbin/ppp/bundle.c
+++ b/usr.sbin/ppp/bundle.c
@@ -23,7 +23,7 @@
  * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  *
- *	$Id: bundle.c,v 1.56 1999/06/02 00:46:50 brian Exp $
+ *	$Id: bundle.c,v 1.57 1999/06/22 11:31:42 brian Exp $
  */
 
 #include <sys/param.h>
@@ -854,6 +854,15 @@ bundle_Create(const char *prefix, int type, const char **argv)
   bundle.filter.dial.logok = 1;
   bundle.filter.alive.name = "ALIVE";
   bundle.filter.alive.logok = 1;
+  {
+    int	i;
+    for (i = 0; i < MAXFILTERS; i++) {
+        bundle.filter.in.rule[i].f_action = A_NONE;
+        bundle.filter.out.rule[i].f_action = A_NONE;
+        bundle.filter.dial.rule[i].f_action = A_NONE;
+        bundle.filter.alive.rule[i].f_action = A_NONE;
+    }
+  }
   memset(&bundle.idle.timer, '\0', sizeof bundle.idle.timer);
   bundle.idle.done = 0;
   bundle.notify.fd = -1;
diff --git a/usr.sbin/ppp/filter.c b/usr.sbin/ppp/filter.c
index aefb43a..08f9552 100644
--- a/usr.sbin/ppp/filter.c
+++ b/usr.sbin/ppp/filter.c
@@ -17,7 +17,7 @@
  * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
  * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
  *
- * $Id: filter.c,v 1.30 1999/06/23 16:48:21 brian Exp $
+ * $Id: filter.c,v 1.31 1999/07/26 11:15:10 brian Exp $
  *
  *	TODO: Shoud send ICMP error message when we discard packets.
  */
@@ -110,14 +110,14 @@ ParseAddr(struct ipcp *ipcp, const char *data,
     s[len] = '\0';
     if (inet_aton(s, paddr) == 0) {
       log_Printf(LogWARN, "ParseAddr: %s: Bad address\n", s);
-      return (0);
+      return 0;
     }
   }
   if (cp && *++cp) {
     bits = strtol(cp, &wp, 0);
     if (cp == wp || bits < 0 || bits > 32) {
       log_Printf(LogWARN, "ParseAddr: bad mask width.\n");
-      return (0);
+      return 0;
     }
   } else if (paddr->s_addr == INADDR_ANY)
     /* An IP of 0.0.0.0 without a width is anything */
@@ -136,7 +136,7 @@ ParseAddr(struct ipcp *ipcp, const char *data,
       *pmask = bits2mask(bits);
   }
 
-  return (1);
+  return 1;
 }
 
 static int
@@ -160,15 +160,15 @@ ParsePort(const char *service, int proto)
 
   servent = getservbyname(service, protocol_name);
   if (servent != 0)
-    return (ntohs(servent->s_port));
+    return ntohs(servent->s_port);
 
   port = strtol(service, &cp, 0);
   if (cp == service) {
     log_Printf(LogWARN, "ParsePort: %s is not a port name or number.\n",
 	      service);
-    return (0);
+    return 0;
   }
-  return (port);
+  return port;
 }
 
 /*
@@ -183,7 +183,7 @@ ParseIcmp(int argc, char const *const *argv, struct filterent *tgt)
   switch (argc) {
   case 0:
     /* permit/deny all ICMP types */
-    tgt->opt.srcop = OP_NONE;
+    tgt->f_srcop = OP_NONE;
     break;
 
   case 3:
@@ -191,18 +191,18 @@ ParseIcmp(int argc, char const *const *argv, struct filterent *tgt)
       type = strtol(argv[2], &cp, 0);
       if (cp == argv[2]) {
 	log_Printf(LogWARN, "ParseIcmp: type is expected.\n");
-	return (0);
+	return 0;
       }
-      tgt->opt.srcop = OP_EQ;
-      tgt->opt.srcport = type;
+      tgt->f_srcop = OP_EQ;
+      tgt->f_srcport = type;
     }
     break;
 
   default:
     log_Printf(LogWARN, "ParseIcmp: bad icmp syntax.\n");
-    return (0);
+    return 0;
   }
-  return (1);
+  return 1;
 }
 
 /*
@@ -212,31 +212,31 @@ static int
 ParseUdpOrTcp(int argc, char const *const *argv, int proto,
               struct filterent *tgt)
 {
-  tgt->opt.srcop = tgt->opt.dstop = OP_NONE;
-  tgt->opt.estab = tgt->opt.syn = tgt->opt.finrst = 0;
+  tgt->f_srcop = tgt->f_dstop = OP_NONE;
+  tgt->f_estab = tgt->f_syn = tgt->f_finrst = 0;
 
   if (argc >= 3 && !strcmp(*argv, "src")) {
-    tgt->opt.srcop = filter_Nam2Op(argv[1]);
-    if (tgt->opt.srcop == OP_NONE) {
+    tgt->f_srcop = filter_Nam2Op(argv[1]);
+    if (tgt->f_srcop == OP_NONE) {
       log_Printf(LogWARN, "ParseUdpOrTcp: bad operation\n");
-      return (0);
+      return 0;
     }
-    tgt->opt.srcport = ParsePort(argv[2], proto);
-    if (tgt->opt.srcport == 0)
-      return (0);
+    tgt->f_srcport = ParsePort(argv[2], proto);
+    if (tgt->f_srcport == 0)
+      return 0;
     argc -= 3;
     argv += 3;
   }
 
   if (argc >= 3 && !strcmp(argv[0], "dst")) {
-    tgt->opt.dstop = filter_Nam2Op(argv[1]);
-    if (tgt->opt.dstop == OP_NONE) {
+    tgt->f_dstop = filter_Nam2Op(argv[1]);
+    if (tgt->f_dstop == OP_NONE) {
       log_Printf(LogWARN, "ParseUdpOrTcp: bad operation\n");
-      return (0);
+      return 0;
     }
-    tgt->opt.dstport = ParsePort(argv[2], proto);
-    if (tgt->opt.dstport == 0)
-      return (0);
+    tgt->f_dstport = ParsePort(argv[2], proto);
+    if (tgt->f_dstport == 0)
+      return 0;
     argc -= 3;
     argv += 3;
   }
@@ -244,11 +244,11 @@ ParseUdpOrTcp(int argc, char const *const *argv, int proto,
   if (proto == P_TCP) {
     for (; argc > 0; argc--, argv++)
       if (!strcmp(*argv, "estab"))
-        tgt->opt.estab = 1;
+        tgt->f_estab = 1;
       else if (!strcmp(*argv, "syn"))
-        tgt->opt.syn = 1;
+        tgt->f_syn = 1;
       else if (!strcmp(*argv, "finrst"))
-        tgt->opt.finrst = 1;
+        tgt->f_finrst = 1;
       else
         break;
   }
@@ -261,14 +261,15 @@ ParseUdpOrTcp(int argc, char const *const *argv, int proto,
   return 1;
 }
 
-static int ParseIgmp(int argc, char const * const *argv, struct filterent *tgt) {
+static int ParseIgmp(int argc, char const * const *argv, struct filterent *tgt)
+{
   /* Filter currently is a catch-all. Requests are either permitted or
      dropped. */
   if (argc != 0) {
     log_Printf(LogWARN, "ParseIgmp: Too many parameters\n");
     return 0;
   } else
-    tgt->opt.srcop = OP_NONE;
+    tgt->f_srcop = OP_NONE;
 
   return 1;
 }
@@ -296,91 +297,111 @@ addrstr(struct in_addr addr, unsigned type)
   return inet_ntoa(addr);
 }
 
+static const char *
+maskstr(int bits)
+{
+  static char str[4];
+
+  if (bits == 32)
+    *str = '\0';
+  else
+    snprintf(str, sizeof str, "/%d", bits);
+
+  return str;
+}
+
 static int
 Parse(struct ipcp *ipcp, int argc, char const *const *argv,
       struct filterent *ofp)
 {
   int action, proto;
-  int val;
+  int val, ruleno;
   char *wp;
   struct filterent filterdata;
 
-  val = strtol(*argv, &wp, 0);
-  if (*argv == wp || val >= MAXFILTERS) {
+  ruleno = strtol(*argv, &wp, 0);
+  if (*argv == wp || ruleno >= MAXFILTERS) {
     log_Printf(LogWARN, "Parse: invalid filter number.\n");
-    return (0);
+    return 0;
   }
-  if (val < 0) {
-    for (val = 0; val < MAXFILTERS; val++) {
-      ofp->action = A_NONE;
+  if (ruleno < 0) {
+    for (ruleno = 0; ruleno < MAXFILTERS; ruleno++) {
+      ofp->f_action = A_NONE;
       ofp++;
     }
     log_Printf(LogWARN, "Parse: filter cleared.\n");
-    return (1);
+    return 1;
   }
-  ofp += val;
+  ofp += ruleno;
 
   if (--argc == 0) {
     log_Printf(LogWARN, "Parse: missing action.\n");
-    return (0);
+    return 0;
   }
   argv++;
 
   proto = P_NONE;
   memset(&filterdata, '\0', sizeof filterdata);
 
-  if (!strcmp(*argv, "permit")) {
+  val = strtol(*argv, &wp, 0);
+  if (!*wp && val >= 0 && val < MAXFILTERS) {
+    if (val <= ruleno) {
+      log_Printf(LogWARN, "Parse: Can only jump forward from rule %d\n",
+                 ruleno);
+      return 0;
+    }
+    action = val;
+  } else if (!strcmp(*argv, "permit")) {
     action = A_PERMIT;
   } else if (!strcmp(*argv, "deny")) {
     action = A_DENY;
   } else if (!strcmp(*argv, "clear")) {
-    ofp->action = A_NONE;
-    return (1);
+    ofp->f_action = A_NONE;
+    return 1;
   } else {
     log_Printf(LogWARN, "Parse: bad action: %s\n", *argv);
-    return (0);
+    return 0;
   }
-  filterdata.action = action;
+  filterdata.f_action = action;
 
   argc--;
   argv++;
 
-  if (argc && filterdata.action == A_DENY) {
-    if (!strcmp(*argv, "host")) {
-      filterdata.action |= A_UHOST;
-      argc--;
-      argv++;
-    } else if (!strcmp(*argv, "port")) {
-      filterdata.action |= A_UPORT;
-      argc--;
-      argv++;
-    }
+  if (argc && argv[0][0] == '!' && !argv[0][1]) {
+    filterdata.f_invert = 1;
+    argc--;
+    argv++;
   }
 
   proto = filter_Nam2Proto(argc, argv);
   if (proto == P_NONE) {
     if (!argc)
       log_Printf(LogWARN, "Parse: address/mask is expected.\n");
-    else if (ParseAddr(ipcp, *argv, &filterdata.src.ipaddr,
-                       &filterdata.src.mask, &filterdata.src.width)) {
-      filterdata.srctype = addrtype(*argv);
+    else if (ParseAddr(ipcp, *argv, &filterdata.f_src.ipaddr,
+                       &filterdata.f_src.mask, &filterdata.f_src.width)) {
+      filterdata.f_srctype = addrtype(*argv);
       argc--;
       argv++;
       proto = filter_Nam2Proto(argc, argv);
       if (!argc)
         log_Printf(LogWARN, "Parse: address/mask is expected.\n");
       else if (proto == P_NONE) {
-	if (ParseAddr(ipcp, *argv, &filterdata.dst.ipaddr, &filterdata.dst.mask,
-                      &filterdata.dst.width)) {
-          filterdata.dsttype = addrtype(*argv);
+	if (ParseAddr(ipcp, *argv, &filterdata.f_dst.ipaddr,
+		      &filterdata.f_dst.mask, &filterdata.f_dst.width)) {
+          filterdata.f_dsttype = addrtype(*argv);
 	  argc--;
 	  argv++;
 	} else
-          filterdata.dsttype = T_ADDR;
-	proto = filter_Nam2Proto(argc, argv);
-	if (argc && proto != P_NONE) {
-	  argc--;
-	  argv++;
+          filterdata.f_dsttype = T_ADDR;
+        if (argc) {
+	  proto = filter_Nam2Proto(argc, argv);
+	  if (proto == P_NONE) {
+            log_Printf(LogWARN, "Parse: %s: Invalid protocol\n", *argv);
+            return 0;
+          } else {
+	    argc--;
+	    argv++;
+	  }
 	}
       } else {
 	argc--;
@@ -388,7 +409,7 @@ Parse(struct ipcp *ipcp, int argc, char const *const *argv,
       }
     } else {
       log_Printf(LogWARN, "Parse: Address/protocol expected.\n");
-      return (0);
+      return 0;
     }
   } else {
     argc--;
@@ -396,7 +417,7 @@ Parse(struct ipcp *ipcp, int argc, char const *const *argv,
   }
 
   val = 1;
-  filterdata.proto = proto;
+  filterdata.f_proto = proto;
 
   switch (proto) {
   case P_TCP:
@@ -413,23 +434,24 @@ Parse(struct ipcp *ipcp, int argc, char const *const *argv,
     break;
   }
 
-  log_Printf(LogDEBUG, "Parse: Src: %s\n", inet_ntoa(filterdata.src.ipaddr));
-  log_Printf(LogDEBUG, "Parse: Src mask: %s\n", inet_ntoa(filterdata.src.mask));
-  log_Printf(LogDEBUG, "Parse: Dst: %s\n", inet_ntoa(filterdata.dst.ipaddr));
-  log_Printf(LogDEBUG, "Parse: Dst mask: %s\n", inet_ntoa(filterdata.dst.mask));
+  log_Printf(LogDEBUG, "Parse: Src: %s\n", inet_ntoa(filterdata.f_src.ipaddr));
+  log_Printf(LogDEBUG, "Parse: Src mask: %s\n", inet_ntoa(filterdata.f_src.mask));
+  log_Printf(LogDEBUG, "Parse: Dst: %s\n", inet_ntoa(filterdata.f_dst.ipaddr));
+  log_Printf(LogDEBUG, "Parse: Dst mask: %s\n", inet_ntoa(filterdata.f_dst.mask));
   log_Printf(LogDEBUG, "Parse: Proto = %d\n", proto);
 
   log_Printf(LogDEBUG, "Parse: src:  %s (%d)\n",
-            filter_Op2Nam(filterdata.opt.srcop), filterdata.opt.srcport);
+            filter_Op2Nam(filterdata.f_srcop), filterdata.f_srcport);
   log_Printf(LogDEBUG, "Parse: dst:  %s (%d)\n",
-            filter_Op2Nam(filterdata.opt.dstop), filterdata.opt.dstport);
-  log_Printf(LogDEBUG, "Parse: estab: %u\n", filterdata.opt.estab);
-  log_Printf(LogDEBUG, "Parse: syn: %u\n", filterdata.opt.syn);
-  log_Printf(LogDEBUG, "Parse: finrst: %u\n", filterdata.opt.finrst);
+            filter_Op2Nam(filterdata.f_dstop), filterdata.f_dstport);
+  log_Printf(LogDEBUG, "Parse: estab: %u\n", filterdata.f_estab);
+  log_Printf(LogDEBUG, "Parse: syn: %u\n", filterdata.f_syn);
+  log_Printf(LogDEBUG, "Parse: finrst: %u\n", filterdata.f_finrst);
 
   if (val)
     *ofp = filterdata;
-  return (val);
+
+  return val;
 }
 
 int
@@ -462,8 +484,16 @@ filter_Set(struct cmdargs const *arg)
 const char *
 filter_Action2Nam(int act)
 {
-  static const char *actname[] = { "none   ", "permit ", "deny   " };
-  return actname[act & (A_PERMIT|A_DENY)];
+  static const char *actname[] = { "  none ", "permit ", "  deny " };
+  static char	buf[8];
+
+  if (act >= 0 && act < MAXFILTERS) {
+    snprintf(buf, 8, "%6d ", act);
+    return buf;
+  } else if (act >= A_NONE && act < A_NONE + sizeof(actname)/sizeof(char *))
+    return actname[act - A_NONE];
+  else
+    return "?????? ";
 }
 
 static void
@@ -472,32 +502,27 @@ doShowFilter(struct filterent *fp, struct prompt *prompt)
   int n;
 
   for (n = 0; n < MAXFILTERS; n++, fp++) {
-    if (fp->action != A_NONE) {
-      prompt_Printf(prompt, "  %2d %s", n, filter_Action2Nam(fp->action));
-      if (fp->action & A_UHOST)
-        prompt_Printf(prompt, "host ");
-      else if (fp->action & A_UPORT)
-        prompt_Printf(prompt, "port ");
-      else
-        prompt_Printf(prompt, "     ");
-      prompt_Printf(prompt, "%s/%d ", addrstr(fp->src.ipaddr, fp->srctype),
-                    fp->src.width);
-      prompt_Printf(prompt, "%s/%d ", addrstr(fp->dst.ipaddr, fp->dsttype),
-                    fp->dst.width);
-      if (fp->proto) {
-	prompt_Printf(prompt, "%s", filter_Proto2Nam(fp->proto));
-
-	if (fp->opt.srcop)
-	  prompt_Printf(prompt, " src %s %d", filter_Op2Nam(fp->opt.srcop),
-		  fp->opt.srcport);
-	if (fp->opt.dstop)
-	  prompt_Printf(prompt, " dst %s %d", filter_Op2Nam(fp->opt.dstop),
-		  fp->opt.dstport);
-	if (fp->opt.estab)
+    if (fp->f_action != A_NONE) {
+      prompt_Printf(prompt, "  %2d %s", n, filter_Action2Nam(fp->f_action));
+      prompt_Printf(prompt, "%c ", fp->f_invert ? '!' : ' ');
+      prompt_Printf(prompt, "%s%s ", addrstr(fp->f_src.ipaddr, fp->f_srctype),
+                    maskstr(fp->f_src.width));
+      prompt_Printf(prompt, "%s%s ", addrstr(fp->f_dst.ipaddr, fp->f_dsttype),
+                    maskstr(fp->f_dst.width));
+      if (fp->f_proto) {
+	prompt_Printf(prompt, "%s", filter_Proto2Nam(fp->f_proto));
+
+	if (fp->f_srcop)
+	  prompt_Printf(prompt, " src %s %d", filter_Op2Nam(fp->f_srcop),
+		  fp->f_srcport);
+	if (fp->f_dstop)
+	  prompt_Printf(prompt, " dst %s %d", filter_Op2Nam(fp->f_dstop),
+		  fp->f_dstport);
+	if (fp->f_estab)
 	  prompt_Printf(prompt, " estab");
-	if (fp->opt.syn)
+	if (fp->f_syn)
 	  prompt_Printf(prompt, " syn");
-	if (fp->opt.finrst)
+	if (fp->f_finrst)
 	  prompt_Printf(prompt, " finrst");
       }
       prompt_Printf(prompt, "\n");
@@ -600,18 +625,18 @@ filter_AdjustAddr(struct filter *filter, struct in_addr *my_ip,
   int n;
 
   for (fp = filter->rule, n = 0; n < MAXFILTERS; fp++, n++)
-    if (fp->action != A_NONE) {
+    if (fp->f_action != A_NONE) {
       if (my_ip) {
-        if (fp->srctype == T_MYADDR)
-          fp->src.ipaddr = *my_ip;
-        if (fp->dsttype == T_MYADDR)
-          fp->dst.ipaddr = *my_ip;
+        if (fp->f_srctype == T_MYADDR)
+          fp->f_src.ipaddr = *my_ip;
+        if (fp->f_dsttype == T_MYADDR)
+          fp->f_dst.ipaddr = *my_ip;
       }
       if (peer_ip) {
-        if (fp->srctype == T_HISADDR)
-          fp->src.ipaddr = *peer_ip;
-        if (fp->dsttype == T_HISADDR)
-          fp->dst.ipaddr = *peer_ip;
+        if (fp->f_srctype == T_HISADDR)
+          fp->f_src.ipaddr = *peer_ip;
+        if (fp->f_dsttype == T_HISADDR)
+          fp->f_dst.ipaddr = *peer_ip;
       }
     }
 }
diff --git a/usr.sbin/ppp/filter.h b/usr.sbin/ppp/filter.h
index e830990..16c15f7 100644
--- a/usr.sbin/ppp/filter.h
+++ b/usr.sbin/ppp/filter.h
@@ -15,56 +15,66 @@
  * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
  * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
  *
- * $Id: filter.h,v 1.15 1999/05/31 23:57:37 brian Exp $
+ * $Id: filter.h,v 1.16 1999/06/23 16:48:22 brian Exp $
  *
  *	TODO:
  */
 
-/* Actions */
-#define	A_NONE		0
-#define	A_PERMIT	1
-#define	A_DENY		2
-#define	A_MASK		3
-#define	A_UHOST		4
-#define	A_UPORT		8
-
-/* Known protocols */
+/* Known protocols - f_proto */
 #define	P_NONE	0
 #define	P_TCP	1
 #define	P_UDP	2
 #define	P_ICMP	3
 #define P_IGMP	4
 
-/* Operations */
+/* Operations - f_srcop, f_dstop */
 #define	OP_NONE	0
 #define	OP_EQ	1
 #define	OP_GT	2
-#define	OP_LT	4
+#define	OP_LT	3
 
 /* srctype or dsttype */
 #define T_ADDR		0
 #define T_MYADDR	1
 #define T_HISADDR	2
 
+/*
+ * There's a struct filterent for each possible filter rule.  The
+ * layout is designed to minimise size (there are 4 * MAXFILTERS of
+ * them) - which is also conveniently a power of 2 (32 bytes) on
+ * architectures where sizeof(int)==4 (this makes indexing faster).
+ *
+ * f_action and f_proto only need to be 6 and 3 bits, respectively,
+ * but making them 8 bits allows them to be efficently accessed using
+ * byte operations as well as allowing space for future expansion
+ * (expanding MAXFILTERS or converting f_proto IPPROTO_... values).
+ *
+ * Note that there are four free bits in the initial word for future
+ * extensions.
+ */
 struct filterent {
-  int action;			/* Filtering action */
-  unsigned srctype : 2;		/* T_ value of src */
-  struct in_range src;		/* Source address */
-  unsigned dsttype : 2;		/* T_ value of dst */
-  struct in_range dst;		/* Destination address */
-  int proto;			/* Protocol */
-  struct {
-    short srcop;
-    u_short srcport;
-    short dstop;
-    u_short dstport;
-    unsigned estab : 1;
-    unsigned syn : 1;
-    unsigned finrst : 1;
-  } opt;
+  unsigned f_action : 8;		/* Filtering action: goto or A_... */
+  unsigned f_proto : 8;		/* Protocol: P_... */
+  unsigned f_srcop : 2;		/* Source port operation: OP_... */
+  unsigned f_dstop : 2;		/* Destination port operation: OP_... */
+  unsigned f_srctype : 2;	/* T_ value of src */
+  unsigned f_dsttype : 2;	/* T_ value of dst */
+  unsigned f_estab : 1;		/* Check TCP ACK bit */
+  unsigned f_syn : 1;		/* Check TCP SYN bit */
+  unsigned f_finrst : 1;	/* Check TCP FIN/RST bits */
+  unsigned f_invert : 1;	/* true to complement match */
+  struct in_range f_src;	/* Source address and mask */
+  struct in_range f_dst;	/* Destination address and mask */
+  u_short f_srcport;		/* Source port, compared with f_srcop */
+  u_short f_dstport;		/* Destination port, compared with f_dstop */
 };
 
-#define	MAXFILTERS		40	/* in each filter set */
+#define	MAXFILTERS	40	/* in each filter set */
+
+/* f_action values [0..MAXFILTERS) specify the next filter rule, others are: */
+#define	A_NONE		(MAXFILTERS)
+#define	A_PERMIT	(A_NONE+1)
+#define	A_DENY		(A_PERMIT+1)
 
 struct filter {
   struct filterent rule[MAXFILTERS];	/* incoming packet filter */
@@ -73,6 +83,7 @@ struct filter {
   unsigned logok : 1;
 };
 
+/* Which filter set */
 #define FL_IN		0
 #define FL_OUT		1
 #define FL_DIAL		2
diff --git a/usr.sbin/ppp/ip.c b/usr.sbin/ppp/ip.c
index a674e43..8ea2e73 100644
--- a/usr.sbin/ppp/ip.c
+++ b/usr.sbin/ppp/ip.c
@@ -17,7 +17,7 @@
  * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
  * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
  *
- * $Id: ip.c,v 1.63 1999/06/02 15:59:00 brian Exp $
+ * $Id: ip.c,v 1.64 1999/06/23 16:48:23 brian Exp $
  *
  *	TODO:
  *		o Return ICMP message for filterd packet
@@ -79,11 +79,11 @@ static const u_short interactive_ports[32] = {
 
 static const char *TcpFlags[] = { "FIN", "SYN", "RST", "PSH", "ACK", "URG" };
 
-static int
+static __inline int
 PortMatch(int op, u_short pport, u_short rport)
 {
   switch (op) {
-    case OP_EQ:
+  case OP_EQ:
     return (pport == rport);
   case OP_GT:
     return (pport > rport);
@@ -96,142 +96,193 @@ PortMatch(int op, u_short pport, u_short rport)
 
 /*
  *  Check a packet against a defined filter
+ *  Returns 0 to accept the packet, non-zero to drop the packet
+ *
+ *  If filtering is enabled, the initial fragment of a datagram must
+ *  contain the complete protocol header, and subsequent fragments
+ *  must not attempt to over-write it.
  */
 static int
-FilterCheck(struct ip *pip, struct filter *filter)
+FilterCheck(const struct ip *pip, const struct filter *filter)
 {
-  int gotinfo, cproto, estab, syn, finrst, n, len, didname;
-  struct tcphdr *th;
-  struct udphdr *uh;
-  struct icmp *ih;
-  char *ptop;
-  u_short sport, dport;
-  struct filterent *fp = filter->rule;
+  int gotinfo;			/* true if IP payload decoded */
+  int cproto;			/* P_* protocol type if (gotinfo) */
+  int estab, syn, finrst;	/* TCP state flags if (gotinfo) */
+  u_short sport, dport;		/* src, dest port from packet if (gotinfo) */
+  int n;			/* filter rule to process */
+  int len;			/* bytes used in dbuff */
+  int didname;			/* true if filter header printed */
+  int match;			/* true if condition matched */
+  const struct filterent *fp = filter->rule;
   char dbuff[100];
 
-  if (fp->action) {
-    cproto = gotinfo = estab = syn = finrst = didname = 0;
-    sport = dport = 0;
-    for (n = 0; n < MAXFILTERS; n++) {
-      if (fp->action != A_NONE) {
-	/* permit fragments on in and out filter */
-        if (filter->fragok && (ntohs(pip->ip_off) & IP_OFFMASK) != 0)
-	  return (A_PERMIT);
-
-        if (!didname)
-          log_Printf(LogDEBUG, "%s filter:\n", filter->name);
-        didname = 1;
-
-	if ((pip->ip_src.s_addr & fp->src.mask.s_addr) ==
-	    (fp->src.ipaddr.s_addr & fp->src.mask.s_addr) &&
-	    (pip->ip_dst.s_addr & fp->dst.mask.s_addr) ==
-	    (fp->dst.ipaddr.s_addr & fp->dst.mask.s_addr)) {
-	  if (fp->proto) {
-	    if (!gotinfo) {
-	      ptop = (char *) pip + (pip->ip_hl << 2);
-
-	      switch (pip->ip_p) {
-	      case IPPROTO_ICMP:
-		cproto = P_ICMP;
-		ih = (struct icmp *) ptop;
-		sport = ih->icmp_type;
-		estab = syn = finrst = -1;
-                if (log_IsKept(LogDEBUG))
-		  snprintf(dbuff, sizeof dbuff, "sport = %d", sport);
-		break;
-	      case IPPROTO_IGMP:
-		cproto = P_IGMP;
-		estab = syn = finrst = -1;
-		sport = ntohs(0);
-		break;
-	      case IPPROTO_UDP:
-	      case IPPROTO_IPIP:
-		cproto = P_UDP;
-		uh = (struct udphdr *) ptop;
-		sport = ntohs(uh->uh_sport);
-		dport = ntohs(uh->uh_dport);
-		estab = syn = finrst = -1;
-                if (log_IsKept(LogDEBUG))
-		  snprintf(dbuff, sizeof dbuff, "sport = %d, dport = %d",
-                           sport, dport);
-		break;
-	      case IPPROTO_TCP:
-		cproto = P_TCP;
-		th = (struct tcphdr *) ptop;
-		sport = ntohs(th->th_sport);
-		dport = ntohs(th->th_dport);
-		estab = (th->th_flags & TH_ACK);
-		syn = (th->th_flags & TH_SYN);
-		finrst = (th->th_flags & (TH_FIN|TH_RST));
-                if (log_IsKept(LogDEBUG)) {
-                  if (!estab)
-		    snprintf(dbuff, sizeof dbuff,
-                             "flags = %02x, sport = %d, dport = %d",
-                             th->th_flags, sport, dport);
-                  else
-                    *dbuff = '\0';
-                }
-		break;
-	      default:
-		return (A_DENY);       /* We'll block unknown type of packet */
-	      }
-              if (log_IsKept(LogDEBUG)) {
-                if (estab != -1) {
-                  len = strlen(dbuff);
-                  snprintf(dbuff + len, sizeof dbuff - len,
-                           ", estab = %d, syn = %d, finrst = %d",
-                           estab, syn, finrst);
-                }
-	        log_Printf(LogDEBUG, " Filter: proto = %s, %s\n",
-                          filter_Proto2Nam(cproto), dbuff);
-              }
-	      gotinfo = 1;
+  if (fp->f_action == A_NONE)
+    return (0);		/* No rule is given. Permit this packet */
+
+  /* Deny any packet fragment that tries to over-write the header.
+   * Since we no longer have the real header available, punt on the
+   * largest normal header - 20 bytes for TCP without options, rounded
+   * up to the next possible fragment boundary.  Since the smallest
+   * `legal' MTU is 576, and the smallest recommended MTU is 296, any
+   * fragmentation within this range is dubious at best */
+  len = ntohs(pip->ip_off) & IP_OFFMASK;	/* fragment offset */ 
+  if (len > 0) {		/* Not first fragment within datagram */
+    if (len < (24 >> 3))	/* don't allow fragment to over-write header */
+      return (1);
+    /* permit fragments on in and out filter */
+    return (filter->fragok);
+  }
+  
+  cproto = gotinfo = estab = syn = finrst = didname = 0;
+  sport = dport = 0;
+  for (n = 0; n < MAXFILTERS; ) {
+    if (fp->f_action == A_NONE) {
+      n++;
+      fp++;
+      continue;
+    }
+
+    if (!didname) {
+      log_Printf(LogDEBUG, "%s filter:\n", filter->name);
+      didname = 1;
+    }
+
+    match = 0;
+    if (!((pip->ip_src.s_addr ^ fp->f_src.ipaddr.s_addr) &
+	  fp->f_src.mask.s_addr) &&
+	!((pip->ip_dst.s_addr ^ fp->f_dst.ipaddr.s_addr) &
+	  fp->f_dst.mask.s_addr)) {
+      if (fp->f_proto != P_NONE) {
+	if (!gotinfo) {
+	  const char *ptop = (const char *) pip + (pip->ip_hl << 2);
+	  const struct tcphdr *th;
+	  const struct udphdr *uh;
+	  const struct icmp *ih;
+	  int datalen;	/* IP datagram length */
+
+	  datalen = ntohs(pip->ip_len) - (pip->ip_hl << 2);
+	  switch (pip->ip_p) {
+	  case IPPROTO_ICMP:
+	    cproto = P_ICMP;
+	    if (datalen < 8)	/* ICMP must be at least 8 octets */
+	      return (1);
+	    ih = (const struct icmp *) ptop;
+	    sport = ih->icmp_type;
+	    estab = syn = finrst = -1;
+	    if (log_IsKept(LogDEBUG))
+	      snprintf(dbuff, sizeof dbuff, "sport = %d", sport);
+	    break;
+	  case IPPROTO_IGMP:
+	    cproto = P_IGMP;
+	    if (datalen < 8)	/* IGMP uses 8-octet messages */
+	      return (1);
+	    estab = syn = finrst = -1;
+	    sport = ntohs(0);
+	    break;
+	  case IPPROTO_UDP:
+	  case IPPROTO_IPIP:
+	    cproto = P_UDP;
+	    if (datalen < 8)	/* UDP header is 8 octets */
+	      return (1);
+	    uh = (const struct udphdr *) ptop;
+	    sport = ntohs(uh->uh_sport);
+	    dport = ntohs(uh->uh_dport);
+	    estab = syn = finrst = -1;
+	    if (log_IsKept(LogDEBUG))
+	      snprintf(dbuff, sizeof dbuff, "sport = %d, dport = %d",
+		       sport, dport);
+	    break;
+	  case IPPROTO_TCP:
+	    cproto = P_TCP;
+	    th = (const struct tcphdr *) ptop;
+	    /* TCP headers are variable length.  The following code
+	     * ensures that the TCP header length isn't de-referenced if
+	     * the datagram is too short
+	     */
+	    if (datalen < 20 || datalen < (th->th_off << 2))
+	      return (1);
+	    sport = ntohs(th->th_sport);
+	    dport = ntohs(th->th_dport);
+	    estab = (th->th_flags & TH_ACK);
+	    syn = (th->th_flags & TH_SYN);
+	    finrst = (th->th_flags & (TH_FIN|TH_RST));
+	    if (log_IsKept(LogDEBUG)) {
+	      if (!estab)
+		snprintf(dbuff, sizeof dbuff,
+			 "flags = %02x, sport = %d, dport = %d",
+			 th->th_flags, sport, dport);
+	      else
+		*dbuff = '\0';
 	    }
-            if (log_IsKept(LogDEBUG)) {
-	      if (fp->opt.srcop != OP_NONE) {
-                snprintf(dbuff, sizeof dbuff, ", src %s %d",
-                         filter_Op2Nam(fp->opt.srcop), fp->opt.srcport);
-                len = strlen(dbuff);
-              } else
-                len = 0;
-	      if (fp->opt.dstop != OP_NONE) {
-                snprintf(dbuff + len, sizeof dbuff - len,
-                         ", dst %s %d", filter_Op2Nam(fp->opt.dstop),
-                         fp->opt.dstport);
-              } else if (!len)
-                *dbuff = '\0';
-
-	      log_Printf(LogDEBUG, "  rule = %d: Address match, "
-                        "check against proto %s%s, action = %s\n",
-                        n, filter_Proto2Nam(fp->proto),
-                        dbuff, filter_Action2Nam(fp->action));
-            }
-
-	    if (cproto == fp->proto) {
-	      if ((fp->opt.srcop == OP_NONE ||
-		   PortMatch(fp->opt.srcop, sport, fp->opt.srcport)) &&
-		  (fp->opt.dstop == OP_NONE ||
-		   PortMatch(fp->opt.dstop, dport, fp->opt.dstport)) &&
-		  (fp->opt.estab == 0 || estab) &&
-		  (fp->opt.syn == 0 || syn) &&
-		  (fp->opt.finrst == 0 || finrst)) {
-		return (fp->action);
-	      }
+	    break;
+	  default:
+	    return (1);	/* We'll block unknown type of packet */
+	  }
+
+	  if (log_IsKept(LogDEBUG)) {
+	    if (estab != -1) {
+	      len = strlen(dbuff);
+	      snprintf(dbuff + len, sizeof dbuff - len,
+		       ", estab = %d, syn = %d, finrst = %d",
+		       estab, syn, finrst);
 	    }
-	  } else {
-	    /* Address is mached. Make a decision. */
-	    log_Printf(LogDEBUG, "  rule = %d: Address match, action = %s\n", n,
-                      filter_Action2Nam(fp->action));
-	    return (fp->action);
+	    log_Printf(LogDEBUG, " Filter: proto = %s, %s\n",
+		       filter_Proto2Nam(cproto), dbuff);
+	  }
+	  gotinfo = 1;
+	}
+	if (log_IsKept(LogDEBUG)) {
+	  if (fp->f_srcop != OP_NONE) {
+	    snprintf(dbuff, sizeof dbuff, ", src %s %d",
+		     filter_Op2Nam(fp->f_srcop), fp->f_srcport);
+	    len = strlen(dbuff);
+	  } else
+	    len = 0;
+	  if (fp->f_dstop != OP_NONE) {
+	    snprintf(dbuff + len, sizeof dbuff - len,
+		     ", dst %s %d", filter_Op2Nam(fp->f_dstop),
+		     fp->f_dstport);
+	  } else if (!len)
+	    *dbuff = '\0';
+
+	  log_Printf(LogDEBUG, "  rule = %d: Address match, "
+		     "check against proto %s%s, action = %s\n",
+		     n, filter_Proto2Nam(fp->f_proto),
+		     dbuff, filter_Action2Nam(fp->f_action));
+	}
+
+	if (cproto == fp->f_proto) {
+	  if ((fp->f_srcop == OP_NONE ||
+	       PortMatch(fp->f_srcop, sport, fp->f_srcport)) &&
+	      (fp->f_dstop == OP_NONE ||
+	       PortMatch(fp->f_dstop, dport, fp->f_dstport)) &&
+	      (fp->f_estab == 0 || estab) &&
+	      (fp->f_syn == 0 || syn) &&
+	      (fp->f_finrst == 0 || finrst)) {
+	    match = 1;
 	  }
-	} else
-	  log_Printf(LogDEBUG, "  rule = %d: Address mismatch\n", n);
+	}
+      } else {
+	/* Address is matched and no protocol specified. Make a decision. */
+	log_Printf(LogDEBUG, "  rule = %d: Address match, action = %s\n", n,
+		   filter_Action2Nam(fp->f_action));
+	match = 1;
       }
+    } else
+      log_Printf(LogDEBUG, "  rule = %d: Address mismatch\n", n);
+
+    if (match != fp->f_invert) {
+      /* Take specified action */
+      if (fp->f_action < A_NONE)
+	fp = &filter->rule[n = fp->f_action];
+      else
+	return (fp->f_action != A_PERMIT);
+    } else {
+      n++;
       fp++;
     }
-    return (A_DENY);		/* No rule is mached. Deny this packet */
   }
-  return (A_PERMIT);		/* No rule is given. Permit this packet */
+  return (1);		/* No rule is mached. Deny this packet */
 }
 
 #ifdef notdef
@@ -364,7 +415,7 @@ PacketCheck(struct bundle *bundle, char *cp, int nb, struct filter *filter)
     break;
   }
 
-  if ((FilterCheck(pip, filter) & A_DENY)) {
+  if (FilterCheck(pip, filter)) {
     if (logit)
       log_Printf(LogTCPIP, "%s - BLOCKED\n", logbuf);
 #ifdef notdef
@@ -375,7 +426,7 @@ PacketCheck(struct bundle *bundle, char *cp, int nb, struct filter *filter)
   } else {
     /* Check Keep Alive filter */
     if (logit) {
-      if (FilterCheck(pip, &bundle->filter.alive) & A_DENY)
+      if (FilterCheck(pip, &bundle->filter.alive))
         log_Printf(LogTCPIP, "%s - NO KEEPALIVE\n", logbuf);
       else
         log_Printf(LogTCPIP, "%s\n", logbuf);
@@ -412,7 +463,7 @@ ip_Input(struct bundle *bundle, struct link *l, struct mbuf *bp)
     return NULL;
 
   pip = (struct ip *)tun.data;
-  if (!(FilterCheck(pip, &bundle->filter.alive) & A_DENY))
+  if (!FilterCheck(pip, &bundle->filter.alive))
     bundle_StartIdleTimer(bundle);
 
   ipcp_AddInOctets(&bundle->ncp.ipcp, nb);
@@ -491,7 +542,7 @@ ip_PushPacket(struct link *l, struct bundle *bundle)
       bp = mbuf_Contiguous(mbuf_Dequeue(queue));
       cnt = mbuf_Length(bp);
       pip = (struct ip *)MBUF_CTOP(bp);
-      if (!(FilterCheck(pip, &bundle->filter.alive) & A_DENY))
+      if (!FilterCheck(pip, &bundle->filter.alive))
         bundle_StartIdleTimer(bundle);
       link_PushPacket(l, bp, bundle, PRI_NORMAL, PROTO_IP);
       ipcp_AddOutOctets(ipcp, cnt);
diff --git a/usr.sbin/ppp/ppp.8 b/usr.sbin/ppp/ppp.8
index 77b4a6d..c7924d1 100644
--- a/usr.sbin/ppp/ppp.8
+++ b/usr.sbin/ppp/ppp.8
@@ -1,4 +1,4 @@
-.\" $Id: ppp.8,v 1.180 1999/07/27 00:30:32 brian Exp $
+.\" $Id: ppp.8,v 1.181 1999/07/27 13:47:59 brian Exp $
 .Dd 20 September 1995
 .nr XX \w'\fC00'
 .Os FreeBSD
@@ -1400,7 +1400,9 @@ set filter
 .Ar name
 .Ar rule-no
 .Ar action
+.Op \&!
 .Oo
+.Op host
 .Ar src_addr Ns Op / Ns Ar width
 .Op Ar dst_addr Ns Op / Ns Ar width
 .Oc
@@ -1432,16 +1434,27 @@ but only if rule
 is defined.
 .It
 .Ar Action
-is either
+may be specified as
 .Sq permit
 or
-.Sq deny .
-If a given packet
-matches the rule, the associated action is taken immediately.
+.Sq deny ,
+in which case, if a given packet matches the rule, the associated action
+is taken immediately.
 .Ar Action
 can also be specified as
 .Sq clear
-to clear the action associated with that particular rule.
+to clear the action associated with that particular rule, or as a new
+rule number greater than the current rule.  In this case, if a given
+packet matches the current rule, the packet will next be matched against
+the new rule number (rather than the next rule number).
+.Pp
+The
+.Ar action
+may optionally be followed with an exclaimation mark
+.Pq Dq ! ,
+telling
+.Nm
+to reverse the sense of the following match.
 .It
 .Op Ar src_addr Ns Op / Ns Ar width
 and
@@ -3843,8 +3856,10 @@ will be
 .Sq escaped
 as they travel across the link.
 .It set filter dial|alive|in|out Ar rule-no Xo
-.No permit|deny
-.Oo Ar src_addr Ns Op / Ns Ar width
+.No permit|deny|clear| Ns Ar rule-no
+.Op \&!
+.Oo Op host
+.Ar src_addr Ns Op / Ns Ar width
 .Op Ar dst_addr Ns Op / Ns Ar width
 .Oc Oo tcp|udp|igmp|icmp Op src lt|eq|gt Ar port
 .Op dst lt|eq|gt Ar port
@@ -3871,9 +3886,12 @@ into the machine and the
 filter specifies packets that are allowed out of the machine.
 .Pp
 Filtering is done prior to any IP alterations that might be done by the
-alias engine.  By default all filter sets allow all packets to pass.
-Rules are processed in order according to
-.Ar rule-no .
+alias engine on outgoing packets and after any IP alterations that might
+be done by the alias engine on incoming packets.  By default all filter
+sets allow all packets to pass.  Rules are processed in order according to
+.Ar rule-no
+(unless skipped by specifying a rule number as the
+.Ar action ) .
 Up to 40 rules may be given for each set.  If a packet doesn't match
 any of the rules in a given set, it is discarded.  In the case of
 .Em in
diff --git a/usr.sbin/ppp/ppp.8.m4 b/usr.sbin/ppp/ppp.8.m4
index 77b4a6d..c7924d1 100644
--- a/usr.sbin/ppp/ppp.8.m4
+++ b/usr.sbin/ppp/ppp.8.m4
@@ -1,4 +1,4 @@
-.\" $Id: ppp.8,v 1.180 1999/07/27 00:30:32 brian Exp $
+.\" $Id: ppp.8,v 1.181 1999/07/27 13:47:59 brian Exp $
 .Dd 20 September 1995
 .nr XX \w'\fC00'
 .Os FreeBSD
@@ -1400,7 +1400,9 @@ set filter
 .Ar name
 .Ar rule-no
 .Ar action
+.Op \&!
 .Oo
+.Op host
 .Ar src_addr Ns Op / Ns Ar width
 .Op Ar dst_addr Ns Op / Ns Ar width
 .Oc
@@ -1432,16 +1434,27 @@ but only if rule
 is defined.
 .It
 .Ar Action
-is either
+may be specified as
 .Sq permit
 or
-.Sq deny .
-If a given packet
-matches the rule, the associated action is taken immediately.
+.Sq deny ,
+in which case, if a given packet matches the rule, the associated action
+is taken immediately.
 .Ar Action
 can also be specified as
 .Sq clear
-to clear the action associated with that particular rule.
+to clear the action associated with that particular rule, or as a new
+rule number greater than the current rule.  In this case, if a given
+packet matches the current rule, the packet will next be matched against
+the new rule number (rather than the next rule number).
+.Pp
+The
+.Ar action
+may optionally be followed with an exclaimation mark
+.Pq Dq ! ,
+telling
+.Nm
+to reverse the sense of the following match.
 .It
 .Op Ar src_addr Ns Op / Ns Ar width
 and
@@ -3843,8 +3856,10 @@ will be
 .Sq escaped
 as they travel across the link.
 .It set filter dial|alive|in|out Ar rule-no Xo
-.No permit|deny
-.Oo Ar src_addr Ns Op / Ns Ar width
+.No permit|deny|clear| Ns Ar rule-no
+.Op \&!
+.Oo Op host
+.Ar src_addr Ns Op / Ns Ar width
 .Op Ar dst_addr Ns Op / Ns Ar width
 .Oc Oo tcp|udp|igmp|icmp Op src lt|eq|gt Ar port
 .Op dst lt|eq|gt Ar port
@@ -3871,9 +3886,12 @@ into the machine and the
 filter specifies packets that are allowed out of the machine.
 .Pp
 Filtering is done prior to any IP alterations that might be done by the
-alias engine.  By default all filter sets allow all packets to pass.
-Rules are processed in order according to
-.Ar rule-no .
+alias engine on outgoing packets and after any IP alterations that might
+be done by the alias engine on incoming packets.  By default all filter
+sets allow all packets to pass.  Rules are processed in order according to
+.Ar rule-no
+(unless skipped by specifying a rule number as the
+.Ar action ) .
 Up to 40 rules may be given for each set.  If a packet doesn't match
 any of the rules in a given set, it is discarded.  In the case of
 .Em in