Major cleanup of the parser and printing routines in an attempt to

render the syntax less ambiguous.

Now rules can be in one of these two forms

	<action> <protocol> from <src> to <dst> [options]
	<action> MAC dst-mac src-mac mac-type [options]

however you can now specify MAC and IP header fields as options e.g.

	ipfw add allow all from any to any mac-type arp
	ipfw add allow all from any to any { dst-ip me or src-ip me }

which makes complex expressions a lot easier to write and parse.
The "all from any to any" part is there just for backward compatibility.

Manpage updated accordingly.

2 files changed, 375 insertions, 209 deletions

diff --git a/sbin/ipfw/ipfw.8 b/sbin/ipfw/ipfw.8
index da00fd7..6dbfd1c 100644
--- a/sbin/ipfw/ipfw.8
+++ b/sbin/ipfw/ipfw.8
@@ -671,27 +671,29 @@ Only one level of parentheses is allowed.
 Beware that most shells have special meanings for parentheses
 or braces, so it is advisable to put a \\ in front of them.
 .Pp
-The body of a rule must in general comprise a source and destination
+The body of a rule must in general include a source and destination
 addres specifier.
 The keyword
 .Ar any
 can be used in various places to specify that the content of
 a required field is irrelevant.
 .Pp
-The general rule body format is one of the following:
+The rule body format is one of the following:
 .Bd -ragged -offset indent
 .Ar proto
 .Cm from Ar src
 .Cm to Ar dst
 .Op Ar options
-.br
-.Cm MAC Ar dst-mac src-mac mac-type
-.Op Cm from Ar src Cm to Ar dst
-.Op Ar options
+.Pp
+.Cm MAC Ar dst-mac src-mac Op Cm not
+.Ar mac-type Op Ar options
 .Ed
 .Pp
 where the second format allows you to specify MAC header fields
-instead (or in addition) of the IPv4 header fields.
+instead of IPv4 header fields.
+Note that in practice both formats are equivalent, because the
+.Ar options
+let you specify match patterns for all IP and MAC header fields.
 .Pp
 Rule fields have the following meaning:
 .Bl -tag -width indent
@@ -704,15 +706,16 @@ The
 or
 .Cm all
 keywords mean any protocol will match.
-.It Ar src No and Ar dst :
+.It Ar src No and Ar dst : ip-address | { ip-address Cm or ... } Op Ar ports
 A single
-.Ar ip address
+.Ar ip-address
 , or an
 .Em or-block
 containing one or more of them,
 optionally followed by
-.Em port numbers.
-.It Ar ip address :
+.Ar ports
+specifiers.
+.It Ar ip-address :
 An address (or set of addresses) specified in one of the following
 ways, optionally preceded by a
 .Cm not
@@ -758,19 +761,13 @@ within a single rule. Because the matching occurs using a
 bitmask, it takes constant time and dramatically reduces
 the complexity of rulesets.
 .El
-.It port numbers
-With protocols which support port numbers (such as TCP and UDP), optional
+.It Ar ports : Oo Cm not Oc Bro Ar port | port Ns \&- Ns Ar port Ns Brc Op , Ns Ar ...
+For protocols which support port numbers (such as TCP and UDP), optional
 .Cm ports
 may be specified as one or more ports or port ranges, separated
 by commas but no spaces, and an optional
 .Cm not
-operator:
-.Bd -ragged -offset indent
-.Op Cm not
-.Brq Ar port | port Ns \&- Ns Ar port Ns
-.Op , Ns Ar ...
-.Ed
-.Pp
+operator.
 The
 .Ql \&-
 notation specifies a range of ports (including boundaries).
@@ -778,13 +775,12 @@ notation specifies a range of ports (including boundaries).
 Service names (from
 .Pa /etc/services )
 may be used instead of numeric port values.
-The length of the port list is limited to 14 ports or ranges,
-though you can also use port ranges within an
+The length of the port list is limited to 30 ports or ranges,
+though one can specify larger ranges by using an
 .Em or-block
-to build essentially unlimited lists:
-.Pp
-.Dl "ipfw add allow tcp from any { 1-20,30-50 or 500-600 } to any"
-.Pp
+in the
+.Cm options
+section of the rule.
 .Pp
 A backslash
 .Pq Ql \e
@@ -800,7 +796,7 @@ specifications.
 See the
 .Cm frag
 option for details on matching fragmented packets.
-.It dst-mac, src-mac
+.It Ar dst-mac, src-mac
 Destination and source MAC addresses, specified as
 groups of hex digits separated by commas, and optionally
 followed by a mask indicating how many bits are significant:
@@ -811,7 +807,7 @@ Note that the order of MAC addresses (destination first,
 source second) is
 the same as on the wire, but the opposite of the one used for
 IP addresses.
-.It mac-type
+.It Ar mac-type
 The value of the Ethernet Type field, specified in the same way as
 .Cm port numbers
 (i.e. one or more comma-separated single values or ranges).
@@ -823,7 +819,7 @@ are always printed as hexadecimal (unless the
 option is used, in which case symbolic resolution will be
 attempted).
 .El
