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.w...@intel.com>
>> Cc: Jiewen Yao <jiewen....@intel.com>
>> Cc: Min Xu <min.m...@intel.com>
>> Cc: Wenyi Xie <xiewen...@huawei.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
>> Signed-off-by: Laszlo Ersek <ler...@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 <phi...@redhat.com>
Thanks!
Laszlo
>
>> break;
>> }
>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#64908): https://edk2.groups.io/g/devel/message/64908
Mute This Topic: https://groups.io/mt/76552541/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
