From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1p5ZWS-00081f-7b for mharc-grub-devel@gnu.org; Wed, 14 Dec 2022 16:42:40 -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 1p5ZWR-00081T-8j for grub-devel@gnu.org; Wed, 14 Dec 2022 16:42:39 -0500 Received: from mout.gmx.net ([212.227.15.19]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p5ZWP-0004dT-Hh for grub-devel@gnu.org; Wed, 14 Dec 2022 16:42:39 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=s31663417; t=1671054135; bh=fg+agc3S+iFwd9D7yn4TFGPU2NhGnvDHzUR6zdCpam8=; h=X-UI-Sender-Class:Date:From:To:Subject:Cc:References:In-Reply-To; b=oynf2iwdJVp2UrXR2idwPX/3O8incssLFn0LlgVDlGMOy9fHwXTKf2QtSSdvKcs2W m68L7W4wGqCbdiH2jwdGFos+STDago+8kPGWBL8RX/evFUlgYF17Z3HxcbHaDslMpX tBWMaTnggm8BvIFdUSEKwIB2f7HX5xOwUxDmo/sXAsNpmyRO+Bq64jO+laRCuZhqmG h5W9XSsX6tocWesvENOh8OhDpyF70YikDbclm9CUFuSla+leaSAFjm8o9MhsLXjnWz A6QMU1BSy4wLi2ZjTb9F6io4W8QsDqquLcQ41wOOKbkwFm1tVzgNZU/ficvMgnXRfv CECK2tZmJ3ruQ== 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 1N4zAs-1oxyqZ1tP2-010siB; Wed, 14 Dec 2022 22:42:15 +0100 Date: Wed, 14 Dec 2022 22:42:55 +0100 From: "Thomas Schmitt" To: grub-devel@gnu.org Subject: Re: [PATCH 0/4] fs/iso9660: Fix out-of-bounds read Content-Type: text/plain; charset="utf-8" Cc: lidong.chen@oracle.com, fengtao40@huawei.com, yanan@huawei.com, daniel.kiper@oracle.com, lichenca2005@gmail.com References: In-Reply-To: Message-Id: <21224390031459650913@scdbackup.webframe.org> X-Provags-ID: V03:K1:E01W9VVOiry2azPk3uu/l1zfKDzpYClUfy/bjsth3Se+Jo2LT2J Im4d48IeYzeTrL3GbZs4aMxzGeHvPnSYdFP0x/OQZ0a9ji3+AsecY2pFNFkU7ttKuxy8cLH CG42i0jb1fUxq96/brxwmMYylvDNUhqRogDCZzH62Ev3aNRXkWBIjJ/G/JyDuMApY8BfmER qKmhmnF1pNPZHAoxNsiPQ== UI-OutboundReport: notjunk:1;M01:P0:VN4dkULS4P0=;VSyqL5ToX6TVl4OgAGJX8uihDZh AWXo3KKIQLnMF4D7LNHxKu7CXU0ZL9/6jVy5RyGVdejKG1R8XxS6wswSEbiuGpQaF7i2uiw2B vWQxmsdD3g9oh46Y7RVlzQicISbSYC8PU8SPUnhp8p2plgZC4e5U3Ism0UMCp7umDTWzWxLE/ pw8oac+zHq/T6xwYZZyIaX6lj+ERQBLlT7Ls+s4RJSvYJpCMT9CvSocd7QQmjbguhhuIlH9ur o5mLd7C3vsmZR+qKO+xM5JQsMMG4eT59LfHdI9toNvXM8zGldgg9s2l6a5Vr9f7GpngeY9rez pwORB3UYAtTa0Y6LdYUEXh/FaJhJfuMci3Q6eQyQGzzbQNG/TLYNECtdpxXSTwRi44iPcGv3u z0e5ODonU+/d++iPEpQTbZ/DTpS6DAhQIDZnaFPIo32TRSACKIOV9Jl5LzUJvsSRVPiAFjnUl vXtjSRD7l8WoZKPWwNAddGsmZ67wUvrr4/c/wexIHy1mewvKJpsSCW2do0BNunRq3uyaRCNp/ F9/bjI8HHFtwzB3Yy1T2I1Xwmn4wP2vxoZxQL9RHpJa4OhJI6Th0fM/h5ZmXBxtRB8N/dlLXE 76nqrm85gnYN8f4KRQoKR5C/54BfY9zaC1sn4srNoWsuopjOlRjYjm5ITNYeBCkd9Xuk43jOB P6/NP1rV8EUyH6FMaFJuNyaWNiwboUo9VobwQPQOnTehuqJaO3Xt551Vgjtky7azBkgr0UQj+ 21d5MdOk3sr/k+dxglC5fsH+1GfnD84Y/7UDrdTwa2Ne4e85a8VpVLZ+oRiXBIEfh1njVC4r9 iemOcLn0Hx4bCk5w77jZJ0qPOiJDpZ0Bk7vxaeCKW80rlDvs3IZOBdPS8OGw4uc292ZxGCtcz Qay1TScp8nbMwp1JF8wwJ8ZEOxZAKjTKKoAVb6KIJuMJcFNG49llIEqGxVMdONGALhzyAk1Xw y2tx1V+ASSTAIC39XS3Xxn0ZZgQ= Received-SPF: pass client-ip=212.227.15.19; 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, 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: Wed, 14 Dec 2022 21:42:39 -0000 Hi, i will review the patches hopefully tomorrow. But in [PATCH 2/4] and [4/4] i stumble over the statement that a SUSP entry has 5 bytes of size. This is not true. The minimum size is 4. In SUSP there are "Padding" PD (if its byte LEN_PD is 0) and "Stop" ST, which have 4 bytes. In RRIP there is "Relocated" RE. Other SUSP-compliant protocols could define 4-byte entries, too. I will have to analyze the patches whether the assumption of 5 bytes minimum can cause real harm. But i see at least two inappropriate applications of the minimum size: In [2/4] a RRIP NM entry is processed: - csize = entry->len - 5; + csize = entry->len - GRUB_ISO9660_SUSP_HEADER_SZ; The number 5 is meant to skip over the 4 bytes of SUSP header and the one byte of "FLAGS" to reach to the "NAME CONTENT" bytes. This is specific to NM (version 1, to be exacting), not to SUSP in general. I propose to leave the 5 as is. In [4/4] a RRIP SL entry is processed: - /* The symlink is not stored as a POSIX symlink, translate it. */ - while (pos + sizeof (*entry) < entry->len) + /* The symlink is not stored as a POSIX symlink, translate it. */ + while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ) < entry->len) At least in a quick test in GNU/Linux userspace struct grub_iso9660_susp_entry { uint8_t sig[2]; uint8_t len; uint8_t version; uint8_t data[0]; }; has sizeof 4, not 5. So i see the risk that this change alters program behavior in situations where we don't perceive a problem yet. It is too late in the evening to analyze what sizeof(*entry) has to do with reading the component records of SL. The component records are the components of the link target path with 2 bytes header plus the name bytes. So a size of 3 is plausible like in .../b/... Even a size of 2 is not totally illegal, as Linux allows paths like ...//.... Have a nice day :) Thomas