From b0fcc48cb37057ccbe29481d3297f7b9243a4b92 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Fri, 14 Oct 2022 19:43:37 +0200 Subject: net: improve check for no IP options 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 Reviewed-by: Ramon Fried --- net/net.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'net/net.c') diff --git a/net/net.c b/net/net.c index b27b021d071..be4374ffc47 100644 --- a/net/net.c +++ b/net/net.c @@ -1226,7 +1226,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)) { -- cgit v1.2.3 From ad359d89ec5424fc18a289fa5fcc1a4947930dba Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Fri, 14 Oct 2022 19:43:38 +0200 Subject: net: compare received length to sizeof(ip_hdr), not sizeof(ip_udp_hdr) 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 --- net/net.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'net/net.c') diff --git a/net/net.c b/net/net.c index be4374ffc47..434c3b411ec 100644 --- a/net/net.c +++ b/net/net.c @@ -1208,9 +1208,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 */ @@ -1219,6 +1219,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); -- cgit v1.2.3 From 1817c3824a08bbad7fd2fbae1a6e73be896e8e5e Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Fri, 14 Oct 2022 19:43:39 +0200 Subject: net: (actually/better) deal with CVE-2022-{30790,30552} 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 --- net/net.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'net/net.c') diff --git a/net/net.c b/net/net.c index 434c3b411ec..987c25931e0 100644 --- a/net/net.c +++ b/net/net.c @@ -924,7 +924,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 */ @@ -934,6 +938,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; -- cgit v1.2.3 From 06653c701040f34e05d587bf14c2600f8cb3460f Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Fri, 14 Oct 2022 19:43:40 +0200 Subject: net: fix ip_len in reassembled IP datagram 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 --- net/net.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'net/net.c') diff --git a/net/net.c b/net/net.c index 987c25931e0..073fb681e55 100644 --- a/net/net.c +++ b/net/net.c @@ -1040,8 +1040,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; } @@ -1289,7 +1289,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, -- cgit v1.2.3 From 068696863980f86504934bdb1e4d0d6359b76092 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Mon, 17 Oct 2022 09:52:51 +0200 Subject: net: deal with fragment-overlapping-two-holes case 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 --- net/net.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) (limited to 'net/net.c') diff --git a/net/net.c b/net/net.c index 073fb681e55..6f0a48361c9 100644 --- a/net/net.c +++ b/net/net.c @@ -985,10 +985,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 */ -- cgit v1.2.3 From a3bf193bf4ea8703bcf96b1a34713fb2ae87aa39 Mon Sep 17 00:00:00 2001 From: "Ying-Chun Liu (PaulLiu)" Date: Tue, 8 Nov 2022 14:17:28 +0800 Subject: net: Add TCP protocol Currently file transfers are done using tftp or NFS both over udp. This requires a request to be sent from client (u-boot) to the boot server. The current standard is TCP with selective acknowledgment. Signed-off-by: Duncan Hare Signed-off-by: Duncan Hare Signed-off-by: Ying-Chun Liu (PaulLiu) Reviewed-by: Simon Glass Cc: Christian Gmeiner Cc: Joe Hershberger Cc: Michal Simek Cc: Ramon Fried Reviewed-by: Ramon Fried --- net/net.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) (limited to 'net/net.c') diff --git a/net/net.c b/net/net.c index 6f0a48361c9..9cb2aab09de 100644 --- a/net/net.c +++ b/net/net.c @@ -117,6 +117,7 @@ #if defined(CONFIG_CMD_WOL) #include "wol.h" #endif +#include /** BOOTP EXTENTIONS **/ @@ -387,6 +388,8 @@ int net_init(void) /* Only need to setup buffer pointers once. */ first_call = 0; + if (IS_ENABLED(CONFIG_PROT_TCP)) + tcp_set_tcp_state(TCP_CLOSED); } return net_init_loop(); @@ -833,6 +836,16 @@ int net_send_udp_packet(uchar *ether, struct in_addr dest, int dport, int sport, IPPROTO_UDP, 0, 0, 0); } +#if defined(CONFIG_PROT_TCP) +int net_send_tcp_packet(int payload_len, int dport, int sport, u8 action, + u32 tcp_seq_num, u32 tcp_ack_num) +{ + return net_send_ip_packet(net_server_ethaddr, net_server_ip, dport, + sport, payload_len, IPPROTO_TCP, action, + tcp_seq_num, tcp_ack_num); +} +#endif + int net_send_ip_packet(uchar *ether, struct in_addr dest, int dport, int sport, int payload_len, int proto, u8 action, u32 tcp_seq_num, u32 tcp_ack_num) @@ -864,6 +877,14 @@ int net_send_ip_packet(uchar *ether, struct in_addr dest, int dport, int sport, payload_len); pkt_hdr_size = eth_hdr_size + IP_UDP_HDR_SIZE; break; +#if defined(CONFIG_PROT_TCP) + case IPPROTO_TCP: + pkt_hdr_size = eth_hdr_size + + tcp_set_tcp_header(pkt + eth_hdr_size, dport, sport, + payload_len, action, tcp_seq_num, + tcp_ack_num); + break; +#endif default: return -EINVAL; } @@ -1289,6 +1310,15 @@ void net_process_received_packet(uchar *in_packet, int len) if (ip->ip_p == IPPROTO_ICMP) { receive_icmp(ip, len, src_ip, et); return; +#if defined(CONFIG_PROT_TCP) + } else if (ip->ip_p == IPPROTO_TCP) { + debug_cond(DEBUG_DEV_PKT, + "TCP PH (to=%pI4, from=%pI4, len=%d)\n", + &dst_ip, &src_ip, len); + + rxhand_tcp_f((union tcp_build_pkt *)ip, len); + return; +#endif } else if (ip->ip_p != IPPROTO_UDP) { /* Only UDP packets */ return; } -- cgit v1.2.3 From cfbae48219fd81f6c9e1a7b5ee160cdd3005f958 Mon Sep 17 00:00:00 2001 From: "Ying-Chun Liu (PaulLiu)" Date: Tue, 8 Nov 2022 14:17:29 +0800 Subject: net: Add wget application This commit adds a simple wget command that can download files from http server. The command syntax is wget ${loadaddr} Signed-off-by: Duncan Hare Signed-off-by: Ying-Chun Liu (PaulLiu) Reviewed-by: Simon Glass Cc: Christian Gmeiner Cc: Joe Hershberger Cc: Michal Simek Cc: Ramon Fried Reviewed-by: Ramon Fried --- net/net.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'net/net.c') diff --git a/net/net.c b/net/net.c index 9cb2aab09de..aca20e43b0b 100644 --- a/net/net.c +++ b/net/net.c @@ -118,6 +118,7 @@ #include "wol.h" #endif #include +#include /** BOOTP EXTENTIONS **/ @@ -517,6 +518,11 @@ restart: nfs_start(); break; #endif +#if defined(CONFIG_CMD_WGET) + case WGET: + wget_start(); + break; +#endif #if defined(CONFIG_CMD_CDP) case CDP: cdp_start(); -- cgit v1.2.3