| Message ID | 20210609142212.3096691-2-maximmi@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix out of bounds when parsing TCP options | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: netfilter-devel@vger.kernel.org coreteam@netfilter.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
Maxim Mikityanskiy <maximmi@nvidia.com> wrote: > The TCP option parser in synproxy (synproxy_parse_options) could read > one byte out of bounds. When the length is 1, the execution flow gets > into the loop, reads one byte of the opcode, and if the opcode is > neither TCPOPT_EOL nor TCPOPT_NOP, it reads one more byte, which exceeds > the length of 1. > > This fix is inspired by commit 9609dad263f8 ("ipv4: tcp_input: fix stack > out of bounds when parsing TCP options."). > > Cc: Young Xiao <92siuyang@gmail.com> > Fixes: 48b1de4c110a ("netfilter: add SYNPROXY core/target") > Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com> > --- > net/netfilter/nf_synproxy_core.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c > index b100c04a0e43..621eb5ef9727 100644 > --- a/net/netfilter/nf_synproxy_core.c > +++ b/net/netfilter/nf_synproxy_core.c > @@ -47,6 +47,8 @@ synproxy_parse_options(const struct sk_buff *skb, unsigned int doff, > length--; > continue; > default: > + if (length < 2) > + return true; Would you mind a v2 that also rejects bogus th->doff value when computing the length? Thanks.
On 2021-06-09 17:51, Florian Westphal wrote: > Maxim Mikityanskiy <maximmi@nvidia.com> wrote: >> The TCP option parser in synproxy (synproxy_parse_options) could read >> one byte out of bounds. When the length is 1, the execution flow gets >> into the loop, reads one byte of the opcode, and if the opcode is >> neither TCPOPT_EOL nor TCPOPT_NOP, it reads one more byte, which exceeds >> the length of 1. >> >> This fix is inspired by commit 9609dad263f8 ("ipv4: tcp_input: fix stack >> out of bounds when parsing TCP options."). >> >> Cc: Young Xiao <92siuyang@gmail.com> >> Fixes: 48b1de4c110a ("netfilter: add SYNPROXY core/target") >> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com> >> --- >> net/netfilter/nf_synproxy_core.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c >> index b100c04a0e43..621eb5ef9727 100644 >> --- a/net/netfilter/nf_synproxy_core.c >> +++ b/net/netfilter/nf_synproxy_core.c >> @@ -47,6 +47,8 @@ synproxy_parse_options(const struct sk_buff *skb, unsigned int doff, >> length--; >> continue; >> default: >> + if (length < 2) >> + return true; > > Would you mind a v2 that also rejects bogus th->doff value when > computing the length? Could you elaborate? The length is a signed int calculated as `(th->doff * 4) - sizeof(*th)`. Invalid doff values (0..4) lead to negative length, so we never enter the loop. Or are you concerned of passing a negative length to skb_header_pointer? > > Thanks. >
Maxim Mikityanskiy <maximmi@nvidia.com> wrote: > On 2021-06-09 17:51, Florian Westphal wrote: > > Maxim Mikityanskiy <maximmi@nvidia.com> wrote: > > > The TCP option parser in synproxy (synproxy_parse_options) could read > > > one byte out of bounds. When the length is 1, the execution flow gets > > > into the loop, reads one byte of the opcode, and if the opcode is > > > neither TCPOPT_EOL nor TCPOPT_NOP, it reads one more byte, which exceeds > > > the length of 1. > > > > > > This fix is inspired by commit 9609dad263f8 ("ipv4: tcp_input: fix stack > > > out of bounds when parsing TCP options."). > > > > > > Cc: Young Xiao <92siuyang@gmail.com> > > > Fixes: 48b1de4c110a ("netfilter: add SYNPROXY core/target") > > > Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com> > > > --- > > > net/netfilter/nf_synproxy_core.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c > > > index b100c04a0e43..621eb5ef9727 100644 > > > --- a/net/netfilter/nf_synproxy_core.c > > > +++ b/net/netfilter/nf_synproxy_core.c > > > @@ -47,6 +47,8 @@ synproxy_parse_options(const struct sk_buff *skb, unsigned int doff, > > > length--; > > > continue; > > > default: > > > + if (length < 2) > > > + return true; > > > > Would you mind a v2 that also rejects bogus th->doff value when > > computing the length? > > Could you elaborate? The length is a signed int calculated as `(th->doff * > 4) - sizeof(*th)`. Invalid doff values (0..4) lead to negative length, so we > never enter the loop. Or are you concerned of passing a negative length to > skb_header_pointer? Yes, negative length to skb_header_pointer. For other usage (mptcp for example) tcp stack validated th->doff already, but thats not the case for synproxy.
diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c index b100c04a0e43..621eb5ef9727 100644 --- a/net/netfilter/nf_synproxy_core.c +++ b/net/netfilter/nf_synproxy_core.c @@ -47,6 +47,8 @@ synproxy_parse_options(const struct sk_buff *skb, unsigned int doff, length--; continue; default: + if (length < 2) + return true; opsize = *ptr++; if (opsize < 2) return true;
The TCP option parser in synproxy (synproxy_parse_options) could read one byte out of bounds. When the length is 1, the execution flow gets into the loop, reads one byte of the opcode, and if the opcode is neither TCPOPT_EOL nor TCPOPT_NOP, it reads one more byte, which exceeds the length of 1. This fix is inspired by commit 9609dad263f8 ("ipv4: tcp_input: fix stack out of bounds when parsing TCP options."). Cc: Young Xiao <92siuyang@gmail.com> Fixes: 48b1de4c110a ("netfilter: add SYNPROXY core/target") Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com> --- net/netfilter/nf_synproxy_core.c | 2 ++ 1 file changed, 2 insertions(+)