Replace 'stateful-ends' with more permissive 'stateful-all'. (#30)

Author: rmind

docs/stateful.md

@@ -25,7 +25,7 @@ http://www.usenix.org/events/sec01/invitedtalks/rooij.pdf) paper.
 
 NPF is a stateful packet filter capable of tracking TCP connections,
 as well as performing limited UDP and ICMP tracking.  Stateful filtering is
-enabled using the `stateful` or `stateful-ends` keywords.  The former creates
+enabled using the `stateful` or `stateful-all` keywords.  The former creates
 a state which is uniquely identified by a 5-tuple (source and destination IP
 addresses, port numbers and an interface identifier).  The latter excludes
 the interface identifier and must be used with precaution.  Once the state is
@@ -59,17 +59,17 @@ IMPORTANT: Stateful rules imply `flags S/SAFR` for TCP packets.
 
 ---
 
-It is important to understand the implications of `stateful-ends`.  Bypassing
+It is important to understand the implications of `stateful-all`.  Bypassing
 the ruleset on other interfaces can have undesirable effects, e.g. a packet
 with a spoofed IP address might bypass ingress filtering.  Associating a state
 with two interfaces (forwarding case) may also cause problems if the routes
 change.  On the other hand, picking up the state on any interface may lead
 to higher performance in certain configurations and may also handle some
 asymmetric routing cases.  The administrator is free to choose whether
-`stateful` or `stateful-ends` is more suitable.
+`stateful` or `stateful-all` is more suitable.
 
 ---
-WARNING: The `stateful-ends` keyword must be used with precaution.
+WARNING: The `stateful-all` keyword must be used with precaution.
 
 ---
 

src/kern/npf.h

@@ -203,7 +203,7 @@ bool		npf_autounload_p(void);
 #define	NPF_RULE_RETRST			0x00000010
 #define	NPF_RULE_RETICMP		0x00000020
 #define	NPF_RULE_DYNAMIC		0x00000040
-#define	NPF_RULE_MULTIENDS		0x00000080
+#define	NPF_RULE_GSTATEFUL		0x00000080
 
 #define	NPF_DYNAMIC_GROUP		(NPF_RULE_GROUP | NPF_RULE_DYNAMIC)
 

src/kern/npf_conn.c

@@ -264,23 +264,36 @@ conn_update_atime(npf_conn_t *con)
 }
 
 /*
- * npf_conn_ok: check if the connection is active and has the right direction.
+ * npf_conn_check: check that:
+ *
+ *	- the connection is active;
+ *
+ *	- the packet is travelling in the right direction with the respect
+ *	  to the connection direction (if interface-id is not zero);
+ *
+ *	- the packet is travelling on the same interface as the
+ *	  connection interface (if interface-id is not zero).
  */
 static bool
