[xt-addons][PATCH 0/4] psd match cleanup patches

[xt-addons][PATCH 0/4] psd match cleanup patches

[Florian Westphal]

The following changes since commit 7cc774641a584803f66b643c9fede7a6bf2664b8:

  all: remove trailing squatspaces (2012-06-10 22:31:10 +0200)

are available in the git repository at:
  git://git.breakpoint.cc/fw/xtables-addons.git psd_cleanups

Florian Westphal (4):
      psd: rip out scanlogd leftovers
      psd: add basic validation of userspace matchinfo data
      psd: reduce size of struct host
      psd: move defines to user/kernelspace part where possible

 doc/changelog.txt      |    3 +-
 extensions/libxt_psd.c |   26 +++++++------
 extensions/xt_psd.c    |  100 +++++++++++++++++++++--------------------------
 extensions/xt_psd.h    |   11 -----
 4 files changed, 61 insertions(+), 79 deletions(-)

Patches will follow up as reply to this email.

[PATCH 1/4] psd: rip out scanlogd leftovers

[Florian Westphal]

scanlogd remembers tcp flags and uses the *_CHANGING values
in its logger function to determine the best log format to
use (e.g. TTL isn't logged if HF_TTL_CHANGING was set, as TTL
values were different).

As psd doesn't log at all, we don't need track this.

Also get rid of bogus/misleading comments.
---
 extensions/xt_psd.c |   43 ++-----------------------------------------
 1 files changed, 2 insertions(+), 41 deletions(-)

diff --git a/extensions/xt_psd.c b/extensions/xt_psd.c
index acb5e8e..c044c25 100644
--- a/extensions/xt_psd.c
+++ b/extensions/xt_psd.c
@@ -40,10 +40,6 @@ MODULE_AUTHOR(" Mohd Nawawi Mohamad Jamili <nawawi@tracenetworkcorporation.com>"
 MODULE_DESCRIPTION("Xtables: PSD - portscan detection");
 MODULE_ALIAS("ipt_psd");
 
-#define HF_DADDR_CHANGING   0x01
-#define HF_SPORT_CHANGING   0x02
-#define HF_TOS_CHANGING	    0x04
-#define HF_TTL_CHANGING	    0x08
 
 /*
  * Information we keep per each target port
@@ -51,8 +47,6 @@ MODULE_ALIAS("ipt_psd");
 struct port {
 	u_int16_t number;      /* port number */
 	u_int8_t proto;        /* protocol number */
-	u_int8_t and_flags;    /* tcp ANDed flags */
-	u_int8_t or_flags;     /* tcp ORed flags */
 };
 
 /*
@@ -67,9 +61,6 @@ struct host {
 	int count;								/* Number of ports in the list */
 	int weight;								/* Total weight of ports in the list */
 	struct port ports[SCAN_MAX_COUNT - 1];	/* List of ports */
-	unsigned char tos;						/* TOS */
-	unsigned char ttl;						/* TTL */
-	unsigned char flags;					/* HF_ flags bitmask */
 };
 
 /*
@@ -111,26 +102,20 @@ xt_psd_match(const struct sk_buff *pskb, struct xt_action_param *match)
 	} _buf;
 	struct in_addr addr;
 	u_int16_t src_port,dest_port;
-  	u_int8_t tcp_flags, proto;
+	u_int8_t proto;
 	unsigned long now;
 	struct host *curr, *last, **head;
 	int hash, index, count;
 	/* Parameters from userspace */
 	const struct xt_psd_info *psdinfo = match->matchinfo;
 
-	/* IP header */
 	iph = ip_hdr(pskb);
-
-	/* Sanity check */
 	if (iph->frag_off & htons(IP_OFFSET)) {
 		pr_debug("sanity check failed\n");
 		return false;
 	}
 
-	/* TCP or UDP ? */
 	proto = iph->protocol;
-	/* Get the source address, source & destination ports, and TCP flags */
-
 	addr.s_addr = iph->saddr;
 	/* We're using IP address 0.0.0.0 for a special purpose here, so don't let
 	 * them spoof us. [DHCP needs this feature - HW] */
@@ -148,7 +133,6 @@ xt_psd_match(const struct sk_buff *pskb, struct xt_action_param *match)
 		/* Yep, it's dirty */
 		src_port = tcph->source;
 		dest_port = tcph->dest;
