[PATCH] xt_u32.c - make negative offsets work near / at the end of packet

[PATCH] xt_u32.c - make negative offsets work near / at the end of packet

[Michal Soltys]

In the older version of u32 match there was a possibility to succesfully
use negative offsets while being at or near the end of the packet.

For example - a method presented in:

http://www.stearns.org/doc/iptables-u32.v0.1.7.html

... to find a tcp packet with no payload :

0>>22&0x3C @ 12>>26&0x3C @ -3&0xFF=0:255

But with current xt_u32.c code this is impossible due to subsequent
sanity checks:

        if (at + val < at)
                return false;
        at += val;
        pos = number;
        if (at + 4 < at || skb->len < at + 4 (#3) ||
            pos > skb->len - at - 4 (#4) )
                return false;

        if (skb_copy_bits(skb, at + pos, &n,
            sizeof(n)) < 0)

In particular, condition (#3) will make the function return false, if
'at' is at / near the end of packet, regardless of the 'pos' value used.
Furthermore, (#4) would inhibit any "negative" 'pos' value anyway.

If I haven't missed anything, the whole sequence could be changed to:

        at += val;
        if (at + number + 4 < 4 || skb->len < at + number + 4)
                return false;

        if (skb_copy_bits(skb, at + number, &n,
                            sizeof(n)) < 0)

This allows both 'pos' and 'val' to be interpreted in any (+/-) way.
Sanity checks will keep the read in bounds, and the first condition will
be fine for unsigned values. I'm not sure how that approach would behave
on non-x86 architectures though.

Iptables doesn't complain about using negative values with -m u32, so I
assumed that original intent was to allow them.

Tiny patch below implements proposed changes.

Michal Soltys (1):
  xt_u32.c - make negative offsets work near / at the end of packet

 net/netfilter/xt_u32.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

[PATCH] xt_u32.c - make negative offsets work near / at the end of packet

[Michal Soltys]

Current sanity checks in xt_u32.c inhibit successful use of negative
offets when the current position is near or at the end of a packet.

The following patch changes the sanity checks to be a bit more
flexible - values read from the packet and specified as @ offsets
can be interpreted both as positive and negative.

Signed-off-by: Michal Soltys <soltys@ziu.info>
---
 net/netfilter/xt_u32.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/xt_u32.c b/net/netfilter/xt_u32.c
index 24a5276..bb3a63d 100644
--- a/net/netfilter/xt_u32.c
+++ b/net/netfilter/xt_u32.c
@@ -58,15 +58,11 @@ static bool u32_match_it(const struct xt_u32 *data,
 				val >>= number;
 				break;
 			case XT_U32_AT:
-				if (at + val < at)
-					return false;
 				at += val;
-				pos = number;
-				if (at + 4 < at || skb->len < at + 4 ||
-				    pos > skb->len - at - 4)
+				if (at + number + 4 < 4 || skb->len < at + number + 4)
 					return false;
 
-				if (skb_copy_bits(skb, at + pos, &n,
+				if (skb_copy_bits(skb, at + number, &n,
 						    sizeof(n)) < 0)
 					BUG();
 				val = ntohl(n);
-- 
1.6.4

Re: [PATCH] xt_u32.c - make negative offsets work near / at the end of packet

[Jan Engelhardt]

On Sunday 2009-09-06 02:36, Michal Soltys wrote:
>But with current xt_u32.c code this is impossible due to subsequent
>sanity checks:
>
>        if (at + val < at)
>                return false;
>        at += val;
>        pos = number;
>        if (at + 4 < at || skb->len < at + 4 (#3) ||
>            pos > skb->len - at - 4 (#4) )
>                return false;
>
>        if (skb_copy_bits(skb, at + pos, &n,
>            sizeof(n)) < 0)
>
>In particular, condition (#3) will make the function return false, if
>'at' is at / near the end of packet, regardless of the 'pos' value used.
>Furthermore, (#4) would inhibit any "negative" 'pos' value anyway.
>
>If I haven't missed anything,

The code stems from Patrick's comments [to my original u32 submission]
about overflow checks. It seems you are basically throwing these
out of the window.

>        at += val;

This can overflow. Or if @val is signed, it may even underflow, and
surely give funny results when @at is unsigned at the same time.
Then there is also the question on what should happen when you
deal with values that are - in their respective context as
defined in an RFC or something - uint32_ts. Though probably a little
unlikely in the real world at this time, I would not count on
never hitting a jumbo frame size field of, say, 2147483656 bytes.

>        if (at + number + 4 < 4 || skb->len < at + number + 4)

at+number can overflow, so can at+number+4.
The check is insufficient too, because let at+number+4 may yield,
through overflows, a value that is still greater than 4.

Re: [PATCH] xt_u32.c - make negative offsets work near / at the end of packet

[Michal Soltys]

Jan Engelhardt wrote:
> On Sunday 2009-09-06 02:36, Michal Soltys wrote:
>>But with current xt_u32.c code this is impossible due to subsequent
>>sanity checks:
>>
>>        if (at + val < at)
>>                return false;
>>        at += val;
>>        pos = number;
>>        if (at + 4 < at || skb->len < at + 4 (#3) ||
>>            pos > skb->len - at - 4 (#4) )
>>                return false;
>>
>>        if (skb_copy_bits(skb, at + pos, &n,
>>            sizeof(n)) < 0)
>>
>>In particular, condition (#3) will make the function return false, if
>>'at' is at / near the end of packet, regardless of the 'pos' value used.
>>Furthermore, (#4) would inhibit any "negative" 'pos' value anyway.
>>
>>If I haven't missed anything,
> 
> The code stems from Patrick's comments [to my original u32 submission]
> about overflow checks. It seems you are basically throwing these
> out of the window.
> 
>>        at += val;
> 
> This can overflow. Or if @val is signed, it may even underflow, and
> surely give funny results when @at is unsigned at the same time.
> Then there is also the question on what should happen when you
> deal with values that are - in their respective context as
> defined in an RFC or something - uint32_ts. Though probably a little
> unlikely in the real world at this time, I would not count on
> never hitting a jumbo frame size field of, say, 2147483656 bytes.
> 
>>        if (at + number + 4 < 4 || skb->len < at + number + 4)
> 
> at+number can overflow, so can at+number+4.
> The check is insufficient too, because let at+number+4 may yield,
> through overflows, a value that is still greater than 4.
> 

The first check is against "left side" and it should be greater 
than 4 for the function to not return false immediately. If it's too great 
it will be caught by the second test.


More detailed rationale:

I know this and previous (as you commented) can overflow, but we're 
checking the final result here. Regardless of the value of at, val and pos 
during computations (or how we interpret them) - the final check is done 
using unsigned values and tested against "where would skb_copy_bits end".

The first check makes sure the read starts at least from 0 (that's why 
at + number + 4 must be bigger or equal 4 for it to pass). And it's 
written this way so it works properly with all the variables being 
unsigned (of course I couldn't do: at + number < 0 as that would never be 
true with unsigned vars).

The second check makes sure skb_copy_bits doesn't try to read past skb. 
For example:

at  = 0x11       (17)
val = 0xfffffffb (user interp. as -5)
pos = 0xfffffff6 (user interp. as -10)
skb->len = 21

at + val + pos + 4 = 2

The tests here: 6 < 4 and 21 < 6 - so all is fine (all tests are false)

But with val -8 and pos -10 we would get: 3 < 4 and 21 < 3 (return on first).
    with val -20 and pos -20 we would get: 0xffffffed < 4 and 21 < 0xffffffed (return on second).

and so on...

All values in xt_ut32.c are defined as strictly unsigned and 32 bits wide. All values 
supplied on the command line or read from packet's data will be treated as such, but 
can be interpreted any way - add and sub instructions don't really care for that, operation 
and data is the same (that's where my qeustion about non-x86 archs came from).

17 + 0xffffffff (-1) is 16
0xfffffff0 + 0xffffffff (-1) is 0xffffffef

... well, you see my point. Iptables will take signed values w/o any problems, but will 
output them (-L) as unsigned.

I've been running u32 patched this way, and tested with clearly wrong rules such as:

'0 << 25 @ -30000 & 0xFF = 0:0xFF'
'0 & 0 @ -30000 & 0xFF = 0:0xFF'
'0 & 0 @ 30000 & 0xFF = 0:0xFF'

and as expected, never ran into any problems.