-.Ss RULE OPTIONS
+.Ss RULE OPTIONS (MATCH PATTERNS)
 Additional match patterns can be used within
 rules. Zero or more of these so-called
 .Em options
@@ -832,44 +828,31 @@ can be present in a rule, optionally prefixed by the
 operand, and possibly grouped into
 .Em or-blocks .
 .Pp
-Note that there is an ambiguity in the syntax: in a rule of
-the form
-.Pp
-.Dl "ipfw add allow ip from any to any { in or layer2 }"
-.Pp
-the or-block could contain either port lists or options.
-To remove the ambiguity, one should specify a destination
-port, which can be done by either using the keyword
-.Cm any
-or an empty or-block
-.Cm { }
-e.g.:
-.Pp
-.Dl "ipfw add allow ip from any to any any { in or layer2 }"
-.Pp
-The following options are available:
+The following match patterns can be used (listed in alphabetical order):
 .Bl -tag -width indent
 .It Cm bridged
 Matches only bridged packets.
+.It Cm dst-ip Ar ip address
+Matches IP packets whose destination IP is one of the address(es)
+specified as argument.
+.It Cm dst-port Ar source ports
+Matches IP packets whose destination port is one of the port(s)
+specified as argument.
 .It Cm established
-TCP packets only.
-Match packets that have the RST or ACK bits set.
+Matches TCP packets that have the RST or ACK bits set.
 .It Cm frag
-Match if the packet is a fragment and this is not the first
-fragment of the datagram.
-.Cm frag
-may not be used in conjunction with either
-.Cm tcpflags
-or TCP/UDP port specifications.
+Matches packets that are fragments and not the first
+fragment of an IP datagram. Note that these packets will not have
+the next protocol header (e.g. TCP, UDP) so options that look into
+these headers cannot match.
 .It Cm gid Ar group
-Match all TCP or UDP packets sent by or received for a
+Matches all TCP or UDP packets sent by or received for a
 .Ar group .
 A
 .Ar group
-may be matched by name or identification number.
+may be specified by name or number.
 .It Cm icmptypes Ar types
-ICMP packets only.
-Match if the ICMP type is in the list
+Matches ICMP packets whose ICMP type is in the list
 .Ar types .
 The list may be specified as any combination of ranges or
 individual types separated by commas.
@@ -906,24 +889,25 @@ address mask request
 and address mask reply
 .Pq Cm 18 .
 .It Cm in | out
-Only match incoming or outgoing packets, respectively.
+Matches incoming or outgoing packets, respectively.
 .Cm in
 and
 .Cm out
 are mutually exclusive (in fact,
 .Cm out
 is implemented as
-.Cm not in
-).
+.Cm not in Ns No ).
 .It Cm ipid Ar id
-Match if the identification of IP datagram is
+Matches IP packets whose
+.Cm ip_id
+field has value
 .Ar id .
 .It Cm iplen Ar len
-Match if the total length of a packet, including header and data, is
+Matches IP packets whose total length, including header and data, is
 .Ar len
 bytes.
 .It Cm ipoptions Ar spec
-Match if the IP header contains the comma separated list of
+Matches packets whose IP header contains the comma separated list of
 options specified in
 .Ar spec .
 The supported IP options are:
@@ -940,10 +924,12 @@ The absence of a particular option may be denoted
 with a
 .Ql \&! .
 .It Cm ipprecedence Ar precedence
-Match if the numeric value of IP datagram's precedence is equal to
+Matches IP packets whose precedence field is equal to
 .Ar precedence .
 .It Cm iptos Ar spec
-Match if the IP header contains the comma separated list of
+Matches IP packets whose
+.Cm tos
+field contains the comma separated list of
 service types specified in
 .Ar spec .
 The supported IP types of service are:
@@ -962,10 +948,10 @@ The absence of a particular type may be denoted
 with a
 .Ql \&! .
 .It Cm ipttl Ar ttl
-Match if the time to live of IP datagram is
+Matches IP packets whose time to live is
 .Ar ttl .
 .It Cm ipversion Ar ver
-Match if the IP header version is
+Matches IP packets whose IP version field is
 .Ar ver .
 .It Cm keep-state
 Upon a match, the firewall will create a dynamic rule, whose
@@ -987,13 +973,22 @@ set of parameters as specified in the rule.
 One or more
 of source and destination addresses and ports can be
 specified.
+.It Cm { MAC | mac } Ar dst-mac src-mac
+Match packets with a given dst-mac and src-mac addresses, specified
+in one of the forms described earlier.
+.It Cm mac-type Ar mac-type
+Matches packets whose
+.Ar mac-type
+corresponds to one of those specified as argument.
+.It Cm proto Ar protocol
+Matches packets with the corresponding IPv4 protocol.
 .It Cm recv | xmit | via Brq Ar ifX | Ar if Ns Cm * | Ar ipno | Ar any
-Packet must be received, transmitted or be going through,
-respectively, the interface specified by exact name (
-.Ar ifX
-), by device name (
-.Ar if Ns Cm *
-), by IP address, or through some interface.
+Matches packets received, transmitted or be going through,
+respectively, the interface specified by exact name
+.Ns No ( Ar ifX Ns No ),
+by device name
+.Ns No ( Ar if Ns Ar * Ns No ),
+by IP address, or through some interface.
 .Pp
 The
 .Cm via
