[PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Marcelo Ricardo Leitner]

Commit d7ee35190427 ("netfilter: nf_ct_sctp: minimal multihoming
support") allowed creating conntrack entries based on the heartbeat
exchange, so that we can track secondary paths too.

This patch adds a vtag verification to that. That is, in order to allow
a HEARTBEAT or a HEARTBEAT_ACK through, the tuple (src port, dst port,
vtag) must be already known.

As it's possible that peer A initiated the connection but peer B ends up
being the first one to send a heartbeat, there is a cross-direction
check for matching the registered conntrack directions with the
heartbeat one.

Note that this vtag verification is only possible on peers or routers
that will see all paths in use by a given association. When used, it
will only allow secondary paths if the primary one is already there.
That is, it's usually only useable at the peers themselves, and thus it
defaults to off, controlled via sysctl
net.netfilter.nf_conntrack_sctp_validate_vtag.

As we can't really track associations but just their paths, it's
possible to have multiple hits of the 3-tuple to actually reflect
multiple associations, just as it's also possible to have different
vtags for a given pair of ports.

When a heartbeat comes by, it will allow it if the vtag is one of the
two (or n) in use, no matter which association it really belongs to.
Yet, if there is a lingering conntrack entry from a previous association
built from heartbeats for that 5-tuple (saddr, daddr, sport, dport,
sctp), like from a fast port reuse, it will reject packets with the new
vtag. This is actually a result of the normal conntrack entry
verification and is not a result of this patch or d7ee35190427.

SCTP has this particularity that peer addresses are negotiated during
initial handshake _and_ also via ASCONF chunks (RFC 5061). ASCONF chunks
must be authenticated, for security reasons, and as conntrack actually
sits in the middle even when used at the peers, it cannot validate the
authentication and thus it cannot trust ASCONF chunks, so we cannot
track IP addresses and thus we can't rebuild the association itself.

It is necessary that the path that initiated the association to continue
up as it's the only one that gets hashed and we cannot promote secondary
paths, due to the above. Once the primary path is down, secondary paths
using those vtags that expires won't be able to re-activate.

Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---

AFAICT the RFC resulted in no comments against this patch itself so I'm
now posting it officially. Thanks!

 Documentation/networking/nf_conntrack-sysctl.txt |   9 +
 include/linux/netfilter/nf_conntrack_sctp.h      |  17 ++
 net/netfilter/nf_conntrack_proto_sctp.c          | 251 ++++++++++++++++++++++-
 3 files changed, 276 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/nf_conntrack-sysctl.txt b/Documentation/networking/nf_conntrack-sysctl.txt
index f55599c62c9d61335005202146fdad75c8c133b9..9113fa12fee9548a99e5ea6b5241a4e882ffbfd0 100644
--- a/Documentation/networking/nf_conntrack-sysctl.txt
+++ b/Documentation/networking/nf_conntrack-sysctl.txt
@@ -175,3 +175,12 @@ nf_conntrack_udp_timeout_stream2 - INTEGER (seconds)
 
 	This extended timeout will be used in case there is an UDP stream
 	detected.
+
+nf_conntrack_sctp_validate_vtag - BOOLEAN
+	0 - disabled (default)
+	not 0 - enabled
+
+	Enable VTAG validation for conntrack entries created from
+	HEARTBEATs. That is, in order to allow such new entry, that vtag
+	must already be known for that port pair, no matter which
+	addresses are used. Most useful in end peers.
diff --git a/include/linux/netfilter/nf_conntrack_sctp.h b/include/linux/netfilter/nf_conntrack_sctp.h
index 22a16a23cd8a7e3d6d000ee7249477a3fef11604..8d20f79f5a2f4d0e93e2185f803100c45bc8662a 100644
--- a/include/linux/netfilter/nf_conntrack_sctp.h
+++ b/include/linux/netfilter/nf_conntrack_sctp.h
@@ -3,11 +3,28 @@
 /* SCTP tracking. */
 
 #include <uapi/linux/netfilter/nf_conntrack_sctp.h>
+#include <linux/rhashtable.h>
+
+struct nf_conn;
+struct sctp_vtaghash_node {
+	struct rhash_head node;
+	__be16 sport;
+	__be16 dport;
+	__be32 vtag;
+	struct net *net;
+	int dir;
+	atomic_t count;
+	struct rcu_head rcu_head;
+};
 
 struct ip_ct_sctp {
 	enum sctp_conntrack state;
 
 	__be32 vtag[IP_CT_DIR_MAX];
+
+	struct sctp_vtaghash_node *vtagnode[IP_CT_DIR_MAX];
+	bool crossed;
+	bool from_heartbeat;
 };
 
 #endif /* _NF_CONNTRACK_SCTP_H */
diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
index 9578a7c371ef2ce04f0a7b10c694b2533e29bd3a..f20be8f0bd5f9bd4ecfcfcdae1d400e89d79a54e 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -23,6 +23,7 @@
 #include <linux/seq_file.h>
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
+#include <net/netns/hash.h>
 
 #include <net/netfilter/nf_conntrack.h>
 #include <net/netfilter/nf_conntrack_l4proto.h>
@@ -144,10 +145,24 @@ static const u8 sctp_conntracks[2][11][SCTP_CONNTRACK_MAX] = {
 	}
 };
 
+/* vtag hash cmp arg, used for validating secondary paths */
+struct sctp_vtaghash_cmp_arg {
+	__be32 vtag;
+	__be16 sport;
+	__be16 dport;
+	struct net *net;
+	int dir;
+};
+
+static struct rhashtable vtaghash;
+
 static int sctp_net_id	__read_mostly;
 struct sctp_net {
 	struct nf_proto_net pn;
 	unsigned int timeouts[SCTP_CONNTRACK_MAX];
+	bool validate_vtag;
+	char *vtag_slabname;
+	struct kmem_cache *vtag_cachep;
 };
 
 static inline struct sctp_net *sctp_pernet(struct net *net)
@@ -200,6 +215,156 @@ static void sctp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
 	seq_printf(s, "%s ", sctp_conntrack_names[state]);
 }
 
