| Message ID | 1368619552-30635-1-git-send-email-pablo@netfilter.org |
|---|---|
| State | Superseded |
| Headers | show |
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? -- 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
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. -- 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 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. -- 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
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. -- 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_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
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(-)