@@ -1030,10 +1025,15 @@ originating from the local host have no receive interface,
 while packets destined for the local host have no transmit
 interface.
 .It Cm setup
-TCP packets only.
-Match packets that have the SYN bit set but no ACK bit.
+Matches TCP packets that have the SYN bit set but no ACK bit.
 This is the short form of
 .Dq Li tcpflags\ syn,!ack .
+.It Cm src-ip Ar ip-address
+Matches IP packets whose source IP is one of the address(es)
+specified as argument.
+.It Cm src-port Ar ports
+Matches IP packets whose source port is one of the port(s)
+specified as argument.
 .It Cm tcpack Ar ack
 TCP packets only.
 Match if the TCP header acknowledgment number field is set to
@@ -1601,6 +1601,21 @@ have in writing your rulesets.
 You might want to consider using these features in order to
 write your rulesets in a more efficient way.
 .Bl -tag -width indent
+.It Handling of non-IPv4 packets
+.Nm ipfw1
+will silently accept all non-IPv4 packets (which
+.Nm ipfw1
+will only see when
+.Em net.link.ether.bridge_ipfw=1 Ns
+).
+.Nm ipfw2
+will filter all packets (including non-IPv4 ones) according to the ruleset.
+To achieve the same behaviour as
+.Nm ipfw1
+you can use the following as the very first rule in your ruleset:
+.Pp
+.Dl "ipfw add 1 allow MAC any any not ip"
+.Pp
 .It Address sets
 .Nm ipfw1
 does not supports address sets (those in the form
diff --git a/sbin/ipfw/ipfw2.c b/sbin/ipfw/ipfw2.c
index 3aac954..0a74717 100644
--- a/sbin/ipfw/ipfw2.c
+++ b/sbin/ipfw/ipfw2.c
@@ -221,6 +221,8 @@ enum tokens {
 	TOK_TCPACK,
 	TOK_TCPWIN,
 	TOK_ICMPTYPES,
+	TOK_MAC,
+	TOK_MACTYPE,
 
 	TOK_PLR,
 	TOK_NOERROR,
@@ -322,6 +324,14 @@ struct _s_x rule_options[] = {
 	{ "tcpwin",		TOK_TCPWIN },
 	{ "icmptype",		TOK_ICMPTYPES },
 	{ "icmptypes",		TOK_ICMPTYPES },
+	{ "dst-ip",		TOK_DSTIP },
+	{ "src-ip",		TOK_SRCIP },
+	{ "dst-port",		TOK_DSTPORT },
+	{ "src-port",		TOK_SRCPORT },
+	{ "proto",		TOK_PROTO },
+	{ "MAC",		TOK_MAC },
+	{ "mac",		TOK_MAC },
+	{ "mac-type",		TOK_MACTYPE },
 
 	{ "not",		TOK_NOT },		/* pseudo option */
 	{ "!", /* escape ? */	TOK_NOT },		/* pseudo option */
@@ -393,7 +403,7 @@ print_port(int proto, u_int16_t port)
  * XXX todo: add support for mask.
  */
 static void
-print_newports(ipfw_insn_u16 *cmd, int proto)
+print_newports(ipfw_insn_u16 *cmd, int proto, int opcode)
 {
 	u_int16_t *p = cmd->ports;
 	int i;
@@ -401,6 +411,9 @@ print_newports(ipfw_insn_u16 *cmd, int proto)
 
 	if (cmd->o.len & F_NOT)
 		printf(" not");
+	if (opcode != 0)
+		printf ("%s", opcode == O_MAC_TYPE ? " mac-type" :
+		    (opcode == O_IP_DSTPORT ? " dst-port" : " src-port"));
 	for (i = F_LEN((ipfw_insn *)cmd) - 1; i > 0; i--, p += 2) {
 		printf(sep);
 		print_port(proto, p[0]);
@@ -487,10 +500,10 @@ fill_newports(ipfw_insn_u16 *cmd, char *av, int proto)
 {
 	u_int16_t *p = cmd->ports;
 	int i = 0;
+	char *s = av;
 
-	for (; *av ; i++, p +=2 ) {
+	while (*s) {
 		u_int16_t a, b;
-		char *s;
 
 		a = strtoport(av, &s, 0, proto);
 		if (s == av) /* no parameter */
@@ -508,11 +521,13 @@ fill_newports(ipfw_insn_u16 *cmd, char *av, int proto)
 			errx(EX_DATAERR, "invalid separator <%c> in <%s>\n",
 				*s, av);
 		}
+		i++;
+		p += 2;
 		av = s+1;
 	}
 	if (i > 0) {
 		if (i+1 > F_LEN_MASK)
-			errx(EX_DATAERR, "too many port range\n");
+			errx(EX_DATAERR, "too many ports/ranges\n");
 		cmd->o.len |= i+1; /* leave F_NOT and F_OR untouched */
 	}
 	return i;
@@ -547,7 +562,7 @@ fill_reject_code(u_short *codep, char *str)
 	val = strtoul(str, &s, 0);
 	if (s == str || *s != '\0' || val >= 0x100)
 		val = match_token(icmpcodes, str);
-	if (val <= 0)
+	if (val < 0)
 		errx(EX_DATAERR, "unknown ICMP unreachable code ``%s''", str);
 	*codep = val;
 	return;
@@ -624,12 +639,12 @@ print_flags(char *name, ipfw_insn *cmd, struct _s_x *list)
  * Print the ip address contained in a command.
  */
 static void
-print_ip(ipfw_insn_ip *cmd)
+print_ip(ipfw_insn_ip *cmd, char *s)
 {
 	struct hostent *he = NULL;
 	int mb;
 
-	printf("%s ", cmd->o.len & F_NOT ? " not": "");
+	printf("%s%s ", cmd->o.len & F_NOT ? " not": "", s);
 
 	if (cmd->o.opcode == O_IP_SRC_ME || cmd->o.opcode == O_IP_DST_ME) {
 		printf("me");
@@ -741,33 +756,47 @@ print_icmptypes(ipfw_insn_u32 *cmd)
  * show_ipfw() prints the body of an ipfw rule.
  * Because the standard rule has at least proto src_ip dst_ip, we use
  * a helper function to produce these entries if not provided explicitly.
+ * The first argument is the list of fields we have, the second is
+ * the list of fields we want to be printed.
  *
- * Special case: if we have provided a MAC header, and no IP specs,
- * just leave it alone.
- * Also, if we have providea a MAC header and no IP protocol, print it
- * as "all" instead of "ip".
+ * Special cases if we have provided a MAC header:
+ *   + if the rule does not contain IP addresses/ports, do not print them;
+ *   + if the rule does not contain an IP proto, print "all" instead of "ip";
+ *
+ * Once we have 'have_options', IP header fields are printed as options.
  */
 #define	HAVE_PROTO	0x0001
 #define	HAVE_SRCIP	0x0002
 #define	HAVE_DSTIP	0x0004
 #define	HAVE_MAC	0x0008
 #define	HAVE_MACTYPE	0x0010
+#define	HAVE_OPTIONS	0x8000
 
 #define	HAVE_IP		(HAVE_PROTO | HAVE_SRCIP | HAVE_DSTIP)
 static void
-show_prerequisites(int *flags, int want)
+show_prerequisites(int *flags, int want, int cmd)
 {
-	if ( (*flags & (HAVE_MAC | HAVE_MACTYPE))  == HAVE_MAC) {
-	     printf(" any");	/* MAC type */
-	     *flags |= HAVE_MACTYPE;
+	if ( (*flags & HAVE_IP) == HAVE_IP)
+		*flags |= HAVE_OPTIONS;
+
+	if ( (*flags & (HAVE_MAC|HAVE_MACTYPE|HAVE_OPTIONS)) == HAVE_MAC &&
+	     cmd != O_MAC_TYPE) {
+		/*
+		 * mac-type was optimized out by the compiler,
+		 * restore it
+		 */
+		printf(" any");
+		*flags |= HAVE_MACTYPE | HAVE_OPTIONS;
+		return;
+	}
+	if ( !(*flags & HAVE_OPTIONS)) {
+		if ( !(*flags & HAVE_PROTO) && (want & HAVE_PROTO))
+			printf(" ip");
+		if ( !(*flags & HAVE_SRCIP) && (want & HAVE_SRCIP))
+			printf(" from any");
+		if ( !(*flags & HAVE_DSTIP) && (want & HAVE_DSTIP))
+			printf(" to any");
 	}
-		
-	if ( !(*flags & HAVE_PROTO) && (want & HAVE_PROTO))
-		printf( (*flags & HAVE_MAC) ? " all" : " ip");
-	if ( !(*flags & HAVE_SRCIP) && (want & HAVE_SRCIP))
-		printf(" from any");
-	if ( !(*flags & HAVE_DSTIP) && (want & HAVE_DSTIP))
-		printf(" to any");
 	*flags |= want;
 }
 
@@ -818,8 +847,7 @@ show_ipfw(struct ip_fw *rule)
 		switch(cmd->opcode) {
 		case O_CHECK_STATE:
 			printf("check-state");
-			/* avoid printing anything else */
-			flags = HAVE_PROTO|HAVE_SRCIP|HAVE_DSTIP;
+			flags = HAVE_IP; /* avoid printing anything else */
 			break;
 
 		case O_PROB:
@@ -898,47 +926,59 @@ show_ipfw(struct ip_fw *rule)
 		else
 			printf(" log");
 	}
+
 	/*
-	 * then print the body
+	 * then print the body.
 	 */
+	if (rule->_pad & 1) {	/* empty rules before options */
+		printf (" all from any to any");
+		flags |= HAVE_IP | HAVE_OPTIONS;
+	}
+
         for (l = rule->act_ofs, cmd = rule->cmd ;
 			l > 0 ; l -= F_LEN(cmd) , cmd += F_LEN(cmd)) {
 		/* useful alias */
 		ipfw_insn_u32 *cmd32 = (ipfw_insn_u32 *)cmd;
 
+		show_prerequisites(&flags, 0, cmd->opcode);
+
 		switch(cmd->opcode) {
 		case O_PROBE_STATE:
 			break; /* no need to print anything here */
 
 		case O_MACADDR2: {
 			ipfw_insn_mac *m = (ipfw_insn_mac *)cmd;
-			if ( (flags & HAVE_MAC) == 0)
-				printf(" MAC");
-			flags |= HAVE_MAC;
+
+			if ((cmd->len & F_OR) && !or_block)
+				printf(" {");
 			if (cmd->len & F_NOT)
 				printf(" not");
+			printf(" MAC");
+			flags |= HAVE_MAC;
 			print_mac( m->addr, m->mask);
 			print_mac( m->addr + 6, m->mask + 6);
 			}
 			break;
 
 		case O_MAC_TYPE:
-			if ( (flags & HAVE_MAC) == 0)
-				printf(" MAC");
-			flags |= (HAVE_MAC | HAVE_MACTYPE);
-			print_newports((ipfw_insn_u16 *)cmd, IPPROTO_ETHERTYPE);
+			if ((cmd->len & F_OR) && !or_block)
+				printf(" {");
+			print_newports((ipfw_insn_u16 *)cmd, IPPROTO_ETHERTYPE,
+				(flags & HAVE_OPTIONS) ? cmd->opcode : 0);
+			flags |= HAVE_MAC | HAVE_MACTYPE | HAVE_OPTIONS;
 			break;
 
 		case O_IP_SRC:
 		case O_IP_SRC_MASK:
 		case O_IP_SRC_ME:
 		case O_IP_SRC_SET:
-			show_prerequisites(&flags, HAVE_PROTO);
+			show_prerequisites(&flags, HAVE_PROTO, 0);
 			if (!(flags & HAVE_SRCIP))
 				printf(" from");
 			if ((cmd->len & F_OR) && !or_block)
 				printf(" {");
-			print_ip((ipfw_insn_ip *)cmd);
+			print_ip((ipfw_insn_ip *)cmd,
+				(flags & HAVE_OPTIONS) ? " src-ip" : "");
 			flags |= HAVE_SRCIP;
 			break;
 
@@ -946,23 +986,24 @@ show_ipfw(struct ip_fw *rule)
 		case O_IP_DST_MASK:
 		case O_IP_DST_ME:
 		case O_IP_DST_SET:
-			show_prerequisites(&flags, HAVE_PROTO|HAVE_SRCIP);
+			show_prerequisites(&flags, HAVE_PROTO|HAVE_SRCIP, 0);
 			if (!(flags & HAVE_DSTIP))
 				printf(" to");
 			if ((cmd->len & F_OR) && !or_block)
 				printf(" {");
-			print_ip((ipfw_insn_ip *)cmd);
+			print_ip((ipfw_insn_ip *)cmd,
+				(flags & HAVE_OPTIONS) ? " dst-ip" : "");
 			flags |= HAVE_DSTIP;
 			break;
 
 		case O_IP_DSTPORT:
-			show_prerequisites(&flags,
-				HAVE_PROTO|HAVE_SRCIP|HAVE_DSTIP);
+			show_prerequisites(&flags, HAVE_IP, 0);
 		case O_IP_SRCPORT:
-			show_prerequisites(&flags, HAVE_PROTO|HAVE_SRCIP);
+			show_prerequisites(&flags, HAVE_PROTO|HAVE_SRCIP, 0);
 			if ((cmd->len & F_OR) && !or_block)
 				printf(" {");
-			print_newports((ipfw_insn_u16 *)cmd, proto);
+			print_newports((ipfw_insn_u16 *)cmd, proto,
+				(flags & HAVE_OPTIONS) ? cmd->opcode : 0);
 			break;
 
 		case O_PROTO: {
@@ -974,6 +1015,8 @@ show_ipfw(struct ip_fw *rule)
 				printf(" not");
 			proto = cmd->arg1;
 			pe = getprotobynumber(cmd->arg1);
+			if (flags & HAVE_OPTIONS)
+				printf(" proto");
 			if (pe)
 				printf(" %s", pe->p_name);
 			else
@@ -983,8 +1026,7 @@ show_ipfw(struct ip_fw *rule)
 			break;
 		
 		default: /*options ... */
-			show_prerequisites(&flags,
-			    HAVE_PROTO|HAVE_SRCIP|HAVE_DSTIP);
+			show_prerequisites(&flags, HAVE_IP | HAVE_OPTIONS, 0);
 			if ((cmd->len & F_OR) && !or_block)
 				printf(" {");
 			if (cmd->len & F_NOT && cmd->opcode != O_IN)
@@ -1137,7 +1179,7 @@ show_ipfw(struct ip_fw *rule)
 			or_block = 0;
 		}
 	}
-	show_prerequisites(&flags, HAVE_PROTO|HAVE_SRCIP|HAVE_DSTIP);
+	show_prerequisites(&flags, HAVE_IP, 0);
 
 	printf("\n");
 }
@@ -2280,10 +2322,10 @@ fill_cmd(ipfw_insn *cmd, enum ipfw_opcodes opcode, int flags, u_int16_t arg)
 static ipfw_insn *
 add_mac(ipfw_insn *cmd, int ac, char *av[])
 {
-	ipfw_insn_mac *mac; /* also *src */
+	ipfw_insn_mac *mac;
 
-	if (ac <2)
-		errx(EX_DATAERR, "MAC dst src [type]");
+	if (ac < 2)
+		errx(EX_DATAERR, "MAC dst src [not] type");
 
 	cmd->opcode = O_MACADDR2;
 	cmd->len = (cmd->len & (F_NOT | F_OR)) | F_INSN_SIZE(ipfw_insn_mac);
@@ -2291,17 +2333,84 @@ add_mac(ipfw_insn *cmd, int ac, char *av[])
 	mac = (ipfw_insn_mac *)cmd;
 	get_mac_addr_mask(av[0], mac->addr, mac->mask);	/* dst */
 	get_mac_addr_mask(av[1], &(mac->addr[6]), &(mac->mask[6])); /* src */
-	
-	if (ac>2 && strcmp(av[2], "any") != 0) { /* we have a non-null type */
-		cmd += F_LEN(cmd);
+	return cmd;
+}
 
-		fill_newports((ipfw_insn_u16 *)cmd, av[2], IPPROTO_ETHERTYPE);
+static ipfw_insn *
+add_mactype(ipfw_insn *cmd, int ac, char *av)
+{
+	if (ac < 1)
+		errx(EX_DATAERR, "missing MAC type");
+	if (strcmp(av, "any") != 0) { /* we have a non-null type */
+		fill_newports((ipfw_insn_u16 *)cmd, av, IPPROTO_ETHERTYPE);
 		cmd->opcode = O_MAC_TYPE;
-	}
+		return cmd;
+	} else
+		return NULL;
+}
+
+static ipfw_insn *
+add_proto(ipfw_insn *cmd, char *av)
+{
+	struct protoent *pe;
+	u_char proto = 0;
 
+	if (!strncmp(av, "all", strlen(av)))
+		; /* same as "ip" */
+	else if ((proto = atoi(av)) > 0)
+		; /* all done! */
+	else if ((pe = getprotobyname(av)) != NULL)
+		proto = pe->p_proto;
+	else
+		errx(EX_DATAERR, "invalid protocol ``%s''", av);
+	if (proto != IPPROTO_IP)
+		fill_cmd(cmd, O_PROTO, 0, proto);
 	return cmd;
 }
 
+static ipfw_insn *
+add_srcip(ipfw_insn *cmd, char *av)
+{
+	fill_ip((ipfw_insn_ip *)cmd, av);
+	if (cmd->opcode == O_IP_DST_SET)			/* set */
+		cmd->opcode = O_IP_SRC_SET;
+	else if (F_LEN(cmd) == F_INSN_SIZE(ipfw_insn))		/* me */
+		cmd->opcode = O_IP_SRC_ME;
+	else if (F_LEN(cmd) == F_INSN_SIZE(ipfw_insn_u32))	/* one IP */
+		cmd->opcode = O_IP_SRC;
+	else if (F_LEN(cmd) == F_INSN_SIZE(ipfw_insn_ip))	/* addr/mask */
+		cmd->opcode = O_IP_SRC_MASK;
+	return cmd;
+}
+
+static ipfw_insn *
+add_dstip(ipfw_insn *cmd, char *av)
+{
+	fill_ip((ipfw_insn_ip *)cmd, av);
+	if (cmd->opcode == O_IP_DST_SET)			/* set */
+		;
+	else if (F_LEN(cmd) == F_INSN_SIZE(ipfw_insn))		/* me */
+		cmd->opcode = O_IP_DST_ME;
+	else if (F_LEN(cmd) == F_INSN_SIZE(ipfw_insn_u32))	/* one IP */
+		cmd->opcode = O_IP_DST;
+	else if (F_LEN(cmd) == F_INSN_SIZE(ipfw_insn_ip))	/* addr/mask */
+		cmd->opcode = O_IP_DST_MASK;
+	return cmd;
+}
+
+static ipfw_insn *
+add_ports(ipfw_insn *cmd, char *av, u_char proto, int opcode)
+{
+	if (!strncmp(av, "any", strlen(av))) {
+		return NULL;
+	} else if (fill_newports((ipfw_insn_u16 *)cmd, av, proto)) {
+		/* XXX todo: check that we have a protocol with ports */
+		cmd->opcode = opcode;
+		return cmd;
+	}
+	return NULL;
+}
+
 /*
  * Parse arguments and assemble the microinstructions which make up a rule.
  * Rules are added into the 'rulebuf' and then copied in the correct order
@@ -2326,13 +2435,13 @@ add(int ac, char *av[])
 	static u_int32_t rulebuf[255], actbuf[255], cmdbuf[255];
 
 	ipfw_insn *src, *dst, *cmd, *action, *prev;
+	ipfw_insn *first_cmd;	/* first match pattern */
 
 	struct ip_fw *rule;
 
 	/*
 	 * various flags used to record that we entered some fields.
 	 */
-	int have_mac = 0;	/* set if we have a MAC address */
 	ipfw_insn *have_state = NULL;	/* check-state or keep-state */
 
 	int i;
@@ -2489,7 +2598,7 @@ add(int ac, char *av[])
 		break;
 
 	default:
-		errx(EX_DATAERR, "invalid action %s\n", *av);
+		errx(EX_DATAERR, "invalid action %s\n", av[-1]);
 	}
 	action = next_cmd(action);
 
@@ -2563,43 +2672,51 @@ add(int ac, char *av[])
 	}							\
 	CLOSE_PAR;
 
+	first_cmd = cmd;
+	/*
+	 * MAC addresses, optional.
+	 * If we have this, we skip the part "proto from src to dst"
+	 * and jump straight to the option parsing.
+	 */
+	NOT_BLOCK;
+	NEED1("missing protocol");
+	if (!strncmp(*av, "MAC", strlen(*av)) ||
+	    !strncmp(*av, "mac", strlen(*av))) {
+		ac--; av++;	/* the "MAC" keyword */
+		add_mac(cmd, ac, av); /* exits in case of errors */
+		cmd = next_cmd(cmd);
+		ac -= 2; av += 2;	/* dst-mac and src-mac */
+		NOT_BLOCK;
+		NEED1("missing mac type");
+		if (add_mactype(cmd, ac, av[0]))
+			cmd = next_cmd(cmd);
+		ac--; av++;	/* any or mac-type */
+		goto read_options;
+	}
+
 	/*
 	 * protocol, mandatory
 	 */
     OR_START(get_proto);
 	NOT_BLOCK;
 	NEED1("missing protocol");
-	{
-	struct protoent *pe;
-
-	if (!strncmp(*av, "all", strlen(*av)))
-		; /* same as "ip" */
-	else if (!strncmp(*av, "MAC", strlen(*av))) {
-		cmd = add_mac(cmd, ac-1, av+1); /* exits in case of errors */
-		av += 3;
-		ac -= 3;
-		have_mac = 1;
-	} else if ((proto = atoi(*av)) > 0)
-		; /* all done! */
-	else if ((pe = getprotobyname(*av)) != NULL)
-		proto = pe->p_proto;
-	else
-		errx(EX_DATAERR, "invalid protocol ``%s''", *av);
-	av++; ac--;
-	if (proto != IPPROTO_IP)
-		fill_cmd(cmd, O_PROTO, 0, proto);
+	if (add_proto(cmd, *av)) {
+		av++; ac--;
+		if (F_LEN(cmd) == 0)	/* plain IP */
+			proto = 0;
+		else {
+			proto = cmd->arg1;
+			prev = cmd;
+			cmd = next_cmd(cmd);
+		}
 	}
-	cmd = next_cmd(cmd);
     OR_BLOCK(get_proto);
 
 	/*
-	 * "from", mandatory (unless we have a MAC address)
+	 * "from", mandatory
 	 */
-	if (!ac || strncmp(*av, "from", strlen(*av))) {
-		if (have_mac)	/* we do not need a "to" address */
-			goto read_to;
+	if (!ac || strncmp(*av, "from", strlen(*av)))
 		errx(EX_USAGE, "missing ``from''");
-	}
 	ac--; av++;
 
 	/*
@@ -2608,51 +2725,33 @@ add(int ac, char *av[])
     OR_START(source_ip);
 	NOT_BLOCK;	/* optional "not" */
 	NEED1("missing source address");
-
-	/* source	-- mandatory */
-	fill_ip((ipfw_insn_ip *)cmd, *av);
-	if (cmd->opcode == O_IP_DST_SET)			/* set */
-		cmd->opcode = O_IP_SRC_SET;
-	else if (F_LEN(cmd) == F_INSN_SIZE(ipfw_insn))		/* me */
-		cmd->opcode = O_IP_SRC_ME;
-	else if (F_LEN(cmd) == F_INSN_SIZE(ipfw_insn_u32))	/* one IP */
-		cmd->opcode = O_IP_SRC;
-	else if (F_LEN(cmd) == F_INSN_SIZE(ipfw_insn_ip))	/* addr/mask */
-		cmd->opcode = O_IP_SRC_MASK;
-	/* otherwise len will be zero and the command skipped */
-	ac--; av++;
-	prev = cmd; /* in case we need to backtrack */
-	cmd = next_cmd(cmd);
+	if (add_srcip(cmd, *av)) {
+		ac--; av++;
+		if (F_LEN(cmd) != 0) {	/* ! any */
+			prev = cmd;
+			cmd = next_cmd(cmd);
+		}
+	}
     OR_BLOCK(source_ip);
 
-    OR_START(source_port);
 	/*
 	 * source ports, optional
 	 */
 	NOT_BLOCK;	/* optional "not" */
 	if (ac) {
-		if (!strncmp(*av, "any", strlen(*av))) {
-			ac--;
-			av++;
-		} else if (fill_newports((ipfw_insn_u16 *)cmd, *av, proto)) {
-			/* XXX todo: check that we have a protocol with ports */
-			cmd->opcode = O_IP_SRCPORT;
-			ac--;
-			av++;
-			cmd = next_cmd(cmd);
+		if (!strncmp(*av, "any", strlen(*av)) ||
+		    add_ports(cmd, *av, proto, O_IP_SRCPORT)) {
+			ac--; av++;
+			if (F_LEN(cmd) != 0)
+				cmd = next_cmd(cmd);
 		}
 	}
-    OR_BLOCK(source_port);
 
-read_to:
 	/*
-	 * "to", mandatory (unless we have a MAC address
+	 * "to", mandatory
 	 */
-	if (!ac || strncmp(*av, "to", strlen(*av))) {
-		if (have_mac)
-			goto read_options;
+	if (!ac || strncmp(*av, "to", strlen(*av)))
 		errx(EX_USAGE, "missing ``to''");
-	}
 	av++; ac--;
 
 	/*
@@ -2661,41 +2760,36 @@ read_to:
     OR_START(dest_ip);
 	NOT_BLOCK;	/* optional "not" */
 	NEED1("missing dst address");
-	fill_ip((ipfw_insn_ip *)cmd, *av);
-	if (cmd->opcode == O_IP_DST_SET)			/* set */
-		;
-	else if (F_LEN(cmd) == F_INSN_SIZE(ipfw_insn))		/* me */
-		cmd->opcode = O_IP_DST_ME;
-	else if (F_LEN(cmd) == F_INSN_SIZE(ipfw_insn_u32))	/* one IP */
-		cmd->opcode = O_IP_DST;
-	else if (F_LEN(cmd) == F_INSN_SIZE(ipfw_insn_ip))	/* addr/mask */
-		cmd->opcode = O_IP_DST_MASK;
-	ac--;
-	av++;
-	prev = cmd;
-	cmd = next_cmd(cmd);
+	if (add_dstip(cmd, *av)) {
+		ac--; av++;
+		if (F_LEN(cmd) != 0) {	/* ! any */
+			prev = cmd;
+			cmd = next_cmd(cmd);
+		}
+	}
     OR_BLOCK(dest_ip);
 
-    OR_START(dest_port);
 	/*
 	 * dest. ports, optional
 	 */
 	NOT_BLOCK;	/* optional "not" */
 	if (ac) {
-		if (!strncmp(*av, "any", strlen(*av))) {
-			ac--;
-			av++;
-		} else if (fill_newports((ipfw_insn_u16 *)cmd, *av, proto)) {
-			/* XXX todo: check that we have a protocol with ports */
-			cmd->opcode = O_IP_DSTPORT;
-			ac--;
-			av++;
-			cmd += F_LEN(cmd);
+		if (!strncmp(*av, "any", strlen(*av)) ||
+		    add_ports(cmd, *av, proto, O_IP_DSTPORT)) {
+			ac--; av++;
+			if (F_LEN(cmd) != 0)
+				cmd = next_cmd(cmd);
 		}
 	}
-    OR_BLOCK(dest_port);
 
 read_options:
+	if (ac && first_cmd == cmd) {
+		/*
+		 * nothing specified so far, store in the rule to ease
+		 * printout later.
+		 */
+		 rule->_pad = 1;
+	}
 	prev = NULL;
 	while (ac) {
 		char *s;
@@ -2735,6 +2829,7 @@ read_options:
 			if (!open_par)
 				errx(EX_USAGE, "+missing \")\"\n");
 			open_par = 0;
+			prev = NULL;
         		break;
 
 		case TOK_IN:
@@ -2940,6 +3035,62 @@ read_options:
 		    }
 			break;
 
+		case TOK_PROTO:
+			NEED1("missing protocol");
+			if (add_proto(cmd, *av)) {
+				proto = cmd->arg1;
+				ac--; av++;
+			}
+			break;
+				
+		case TOK_SRCIP:
+			NEED1("missing source IP");
+			if (add_srcip(cmd, *av)) {
+				ac--; av++;
+			}
+			break;
+
+		case TOK_DSTIP:
+			NEED1("missing destination IP");
+			if (add_dstip(cmd, *av)) {
+				ac--; av++;
+			}
+			break;
+
+		case TOK_SRCPORT:
+			NEED1("missing source port");
+			if (!strncmp(*av, "any", strlen(*av)) ||
+			    add_ports(cmd, *av, proto, O_IP_SRCPORT)) {
+				ac--; av++;
+			} else
+				errx(EX_DATAERR, "invalid source port %s", *av);
+			break;
+
+		case TOK_DSTPORT:
+			NEED1("missing destination port");
+			if (!strncmp(*av, "any", strlen(*av)) ||
+			    add_ports(cmd, *av, proto, O_IP_DSTPORT)) {
+				ac--; av++;
+			} else
+				errx(EX_DATAERR, "invalid destination port %s",
+				    *av);
+			break;
+
+		case TOK_MAC:
+			if (ac < 2)
+				errx(EX_USAGE, "MAC dst-mac src-mac");
+			if (add_mac(cmd, ac, av)) {
+				ac -= 2; av += 2;
+			}
+			break;
+
+		case TOK_MACTYPE:
+			NEED1("missing mac type");
+			if (!add_mactype(cmd, ac, *av))
+				errx(EX_DATAERR, "invalid mac type %s", av);
+			ac--; av++;
+			break;
+
 		default:
 			errx(EX_USAGE, "unrecognised option [%d] %s\n", i, s);
 		}