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

login
register
mail settings
Submitter Pablo Neira
Date July 31, 2013, 2:10 p.m.
Message ID <1375279852-4059-1-git-send-email-pablo@netfilter.org>
Download mbox | patch
Permalink /patch/263714/
State Superseded
Headers show

Comments

Pablo Neira - July 31, 2013, 2:10 p.m.
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(-)
Julian Anastasov - July 31, 2013, 7:54 p.m.
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
Pablo Neira - Aug. 1, 2013, 12:39 a.m.
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

Patch

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;