Hi Jan,
As far as the security hole goes, only the changes to
make it parse the in ipv4options_rd is needed. If my understanding for
the option *any* was wrong given the manual, then I'll take that back.
Let me know how you want to proceed with this, I can provide you another
patch with the update to ipv4options_rd only, and with proper
indentation.
Regards,
- Eivind
----- Original Message -----
From: Jan Engelhardt <jengelh@xxxxxxxxxx>
To: Eivind Naess <eivnaes@xxxxxxxxx>
Cc: "netfilter-devel@xxxxxxxxxxxxxxx" <netfilter-devel@xxxxxxxxxxxxxxx>
Sent: Saturday, November 5, 2011 7:34 AM
Subject: Re: Found a dead-freeze bug, in xt_ipv4options.c -- patch provided
On Thursday 2011-11-03 17:28, Eivind Naess wrote:
>-static uint32_t ipv4options_rd(const uint8_t *data, int len)
>+static uint32_t ipv4options_rd(const uint8_t *ptr, int len)
> {
> uint32_t opts = 0;
>+ uint32_t olen = 0;
>
> while (len >= 2) {
>- opts |= 1 << (data[0] & 0x1F);
>- len -= data[1];
>- data += data[1];
>+
>+ /* Handle special options */
>+ switch (*ptr) {
>+ case IPOPT_END:
>+ return opts;
>+ case IPOPT_NOOP:
>+ len--;
>+ ptr++;
>+ continue;
>+ }
>+
>+ /* Get the length of the option */
>+ olen = ptr[1];
>+ if (olen < 2 || olen > len)
>+ return opts;
>+
>+ /* Set the option bit, and incr/decr the invariants */
>+ opts |= 1 << (*ptr & 0x1F);
>+ len -= olen;
>+ ptr += olen;
> }
>
> return opts;
Whitespace damage, I had to apply this manually, and in doing so,
reduced the size. You can test the patch from the "v4options" branch,
copied below.
>@@ -36,13 +54,20 @@
> uint32_t opts = 0;
> uint16_t len = ip_hdrlen(skb) - sizeof(struct iphdr);
>
>- if (len > 0)
>+ if (len > 0) {
> opts = ipv4options_rd((const void *)iph +
> sizeof(struct iphdr), len);
>+ if(info->flags & XT_V4OPTS_ANY) {
>+ return !!opts; /* !! to make bool value */
>+ }else {
>
> opts ^= info->invert;
> opts &= info->map;
>- return (info->flags & XT_V4OPTS_ANY) ? opts : opts == info->map;
>+ return (opts == info->map);
>+ }
>+ }
>+
>+ return 0;
> }
XT_V4OPTS_ANY is meant to match on {any of {the bits present in
info->map}}, not {any of {all possible IPv4 options}} according to
the documentation. The test for XT_V4OPTS_ANY is therefore supposed
to come after the ^ and & operations, and that is what the current
(/^-/) code does AFAICS, or did I miss something?
commit 75cd1d7d6a3747ec297f24f67f589acd8f5a9859
Author: Eivind Naess <eivnaes@xxxxxxxxx>
Date: Thu Nov 3 09:28:46 2011 -0700
xt_ipv4options: fix an infinite loop
---
doc/changelog.txt | 1 +
extensions/xt_ipv4options.c | 11 +++++++++++
2 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/doc/changelog.txt b/doc/changelog.txt
index 3557175..81fcbdc 100644
--- a/doc/changelog.txt
+++ b/doc/changelog.txt
@@ -5,6 +5,7 @@ Fixes:
- build: the code actually requires at least iptables 1.4.5 (would yield a
compile error otherwise), make sure configure checks for it; update INSTALL
- xt_ECHO: fix kernel warning about RTAX_HOPLIMIT being used
+- xt_ipv4options: fix an infinite loop
Changes:
- xt_ECHO: now calculates UDP checksum
Enhancements:
diff --git a/extensions/xt_ipv4options.c b/extensions/xt_ipv4options.c
index 42481f7..5e9d34c 100644
--- a/extensions/xt_ipv4options.c
+++ b/extensions/xt_ipv4options.c
@@ -20,6 +20,17 @@ static uint32_t ipv4options_rd(const uint8_t *data, int len)
uint32_t opts = 0;
while (len >= 2) {
+ switch (data[0]) {
+ case IPOPT_END:
+ return opts;
+ case IPOPT_NOOP:
+ --len;
+ ++data;
+ continue;
+ }
+
+ if (data[1] < 2 || data[1] > len)
+ return opts;
opts |= 1 << (data[0] & 0x1F);
len -= data[1];
data += data[1];
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
