From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web11.93725.1677513198090762281 for ; Mon, 27 Feb 2023 07:53:19 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=oOXqHSpK; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 9C7882402D3 for ; Mon, 27 Feb 2023 16:53:15 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1677513195; bh=4VIVbVgAlOe/pVZkjnv8pEEI2ONhBWHYdFZSagsvvFY=; h=Subject:From:Date:Cc:To:From; b=oOXqHSpKi4C49ROtqrGc/HseRuvw4v3zZoMd4XTTxHQjxttqng2JuKPCoxnGwKuaL 3xpsw3qweAHcBnHqpbxu7ean0EeiRtdOoDyebyXdtPDM/pjARJ4WieGwxcQs8soKn5 bmClVPhCfvY/rjq/f9T5qUuLYBSK1h3wNVEc7h4ElVfgibDePEkGHoPCrTerS76Dmj wC1qrlisDwUsBcthc+DcY3so92r9yTatpd8hJw42F+ZUwEukhyHeBld0Au4PtgAQYx sXDwj4vAKScaJSIrhbqK2uC6YpCfFXo2VL6xvNtW4d1zlw7ID9GbTx7FLszCq1pB+X 3XuU8jweQMiVg== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4PQQ553PQwz9rxD; Mon, 27 Feb 2023 16:53:13 +0100 (CET) Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.400.51.1.1\)) Subject: Re: [PATCH 1/1] SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2 From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= In-Reply-To: <20230227125517.821136-1-kraxel@redhat.com> Date: Mon, 27 Feb 2023 15:53:03 +0000 Cc: edk2-devel-groups-io , Pawel Polawski , Jiewen Yao , jmaloy@redhat.com, Min Xu , Jian J Wang , Oliver Steffen Message-Id: <477307A5-5A37-4E89-8005-3B654A4097A6@posteo.de> References: <20230227125517.821136-1-kraxel@redhat.com> To: Gerd Hoffmann Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable *Thanks*, Gerd! Two comments inline. > On 27. Feb 2023, at 13:55, Gerd Hoffmann wrote: >=20 > Call gRT->GetVariable() directly to read the SecureBoot variable. It = is > one byte in size so we can easily place it on the stack instead of > having GetEfiGlobalVariable2() allocate it for us, which avoids a few > possible error cases. >=20 > Skip secure boot checks if (and only if): >=20 > (a) the SecureBoot variable is not present (EFI_NOT_FOUND) according = to > the return value, or @Maintainers Would there be any objection to drop this and skip the SB = checks only when explicitly disabled? Please explicitly respond even if not, so we don't end up with everyone = silently agreeing, but forgetting about the patch after. Thanks! :) > (b) the SecureBoot variable was read successfully and is set to > SECURE_BOOT_MODE_DISABLE. >=20 > Previously the code skipped the secure boot checks on *any* > gRT->GetVariable() error (GetEfiGlobalVariable2 sets the variable > value to NULL in that case) and also on memory allocation failures. >=20 > Fixes: CVE-2019-14560 > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2167 > Signed-off-by: Gerd Hoffmann > --- > .../DxeImageVerificationLib.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) >=20 > diff --git = a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c = b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > index 66e2f5eaa3c0..f29a27e5a053 100644 > --- = a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > +++ = b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > @@ -1671,7 +1671,8 @@ DxeImageVerificationHandler ( > EFI_IMAGE_EXECUTION_ACTION Action; > WIN_CERTIFICATE *WinCertificate; > UINT32 Policy; > - UINT8 *SecureBoot; > + UINT8 SecureBoot; > + UINTN SecureBootSize; > PE_COFF_LOADER_IMAGE_CONTEXT ImageContext; > UINT32 NumberOfRvaAndSizes; > WIN_CERTIFICATE_EFI_PKCS *PkcsCertData; > @@ -1686,6 +1687,7 @@ DxeImageVerificationHandler ( > RETURN_STATUS PeCoffStatus; > EFI_STATUS HashStatus; > EFI_STATUS DbStatus; > + EFI_STATUS VarStatus; > BOOLEAN IsFound; >=20 > SignatureList =3D NULL; > @@ -1742,24 +1744,22 @@ DxeImageVerificationHandler ( > CpuDeadLoop (); > } >=20 > - GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID = **)&SecureBoot, NULL); > + SecureBootSize =3D sizeof (SecureBoot); > + VarStatus =3D gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, = &gEfiGlobalVariableGuid, NULL, &SecureBootSize, &SecureBoot); > // > // Skip verification if SecureBoot variable doesn't exist. > // > - if (SecureBoot =3D=3D NULL) { > + if (VarStatus =3D=3D EFI_NOT_FOUND) { > return EFI_SUCCESS; > } >=20 > // > // Skip verification if SecureBoot is disabled but not AuditMode > // > - if (*SecureBoot =3D=3D SECURE_BOOT_MODE_DISABLE) { > - FreePool (SecureBoot); > + if ((VarStatus =3D=3D EFI_SUCCESS) && (SecureBoot =3D=3D = SECURE_BOOT_MODE_DISABLE)) { I would check the attributes here as well. They should be BS | RT, but = explicitly not NV. This would force the SB checks in case a malicious = actor somehow managed to store a persistent disable-value variable (be = that a bug, physical access, or other means). Best regards, Marvin > return EFI_SUCCESS; > } >=20 > - FreePool (SecureBoot); > - > // > // Read the Dos header. > // > --=20 > 2.39.2 >=20