net: fix search limit handling in skb_find_text()

Suppose that we're trying to use an xt_string netfilter module to match a
string in a specially crafted packet that has "a nice string" starting at
offset 28.

It could be done in iptables like this:

-A some_chain -m string --string "a nice string" --algo bm --from 28 --to 38 -j DROP

And it would work as expected. Now changing that to

-A some_chain -m string --string "a nice string" --algo bm --from 29 --to 38 -j DROP

breaks the match, as expected. But, if we try to make

-A some_chain -m string --string "a nice string" --algo bm --from 20 --to 28 -j DROP

then it suddenly works again! So the 'to' parameter seems to be inclusive, not
working as an offset after which no search should be done. OK, now if we try:

-A some_chain -m string --string "a nice string" --algo bm --from 28 --to 28 -j DROP

it doesn't work. So, for the case of equal 'from' and 'to' it's treated in a
different way.

The first behaviour (matching at 'to' offset) comes from skb_find_text()
comparison. The second one (not matching if 'from' and 'to' are equal) comes
from skb_seq_read() check for (abs_offset >= st->upper_offset).

I think that the way skb_find_text() handles 'to' is wrong and should be fixed
so that we always have predictable behaviour -- only match before 'to' offset.

There are currently only five usages of skb_find_text() in the kernel and it
looks to me that none of them expect to match something at the 'to' offset,
so probably this change is safe.

Reported-by: Edward Makarov <makarov@altell.ru>
Tested-by: Edward Makarov <makarov@altell.ru>
Signed-off-by: Roman I Khimov <khimov@altell.ru>
Cc: Phil Oester <kernel@linuxace.com>
---
 net/core/skbuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Cc'ing Thomas.

On Mon, Jun 15, 2015 at 12:11:58PM +0300, Roman I Khimov wrote:
> Suppose that we're trying to use an xt_string netfilter module to match a
> string in a specially crafted packet that has "a nice string" starting at
> offset 28.
> 
> It could be done in iptables like this:
> 
> -A some_chain -m string --string "a nice string" --algo bm --from 28 --to 38 -j DROP
> 
> And it would work as expected. Now changing that to
> 
> -A some_chain -m string --string "a nice string" --algo bm --from 29 --to 38 -j DROP
> 
> breaks the match, as expected. But, if we try to make
> 
> -A some_chain -m string --string "a nice string" --algo bm --from 20 --to 28 -j DROP
>
> then it suddenly works again! So the 'to' parameter seems to be inclusive, not
> working as an offset after which no search should be done. OK, now if we try:
> 
> -A some_chain -m string --string "a nice string" --algo bm --from 28 --to 28 -j DROP

Can you reproduce the same behaviour with the km algo?

> it doesn't work. So, for the case of equal 'from' and 'to' it's treated in a
> different way.
>
> The first behaviour (matching at 'to' offset) comes from skb_find_text()
> comparison. The second one (not matching if 'from' and 'to' are equal) comes
> from skb_seq_read() check for (abs_offset >= st->upper_offset).
> 
> I think that the way skb_find_text() handles 'to' is wrong and should be fixed
> so that we always have predictable behaviour -- only match before 'to' offset.
>
> There are currently only five usages of skb_find_text() in the kernel and it
> looks to me that none of them expect to match something at the 'to' offset,
> so probably this change is safe.

So both 'from' and 'to' are inclusive which seems consistent to me. If
you make 'to' non-inclusive, then that will change the third example
above, right? That will break existing setups for people that are
relying on this behaviour. This has been exposed in this way for long
time, so we should avoid that breakage.

I would suggest you fix the --from X --to Y where X == Y which is not
doing what people would expect.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

В письме от 15 июня 2015 19:06:39 пользователь Pablo Neira Ayuso написал:
> On Mon, Jun 15, 2015 at 12:11:58PM +0300, Roman I Khimov wrote:
> > Suppose that we're trying to use an xt_string netfilter module to match a
> > string in a specially crafted packet that has "a nice string" starting at
> > offset 28.
> > 
> > It could be done in iptables like this:
> > 
> > -A some_chain -m string --string "a nice string" --algo bm --from 28 --to
> > 38 -j DROP
> > 
> > And it would work as expected. Now changing that to
> > 
> > -A some_chain -m string --string "a nice string" --algo bm --from 29 --to
> > 38 -j DROP
> > 
> > breaks the match, as expected. But, if we try to make
> > 
> > -A some_chain -m string --string "a nice string" --algo bm --from 20 --to
> > 28 -j DROP
> > 
> > then it suddenly works again! So the 'to' parameter seems to be inclusive,
> > not working as an offset after which no search should be done. OK, now if
> > we try:
> > 
> > -A some_chain -m string --string "a nice string" --algo bm --from 28 --to
> > 28 -j DROP
> Can you reproduce the same behaviour with the km algo?