-		tcp_flags = *((u_int8_t*)tcph + 13);
 	} else if (proto == IPPROTO_UDP || proto == IPPROTO_UDPLITE) {
 		udph = skb_header_pointer(pskb, match->thoff,
 		       sizeof(_buf.udph), &_buf.udph);
@@ -156,14 +140,11 @@ xt_psd_match(const struct sk_buff *pskb, struct xt_action_param *match)
 			return false;
 		src_port  = udph->source;
 		dest_port = udph->dest;
-		tcp_flags = 0;
 	} else {
 		pr_debug("protocol not supported\n");
 		return false;
 	}
 
-	/* Use jiffies here not to depend on someone setting the time while we're
-	 * running; we need to be careful with possible return value overflows. */
 	now = jiffies;
 
 	spin_lock(&state.lock);
@@ -181,7 +162,6 @@ xt_psd_match(const struct sk_buff *pskb, struct xt_action_param *match)
 		} while ((curr = curr->next) != NULL);
 
 	if (curr != NULL) {
-
 		/* We know this address, and the entry isn't too old. Update it. */
 		if (now - curr->timestamp <= (psdinfo->delay_threshold*HZ)/100 &&
 		    time_after_eq(now, curr->timestamp)) {
@@ -190,8 +170,6 @@ xt_psd_match(const struct sk_buff *pskb, struct xt_action_param *match)
 			for (index = 0; index < curr->count; index++) {
 				if (curr->ports[index].number == dest_port) {
 					curr->ports[index].proto = proto;
-					curr->ports[index].and_flags &= tcp_flags;
-					curr->ports[index].or_flags |= tcp_flags;
 					goto out_no_match;
 				}
 			}
@@ -203,26 +181,15 @@ xt_psd_match(const struct sk_buff *pskb, struct xt_action_param *match)
 			/* Packet to a new port, and not TCP/ACK: update the timestamp */
 			curr->timestamp = now;
 
-			/* Logged this scan already? Then drop the packet. */
+			/* Matched this scan already? Then Leave. */
 			if (curr->weight >= psdinfo->weight_threshold)
 				goto out_match;
 
-			/* Specify if destination address, source port, TOS or TTL are not fixed */
-			if (curr->dest_addr.s_addr != iph->daddr)
-				curr->flags |= HF_DADDR_CHANGING;
-			if (curr->src_port != src_port)
-				curr->flags |= HF_SPORT_CHANGING;
-			if (curr->tos != iph->tos)
-				curr->flags |= HF_TOS_CHANGING;
-			if (curr->ttl != iph->ttl)
-				curr->flags |= HF_TTL_CHANGING;
-
 			/* Update the total weight */
 			curr->weight += (ntohs(dest_port) < 1024) ?
 				psdinfo->lo_ports_weight : psdinfo->hi_ports_weight;
 
 			/* Got enough destination ports to decide that this is a scan? */
-			/* Then log it and drop the packet. */
 			if (curr->weight >= psdinfo->weight_threshold)
 				goto out_match;
 
@@ -230,8 +197,6 @@ xt_psd_match(const struct sk_buff *pskb, struct xt_action_param *match)
 			if (curr->count < ARRAY_SIZE(curr->ports)) {
 				curr->ports[curr->count].number = dest_port;
 				curr->ports[curr->count].proto = proto;
-				curr->ports[curr->count].and_flags = tcp_flags;
-				curr->ports[curr->count].or_flags = tcp_flags;
 				curr->count++;
 			}
 
@@ -303,10 +268,6 @@ xt_psd_match(const struct sk_buff *pskb, struct xt_action_param *match)
 	curr->weight = (ntohs(dest_port) < 1024) ? psdinfo->lo_ports_weight : psdinfo->hi_ports_weight;
 	curr->ports[0].number = dest_port;
 	curr->ports[0].proto = proto;
-	curr->ports[0].and_flags = tcp_flags;
-	curr->ports[0].or_flags = tcp_flags;
-	curr->tos = iph->tos;
-	curr->ttl = iph->ttl;
 
 out_no_match:
 	spin_unlock(&state.lock);
-- 
1.7.3.4

[PATCH 2/4] psd: add basic validation of userspace matchinfo data

[Florian Westphal]

psd multiplies weight_thresh by HZ, so it could overflow.

Userspace libxt_psd refuses values exceeding PSD_MAX_RATE,
so check that on kernel side, too.

Also, setting 0 weight for both privileged and highports
will cause psd to never match at all.

Reject 0 weight threshold, too because it makes no sense
(triggers match for every initial packet).
---
 doc/changelog.txt   |    3 ++-
 extensions/xt_psd.c |   32 ++++++++++++++++++++++++++------
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/doc/changelog.txt b/doc/changelog.txt
index 2fe752b..d266550 100644
--- a/doc/changelog.txt
+++ b/doc/changelog.txt
@@ -3,7 +3,8 @@ HEAD
 ====
 Fixes:
 - xt_psd: avoid crash due to curr->next corruption
-
+Changes:
+- xt_psd: reject invalid match options
 
 v1.42 (2012-04-05)
 ==================
diff --git a/extensions/xt_psd.c b/extensions/xt_psd.c
index c044c25..f3fa336 100644
--- a/extensions/xt_psd.c
+++ b/extensions/xt_psd.c
@@ -278,13 +278,33 @@ out_match:
 	return true;
 }
 
+static int psd_mt_check(const struct xt_mtchk_param *par)
+{
+	const struct xt_psd_info *info = par->matchinfo;
+
+	if (info->weight_threshold == 0) /* 0 would match on every 1st packet */
+		return -EINVAL;
+
+	if ((info->lo_ports_weight|info->hi_ports_weight) == 0) /* would never match */
+		return -EINVAL;
+
+	if (info->delay_threshold > PSD_MAX_RATE ||
+	    info->weight_threshold > PSD_MAX_RATE ||
+	    info->lo_ports_weight > PSD_MAX_RATE ||
+	    info->hi_ports_weight > PSD_MAX_RATE)
+		return -EINVAL;
+
+	return 0;
+}
+
 static struct xt_match xt_psd_reg __read_mostly = {
-	.name		= "psd",
-	.family    = NFPROTO_IPV4,
-	.revision  = 1,
-	.match		= xt_psd_match,
-	.matchsize	= sizeof(struct xt_psd_info),
-	.me			= THIS_MODULE,
+	.name       = "psd",
+	.family     = NFPROTO_IPV4,
+	.revision   = 1,
+	.checkentry = psd_mt_check,
+	.match      = xt_psd_match,
+	.matchsize  = sizeof(struct xt_psd_info),
+	.me         = THIS_MODULE,
 };
 
 static int __init xt_psd_init(void)
-- 
1.7.3.4

[PATCH 3/4] psd: reduce size of struct host

[Florian Westphal]

We can use u16, saving 8 bytes total (weight cant't exceed PSD_MAX_RATE,
10000).  Also re-format comments & struct initializers.

no functional changes.
---
 extensions/libxt_psd.c |   24 ++++++++++++------------
 extensions/xt_psd.c    |   16 ++++++++--------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/extensions/libxt_psd.c b/extensions/libxt_psd.c
index e60178b..483c69a 100644
--- a/extensions/libxt_psd.c
+++ b/extensions/libxt_psd.c
@@ -137,19 +137,19 @@ static void psd_mt_save(const void *ip, const struct xt_entry_match *match)
 }
 
 static struct xtables_match psd_mt_reg = {
-	.name			= "psd",
-	.version		= XTABLES_VERSION,
-	.revision   	= 1,
-	.family        = NFPROTO_IPV4,
-	.size			= XT_ALIGN(sizeof(struct xt_psd_info)),
+	.name           = "psd",
+	.version        = XTABLES_VERSION,
+	.revision       = 1,
+	.family         = NFPROTO_IPV4,
+	.size           = XT_ALIGN(sizeof(struct xt_psd_info)),
 	.userspacesize	= XT_ALIGN(sizeof(struct xt_psd_info)),
-	.help			= psd_mt_help,
-	.init			= psd_mt_init,
-	.parse			= psd_mt_parse,
-	.final_check	= psd_mt_final_check,
-	.print			= psd_mt_print,
-	.save			= psd_mt_save,
-	.extra_opts		= psd_mt_opts,
+	.help           = psd_mt_help,
+	.init           = psd_mt_init,
+	.parse          = psd_mt_parse,
+	.final_check    = psd_mt_final_check,
+	.print          = psd_mt_print,
+	.save           = psd_mt_save,
+	.extra_opts     = psd_mt_opts,
 };
 
 static __attribute__((constructor)) void psd_mt_ldr(void)