-npf_conn_ok(const npf_conn_t *con, const int di, bool forw)
+npf_conn_check(const npf_conn_t *con, const nbuf_t *nbuf,
+    const unsigned di, const bool forw)
 {
 	const uint32_t flags = con->c_flags;
+	const unsigned ifid = con->c_ifid;
+	bool active, pforw;
 
-	/* Check if connection is active and not expired. */
-	bool ok = (flags & (CONN_ACTIVE | CONN_EXPIRE)) == CONN_ACTIVE;
-	if (__predict_false(!ok)) {
+	active = (flags & (CONN_ACTIVE | CONN_EXPIRE)) == CONN_ACTIVE;
+	if (__predict_false(!active)) {
 		return false;
 	}
-
-	/* Check if the direction is consistent */
-	bool pforw = (flags & PFIL_ALL) == (unsigned)di;
-	if (__predict_false(forw != pforw)) {
-		return false;
+	if (ifid && nbuf) {
+		pforw = (flags & PFIL_ALL) == (unsigned)di;
+		if (__predict_false(forw != pforw)) {
+			return false;
+		}
+		if (__predict_false(ifid != nbuf->nb_ifid)) {
+			return false;
+		}
 	}
 	return true;
 }
@@ -297,7 +310,6 @@ npf_conn_lookup(const npf_cache_t *npc, const int di, bool *forw)
 	const nbuf_t *nbuf = npc->npc_nbuf;
 	npf_conn_t *con;
 	npf_connkey_t key;
-	u_int cifid;
 
 	/* Construct a key and lookup for a connection in the store. */
 	if (!npf_conn_conkey(npc, &key, true)) {
@@ -309,18 +321,8 @@ npf_conn_lookup(const npf_cache_t *npc, const int di, bool *forw)
 	}
 	KASSERT(npc->npc_proto == con->c_proto);
 
-	/* Check if connection is active and not expired. */
-	if (!npf_conn_ok(con, di, *forw)) {
-		atomic_dec_uint(&con->c_refcnt);
-		return NULL;
-	}
-
-	/*
-	 * Match the interface and the direction of the connection entry
-	 * and the packet.
-	 */
-	cifid = con->c_ifid;
-	if (__predict_false(cifid && cifid != nbuf->nb_ifid)) {
+	/* Extra checks for the connection and packet. */
+	if (!npf_conn_check(con, nbuf, di, *forw)) {
 		atomic_dec_uint(&con->c_refcnt);
 		return NULL;
 	}
@@ -394,7 +396,7 @@ npf_conn_inspect(npf_cache_t *npc, const int di, int *error)
  * => Connection will be activated on the first reference release.
  */
 npf_conn_t *
-npf_conn_establish(npf_cache_t *npc, int di, bool per_if)
+npf_conn_establish(npf_cache_t *npc, int di, bool global)
 {
 	npf_t *npf = npc->npc_ctx;
 	const unsigned alen = npc->npc_alen;
@@ -447,7 +449,7 @@ npf_conn_establish(npf_cache_t *npc, int di, bool per_if)
 		npf_conn_destroy(npf, con);
 		return NULL;
 	}
-	con->c_ifid = per_if ? nbuf->nb_ifid : 0;
+	con->c_ifid = global ? nbuf->nb_ifid : 0;
 
 	/*
 	 * Set last activity time for a new connection and acquire
@@ -914,7 +916,7 @@ npf_conn_find(npf_t *npf, const nvlist_t *idict, nvlist_t **odict)
 		return ESRCH;
 	}
 	dir = dnvlist_get_number(idict, "direction", 0);
-	if (!npf_conn_ok(con, dir, true)) {
+	if (!npf_conn_check(con, NULL, dir, true)) {
 		atomic_dec_uint(&con->c_refcnt);
 		return ESRCH;
 	}

src/kern/npf_handler.c

@@ -232,7 +232,7 @@ npf_packet_handler(npf_t *npf, struct mbuf **mp, ifnet_t *ifp, int di)
 	 */
 	if ((mi.mi_retfl & NPF_RULE_STATEFUL) != 0 && !con) {
 		con = npf_conn_establish(&npc, di,
-		    (mi.mi_retfl & NPF_RULE_MULTIENDS) == 0);
+		    (mi.mi_retfl & NPF_RULE_GSTATEFUL) == 0);
 		if (con) {
 			/*
 			 * Note: the reference on the rule procedure is

src/libnpf/libnpf.3

@@ -178,8 +178,11 @@ Create a state (session) on match, track the connection and pass the
 backwards stream (the returning packets) without the ruleset inspection.
 The state is uniquely identified by a 5-tuple (source and destination
 IP addresses, port numbers and an interface identifier).
-.It Dv NPF_RULE_MULTIENDS
+.It Dv NPF_RULE_GSTATEFUL
 Exclude the interface identifier from the state key i.e. use a 4-tuple.
+This makes the state global with the respect network interfaces.
+The state is also picked on packet travelling different direction that
+originally.
 .It Dv NPF_RULE_RETRST
 Return TCP RST packet in a case of packet block.
 .It Dv NPF_RULE_RETICMP

src/npfctl/npf.conf.5

@@ -299,12 +299,12 @@ before further processing.
 Stateful packet inspection is enabled using the
 .Cm stateful
 or
-.Cm stateful-ends
+.Cm stateful-all
 keywords.
 The former creates a state which is uniquely identified by a 5-tuple (source
 and destination IP addresses, port numbers and an interface identifier).
-The latter excludes the interface identifier and must be used with
-precaution.
+The latter excludes the interface identifier, i.e. making the state global,
+and must be used with precaution.
 In both cases, a full TCP state tracking is performed for TCP connections
 and a limited tracking for message-based protocols (UDP and ICMP).
 .Pp
@@ -550,7 +550,7 @@ rule-list	= [ rule new-line ] rule-list
 
 npf-filter	= [ "family" family-opt ] [ proto ] ( "all" | filt-opts )
 static-rule	= ( "block" [ block-opts ] | "pass" )
-		  [ "stateful" | "stateful-ends" ]
+		  [ "stateful" | "stateful-all" ]
 		  [ "in" | "out" ] [ "final" ] [ "on" interface ]
 		  ( npf-filter | "pcap-filter" pcap-filter-expr )
 		  [ "apply" proc-name ]

src/npfctl/npf_parse.y

@@ -158,7 +158,7 @@ yyerror(const char *fmt, ...)
 %token			SET
 %token			SLASH
 %token			STATEFUL
-%token			STATEFUL_ENDS
+%token			STATEFUL_ALL
 %token			TABLE
 %token			TCP
 %token			TO
@@ -643,7 +643,7 @@ all_or_filt_opts
 
 opt_stateful
 	: STATEFUL	{ $$ = NPF_RULE_STATEFUL; }
-	| STATEFUL_ENDS	{ $$ = NPF_RULE_STATEFUL | NPF_RULE_MULTIENDS; }
+	| STATEFUL_ALL	{ $$ = NPF_RULE_STATEFUL | NPF_RULE_GSTATEFUL; }
 	|		{ $$ = 0; }
 	;
 

src/npfctl/npf_scan.l

@@ -125,7 +125,7 @@ block			return BLOCK;
 pass			return PASS;
 pcap-filter		return PCAP_FILTER;
 stateful		return STATEFUL;
-stateful-ends		return STATEFUL_ENDS;
+stateful-all		return STATEFUL_ALL;
 apply			return APPLY;
 final			return FINAL;
 quick			return FINAL;

src/npfctl/npf_show.c

@@ -246,7 +246,7 @@ print_portrange(npf_conf_info_t *ctx, const uint32_t *words)
  */
 
 #define	F(name)		__CONCAT(NPF_RULE_, name)
-#define	STATEFUL_ENDS	(NPF_RULE_STATEFUL | NPF_RULE_MULTIENDS)
+#define	STATEFUL_ALL	(NPF_RULE_STATEFUL | NPF_RULE_GSTATEFUL)
 #define	NAME_AT		2
 
 static const struct attr_keyword_mapent {
@@ -261,8 +261,8 @@ static const struct attr_keyword_mapent {
 	{ F(RETRST)|F(RETICMP),	F(RETRST)|F(RETICMP),	"return"	},
 	{ F(RETRST)|F(RETICMP),	F(RETRST),		"return-rst"	},
 	{ F(RETRST)|F(RETICMP),	F(RETICMP),		"return-icmp"	},
-	{ STATEFUL_ENDS,	F(STATEFUL),		"stateful"	},
-	{ STATEFUL_ENDS,	STATEFUL_ENDS,		"stateful-ends"	},
+	{ STATEFUL_ALL,		F(STATEFUL),		"stateful"	},
+	{ STATEFUL_ALL,		STATEFUL_ALL,		"stateful-all"	},
 	{ F(DIMASK),		F(IN),			"in"		},
 	{ F(DIMASK),		F(OUT),			"out"		},
 	{ F(FINAL),		F(FINAL),		"final"		},