Will try tomorrow MSK time.

> > The first behaviour (matching at 'to' offset) comes from skb_find_text()
> > comparison. The second one (not matching if 'from' and 'to' are equal)
> > comes from skb_seq_read() check for (abs_offset >= st->upper_offset).
> > 
> > I think that the way skb_find_text() handles 'to' is wrong and should be
> > fixed so that we always have predictable behaviour -- only match before
> > 'to' offset.
> > 
> > There are currently only five usages of skb_find_text() in the kernel and
> > it looks to me that none of them expect to match something at the 'to'
> > offset, so probably this change is safe.
> 
> So both 'from' and 'to' are inclusive which seems consistent to me. If
> you make 'to' non-inclusive, then that will change the third example
> above, right? 

Yep.

> That will break existing setups for people that are
> relying on this behaviour. This has been exposed in this way for long
> time, so we should avoid that breakage.

Yes, that could be an issue, but there are other skb_find_text() usages and to 
me they suggest that the intended behaviour is to stop search at 'to' offset. 
In nf_conntrack_amanda.c, for example:

        start = skb_find_text(skb, dataoff, skb->len,
                              search[SEARCH_CONNECT].ts);
...
        stop = skb_find_text(skb, start, skb->len,
                             search[SEARCH_NEWLINE].ts);
...
        stop += start;
...
                off = skb_find_text(skb, start, stop, search[i].ts);

First of all, nothing can ever match at skb->len, and second, it looks like 
the third usage is also expecting to search from offset 'start' to offset 
'stop', not to 'stop + 1'.

em_text_match() in net/sched/em_text.c is also suspicious.

> I would suggest you fix the --from X --to Y where X == Y which is not
> doing what people would expect.

That would certainly make things consistent, but I'm not sure we want it to be 
consistent this way.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jun 15, 2015 at 10:37:31PM +0300, Roman Khimov wrote:
> В письме от 15 июня 2015 19:06:39 пользователь Pablo Neira Ayuso написал:
> > On Mon, Jun 15, 2015 at 12:11:58PM +0300, Roman I Khimov wrote:
> > > Suppose that we're trying to use an xt_string netfilter module to match a
> > > string in a specially crafted packet that has "a nice string" starting at
> > > offset 28.
> > > 
> > > It could be done in iptables like this:
> > > 
> > > -A some_chain -m string --string "a nice string" --algo bm --from 28 --to
> > > 38 -j DROP
> > > 
> > > And it would work as expected. Now changing that to
> > > 
> > > -A some_chain -m string --string "a nice string" --algo bm --from 29 --to
> > > 38 -j DROP
> > > 
> > > breaks the match, as expected. But, if we try to make
> > > 
> > > -A some_chain -m string --string "a nice string" --algo bm --from 20 --to
> > > 28 -j DROP
> > > 
> > > then it suddenly works again! So the 'to' parameter seems to be inclusive,
> > > not working as an offset after which no search should be done. OK, now if
> > > we try:
> > > 
> > > -A some_chain -m string --string "a nice string" --algo bm --from 28 --to
> > > 28 -j DROP
> > Can you reproduce the same behaviour with the km algo?
> 
> Will try tomorrow MSK time.

Thanks, wait for your feedback on this.

> > > The first behaviour (matching at 'to' offset) comes from skb_find_text()
> > > comparison. The second one (not matching if 'from' and 'to' are equal)
> > > comes from skb_seq_read() check for (abs_offset >= st->upper_offset).
> > > 
> > > I think that the way skb_find_text() handles 'to' is wrong and should be
> > > fixed so that we always have predictable behaviour -- only match before
> > > 'to' offset.
> > > 
> > > There are currently only five usages of skb_find_text() in the kernel and
> > > it looks to me that none of them expect to match something at the 'to'
> > > offset, so probably this change is safe.
> > 
> > So both 'from' and 'to' are inclusive which seems consistent to me. If
> > you make 'to' non-inclusive, then that will change the third example
> > above, right? 
> 
> Yep.
> 
> > That will break existing setups for people that are
> > relying on this behaviour. This has been exposed in this way for long
> > time, so we should avoid that breakage.
> 
> Yes, that could be an issue, but there are other skb_find_text() usages and to 
> me they suggest that the intended behaviour is to stop search at 'to' offset. 
> In nf_conntrack_amanda.c, for example:
> 
>         start = skb_find_text(skb, dataoff, skb->len,
>                               search[SEARCH_CONNECT].ts);
> ...
>         stop = skb_find_text(skb, start, skb->len,
>                              search[SEARCH_NEWLINE].ts);
> ...
>         stop += start;
> ...
>                 off = skb_find_text(skb, start, stop, search[i].ts);
>
> First of all, nothing can ever match at skb->len, and second, it looks like 
> the third usage is also expecting to search from offset 'start' to offset 
> 'stop', not to 'stop + 1'.

