xt_TCPMSS target dropping SYN packets with data: suggested mod

xt_TCPMSS target dropping SYN packets with data: suggested mod

[Luca Pesce]

Hi all,
   I have a question and a possible patch/mod for target TCPMSS (xt_TCPMSS.c).
At the very beginning of function tcpmss_mangle_packet(), the skb containing the
TCP SYN packet is checked to see if it is containing data (on a side note, SYN
with data is quite unusual...); if so, the packet is drastically dropped.
The reason is explained in RR's comment to the code, I am copy/pasting the
beginning of this function with the length check at the bottom of this mail.
RR says that we cannot change MSS on a packet which is already carrying data
(it would be too late): could we relax this check, seeing if the tcp payload is
less than the MSS we are about to set?
Something like:

if (tcplen > info->mss) {
      if (net_ratelimit())
         printk(KERN_ERR "xt_TCPMSS: bad length (%u bytes)\n",
                (*pskb)->len);
      return -1;
}

With such a mod, MSS clamping would happen even for TCP SYN with data
if the data
length (instead of dropping the packet) is not exceeding the MSS we want to set.
Thanks and regards,
Luca Pesce



static int
tcpmss_mangle_packet(struct sk_buff **pskb,
           const struct xt_tcpmss_info *info,
           unsigned int tcphoff,
           unsigned int minlen)
{
   struct tcphdr *tcph;
   unsigned int tcplen, i;
   __be16 oldval;
   u16 newmss;
   u8 *opt;

   if (!skb_make_writable(pskb, (*pskb)->len))
      return -1;

   tcplen = (*pskb)->len - tcphoff;
   tcph = (struct tcphdr *)((*pskb)->nh.raw + tcphoff);

   /* Since it passed flags test in tcp match, we know it is is
      not a fragment, and has data >= tcp header length.  SYN
      packets should not contain data: if they did, then we risk
      running over MTU, sending Frag Needed and breaking things
      badly. --RR */
   if (tcplen != tcph->doff*4) {
      if (net_ratelimit())
         printk(KERN_ERR "xt_TCPMSS: bad length (%u bytes)\n",
                (*pskb)->len);
      return -1;
   }

    [...]

Re: xt_TCPMSS target dropping SYN packets with data: suggested mod

[Patrick McHardy]

Luca Pesce wrote:
> Hi all,
>    I have a question and a possible patch/mod for target TCPMSS (xt_TCPMSS.c).
> At the very beginning of function tcpmss_mangle_packet(), the skb containing the
> TCP SYN packet is checked to see if it is containing data (on a side note, SYN
> with data is quite unusual...); if so, the packet is drastically dropped.
> The reason is explained in RR's comment to the code, I am copy/pasting the
> beginning of this function with the length check at the bottom of this mail.
> RR says that we cannot change MSS on a packet which is already carrying data
> (it would be too late): could we relax this check, seeing if the tcp payload is
> less than the MSS we are about to set?

We probably could change that. I'm wondering though, did you actually
see this in real life? It doesn't seem like a very useful feature,
considering all the stacks supporting syn cookies.

Re: xt_TCPMSS target dropping SYN packets with data: suggested mod

[Luca Pesce]

Hi Patrick,
    thanks for your answer.
I completely agree with you about the actual usefulness of such a mod:
in real life SYNs with payload are almost not present, unfortunately I
found one of those cases and I was thinking about this mod.

Apart from the real life scenarios, I do not understand your sentence:

> It doesn't seem like a very useful feature,
> considering all the stacks supporting syn cookies.

Did you mean that syn cookies would drop that SYN with data anyway? Or
that MSS mangling itself would harm syn cookies tecniques?
Thanks for your clarifications.

Luca


On Wed, Jul 15, 2009 at 5:38 PM, Patrick McHardy <kaber@trash.net> wrote:
>
> Luca Pesce wrote:
> > Hi all,
> >    I have a question and a possible patch/mod for target TCPMSS (xt_TCPMSS.c).
> > At the very beginning of function tcpmss_mangle_packet(), the skb containing the
> > TCP SYN packet is checked to see if it is containing data (on a side note, SYN
> > with data is quite unusual...); if so, the packet is drastically dropped.
> > The reason is explained in RR's comment to the code, I am copy/pasting the
> > beginning of this function with the length check at the bottom of this mail.
> > RR says that we cannot change MSS on a packet which is already carrying data
> > (it would be too late): could we relax this check, seeing if the tcp payload is
> > less than the MSS we are about to set?
>
> We probably could change that. I'm wondering though, did you actually
> see this in real life? It doesn't seem like a very useful feature,
> considering all the stacks supporting syn cookies.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: xt_TCPMSS target dropping SYN packets with data: suggested mod

[Pascal Hambourg]

Hello,

Luca Pesce a écrit :
> 
>> It doesn't seem like a very useful feature,
>> considering all the stacks supporting syn cookies.
> 
> Did you mean that syn cookies would drop that SYN with data anyway?

When the receiver has SYN cookies enabled, it replies to a SYN with a 
SYN-ACK as usual, but does not keep any state for it. Instead the state 
is stored in the ISN (used as a "cookie") of the SYN-ACK and will come 
back in the final ACK of the 3-way handshake. Not keeping state means 
that any data contained in the first SYN segment are discarded.

> Or that MSS mangling itself would harm syn cookies tecniques?

I don't see why.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: xt_TCPMSS target dropping SYN packets with data: suggested mod

[Luca Pesce]

Hi,

Pascal Hambourg<pascal.mail@plouf.fr.eu.org> wrote:
> When the receiver has SYN cookies enabled, it replies to a SYN with a
> SYN-ACK as usual, but does not keep any state for it. Instead the state is
> stored in the ISN (used as a "cookie") of the SYN-ACK and will come back in
> the final ACK of the 3-way handshake. Not keeping state means that any data
> contained in the first SYN segment are discarded.

thanks for your explanation!
Ok, so if the receiver is using syn cookies, the data in the SYN would
be discarded,
and that is fine. But the current implementation of TCPMSS target is
dropping the
whole syn packet (if it is carrying any payload), so the receiver is
not receiving
the syn - in that case, the TCP connection could not be established.

Again, I know that this scenario is very rare and awkward, I wan only thinking
about relaxing that check in the TCPMSS target to let this SYN with data go
through and establish the TCP connection, without caring too much
about the payload.

Re: xt_TCPMSS target dropping SYN packets with data: suggested mod

[Pascal Hambourg]

Luca Pesce a écrit :
> 
> Ok, so if the receiver is using syn cookies, the data in the SYN would
> be discarded, and that is fine.

Actually I don't know whether the data are discared or the whole SYN 
packet is. My feeling is that the receiver should not ACK a discarded 
segment, so the whole SYN packet should be discarded, maybe rejected 
with a RST.

> But the current implementation of TCPMSS target is dropping the
> whole syn packet (if it is carrying any payload), so the receiver is
> not receiving the syn

I think this behaviour is wrong. As a general rule, I think that 
matches, targets or conntrack should not drop packets implicitly. If a 
target cannot handle the packet, just leave it unmodified (and possibly 
log a warning).
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html