В письме от 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.
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