Then, please fix the Amanda helper.

Look, Amanda is an in-tree client of this textsearch infrastructure,
so it's not exposed to userspace, we can fix it.

But if we change the existing behaviour, users may be relying on it
and we'll get things broken for them. Someone else will come later one
with another patch to say: "hey, --to used to be inclusive but this is
not the case anymore and it's breaking my setup".

> em_text_match() in net/sched/em_text.c is also suspicious.

Please, elaborate.

> > I would suggest you fix the --from X --to Y where X == Y which is not
> > doing what people would expect.
> 
> That would certainly make things consistent, but I'm not sure we want it to be 
> consistent this way.

We have a contract with users that we can't break, so far the corner
case that doesn't work if --from X --to Y where X == Y. If that
didn't work since the beginning, then we're 100% sure nobody has been
using it, so let's fix it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

В письме от 16 июня 2015 12:48:41 пользователь Pablo Neira Ayuso написал:
> On Mon, Jun 15, 2015 at 10:37:31PM +0300, Roman Khimov wrote:
> > В письме от 15 июня 2015 19:06:39 пользователь Pablo Neira Ayuso написал:
> > > On Mon, Jun 15, 2015 at 12:11:58PM +0300, Roman I Khimov wrote:
> > > > Suppose that we're trying to use an xt_string netfilter module to
> > > > match a
> > > > string in a specially crafted packet that has "a nice string" starting
> > > > at
> > > > offset 28.
> > > > 
> > > > It could be done in iptables like this:
> > > > 
> > > > -A some_chain -m string --string "a nice string" --algo bm --from 28
> > > > --to
> > > > 38 -j DROP
> > > > 
> > > > And it would work as expected. Now changing that to
> > > > 
> > > > -A some_chain -m string --string "a nice string" --algo bm --from 29
> > > > --to
> > > > 38 -j DROP
> > > > 
> > > > breaks the match, as expected. But, if we try to make
> > > > 
> > > > -A some_chain -m string --string "a nice string" --algo bm --from 20
> > > > --to
> > > > 28 -j DROP
> > > > 
> > > > then it suddenly works again! So the 'to' parameter seems to be
> > > > inclusive,
> > > > not working as an offset after which no search should be done. OK, now
> > > > if
> > > > we try:
> > > > 
> > > > -A some_chain -m string --string "a nice string" --algo bm --from 28
> > > > --to
> > > > 28 -j DROP
> > > 
> > > Can you reproduce the same behaviour with the km algo?
> > 
> > Will try tomorrow MSK time.
> 
> Thanks, wait for your feedback on this.

Same behaviour with kmp.

> > > That will break existing setups for people that are
> > > relying on this behaviour. This has been exposed in this way for long
> > > time, so we should avoid that breakage.
> > 
> > Yes, that could be an issue, but there are other skb_find_text() usages
> > and to me they suggest that the intended behaviour is to stop search at
> > 'to' offset.> 
> > In nf_conntrack_amanda.c, for example:
> >         start = skb_find_text(skb, dataoff, skb->len,
> >         
> >                               search[SEARCH_CONNECT].ts);
> > 
> > ...
> > 
> >         stop = skb_find_text(skb, start, skb->len,
> >         
> >                              search[SEARCH_NEWLINE].ts);
> > 
> > ...
> > 
> >         stop += start;
> > 
> > ...
> > 
> >                 off = skb_find_text(skb, start, stop, search[i].ts);
> > 
> > First of all, nothing can ever match at skb->len, and second, it looks
> > like
> > the third usage is also expecting to search from offset 'start' to offset
> > 'stop', not to 'stop + 1'.
> 
> Then, please fix the Amanda helper.
> 
> Look, Amanda is an in-tree client of this textsearch infrastructure,
> so it's not exposed to userspace, we can fix it.
>
> But if we change the existing behaviour, users may be relying on it
> and we'll get things broken for them. Someone else will come later one
> with another patch to say: "hey, --to used to be inclusive but this is
> not the case anymore and it's breaking my setup".

