From ac58f2e94bb205b83c448fd70420b45d4270d4ef Mon Sep 17 00:00:00 2001 From: Florian Westphal Date: Fri, 1 Jun 2012 22:45:49 +0200 Subject: [PATCH 1/5] psd: rip out scanlogd leftovers scanlogd remembers tcp flags and uses the *_CHANGING values in its logger function to determine the best log format to use (e.g. TTL is not logged if HF_TTL_CHANGING was set, as TTL values were different). As psd does not log at all, we do not need track this. Also get rid of bogus/misleading comments. --- extensions/xt_psd.c | 43 ++----------------------------------------- 1 file 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 " 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); From f6b8767228afe33dc92ae8d09c54091ccceaef18 Mon Sep 17 00:00:00 2001 From: Florian Westphal Date: Sat, 2 Jun 2012 21:13:58 +0200 Subject: [PATCH 2/5] psd: add basic validation of userspace matchinfo data 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 | 2 ++ extensions/xt_psd.c | 34 ++++++++++++++++++++++++++++------ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/doc/changelog.txt b/doc/changelog.txt index 2fe752b..a2c37df 100644 --- a/doc/changelog.txt +++ b/doc/changelog.txt @@ -3,6 +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..dc53466 100644 --- a/extensions/xt_psd.c +++ b/extensions/xt_psd.c @@ -278,13 +278,35 @@ 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) From 5b2649b1a29f87b27e963dd68ffa57fe4c01a04b Mon Sep 17 00:00:00 2001 From: Jan Engelhardt Date: Fri, 15 Jun 2012 15:14:32 +0200 Subject: [PATCH 3/5] psd: re-format comments --- extensions/xt_psd.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/extensions/xt_psd.c b/extensions/xt_psd.c index dc53466..5f19351 100644 --- a/extensions/xt_psd.c +++ b/extensions/xt_psd.c @@ -49,28 +49,34 @@ struct port { u_int8_t proto; /* protocol number */ }; -/* +/** * Information we keep per each source address. + * @next: next entry with the same hash + * @timestamp: last update time + * @count: number of ports in the list + * @weight: total weight of ports in the list */ 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 port ports[SCAN_MAX_COUNT - 1]; /* List of ports */ + struct host *next; + unsigned long timestamp; + struct in_addr src_addr; + struct in_addr dest_addr; + unsigned short src_port; + int count; + int weight; + struct port ports[SCAN_MAX_COUNT-1]; }; -/* +/** * State information. + * @list: list of source addresses + * @hash: pointers into the list + * @index: oldest entry to be replaced */ 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 */ + struct host list[LIST_SIZE]; + struct host *hash[HASH_SIZE]; } state; /* From 3736a265d868f29f2c22fe04292435b191efd61e Mon Sep 17 00:00:00 2001 From: Florian Westphal Date: Thu, 14 Jun 2012 10:33:15 +0200 Subject: [PATCH 4/5] psd: reduce size of struct host We can use u16, saving 8 bytes total (weight cannot exceed PSD_MAX_RATE, 10000). Also re-format comments & struct initializers. No functional changes. --- extensions/libxt_psd.c | 24 ++++++++++++------------ extensions/xt_psd.c | 7 ++++--- 2 files changed, 16 insertions(+), 15 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 5f19351..f5fcca0 100644 --- a/extensions/xt_psd.c +++ b/extensions/xt_psd.c @@ -61,9 +61,9 @@ struct host { unsigned long timestamp; struct in_addr src_addr; struct in_addr dest_addr; - unsigned short src_port; - int count; - int weight; + __be16 src_port; + uint16_t count; + uint8_t weight; struct port ports[SCAN_MAX_COUNT-1]; }; @@ -77,6 +77,7 @@ static struct { spinlock_t lock; struct host list[LIST_SIZE]; struct host *hash[HASH_SIZE]; + int index; } state; /* From d66d07d01df7eb975908350fed3010f8622a16fe Mon Sep 17 00:00:00 2001 From: Florian Westphal Date: Thu, 14 Jun 2012 10:53:15 +0200 Subject: [PATCH 5/5] psd: move defines to user/kernelspace part where possible 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 f5fcca0..b67b64e 100644 --- a/extensions/xt_psd.c +++ b/extensions/xt_psd.c @@ -40,6 +40,15 @@ MODULE_AUTHOR(" Mohd Nawawi Mohamad Jamili " 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;