[PATCH] netfilter: xt_TCPOPTSTRIP: fix possible mangling beyond packet boundary

[PATCH] netfilter: xt_TCPOPTSTRIP: fix possible mangling beyond packet boundary

[Pablo Neira Ayuso]

This target assumes that tcph->doff is well-formed, that may be well
not the case. Add extra sanity checkings to avoid possible crash due
to read/write out of the real packet boundary.

Reported-by: Rafal Kupka <rkupka@telemetry.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_TCPOPTSTRIP.c |   24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/xt_TCPOPTSTRIP.c b/net/netfilter/xt_TCPOPTSTRIP.c
index 25fd1c4..3cdd56f 100644
--- a/net/netfilter/xt_TCPOPTSTRIP.c
+++ b/net/netfilter/xt_TCPOPTSTRIP.c
@@ -30,17 +30,33 @@ static inline unsigned int optlen(const u_int8_t *opt, unsigned int offset)
 
 static unsigned int
 tcpoptstrip_mangle_packet(struct sk_buff *skb,
-			  const struct xt_tcpoptstrip_target_info *info,
+			  const struct xt_action_param *par,
 			  unsigned int tcphoff, unsigned int minlen)
 {
+	const struct xt_tcpoptstrip_target_info *info = par->targinfo;
 	unsigned int optl, i, j;
 	struct tcphdr *tcph;
 	u_int16_t n, o;
 	u_int8_t *opt;
+	int len;
+
+	/* This is a fragment, no TCP header is available */
+	if (par->fragoff != 0)
+		return XT_CONTINUE;
 
 	if (!skb_make_writable(skb, skb->len))
 		return NF_DROP;
 
+	len = skb->len - tcphoff;
+	if (len < sizeof(struct tcphdr))
+		return NF_DROP;
+
+	len -= sizeof(*tcph);
+
+	/* Truncated TCP options, skip */
+	if ((tcp_hdr(skb)->doff - 5) * 4 > len)
+		return NF_DROP;
+
 	tcph = (struct tcphdr *)(skb_network_header(skb) + tcphoff);
 	opt  = (u_int8_t *)tcph;
 
@@ -51,7 +67,7 @@ tcpoptstrip_mangle_packet(struct sk_buff *skb,
 	for (i = sizeof(struct tcphdr); i < tcp_hdrlen(skb); i += optl) {
 		optl = optlen(opt, i);
 
-		if (i + optl > tcp_hdrlen(skb))
+		if (i + optl > len)
 			break;
 
 		if (!tcpoptstrip_test_bit(info->strip_bmap, opt[i]))
@@ -76,7 +92,7 @@ tcpoptstrip_mangle_packet(struct sk_buff *skb,
 static unsigned int
 tcpoptstrip_tg4(struct sk_buff *skb, const struct xt_action_param *par)
 {
-	return tcpoptstrip_mangle_packet(skb, par->targinfo, ip_hdrlen(skb),
+	return tcpoptstrip_mangle_packet(skb, par, ip_hdrlen(skb),
 	       sizeof(struct iphdr) + sizeof(struct tcphdr));
 }
 
@@ -94,7 +110,7 @@ tcpoptstrip_tg6(struct sk_buff *skb, const struct xt_action_param *par)
 	if (tcphoff < 0)
 		return NF_DROP;
 
-	return tcpoptstrip_mangle_packet(skb, par->targinfo, tcphoff,
+	return tcpoptstrip_mangle_packet(skb, par, tcphoff,
 	       sizeof(*ipv6h) + sizeof(struct tcphdr));
 }
 #endif
-- 
1.7.10.4

Re: [PATCH] netfilter: xt_TCPOPTSTRIP: fix possible mangling beyond packet boundary

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> This target assumes that tcph->doff is well-formed, that may be well
> not the case. Add extra sanity checkings to avoid possible crash due
> to read/write out of the real packet boundary.

>  static unsigned int
>  tcpoptstrip_mangle_packet(struct sk_buff *skb,
> -			  const struct xt_tcpoptstrip_target_info *info,
> +			  const struct xt_action_param *par,
>  			  unsigned int tcphoff, unsigned int minlen)
>  {
> +	const struct xt_tcpoptstrip_target_info *info = par->targinfo;
>  	unsigned int optl, i, j;
>  	struct tcphdr *tcph;
>  	u_int16_t n, o;
>  	u_int8_t *opt;
> +	int len;
> +
[..]
> +	len = skb->len - tcphoff;
> +	if (len < sizeof(struct tcphdr))

I think this needs a cast 'if (len < (int) sizeof( ...?

> @@ -51,7 +67,7 @@ tcpoptstrip_mangle_packet(struct sk_buff *skb,
>  	for (i = sizeof(struct tcphdr); i < tcp_hdrlen(skb); i += optl) {
>  		optl = optlen(opt, i);
>  
> -		if (i + optl > tcp_hdrlen(skb))
> +		if (i + optl > len)
>  			break;

I don't understand this change.  This should stop parsing if the
option length points outside of the tcp option size.

But doesn't len include the size of the actual payload?

Re: [PATCH] netfilter: xt_TCPOPTSTRIP: fix possible mangling beyond packet boundary

[Pablo Neira Ayuso]

On Wed, May 15, 2013 at 02:33:36PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > This target assumes that tcph->doff is well-formed, that may be well
> > not the case. Add extra sanity checkings to avoid possible crash due
> > to read/write out of the real packet boundary.
> 
> >  static unsigned int
> >  tcpoptstrip_mangle_packet(struct sk_buff *skb,
> > -			  const struct xt_tcpoptstrip_target_info *info,
> > +			  const struct xt_action_param *par,
> >  			  unsigned int tcphoff, unsigned int minlen)
> >  {
> > +	const struct xt_tcpoptstrip_target_info *info = par->targinfo;
> >  	unsigned int optl, i, j;
> >  	struct tcphdr *tcph;
> >  	u_int16_t n, o;
> >  	u_int8_t *opt;
> > +	int len;
> > +
> [..]
> > +	len = skb->len - tcphoff;
> > +	if (len < sizeof(struct tcphdr))
> 
> I think this needs a cast 'if (len < (int) sizeof( ...?

I'm not hitting any compilation warning.

> > @@ -51,7 +67,7 @@ tcpoptstrip_mangle_packet(struct sk_buff *skb,
> >  	for (i = sizeof(struct tcphdr); i < tcp_hdrlen(skb); i += optl) {
> >  		optl = optlen(opt, i);
> >  
> > -		if (i + optl > tcp_hdrlen(skb))
> > +		if (i + optl > len)
> >  			break;
> 
> I don't understand this change.  This should stop parsing if the
> option length points outside of the tcp option size.
> 
> But doesn't len include the size of the actual payload?

We already validated a bit above that there's room for the TCP
options in the packet.

        /* Truncated TCP options, skip */
        if ((tcp_hdr(skb)->doff - 5) * 4 > len - sizeof(struct tcphdr))
                return NF_DROP;

So after that point we can trust the tcph->doff field.

Therefore, you're right, I'll remove that change. Thanks.

Re: [PATCH] netfilter: xt_TCPOPTSTRIP: fix possible mangling beyond packet boundary

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, May 15, 2013 at 02:33:36PM +0200, Florian Westphal wrote:
> > > +	const struct xt_tcpoptstrip_target_info *info = par->targinfo;
> > >  	unsigned int optl, i, j;
> > >  	struct tcphdr *tcph;
> > >  	u_int16_t n, o;
> > >  	u_int8_t *opt;
> > > +	int len;
> > > +
> > [..]
> > > +	len = skb->len - tcphoff;
> > > +	if (len < sizeof(struct tcphdr))
> > 
> > I think this needs a cast 'if (len < (int) sizeof( ...?
> 
> I'm not hitting any compilation warning.

len is signed, thats why i asked, and, afair recall sizeof
is unsigned and thus len is treated as unsigned.

Test program yields:

#include <stdio.h>
#include <stdlib.h>
int main (int argc, char *argv[])
{
        char foo[20];
        int len = atoi(argc > 1 ? argv[1] : "0");
        if (len < sizeof(foo))
                printf("%d lt %lu\n", len, sizeof(foo));
        else
                printf("%d ge %lu\n", len, sizeof(foo));
        return 0;
}
t.c: In function 'main':
t.c:7:10: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
./t -42
-42 ge 20

gcc version 4.6.3.

Re: [PATCH] netfilter: xt_TCPOPTSTRIP: fix possible mangling beyond packet boundary

[Pablo Neira Ayuso]

On Wed, May 15, 2013 at 04:48:24PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Wed, May 15, 2013 at 02:33:36PM +0200, Florian Westphal wrote:
> > > > +	const struct xt_tcpoptstrip_target_info *info = par->targinfo;
> > > >  	unsigned int optl, i, j;
> > > >  	struct tcphdr *tcph;
> > > >  	u_int16_t n, o;
> > > >  	u_int8_t *opt;
> > > > +	int len;
> > > > +
> > > [..]
> > > > +	len = skb->len - tcphoff;
> > > > +	if (len < sizeof(struct tcphdr))
> > > 
> > > I think this needs a cast 'if (len < (int) sizeof( ...?
> > 
> > I'm not hitting any compilation warning.
> 
> len is signed, thats why i asked, and, afair recall sizeof
> is unsigned and thus len is treated as unsigned.

Right. I'll fix it, thanks.