From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1p5sX6-0001Jr-DM for mharc-grub-devel@gnu.org; Thu, 15 Dec 2022 13:00:36 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p5sX4-0001Ix-63 for grub-devel@gnu.org; Thu, 15 Dec 2022 13:00:34 -0500 Received: from mout.gmx.net ([212.227.17.22]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p5sX1-0008R5-Vg for grub-devel@gnu.org; Thu, 15 Dec 2022 13:00:33 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=s31663417; t=1671127213; bh=RJR4UUB09JoiZ7BJq9JqYh9ckaHI3//kP5mV/eNUfL0=; h=X-UI-Sender-Class:Date:From:To:Subject:Cc:References:In-Reply-To; b=cW2MtwdygcDHnzsqryst8bc4PU9za/tRN0BjPbq9d7N9gd5UXN+UUfhRHP4zEFC0Q YE5F8sFCXKCaExzMLvAOy36gVGbK5z3Xgs9X4HmOyToVedf6BelYcJpkaAt29o+gfi rlaK5H7g25abyy6gSW6ZRCUjuGYq5uxo5GbiuC5h3DTedvyIe916h74qpFcLUrOkG3 Xlc4s25DtS7Kc1HhjFOieOQEU+Bq+qs3nVocyGtIsKP9HkShBBjhOfXN1yk1RLtmPG g1s+ZiS7yJO3MV0eH7whomoQPzVOO98Z7PoFjq0v+wHsNwWYtHBuP4J5t7YF7X8pPK r/VKG5Iqi2R+Q== X-UI-Sender-Class: 724b4f7f-cbec-4199-ad4e-598c01a50d3a Received: from scdbackup.webframe.org ([84.179.236.73]) by mail.gmx.net (mrgmx105 [212.227.17.168]) with ESMTPSA (Nemesis) id 1MLzBp-1pN8Qe24Y5-00Hx61; Thu, 15 Dec 2022 19:00:13 +0100 Date: Thu, 15 Dec 2022 19:00:56 +0100 From: "Thomas Schmitt" To: grub-devel@gnu.org Subject: Re: [PATCH 2/4] fs/iso9660: Prevent read past the end of system use area Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Cc: lidong.chen@oracle.com, fengtao40@huawei.com, yanan@huawei.com, daniel.kiper@oracle.com, lichenca2005@gmail.com References: <307c2d0fba1b8fb12c0724425b8037d20b7aed06.1671042887.git.lidong.chen@oracle.com> In-Reply-To: <307c2d0fba1b8fb12c0724425b8037d20b7aed06.1671042887.git.lidong.chen@oracle.com> Message-Id: <16480389958628461924@scdbackup.webframe.org> X-Provags-ID: V03:K1:tJ8X2R8opQP2s/Gji+mx+FJe6Fl+LXqtLNCdIINKBcLGSVq9fGQ Jzlxl25RqbiwBi418vKeIXyVuuCI8oMbkKN2BV7gNZ6FpaXfEBq7/RbOEB/9MIdSqD+rlKF ByrCcKsOmGX9ebp7f2pXW4PCUd+snNxyuMkeNXGI4acWkp8MtxhP84HylhTk9jIDe5P4S0R SDmtF9dQhHVFcbXABs57A== UI-OutboundReport: notjunk:1;M01:P0:BTr/ASrhcxE=;GexmUBkRlpanIqyJAQ1X0VFwGlJ FyRin11H20aYUlWyfvfLKTEoNcQMW5JmzqZX1EFOi1/ARwgiapD79p5NcWeOyniVF4hsTVgwX 1ztCMPyAlJR43Qoj3+DAze7lF6cbnTXJ2dxVQ03weLOpIey5IFQ2A8nA9Dzh2YGes88zTYZBd b4c6U0R/8y+fACl/dIoSfnxAKi31gBbXWES206G6/6fVwXymDwcoRV2ys9zA1JRlb9Pi6lgxM edwYtn9PAsWYU0611g+SKSa+kMrmX2eAGjwM32DsenAMTZ+KBn/ffdWmK1sF4FyQCVh+0dS5z z3ouEmGgSd2SvC4umETcf5mwoWMtHtXKlz9JLpzocxm7+FzwVweJNvvn/ZnKu8DfFEPlFANOS jG2/TyR+n5/HYBlWVY1qSh0HfXJNidkjxGiqTGW4AuqL1am4FQlIE2p8qYl/9ZnDeF9FPRm6G cNn/dyECDfPvDdrE8aSSqrwCkBKCz9Bd+dlDnfdxLvnJKY2Gtaz/oejiwIj8FZNpU3Gg4MXlw /VLmOwT8/f9n6xxP7OdW2fX/EEcwkm7TfGL12js8lO669EfJGMmv4MHgdMsmphwQ8ogoxpAUl gUyJ++0AYqvfDtYMqo/v6PsLLuU4AE7vYXH06SA09FBK0cutjnSRe0LsnMmR1hBcbC9a4WP5o hoUG9r7YwRr+gMmlvcpKvYOp9y2CFoQmJhSGVaR9swSf5FLaMx4uwBgnc4zkTd85qPkWfFSCt 3uSQIkVUCxZUI2JcDWsMjeNJWpY5G3eGRBAM2lSRiF1aTKWEG/R40mp2GI/k+FGtu0vFupnOj 5iBroRF74I5AudtJD6P/wrU6qxfqsj0hAzPhbRVhKL0ZnIPqKR0CEp/0OF59UttIy0cG0HBXe M+23YilthpfNUmJ2/cLr8VKPGCz/9OB36A2b6wwiLTeGg36xQ/LZj//F1ACrXvhH3YsQCJQaY 2x4NtA== Received-SPF: pass client-ip=212.227.17.22; envelope-from=scdbackup@gmx.net; helo=mout.gmx.net X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Dec 2022 18:00:34 -0000 Hi, On Wed, 14 Dec 2022 18:55:03 +0000 Lidong Chen wr= ote: > In the code, the for loop advanced the entry pointer to the > next entry before checking if the next entry is within the > system use area boundary. Another issue in the code was that > there is no check for the size of system use area. For a > corrupted system, the size of system use area can be less than > the size of SUSP entry size (5 bytes). The minimum size of a SUSP entry is 4, not 5. Examples of 4-byte entries are ST in SUSP and RE in RRIP. (Ending before ST would be harmless, becaus= e ST marks the end of the SUSP entry chain. But RE may appear before the end= .) > These can cause buffer > overrun. The fixes added the checks to ensure the read is > valid and within the boundary. > > Signed-off-by: Lidong Chen > --- > grub-core/fs/iso9660.c | 31 +++++++++++++++++++++++++++---- > 1 file changed, 27 insertions(+), 4 deletions(-) > > diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c > index 4f4cd6165..9170fa820 100644 > --- a/grub-core/fs/iso9660.c > +++ b/grub-core/fs/iso9660.c > @@ -49,6 +49,8 @@ GRUB_MOD_LICENSE ("GPLv3+"); > #define GRUB_ISO9660_VOLDESC_PART 3 > #define GRUB_ISO9660_VOLDESC_END 255 > > +#define GRUB_ISO9660_SUSP_HEADER_SZ 5 So i think this should be 4, not 5. If we find reason why it should be 5, then the name SUSP_HEADER_SZ is not appropriate. The SUSP header size is surely 4. > + > /* The head of a volume descriptor. */ > struct grub_iso9660_voldesc > { > @@ -272,6 +274,9 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, = grub_off_t off, > if (sua_size <=3D 0) > return GRUB_ERR_NONE; > > + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ) > + return grub_error (GRUB_ERR_BAD_FS, "invalid susp entry size"); > + Here we need 4. > sua =3D grub_malloc (sua_size); > if (!sua) > return grub_errno; > @@ -281,10 +286,14 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node= , grub_off_t off, > if (err) > return err; > > - for (entry =3D (struct grub_iso9660_susp_entry *) sua; (char *) entry= < (char *) sua + sua_size - 1 && entry->len > 0; > - entry =3D (struct grub_iso9660_susp_entry *) > - ((char *) entry + entry->len)) > + entry =3D (struct grub_iso9660_susp_entry *) sua; > + > + while (entry->len > 0) > { > + /* Ensure the entry is within System Use Area */ > + if ((char *) entry + entry->len > (sua + sua_size)) > + break; > + > /* The last entry. */ > if (grub_strncmp ((char *) entry->sig, "ST", 2) =3D=3D 0) > break; > @@ -300,6 +309,15 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,= grub_off_t off, > off =3D grub_le_to_cpu32 (ce->off); > ce_block =3D grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ; > > + if (sua_size <=3D 0) > + break; > + > + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ) 4 would be appropriate here. > + { > + grub_free (sua); > + return grub_error (GRUB_ERR_BAD_FS, "invalid CE entry size"); > + } > + > grub_free (sua); > sua =3D grub_malloc (sua_size); > if (!sua) > @@ -319,6 +337,11 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,= grub_off_t off, > grub_free (sua); > return 0; > } > + > + entry =3D (struct grub_iso9660_susp_entry *) ((char *) entry + en= try->len); This is equivalent to the third statement in the "for" which was replaced by a while-loop. So far so good. But i believe to see an old bug. This advancing of entry breaks the chain of CE if the first entry of the Continuation Area is again a CE. err =3D grub_disk_read (node->data->disk, ce_block, off, sua_size, sua); ... entry =3D (struct grub_iso9660_susp_entry *) sua; } if (hook (entry, hook_arg)) { grub_free (sua); return 0; } entry =3D (struct grub_iso9660_susp_entry *) ((char *) entry + entry= ->len); hook() will not interpret CE (and has no means to advance "entry"). entry =3D entry + entry->len; will then skip over the entry so that the lo= op will not interpret it either. The SUSP area will be perceived as having ended, although more SUSP entrie= s are present for the file in question. I hereby ask for more opinions about this. Maybe i misinterpret the old lo= op. Background: CE points to the block and offset where the chain of SUSP entries goes on. It is needed if the SUSP entry set exceeds the size limit of 255 bytes for a directory record. The byte at (ce->blk * block_size + ce->off) is the first byte of the next SUSP entry in the chain. Linux hates SUSP crossing block boundaries. So we ISO producers use furthe= r CE entries to hop over block boundaries. libisofs can produce a Continuation Area with first and only entry CE, which then points to a new block with more SUSP payload. This happens if a continuation block offers not much more than 28 free bytes, so that only the 28 bytes of CE have room. (GRUB is not really correct by performing the CE jump immediately when CE = is seen. The specs allow more SUSP entries to follow up to the end of the directory record's SUSP range. Only after interpreting them, an encountere= d CE should cause the jump to its Continuation Area. SUSP-1.12, 5.1: "The "CE" System Use Entry indicates a Continuation Area that shall be processed after the current System Use field or Continuation Area is processed." mkisofs and libisofs both put their CE at the end of the directory record of Continuation Area. So an exotic ISO would be needed to demonstrate a problem with GRUB's immediate CE interpretation.) > + > + if (((sua + sua_size) - (char *) entry) < GRUB_ISO9660_SUSP_HEADE= R_SZ) > + break; 4 would be appropriate. > } > > grub_free (sua); > @@ -574,7 +597,7 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *en= try, > char *old; > grub_size_t sz; > > - csize =3D entry->len - 5; > + csize =3D entry->len - GRUB_ISO9660_SUSP_HEADER_SZ; Here it is not appropriate to use GRUB_ISO9660_SUSP_HEADER_SZ. This code subtracts the count of the 4 header bytes of an NM entry and of the 1 FLAGS byte of NM. Goal is to compute the size of the name string. So if GRUB_ISO9660_SUSP_HEADER_SZ is defined as 4, then (GRUB_ISO9660_SUSP_HEADER_SZ + 1) would be ok. But a plain 5 is ok, too, because this is a specific property of the NM definition of version 1. (Actually one should verify (entry->version =3D=3D 1) before interpreting an NM entry that way. Well, i see that libisofs doesn't check either. Excuse is that the RRIP specs did not change since the mid 1990s.) Whatever gets decided, the "5", which is six lines higher, should be chang= ed to the same expression: else if (entry->len >=3D 5) > old =3D ctx->filename; > if (ctx->filename_alloc) > { > -- > 2.35.1 > So no "Reviewed-by:" yet. Open issues: The definition of GRUB_ISO9660_SUSP_HEADER_SZ should be changed to 4. Within the interpretation of NM this change should not happen: > - csize =3D entry->len - 5; > + csize =3D entry->len - GRUB_ISO9660_SUSP_HEADER_SZ; We need to decide what to do with the potential old bug that a CE entry as the first entry of a Continuation Area gets skipped without interpretation. Have a nice day :) Thomas