Re: [PATCH net 1/3] netfilter: synproxy: Fix out of bounds when parsing TCP options

Maxim Mikityanskiy [off-list ref] wrote:

On 2021-06-09 17:51, Florian Westphal wrote:

quoted

Maxim Mikityanskiy [off-list ref] wrote:

quoted

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 <redacted>
Fixes: 48b1de4c110a ("netfilter: add SYNPROXY core/target")
Signed-off-by: Maxim Mikityanskiy <redacted>
---
  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.