From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1pDpBK-0001xA-HF for mharc-grub-devel@gnu.org; Fri, 06 Jan 2023 11:02:58 -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 1pDpBF-0001wu-QU for grub-devel@gnu.org; Fri, 06 Jan 2023 11:02:56 -0500 Received: from mout.gmx.net ([212.227.15.18]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pDpBB-0007EF-Nm for grub-devel@gnu.org; Fri, 06 Jan 2023 11:02:53 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=s31663417; t=1673020935; bh=VxeaX+o2zeIxhw5v59/vES98G0CHyYnXQiz/pnGLW3M=; h=X-UI-Sender-Class:Date:From:To:Subject:Cc:References:In-Reply-To; b=nAE4aQ7aRmDFcHJ/yK5WCcbfyC3qs4Hds7kWzQuzfHbGZ008FEgoZMKv9hl9ZXGk1 J84T7XneHOAcmcituBpfhGNYJE0D4yazgX1JHmETEXntWqSzDKrJRd/WgJ/l6TvAE1 2QFnsoi6UaHdyrZG0bVYx4kSJkH6pP0uxHwEToCFX2kclhiflKLYeUKuQ1SXOFy7qX lgNfTTHyKAnW9IjWtbu8/Hocz79Pvg4BMRAj3evNy1pSbQDV+J9P3oAaWaMftLI6j7 5712xLqDKPysPL4k2NcK2NfA7scjT2NabAwyV8dhxvcCgSHL3767YFoCVECAohYVDG uHE/YG505UodQ== X-UI-Sender-Class: 724b4f7f-cbec-4199-ad4e-598c01a50d3a Received: from scdbackup.webframe.org ([84.179.236.73]) by mail.gmx.net (mrgmx005 [212.227.17.190]) with ESMTPSA (Nemesis) id 1MTRR0-1pLUhX25pE-00TjRD; Fri, 06 Jan 2023 17:02:15 +0100 Date: Fri, 06 Jan 2023 17:00:52 +0100 From: "Thomas Schmitt" To: grub-devel@gnu.org Subject: Re: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation 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: <2988551A-FABB-4DA9-897D-2E4D9A7AAB47@ORACLE.COM> In-Reply-To: <2988551A-FABB-4DA9-897D-2E4D9A7AAB47@ORACLE.COM> Message-Id: <21649387804808124315@scdbackup.webframe.org> X-Provags-ID: V03:K1:DyToVO/UtsNUgz0axak84zTsWdA+oRJZIGDMNkr9lG/gtTH+Pqj 9pOcYbDH3h5vj+GvNyAZsqzO5tz/dp+22V8Vg+AedW9ijcqQ4w6Etef0N2K6D6dA1qc7vKh Z8xNLEoaEir5haXtqDNlDOZ8LQ4EsvpuKEPld5oa+zOgTqSUkGQdBqPBvqfqBcHooNcv7j5 i3veqUalbpkd5f4xZxzOQ== UI-OutboundReport: notjunk:1;M01:P0:dzs2csFJa6Y=;qL9oRA0v5XcdGHi97Z+MOyUrNfl fsmi6dj3XzP77mdGk+K4XHuCrR08PXDO1rvylLRQzjYI4WLJNp7h/vj8GK3bxbAtnB9QvF07S NaLrmstu5KzlUMJpRmTjCBwMG8yBHL8+pJPp3TfGuPk34bEXH0QCKKglIMnzXC18HgvYKIcxG S3jpb/lKpRFAuoSNfTsynal/f+d6d6nA7yCaaZBxm84lqx+q3FsWGUicpQFh0M/DxXhMqWTga sBiVzH7hb42HwvkxeW2ASwwdUlR9yuSx8drVRR0fScpU1UarLV9zveWG39uJjpwdjJCc8Nw3F jJ4fU4ZhGQ5xq839zITLZKdIgymGkQ5+B+RPNwiotdopTXOGrui+drxzqvX4OhTxCZLUzLK30 djxzkXpo+10UWtdFd5aouq5ECfNMRVUpSLzFmbAiRZDZ4Jkk+8Xb/ZEP96qtRAuMfXvzk6E82 Vc7xuXj+e3UkOW+tTszLmXip+XtbNYKlo3ukvmf8K2qS7gvQn7PbRIL9p2hHvwef3HNG6jLcJ xGxWGTxub3YKfyFkGpvURtMjXmzyqmY1DL8c7otQDi5Sa8Q7BXRgTMUp/rciW2KazKMA8Z8wq xUjrs+4Rta+W0p3RnChVx9IEvgZt8tCaiRXBQF6qdoG0C0BCJ4+E3cR2/lRDieoDlZDG9Pfgj 1HjwiMn4dkH9VMlc5cok2rqPcos73wxfbDtietKucsFLrI054Z7CR3CFp9nIFUlXXuRW5QGrX UnLUC5kOmEBlce0nIbtPdKRmxWK2MYp5QaX5TEeFUid7Ml5XvRC/Dv2EzIiS1QsymWexbY09q 9GB+u3Mbwhuh8OFsUUzkD8VqGsHKPWmWd+0+s3aweOGmDw+mP0nYGL68k1Id8uJPFbcMn5WG7 PqJ6UkR975GSjEoxmPE9WheJ0zp+zzavInCjjZMXCquBNqJ8xLE7ksy/n8eNF8OKIlP7pqCZ1 A0fIxk9PX25xe/XkV/JDMNUFDko= Received-SPF: pass client-ip=212.227.15.18; 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: Fri, 06 Jan 2023 16:02:56 -0000 Hi, i wrote on Fri, 16 Dec 2022 10:42:13 +0100: > > If processing of a SUSP CE entry leads to a continuation area which be= gins > > by entry CE or ST, then these entries were skipped without interpretat= ion. > > In case of CE this would lead to premature end of processing the SUSP > > entries of the file. > > In case of ST this could cause following non-SUSP bytes to be interpre= ted > > as SUSP entries. > > [...] > > } > > > > entry =3D (struct grub_iso9660_susp_entry *) sua; > > + > > + if (grub_strncmp ((char *) entry->sig, "CE", 2) =3D=3D 0 > > + || grub_strncmp ((char *) entry->sig, "ST", 2) =3D=3D 0) > > + /* The hook function will not process CE or ST. > > + Advancing to the next entry would skip them. */ > > + continue; > > } > > > > if (hook (entry, hook_arg)) I wrote on Fri, 16 Dec 2022 13:57:04 +0100: > > i realize that my previous proposal opens a possibility for regression > > with a very bad ISO image. > > The danger is in an endless loop by a CE entry which points to itself. > > [...] So i now propose: > > } > > > > entry =3D (struct grub_iso9660_susp_entry *) sua; > > + > > + /* The hook function will not process CE or ST. > > + Advancing to the next entry would skip them. */ > > + if (grub_strncmp ((char *) entry->sig, "CE", 2) =3D=3D 0) > > + { > > + ce =3D (struct grub_iso9660_susp_ce *) entry; > > + if (ce_block > > + !=3D grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2= _BLKSZ > > + || off !=3D grub_le_to_cpu32 (ce->off)) > > + continue; > > + /* Ending up here indicates an endless loop by self refe= rence. > > + So skip this bad CE as was done before december 2022.= */ > > + } > > + if (grub_strncmp ((char *) entry->sig, "ST", 2) =3D=3D 0) > > + break; > > } > > > > if (hook (entry, hook_arg)) Lidong Chen wrote: > In the original the CE code, =E2=80=98off=E2=80=99 and =E2=80=98ce_block= =E2=80=99 were assigned with=C2=A0 > the values (highlighted below) that the above suggested fix tries to=C2= =A0 > check against. So, it looks like it will never end here. > =C2=A0 =C2=A0 =C2=A0 =C2=A0=C2=A0off =3D grub_le_to_cpu32 (ce->off); > =C2=A0 =C2=A0 =C2=A0 =C2=A0=C2=A0ce_block =3D grub_le_to_cpu32 (ce->blk)= << GRUB_ISO9660_LOG2_BLKSZ; This happens before "entry" gets pointed to newly allocated memory which then gets filled with the data from the continuation area grub_free (sua); sua =3D grub_malloc (sua_size); if (!sua) return grub_errno; /* Load a part of the System Usage Area. */ err =3D grub_disk_read (node->data->disk, ce_block, off, sua_size, sua); if (err) { grub_free (sua); return err; } entry =3D (struct grub_iso9660_susp_entry *) sua; Afterwards my proposed check shall peek ahead whether the suspicious new CE at the begin of this continuation area is a simple hoax which points to itself. =2D----------------------------------------------------------------------- But meanwhile i think that a full fledged test for an endless loop is more appropriate. E.g. by a counter which breaks the loop: =2D-- grub-core/fs/iso9660.c.lidong_chen_patch_2 2022-12-15 11:46:50.65429= 5924 +0100 +++ grub-core/fs/iso9660.c.lidong_chen_patch_2_ce_fix_v3 2023-01-06 16:31:= 30.226698552 +0100 @@ -176,6 +176,8 @@ enum FLAG_MORE_EXTENTS =3D 0x80 }; +#define GRUB_ISO9660_MAX_CE_HOPS 1000000 + static grub_dl_t my_mod; =0C @@ -270,6 +272,7 @@ grub_iso9660_susp_iterate (grub_fshelp_n char *sua; struct grub_iso9660_susp_entry *entry; grub_err_t err; + int ce_counter =3D 0; if (sua_size <=3D 0) return GRUB_ERR_NONE; @@ -307,6 +310,13 @@ grub_iso9660_susp_iterate (grub_fshelp_n struct grub_iso9660_susp_ce *ce; grub_disk_addr_t ce_block; + if (++ce_counter > GRUB_ISO9660_MAX_CE_HOPS) + { + grub_free (sua); + return grub_error (GRUB_ERR_BAD_FS, + "suspecting endless CE loop"); + } + ce =3D (struct grub_iso9660_susp_ce *) entry; sua_size =3D grub_le_to_cpu32 (ce->len); off =3D grub_le_to_cpu32 (ce->off); I don't add a signed-off-by because this is uncompiled and still needs thought about the maximum number of continuation hops and the reaction to a detected overflow of that number. Who does that work is the author of the patch. (1 million suffices for 2048 million bytes of SUSP data per file. You could silently bail out if this number is surpassed.) The check would be a separate patch, accompanied by my proposal v1 which then would need no own safety check: @@ -336,6 +346,12 @@ grub_iso9660_susp_iterate (grub_fshelp_n } entry =3D (struct grub_iso9660_susp_entry *) sua; + + if (grub_strncmp ((char *) entry->sig, "CE", 2) =3D=3D 0 + || grub_strncmp ((char *) entry->sig, "ST", 2) =3D=3D 0) + /* The hook function will not process CE or ST. + Advancing to the next entry would skip them. */ + continue; } if (hook (entry, hook_arg)) (This one is already signed off by me.) Have a nice dday :) Thomas