From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 572CCC4332F for ; Mon, 28 Nov 2022 19:51:58 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 7F3CE852D0; Mon, 28 Nov 2022 20:51:38 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=konsulko.com header.i=@konsulko.com header.b="Z0T7DHR5"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 43D7C852DE; Mon, 28 Nov 2022 20:51:35 +0100 (CET) Received: from mail-ej1-x633.google.com (mail-ej1-x633.google.com [IPv6:2a00:1450:4864:20::633]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 31054852EB for ; Mon, 28 Nov 2022 20:51:27 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=trini@konsulko.com Received: by mail-ej1-x633.google.com with SMTP id b2so12299339eja.7 for ; Mon, 28 Nov 2022 11:51:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=lbCG8xOBb3EoGLEwfh9b+T2zEyEuuLiYHc6TEYbJu7c=; b=Z0T7DHR5e0sKbw59aws97iRWoankLRV7F+zRvYaVAQ0+4SRChhG7d0tlmx2M77AlDX G/xDqBmRPCSPsjDxavrlKxshdnE+q9BzPWBS9nQLEoE57c8x+iUwK0FmmrELAPkL02Sk qGMyQwRX2aJUJAa3vU9dShf3CZVU2WFUr2s2g= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=lbCG8xOBb3EoGLEwfh9b+T2zEyEuuLiYHc6TEYbJu7c=; b=j3K6wkU4i+WC/KZi7RDnzq0lt2mzU1o/p6GNnxLGlcd1tW8RvW4pYI73gPVKO8QAdU cjM9ryl9ixGQqmyWPpLtg7bLLX9eTYyOdp0UMZU975a8vDipqM3gwHfaSUrs1FgE3/oD 7RD++I/2jY2XHT6i4voYwRMsmpV9wNtWGcM0PwlyHFo0F9uOM1zQP9E/HDFVxOgrbaG2 PfbPtYoK15gw5rAOYapKSfX5BdBufsRAloF+KUcGTaGESWxeMR8BExlpbA4W0/UBIhAe fySl3hUKaXDHcH5cwnCOGch9HUEZ/nlbvuM9OQuyntQJ0qO+lQyDgDoI2/y61qpIJvQP sfOA== X-Gm-Message-State: ANoB5pmRZcW61MJ261HrhAENUVdqdk50G9KnqNbJ4ffU9ht0dWc+makl NSdFnMR2wUVuZozaapPDr/QqYQ== X-Google-Smtp-Source: AA0mqf5RrDocD6QBf2mBiEhxT5V4lXI6F4/xQbNfPD/BXBOucdnT2NY8tKEvTeSHj0xxHwDYLj9+oQ== X-Received: by 2002:a17:906:7ce:b0:7b9:a74b:efbb with SMTP id m14-20020a17090607ce00b007b9a74befbbmr25489723ejc.18.1669665086779; Mon, 28 Nov 2022 11:51:26 -0800 (PST) Received: from bill-the-cat (2603-6081-7b00-6400-69da-bdfc-6a2b-e036.res6.spectrum.com. [2603:6081:7b00:6400:69da:bdfc:6a2b:e036]) by smtp.gmail.com with ESMTPSA id t32-20020a056402242000b00461bb7e7ef1sm5513332eda.30.2022.11.28.11.51.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 28 Nov 2022 11:51:26 -0800 (PST) Date: Mon, 28 Nov 2022 14:51:22 -0500 From: Tom Rini To: Rasmus Villemoes Cc: u-boot@lists.denx.de, Fabio Estevam , Nicolas Bidron , Joe Hershberger , Ramon Fried Subject: Re: [PATCH 3/6] net: (actually/better) deal with CVE-2022-{30790,30552} Message-ID: <20221128195122.GD3787616@bill-the-cat> References: <20221014174342.3216982-1-rasmus.villemoes@prevas.dk> <20221014174342.3216982-4-rasmus.villemoes@prevas.dk> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="HJ7DkX9BXQhna2ov" Content-Disposition: inline In-Reply-To: <20221014174342.3216982-4-rasmus.villemoes@prevas.dk> X-Clacks-Overhead: GNU Terry Pratchett X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean --HJ7DkX9BXQhna2ov Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > v2022.01 showed no problems with any of the files or any value of > tftpblocksize. >=20 > Looking at what had changed in net.c or tftp.c since January showed > only one remotely interesting thing, b85d130ea0ca. >=20 > 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. >=20 > 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]. >=20 > 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. >=20 > =3D=3D=3D=3D >=20 > 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. >=20 > To fix that, replace the comparison with <=3D 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 >=20 > (ip_off & (IP_OFFS | IP_FLAGS_MFRAG)). >=20 > =3D=3D=3D=3D >=20 > 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 >=20 > if ((h >=3D thisfrag) && (h->last_byte <=3D start + len)) { >=20 > or >=20 > } else if (h->last_byte <=3D start + len) { >=20 > and thus won't reach any of the >=20 > /* overlaps with initial part of the hole: move this hole */ > newh =3D thisfrag + (len / 8); >=20 > /* fragment sits in the middle: split the hole */ > newh =3D thisfrag + (len / 8); >=20 > IOW these division are now guaranteed to be exact, and thus I think > the scenario in CVE-2022-30790 cannot happen anymore. >=20 > =3D=3D=3D=3D >=20 > 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: >=20 > This code doesn't deal with a fragment that overlaps with two > different holes (thus being a superset of a previously-received > fragment). >=20 > 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: >=20 > DDDHhhhhDDDDHhhhDDDD > PPPPPPPPP >=20 > I'm pretty sure in this case we'd end up with h being the first hole, > enter the simple >=20 > } else if (h->last_byte <=3D start + len) { > /* overlaps with final part of the hole: shorten this hole */ > h->last_byte =3D start; >=20 > case, and thus in the memcpy happily overwrite the second H with our > chosen payload. This is probably worth fixing... >=20 > Signed-off-by: Rasmus Villemoes Applied to u-boot/master, thanks! --=20 Tom --HJ7DkX9BXQhna2ov Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmOFEToACgkQFHw5/5Y0 tyzo4QwAtJS9qRqL5N/gtpIJx4wgKQ+bvi7cdI/SJYFEnWixkrrJbKzApfB/LCCE STf75PZzVWRuJ+oP6dJmUrWvrKvhKmv2/SRqbW+6J+MybhgZC1VQ+7fPKKA+ouV5 dAA2/LsXAQWaw0M3o/y4cfPiuZIlG5X5YXd9MIiiVbYywrnc8ZllqhI1HEVQC0kd ZZkdcRJQ0PWK0ht48Gbow3h1M5Jqjwb5oG/V7PucHsjRSbBCAeJkcLXHsppBXhOO i+Brv0NmAMWOOodkS0/2pxNDWyFVA1kxTl9LsbQbtsuuMtSP7vhIjQ5FZSRtkzkU uscn+CxZgsHScYgvLHzzOecVpjuou/Iv9uywCmnXffwVfyNb0qqAoECtVCBFfqQO +4x+RBn/0e+/n5VCqXozTtpGux8shBITbkmP0BE9OxCt8T1brQtt3+uEMb4hY6Lm Su9VQ+8W66nfOgpeUPD4mJo/H9LOtujEqSaf0UUDBTGTnTF2Leb8D5GhyEvLSaEX 3oe/bBwD =ZASo -----END PGP SIGNATURE----- --HJ7DkX9BXQhna2ov--