Re: [PATCH 0/3] UDF partition driver fix

["Yao, Jiewen" <jiewen.yao@intel.com>]

Thank you Laszlo. You analysis is great!

Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Sunday, September 17, 2017 6:07 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; Paulo Alcantara <pcacjr@zytor.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
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 = 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel