[PATCH 0/6] broken CVE fix (b85d130ea0ca)

[PATCH 0/6] broken CVE fix (b85d130ea0ca)

[Rasmus Villemoes]

tl;dr: b85d130ea0ca didn't fix the CVE(s), but did break tftp of
certain file sizes - which is somewhat lucky, since that's how I
noticed in the first place.

What I at first hoped would be a one-liner trivial fix turned out to
be much more complicated and led me down a rabbit hole of related
fixes. And this isn't even complete, I'm afraid. Details in 3/6.

1 and 4 are independent of all the others. 5 is a trivial preparation
for 6; otherwise those are also independent of the others. Finally, 2
and 3 are my attempts at actually fixing CVE-2022-{30790,30552}, with
2 essentially lifting the "ensure the payload has non-negative size"
to the first place we can check that instead of relying on that check
to happen in several places.


Rasmus Villemoes (6):
  net: improve check for no IP options
  net: compare received length to sizeof(ip_hdr), not sizeof(ip_udp_hdr)
  net: (actually/better) deal with CVE-2022-{30790,30552}
  net: fix ip_len in reassembled IP datagram
  net: tftp: use IS_ENABLED(CONFIG_NET_TFTP_VARS) instead of #if
  net: tftp: sanitize tftp block size, especially for TX

 net/net.c  |  24 +++++++++----
 net/tftp.c | 102 ++++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 92 insertions(+), 34 deletions(-)

-- 
2.37.2

[PATCH 1/6] net: improve check for no IP options

[Rasmus Villemoes]

There's no reason we should accept an IP packet with a malformed IHL
field. So ensure that it is exactly 5, not just <= 5.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 net/net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/net.c b/net/net.c
index 81905f6315..536731245b 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1209,7 +1209,7 @@ void net_process_received_packet(uchar *in_packet, int len)
 		if ((ip->ip_hl_v & 0xf0) != 0x40)
 			return;
 		/* Can't deal with IP options (headers != 20 bytes) */
-		if ((ip->ip_hl_v & 0x0f) > 0x05)
+		if ((ip->ip_hl_v & 0x0f) != 0x05)
 			return;
 		/* Check the Checksum of the header */
 		if (!ip_checksum_ok((uchar *)ip, IP_HDR_SIZE)) {
-- 
2.37.2

Re: [PATCH 1/6] net: improve check for no IP options

[Ramon Fried]

On Fri, Oct 14, 2022 at 8:43 PM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> There's no reason we should accept an IP packet with a malformed IHL
> field. So ensure that it is exactly 5, not just <= 5.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  net/net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/net.c b/net/net.c
> index 81905f6315..536731245b 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1209,7 +1209,7 @@ void net_process_received_packet(uchar *in_packet, int len)
>                 if ((ip->ip_hl_v & 0xf0) != 0x40)
>                         return;
>                 /* Can't deal with IP options (headers != 20 bytes) */
> -               if ((ip->ip_hl_v & 0x0f) > 0x05)
> +               if ((ip->ip_hl_v & 0x0f) != 0x05)
>                         return;
>                 /* Check the Checksum of the header */
>                 if (!ip_checksum_ok((uchar *)ip, IP_HDR_SIZE)) {
> --
> 2.37.2
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 1/6] net: improve check for no IP options

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

On Fri, Oct 14, 2022 at 07:43:37PM +0200, Rasmus Villemoes wrote:

> There's no reason we should accept an IP packet with a malformed IHL
> field. So ensure that it is exactly 5, not just <= 5.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 2/6] net: compare received length to sizeof(ip_hdr), not sizeof(ip_udp_hdr)

[Rasmus Villemoes]

While the code mostly/only handles UDP packets, it's possible for the
last fragment of a fragmented UDP packet to be smaller than 28 bytes;
it can be as small as 21 bytes (an IP header plus one byte of
payload). So until we've performed the defragmentation step and thus
know whether we're now holding a full packet, we should only check for
the existence of the fields in the ip header, i.e. that there are at
least 20 bytes present.

In practice, we always seem to be handed a "len" of minimum 60 from the
device layer, i.e. minimal ethernet frame length minus FCS, so this is
mostly theoretical.

After we've fetched the header's claimed length and used that to
update the len variable, check that the header itself claims to be the
minimal possible length.

This is probably how CVE-2022-30552 should have been dealt with in the
first place, because net_defragment() is not the only place that wants
to know the size of the IP datagram payload: If we receive a
non-fragmented ICMP packet, we pass "len" to receive_icmp() which in
turn may pass it to ping_receive() which does

  compute_ip_checksum(icmph, len - IP_HDR_SIZE)

and due to the signature of compute_ip_checksum(), that would then
lead to accessing ~4G of address space, very likely leading to a
crash.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 net/net.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/net.c b/net/net.c
index 536731245b..86b1d90159 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1191,9 +1191,9 @@ void net_process_received_packet(uchar *in_packet, int len)
 	case PROT_IP:
 		debug_cond(DEBUG_NET_PKT, "Got IP\n");
 		/* Before we start poking the header, make sure it is there */
-		if (len < IP_UDP_HDR_SIZE) {
+		if (len < IP_HDR_SIZE) {
 			debug("len bad %d < %lu\n", len,
-			      (ulong)IP_UDP_HDR_SIZE);
+			      (ulong)IP_HDR_SIZE);
 			return;
 		}
 		/* Check the packet length */
@@ -1202,6 +1202,10 @@ void net_process_received_packet(uchar *in_packet, int len)
 			return;
 		}
 		len = ntohs(ip->ip_len);
+		if (len < IP_HDR_SIZE) {
+			debug("bad ip->ip_len %d < %d\n", len, (int)IP_HDR_SIZE);
+			return;
+		}
 		debug_cond(DEBUG_NET_PKT, "len=%d, v=%02x\n",
 			   len, ip->ip_hl_v & 0xff);
 
-- 
2.37.2

Re: [PATCH 2/6] net: compare received length to sizeof(ip_hdr), not sizeof(ip_udp_hdr)

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1536 bytes --]

On Fri, Oct 14, 2022 at 07:43:38PM +0200, Rasmus Villemoes wrote:

> While the code mostly/only handles UDP packets, it's possible for the
> last fragment of a fragmented UDP packet to be smaller than 28 bytes;
> it can be as small as 21 bytes (an IP header plus one byte of
> payload). So until we've performed the defragmentation step and thus
> know whether we're now holding a full packet, we should only check for
> the existence of the fields in the ip header, i.e. that there are at
> least 20 bytes present.
> 
> In practice, we always seem to be handed a "len" of minimum 60 from the
> device layer, i.e. minimal ethernet frame length minus FCS, so this is
> mostly theoretical.
> 
> After we've fetched the header's claimed length and used that to
> update the len variable, check that the header itself claims to be the
> minimal possible length.
> 
> This is probably how CVE-2022-30552 should have been dealt with in the
> first place, because net_defragment() is not the only place that wants
> to know the size of the IP datagram payload: If we receive a
> non-fragmented ICMP packet, we pass "len" to receive_icmp() which in
> turn may pass it to ping_receive() which does
> 
>   compute_ip_checksum(icmph, len - IP_HDR_SIZE)
> 
> and due to the signature of compute_ip_checksum(), that would then
> lead to accessing ~4G of address space, very likely leading to a
> crash.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 3/6] net: (actually/better) deal with CVE-2022-{30790,30552}

[Rasmus Villemoes]

I hit a strange problem with v2022.10: Sometimes my tftp transfer
would seemingly just hang. It only happened for some files. Moreover,
changing tftpblocksize from 65464 to 65460 or 65000 made it work again
for all the files I tried. So I started suspecting it had something to
do with the file sizes and in particular the way the tftp blocks get
fragmented and reassembled.

v2022.01 showed no problems with any of the files or any value of
tftpblocksize.

Looking at what had changed in net.c or tftp.c since January showed
only one remotely interesting thing, b85d130ea0ca.

So I fired up wireshark on my host to see if somehow one of the
packets would be too small. But no, with both v2022.01 and v2022.10,
the exact same sequence of packets were sent, all but the last of size
1500, and the last being 1280 bytes.

But then it struck me that 1280 is 5*256, so one of the two bytes
on-the-wire is 0 and the other is 5, and when then looking at the code
again the lack of endianness conversion becomes obvious. [ntohs is
both applied to ip->ip_off just above, as well as to ip->ip_len just a
little further down when the "len" is actually computed].

IOWs the current code would falsely reject any packet which happens to
be a multiple of 256 bytes in size, breaking tftp transfers somewhat
randomly, and if it did get one of those "malicious" packets with
ip_len set to, say, 27, it would be seen by this check as being 6912
and hence not rejected.

====

Now, just adding the missing ntohs() would make my initial problem go
away, in that I can now download the file where the last fragment ends
up being 1280 bytes. But there's another bug in the code and/or
analysis: The right-hand side is too strict, in that it is ok for the
last fragment not to have a multiple of 8 bytes as payload - it really
must be ok, because nothing in the IP spec says that IP datagrams must
have a multiple of 8 bytes as payload. And comments in the code also
mention this.

To fix that, replace the comparison with <= IP_HDR_SIZE and add
another check that len is actually a multiple of 8 when the "more
fragments" bit is set - which it necessarily is for the case where
offset8 ends up being 0, since we're only called when

  (ip_off & (IP_OFFS | IP_FLAGS_MFRAG)).

====

So, does this fix CVE-2022-30790 for real? It certainly correctly
rejects the POC code which relies on sending a packet of size 27 with
the MFRAG flag set. Can the attack be carried out with a size 27
packet that doesn't set MFRAG (hence must set a non-zero fragment
offset)? I dunno. If we get a packet without MFRAG, we update
h->last_byte in the hole we've found to be start+len, hence we'd enter
one of

	if ((h >= thisfrag) && (h->last_byte <= start + len)) {

or

	} else if (h->last_byte <= start + len) {

and thus won't reach any of the

		/* overlaps with initial part of the hole: move this hole */
		newh = thisfrag + (len / 8);

		/* fragment sits in the middle: split the hole */
		newh = thisfrag + (len / 8);

IOW these division are now guaranteed to be exact, and thus I think
the scenario in CVE-2022-30790 cannot happen anymore.

====

However, there's a big elephant in the room, which has always been
spelled out in the comments, and which makes me believe that one can
still cause mayhem even with packets whose payloads are all 8-byte
aligned:

    This code doesn't deal with a fragment that overlaps with two
    different holes (thus being a superset of a previously-received
    fragment).

Suppose each character below represents 8 bytes, with D being already
received data, H being a hole descriptor (struct hole), h being
non-populated chunks, and P representing where the payload of a just
received packet should go:

  DDDHhhhhDDDDHhhhDDDD
        PPPPPPPPP

I'm pretty sure in this case we'd end up with h being the first hole,
enter the simple

	} else if (h->last_byte <= start + len) {
		/* overlaps with final part of the hole: shorten this hole */
		h->last_byte = start;

case, and thus in the memcpy happily overwrite the second H with our
chosen payload. This is probably worth fixing...

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 net/net.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/net.c b/net/net.c
index 86b1d90159..340e7b8f18 100644
--- a/net/net.c
+++ b/net/net.c
@@ -907,7 +907,11 @@ static struct ip_udp_hdr *__net_defragment(struct ip_udp_hdr *ip, int *lenp)
 	int offset8, start, len, done = 0;
 	u16 ip_off = ntohs(ip->ip_off);
 
-	if (ip->ip_len < IP_MIN_FRAG_DATAGRAM_SIZE)
+	/*
+	 * Calling code already rejected <, but we don't have to deal
+	 * with an IP fragment with no payload.
+	 */
+	if (ntohs(ip->ip_len) <= IP_HDR_SIZE)
 		return NULL;
 
 	/* payload starts after IP header, this fragment is in there */
@@ -917,6 +921,10 @@ static struct ip_udp_hdr *__net_defragment(struct ip_udp_hdr *ip, int *lenp)
 	start = offset8 * 8;
 	len = ntohs(ip->ip_len) - IP_HDR_SIZE;
 
+	/* All but last fragment must have a multiple-of-8 payload. */
+	if ((len & 7) && (ip_off & IP_FLAGS_MFRAG))
+		return NULL;
+
 	if (start + len > IP_MAXUDP) /* fragment extends too far */
 		return NULL;
 
-- 
2.37.2

Re: [PATCH 3/6] net: (actually/better) deal with CVE-2022-{30790,30552}

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4585 bytes --]

On Fri, Oct 14, 2022 at 07:43:39PM +0200, Rasmus Villemoes wrote:

> I hit a strange problem with v2022.10: Sometimes my tftp transfer
> would seemingly just hang. It only happened for some files. Moreover,
> changing tftpblocksize from 65464 to 65460 or 65000 made it work again
> for all the files I tried. So I started suspecting it had something to
> do with the file sizes and in particular the way the tftp blocks get
> fragmented and reassembled.
> 
> v2022.01 showed no problems with any of the files or any value of
> tftpblocksize.
> 
> Looking at what had changed in net.c or tftp.c since January showed
> only one remotely interesting thing, b85d130ea0ca.
> 
> So I fired up wireshark on my host to see if somehow one of the
> packets would be too small. But no, with both v2022.01 and v2022.10,
> the exact same sequence of packets were sent, all but the last of size
> 1500, and the last being 1280 bytes.
> 
> But then it struck me that 1280 is 5*256, so one of the two bytes
> on-the-wire is 0 and the other is 5, and when then looking at the code
> again the lack of endianness conversion becomes obvious. [ntohs is
> both applied to ip->ip_off just above, as well as to ip->ip_len just a
> little further down when the "len" is actually computed].
> 
> IOWs the current code would falsely reject any packet which happens to
> be a multiple of 256 bytes in size, breaking tftp transfers somewhat
> randomly, and if it did get one of those "malicious" packets with
> ip_len set to, say, 27, it would be seen by this check as being 6912
> and hence not rejected.
> 
> ====
> 
> Now, just adding the missing ntohs() would make my initial problem go
> away, in that I can now download the file where the last fragment ends
> up being 1280 bytes. But there's another bug in the code and/or
> analysis: The right-hand side is too strict, in that it is ok for the
> last fragment not to have a multiple of 8 bytes as payload - it really
> must be ok, because nothing in the IP spec says that IP datagrams must
> have a multiple of 8 bytes as payload. And comments in the code also
> mention this.
> 
> To fix that, replace the comparison with <= IP_HDR_SIZE and add
> another check that len is actually a multiple of 8 when the "more
> fragments" bit is set - which it necessarily is for the case where
> offset8 ends up being 0, since we're only called when
> 
>   (ip_off & (IP_OFFS | IP_FLAGS_MFRAG)).
> 
> ====
> 
> So, does this fix CVE-2022-30790 for real? It certainly correctly
> rejects the POC code which relies on sending a packet of size 27 with
> the MFRAG flag set. Can the attack be carried out with a size 27
> packet that doesn't set MFRAG (hence must set a non-zero fragment
> offset)? I dunno. If we get a packet without MFRAG, we update
> h->last_byte in the hole we've found to be start+len, hence we'd enter
> one of
> 
> 	if ((h >= thisfrag) && (h->last_byte <= start + len)) {
> 
> or
> 
> 	} else if (h->last_byte <= start + len) {
> 
> and thus won't reach any of the
> 
> 		/* overlaps with initial part of the hole: move this hole */
> 		newh = thisfrag + (len / 8);
> 
> 		/* fragment sits in the middle: split the hole */
> 		newh = thisfrag + (len / 8);
> 
> IOW these division are now guaranteed to be exact, and thus I think
> the scenario in CVE-2022-30790 cannot happen anymore.
> 
> ====
> 
> However, there's a big elephant in the room, which has always been
> spelled out in the comments, and which makes me believe that one can
> still cause mayhem even with packets whose payloads are all 8-byte
> aligned:
> 
>     This code doesn't deal with a fragment that overlaps with two
>     different holes (thus being a superset of a previously-received
>     fragment).
> 
> Suppose each character below represents 8 bytes, with D being already
> received data, H being a hole descriptor (struct hole), h being
> non-populated chunks, and P representing where the payload of a just
> received packet should go:
> 
>   DDDHhhhhDDDDHhhhDDDD
>         PPPPPPPPP
> 
> I'm pretty sure in this case we'd end up with h being the first hole,
> enter the simple
> 
> 	} else if (h->last_byte <= start + len) {
> 		/* overlaps with final part of the hole: shorten this hole */
> 		h->last_byte = start;
> 
> case, and thus in the memcpy happily overwrite the second H with our
> chosen payload. This is probably worth fixing...
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 4/6] net: fix ip_len in reassembled IP datagram

[Rasmus Villemoes]

For some reason, the ip_len field in a reassembled IP datagram is set
to just the size of the payload, but it should be set to the value it
would have had if the datagram had never been fragmented in the first
place, i.e. size of payload plus size of IP header.

That latter value is currently returned correctly via the "len"
variable. And before entering net_defragment(), len does have the
value ntohs(ip->ip_len), so if we're not dealing with a
fragment (so net_defragment leaves *len alone), that relationship of
course also holds after the net_defragment() call.

The only use I can find of ip->ip_len after the net_defragment call is
the ntohs(ip->udp_len) > ntohs(ip->ip_len) sanity check - none of the
functions that are passed the "ip" pointer themselves inspect ->ip_len
but instead use the passed len.

But that sanity check is a bit odd, since the RHS really should be
"ntohs(ip->ip_len) - 20", i.e. the IP payload size.

Now that we've fixed things so that len == ntohs(ip->ip_len) in all
cases, change that sanity check to use len-20 as the RHS.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 net/net.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/net.c b/net/net.c
index 340e7b8f18..d3ff871bca 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1023,8 +1023,8 @@ static struct ip_udp_hdr *__net_defragment(struct ip_udp_hdr *ip, int *lenp)
 	if (!done)
 		return NULL;
 
-	localip->ip_len = htons(total_len);
 	*lenp = total_len + IP_HDR_SIZE;
+	localip->ip_len = htons(*lenp);
 	return localip;
 }
 
@@ -1272,7 +1272,7 @@ void net_process_received_packet(uchar *in_packet, int len)
 			return;
 		}
 
-		if (ntohs(ip->udp_len) < UDP_HDR_SIZE || ntohs(ip->udp_len) > ntohs(ip->ip_len))
+		if (ntohs(ip->udp_len) < UDP_HDR_SIZE || ntohs(ip->udp_len) > len - IP_HDR_SIZE)
 			return;
 
 		debug_cond(DEBUG_DEV_PKT,
-- 
2.37.2

Re: [PATCH 4/6] net: fix ip_len in reassembled IP datagram

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

On Fri, Oct 14, 2022 at 07:43:40PM +0200, Rasmus Villemoes wrote:

> For some reason, the ip_len field in a reassembled IP datagram is set
> to just the size of the payload, but it should be set to the value it
> would have had if the datagram had never been fragmented in the first
> place, i.e. size of payload plus size of IP header.
> 
> That latter value is currently returned correctly via the "len"
> variable. And before entering net_defragment(), len does have the
> value ntohs(ip->ip_len), so if we're not dealing with a
> fragment (so net_defragment leaves *len alone), that relationship of
> course also holds after the net_defragment() call.
> 
> The only use I can find of ip->ip_len after the net_defragment call is
> the ntohs(ip->udp_len) > ntohs(ip->ip_len) sanity check - none of the
> functions that are passed the "ip" pointer themselves inspect ->ip_len
> but instead use the passed len.
> 
> But that sanity check is a bit odd, since the RHS really should be
> "ntohs(ip->ip_len) - 20", i.e. the IP payload size.
> 
> Now that we've fixed things so that len == ntohs(ip->ip_len) in all
> cases, change that sanity check to use len-20 as the RHS.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 5/6] net: tftp: use IS_ENABLED(CONFIG_NET_TFTP_VARS) instead of #if

[Rasmus Villemoes]

Nothing inside this block depends on NET_TFTP_VARS to be set to parse
correctly. Switch to C if() in preparation for adding code before
this (to avoid a declaration-after-statement warning).

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 net/tftp.c | 56 +++++++++++++++++++++++++++---------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/net/tftp.c b/net/tftp.c
index dea9c25ffd..e5e140bcd5 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -710,42 +710,42 @@ static int tftp_init_load_addr(void)
 
 void tftp_start(enum proto_t protocol)
 {
-#if CONFIG_NET_TFTP_VARS
-	char *ep;             /* Environment pointer */
+	if (IS_ENABLED(CONFIG_NET_TFTP_VARS)) {
+		char *ep;             /* Environment pointer */
 
-	/*
-	 * Allow the user to choose TFTP blocksize and timeout.
-	 * TFTP protocol has a minimal timeout of 1 second.
-	 */
+		/*
+		 * Allow the user to choose TFTP blocksize and timeout.
+		 * TFTP protocol has a minimal timeout of 1 second.
+		 */
 
-	ep = env_get("tftpblocksize");
-	if (ep != NULL)
-		tftp_block_size_option = simple_strtol(ep, NULL, 10);
+		ep = env_get("tftpblocksize");
+		if (ep != NULL)
+			tftp_block_size_option = simple_strtol(ep, NULL, 10);
 
-	ep = env_get("tftpwindowsize");
-	if (ep != NULL)
-		tftp_window_size_option = simple_strtol(ep, NULL, 10);
+		ep = env_get("tftpwindowsize");
+		if (ep != NULL)
+			tftp_window_size_option = simple_strtol(ep, NULL, 10);
 
-	ep = env_get("tftptimeout");
-	if (ep != NULL)
-		timeout_ms = simple_strtol(ep, NULL, 10);
+		ep = env_get("tftptimeout");
+		if (ep != NULL)
+			timeout_ms = simple_strtol(ep, NULL, 10);
 
-	if (timeout_ms < 1000) {
-		printf("TFTP timeout (%ld ms) too low, set min = 1000 ms\n",
-		       timeout_ms);
-		timeout_ms = 1000;
-	}
+		if (timeout_ms < 1000) {
+			printf("TFTP timeout (%ld ms) too low, set min = 1000 ms\n",
+			       timeout_ms);
+			timeout_ms = 1000;
+		}
 
-	ep = env_get("tftptimeoutcountmax");
-	if (ep != NULL)
-		tftp_timeout_count_max = simple_strtol(ep, NULL, 10);
+		ep = env_get("tftptimeoutcountmax");
+		if (ep != NULL)
+			tftp_timeout_count_max = simple_strtol(ep, NULL, 10);
 
-	if (tftp_timeout_count_max < 0) {
-		printf("TFTP timeout count max (%d ms) negative, set to 0\n",
-		       tftp_timeout_count_max);
-		tftp_timeout_count_max = 0;
+		if (tftp_timeout_count_max < 0) {
+			printf("TFTP timeout count max (%d ms) negative, set to 0\n",
+			       tftp_timeout_count_max);
+			tftp_timeout_count_max = 0;
+		}
 	}
-#endif
 
 	debug("TFTP blocksize = %i, TFTP windowsize = %d timeout = %ld ms\n",
 	      tftp_block_size_option, tftp_window_size_option, timeout_ms);
-- 
2.37.2

Re: [PATCH 5/6] net: tftp: use IS_ENABLED(CONFIG_NET_TFTP_VARS) instead of #if

[Ramon Fried]

On Fri, Oct 14, 2022 at 8:44 PM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Nothing inside this block depends on NET_TFTP_VARS to be set to parse
> correctly. Switch to C if() in preparation for adding code before
> this (to avoid a declaration-after-statement warning).
What's the motivation here ? The #ifdef is supposed to allow smaller
code size if feature is not used.

Re: [PATCH 5/6] net: tftp: use IS_ENABLED(CONFIG_NET_TFTP_VARS) instead of #if

[Rasmus Villemoes]

On 16/10/2022 20.28, Ramon Fried wrote:
> On Fri, Oct 14, 2022 at 8:44 PM Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> Nothing inside this block depends on NET_TFTP_VARS to be set to parse
>> correctly. Switch to C if() in preparation for adding code before
>> this (to avoid a declaration-after-statement warning).
> What's the motivation here ? The #ifdef is supposed to allow smaller
> code size if feature is not used.

And as always with these types of conversions, nothing changes in that
regard - an "if (0)" is optimized out by the compiler just fine.

The motivation is in the commit message: The following patch will need
to add a bit of logic at the very beginning of the function, so if I
left the #ifdef block as-is, the declaration of the ep variable would
lead to a declaration-after-statement warning; lifting the declaration
to the top would either require repeating the ugly ifdeffery, or give a
"variable not used" warning.

It's also in general the preferred method, because it means the
contained code gets checked for syntactic correctness regardless of
.config - but that very same thing is also why it cannot be applied
universally, if the contained code e.g. refers to struct fields that are
only defined with certain CONFIG_FOO set [and this also was alluded to
in the commit message].

Rasmus

Re: [PATCH 5/6] net: tftp: use IS_ENABLED(CONFIG_NET_TFTP_VARS) instead of #if

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

On Fri, Oct 14, 2022 at 07:43:41PM +0200, Rasmus Villemoes wrote:

> Nothing inside this block depends on NET_TFTP_VARS to be set to parse
> correctly. Switch to C if() in preparation for adding code before
> this (to avoid a declaration-after-statement warning).
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 6/6] net: tftp: sanitize tftp block size, especially for TX

[Rasmus Villemoes]

U-Boot does not support IP fragmentation on TX (and unless
CONFIG_IP_DEFRAG is set, neither on RX). So the blocks we send must
fit in a single ethernet packet.

Currently, if tftpblocksize is set to something like 5000 and I
tftpput a large enough file, U-Boot crashes because we overflow
net_tx_packet (which only has room for 1500 bytes plus change).

Similarly, if tftpblocksize is set to something larger than what we
can actually receive (e.g. 50000, with NET_MAXDEFRAG being 16384), any
tftp get just hangs because we never receive any packets.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 net/tftp.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/net/tftp.c b/net/tftp.c
index e5e140bcd5..60e1273332 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -708,8 +708,52 @@ static int tftp_init_load_addr(void)
 	return 0;
 }
 
