From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 71F6A21E945E3 for ; Sun, 17 Sep 2017 06:18:21 -0700 (PDT) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga105.jf.intel.com with ESMTP; 17 Sep 2017 06:21:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,407,1500966000"; d="scan'208,217";a="152787369" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga006.fm.intel.com with ESMTP; 17 Sep 2017 06:21:23 -0700 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 17 Sep 2017 06:21:23 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 17 Sep 2017 06:21:22 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.175]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.213]) with mapi id 14.03.0319.002; Sun, 17 Sep 2017 21:21:20 +0800 From: "Yao, Jiewen" To: Laszlo Ersek , Paulo Alcantara CC: "Ni, Ruiyu" , "Dong, Eric" , "edk2-devel@lists.01.org" , "Gao, Liming" , "Kinney, Michael D" , "Zeng, Star" Thread-Topic: [edk2] [PATCH 0/3] UDF partition driver fix Thread-Index: AQHTLzRrxmS909xN8k6aPBu8ozZP6KK3jn4AgACSPECAADRKgIAAu7Yw Date: Sun, 17 Sep 2017 13:21:19 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A9BC8D8@shsmsx102.ccr.corp.intel.com> References: <4d74e669-964e-7910-2de6-b7e831f9c2eb@redhat.com> <74D8A39837DF1E4DA445A8C0B3885C503A9BC765@shsmsx102.ccr.corp.intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.22 Subject: Re: [PATCH 0/3] UDF partition driver fix X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 17 Sep 2017 13:18:21 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thank you Laszlo. You analysis is great! Thank you Yao Jiewen From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Lasz= lo Ersek Sent: Sunday, September 17, 2017 6:07 PM To: Yao, Jiewen ; Paulo Alcantara Cc: Ni, Ruiyu ; Dong, Eric ; edk2-= devel@lists.01.org; Gao, Liming ; Kinney, Michael D <= michael.d.kinney@intel.com>; Zeng, Star Subject: Re: [edk2] [PATCH 0/3] UDF partition driver fix Hi Jiewen, I agree these are important questions; even earlier I noticed the following remark in "PartitionDxe.inf": # Caution: This module requires additional review when modified. # This driver will have external input - disk partition. # This external input must be validated carefully to avoid security issue = like # buffer overflow, integer overflow. I'll let Paulo answer your questions. I'll just comment on one thing because your specific question refers to code that I recommended. On 09/17/17 01:52, Yao, Jiewen wrote: > 4) The last but not least important, which negative test > (security test) has been done? > Have you run some fuzzing test? > Have you tried a mal-format UDF disc? For example, with bad offset or > length? > Have you test the integer overflow / buffer over flow handing in the > code? > > NOTE: we should not use ASSERT to handle such error. > When I review the code, I found below: > > Status =3D ReadFileData ( > BlockIo, > DiskIo, > Volume, > Parent, > PrivFileData->FileSize, > &PrivFileData->FilePosition, > Buffer, > &BufferSizeUint64 > ); > ASSERT (BufferSizeUint64 <=3D MAX_UINTN); > *BufferSize =3D (UINTN)BufferSizeUint64; > > I am not sure if we can use ASSERT (BufferSizeUint64 <=3D MAX_UINTN); > Can a malicious user construct a bad UDF and make BufferSizeUint64 > > MAX_UINTN? > Does it might happen? Or never happen? > > We documented Appendix B - EDKII code review top 5 in > https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Bey= ond_BIOS_Security_Design_Guide_in_EDK_II.pdf > 3 of them are matched in these partition and file system drivers. I > quote below: > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D > If the code consumes input from an untrusted source or region, > Ensure that the input is rigorously and adequately validated. > Verify buffer overflow is handled. Avoid integer overflow. > Try to use subtraction instead of addition and division instead of > multiplication. > Verify that ASSERT is used properly. > ASSERT is used to catch conditions that should never happen. If > something might happen, use error handling instead. We can use both > ASSERT and error handling to facilitate debugging - ASSERT allows for > earlier detection and isolation of several classes of issues. > [McConnell] > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D You can find the discussion about the code above here: http://mid.mail-archive.com/8ca69ac9-4f5a-3aa9-f150-844b0aeeb898@redhat.com I can describe it in more detail here: The UdfRead() function, which implements EFI_FILE_PROTOCOL.Read(), takes an IN OUT parameter called BufferSize: typedef EFI_STATUS (EFIAPI *EFI_FILE_READ)( IN EFI_FILE_PROTOCOL *This, IN OUT UINTN *BufferSize, OUT VOID *Buffer ); BufferSize points to an UINTN variable. On input BufferSize says how much data the caller is trying to read, and on output it tells the caller how much data was actually read. In the UdfRead() function, we pass the pointer to a helper function called ReadFileData(). ReadFileData() takes a similar IN OUT BufferSize parameter, but in ReadFileData() the pointer is to UINT64, not UINTN: EFI_STATUS ReadFileData ( IN EFI_BLOCK_IO_PROTOCOL *BlockIo, IN EFI_DISK_IO_PROTOCOL *DiskIo, IN UDF_VOLUME_INFO *Volume, IN UDF_FILE_INFO *File, IN UINT64 FileSize, IN OUT UINT64 *FilePosition, IN OUT VOID *Buffer, IN OUT UINT64 *BufferSize ) Therefore we cannot pass the original pointer directly, because in 32-bit builds, ReadFileData() would access 64 bits through the pointer, even though the caller of UdfRead() originally allocated only 32 bits for the outermost BufferSize variable. Therefore, in UdfRead(), we have a local variable UINT64 BufferSizeUint64; and we use it like this: BufferSizeUint64 =3D *BufferSize; Status =3D ReadFileData ( BlockIo, DiskIo, Volume, Parent, PrivFileData->FileSize, &PrivFileData->FilePosition, Buffer, &BufferSizeUint64 ); ASSERT (BufferSizeUint64 <=3D MAX_UINTN); *BufferSize =3D (UINTN)BufferSizeUint64; First we set the helper variable to *BufferSize, from the caller. In 64-bit builds, UINTN is UINT64, hence this is a UINT64-to-UINT64 assignment. In 32-bit builds, UINTN is UINT32, hence this is a UINT32-to-UINT64 assignment. Then we call ReadFileData() with a pointer to the helper variable. This is safe because the helper variable has enough room (64 bits) so that ReadFileData() will not access data beyond it. Finally, we must put back the result from BufferSizeUint64, to the caller's (*BufferSize) object. In 64-bit builds, this is a UINT64-to-UINT64 assignment, so that is safe. However, in 32-bit builds, this is a UINT64-to-UINT32 assignment, and we must prevent the value from being truncated. The insight here is that ReadFileData() must never *increase* the value -- it may read exactly as much as, or less than, the amount of data requested, but it must never read more than requested. Therefore, given that the input value of BufferSizeUint64 was *BufferSize, i.e., a UINTN, ReadFileData() guarantees that the output value, which may never be larger than the input value, will also fit into a UINTN. This is why the ASSERT() is appropriate -- if this invariant were broken, then it would not be a consequence of user input (that is, not caused by a user-provided UDF filesystem), but a bug in ReadFileData(). The ASSERT() states that the conversion to (UINTN) that comes right after is safe, because it should never occur that ReadFileData() read more data than requested. The ASSERT() doesn't concern user input -- i.e., filesystem data, or EFI_FILE_PROTOCOL.Read() parameters --, it concerns the interface contract of the internal ReadFileData() function. If the ASSERT() ever fires, then there's a bug in ReadFileData(). Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel