From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 AFB7E21D492F7 for ; Sun, 17 Sep 2017 03:04:32 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 644A0356C7; Sun, 17 Sep 2017 10:07:34 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 644A0356C7 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-8.rdu2.redhat.com [10.10.120.8]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4C8085D96A; Sun, 17 Sep 2017 10:07:30 +0000 (UTC) To: "Yao, Jiewen" , Paulo Alcantara Cc: "Ni, Ruiyu" , "Dong, Eric" , "edk2-devel@lists.01.org" , "Gao, Liming" , "Kinney, Michael D" , "Zeng, Star" References: <4d74e669-964e-7910-2de6-b7e831f9c2eb@redhat.com> <74D8A39837DF1E4DA445A8C0B3885C503A9BC765@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: Date: Sun, 17 Sep 2017 12:07:29 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503A9BC765@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Sun, 17 Sep 2017 10:07:34 +0000 (UTC) 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 10:04:32 -0000 Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 = ReadFileData ( > BlockIo, > DiskIo, > Volume, > Parent, > PrivFileData->FileSize, > &PrivFileData->FilePosition, > Buffer, > &BufferSizeUint64 > ); > ASSERT (BufferSizeUint64 <= MAX_UINTN); > *BufferSize = (UINTN)BufferSizeUint64; > > I am not sure if we can use ASSERT (BufferSizeUint64 <= 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_Beyond_BIOS_Security_Design_Guide_in_EDK_II.pdf > 3 of them are matched in these partition and file system drivers. I > quote below: > =============================== > 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] > =============================== 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 = *BufferSize; Status = ReadFileData ( BlockIo, DiskIo, Volume, Parent, PrivFileData->FileSize, &PrivFileData->FilePosition, Buffer, &BufferSizeUint64 ); ASSERT (BufferSizeUint64 <= MAX_UINTN); *BufferSize = (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