[PATCH net v2 0/3] Fix out of bounds when parsing TCP options

[PATCH net v2 0/3] Fix out of bounds when parsing TCP options

[Maxim Mikityanskiy]

This series fixes out-of-bounds access in various places in the kernel
where parsing of TCP options takes place. Fortunately, many more
occurrences don't have this bug.

v2 changes:

synproxy: Added an early return when length < 0 to avoid calling
skb_header_pointer with negative length.

sch_cake: Added doff validation to avoid parsing garbage.

Maxim Mikityanskiy (3):
  netfilter: synproxy: Fix out of bounds when parsing TCP options
  mptcp: Fix out of bounds when parsing TCP options
  sch_cake: Fix out of bounds when parsing TCP options and header

 net/mptcp/options.c              | 2 ++
 net/netfilter/nf_synproxy_core.c | 5 +++++
 net/sched/sch_cake.c             | 6 +++++-
 3 files changed, 12 insertions(+), 1 deletion(-)

-- 
2.25.1

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

[Maxim Mikityanskiy]

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.").

v2 changes:

Added an early return when length < 0 to avoid calling
skb_header_pointer with negative length.

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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c
index b100c04a0e43..3d6d49420db8 100644
--- a/net/netfilter/nf_synproxy_core.c
+++ b/net/netfilter/nf_synproxy_core.c
@@ -31,6 +31,9 @@ synproxy_parse_options(const struct sk_buff *skb, unsigned int doff,
 	int length = (th->doff * 4) - sizeof(*th);
 	u8 buf[40], *ptr;
 
+	if (unlikely(length < 0))
+		return false;
+
 	ptr = skb_header_pointer(skb, doff + sizeof(*th), length, buf);
 	if (ptr == NULL)
 		return false;
@@ -47,6 +50,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;
-- 
2.25.1

[PATCH net v2 2/3] mptcp: Fix out of bounds when parsing TCP options

[Maxim Mikityanskiy]

The TCP option parser in mptcp (mptcp_get_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: cec37a6e41aa ("mptcp: Handle MP_CAPABLE options for outgoing connections")
Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
 net/mptcp/options.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 6b825fb3fa83..9b263f27ce9b 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -356,6 +356,8 @@ void mptcp_get_options(const struct sk_buff *skb,
 			length--;
 			continue;
 		default:
+			if (length < 2)
+				return;
 			opsize = *ptr++;
 			if (opsize < 2) /* "silly options" */
 				return;
-- 
2.25.1

[PATCH net v2 3/3] sch_cake: Fix out of bounds when parsing TCP options and header

[Maxim Mikityanskiy]

The TCP option parser in cake qdisc (cake_get_tcpopt and
cake_tcph_may_drop) 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.").

v2 changes:

Added doff validation in cake_get_tcphdr to avoid parsing garbage as TCP
header. Although it wasn't strictly an out-of-bounds access (memory was
allocated), garbage values could be read where CAKE expected the TCP
header if doff was smaller than 5.

Cc: Young Xiao <92siuyang@gmail.com>
Fixes: 8b7138814f29 ("sch_cake: Add optional ACK filter")
Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 net/sched/sch_cake.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index 7d37638ee1c7..5c15968b5155 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -943,7 +943,7 @@ static struct tcphdr *cake_get_tcphdr(const struct sk_buff *skb,
 	}
 
 	tcph = skb_header_pointer(skb, offset, sizeof(_tcph), &_tcph);
-	if (!tcph)
+	if (!tcph || tcph->doff < 5)
 		return NULL;
 
 	return skb_header_pointer(skb, offset,
@@ -967,6 +967,8 @@ static const void *cake_get_tcpopt(const struct tcphdr *tcph,
 			length--;
 			continue;
 		}
+		if (length < 2)
+			break;
 		opsize = *ptr++;
 		if (opsize < 2 || opsize > length)
 			break;
@@ -1104,6 +1106,8 @@ static bool cake_tcph_may_drop(const struct tcphdr *tcph,
 			length--;
 			continue;
 		}
+		if (length < 2)
+			break;
 		opsize = *ptr++;
 		if (opsize < 2 || opsize > length)
 			break;
-- 
2.25.1

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

[Florian Westphal]

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.").

Reviewed-by: Florian Westphal <fw@strlen.de>

Re: [PATCH net v2 2/3] mptcp: Fix out of bounds when parsing TCP options

[Mat Martineau]

On Thu, 10 Jun 2021, Maxim Mikityanskiy wrote:

> The TCP option parser in mptcp (mptcp_get_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: cec37a6e41aa ("mptcp: Handle MP_CAPABLE options for outgoing connections")
> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> ---
> net/mptcp/options.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 6b825fb3fa83..9b263f27ce9b 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -356,6 +356,8 @@ void mptcp_get_options(const struct sk_buff *skb,
> 			length--;
> 			continue;
> 		default:
> +			if (length < 2)
> +				return;
> 			opsize = *ptr++;
> 			if (opsize < 2) /* "silly options" */
> 				return;
> -- 
> 2.25.1

Matthieu -

Could you apply this in mptcp_net-next so it's easier to track when the 
patch finds its way to net-next?

(MPTCP patchwork status set to "queued")

Thanks!

--
Mat Martineau
Intel

Re: [PATCH net v2 2/3] mptcp: Fix out of bounds when parsing TCP options

[Mat Martineau]

On Thu, 10 Jun 2021, Mat Martineau wrote:

> On Thu, 10 Jun 2021, Maxim Mikityanskiy wrote:
>
>> The TCP option parser in mptcp (mptcp_get_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: cec37a6e41aa ("mptcp: Handle MP_CAPABLE options for outgoing 
>> connections")
>> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
>> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>> ---
>> net/mptcp/options.c | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index 6b825fb3fa83..9b263f27ce9b 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -356,6 +356,8 @@ void mptcp_get_options(const struct sk_buff *skb,
>> 			length--;
>> 			continue;
>> 		default:
>> +			if (length < 2)
>> +				return;
>> 			opsize = *ptr++;
>> 			if (opsize < 2) /* "silly options" */
>> 				return;
>> -- 
>> 2.25.1
>
> Matthieu -
>
> Could you apply this in mptcp_net-next so it's easier to track when the patch 
> finds its way to net-next?
>
> (MPTCP patchwork status set to "queued")

And to clarify for Maxim: we have a separate MPTCP git tree where we stage 
MPTCP changes before submitting to net- or net-next. For your patch set 
that has related changes across a few networking subsystems, sending 
directly to netdev is the best thing to do.

We do have other queued changes for netdev so I think it would work well 
to include this patch in our tree now, and it will automatically disappear 
once it is fully merged upstream. Sometimes it takes a couple of weeks for 
the net/master branch to get merged in to net-next/master.

--
Mat Martineau
Intel

Re: [PATCH net v2 0/3] Fix out of bounds when parsing TCP options

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (refs/heads/master):

On Thu, 10 Jun 2021 19:40:28 +0300 you wrote:
> This series fixes out-of-bounds access in various places in the kernel
> where parsing of TCP options takes place. Fortunately, many more
> occurrences don't have this bug.
> 
> v2 changes:
> 
> synproxy: Added an early return when length < 0 to avoid calling
> skb_header_pointer with negative length.
> 
> [...]

Here is the summary with links:
  - [net,v2,1/3] netfilter: synproxy: Fix out of bounds when parsing TCP options
    https://git.kernel.org/netdev/net/c/5fc177ab7594
  - [net,v2,2/3] mptcp: Fix out of bounds when parsing TCP options
    https://git.kernel.org/netdev/net/c/07718be26568
  - [net,v2,3/3] sch_cake: Fix out of bounds when parsing TCP options and header
    https://git.kernel.org/netdev/net/c/ba91c49dedbd

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net v2 2/3] mptcp: Fix out of bounds when parsing TCP options

[Matthieu Baerts]

Hi Mat,

On 10/06/2021 23:03, Mat Martineau wrote:
> On Thu, 10 Jun 2021, Maxim Mikityanskiy wrote:
> 
>> The TCP option parser in mptcp (mptcp_get_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: cec37a6e41aa ("mptcp: Handle MP_CAPABLE options for outgoing
>> connections")
>> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
>> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> 
> Matthieu -
> 
> Could you apply this in mptcp_net-next so it's easier to track when the
> patch finds its way to net-next?

Sure, good idea!
Thank you for the patch and the review!

Just applied in our tree:

- fdd037a5564a: mptcp: Fix out of bounds when parsing TCP options
- Results: 083034593cec..1ba3eb627299

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210611T142646
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210611T142646

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net