+static int saved_tftp_block_size_option;
+static void sanitize_tftp_block_size_option(enum proto_t protocol)
+{
+	int cap, max_defrag;
+
+	switch (protocol) {
+	case TFTPGET:
+		max_defrag = config_opt_enabled(CONFIG_IP_DEFRAG, CONFIG_NET_MAXDEFRAG, 0);
+		if (max_defrag) {
+			/* Account for IP, UDP and TFTP headers. */
+			cap = max_defrag - (20 + 8 + 4);
+			/* RFC2348 sets a hard upper limit. */
+			cap = min(cap, 65464);
+			break;
+		}
+		/*
+		 * If not CONFIG_IP_DEFRAG, cap at the same value as
+		 * for tftp put, namely normal MTU minus protocol
+		 * overhead.
+		 */
+		/* fall through */
+	case TFTPPUT:
+	default:
+		/*
+		 * U-Boot does not support IP fragmentation on TX, so
+		 * this must be small enough that it fits normal MTU
+		 * (and small enough that it fits net_tx_packet which
+		 * has room for PKTSIZE_ALIGN bytes).
+		 */
+		cap = 1468;
+	}
+	if (tftp_block_size_option > cap) {
+		printf("Capping tftp block size option to %d (was %d)\n",
+		       cap, tftp_block_size_option);
+		saved_tftp_block_size_option = tftp_block_size_option;
+		tftp_block_size_option = cap;
+	}
+}
+
 void tftp_start(enum proto_t protocol)
 {
+	if (saved_tftp_block_size_option) {
+		tftp_block_size_option = saved_tftp_block_size_option;
+		saved_tftp_block_size_option = 0;
+	}
+
 	if (IS_ENABLED(CONFIG_NET_TFTP_VARS)) {
 		char *ep;             /* Environment pointer */
 
@@ -747,6 +791,8 @@ void tftp_start(enum proto_t protocol)
 		}
 	}
 
+	sanitize_tftp_block_size_option(protocol);
+
 	debug("TFTP blocksize = %i, TFTP windowsize = %d timeout = %ld ms\n",
 	      tftp_block_size_option, tftp_window_size_option, timeout_ms);
 
-- 
2.37.2

Re: [PATCH 6/6] net: tftp: sanitize tftp block size, especially for TX

[Ramon Fried]

On Fri, Oct 14, 2022 at 8:44 PM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> U-Boot does not support IP fragmentation on TX (and unless
> CONFIG_IP_DEFRAG is set, neither on RX). So the blocks we send must
> fit in a single ethernet packet.
>
> Currently, if tftpblocksize is set to something like 5000 and I
> tftpput a large enough file, U-Boot crashes because we overflow
> net_tx_packet (which only has room for 1500 bytes plus change).
>
> Similarly, if tftpblocksize is set to something larger than what we
> can actually receive (e.g. 50000, with NET_MAXDEFRAG being 16384), any
> tftp get just hangs because we never receive any packets.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  net/tftp.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/net/tftp.c b/net/tftp.c
> index e5e140bcd5..60e1273332 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -708,8 +708,52 @@ static int tftp_init_load_addr(void)
>         return 0;
>  }
>
> +static int saved_tftp_block_size_option;
> +static void sanitize_tftp_block_size_option(enum proto_t protocol)
> +{
> +       int cap, max_defrag;
> +
> +       switch (protocol) {
> +       case TFTPGET:
> +               max_defrag = config_opt_enabled(CONFIG_IP_DEFRAG, CONFIG_NET_MAXDEFRAG, 0);
> +               if (max_defrag) {
> +                       /* Account for IP, UDP and TFTP headers. */
> +                       cap = max_defrag - (20 + 8 + 4);
> +                       /* RFC2348 sets a hard upper limit. */
> +                       cap = min(cap, 65464);
> +                       break;
> +               }
> +               /*
> +                * If not CONFIG_IP_DEFRAG, cap at the same value as
> +                * for tftp put, namely normal MTU minus protocol
> +                * overhead.
> +                */
> +               /* fall through */
> +       case TFTPPUT:
> +       default:
> +               /*
> +                * U-Boot does not support IP fragmentation on TX, so
> +                * this must be small enough that it fits normal MTU
> +                * (and small enough that it fits net_tx_packet which
> +                * has room for PKTSIZE_ALIGN bytes).
> +                */
> +               cap = 1468;
> +       }
> +       if (tftp_block_size_option > cap) {
> +               printf("Capping tftp block size option to %d (was %d)\n",
> +                      cap, tftp_block_size_option);
> +               saved_tftp_block_size_option = tftp_block_size_option;
> +               tftp_block_size_option = cap;
> +       }
> +}
> +
>  void tftp_start(enum proto_t protocol)
>  {
> +       if (saved_tftp_block_size_option) {
> +               tftp_block_size_option = saved_tftp_block_size_option;
> +               saved_tftp_block_size_option = 0;
> +       }
> +
>         if (IS_ENABLED(CONFIG_NET_TFTP_VARS)) {
>                 char *ep;             /* Environment pointer */
>
> @@ -747,6 +791,8 @@ void tftp_start(enum proto_t protocol)
>                 }
>         }
>
> +       sanitize_tftp_block_size_option(protocol);
> +
>         debug("TFTP blocksize = %i, TFTP windowsize = %d timeout = %ld ms\n",
>               tftp_block_size_option, tftp_window_size_option, timeout_ms);
>
> --
> 2.37.2
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 6/6] net: tftp: sanitize tftp block size, especially for TX

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

On Fri, Oct 14, 2022 at 07:43:42PM +0200, Rasmus Villemoes wrote:

> U-Boot does not support IP fragmentation on TX (and unless
> CONFIG_IP_DEFRAG is set, neither on RX). So the blocks we send must
> fit in a single ethernet packet.
> 
> Currently, if tftpblocksize is set to something like 5000 and I
> tftpput a large enough file, U-Boot crashes because we overflow
> net_tx_packet (which only has room for 1500 bytes plus change).
> 
> Similarly, if tftpblocksize is set to something larger than what we
> can actually receive (e.g. 50000, with NET_MAXDEFRAG being 16384), any
> tftp get just hangs because we never receive any packets.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/6] broken CVE fix (b85d130ea0ca)

[Fabio Estevam]

Hi Rasmus,

On Fri, Oct 14, 2022 at 2:44 PM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> tl;dr: b85d130ea0ca didn't fix the CVE(s), but did break tftp of
> certain file sizes - which is somewhat lucky, since that's how I
> noticed in the first place.
>
> What I at first hoped would be a one-liner trivial fix turned out to
> be much more complicated and led me down a rabbit hole of related
> fixes. And this isn't even complete, I'm afraid. Details in 3/6.
>
> 1 and 4 are independent of all the others. 5 is a trivial preparation
> for 6; otherwise those are also independent of the others. Finally, 2
> and 3 are my attempts at actually fixing CVE-2022-{30790,30552}, with
> 2 essentially lifting the "ensure the payload has non-negative size"
> to the first place we can check that instead of relying on that check
> to happen in several places.

Thanks for the fix:

Reviewed-by: Fabio Estevam <festevam@denx.de>

[PATCH 7/6] net: deal with fragment-overlapping-two-holes case

[Rasmus Villemoes]

With a suitable sequence of malicious packets, it's currently possible
to get a hole descriptor to contain arbitrary attacker-controlled
contents, and then with one more packet to use that as an arbitrary
write vector.

While one could possibly change the algorithm so we instead loop over
all holes, and in each hole puts as much of the current fragment as
belongs there (taking care to carefully update the hole list as
appropriate), it's not worth the complexity: In real, non-malicious
scenarios, one never gets overlapping fragments, and certainly not
fragments that would be supersets of one another.

So instead opt for this simple protection: Simply don't allow the
eventual memcpy() to write beyond the last_byte of the current hole.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---

I've been mulling over this over the weekend, and concluded that this
should be enough for now. Even if I'm wrong about overlapping
fragments not happening in real life, this doesn't break something
that works currently, and does prevent the trivial attacker-controller
hole descriptor.

 net/net.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/net.c b/net/net.c
index d3ff871bca..5c6aea0c55 100644
--- a/net/net.c
+++ b/net/net.c
@@ -968,10 +968,14 @@ static struct ip_udp_hdr *__net_defragment(struct ip_udp_hdr *ip, int *lenp)
 	}
 
 	/*
-	 * There is some overlap: fix the hole list. This code doesn't
-	 * deal with a fragment that overlaps with two different holes
-	 * (thus being a superset of a previously-received fragment).
+	 * There is some overlap: fix the hole list. This code deals
+	 * with a fragment that overlaps with two different holes
+	 * (thus being a superset of a previously-received fragment)
+	 * by only using the part of the fragment that fits in the
+	 * first hole.
 	 */
+	if (h->last_byte < start + len)
+		len = h->last_byte - start;
 
 	if ((h >= thisfrag) && (h->last_byte <= start + len)) {
 		/* complete overlap with hole: remove hole */
-- 
2.37.2

Re: [PATCH 7/6] net: deal with fragment-overlapping-two-holes case

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 971 bytes --]

On Mon, Oct 17, 2022 at 09:52:51AM +0200, Rasmus Villemoes wrote:

> With a suitable sequence of malicious packets, it's currently possible
> to get a hole descriptor to contain arbitrary attacker-controlled
> contents, and then with one more packet to use that as an arbitrary
> write vector.
> 
> While one could possibly change the algorithm so we instead loop over
> all holes, and in each hole puts as much of the current fragment as
> belongs there (taking care to carefully update the hole list as
> appropriate), it's not worth the complexity: In real, non-malicious
> scenarios, one never gets overlapping fragments, and certainly not
> fragments that would be supersets of one another.
> 
> So instead opt for this simple protection: Simply don't allow the
> eventual memcpy() to write beyond the last_byte of the current hole.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/6] broken CVE fix (b85d130ea0ca)

[Rasmus Villemoes]

On 14/10/2022 19.43, Rasmus Villemoes wrote:
> tl;dr: b85d130ea0ca didn't fix the CVE(s), but did break tftp of
> certain file sizes - which is somewhat lucky, since that's how I
> noticed in the first place.
> 

At this point it seems unlikely that any more comments or reviews will
come, so perhaps its time to get these (all 7) merged to master, so that
they will get some wider testing before the January release?

Rasmus

Re: [PATCH 0/6] broken CVE fix (b85d130ea0ca)

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

On Mon, Nov 14, 2022 at 10:35:51AM +0100, Rasmus Villemoes wrote:
> On 14/10/2022 19.43, Rasmus Villemoes wrote:
> > tl;dr: b85d130ea0ca didn't fix the CVE(s), but did break tftp of
> > certain file sizes - which is somewhat lucky, since that's how I
> > noticed in the first place.
> > 
> 
> At this point it seems unlikely that any more comments or reviews will
> come, so perhaps its time to get these (all 7) merged to master, so that
> they will get some wider testing before the January release?

Yes, I'd like to see a net PR with this and perhaps a few other mature
things?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/6] broken CVE fix (b85d130ea0ca)

[Fabio Estevam]

On Mon, Nov 14, 2022 at 10:04 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Nov 14, 2022 at 10:35:51AM +0100, Rasmus Villemoes wrote:
> > On 14/10/2022 19.43, Rasmus Villemoes wrote:
> > > tl;dr: b85d130ea0ca didn't fix the CVE(s), but did break tftp of
> > > certain file sizes - which is somewhat lucky, since that's how I
> > > noticed in the first place.
> > >
> >
> > At this point it seems unlikely that any more comments or reviews will
> > come, so perhaps its time to get these (all 7) merged to master, so that
> > they will get some wider testing before the January release?
>
> Yes, I'd like to see a net PR with this and perhaps a few other mature
> things?

Ramon, Joe?

Re: [PATCH 0/6] broken CVE fix (b85d130ea0ca)

[Rasmus Villemoes]

On 17/11/2022 01.32, Fabio Estevam wrote:
> On Mon, Nov 14, 2022 at 10:04 AM Tom Rini <trini@konsulko.com> wrote:
>>
>> On Mon, Nov 14, 2022 at 10:35:51AM +0100, Rasmus Villemoes wrote:
>>> On 14/10/2022 19.43, Rasmus Villemoes wrote:
>>>> tl;dr: b85d130ea0ca didn't fix the CVE(s), but did break tftp of
>>>> certain file sizes - which is somewhat lucky, since that's how I
>>>> noticed in the first place.
>>>>
>>>
>>> At this point it seems unlikely that any more comments or reviews will
>>> come, so perhaps its time to get these (all 7) merged to master, so that
>>> they will get some wider testing before the January release?
>>
>> Yes, I'd like to see a net PR with this and perhaps a few other mature
>> things?
> 
> Ramon, Joe?

Ping. If those two CVEs and the tftp brokenness are to get fixed in
2023.01, now is the time to pull in these patches, or provide a viable
alternative.