| Message ID | 1375279852-4059-1-git-send-email-pablo@netfilter.org |
|---|---|
| State | Superseded |
| Headers | show |
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 > + if (opt[i] == TCPOPT_MSS && tcp_hdrlen - i >= TCPOLEN_MSS && > opt[i+1] == TCPOLEN_MSS) { > u_int16_t oldmss; Regards -- Julian Anastasov <ja@ssi.bg> -- 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
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. -- 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
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 @@ -52,7 +52,8 @@ tcpmss_mangle_packet(struct sk_buff *skb, { const struct xt_tcpmss_info *info = par->targinfo; struct tcphdr *tcph; - unsigned int tcplen, i; + int len, tcp_hdrlen; + unsigned int i; __be16 oldval; u16 newmss; u8 *opt; @@ -64,11 +65,14 @@ tcpmss_mangle_packet(struct sk_buff *skb, if (!skb_make_writable(skb, skb->len)) return -1; - tcplen = skb->len - tcphoff; + len = skb->len - tcphoff; + if (len < (int)sizeof(struct tcphdr)) + return -1; + tcph = (struct tcphdr *)(skb_network_header(skb) + tcphoff); + tcp_hdrlen = tcph->doff * 4; - /* Header cannot be larger than the packet */ - if (tcplen < tcph->doff*4) + if (len < tcp_hdrlen) return -1; if (info->mss == XT_TCPMSS_CLAMP_PMTU) { @@ -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 (opt[i] == TCPOPT_MSS && tcp_hdrlen - i >= TCPOLEN_MSS && opt[i+1] == TCPOLEN_MSS) { u_int16_t oldmss; @@ -112,9 +116,10 @@ tcpmss_mangle_packet(struct sk_buff *skb, } /* There is data after the header so the option can't be added - without moving it, and doing so may make the SYN packet - itself too large. Accept the packet unmodified instead. */ - if (tcplen > tcph->doff*4) + * without moving it, and doing so may make the SYN packet + * itself too large. Accept the packet unmodified instead. + */ + if (len > tcp_hdrlen) return 0; /* @@ -143,10 +148,10 @@ tcpmss_mangle_packet(struct sk_buff *skb, newmss = min(newmss, (u16)1220); opt = (u_int8_t *)tcph + sizeof(struct tcphdr); - memmove(opt + TCPOLEN_MSS, opt, tcplen - sizeof(struct tcphdr)); + memmove(opt + TCPOLEN_MSS, opt, len - sizeof(struct tcphdr)); inet_proto_csum_replace2(&tcph->check, skb, - htons(tcplen), htons(tcplen + TCPOLEN_MSS), 1); + htons(len), htons(len + TCPOLEN_MSS), 1); opt[0] = TCPOPT_MSS; opt[1] = TCPOLEN_MSS; opt[2] = (newmss & 0xff00) >> 8;
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(-)