+/* vtag hash functions */
+static inline int sctp_vtag_hash_cmp(struct rhashtable_compare_arg *_arg,
+				     const void *ptr)
+{
+	const struct sctp_vtaghash_cmp_arg *arg = _arg->key;
+	const struct sctp_vtaghash_node *node = ptr;
+
+	if (arg->dir = node->dir)
+		return node->vtag != arg->vtag ||
+		       !net_eq(node->net, arg->net) ||
+		       node->sport != arg->sport ||
+		       node->dport != arg->dport;
+
+	return node->vtag != arg->vtag ||
+	       !net_eq(node->net, arg->net) ||
+	       node->sport != arg->dport ||
+	       node->dport != arg->sport;
+}
+
+static inline u32 sctp_vtag_hashfn(const void *data, u32 len, u32 seed)
+{
+	const struct sctp_vtaghash_cmp_arg *arg = data;
+
+	return jhash_3words(arg->vtag, arg->sport, arg->dport,
+			    seed + net_hash_mix(arg->net));
+}
+
+static inline u32 sctp_vtag_hash_obj(const void *data, u32 len, u32 seed)
+{
+	const struct sctp_vtaghash_node *node = data;
+
+	return jhash_3words(node->vtag, node->sport, node->dport,
+			    seed + net_hash_mix(node->net));
+}
+
+static const struct rhashtable_params sctp_vtaghash_params = {
+	.head_offset = offsetof(struct sctp_vtaghash_node, node),
+	.hashfn = sctp_vtag_hashfn,
+	.obj_hashfn = sctp_vtag_hash_obj,
+	.obj_cmpfn = sctp_vtag_hash_cmp,
+	.automatic_shrinking = true,
+};
+
+static inline void sctp_vtag_arg_init(struct sctp_vtaghash_cmp_arg *arg,
+				      struct nf_conn *ct, int dir)
+{
+	arg->vtag = ct->proto.sctp.vtag[dir];
+	arg->net = nf_ct_net(ct);
+	arg->sport = nf_ct_tuple(ct, dir)->src.u.sctp.port;
+	arg->dport = nf_ct_tuple(ct, dir)->dst.u.sctp.port;
+	arg->dir = ct->proto.sctp.crossed ? !dir : dir;
+}
+
+static inline void sctp_vtag_unhash(struct nf_conn *ct, int dir)
+{
+	struct sctp_vtaghash_node *node = ct->proto.sctp.vtagnode[dir];
+
+	if (!node)
+		return;
+
+	if (atomic_dec_and_test(&node->count)) {
+		rhashtable_remove_fast(&vtaghash,
+				       &node->node,
+				       sctp_vtaghash_params);
+		kfree_rcu(node, rcu_head);
+	}
+
+	ct->proto.sctp.vtagnode[dir] = NULL;
+}
+
+static inline int sctp_vtag_hash(struct nf_conn *ct, int dir)
+{
+	struct sctp_net *sn = sctp_pernet(nf_ct_net(ct));
+	struct sctp_vtaghash_cmp_arg arg;
+	struct sctp_vtaghash_node *node;
+	int ret;
+
+	if (ct->proto.sctp.vtagnode[dir])
+		sctp_vtag_unhash(ct, dir);
+
+again:
+	sctp_vtag_arg_init(&arg, ct, dir);
+	rcu_read_lock();
+	node = rhashtable_lookup_fast(&vtaghash, &arg, sctp_vtaghash_params);
+	if (node && atomic_add_unless(&node->count, 1, 0)) {
+		ct->proto.sctp.vtagnode[dir] = node;
+		rcu_read_unlock();
+		return 0;
+	}
+	rcu_read_unlock();
+
+	node = kmem_cache_alloc(sn->vtag_cachep, GFP_ATOMIC);
+	if (!node)
+		return -ENOMEM;
+
+	node->vtag = arg.vtag;
+	node->sport = arg.sport;
+	node->dport = arg.dport;
+	node->net = arg.net;
+	node->dir = dir;
+	atomic_set(&node->count, 1);
+
+	ret = rhashtable_lookup_insert_key(&vtaghash, &arg, &node->node,
+					   sctp_vtaghash_params);
+	if (ret = 0) {
+		ct->proto.sctp.vtagnode[dir] = node;
+	} else {
+		kfree(node);
+		if (ret = -EEXIST)
+			goto again;
+	}
+
+	return ret;
+}
+
+/* This function checks if there is a known vtag being used with the
+ * given pair of src/dst ports. If yes, it returns true.
+ * Note that it doesn't, as it can't, validate any IP addresses.
+ */
+static inline bool sctp_vtag_is_hashed(struct nf_conn *ct, int dir)
+{
+	struct sctp_vtaghash_cmp_arg arg;
+
+	sctp_vtag_arg_init(&arg, ct, dir);
+
+	return rhashtable_lookup_fast(&vtaghash, &arg,
+				      sctp_vtaghash_params) != NULL;
+}
+
+/* This function checks if there is a known vtag being used with the
+ * given pair of src/dst ports.
+ * Note that it doesn't, as it can't, validate any IP addresses.
+ * Returns the direction on which the tag is hash, or -1 if none
+ */
+static inline int sctp_vtag_hash_probe(struct nf_conn *ct)
+{
+	struct sctp_vtaghash_cmp_arg arg;
+
+	sctp_vtag_arg_init(&arg, ct, IP_CT_DIR_ORIGINAL);
+
+	if (rhashtable_lookup_fast(&vtaghash, &arg, sctp_vtaghash_params))
+		return arg.dir;
+
+	arg.dir = !arg.dir;
+	if (rhashtable_lookup_fast(&vtaghash, &arg, sctp_vtaghash_params))
+		return arg.dir;
+
+	return -1;
+}
+
 #define for_each_sctp_chunk(skb, sch, _sch, offset, dataoff, count)	\
 for ((offset) = (dataoff) + sizeof(sctp_sctphdr_t), (count) = 0;	\
 	(offset) < (skb)->len &&					\
@@ -328,6 +493,7 @@ static int sctp_packet(struct nf_conn *ct,
 		       unsigned int hooknum,
 		       unsigned int *timeouts)
 {
+	struct sctp_net *sn = sctp_pernet(nf_ct_net(ct));
 	enum sctp_conntrack new_state, old_state;
 	enum ip_conntrack_dir dir = CTINFO2DIR(ctinfo);
 	const struct sctphdr *sh;
@@ -390,6 +556,12 @@ static int sctp_packet(struct nf_conn *ct,
 				pr_debug("Verification tag check failed\n");
 				goto out_unlock;
 			}
+			if (sn->validate_vtag &&
+			    ct->proto.sctp.from_heartbeat &&
+			    !sctp_vtag_is_hashed(ct, dir)) {
+				pr_debug("heartbeat with unknown verification tag\n");
+				goto out_unlock;
+			}
 		}
 
 		old_state = ct->proto.sctp.state;
@@ -415,6 +587,10 @@ static int sctp_packet(struct nf_conn *ct,
 			pr_debug("Setting vtag %x for dir %d\n",
 				 ih->init_tag, !dir);
 			ct->proto.sctp.vtag[!dir] = ih->init_tag;
+			ct->proto.sctp.from_heartbeat = false;
+			ct->proto.sctp.crossed = false;
+			if (sn->validate_vtag && sctp_vtag_hash(ct, !dir))
+				goto out_unlock;
 		}
 
 		ct->proto.sctp.state = new_state;
@@ -445,6 +621,7 @@ out:
 static bool sctp_new(struct nf_conn *ct, const struct sk_buff *skb,
 		     unsigned int dataoff, unsigned int *timeouts)
 {
+	struct sctp_net *sn = sctp_pernet(nf_ct_net(ct));
 	enum sctp_conntrack new_state;
 	const struct sctphdr *sh;
 	struct sctphdr _sctph;
@@ -503,6 +680,19 @@ static bool sctp_new(struct nf_conn *ct, const struct sk_buff *skb,
 			pr_debug("Setting vtag %x for secondary conntrack\n",
 				 sh->vtag);
 			ct->proto.sctp.vtag[IP_CT_DIR_ORIGINAL] = sh->vtag;
+
+			if (sn->validate_vtag) {
+				int hashdir;
+
+				hashdir = sctp_vtag_hash_probe(ct);
+				if (hashdir = -1) {
+					pr_debug("but vtag is not in use\n");
+					return false;
+				}
+				ct->proto.sctp.crossed +						hashdir != IP_CT_DIR_ORIGINAL;
+				ct->proto.sctp.from_heartbeat = true;
+			}
 		}
 		/* If it is a shutdown ack OOTB packet, we expect a return
 		   shutdown complete, otherwise an ABORT Sec 8.4 (5) and (8) */
@@ -518,6 +708,15 @@ static bool sctp_new(struct nf_conn *ct, const struct sk_buff *skb,
 	return true;
 }
 
+static void sctp_destroy(struct nf_conn *ct)
+{
+	/* Don't check for validate_vtag because it may have been
+	 * disabled while we already had an entry hashed.
+	 */
+	sctp_vtag_unhash(ct, IP_CT_DIR_ORIGINAL);
+	sctp_vtag_unhash(ct, IP_CT_DIR_REPLY);
+}
+
 #if IS_ENABLED(CONFIG_NF_CT_NETLINK)
 
 #include <linux/netfilter/nfnetlink.h>
@@ -559,6 +758,7 @@ static const struct nla_policy sctp_nla_policy[CTA_PROTOINFO_SCTP_MAX+1] = {
 
 static int nlattr_to_sctp(struct nlattr *cda[], struct nf_conn *ct)
 {
+	struct sctp_net *sn = sctp_pernet(nf_ct_net(ct));
 	struct nlattr *attr = cda[CTA_PROTOINFO_SCTP];
 	struct nlattr *tb[CTA_PROTOINFO_SCTP_MAX+1];
 	int err;
@@ -585,9 +785,16 @@ static int nlattr_to_sctp(struct nlattr *cda[], struct nf_conn *ct)
 		nla_get_be32(tb[CTA_PROTOINFO_SCTP_VTAG_ORIGINAL]);
 	ct->proto.sctp.vtag[IP_CT_DIR_REPLY]  		nla_get_be32(tb[CTA_PROTOINFO_SCTP_VTAG_REPLY]);
+	ct->proto.sctp.crossed = false;
+	ct->proto.sctp.from_heartbeat = false;
+	if (sn->validate_vtag) {
+		err = sctp_vtag_hash(ct, IP_CT_DIR_ORIGINAL);
+		if (!err)
+			err = sctp_vtag_hash(ct, IP_CT_DIR_REPLY);
+	}
 	spin_unlock_bh(&ct->lock);
 
-	return 0;
+	return err;
 }
 
 static int sctp_nlattr_size(void)
@@ -709,6 +916,12 @@ static struct ctl_table sctp_sysctl_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_jiffies,
 	},
+	{
+		.procname	= "nf_conntrack_sctp_validate_vtag",
+		.maxlen		= sizeof(unsigned int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_jiffies,
+	},
 	{ }
 };
 
@@ -783,6 +996,7 @@ static int sctp_kmemdup_sysctl_table(struct nf_proto_net *pn,
 	pn->ctl_table[6].data = &sn->timeouts[SCTP_CONNTRACK_SHUTDOWN_ACK_SENT];
 	pn->ctl_table[7].data = &sn->timeouts[SCTP_CONNTRACK_HEARTBEAT_SENT];
 	pn->ctl_table[8].data = &sn->timeouts[SCTP_CONNTRACK_HEARTBEAT_ACKED];
+	pn->ctl_table[9].data = &sn->validate_vtag;
 #endif
 	return 0;
 }
@@ -848,6 +1062,7 @@ static struct nf_conntrack_l4proto nf_conntrack_l4proto_sctp4 __read_mostly = {
 	.packet 		= sctp_packet,
 	.get_timeouts		= sctp_get_timeouts,
 	.new 			= sctp_new,
+	.destroy		= sctp_destroy,
 	.me 			= THIS_MODULE,
 #if IS_ENABLED(CONFIG_NF_CT_NETLINK)
 	.to_nlattr		= sctp_to_nlattr,
@@ -882,6 +1097,7 @@ static struct nf_conntrack_l4proto nf_conntrack_l4proto_sctp6 __read_mostly = {
 	.packet 		= sctp_packet,
 	.get_timeouts		= sctp_get_timeouts,
 	.new 			= sctp_new,
+	.destroy		= sctp_destroy,
 	.me 			= THIS_MODULE,
 #if IS_ENABLED(CONFIG_NF_CT_NETLINK)
 	.to_nlattr		= sctp_to_nlattr,
@@ -907,6 +1123,7 @@ static struct nf_conntrack_l4proto nf_conntrack_l4proto_sctp6 __read_mostly = {
 
 static int sctp_net_init(struct net *net)
 {
+	struct sctp_net *sn = sctp_pernet(net);
 	int ret = 0;
 
 	ret = nf_ct_l4proto_pernet_register(net, &nf_conntrack_l4proto_sctp4);
@@ -914,13 +1131,34 @@ static int sctp_net_init(struct net *net)
 		pr_err("nf_conntrack_sctp4: pernet registration failed.\n");
 		goto out;
 	}
+
 	ret = nf_ct_l4proto_pernet_register(net, &nf_conntrack_l4proto_sctp6);
 	if (ret < 0) {
 		pr_err("nf_conntrack_sctp6: pernet registration failed.\n");
 		goto cleanup_sctp4;
 	}
+
+	sn->vtag_slabname = kasprintf(GFP_KERNEL, "nf_conntrack_%p_sctp_vtag",
+				      net);
+	if (!sn->vtag_slabname) {
+		ret = -ENOMEM;
+		goto cleanup_sctp6;
+	}
+
+	sn->vtag_cachep = kmem_cache_create(sn->vtag_slabname,
+					    sizeof(struct sctp_vtaghash_node),
+					    0, 0, NULL);
+	if (!sn->vtag_cachep) {
+		ret = -ENOMEM;
+		goto slab_err;
+	}
+
 	return 0;
 
+slab_err:
+	kfree(sn->vtag_slabname);
+cleanup_sctp6:
+	nf_ct_l4proto_pernet_unregister(net, &nf_conntrack_l4proto_sctp6);
 cleanup_sctp4:
 	nf_ct_l4proto_pernet_unregister(net, &nf_conntrack_l4proto_sctp4);
 out:
@@ -929,8 +1167,12 @@ out:
 
 static void sctp_net_exit(struct net *net)
 {
+	struct sctp_net *sn = sctp_pernet(net);
+
 	nf_ct_l4proto_pernet_unregister(net, &nf_conntrack_l4proto_sctp6);
 	nf_ct_l4proto_pernet_unregister(net, &nf_conntrack_l4proto_sctp4);
+	kfree(sn->vtag_slabname);
+	kmem_cache_destroy(sn->vtag_cachep);
 }
 
 static struct pernet_operations sctp_net_ops = {
@@ -944,6 +1186,10 @@ static int __init nf_conntrack_proto_sctp_init(void)
 {
 	int ret;
 
+	ret = rhashtable_init(&vtaghash, &sctp_vtaghash_params);
+	if (ret < 0)
+		goto out_hash;
+
 	ret = register_pernet_subsys(&sctp_net_ops);
 	if (ret < 0)
 		goto out_pernet;
@@ -962,6 +1208,8 @@ out_sctp6:
 out_sctp4:
 	unregister_pernet_subsys(&sctp_net_ops);
 out_pernet:
+	rhashtable_destroy(&vtaghash);
+out_hash:
 	return ret;
 }
 
@@ -970,6 +1218,7 @@ static void __exit nf_conntrack_proto_sctp_fini(void)
 	nf_ct_l4proto_unregister(&nf_conntrack_l4proto_sctp6);
 	nf_ct_l4proto_unregister(&nf_conntrack_l4proto_sctp4);
 	unregister_pernet_subsys(&sctp_net_ops);
+	rhashtable_destroy(&vtaghash);
 }
 
 module_init(nf_conntrack_proto_sctp_init);
-- 
2.5.0

[PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Marcelo Ricardo Leitner]

Commit d7ee35190427 ("netfilter: nf_ct_sctp: minimal multihoming
support") allowed creating conntrack entries based on the heartbeat
exchange, so that we can track secondary paths too.

This patch adds a vtag verification to that. That is, in order to allow
a HEARTBEAT or a HEARTBEAT_ACK through, the tuple (src port, dst port,
vtag) must be already known.

As it's possible that peer A initiated the connection but peer B ends up
being the first one to send a heartbeat, there is a cross-direction
check for matching the registered conntrack directions with the
heartbeat one.

Note that this vtag verification is only possible on peers or routers
that will see all paths in use by a given association. When used, it
will only allow secondary paths if the primary one is already there.
That is, it's usually only useable at the peers themselves, and thus it
defaults to off, controlled via sysctl
net.netfilter.nf_conntrack_sctp_validate_vtag.

As we can't really track associations but just their paths, it's
possible to have multiple hits of the 3-tuple to actually reflect
multiple associations, just as it's also possible to have different
vtags for a given pair of ports.

When a heartbeat comes by, it will allow it if the vtag is one of the
two (or n) in use, no matter which association it really belongs to.
Yet, if there is a lingering conntrack entry from a previous association
built from heartbeats for that 5-tuple (saddr, daddr, sport, dport,
sctp), like from a fast port reuse, it will reject packets with the new
vtag. This is actually a result of the normal conntrack entry
verification and is not a result of this patch or d7ee35190427.

SCTP has this particularity that peer addresses are negotiated during
initial handshake _and_ also via ASCONF chunks (RFC 5061). ASCONF chunks
must be authenticated, for security reasons, and as conntrack actually
sits in the middle even when used at the peers, it cannot validate the
authentication and thus it cannot trust ASCONF chunks, so we cannot
track IP addresses and thus we can't rebuild the association itself.

It is necessary that the path that initiated the association to continue
up as it's the only one that gets hashed and we cannot promote secondary
paths, due to the above. Once the primary path is down, secondary paths
using those vtags that expires won't be able to re-activate.

Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---

AFAICT the RFC resulted in no comments against this patch itself so I'm
now posting it officially. Thanks!

 Documentation/networking/nf_conntrack-sysctl.txt |   9 +
 include/linux/netfilter/nf_conntrack_sctp.h      |  17 ++
 net/netfilter/nf_conntrack_proto_sctp.c          | 251 ++++++++++++++++++++++-
 3 files changed, 276 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/nf_conntrack-sysctl.txt b/Documentation/networking/nf_conntrack-sysctl.txt
index f55599c62c9d61335005202146fdad75c8c133b9..9113fa12fee9548a99e5ea6b5241a4e882ffbfd0 100644
--- a/Documentation/networking/nf_conntrack-sysctl.txt
+++ b/Documentation/networking/nf_conntrack-sysctl.txt
@@ -175,3 +175,12 @@ nf_conntrack_udp_timeout_stream2 - INTEGER (seconds)
 
 	This extended timeout will be used in case there is an UDP stream
 	detected.
+
+nf_conntrack_sctp_validate_vtag - BOOLEAN
+	0 - disabled (default)
+	not 0 - enabled
+
+	Enable VTAG validation for conntrack entries created from
+	HEARTBEATs. That is, in order to allow such new entry, that vtag
+	must already be known for that port pair, no matter which
+	addresses are used. Most useful in end peers.
diff --git a/include/linux/netfilter/nf_conntrack_sctp.h b/include/linux/netfilter/nf_conntrack_sctp.h
index 22a16a23cd8a7e3d6d000ee7249477a3fef11604..8d20f79f5a2f4d0e93e2185f803100c45bc8662a 100644
--- a/include/linux/netfilter/nf_conntrack_sctp.h
+++ b/include/linux/netfilter/nf_conntrack_sctp.h
@@ -3,11 +3,28 @@
 /* SCTP tracking. */
 
 #include <uapi/linux/netfilter/nf_conntrack_sctp.h>
+#include <linux/rhashtable.h>
+
+struct nf_conn;
+struct sctp_vtaghash_node {
+	struct rhash_head node;
+	__be16 sport;
+	__be16 dport;
+	__be32 vtag;
+	struct net *net;
+	int dir;
+	atomic_t count;
+	struct rcu_head rcu_head;
+};
 
 struct ip_ct_sctp {
 	enum sctp_conntrack state;
 
 	__be32 vtag[IP_CT_DIR_MAX];
+
+	struct sctp_vtaghash_node *vtagnode[IP_CT_DIR_MAX];
+	bool crossed;
+	bool from_heartbeat;
 };
 
 #endif /* _NF_CONNTRACK_SCTP_H */
diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
index 9578a7c371ef2ce04f0a7b10c694b2533e29bd3a..f20be8f0bd5f9bd4ecfcfcdae1d400e89d79a54e 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -23,6 +23,7 @@
 #include <linux/seq_file.h>
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
+#include <net/netns/hash.h>
 
 #include <net/netfilter/nf_conntrack.h>
 #include <net/netfilter/nf_conntrack_l4proto.h>
@@ -144,10 +145,24 @@ static const u8 sctp_conntracks[2][11][SCTP_CONNTRACK_MAX] = {
 	}
 };
 
+/* vtag hash cmp arg, used for validating secondary paths */
+struct sctp_vtaghash_cmp_arg {
+	__be32 vtag;
+	__be16 sport;
+	__be16 dport;
+	struct net *net;
+	int dir;
+};
+
+static struct rhashtable vtaghash;
+
 static int sctp_net_id	__read_mostly;
 struct sctp_net {
 	struct nf_proto_net pn;
 	unsigned int timeouts[SCTP_CONNTRACK_MAX];
+	bool validate_vtag;
+	char *vtag_slabname;
+	struct kmem_cache *vtag_cachep;
 };
 
 static inline struct sctp_net *sctp_pernet(struct net *net)
@@ -200,6 +215,156 @@ static void sctp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
 	seq_printf(s, "%s ", sctp_conntrack_names[state]);
 }
 
+/* vtag hash functions */
+static inline int sctp_vtag_hash_cmp(struct rhashtable_compare_arg *_arg,
+				     const void *ptr)
+{
+	const struct sctp_vtaghash_cmp_arg *arg = _arg->key;
+	const struct sctp_vtaghash_node *node = ptr;
+
+	if (arg->dir == node->dir)
+		return node->vtag != arg->vtag ||
+		       !net_eq(node->net, arg->net) ||
+		       node->sport != arg->sport ||
+		       node->dport != arg->dport;
+
+	return node->vtag != arg->vtag ||
+	       !net_eq(node->net, arg->net) ||
+	       node->sport != arg->dport ||
+	       node->dport != arg->sport;
+}
+
+static inline u32 sctp_vtag_hashfn(const void *data, u32 len, u32 seed)
+{
+	const struct sctp_vtaghash_cmp_arg *arg = data;
+
+	return jhash_3words(arg->vtag, arg->sport, arg->dport,
+			    seed + net_hash_mix(arg->net));
+}
+
+static inline u32 sctp_vtag_hash_obj(const void *data, u32 len, u32 seed)
+{
+	const struct sctp_vtaghash_node *node = data;
+
+	return jhash_3words(node->vtag, node->sport, node->dport,
+			    seed + net_hash_mix(node->net));
+}
+
+static const struct rhashtable_params sctp_vtaghash_params = {
+	.head_offset = offsetof(struct sctp_vtaghash_node, node),
+	.hashfn = sctp_vtag_hashfn,
+	.obj_hashfn = sctp_vtag_hash_obj,
+	.obj_cmpfn = sctp_vtag_hash_cmp,
+	.automatic_shrinking = true,
+};
+
+static inline void sctp_vtag_arg_init(struct sctp_vtaghash_cmp_arg *arg,
+				      struct nf_conn *ct, int dir)
+{
+	arg->vtag = ct->proto.sctp.vtag[dir];
+	arg->net = nf_ct_net(ct);
+	arg->sport = nf_ct_tuple(ct, dir)->src.u.sctp.port;
+	arg->dport = nf_ct_tuple(ct, dir)->dst.u.sctp.port;
+	arg->dir = ct->proto.sctp.crossed ? !dir : dir;
+}
+
+static inline void sctp_vtag_unhash(struct nf_conn *ct, int dir)
+{
+	struct sctp_vtaghash_node *node = ct->proto.sctp.vtagnode[dir];
+
+	if (!node)
+		return;
+
+	if (atomic_dec_and_test(&node->count)) {
+		rhashtable_remove_fast(&vtaghash,
+				       &node->node,
+				       sctp_vtaghash_params);
+		kfree_rcu(node, rcu_head);
+	}
+
+	ct->proto.sctp.vtagnode[dir] = NULL;
+}
+
+static inline int sctp_vtag_hash(struct nf_conn *ct, int dir)
+{
+	struct sctp_net *sn = sctp_pernet(nf_ct_net(ct));
+	struct sctp_vtaghash_cmp_arg arg;
+	struct sctp_vtaghash_node *node;
+	int ret;
+
+	if (ct->proto.sctp.vtagnode[dir])
+		sctp_vtag_unhash(ct, dir);
+
+again:
+	sctp_vtag_arg_init(&arg, ct, dir);
+	rcu_read_lock();
+	node = rhashtable_lookup_fast(&vtaghash, &arg, sctp_vtaghash_params);
+	if (node && atomic_add_unless(&node->count, 1, 0)) {
+		ct->proto.sctp.vtagnode[dir] = node;
+		rcu_read_unlock();
+		return 0;
+	}
+	rcu_read_unlock();
+
+	node = kmem_cache_alloc(sn->vtag_cachep, GFP_ATOMIC);
+	if (!node)
+		return -ENOMEM;
+
+	node->vtag = arg.vtag;
+	node->sport = arg.sport;
+	node->dport = arg.dport;
+	node->net = arg.net;
+	node->dir = dir;
+	atomic_set(&node->count, 1);
+
+	ret = rhashtable_lookup_insert_key(&vtaghash, &arg, &node->node,
+					   sctp_vtaghash_params);
+	if (ret == 0) {
+		ct->proto.sctp.vtagnode[dir] = node;
+	} else {
+		kfree(node);
+		if (ret == -EEXIST)
+			goto again;
+	}
+
+	return ret;
+}
+
+/* This function checks if there is a known vtag being used with the
+ * given pair of src/dst ports. If yes, it returns true.
+ * Note that it doesn't, as it can't, validate any IP addresses.
+ */
+static inline bool sctp_vtag_is_hashed(struct nf_conn *ct, int dir)
+{
+	struct sctp_vtaghash_cmp_arg arg;
+
+	sctp_vtag_arg_init(&arg, ct, dir);
+
+	return rhashtable_lookup_fast(&vtaghash, &arg,
+				      sctp_vtaghash_params) != NULL;
+}
+
+/* This function checks if there is a known vtag being used with the
+ * given pair of src/dst ports.
+ * Note that it doesn't, as it can't, validate any IP addresses.
+ * Returns the direction on which the tag is hash, or -1 if none
+ */
+static inline int sctp_vtag_hash_probe(struct nf_conn *ct)
+{
+	struct sctp_vtaghash_cmp_arg arg;
+
+	sctp_vtag_arg_init(&arg, ct, IP_CT_DIR_ORIGINAL);
+
+	if (rhashtable_lookup_fast(&vtaghash, &arg, sctp_vtaghash_params))
+		return arg.dir;
+
+	arg.dir = !arg.dir;
+	if (rhashtable_lookup_fast(&vtaghash, &arg, sctp_vtaghash_params))
+		return arg.dir;
+
+	return -1;
+}
+
 #define for_each_sctp_chunk(skb, sch, _sch, offset, dataoff, count)	\
 for ((offset) = (dataoff) + sizeof(sctp_sctphdr_t), (count) = 0;	\
 	(offset) < (skb)->len &&					\
@@ -328,6 +493,7 @@ static int sctp_packet(struct nf_conn *ct,
 		       unsigned int hooknum,
 		       unsigned int *timeouts)
 {
+	struct sctp_net *sn = sctp_pernet(nf_ct_net(ct));
 	enum sctp_conntrack new_state, old_state;
 	enum ip_conntrack_dir dir = CTINFO2DIR(ctinfo);
 	const struct sctphdr *sh;
@@ -390,6 +556,12 @@ static int sctp_packet(struct nf_conn *ct,
 				pr_debug("Verification tag check failed\n");
 				goto out_unlock;
 			}
+			if (sn->validate_vtag &&
+			    ct->proto.sctp.from_heartbeat &&
+			    !sctp_vtag_is_hashed(ct, dir)) {
+				pr_debug("heartbeat with unknown verification tag\n");
+				goto out_unlock;
+			}
 		}
 
 		old_state = ct->proto.sctp.state;
@@ -415,6 +587,10 @@ static int sctp_packet(struct nf_conn *ct,
 			pr_debug("Setting vtag %x for dir %d\n",
 				 ih->init_tag, !dir);
 			ct->proto.sctp.vtag[!dir] = ih->init_tag;
+			ct->proto.sctp.from_heartbeat = false;
+			ct->proto.sctp.crossed = false;
+			if (sn->validate_vtag && sctp_vtag_hash(ct, !dir))
+				goto out_unlock;
 		}
 
 		ct->proto.sctp.state = new_state;
@@ -445,6 +621,7 @@ out:
 static bool sctp_new(struct nf_conn *ct, const struct sk_buff *skb,
 		     unsigned int dataoff, unsigned int *timeouts)
 {
+	struct sctp_net *sn = sctp_pernet(nf_ct_net(ct));
 	enum sctp_conntrack new_state;
 	const struct sctphdr *sh;
 	struct sctphdr _sctph;
@@ -503,6 +680,19 @@ static bool sctp_new(struct nf_conn *ct, const struct sk_buff *skb,
 			pr_debug("Setting vtag %x for secondary conntrack\n",
 				 sh->vtag);
 			ct->proto.sctp.vtag[IP_CT_DIR_ORIGINAL] = sh->vtag;
+
+			if (sn->validate_vtag) {
+				int hashdir;
+
+				hashdir = sctp_vtag_hash_probe(ct);
+				if (hashdir == -1) {
+					pr_debug("but vtag is not in use\n");
+					return false;
+				}
+				ct->proto.sctp.crossed =
+						hashdir != IP_CT_DIR_ORIGINAL;
+				ct->proto.sctp.from_heartbeat = true;
+			}
 		}
 		/* If it is a shutdown ack OOTB packet, we expect a return
 		   shutdown complete, otherwise an ABORT Sec 8.4 (5) and (8) */
@@ -518,6 +708,15 @@ static bool sctp_new(struct nf_conn *ct, const struct sk_buff *skb,
 	return true;
 }
 
+static void sctp_destroy(struct nf_conn *ct)
+{
+	/* Don't check for validate_vtag because it may have been
+	 * disabled while we already had an entry hashed.
+	 */
+	sctp_vtag_unhash(ct, IP_CT_DIR_ORIGINAL);
+	sctp_vtag_unhash(ct, IP_CT_DIR_REPLY);
+}
+
 #if IS_ENABLED(CONFIG_NF_CT_NETLINK)
 
 #include <linux/netfilter/nfnetlink.h>
@@ -559,6 +758,7 @@ static const struct nla_policy sctp_nla_policy[CTA_PROTOINFO_SCTP_MAX+1] = {
 
 static int nlattr_to_sctp(struct nlattr *cda[], struct nf_conn *ct)
 {
+	struct sctp_net *sn = sctp_pernet(nf_ct_net(ct));
 	struct nlattr *attr = cda[CTA_PROTOINFO_SCTP];
 	struct nlattr *tb[CTA_PROTOINFO_SCTP_MAX+1];
 	int err;
@@ -585,9 +785,16 @@ static int nlattr_to_sctp(struct nlattr *cda[], struct nf_conn *ct)
 		nla_get_be32(tb[CTA_PROTOINFO_SCTP_VTAG_ORIGINAL]);
 	ct->proto.sctp.vtag[IP_CT_DIR_REPLY] =
 		nla_get_be32(tb[CTA_PROTOINFO_SCTP_VTAG_REPLY]);
+	ct->proto.sctp.crossed = false;
+	ct->proto.sctp.from_heartbeat = false;
+	if (sn->validate_vtag) {
+		err = sctp_vtag_hash(ct, IP_CT_DIR_ORIGINAL);
+		if (!err)
+			err = sctp_vtag_hash(ct, IP_CT_DIR_REPLY);
+	}
 	spin_unlock_bh(&ct->lock);
 
-	return 0;
+	return err;
 }
 
 static int sctp_nlattr_size(void)
@@ -709,6 +916,12 @@ static struct ctl_table sctp_sysctl_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_jiffies,
 	},
+	{
+		.procname	= "nf_conntrack_sctp_validate_vtag",
+		.maxlen		= sizeof(unsigned int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_jiffies,
+	},
 	{ }
 };
 
@@ -783,6 +996,7 @@ static int sctp_kmemdup_sysctl_table(struct nf_proto_net *pn,
 	pn->ctl_table[6].data = &sn->timeouts[SCTP_CONNTRACK_SHUTDOWN_ACK_SENT];
 	pn->ctl_table[7].data = &sn->timeouts[SCTP_CONNTRACK_HEARTBEAT_SENT];
 	pn->ctl_table[8].data = &sn->timeouts[SCTP_CONNTRACK_HEARTBEAT_ACKED];
+	pn->ctl_table[9].data = &sn->validate_vtag;
 #endif
 	return 0;
 }
@@ -848,6 +1062,7 @@ static struct nf_conntrack_l4proto nf_conntrack_l4proto_sctp4 __read_mostly = {
 	.packet 		= sctp_packet,
 	.get_timeouts		= sctp_get_timeouts,
 	.new 			= sctp_new,
+	.destroy		= sctp_destroy,
 	.me 			= THIS_MODULE,
 #if IS_ENABLED(CONFIG_NF_CT_NETLINK)
 	.to_nlattr		= sctp_to_nlattr,
@@ -882,6 +1097,7 @@ static struct nf_conntrack_l4proto nf_conntrack_l4proto_sctp6 __read_mostly = {
 	.packet 		= sctp_packet,
 	.get_timeouts		= sctp_get_timeouts,
 	.new 			= sctp_new,
+	.destroy		= sctp_destroy,
 	.me 			= THIS_MODULE,
 #if IS_ENABLED(CONFIG_NF_CT_NETLINK)
 	.to_nlattr		= sctp_to_nlattr,
@@ -907,6 +1123,7 @@ static struct nf_conntrack_l4proto nf_conntrack_l4proto_sctp6 __read_mostly = {
 
 static int sctp_net_init(struct net *net)
 {
+	struct sctp_net *sn = sctp_pernet(net);
 	int ret = 0;
 
 	ret = nf_ct_l4proto_pernet_register(net, &nf_conntrack_l4proto_sctp4);
@@ -914,13 +1131,34 @@ static int sctp_net_init(struct net *net)
 		pr_err("nf_conntrack_sctp4: pernet registration failed.\n");
 		goto out;
 	}
+
 	ret = nf_ct_l4proto_pernet_register(net, &nf_conntrack_l4proto_sctp6);
 	if (ret < 0) {
 		pr_err("nf_conntrack_sctp6: pernet registration failed.\n");
 		goto cleanup_sctp4;
 	}
+
+	sn->vtag_slabname = kasprintf(GFP_KERNEL, "nf_conntrack_%p_sctp_vtag",
+				      net);
+	if (!sn->vtag_slabname) {
+		ret = -ENOMEM;
+		goto cleanup_sctp6;
+	}
+
+	sn->vtag_cachep = kmem_cache_create(sn->vtag_slabname,
+					    sizeof(struct sctp_vtaghash_node),
+					    0, 0, NULL);
+	if (!sn->vtag_cachep) {
+		ret = -ENOMEM;
+		goto slab_err;
+	}
+
 	return 0;
 
+slab_err:
+	kfree(sn->vtag_slabname);
+cleanup_sctp6:
+	nf_ct_l4proto_pernet_unregister(net, &nf_conntrack_l4proto_sctp6);
 cleanup_sctp4:
 	nf_ct_l4proto_pernet_unregister(net, &nf_conntrack_l4proto_sctp4);
 out:
@@ -929,8 +1167,12 @@ out:
 
 static void sctp_net_exit(struct net *net)
 {
+	struct sctp_net *sn = sctp_pernet(net);
+
 	nf_ct_l4proto_pernet_unregister(net, &nf_conntrack_l4proto_sctp6);
 	nf_ct_l4proto_pernet_unregister(net, &nf_conntrack_l4proto_sctp4);
+	kfree(sn->vtag_slabname);
+	kmem_cache_destroy(sn->vtag_cachep);
 }
 
 static struct pernet_operations sctp_net_ops = {
@@ -944,6 +1186,10 @@ static int __init nf_conntrack_proto_sctp_init(void)
 {
 	int ret;
 
+	ret = rhashtable_init(&vtaghash, &sctp_vtaghash_params);
+	if (ret < 0)
+		goto out_hash;
+
 	ret = register_pernet_subsys(&sctp_net_ops);
 	if (ret < 0)
 		goto out_pernet;
@@ -962,6 +1208,8 @@ out_sctp6:
 out_sctp4:
 	unregister_pernet_subsys(&sctp_net_ops);
 out_pernet:
+	rhashtable_destroy(&vtaghash);
+out_hash:
 	return ret;
 }
 
@@ -970,6 +1218,7 @@ static void __exit nf_conntrack_proto_sctp_fini(void)
 	nf_ct_l4proto_unregister(&nf_conntrack_l4proto_sctp6);
 	nf_ct_l4proto_unregister(&nf_conntrack_l4proto_sctp4);
 	unregister_pernet_subsys(&sctp_net_ops);
+	rhashtable_destroy(&vtaghash);
 }
 
 module_init(nf_conntrack_proto_sctp_init);
-- 
2.5.0

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Pablo Neira Ayuso]

Hi Marcelo,

On Tue, Dec 08, 2015 at 11:11:10AM -0200, Marcelo Ricardo Leitner wrote:
> Commit d7ee35190427 ("netfilter: nf_ct_sctp: minimal multihoming
> support") allowed creating conntrack entries based on the heartbeat
> exchange, so that we can track secondary paths too.
> 
> This patch adds a vtag verification to that. That is, in order to allow
> a HEARTBEAT or a HEARTBEAT_ACK through, the tuple (src port, dst port,
> vtag) must be already known.

This infrastructure that you're adding in this patch looks very
similar to me to conntrack expectations.

Did you evaluate this possibility?

The idea would be to add the vtag to the tuples since it allows us to
uniquely identify the SCTP flow. Then, if you see the hearbeat, you
can register an expectation for the tuple (any-src-ip, any-dst-ip,
sctp, specific-sport, specific-dport, specific-vtag-value).

Then, any secondary STCP flow matching that expectation in the future
will be accepted as RELATED traffic.

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Pablo Neira Ayuso]

Hi Marcelo,

On Tue, Dec 08, 2015 at 11:11:10AM -0200, Marcelo Ricardo Leitner wrote:
> Commit d7ee35190427 ("netfilter: nf_ct_sctp: minimal multihoming
> support") allowed creating conntrack entries based on the heartbeat
> exchange, so that we can track secondary paths too.
> 
> This patch adds a vtag verification to that. That is, in order to allow
> a HEARTBEAT or a HEARTBEAT_ACK through, the tuple (src port, dst port,
> vtag) must be already known.

This infrastructure that you're adding in this patch looks very
similar to me to conntrack expectations.

Did you evaluate this possibility?

The idea would be to add the vtag to the tuples since it allows us to
uniquely identify the SCTP flow. Then, if you see the hearbeat, you
can register an expectation for the tuple (any-src-ip, any-dst-ip,
sctp, specific-sport, specific-dport, specific-vtag-value).

Then, any secondary STCP flow matching that expectation in the future
will be accepted as RELATED traffic.

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Marcelo Ricardo Leitner]

Hello,

Em 10-12-2015 10:02, Pablo Neira Ayuso escreveu:
> Hi Marcelo,
>
> On Tue, Dec 08, 2015 at 11:11:10AM -0200, Marcelo Ricardo Leitner wrote:
>> Commit d7ee35190427 ("netfilter: nf_ct_sctp: minimal multihoming
>> support") allowed creating conntrack entries based on the heartbeat
>> exchange, so that we can track secondary paths too.
>>
>> This patch adds a vtag verification to that. That is, in order to allow
>> a HEARTBEAT or a HEARTBEAT_ACK through, the tuple (src port, dst port,
>> vtag) must be already known.
>
> This infrastructure that you're adding in this patch looks very
> similar to me to conntrack expectations.
>
> Did you evaluate this possibility?

Yes,

> The idea would be to add the vtag to the tuples since it allows us to
> uniquely identify the SCTP flow. Then, if you see the hearbeat, you
> can register an expectation for the tuple (any-src-ip, any-dst-ip,
> sctp, specific-sport, specific-dport, specific-vtag-value).
>
> Then, any secondary STCP flow matching that expectation in the future
> will be accepted as RELATED traffic.

When I first evaluated using expectations, I was going to track all 
addresses that the association was announcing. This would mean we would 
have to add expectations for all address combinations that might have 
been possible. This was the main reason that I didn't use expectations. 
  Yet this req changed when I realized that we can't process ASCONF 
chunks without validating the AUTH chunk first, which we just can't just 
when in the middle of the communication.

After that went down it's just two other:
- by removing the addresses from it, we have the possibility that a host 
may use multiple addresses but not for a single sctp association, but 
like running two distinct assocs, one using each IP address, but same 
ports, and same vtags. It could happen.. it would cause a clash as the 
expectation would be the same but for different masters.

- adding vtag to it increases nf_conntrack_tuple by 4 bytes, so 8 bytes 
per nf_conn, while this feature will be off for the majority of the 
installations.

The possibility of using RELATED is very attractive, though. Would make 
more sense, I think. The extra bytes, we might do it, but for that 
conflict, only if we require the usage of conntrack zones in such cases. 
It would work for me..

   Marcelo

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Marcelo Ricardo Leitner]

Hello,

Em 10-12-2015 10:02, Pablo Neira Ayuso escreveu:
> Hi Marcelo,
>
> On Tue, Dec 08, 2015 at 11:11:10AM -0200, Marcelo Ricardo Leitner wrote:
>> Commit d7ee35190427 ("netfilter: nf_ct_sctp: minimal multihoming
>> support") allowed creating conntrack entries based on the heartbeat
>> exchange, so that we can track secondary paths too.
>>
>> This patch adds a vtag verification to that. That is, in order to allow
>> a HEARTBEAT or a HEARTBEAT_ACK through, the tuple (src port, dst port,
>> vtag) must be already known.
>
> This infrastructure that you're adding in this patch looks very
> similar to me to conntrack expectations.
>
> Did you evaluate this possibility?

Yes,

> The idea would be to add the vtag to the tuples since it allows us to
> uniquely identify the SCTP flow. Then, if you see the hearbeat, you
> can register an expectation for the tuple (any-src-ip, any-dst-ip,
> sctp, specific-sport, specific-dport, specific-vtag-value).
>
> Then, any secondary STCP flow matching that expectation in the future
> will be accepted as RELATED traffic.

When I first evaluated using expectations, I was going to track all 
addresses that the association was announcing. This would mean we would 
have to add expectations for all address combinations that might have 
been possible. This was the main reason that I didn't use expectations. 
  Yet this req changed when I realized that we can't process ASCONF 
chunks without validating the AUTH chunk first, which we just can't just 
when in the middle of the communication.

After that went down it's just two other:
- by removing the addresses from it, we have the possibility that a host 
may use multiple addresses but not for a single sctp association, but 
like running two distinct assocs, one using each IP address, but same 
ports, and same vtags. It could happen.. it would cause a clash as the 
expectation would be the same but for different masters.

- adding vtag to it increases nf_conntrack_tuple by 4 bytes, so 8 bytes 
per nf_conn, while this feature will be off for the majority of the 
installations.

The possibility of using RELATED is very attractive, though. Would make 
more sense, I think. The extra bytes, we might do it, but for that 
conflict, only if we require the usage of conntrack zones in such cases. 
It would work for me..

   Marcelo

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Pablo Neira Ayuso]

On Thu, Dec 10, 2015 at 11:16:04AM -0200, Marcelo Ricardo Leitner wrote:
> Hello,
> 
> Em 10-12-2015 10:02, Pablo Neira Ayuso escreveu:
> >Hi Marcelo,
> >
> >On Tue, Dec 08, 2015 at 11:11:10AM -0200, Marcelo Ricardo Leitner wrote:
> >>Commit d7ee35190427 ("netfilter: nf_ct_sctp: minimal multihoming
> >>support") allowed creating conntrack entries based on the heartbeat
> >>exchange, so that we can track secondary paths too.
> >>
> >>This patch adds a vtag verification to that. That is, in order to allow
> >>a HEARTBEAT or a HEARTBEAT_ACK through, the tuple (src port, dst port,
> >>vtag) must be already known.
> >
> >This infrastructure that you're adding in this patch looks very
> >similar to me to conntrack expectations.
> >
> >Did you evaluate this possibility?
> 
> Yes,
> 
> >The idea would be to add the vtag to the tuples since it allows us to
> >uniquely identify the SCTP flow. Then, if you see the hearbeat, you
> >can register an expectation for the tuple (any-src-ip, any-dst-ip,
> >sctp, specific-sport, specific-dport, specific-vtag-value).
> >
> >Then, any secondary STCP flow matching that expectation in the future
> >will be accepted as RELATED traffic.
> 
> When I first evaluated using expectations, I was going to track all
> addresses that the association was announcing. This would mean we would have
> to add expectations for all address combinations that might have been
> possible.

You can use a mask in expectations for wildcard matching, I think
you're basically doing this in your patch. So it would be just one
single expectation for that combination with the permanent flags set
on. I think we may need another flag to make new conntracks
independent from the master (IIRC currently if the master conntrack is
gone, related ones will be gone too and we don't want this to happen
in this case).

> This was the main reason that I didn't use expectations.  Yet this
> req changed when I realized that we can't process ASCONF chunks without
> validating the AUTH chunk first, which we just can't just when in the middle
> of the communication.

OK, so that's why we cannot create expectations for specific
addresses, right?

> After that went down it's just two other:
> - by removing the addresses from it, we have the possibility that a host may
> use multiple addresses but not for a single sctp association, but like
> running two distinct assocs, one using each IP address, but same ports, and
> same vtags. It could happen.. it would cause a clash as the expectation
> would be the same but for different masters.
> 
> - adding vtag to it increases nf_conntrack_tuple by 4 bytes, so 8 bytes per
> nf_conn, while this feature will be off for the majority of the
> installations.

Yes, there is a bit more extra memory.

I think we can shrink this back by moving the expectation master
pointer to an extension (they are rarely used). Another one to
consider is secmark, I don't know of many using this but I may be wrong.

> The possibility of using RELATED is very attractive, though. Would make more
> sense, I think.

OK.

> The extra bytes, we might do it, but for that conflict, only if we
> require the usage of conntrack zones in such cases. It would work
> for me..

Not sure what you mean.

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Pablo Neira Ayuso]

On Thu, Dec 10, 2015 at 11:16:04AM -0200, Marcelo Ricardo Leitner wrote:
> Hello,
> 
> Em 10-12-2015 10:02, Pablo Neira Ayuso escreveu:
> >Hi Marcelo,
> >
> >On Tue, Dec 08, 2015 at 11:11:10AM -0200, Marcelo Ricardo Leitner wrote:
> >>Commit d7ee35190427 ("netfilter: nf_ct_sctp: minimal multihoming
> >>support") allowed creating conntrack entries based on the heartbeat
> >>exchange, so that we can track secondary paths too.
> >>
> >>This patch adds a vtag verification to that. That is, in order to allow
> >>a HEARTBEAT or a HEARTBEAT_ACK through, the tuple (src port, dst port,
> >>vtag) must be already known.
> >
> >This infrastructure that you're adding in this patch looks very
> >similar to me to conntrack expectations.
> >
> >Did you evaluate this possibility?
> 
> Yes,
> 
> >The idea would be to add the vtag to the tuples since it allows us to
> >uniquely identify the SCTP flow. Then, if you see the hearbeat, you
> >can register an expectation for the tuple (any-src-ip, any-dst-ip,
> >sctp, specific-sport, specific-dport, specific-vtag-value).
> >
> >Then, any secondary STCP flow matching that expectation in the future
> >will be accepted as RELATED traffic.
> 
> When I first evaluated using expectations, I was going to track all
> addresses that the association was announcing. This would mean we would have
> to add expectations for all address combinations that might have been
> possible.

You can use a mask in expectations for wildcard matching, I think
you're basically doing this in your patch. So it would be just one
single expectation for that combination with the permanent flags set
on. I think we may need another flag to make new conntracks
independent from the master (IIRC currently if the master conntrack is
gone, related ones will be gone too and we don't want this to happen
in this case).

> This was the main reason that I didn't use expectations.  Yet this
> req changed when I realized that we can't process ASCONF chunks without
> validating the AUTH chunk first, which we just can't just when in the middle
> of the communication.

OK, so that's why we cannot create expectations for specific
addresses, right?

> After that went down it's just two other:
> - by removing the addresses from it, we have the possibility that a host may
> use multiple addresses but not for a single sctp association, but like
> running two distinct assocs, one using each IP address, but same ports, and
> same vtags. It could happen.. it would cause a clash as the expectation
> would be the same but for different masters.
> 
> - adding vtag to it increases nf_conntrack_tuple by 4 bytes, so 8 bytes per
> nf_conn, while this feature will be off for the majority of the
> installations.

Yes, there is a bit more extra memory.

I think we can shrink this back by moving the expectation master
pointer to an extension (they are rarely used). Another one to
consider is secmark, I don't know of many using this but I may be wrong.

> The possibility of using RELATED is very attractive, though. Would make more
> sense, I think.

OK.

> The extra bytes, we might do it, but for that conflict, only if we
> require the usage of conntrack zones in such cases. It would work
> for me..

Not sure what you mean.

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Marcelo Ricardo Leitner]

On Thu, Dec 10, 2015 at 02:42:47PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Dec 10, 2015 at 11:16:04AM -0200, Marcelo Ricardo Leitner wrote:
> > Hello,
> > 
> > Em 10-12-2015 10:02, Pablo Neira Ayuso escreveu:
> > >Hi Marcelo,
> > >
> > >On Tue, Dec 08, 2015 at 11:11:10AM -0200, Marcelo Ricardo Leitner wrote:
> > >>Commit d7ee35190427 ("netfilter: nf_ct_sctp: minimal multihoming
> > >>support") allowed creating conntrack entries based on the heartbeat
> > >>exchange, so that we can track secondary paths too.
> > >>
> > >>This patch adds a vtag verification to that. That is, in order to allow
> > >>a HEARTBEAT or a HEARTBEAT_ACK through, the tuple (src port, dst port,
> > >>vtag) must be already known.
> > >
> > >This infrastructure that you're adding in this patch looks very
> > >similar to me to conntrack expectations.
> > >
> > >Did you evaluate this possibility?
> > 
> > Yes,
> > 
> > >The idea would be to add the vtag to the tuples since it allows us to
> > >uniquely identify the SCTP flow. Then, if you see the hearbeat, you
> > >can register an expectation for the tuple (any-src-ip, any-dst-ip,
> > >sctp, specific-sport, specific-dport, specific-vtag-value).
> > >
> > >Then, any secondary STCP flow matching that expectation in the future
> > >will be accepted as RELATED traffic.
> > 
> > When I first evaluated using expectations, I was going to track all
> > addresses that the association was announcing. This would mean we would have
> > to add expectations for all address combinations that might have been
> > possible.
> 
> You can use a mask in expectations for wildcard matching, I think
> you're basically doing this in your patch. So it would be just one
> single expectation for that combination with the permanent flags set
> on. I think we may need another flag to make new conntracks

Yes

> independent from the master (IIRC currently if the master conntrack is
> gone, related ones will be gone too and we don't want this to happen
> in this case).

Ah yes, that too.

If such entry times out, we could promote a related entry to master,
maybe.  Because with this link in there we are able to remove all the
entries when we see a shutdown/abort instead of leaving them to timeout.

> > This was the main reason that I didn't use expectations.  Yet this
> > req changed when I realized that we can't process ASCONF chunks without
> > validating the AUTH chunk first, which we just can't just when in the middle
> > of the communication.
> 
> OK, so that's why we cannot create expectations for specific
> addresses, right?

Right. We would be trusting un-trusty data.

> > After that went down it's just two other:
> > - by removing the addresses from it, we have the possibility that a host may
> > use multiple addresses but not for a single sctp association, but like
> > running two distinct assocs, one using each IP address, but same ports, and
> > same vtags. It could happen.. it would cause a clash as the expectation
> > would be the same but for different masters.
> > 
> > - adding vtag to it increases nf_conntrack_tuple by 4 bytes, so 8 bytes per
> > nf_conn, while this feature will be off for the majority of the
> > installations.
> 
> Yes, there is a bit more extra memory.
> 
> I think we can shrink this back by moving the expectation master
> pointer to an extension (they are rarely used). Another one to
> consider is secmark, I don't know of many using this but I may be wrong.

Ok

> > The possibility of using RELATED is very attractive, though. Would make more
> > sense, I think.
> 
> OK.
> 
> > The extra bytes, we might do it, but for that conflict, only if we
> > require the usage of conntrack zones in such cases. It would work
> > for me..
> 
> Not sure what you mean.

That we can go this other way if you think it's best, not a problem. :-)

For not breaking d7ee35190427 ("netfilter: nf_ct_sctp: minimal
multihoming support"), we would still need that sysctl, something like
'expected_heartbeats', still defaulting to false.

  Marcelo

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Marcelo Ricardo Leitner]

On Thu, Dec 10, 2015 at 02:42:47PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Dec 10, 2015 at 11:16:04AM -0200, Marcelo Ricardo Leitner wrote:
> > Hello,
> > 
> > Em 10-12-2015 10:02, Pablo Neira Ayuso escreveu:
> > >Hi Marcelo,
> > >
> > >On Tue, Dec 08, 2015 at 11:11:10AM -0200, Marcelo Ricardo Leitner wrote:
> > >>Commit d7ee35190427 ("netfilter: nf_ct_sctp: minimal multihoming
> > >>support") allowed creating conntrack entries based on the heartbeat
> > >>exchange, so that we can track secondary paths too.
> > >>
> > >>This patch adds a vtag verification to that. That is, in order to allow
> > >>a HEARTBEAT or a HEARTBEAT_ACK through, the tuple (src port, dst port,
> > >>vtag) must be already known.
> > >
> > >This infrastructure that you're adding in this patch looks very
> > >similar to me to conntrack expectations.
> > >
> > >Did you evaluate this possibility?
> > 
> > Yes,
> > 
> > >The idea would be to add the vtag to the tuples since it allows us to
> > >uniquely identify the SCTP flow. Then, if you see the hearbeat, you
> > >can register an expectation for the tuple (any-src-ip, any-dst-ip,
> > >sctp, specific-sport, specific-dport, specific-vtag-value).
> > >
> > >Then, any secondary STCP flow matching that expectation in the future
> > >will be accepted as RELATED traffic.
> > 
> > When I first evaluated using expectations, I was going to track all
> > addresses that the association was announcing. This would mean we would have
> > to add expectations for all address combinations that might have been
> > possible.
> 
> You can use a mask in expectations for wildcard matching, I think
> you're basically doing this in your patch. So it would be just one
> single expectation for that combination with the permanent flags set
> on. I think we may need another flag to make new conntracks

Yes

> independent from the master (IIRC currently if the master conntrack is
> gone, related ones will be gone too and we don't want this to happen
> in this case).

Ah yes, that too.

If such entry times out, we could promote a related entry to master,
maybe.  Because with this link in there we are able to remove all the
entries when we see a shutdown/abort instead of leaving them to timeout.

> > This was the main reason that I didn't use expectations.  Yet this
> > req changed when I realized that we can't process ASCONF chunks without
> > validating the AUTH chunk first, which we just can't just when in the middle
> > of the communication.
> 
> OK, so that's why we cannot create expectations for specific
> addresses, right?

Right. We would be trusting un-trusty data.

> > After that went down it's just two other:
> > - by removing the addresses from it, we have the possibility that a host may
> > use multiple addresses but not for a single sctp association, but like
> > running two distinct assocs, one using each IP address, but same ports, and
> > same vtags. It could happen.. it would cause a clash as the expectation
> > would be the same but for different masters.
> > 
> > - adding vtag to it increases nf_conntrack_tuple by 4 bytes, so 8 bytes per
> > nf_conn, while this feature will be off for the majority of the
> > installations.
> 
> Yes, there is a bit more extra memory.
> 
> I think we can shrink this back by moving the expectation master
> pointer to an extension (they are rarely used). Another one to
> consider is secmark, I don't know of many using this but I may be wrong.

Ok

> > The possibility of using RELATED is very attractive, though. Would make more
> > sense, I think.
> 
> OK.
> 
> > The extra bytes, we might do it, but for that conflict, only if we
> > require the usage of conntrack zones in such cases. It would work
> > for me..
> 
> Not sure what you mean.

That we can go this other way if you think it's best, not a problem. :-)

For not breaking d7ee35190427 ("netfilter: nf_ct_sctp: minimal
multihoming support"), we would still need that sysctl, something like
'expected_heartbeats', still defaulting to false.

  Marcelo

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Pablo Neira Ayuso]

On Thu, Dec 10, 2015 at 12:06:25PM -0200, Marcelo Ricardo Leitner wrote:
> On Thu, Dec 10, 2015 at 02:42:47PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Dec 10, 2015 at 11:16:04AM -0200, Marcelo Ricardo Leitner wrote:
> >
> > You can use a mask in expectations for wildcard matching, I think
> > you're basically doing this in your patch. So it would be just one
> > single expectation for that combination with the permanent flags set
> > on. I think we may need another flag to make new conntracks
> 
> Yes
> 
> > independent from the master (IIRC currently if the master conntrack is
> > gone, related ones will be gone too and we don't want this to happen
> > in this case).
> 
> Ah yes, that too.
> 
> If such entry times out, we could promote a related entry to master,
> maybe.
>
> Because with this link in there we are able to remove all the
> entries when we see a shutdown/abort instead of leaving them to timeout.

I think in this case it doesn't make sense to have a pointer to the
master conntrack for a new sctp flow that got established via
expectation. That new sctp flow would be completely independent. I
would need to audit the core code to evaluate the impact of having a
conntrack with the IPS_EXPECTED_BIT set with no master set. It would
be good if you can investigate this.

> > > This was the main reason that I didn't use expectations.  Yet this
> > > req changed when I realized that we can't process ASCONF chunks without
> > > validating the AUTH chunk first, which we just can't just when in the middle
> > > of the communication.
> > 
> > OK, so that's why we cannot create expectations for specific
> > addresses, right?
> 
> Right. We would be trusting un-trusty data.

I see, but without IP source and destination address in the
expectation, how is this effective from a security point of view? My
concerns go in the direction of possible spoofing that result in holes
in the firewall for sctp traffic.

> > > After that went down it's just two other:
> > > - by removing the addresses from it, we have the possibility that a host may
> > > use multiple addresses but not for a single sctp association, but like
> > > running two distinct assocs, one using each IP address, but same ports, and
> > > same vtags. It could happen.. it would cause a clash as the expectation
> > > would be the same but for different masters.
> > > 
> > > - adding vtag to it increases nf_conntrack_tuple by 4 bytes, so 8 bytes per
> > > nf_conn, while this feature will be off for the majority of the
> > > installations.> > 
> > 
> > Yes, there is a bit more extra memory.
> > 
> > I think we can shrink this back by moving the expectation master
> > pointer to an extension (they are rarely used). Another one to
> > consider is secmark, I don't know of many using this but I may be wrong.
> 
> Ok

Anyway, it would be good to evaluate the performance impact on
enlarging the tuple. Not only extra memory, the hashing for lookups
will take 4 extra bytes per packet after such change.

I would need to have a look, but there must be a nice way to
accomodate this into the expectation infrastructure without having any
impact on that front. Probably we can have a per-protocol family
expectation table (instead of a global table) where the hashing to
look up for expectations depends on the protocol (so in the sctp case,
we can include the vtag from the ct->state in the hash lookup).

Just an idea, we should avoid any significant impact on performance
for everyone else just to handle this sctp multihoming case.

> > > The possibility of using RELATED is very attractive, though. Would make more
> > > sense, I think.
> > 
> > OK.
> > 
> > > The extra bytes, we might do it, but for that conflict, only if we
> > > require the usage of conntrack zones in such cases. It would work
> > > for me..
> > 
> > Not sure what you mean.
> 
> That we can go this other way if you think it's best, not a problem. :-)
> 
> For not breaking d7ee35190427 ("netfilter: nf_ct_sctp: minimal
> multihoming support"), we would still need that sysctl, something like
> 'expected_heartbeats', still defaulting to false.

You mean we need that new sysctl to explicitly enable multihoming
tracking in any case, right?

Thanks.

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Pablo Neira Ayuso]

On Thu, Dec 10, 2015 at 12:06:25PM -0200, Marcelo Ricardo Leitner wrote:
> On Thu, Dec 10, 2015 at 02:42:47PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Dec 10, 2015 at 11:16:04AM -0200, Marcelo Ricardo Leitner wrote:
> >
> > You can use a mask in expectations for wildcard matching, I think
> > you're basically doing this in your patch. So it would be just one
> > single expectation for that combination with the permanent flags set
> > on. I think we may need another flag to make new conntracks
> 
> Yes
> 
> > independent from the master (IIRC currently if the master conntrack is
> > gone, related ones will be gone too and we don't want this to happen
> > in this case).
> 
> Ah yes, that too.
> 
> If such entry times out, we could promote a related entry to master,
> maybe.
>
> Because with this link in there we are able to remove all the
> entries when we see a shutdown/abort instead of leaving them to timeout.

I think in this case it doesn't make sense to have a pointer to the
master conntrack for a new sctp flow that got established via
expectation. That new sctp flow would be completely independent. I
would need to audit the core code to evaluate the impact of having a
conntrack with the IPS_EXPECTED_BIT set with no master set. It would
be good if you can investigate this.

> > > This was the main reason that I didn't use expectations.  Yet this
> > > req changed when I realized that we can't process ASCONF chunks without
> > > validating the AUTH chunk first, which we just can't just when in the middle
> > > of the communication.
> > 
> > OK, so that's why we cannot create expectations for specific
> > addresses, right?
> 
> Right. We would be trusting un-trusty data.

I see, but without IP source and destination address in the
expectation, how is this effective from a security point of view? My
concerns go in the direction of possible spoofing that result in holes
in the firewall for sctp traffic.

> > > After that went down it's just two other:
> > > - by removing the addresses from it, we have the possibility that a host may
> > > use multiple addresses but not for a single sctp association, but like
> > > running two distinct assocs, one using each IP address, but same ports, and
> > > same vtags. It could happen.. it would cause a clash as the expectation
> > > would be the same but for different masters.
> > > 
> > > - adding vtag to it increases nf_conntrack_tuple by 4 bytes, so 8 bytes per
> > > nf_conn, while this feature will be off for the majority of the
> > > installations.> > 
> > 
> > Yes, there is a bit more extra memory.
> > 
> > I think we can shrink this back by moving the expectation master
> > pointer to an extension (they are rarely used). Another one to
> > consider is secmark, I don't know of many using this but I may be wrong.
> 
> Ok

Anyway, it would be good to evaluate the performance impact on
enlarging the tuple. Not only extra memory, the hashing for lookups
will take 4 extra bytes per packet after such change.

I would need to have a look, but there must be a nice way to
accomodate this into the expectation infrastructure without having any
impact on that front. Probably we can have a per-protocol family
expectation table (instead of a global table) where the hashing to
look up for expectations depends on the protocol (so in the sctp case,
we can include the vtag from the ct->state in the hash lookup).

Just an idea, we should avoid any significant impact on performance
for everyone else just to handle this sctp multihoming case.

> > > The possibility of using RELATED is very attractive, though. Would make more
> > > sense, I think.
> > 
> > OK.
> > 
> > > The extra bytes, we might do it, but for that conflict, only if we
> > > require the usage of conntrack zones in such cases. It would work
> > > for me..
> > 
> > Not sure what you mean.
> 
> That we can go this other way if you think it's best, not a problem. :-)
> 
> For not breaking d7ee35190427 ("netfilter: nf_ct_sctp: minimal
> multihoming support"), we would still need that sysctl, something like
> 'expected_heartbeats', still defaulting to false.

You mean we need that new sysctl to explicitly enable multihoming
tracking in any case, right?

Thanks.

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Marcelo Ricardo Leitner]

On Thu, Dec 10, 2015 at 06:06:57PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Dec 10, 2015 at 12:06:25PM -0200, Marcelo Ricardo Leitner wrote:
> > On Thu, Dec 10, 2015 at 02:42:47PM +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Dec 10, 2015 at 11:16:04AM -0200, Marcelo Ricardo Leitner wrote:
> > >
> > > You can use a mask in expectations for wildcard matching, I think
> > > you're basically doing this in your patch. So it would be just one
> > > single expectation for that combination with the permanent flags set
> > > on. I think we may need another flag to make new conntracks
> > 
> > Yes
> > 
> > > independent from the master (IIRC currently if the master conntrack is
> > > gone, related ones will be gone too and we don't want this to happen
> > > in this case).
> > 
> > Ah yes, that too.
> > 
> > If such entry times out, we could promote a related entry to master,
> > maybe.
> >
> > Because with this link in there we are able to remove all the
> > entries when we see a shutdown/abort instead of leaving them to timeout.
> 
> I think in this case it doesn't make sense to have a pointer to the
> master conntrack for a new sctp flow that got established via
> expectation. That new sctp flow would be completely independent. I

Not really. It's just another path/transport for the same sctp
association. From sctp point of view, these two conntrack entries share
their end points, they refer to the same sockets.

> would need to audit the core code to evaluate the impact of having a
> conntrack with the IPS_EXPECTED_BIT set with no master set. It would
> be good if you can investigate this.

Sure. I'll get to that after considering the tuple enlargement.

> > > > This was the main reason that I didn't use expectations.  Yet this
> > > > req changed when I realized that we can't process ASCONF chunks without
> > > > validating the AUTH chunk first, which we just can't just when in the middle
> > > > of the communication.
> > > 
> > > OK, so that's why we cannot create expectations for specific
> > > addresses, right?
> > 
> > Right. We would be trusting un-trusty data.
> 
> I see, but without IP source and destination address in the
> expectation, how is this effective from a security point of view? My
> concerns go in the direction of possible spoofing that result in holes
> in the firewall for sctp traffic.

It may not be 100% secure without the IP addresses in it but it adds
another 32bit that the attacker must be able to match in order to get
through that door.

Currently, if the attacker knows the ports used, it's enough to get
at least a heartbeat through.

> > > > After that went down it's just two other:
> > > > - by removing the addresses from it, we have the possibility that a host may
> > > > use multiple addresses but not for a single sctp association, but like
> > > > running two distinct assocs, one using each IP address, but same ports, and
> > > > same vtags. It could happen.. it would cause a clash as the expectation
> > > > would be the same but for different masters.
> > > > 
> > > > - adding vtag to it increases nf_conntrack_tuple by 4 bytes, so 8 bytes per
> > > > nf_conn, while this feature will be off for the majority of the
> > > > installations.> > 
> > > 
> > > Yes, there is a bit more extra memory.
> > > 
> > > I think we can shrink this back by moving the expectation master
> > > pointer to an extension (they are rarely used). Another one to
> > > consider is secmark, I don't know of many using this but I may be wrong.
> > 
> > Ok
> 
> Anyway, it would be good to evaluate the performance impact on
> enlarging the tuple. Not only extra memory, the hashing for lookups
> will take 4 extra bytes per packet after such change.
> 
> I would need to have a look, but there must be a nice way to
> accomodate this into the expectation infrastructure without having any
> impact on that front. Probably we can have a per-protocol family
> expectation table (instead of a global table) where the hashing to
> look up for expectations depends on the protocol (so in the sctp case,
> we can include the vtag from the ct->state in the hash lookup).
> 
> Just an idea, we should avoid any significant impact on performance
> for everyone else just to handle this sctp multihoming case.

This is very interesting, specially considering the below ---v

> > > > The possibility of using RELATED is very attractive, though. Would make more
> > > > sense, I think.
> > > 
> > > OK.
> > > 
> > > > The extra bytes, we might do it, but for that conflict, only if we
> > > > require the usage of conntrack zones in such cases. It would work
> > > > for me..
> > > 
> > > Not sure what you mean.
> > 
> > That we can go this other way if you think it's best, not a problem. :-)
> > 
> > For not breaking d7ee35190427 ("netfilter: nf_ct_sctp: minimal
> > multihoming support"), we would still need that sysctl, something like
> > 'expected_heartbeats', still defaulting to false.
> 
> You mean we need that new sysctl to explicitly enable multihoming
> tracking in any case, right?

I think you got it but I have a feeling that there is a small
misunderstanding in here. Let me try to rephrase it. We need that new
sysctl in order to know if we should allow conntrack entries to be
created from heartbeats alone (as it is today) or if even these
heartbeats should be matched against a known sport/dport/vtag tuple (as
I'm proposing with this patch).


About the idea to put the vtag into the tuple itself, this is more like a heads
up, sharing it early. I'm afraid that's going to be very complicated because we
store all infos in u union in big endian.

                union {
                        /* Add other protocols here. */
                        __be64 all;
			    ^^-- to fit the new be32 (was be16)

                        struct {
                                __be16 port;
                        } tcp;
                        struct {
                                __be16 port;
                        } udp;
                        struct {
                                u_int8_t type, code;
                        } icmp;
                        struct {
                                __be16 port;
                        } dccp;
                        struct {
                                __be16 port;
                                __be32 vtag;
                        } sctp;  <-- expanded

but this change will cause the structs with __be16 to read the higher
portion of that be64 instead if on a little-endian system.

$ pahole test
struct pack64 {
	union {
		__be64             all;                  /*           8 */
		struct {
			__be16     port;                 /*     0     2 */
		} tcp;                                   /*           2 */
	} u;                                             /*     0     8 */

	/* size: 8, cachelines: 1, members: 1 */
	/* last cacheline: 8 bytes */
};
struct pack16 {
	union {
		__be16             all;                  /*           2 */
		struct {
			__be16     port;                 /*     0     2 */
		} tcp;                                   /*           2 */
	} u;                                             /*     0     2 */

	/* size: 2, cachelines: 1, members: 1 */
	/* last cacheline: 2 bytes */
};

and there is code that relies on this direct mapping, like in:

nf_nat_l4proto_in_range():
        __be16 port;

        if (maniptype = NF_NAT_MANIP_SRC)
                port = tuple->src.u.all;
        else
                port = tuple->dst.u.all;

nf_nat_ipv6_decode_session():
        if (ct->status & statusbit) {
                fl6->daddr = t->dst.u3.in6;
                if (t->dst.protonum = IPPROTO_TCP ||
                    t->dst.protonum = IPPROTO_UDP ||
                    t->dst.protonum = IPPROTO_UDPLITE ||
                    t->dst.protonum = IPPROTO_DCCP ||
                    t->dst.protonum = IPPROTO_SCTP)
                        fl6->fl6_dport = t->dst.u.all;
        }

ip_vs_conn_drop_conntrack():
        tuple.src.u3 = cp->caddr;                                         
        tuple.src.u.all = cp->cport;                                      
        tuple.src.l3num = cp->af;                                         
        tuple.dst.u3 = cp->vaddr;                                         
        tuple.dst.u.all = cp->vport;                                      

amongst others, including the the masks, like in:

__nf_ct_helper_find():
        struct nf_conntrack_tuple_mask mask = { .src.u.all = htons(0xFFFF) };
...
        hlist_for_each_entry_rcu(helper, &nf_ct_helper_hash[h], hnode) {
                if (nf_ct_tuple_src_mask_cmp(tuple, &helper->tuple, &mask))
                        return helper;
        }

and:
ct_proto_port_check():
        /* Shortcut to match all recognized protocols by using ->src.all. */
        if ((info->match_flags & XT_CONNTRACK_ORIGSRC_PORT) &&
            (tuple->src.u.all = info->origsrc_port) ^
            !(info->invert_flags & XT_CONNTRACK_ORIGSRC_PORT))
                return false;

Seems some bit-dancing will be necessary.

Thanks,
Marcelo

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Marcelo Ricardo Leitner]

On Thu, Dec 10, 2015 at 06:06:57PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Dec 10, 2015 at 12:06:25PM -0200, Marcelo Ricardo Leitner wrote:
> > On Thu, Dec 10, 2015 at 02:42:47PM +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Dec 10, 2015 at 11:16:04AM -0200, Marcelo Ricardo Leitner wrote:
> > >
> > > You can use a mask in expectations for wildcard matching, I think
> > > you're basically doing this in your patch. So it would be just one
> > > single expectation for that combination with the permanent flags set
> > > on. I think we may need another flag to make new conntracks
> > 
> > Yes
> > 
> > > independent from the master (IIRC currently if the master conntrack is
> > > gone, related ones will be gone too and we don't want this to happen
> > > in this case).
> > 
> > Ah yes, that too.
> > 
> > If such entry times out, we could promote a related entry to master,
> > maybe.
> >
> > Because with this link in there we are able to remove all the
> > entries when we see a shutdown/abort instead of leaving them to timeout.
> 
> I think in this case it doesn't make sense to have a pointer to the
> master conntrack for a new sctp flow that got established via
> expectation. That new sctp flow would be completely independent. I

Not really. It's just another path/transport for the same sctp
association. From sctp point of view, these two conntrack entries share
their end points, they refer to the same sockets.

> would need to audit the core code to evaluate the impact of having a
> conntrack with the IPS_EXPECTED_BIT set with no master set. It would
> be good if you can investigate this.

Sure. I'll get to that after considering the tuple enlargement.

> > > > This was the main reason that I didn't use expectations.  Yet this
> > > > req changed when I realized that we can't process ASCONF chunks without
> > > > validating the AUTH chunk first, which we just can't just when in the middle
> > > > of the communication.
> > > 
> > > OK, so that's why we cannot create expectations for specific
> > > addresses, right?
> > 
> > Right. We would be trusting un-trusty data.
> 
> I see, but without IP source and destination address in the
> expectation, how is this effective from a security point of view? My
> concerns go in the direction of possible spoofing that result in holes
> in the firewall for sctp traffic.

It may not be 100% secure without the IP addresses in it but it adds
another 32bit that the attacker must be able to match in order to get
through that door.

Currently, if the attacker knows the ports used, it's enough to get
at least a heartbeat through.

> > > > After that went down it's just two other:
> > > > - by removing the addresses from it, we have the possibility that a host may
> > > > use multiple addresses but not for a single sctp association, but like
> > > > running two distinct assocs, one using each IP address, but same ports, and
> > > > same vtags. It could happen.. it would cause a clash as the expectation
> > > > would be the same but for different masters.
> > > > 
> > > > - adding vtag to it increases nf_conntrack_tuple by 4 bytes, so 8 bytes per
> > > > nf_conn, while this feature will be off for the majority of the
> > > > installations.> > 
> > > 
> > > Yes, there is a bit more extra memory.
> > > 
> > > I think we can shrink this back by moving the expectation master
> > > pointer to an extension (they are rarely used). Another one to
> > > consider is secmark, I don't know of many using this but I may be wrong.
> > 
> > Ok
> 
> Anyway, it would be good to evaluate the performance impact on
> enlarging the tuple. Not only extra memory, the hashing for lookups
> will take 4 extra bytes per packet after such change.
> 
> I would need to have a look, but there must be a nice way to
> accomodate this into the expectation infrastructure without having any
> impact on that front. Probably we can have a per-protocol family
> expectation table (instead of a global table) where the hashing to
> look up for expectations depends on the protocol (so in the sctp case,
> we can include the vtag from the ct->state in the hash lookup).
> 
> Just an idea, we should avoid any significant impact on performance
> for everyone else just to handle this sctp multihoming case.

This is very interesting, specially considering the below ---v

> > > > The possibility of using RELATED is very attractive, though. Would make more
> > > > sense, I think.
> > > 
> > > OK.
> > > 
> > > > The extra bytes, we might do it, but for that conflict, only if we
> > > > require the usage of conntrack zones in such cases. It would work
> > > > for me..
> > > 
> > > Not sure what you mean.
> > 
> > That we can go this other way if you think it's best, not a problem. :-)
> > 
> > For not breaking d7ee35190427 ("netfilter: nf_ct_sctp: minimal
> > multihoming support"), we would still need that sysctl, something like
> > 'expected_heartbeats', still defaulting to false.
> 
> You mean we need that new sysctl to explicitly enable multihoming
> tracking in any case, right?

I think you got it but I have a feeling that there is a small
misunderstanding in here. Let me try to rephrase it. We need that new
sysctl in order to know if we should allow conntrack entries to be
created from heartbeats alone (as it is today) or if even these
heartbeats should be matched against a known sport/dport/vtag tuple (as
I'm proposing with this patch).


About the idea to put the vtag into the tuple itself, this is more like a heads
up, sharing it early. I'm afraid that's going to be very complicated because we
store all infos in u union in big endian.

                union {
                        /* Add other protocols here. */
                        __be64 all;
			    ^^-- to fit the new be32 (was be16)

                        struct {
                                __be16 port;
                        } tcp;
                        struct {
                                __be16 port;
                        } udp;
                        struct {
                                u_int8_t type, code;
                        } icmp;
                        struct {
                                __be16 port;
                        } dccp;
                        struct {
                                __be16 port;
                                __be32 vtag;
                        } sctp;  <-- expanded

but this change will cause the structs with __be16 to read the higher
portion of that be64 instead if on a little-endian system.

$ pahole test
struct pack64 {
	union {
		__be64             all;                  /*           8 */
		struct {
			__be16     port;                 /*     0     2 */
		} tcp;                                   /*           2 */
	} u;                                             /*     0     8 */

	/* size: 8, cachelines: 1, members: 1 */
	/* last cacheline: 8 bytes */
};
struct pack16 {
	union {
		__be16             all;                  /*           2 */
		struct {
			__be16     port;                 /*     0     2 */
		} tcp;                                   /*           2 */
	} u;                                             /*     0     2 */

	/* size: 2, cachelines: 1, members: 1 */
	/* last cacheline: 2 bytes */
};

and there is code that relies on this direct mapping, like in:

nf_nat_l4proto_in_range():
        __be16 port;

        if (maniptype == NF_NAT_MANIP_SRC)
                port = tuple->src.u.all;
        else
                port = tuple->dst.u.all;

nf_nat_ipv6_decode_session():
        if (ct->status & statusbit) {
                fl6->daddr = t->dst.u3.in6;
                if (t->dst.protonum == IPPROTO_TCP ||
                    t->dst.protonum == IPPROTO_UDP ||
                    t->dst.protonum == IPPROTO_UDPLITE ||
                    t->dst.protonum == IPPROTO_DCCP ||
                    t->dst.protonum == IPPROTO_SCTP)
                        fl6->fl6_dport = t->dst.u.all;
        }

ip_vs_conn_drop_conntrack():
        tuple.src.u3 = cp->caddr;                                         
        tuple.src.u.all = cp->cport;                                      
        tuple.src.l3num = cp->af;                                         
        tuple.dst.u3 = cp->vaddr;                                         
        tuple.dst.u.all = cp->vport;                                      

amongst others, including the the masks, like in:

__nf_ct_helper_find():
        struct nf_conntrack_tuple_mask mask = { .src.u.all = htons(0xFFFF) };
...
        hlist_for_each_entry_rcu(helper, &nf_ct_helper_hash[h], hnode) {
                if (nf_ct_tuple_src_mask_cmp(tuple, &helper->tuple, &mask))
                        return helper;
        }

and:
ct_proto_port_check():
        /* Shortcut to match all recognized protocols by using ->src.all. */
        if ((info->match_flags & XT_CONNTRACK_ORIGSRC_PORT) &&
            (tuple->src.u.all == info->origsrc_port) ^
            !(info->invert_flags & XT_CONNTRACK_ORIGSRC_PORT))
                return false;

Seems some bit-dancing will be necessary.

Thanks,
Marcelo

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Pablo Neira Ayuso]

On Tue, Dec 15, 2015 at 05:03:04PM -0200, Marcelo Ricardo Leitner wrote:
> On Thu, Dec 10, 2015 at 06:06:57PM +0100, Pablo Neira Ayuso wrote:
> > I think in this case it doesn't make sense to have a pointer to the
> > master conntrack for a new sctp flow that got established via
> > expectation. That new sctp flow would be completely independent. I
> 
> Not really. It's just another path/transport for the same sctp
> association. From sctp point of view, these two conntrack entries share
> their end points, they refer to the same sockets.

Then, you will need add support for expected conntrack with no master.

> > > > > This was the main reason that I didn't use expectations.  Yet this
> > > > > req changed when I realized that we can't process ASCONF chunks without
> > > > > validating the AUTH chunk first, which we just can't just when in the middle
> > > > > of the communication.
> > > > 
> > > > OK, so that's why we cannot create expectations for specific
> > > > addresses, right?
> > > 
> > > Right. We would be trusting un-trusty data.
> > 
> > I see, but without IP source and destination address in the
> > expectation, how is this effective from a security point of view? My
> > concerns go in the direction of possible spoofing that result in holes
> > in the firewall for sctp traffic.
> 
> It may not be 100% secure without the IP addresses in it but it adds
> another 32bit that the attacker must be able to match in order to get
> through that door.
> 
> Currently, if the attacker knows the ports used, it's enough to get
> at least a heartbeat through.

You have to come back to us to evaluate how feasible is to push holes
in the firewall through this helper and what the attacker would get
from this attack.

Anyway, what I'm going to propose you below will allow the user to
narrow down what peers are allowed to create sctp multihoming
expectations.

> About the idea to put the vtag into the tuple itself, this is more like a heads
> up, sharing it early. I'm afraid that's going to be very complicated because we
> store all infos in u union in big endian.
> 
>                 union {
>                         /* Add other protocols here. */
>                         __be64 all;
> 			    ^^-- to fit the new be32 (was be16)
> 
>                         struct {
>                                 __be16 port;
>                         } tcp;
>                         struct {
>                                 __be16 port;
>                         } udp;
>                         struct {
>                                 u_int8_t type, code;
>                         } icmp;
>                         struct {
>                                 __be16 port;
>                         } dccp;
>                         struct {
>                                 __be16 port;
>                                 __be32 vtag;
>                         } sctp;  <-- expanded
> 
> but this change will cause the structs with __be16 to read the higher
> portion of that be64 instead if on a little-endian system.

We should avoid this change in the structure. I'd suggest you better
extended nf_ct_find_expectation so it handles the special case to
include the vtag in the search when this is SCTP traffic. There must
be a generic way to accomodate this. You may need a specific
expectation for SCTP (so the hashing includes the vtag from the
ct->state). So the impact to add SCTP multihoming support is minimal
given that we'll have very little clients of this code.

To be more concrete on the path I would follow, I think you have to a
sctp-multihoming helper just like we do for FTP, IRC, GRE, Netbios and
friends. The idea is that you register a new struct
nf_conntrack_helper into the infrastructure. The help function will
inspect for HEARTBEAT chunks, if found, it will create the
expectation.

Given that we'll be disabling automagic helper assignment soon (the
existing behaviour is problematic from the security point of view, see
Eric Leblond's documentation on this), the user can probably skip the
possible security problems that we're discussing above by narrowing
down the possible SCTP peers that are allowed to create multihoming
expectations when explicitly configuring the helper through iptables.
This should also allow you to skip the sysctl that you need.

Have a look at: https://home.regit.org/netfilter-en/secure-use-of-helpers/

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Pablo Neira Ayuso]

On Tue, Dec 15, 2015 at 05:03:04PM -0200, Marcelo Ricardo Leitner wrote:
> On Thu, Dec 10, 2015 at 06:06:57PM +0100, Pablo Neira Ayuso wrote:
> > I think in this case it doesn't make sense to have a pointer to the
> > master conntrack for a new sctp flow that got established via
> > expectation. That new sctp flow would be completely independent. I
> 
> Not really. It's just another path/transport for the same sctp
> association. From sctp point of view, these two conntrack entries share
> their end points, they refer to the same sockets.

Then, you will need add support for expected conntrack with no master.

> > > > > This was the main reason that I didn't use expectations.  Yet this
> > > > > req changed when I realized that we can't process ASCONF chunks without
> > > > > validating the AUTH chunk first, which we just can't just when in the middle
> > > > > of the communication.
> > > > 
> > > > OK, so that's why we cannot create expectations for specific
> > > > addresses, right?
> > > 
> > > Right. We would be trusting un-trusty data.
> > 
> > I see, but without IP source and destination address in the
> > expectation, how is this effective from a security point of view? My
> > concerns go in the direction of possible spoofing that result in holes
> > in the firewall for sctp traffic.
> 
> It may not be 100% secure without the IP addresses in it but it adds
> another 32bit that the attacker must be able to match in order to get
> through that door.
> 
> Currently, if the attacker knows the ports used, it's enough to get
> at least a heartbeat through.

You have to come back to us to evaluate how feasible is to push holes
in the firewall through this helper and what the attacker would get
from this attack.

Anyway, what I'm going to propose you below will allow the user to
narrow down what peers are allowed to create sctp multihoming
expectations.

> About the idea to put the vtag into the tuple itself, this is more like a heads
> up, sharing it early. I'm afraid that's going to be very complicated because we
> store all infos in u union in big endian.
> 
>                 union {
>                         /* Add other protocols here. */
>                         __be64 all;
> 			    ^^-- to fit the new be32 (was be16)
> 
>                         struct {
>                                 __be16 port;
>                         } tcp;
>                         struct {
>                                 __be16 port;
>                         } udp;
>                         struct {
>                                 u_int8_t type, code;
>                         } icmp;
>                         struct {
>                                 __be16 port;
>                         } dccp;
>                         struct {
>                                 __be16 port;
>                                 __be32 vtag;
>                         } sctp;  <-- expanded
> 
> but this change will cause the structs with __be16 to read the higher
> portion of that be64 instead if on a little-endian system.

We should avoid this change in the structure. I'd suggest you better
extended nf_ct_find_expectation so it handles the special case to
include the vtag in the search when this is SCTP traffic. There must
be a generic way to accomodate this. You may need a specific
expectation for SCTP (so the hashing includes the vtag from the
ct->state). So the impact to add SCTP multihoming support is minimal
given that we'll have very little clients of this code.

To be more concrete on the path I would follow, I think you have to a
sctp-multihoming helper just like we do for FTP, IRC, GRE, Netbios and
friends. The idea is that you register a new struct
nf_conntrack_helper into the infrastructure. The help function will
inspect for HEARTBEAT chunks, if found, it will create the
expectation.

Given that we'll be disabling automagic helper assignment soon (the
existing behaviour is problematic from the security point of view, see
Eric Leblond's documentation on this), the user can probably skip the
possible security problems that we're discussing above by narrowing
down the possible SCTP peers that are allowed to create multihoming
expectations when explicitly configuring the helper through iptables.
This should also allow you to skip the sysctl that you need.

Have a look at: https://home.regit.org/netfilter-en/secure-use-of-helpers/

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Marcelo Ricardo Leitner]

Em 17-12-2015 09:05, Pablo Neira Ayuso escreveu:
> On Tue, Dec 15, 2015 at 05:03:04PM -0200, Marcelo Ricardo Leitner wrote:
>> On Thu, Dec 10, 2015 at 06:06:57PM +0100, Pablo Neira Ayuso wrote:
>>>>>> This was the main reason that I didn't use expectations.  Yet this
>>>>>> req changed when I realized that we can't process ASCONF chunks without
>>>>>> validating the AUTH chunk first, which we just can't just when in the middle
>>>>>> of the communication.
>>>>>
>>>>> OK, so that's why we cannot create expectations for specific
>>>>> addresses, right?
>>>>
>>>> Right. We would be trusting un-trusty data.
>>>
>>> I see, but without IP source and destination address in the
>>> expectation, how is this effective from a security point of view? My
>>> concerns go in the direction of possible spoofing that result in holes
>>> in the firewall for sctp traffic.
>>
>> It may not be 100% secure without the IP addresses in it but it adds
>> another 32bit that the attacker must be able to match in order to get
>> through that door.
>>
>> Currently, if the attacker knows the ports used, it's enough to get
>> at least a heartbeat through.
>
> You have to come back to us to evaluate how feasible is to push holes
> in the firewall through this helper and what the attacker would get
> from this attack.

It's not pushing any more than what it already is. It's actually 
restricting the hole we have (by accepting heartbeats with no validation 
whatsoever, which is not big), but ok, let's put this topic aside for 
now, no problem.

> Anyway, what I'm going to propose you below will allow the user to
> narrow down what peers are allowed to create sctp multihoming
> expectations.
>
>> About the idea to put the vtag into the tuple itself, this is more like a heads
>> up, sharing it early. I'm afraid that's going to be very complicated because we
>> store all infos in u union in big endian.
>>

<snip>

>> but this change will cause the structs with __be16 to read the higher
>> portion of that be64 instead if on a little-endian system.
>
> We should avoid this change in the structure. I'd suggest you better
> extended nf_ct_find_expectation so it handles the special case to
> include the vtag in the search when this is SCTP traffic. There must
> be a generic way to accomodate this. You may need a specific
> expectation for SCTP (so the hashing includes the vtag from the
> ct->state). So the impact to add SCTP multihoming support is minimal
> given that we'll have very little clients of this code.
>
> To be more concrete on the path I would follow, I think you have to a
> sctp-multihoming helper just like we do for FTP, IRC, GRE, Netbios and
> friends. The idea is that you register a new struct
> nf_conntrack_helper into the infrastructure. The help function will
> inspect for HEARTBEAT chunks, if found, it will create the
> expectation.
>
> Given that we'll be disabling automagic helper assignment soon (the
> existing behaviour is problematic from the security point of view, see
> Eric Leblond's documentation on this), the user can probably skip the
> possible security problems that we're discussing above by narrowing
> down the possible SCTP peers that are allowed to create multihoming
> expectations when explicitly configuring the helper through iptables.
> This should also allow you to skip the sysctl that you need.
>
> Have a look at: https://home.regit.org/netfilter-en/secure-use-of-helpers/

I'm still forming this idea in my head, but it's starting to make sense 
to me now. Thanks for the idea, Pablo.

I can see how it would work for the vtag-tracking scenario, I think. I'm 
just not seeing the non-tracking one (routers in the middle) yet. It 
doesn't seem it can fit the helper way because it can't prepare 
expectations as it doesn't see the packets in the initial path, meaning 
that we would have to have 2 ways of handling such new entries.

It would work like: if the helper is not present, allow heartbeats with 
any vtag like it currently is. Otherwise, for validating vtag too, only 
allow them via expectations.

   Marcelo

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Marcelo Ricardo Leitner]

Em 17-12-2015 09:05, Pablo Neira Ayuso escreveu:
> On Tue, Dec 15, 2015 at 05:03:04PM -0200, Marcelo Ricardo Leitner wrote:
>> On Thu, Dec 10, 2015 at 06:06:57PM +0100, Pablo Neira Ayuso wrote:
>>>>>> This was the main reason that I didn't use expectations.  Yet this
>>>>>> req changed when I realized that we can't process ASCONF chunks without
>>>>>> validating the AUTH chunk first, which we just can't just when in the middle
>>>>>> of the communication.
>>>>>
>>>>> OK, so that's why we cannot create expectations for specific
>>>>> addresses, right?
>>>>
>>>> Right. We would be trusting un-trusty data.
>>>
>>> I see, but without IP source and destination address in the
>>> expectation, how is this effective from a security point of view? My
>>> concerns go in the direction of possible spoofing that result in holes
>>> in the firewall for sctp traffic.
>>
>> It may not be 100% secure without the IP addresses in it but it adds
>> another 32bit that the attacker must be able to match in order to get
>> through that door.
>>
>> Currently, if the attacker knows the ports used, it's enough to get
>> at least a heartbeat through.
>
> You have to come back to us to evaluate how feasible is to push holes
> in the firewall through this helper and what the attacker would get
> from this attack.

It's not pushing any more than what it already is. It's actually 
restricting the hole we have (by accepting heartbeats with no validation 
whatsoever, which is not big), but ok, let's put this topic aside for 
now, no problem.

> Anyway, what I'm going to propose you below will allow the user to
> narrow down what peers are allowed to create sctp multihoming
> expectations.
>
>> About the idea to put the vtag into the tuple itself, this is more like a heads
>> up, sharing it early. I'm afraid that's going to be very complicated because we
>> store all infos in u union in big endian.
>>

<snip>

>> but this change will cause the structs with __be16 to read the higher
>> portion of that be64 instead if on a little-endian system.
>
> We should avoid this change in the structure. I'd suggest you better
> extended nf_ct_find_expectation so it handles the special case to
> include the vtag in the search when this is SCTP traffic. There must
> be a generic way to accomodate this. You may need a specific
> expectation for SCTP (so the hashing includes the vtag from the
> ct->state). So the impact to add SCTP multihoming support is minimal
> given that we'll have very little clients of this code.
>
> To be more concrete on the path I would follow, I think you have to a
> sctp-multihoming helper just like we do for FTP, IRC, GRE, Netbios and
> friends. The idea is that you register a new struct
> nf_conntrack_helper into the infrastructure. The help function will
> inspect for HEARTBEAT chunks, if found, it will create the
> expectation.
>
> Given that we'll be disabling automagic helper assignment soon (the
> existing behaviour is problematic from the security point of view, see
> Eric Leblond's documentation on this), the user can probably skip the
> possible security problems that we're discussing above by narrowing
> down the possible SCTP peers that are allowed to create multihoming
> expectations when explicitly configuring the helper through iptables.
> This should also allow you to skip the sysctl that you need.
>
> Have a look at: https://home.regit.org/netfilter-en/secure-use-of-helpers/

I'm still forming this idea in my head, but it's starting to make sense 
to me now. Thanks for the idea, Pablo.

I can see how it would work for the vtag-tracking scenario, I think. I'm 
just not seeing the non-tracking one (routers in the middle) yet. It 
doesn't seem it can fit the helper way because it can't prepare 
expectations as it doesn't see the packets in the initial path, meaning 
that we would have to have 2 ways of handling such new entries.

It would work like: if the helper is not present, allow heartbeats with 
any vtag like it currently is. Otherwise, for validating vtag too, only 
allow them via expectations.

   Marcelo

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Pablo Neira Ayuso]

Hi Marcelo,

On Thu, Dec 24, 2015 at 10:50:28AM -0200, Marcelo Ricardo Leitner wrote:
> I can see how it would work for the vtag-tracking scenario, I think. I'm
> just not seeing the non-tracking one (routers in the middle) yet.
> It doesn't seem it can fit the helper way because it can't prepare
> expectations as it doesn't see the packets in the initial path,
> meaning that we would have to have 2 ways of handling such new
> entries.

We assume that conntrack sees all traffic in our existing helpers,
think of tcp. Otherwise, we may classify packets as invalid if we
don't all packets that belong to the flow.

> It would work like: if the helper is not present, allow heartbeats with any
> vtag like it currently is. Otherwise, for validating vtag too, only allow
> them via expectations.

I'm starting to think the lazy multihoming support is problematic
since you can actually allow pushing holes into the firewall, how easy
would be to push holes with this mode?

You can probably achieve the same effect by allowing heartbearts go
through via NOTRACK and then have some connection pickup from the
middle feature. That, however, would make the user fully aware of the
limitations since it would require explicit configuration.

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Pablo Neira Ayuso]

Hi Marcelo,

On Thu, Dec 24, 2015 at 10:50:28AM -0200, Marcelo Ricardo Leitner wrote:
> I can see how it would work for the vtag-tracking scenario, I think. I'm
> just not seeing the non-tracking one (routers in the middle) yet.
> It doesn't seem it can fit the helper way because it can't prepare
> expectations as it doesn't see the packets in the initial path,
> meaning that we would have to have 2 ways of handling such new
> entries.

We assume that conntrack sees all traffic in our existing helpers,
think of tcp. Otherwise, we may classify packets as invalid if we
don't all packets that belong to the flow.

> It would work like: if the helper is not present, allow heartbeats with any
> vtag like it currently is. Otherwise, for validating vtag too, only allow
> them via expectations.

I'm starting to think the lazy multihoming support is problematic
since you can actually allow pushing holes into the firewall, how easy
would be to push holes with this mode?

You can probably achieve the same effect by allowing heartbearts go
through via NOTRACK and then have some connection pickup from the
middle feature. That, however, would make the user fully aware of the
limitations since it would require explicit configuration.

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Marcelo Ricardo Leitner]

Hi there,

On Wed, Dec 30, 2015 at 01:03:17AM +0100, Pablo Neira Ayuso wrote:
> Hi Marcelo,
> 
> On Thu, Dec 24, 2015 at 10:50:28AM -0200, Marcelo Ricardo Leitner wrote:
> > I can see how it would work for the vtag-tracking scenario, I think. I'm
> > just not seeing the non-tracking one (routers in the middle) yet.
> > It doesn't seem it can fit the helper way because it can't prepare
> > expectations as it doesn't see the packets in the initial path,
> > meaning that we would have to have 2 ways of handling such new
> > entries.
> 
> We assume that conntrack sees all traffic in our existing helpers,
> think of tcp. Otherwise, we may classify packets as invalid if we
> don't all packets that belong to the flow.

Yes, okay.

> > It would work like: if the helper is not present, allow heartbeats with any
> > vtag like it currently is. Otherwise, for validating vtag too, only allow
> > them via expectations.
> 
> I'm starting to think the lazy multihoming support is problematic
> since you can actually allow pushing holes into the firewall, how easy
> would be to push holes with this mode?

Please don't. Currently there is no other way to do it. The check I want
to add only works on a corner case of what we already have, on which we
can do better. It's just that. The way Michal handled the state
transitions is very good and the fact that the conntrack entries are
created as NEW, makes them pass the same user validation rules as a real
new association would do. So there can't be any hitch-hicking...

And for vtag probing, that's not an issue either because SCTP just drops
such heartbeat requests with invalid vtags (at sctp_sf_beat_8_3).

The only vector of attack I can think of that the initial multi-homing
support would allow is a DoS, a flood of incoming heartbeat requests.
Such flood would _not_ end up on the association buffer because if the
transport tuple (src ip, dst ip, src port, dst port) doesn't match a
known association, it's discarded. It's just as any other DoS, but as
they pass the same user validation rules, there should be rules
restricting the rate or IP range or something like that if user is
worried with that. Nothing that could jeopardize the original
association.

Note that the transport validation is performed before the vtag one, and
the stack behavior is to also drop out of the blue packets silently.
Meaning that even if the attacker get a hit at the 32-bit vtag, it will
be discarded by the transport validation firstly.

So what my patch add to it, it pulls/adds this vtag check to an earlier
moment, from the stack itself to the firewall, so that the peer
firewall will be a bit more stateful.

> You can probably achieve the same effect by allowing heartbearts go
> through via NOTRACK and then have some connection pickup from the
> middle feature. That, however, would make the user fully aware of the
> limitations since it would require explicit configuration.

Interesting idea, thanks.

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Marcelo Ricardo Leitner]

Hi there,

On Wed, Dec 30, 2015 at 01:03:17AM +0100, Pablo Neira Ayuso wrote:
> Hi Marcelo,
> 
> On Thu, Dec 24, 2015 at 10:50:28AM -0200, Marcelo Ricardo Leitner wrote:
> > I can see how it would work for the vtag-tracking scenario, I think. I'm
> > just not seeing the non-tracking one (routers in the middle) yet.
> > It doesn't seem it can fit the helper way because it can't prepare
> > expectations as it doesn't see the packets in the initial path,
> > meaning that we would have to have 2 ways of handling such new
> > entries.
> 
> We assume that conntrack sees all traffic in our existing helpers,
> think of tcp. Otherwise, we may classify packets as invalid if we
> don't all packets that belong to the flow.

Yes, okay.

> > It would work like: if the helper is not present, allow heartbeats with any
> > vtag like it currently is. Otherwise, for validating vtag too, only allow
> > them via expectations.
> 
> I'm starting to think the lazy multihoming support is problematic
> since you can actually allow pushing holes into the firewall, how easy
> would be to push holes with this mode?

Please don't. Currently there is no other way to do it. The check I want
to add only works on a corner case of what we already have, on which we
can do better. It's just that. The way Michal handled the state
transitions is very good and the fact that the conntrack entries are
created as NEW, makes them pass the same user validation rules as a real
new association would do. So there can't be any hitch-hicking...

And for vtag probing, that's not an issue either because SCTP just drops
such heartbeat requests with invalid vtags (at sctp_sf_beat_8_3).

The only vector of attack I can think of that the initial multi-homing
support would allow is a DoS, a flood of incoming heartbeat requests.
Such flood would _not_ end up on the association buffer because if the
transport tuple (src ip, dst ip, src port, dst port) doesn't match a
known association, it's discarded. It's just as any other DoS, but as
they pass the same user validation rules, there should be rules
restricting the rate or IP range or something like that if user is
worried with that. Nothing that could jeopardize the original
association.

Note that the transport validation is performed before the vtag one, and
the stack behavior is to also drop out of the blue packets silently.
Meaning that even if the attacker get a hit at the 32-bit vtag, it will
be discarded by the transport validation firstly.

So what my patch add to it, it pulls/adds this vtag check to an earlier
moment, from the stack itself to the firewall, so that the peer
firewall will be a bit more stateful.

> You can probably achieve the same effect by allowing heartbearts go
> through via NOTRACK and then have some connection pickup from the
> middle feature. That, however, would make the user fully aware of the
> limitations since it would require explicit configuration.

Interesting idea, thanks.

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Pablo Neira Ayuso]

Hi Marcelo,

On Wed, Dec 30, 2015 at 09:49:18AM -0200, Marcelo Ricardo Leitner wrote:
> Please don't. Currently there is no other way to do it. The check I want
> to add only works on a corner case of what we already have, on which we
> can do better. It's just that. The way Michal handled the state
> transitions is very good and the fact that the conntrack entries are
> created as NEW, makes them pass the same user validation rules as a real
> new association would do. So there can't be any hitch-hicking...
>
> And for vtag probing, that's not an issue either because SCTP just drops
> such heartbeat requests with invalid vtags (at sctp_sf_beat_8_3).
> 
> The only vector of attack I can think of that the initial multi-homing
> support would allow is a DoS, a flood of incoming heartbeat requests.
> Such flood would _not_ end up on the association buffer because if the
> transport tuple (src ip, dst ip, src port, dst port) doesn't match a
> known association, it's discarded. It's just as any other DoS, but as
> they pass the same user validation rules, there should be rules
> restricting the rate or IP range or something like that if user is
> worried with that. Nothing that could jeopardize the original
> association.
>
> Note that the transport validation is performed before the vtag one, and
> the stack behavior is to also drop out of the blue packets silently.
> Meaning that even if the attacker get a hit at the 32-bit vtag, it will
> be discarded by the transport validation firstly.

Makes sense indeed, thanks for explaining. Then we have to find a
incremental path to extend Michal change to make it fit into what we
have.

> So what my patch add to it, it pulls/adds this vtag check to an earlier
> moment, from the stack itself to the firewall, so that the peer
> firewall will be a bit more stateful.

Please check if you can fit part of this logic into l4proto->error(),
just like ICMP v4 protocol tracker.

BTW, I think these new flows should enter as RELATED, not as NEW, just
like ICMP protocol tracker does.

Your patch basically looks to me like an ad-hoc expectation
infrastructure to handle this case.

Thanks.

Re: [PATCH] netfilter: nf_ct_sctp: validate vtag for new conntrack entries

[Pablo Neira Ayuso]

Hi Marcelo,

On Wed, Dec 30, 2015 at 09:49:18AM -0200, Marcelo Ricardo Leitner wrote:
> Please don't. Currently there is no other way to do it. The check I want
> to add only works on a corner case of what we already have, on which we
> can do better. It's just that. The way Michal handled the state
> transitions is very good and the fact that the conntrack entries are
> created as NEW, makes them pass the same user validation rules as a real
> new association would do. So there can't be any hitch-hicking...
>
> And for vtag probing, that's not an issue either because SCTP just drops
> such heartbeat requests with invalid vtags (at sctp_sf_beat_8_3).
> 
> The only vector of attack I can think of that the initial multi-homing
> support would allow is a DoS, a flood of incoming heartbeat requests.
> Such flood would _not_ end up on the association buffer because if the
> transport tuple (src ip, dst ip, src port, dst port) doesn't match a
> known association, it's discarded. It's just as any other DoS, but as
> they pass the same user validation rules, there should be rules
> restricting the rate or IP range or something like that if user is
> worried with that. Nothing that could jeopardize the original
> association.
>
> Note that the transport validation is performed before the vtag one, and
> the stack behavior is to also drop out of the blue packets silently.
> Meaning that even if the attacker get a hit at the 32-bit vtag, it will
> be discarded by the transport validation firstly.

Makes sense indeed, thanks for explaining. Then we have to find a
incremental path to extend Michal change to make it fit into what we
have.

> So what my patch add to it, it pulls/adds this vtag check to an earlier
> moment, from the stack itself to the firewall, so that the peer
> firewall will be a bit more stateful.

Please check if you can fit part of this logic into l4proto->error(),
just like ICMP v4 protocol tracker.

BTW, I think these new flows should enter as RELATED, not as NEW, just
like ICMP protocol tracker does.

Your patch basically looks to me like an ad-hoc expectation
infrastructure to handle this case.

Thanks.