diff --git a/extensions/xt_psd.c b/extensions/xt_psd.c
index f3fa336..00ff1d4 100644
--- a/extensions/xt_psd.c
+++ b/extensions/xt_psd.c
@@ -53,13 +53,13 @@ struct port {
  * Information we keep per each source address.
  */
 struct host {
-	struct host *next;						/* Next entry with the same hash */
-	unsigned long timestamp;					/* Last update time */
-	struct in_addr src_addr;				/* Source address */
-	struct in_addr dest_addr;				/* Destination address */
-	unsigned short src_port;				/* Source port */
-	int count;								/* Number of ports in the list */
-	int weight;								/* Total weight of ports in the list */
+	struct host *next;			/* Next entry with the same hash */
+	unsigned long timestamp;		/* Last update time */
+	struct in_addr src_addr;		/* Source address */
+	struct in_addr dest_addr;		/* Destination address */
+	__be16 src_port;			/* Source port */
+	u16 weight;				/* Total weight of ports in the list */
+	u8 count;				/* Number of ports in the list */
 	struct port ports[SCAN_MAX_COUNT - 1];	/* List of ports */
 };
 
@@ -70,7 +70,7 @@ static struct {
 	spinlock_t lock;
 	struct host list[LIST_SIZE];	/* List of source addresses */
 	struct host *hash[HASH_SIZE];	/* Hash: pointers into the list */
-	int index;						/* Oldest entry to be replaced */
+	int index;			/* Oldest entry to be replaced */
 } state;
 
 /*
-- 
1.7.3.4

[PATCH 4/4] psd: move defines to user/kernelspace part where possible

[Florian Westphal]

Some of these defines have no meaning in userspace, so there
is no need to make those available.
---
 extensions/libxt_psd.c |    2 ++
 extensions/xt_psd.c    |    9 +++++++++
 extensions/xt_psd.h    |   11 -----------
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/extensions/libxt_psd.c b/extensions/libxt_psd.c
index 483c69a..bd03480 100644
--- a/extensions/libxt_psd.c
+++ b/extensions/libxt_psd.c
@@ -30,6 +30,8 @@
 #include "xt_psd.h"
 #include "compat_user.h"
 
+#define SCAN_DELAY_THRESHOLD		300
+
 /* Function which prints out usage message. */
 static void psd_mt_help(void) {
 	printf(
diff --git a/extensions/xt_psd.c b/extensions/xt_psd.c
index 00ff1d4..f459605 100644
--- a/extensions/xt_psd.c
+++ b/extensions/xt_psd.c
@@ -40,6 +40,15 @@ MODULE_AUTHOR(" Mohd Nawawi Mohamad Jamili <nawawi@tracenetworkcorporation.com>"
 MODULE_DESCRIPTION("Xtables: PSD - portscan detection");
 MODULE_ALIAS("ipt_psd");
 
+/*
+ * Keep track of up to LIST_SIZE source addresses, using a hash table of
+ * HASH_SIZE entries for faster lookups, but limiting hash collisions to
+ * HASH_MAX source addresses per the same hash value.
+ */
+#define LIST_SIZE			0x100
+#define HASH_LOG			9
+#define HASH_SIZE			(1 << HASH_LOG)
+#define HASH_MAX			0x10
 
 /*
  * Information we keep per each target port
diff --git a/extensions/xt_psd.h b/extensions/xt_psd.h
index ac65687..89b48fe 100644
--- a/extensions/xt_psd.h
+++ b/extensions/xt_psd.h
@@ -19,17 +19,6 @@
 #define SCAN_MIN_COUNT			7
 #define SCAN_MAX_COUNT			(SCAN_MIN_COUNT * PORT_WEIGHT_PRIV)
 #define SCAN_WEIGHT_THRESHOLD		SCAN_MAX_COUNT
-#define SCAN_DELAY_THRESHOLD		(300) /* old usage of HZ here was erroneously and broke under uml */
-
-/*
- * Keep track of up to LIST_SIZE source addresses, using a hash table of
- * HASH_SIZE entries for faster lookups, but limiting hash collisions to
- * HASH_MAX source addresses per the same hash value.
- */
-#define LIST_SIZE			0x100
-#define HASH_LOG			9
-#define HASH_SIZE			(1 << HASH_LOG)
-#define HASH_MAX			0x10
 
 struct xt_psd_info {
 	__u32 weight_threshold;
-- 
1.7.3.4

Re: [xt-addons][PATCH 0/4] psd match cleanup patches

[Jan Engelhardt]

On Thursday 2012-06-14 22:13, Florian Westphal wrote:

>are available in the git repository at:
>  git://git.breakpoint.cc/fw/xtables-addons.git psd_cleanups

Taken, with some edits w.r.t. formatting. (You can use `git diff 
..origin/master` to see)