From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web11.512.1598979143541997201 for ; Tue, 01 Sep 2020 09:52:23 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=iL0p+90H; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1598979142; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=mZCbhZ62xdDzIW6pDs/M9KvnqRsHgKzf+qFxpIBCNxk=; b=iL0p+90H7YdRYy8AEnxD25/9pCz9KdumfkJe6fy3EHQlX+BEPjQFlW97CeR3ehKWwguY5r kea2y9JQpP7nRznSSDnMqCyacD6zofx/uKnpHHjz5ktxJ830yGdveIsDSzBEG70JLt+TGG awabxT2tLaxKMVzNv2VYNyplPLuzGZk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-94-7t4FOSUyMbWtFMLPEjuDkg-1; Tue, 01 Sep 2020 12:52:14 -0400 X-MC-Unique: 7t4FOSUyMbWtFMLPEjuDkg-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 0B7B618A225F; Tue, 1 Sep 2020 16:52:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-251.ams2.redhat.com [10.36.112.251]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7941B7B931; Tue, 1 Sep 2020 16:52:11 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562) To: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , devel@edk2.groups.io Cc: Jian J Wang , Jiewen Yao , Min Xu , Wenyi Xie References: <20200901091221.20948-1-lersek@redhat.com> <20200901091221.20948-4-lersek@redhat.com> <7fae1361-e773-bb0f-21c8-fd548b4bbdab@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 1 Sep 2020 18:52:10 +0200 MIME-Version: 1.0 In-Reply-To: <7fae1361-e773-bb0f-21c8-fd548b4bbdab@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0.002 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-Language: en-US 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 >> Cc: Jiewen Yao >> Cc: Min Xu >> Cc: Wenyi Xie >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215 >> Signed-off-by: Laszlo Ersek >> --- >> 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 Thanks! Laszlo > >> break; >> } >> >> >