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

[Tom Rini <trini@konsulko.com>]

[-- 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 --]