I do understand your concerns, but fixing it this way would require changing 
skb_seq_read() and basicaly would propagate "'to' offset included" semantics 
(which seems a bit strange for programmers, IMO) further. And initially I 
thought that changing skb_seq_read() would be more intrusive, although looking 
at all this now it looks like the only real user of upper_offset field in 
ts_config struct is skb_find_text(), because other invocations of 
skb_seq_read() from drivers/scsi/libiscsi_tcp.c and net/batman-adv/main.c use 
skb->len as an upper limit.

> > em_text_match() in net/sched/em_text.c is also suspicious.
> 
> Please, elaborate.

The way it constructs 'to' offset, I think it doesn't expect something to 
match at 'to'. Although I might be wrong here.

From: Roman I Khimov <khimov@altell.ru>
Date: Mon, 15 Jun 2015 12:11:58 +0300

> Suppose that we're trying to use an xt_string netfilter module to match a
> string in a specially crafted packet that has "a nice string" starting at
> offset 28.
> 
> It could be done in iptables like this:
> 
> -A some_chain -m string --string "a nice string" --algo bm --from 28 --to 38 -j DROP
> 
> And it would work as expected. Now changing that to
> 
> -A some_chain -m string --string "a nice string" --algo bm --from 29 --to 38 -j DROP
> 
> breaks the match, as expected. But, if we try to make
> 
> -A some_chain -m string --string "a nice string" --algo bm --from 20 --to 28 -j DROP
> 
> then it suddenly works again! So the 'to' parameter seems to be inclusive, not
> working as an offset after which no search should be done. OK, now if we try:
> 
> -A some_chain -m string --string "a nice string" --algo bm --from 28 --to 28 -j DROP
> 
> it doesn't work. So, for the case of equal 'from' and 'to' it's treated in a
> different way.
> 
> The first behaviour (matching at 'to' offset) comes from skb_find_text()
> comparison. The second one (not matching if 'from' and 'to' are equal) comes
> from skb_seq_read() check for (abs_offset >= st->upper_offset).
> 
> I think that the way skb_find_text() handles 'to' is wrong and should be fixed
> so that we always have predictable behaviour -- only match before 'to' offset.
> 
> There are currently only five usages of skb_find_text() in the kernel and it
> looks to me that none of them expect to match something at the 'to' offset,
> so probably this change is safe.
> 
> Reported-by: Edward Makarov <makarov@altell.ru>
> Tested-by: Edward Makarov <makarov@altell.ru>
> Signed-off-by: Roman I Khimov <khimov@altell.ru>

Unfortunately any aspect of this exposed to userspace is pretty much locked
in place, and we can't change it without potentially breaking someone's
setup.  This has been this way for a long time, so the risk of breaking
things is very real.

I'm not applying this, sorry.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Jun 16, 2015 at 03:13:41PM +0300, Roman Khimov wrote:
> В письме от 16 июня 2015 12:48:41 пользователь Pablo Neira Ayuso написал:
[...]
> > But if we change the existing behaviour, users may be relying on it
> > and we'll get things broken for them. Someone else will come later one
> > with another patch to say: "hey, --to used to be inclusive but this is
> > not the case anymore and it's breaking my setup".
> 
> I do understand your concerns, but fixing it this way would require changing 
> skb_seq_read() and basicaly would propagate "'to' offset included" semantics 
> (which seems a bit strange for programmers, IMO) further. And initially I 
> thought that changing skb_seq_read() would be more intrusive, although looking 
> at all this now it looks like the only real user of upper_offset field in 
> ts_config struct is skb_find_text(), because other invocations of 
> skb_seq_read() from drivers/scsi/libiscsi_tcp.c and net/batman-adv/main.c use 
> skb->len as an upper limit.
> 
> > > em_text_match() in net/sched/em_text.c is also suspicious.
> > 
> > Please, elaborate.
> 
> The way it constructs 'to' offset, I think it doesn't expect something to 
> match at 'to'. Although I might be wrong here.

Could you send a patch that resolves the inconsistency for programmers
while leaving the userspace exposed behaviour through xt_string and
em_string intact? Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html