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;