* [PATCH v2 1/2] netfilter: xt_TCPMSS: fix handling of malformed TCP header and options
@ 2013-08-05 15:21 Pablo Neira Ayuso
2013-08-05 15:21 ` [PATCH v2 2/2] netfilter: xt_TCPOPTSTRIP: fix possible off by one access Pablo Neira Ayuso
2013-08-09 7:11 ` [PATCH v2 1/2] netfilter: xt_TCPMSS: fix handling of malformed TCP header and options Julian Anastasov
0 siblings, 2 replies; 3+ messages in thread
From: Pablo Neira Ayuso @ 2013-08-05 15:21 UTC (permalink / raw)
To: netfilter-devel; +Cc: Julian Anastasov
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.
This patch also fixes a possible off by one in case of malformed
TCP options.
Reported-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: Use i <= tcp_hdrlen - TCPOLEN_MSS to avoid walking out of the
packet boundary, as suggested by Julian.
net/netfilter/xt_TCPMSS.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
index 7011c71..6113cc7 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,9 +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 &&
- opt[i+1] == TCPOLEN_MSS) {
+ for (i = sizeof(struct tcphdr); i <= tcp_hdrlen - TCPOLEN_MSS; i += optlen(opt, i)) {
+ if (opt[i] == TCPOPT_MSS && opt[i+1] == TCPOLEN_MSS) {
u_int16_t oldmss;
oldmss = (opt[i+2] << 8) | opt[i+3];
@@ -112,9 +115,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 +147,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
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH v2 2/2] netfilter: xt_TCPOPTSTRIP: fix possible off by one access
2013-08-05 15:21 [PATCH v2 1/2] netfilter: xt_TCPMSS: fix handling of malformed TCP header and options Pablo Neira Ayuso
@ 2013-08-05 15:21 ` Pablo Neira Ayuso
2013-08-09 7:11 ` [PATCH v2 1/2] netfilter: xt_TCPMSS: fix handling of malformed TCP header and options Julian Anastasov
1 sibling, 0 replies; 3+ messages in thread
From: Pablo Neira Ayuso @ 2013-08-05 15:21 UTC (permalink / raw)
To: netfilter-devel; +Cc: Julian Anastasov
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>
---
v2: Avoid possible off by one access from the for loop condition,
based on idea from Julian.
net/netfilter/xt_TCPOPTSTRIP.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/netfilter/xt_TCPOPTSTRIP.c b/net/netfilter/xt_TCPOPTSTRIP.c
index b68fa19..625fa1d 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,10 @@ 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 - 1; i += optl) {
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2 1/2] netfilter: xt_TCPMSS: fix handling of malformed TCP header and options
2013-08-05 15:21 [PATCH v2 1/2] netfilter: xt_TCPMSS: fix handling of malformed TCP header and options Pablo Neira Ayuso
2013-08-05 15:21 ` [PATCH v2 2/2] netfilter: xt_TCPOPTSTRIP: fix possible off by one access Pablo Neira Ayuso
@ 2013-08-09 7:11 ` Julian Anastasov
1 sibling, 0 replies; 3+ messages in thread
From: Julian Anastasov @ 2013-08-09 7:11 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
Hello,
On Mon, 5 Aug 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.
>
> This patch also fixes a possible off by one in case of malformed
> TCP options.
>
> Reported-by: Julian Anastasov <ja@ssi.bg>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Both patches look good to me,
Reviewed-by: Julian Anastasov <ja@ssi.bg>
> ---
> v2: Use i <= tcp_hdrlen - TCPOLEN_MSS to avoid walking out of the
> packet boundary, as suggested by Julian.
>
> net/netfilter/xt_TCPMSS.c | 28 ++++++++++++++++------------
> 1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
> index 7011c71..6113cc7 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,9 +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 &&
> - opt[i+1] == TCPOLEN_MSS) {
> + for (i = sizeof(struct tcphdr); i <= tcp_hdrlen - TCPOLEN_MSS; i += optlen(opt, i)) {
> + if (opt[i] == TCPOPT_MSS && opt[i+1] == TCPOLEN_MSS) {
> u_int16_t oldmss;
>
> oldmss = (opt[i+2] << 8) | opt[i+3];
> @@ -112,9 +115,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 +147,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
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-08-09 7:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-05 15:21 [PATCH v2 1/2] netfilter: xt_TCPMSS: fix handling of malformed TCP header and options Pablo Neira Ayuso
2013-08-05 15:21 ` [PATCH v2 2/2] netfilter: xt_TCPOPTSTRIP: fix possible off by one access Pablo Neira Ayuso
2013-08-09 7:11 ` [PATCH v2 1/2] netfilter: xt_TCPMSS: fix handling of malformed TCP header and options Julian Anastasov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.