From: "Min Xu" <min.m.xu@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
"mhaeuser@posteo.de" <mhaeuser@posteo.de>
Cc: "Yao, Jiewen" <jiewen.yao@intel.com>,
"Wang, Jian J" <jian.j.wang@intel.com>,
Vitaly Cheptsov <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [PATCH v2 2/2] SecurityPkg/SecureBootConfigDxe: Fix certificate lookup algorithm
Date: Thu, 12 Aug 2021 01:12:11 +0000 [thread overview]
Message-ID: <PH0PR11MB506465FDC82E9CD80C240B22C5F99@PH0PR11MB5064.namprd11.prod.outlook.com> (raw)
In-Reply-To: <7cedc9b336ec5410d833b4ecac53f5b366a636a5.1628501623.git.mhaeuser@posteo.de>
On August 9, 2021 5:51 PM, Marvin Häuser wrote:
> The current certificate lookup code does not check the bounds of the
> authentication data before accessing it. Abort if the header cannot fit, and
> proceed to the next hashing algortihm if the OID of the current one exceeds the
> authentication data bounds.
>
> Additionally move the two-byte encoding check out of the loop as the data is
> invariant.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
> ---
>
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigIm
> pl.c | 45 ++++++++++++--------
> 1 file changed, 28 insertions(+), 17 deletions(-)
>
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> mpl.c
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> mpl.c
> index 65a8188d6d03..fd7629f61862 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> mpl.c
> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCo
> +++ nfigImpl.c
> @@ -1969,30 +1969,41 @@ HashPeImageByType ( {
>
> UINT8 Index;
>
> WIN_CERTIFICATE_EFI_PKCS *PkcsCertData;
>
> + UINT32 AuthDataSize;
>
>
>
> PkcsCertData = (WIN_CERTIFICATE_EFI_PKCS *) (mImageBase + mSecDataDir-
> >Offset);
>
> + if (PkcsCertData->Hdr.dwLength <= sizeof (PkcsCertData->Hdr)) {
>
> + return EFI_UNSUPPORTED;
>
> + }
>
> +
>
> + AuthDataSize = PkcsCertData->Hdr.dwLength - sizeof
> + (PkcsCertData->Hdr);
>
> + if (AuthDataSize < 32) {
>
> + return EFI_UNSUPPORTED;
>
> + }
>
> + //
>
> + // Check the Hash algorithm in PE/COFF Authenticode.
>
> + // According to PKCS#7 Definition:
>
> + // SignedData ::= SEQUENCE {
>
> + // version Version,
>
> + // digestAlgorithms DigestAlgorithmIdentifiers,
>
> + // contentInfo ContentInfo,
>
> + // .... }
>
> + // The DigestAlgorithmIdentifiers can be used to determine the hash
> algorithm in PE/COFF hashing
>
> + // This field has the fixed offset (+32) in final Authenticode ASN.1 data.
>
> + // Fixed offset (+32) is calculated based on two bytes of length encoding.
>
> + //
>
> + if ((*(PkcsCertData->CertData + 1) & TWO_BYTE_ENCODE) !=
> + TWO_BYTE_ENCODE) {
>
> + //
>
> + // Only support two bytes of Long Form of Length Encoding.
>
> + //
>
> + return EFI_UNSUPPORTED;
>
> + }
>
>
>
> for (Index = 0; Index < HASHALG_MAX; Index++) {
>
> - //
>
> - // Check the Hash algorithm in PE/COFF Authenticode.
>
> - // According to PKCS#7 Definition:
>
> - // SignedData ::= SEQUENCE {
>
> - // version Version,
>
> - // digestAlgorithms DigestAlgorithmIdentifiers,
>
> - // contentInfo ContentInfo,
>
> - // .... }
>
> - // The DigestAlgorithmIdentifiers can be used to determine the hash
> algorithm in PE/COFF hashing
>
> - // This field has the fixed offset (+32) in final Authenticode ASN.1 data.
>
> - // Fixed offset (+32) is calculated based on two bytes of length encoding.
>
> - //
>
> - if ((*(PkcsCertData->CertData + 1) & TWO_BYTE_ENCODE) !=
> TWO_BYTE_ENCODE) {
>
> - //
>
> - // Only support two bytes of Long Form of Length Encoding.
>
> - //
>
> + if (AuthDataSize - 32 < mHash[Index].OidLength) {
>
> continue;
>
> }
>
>
>
> - //
>
> if (CompareMem (PkcsCertData->CertData + 32, mHash[Index].OidValue,
> mHash[Index].OidLength) == 0) {
>
> break;
>
> }
>
> --
> 2.31.1
Reviewed-by: Min Xu <min.m.xu@intel.com>
Thanks!
Xu, Min
next prev parent reply other threads:[~2021-08-12 1:12 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-09 9:51 [PATCH v2 0/7] Fix various issues regarding DebugImageInfoTable Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 1/2] BaseTools: Define the read-only data section name per toolchain Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 2/2] UefiCpuPkg/BaseUefiCpuLib: Use toolchain-specific rodata section name Marvin Häuser
2021-08-10 2:43 ` Ni, Ray
2021-08-10 4:40 ` [edk2-devel] " Andrew Fish
2021-08-10 8:43 ` Marvin Häuser
2021-08-10 4:19 ` [edk2-devel] [PATCH v2 1/2] BaseTools: Define the read-only data section name per toolchain Andrew Fish
2021-08-10 8:27 ` Marvin Häuser
2021-08-10 19:35 ` Andrew Fish
2021-08-10 21:30 ` Marvin Häuser
2021-08-10 21:58 ` Andrew Fish
2021-08-11 8:11 ` Marvin Häuser
2021-08-11 17:19 ` Andrew Fish
2021-08-12 7:26 ` Marvin Häuser
2021-08-12 20:25 ` Marvin Häuser
2021-08-12 22:53 ` Andrew Fish
[not found] ` <169AB0F8BD9C50BA.13770@groups.io>
2021-08-16 21:13 ` Andrew Fish
[not found] ` <169A090BBBBE12C1.15606@groups.io>
2021-08-10 19:49 ` Andrew Fish
2021-08-10 21:24 ` Marvin Häuser
2021-08-10 21:54 ` Andrew Fish
2021-08-09 9:51 ` [PATCH v2 1/7] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 1/2] MdePkg/BaseLib: Fix unaligned API prototypes Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 2/2] BaseTools/CommonLib: " Marvin Häuser
2021-08-09 16:15 ` [PATCH v2 1/2] MdePkg/BaseLib: " Michael D Kinney
2021-08-09 21:32 ` [edk2-devel] " Andrew Fish
2021-08-10 8:53 ` Marvin Häuser
2021-08-10 17:36 ` Andrew Fish
2021-08-10 21:14 ` Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 1/2] SecurityPkg/DxeImageVerificationLib: Fix certificate lookup algorithm Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 2/2] SecurityPkg/SecureBootConfigDxe: " Marvin Häuser
2021-08-12 1:12 ` Min Xu [this message]
2021-08-12 1:11 ` [edk2-devel] [PATCH v2 1/2] SecurityPkg/DxeImageVerificationLib: " Min Xu
2021-08-09 9:51 ` [PATCH v2 2/7] MdeModulePkg/DxeCore: Fix DebugImageInfoTable size report Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 3/7] EmbeddedPkg/GdbStub: Check DebugImageInfoTable type safely Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 4/7] ArmPkg/DefaultExceptionHandlerLib: " Marvin Häuser
2021-08-09 11:55 ` Ard Biesheuvel
2021-08-09 12:40 ` [edk2-devel] " Marvin Häuser
2021-08-09 21:19 ` Marvin Häuser
2021-08-16 9:50 ` Ard Biesheuvel
2021-08-09 9:51 ` [PATCH v2 5/7] MdeModulePkg/CoreDxe: Mandatory LoadedImage for DebugImageInfoTable Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 6/7] EmbeddedPkg/GdbStub: " Marvin Häuser
2021-08-09 9:51 ` [PATCH v2 7/7] ArmPkg/DefaultExceptionHandlerLib: " Marvin Häuser
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=PH0PR11MB506465FDC82E9CD80C240B22C5F99@PH0PR11MB5064.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox