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

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

[Pablo Neira Ayuso]

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
@@ -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;
-- 
1.7.10.4

[PATCH 2/2] netfilter: xt_TCPOPTSTRIP: fix off by one access

[Pablo Neira Ayuso]

Fix a possible off by one access since optlen()
touches opt[offset+1] unsafely when i == tcp_hdrlen(skb) - 1.

This patch replaces tcp_hdrlen() by the local variable tcp_hdrlen
that stores the TCP header length, to save some cycles.

Reported-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_TCPOPTSTRIP.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/xt_TCPOPTSTRIP.c b/net/netfilter/xt_TCPOPTSTRIP.c
index b68fa19..68c5385 100644
--- a/net/netfilter/xt_TCPOPTSTRIP.c
+++ b/net/netfilter/xt_TCPOPTSTRIP.c
@@ -38,7 +38,7 @@ tcpoptstrip_mangle_packet(struct sk_buff *skb,
 	struct tcphdr *tcph;
 	u_int16_t n, o;
 	u_int8_t *opt;
-	int len;
+	int len, tcp_hdrlen;
 
 	/* This is a fragment, no TCP header is available */
 	if (par->fragoff != 0)
@@ -52,7 +52,9 @@ tcpoptstrip_mangle_packet(struct sk_buff *skb,
 		return NF_DROP;
 
 	tcph = (struct tcphdr *)(skb_network_header(skb) + tcphoff);
-	if (tcph->doff * 4 > len)
+	tcp_hdrlen = tcph->doff * 4;
+
+	if (len < tcp_hdrlen)
 		return NF_DROP;
 
 	opt  = (u_int8_t *)tcph;
@@ -61,10 +63,14 @@ tcpoptstrip_mangle_packet(struct sk_buff *skb,
 	 * Walk through all TCP options - if we find some option to remove,
 	 * set all octets to %TCPOPT_NOP and adjust checksum.
 	 */
-	for (i = sizeof(struct tcphdr); i < tcp_hdrlen(skb); i += optl) {
+	for (i = sizeof(struct tcphdr); i < tcp_hdrlen; i += optl) {
+		/* Avoid possible off by one access in optlen */
+		if (i + 1 == tcp_hdrlen)
+			break;
+
 		optl = optlen(opt, i);
 
-		if (i + optl > tcp_hdrlen(skb))
+		if (i + optl > tcp_hdrlen)
 			break;
 
 		if (!tcpoptstrip_test_bit(info->strip_bmap, opt[i]))
-- 
1.7.10.4

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

[Julian Anastasov]

	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>

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

[Pablo Neira Ayuso]

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.