Re: [PATCH 1/2] netfilter: xt_TCPMSS: fix handling of malformed TCP header

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Julian,

On Wed, Jul 31, 2013 at 10:54:16PM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Wed, 31 Jul 2013, Pablo Neira Ayuso wrote:
> 
> > Make sure the packet has enough room for the TCP header and
> > that it is not malformed.
> > 
> > While at it, store tcph->doff*4 in a variable, as it is used
> > several times.
> > 
> > Reported-by: Julian Anastasov <ja@ssi.bg>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  net/netfilter/xt_TCPMSS.c |   27 ++++++++++++++++-----------
> >  1 file changed, 16 insertions(+), 11 deletions(-)
> > 
> > diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
> > index 7011c71..2883c1c 100644
> > --- a/net/netfilter/xt_TCPMSS.c
> > +++ b/net/netfilter/xt_TCPMSS.c
> 
> > @@ -87,8 +91,8 @@ tcpmss_mangle_packet(struct sk_buff *skb,
> >  		newmss = info->mss;
> >  
> >  	opt = (u_int8_t *)tcph;
> > -	for (i = sizeof(struct tcphdr); i < tcph->doff*4; i += optlen(opt, i)) {
> > -		if (opt[i] == TCPOPT_MSS && tcph->doff*4 - i >= TCPOLEN_MSS &&
> > +	for (i = sizeof(struct tcphdr); i < tcp_hdrlen; i += optlen(opt, i)) {
> 
> 	If we also want to avoid the wrong access in optlen()
> we have 2 options for the above line:
> 
> 1. Use 'i < tcp_hdrlen - 1' or 'i <= tcp_hdrlen - 2'
> 
> 2. Use 'i <= tcp_hdrlen - TCPOLEN_MSS' and remove the
> below 'tcp_hdrlen - i >= TCPOLEN_MSS' check

Indeed. I fixed the optlen issue in xt_TCPOPTSTRIP but forgot to make
it here.

Will send a new version, thanks for reviewing.