Re: [edk2-devel] [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)

["Laszlo Ersek" <lersek@redhat.com>]

On 09/01/20 17:53, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
>
> On 9/1/20 11:12 AM, Laszlo Ersek wrote:
>> The DxeImageVerificationHandler() function currently checks whether
>> "SecDataDir" has enough room for "WinCertificate->dwLength". However, for
>> advancing "OffSet", "WinCertificate->dwLength" is aligned to the next
>> multiple of 8. If "WinCertificate->dwLength" is large enough, the
>> alignment will return 0, and "OffSet" will be stuck at the same value.
>>
>> Check whether "SecDataDir" has room left for both
>> "WinCertificate->dwLength" and the alignment.
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Wenyi Xie <xiewenyi2@huawei.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> index 100739eb3eb6..11154b6cc58a 100644
>> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> @@ -1860,7 +1860,9 @@ DxeImageVerificationHandler (
>>        break;
>>      }
>>      WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
>> -    if (SecDataDirLeft < WinCertificate->dwLength) {
>> +    if (SecDataDirLeft < WinCertificate->dwLength ||
>> +        (SecDataDirLeft - WinCertificate->dwLength <
>> +         ALIGN_SIZE (WinCertificate->dwLength))) {
>
> I dare to ask (probably again, I remember some similar boundary check
> style question once), why not as (which is simpler for me to review):
>
>        if (SecDataDirLeft < WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength)) {

Indeed, the mathematical relation that we want to catch (and reject) is:

  SecDataDirLeft < WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength)

But the problem is precisely with the addition on the right hand side.

(1) The "WinCertificate->dwLength" field is of type UINT32 (meaning, in
    standard C terms, "unsigned int").

(2) The ALIGN_SIZE() macro is defined as follows, in
    "SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h":

  #define ALIGNMENT_SIZE                    8
  #define ALIGN_SIZE(a) (((a) % ALIGNMENT_SIZE) ? ALIGNMENT_SIZE - ((a) % ALIGNMENT_SIZE) : 0)

    If you substitute "WinCertificate->dwLength" for "a", and 8 for
    ALIGNMENT_SIZE, you get the following replacement text for the macro
    invocation:

      (((WinCertificate->dwLength) % 8) ? 8 - ((WinCertificate->dwLength) % 8) : 0)

    Here the third operand of the conditional operator (that is, 0) is
    of type "int".

    The 2nd operand of the conditional operator ultimately has type
    "unsigned int". That's because the type of
    "WinCertificate->dwLength" conceptually "propagates" outwards across
    the "%" and "-" operators, via the "usual arithmetic conversions".
    Basically you have

      (unsigned int) % (int) -->
      converts the int to unsigned int, produces unsigned int

      (int) - (unsigned int) -->
      converts the int to unsigned int, produces unsigned int

    And then we have the following, for the conditional operator too
    (C99 6.5.15p5):

      If both the second and third operands have arithmetic type, the
      result type that would be determined by the usual arithmetic
      conversions, were they applied to those two operands, is the type
      of the result.

    Which means that ALIGN_SIZE (WinCertificate->dwLength) produces a
    UINT32 (unsigned int) as well.

(3) Furthermore, "SecDataDirLeft" is of type UINT32 (unsigned int) too.

(4) Ultimately, in the 2nd subcondition of the "if", we would have

      UINT32 < UINT32 + UINT32

    If the addition overflows on the right hand size (which is well
    defined behavior for "unsigned int"s), possibly to a really small
    number, then the comparison may evaluate to FALSE, even though the
    *mathematical* result would be TRUE.

    Specifically, if

      WinCertificate->dwLength >= MAX_UINT32 - 6

    then the sum

      WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength)

    wraps around to zero precisely, and then the 2nd subcondition would
    decay to

      SecDataDirLeft < 0

    which would always evaluate to FALSE.


We can avoid this by taking the mathematically relevant expression

  SecDataDirLeft < WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength)

and by subtracting "WinCertificate->dwLength" from both sides. That
yields the following (mathematically equivalent) inequality:

  SecDataDirLeft - WinCertificate->dwLength < ALIGN_SIZE (WinCertificate->dwLength)

which we can safely evaluate in C.

The reason for that "safety" is the *directly preceding* subcondition in
the "if" statement:

  if (SecDataDirLeft < WinCertificate->dwLength || ...

Namely, if this (1st) condition evaluates to TRUE, then we ignore the second
subcondition (due to the logical OR), and take the "break" at once. And
if the 1st condition evaluates to FALSE (so that we need to consider the
new, 2nd, subcondition), *then* we know:

  SecDataDirLeft >= WinCertificate->dwLength

and then just subtract "WinCertificate->dwLength" from both sides, to
realize the following mathematical assurance:

  SecDataDirLeft - WinCertificate->dwLength >= 0

Thus, the difference that is visible on the left-hand side above is
safe to *express* in the second subcondition that we actually care
about:

  SecDataDirLeft - WinCertificate->dwLength < ALIGN_SIZE (WinCertificate->dwLength)
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                   |
                   |
  this UINT32 subtraction will not
  underflow because the first subcondition
  evaluated to FALSE


In brief,

- we re-formulate the 2nd subcondition, using a subtraction in place of
  the addition, for preventing an overflow in the addition,

- and the subtraction is safe due to the 1st subcondition, which we
  check directly before.

So it's not just a style question, but a safety/security one.

> At any rate, for this patch:
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Thanks!
Laszlo

>
>>        break;
>>      }
>>
>>
>