[PATCH 0/6] broken CVE fix (b85d130ea0ca)
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(-)
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)) {
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
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!
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);
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!
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;
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!
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,
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!
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);
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.
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
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!
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);
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
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!
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
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 */
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!
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
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?
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?
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.
participants (4)
-
Fabio Estevam -
Ramon Fried -
Rasmus Villemoes -
Tom Rini