Skip to content

Commit

Permalink
Partial Revert "SecurityPkg/SecureBoot: Support RSA4096 and RSA3072" (#…
Browse files Browse the repository at this point in the history
…224)

## Description

Edk2 updated AuthVariable and secureboot to allow them to use SHA384 and
SHA512. The AuthVariable addition is good because it allows signing this
with the PK but the secureboot addition is unnecessary.

The secureboot change has things hashed by all three algorithms and then
checking them in the DBX for SHA256, SHA384 and SHA512 lists to make
sure it's not on any of them. The issue with this is two fold.

1. This will have a performance impact. One that many platforms will not
want.
2. This is completely unnecessary because the only group putting things
in the DBX is Microsoft and we only use SHA256.

For these reasons it makes sense to revert the change in the secureboot
logic and keep the AuthVariable changes.
Commit in edk2 for reference:
tianocore/edk2@bbf1822

- [] Impacts functionality?
- **Functionality** - Does the change ultimately impact how firmware
functions?
- Examples: Add a new library, publish a new PPI, update an algorithm,
...
- [] Impacts security?
- **Security** - Does the change have a direct security impact on an
application,
    flow, or firmware?
  - Examples: Crypto algorithm change, buffer overflow fix, parameter
    validation improvement, ...
- [ ] Breaking change?
- **Breaking change** - Will anyone consuming this change experience a
break
    in build or boot behavior?
- Examples: Add a new library class, move a module to a different repo,
call
    a function in a new library class in a pre-existing module, ...
- [ ] Includes tests?
  - **Tests** - Does the change include any explicit test code?
  - Examples: Unit tests, integration tests, robot tests, ...
- [ ] Includes documentation?
- **Documentation** - Does the change contain explicit documentation
additions
    outside direct code modifications (and comments)?
- Examples: Update readme file, add feature readme file, link to
documentation
    on an a separate Web page, ...

## How This Was Tested

Tested on Intel Physical systems.  No issues seen.

## Integration Instructions

N/A
  • Loading branch information
kenlautner committed Feb 7, 2024
1 parent dd5922c commit 36b848b
Show file tree
Hide file tree
Showing 5 changed files with 35 additions and 108 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1630,7 +1630,7 @@ IsAllowedByDb (
in the security database "db", and no valid signature nor any hash value of the image may
be reflected in the security database "dbx".
Otherwise, the image is not signed,
The hash value of the image must match a record in the security database "db", and
The SHA256 hash value of the image must match a record in the security database "db", and
not be reflected in the security data base "dbx".
Caution: This function may receive untrusted input.
Expand Down Expand Up @@ -1700,8 +1700,6 @@ DxeImageVerificationHandler (
EFI_STATUS VarStatus;
UINT32 VarAttr;
BOOLEAN IsFound;
UINT8 HashAlg;
BOOLEAN IsFoundInDatabase;

SignatureList = NULL;
SignatureListSize = 0;
Expand All @@ -1711,7 +1709,6 @@ DxeImageVerificationHandler (
Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
IsVerified = FALSE;
IsFound = FALSE;
IsFoundInDatabase = FALSE;

//
// Check the image type and get policy setting.
Expand Down Expand Up @@ -1850,51 +1847,40 @@ DxeImageVerificationHandler (
//
if ((SecDataDir == NULL) || (SecDataDir->Size == 0)) {
//
// This image is not signed. The hash value of the image must match a record in the security database "db",
// This image is not signed. The SHA256 hash value of the image must match a record in the security database "db",
// and not be reflected in the security data base "dbx".
//
HashAlg = sizeof (mHash) / sizeof (HASH_TABLE);
while (HashAlg > 0) {
HashAlg--;
if ((mHash[HashAlg].GetContextSize == NULL) || (mHash[HashAlg].HashInit == NULL) || (mHash[HashAlg].HashUpdate == NULL) || (mHash[HashAlg].HashFinal == NULL)) {
continue;
}

if (!HashPeImage (HashAlg)) {
continue;
}

DbStatus = IsSignatureFoundInDatabase (
EFI_IMAGE_SECURITY_DATABASE1,
mImageDigest,
&mCertType,
mImageDigestSize,
&IsFound
);
if (EFI_ERROR (DbStatus) || IsFound) {
//
// Image Hash is in forbidden database (DBX).
//
DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed and %s hash of image is forbidden by DBX.\n", mHashTypeStr));
goto Failed;
}
if (!HashPeImage (HASHALG_SHA256)) {
DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Failed to hash this image using %s.\n", mHashTypeStr));
goto Failed;
}

DbStatus = IsSignatureFoundInDatabase (
EFI_IMAGE_SECURITY_DATABASE,
mImageDigest,
&mCertType,
mImageDigestSize,
&IsFound
);
if (!EFI_ERROR (DbStatus) && IsFound) {
//
// Image Hash is in allowed database (DB).
//
IsFoundInDatabase = TRUE;
}
DbStatus = IsSignatureFoundInDatabase (
EFI_IMAGE_SECURITY_DATABASE1,
mImageDigest,
&mCertType,
mImageDigestSize,
&IsFound
);
if (EFI_ERROR (DbStatus) || IsFound) {
//
// Image Hash is in forbidden database (DBX).
//
DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed and %s hash of image is forbidden by DBX.\n", mHashTypeStr));
goto Failed;
}

if (IsFoundInDatabase) {
DbStatus = IsSignatureFoundInDatabase (
EFI_IMAGE_SECURITY_DATABASE,
mImageDigest,
&mCertType,
mImageDigestSize,
&IsFound
);
if (!EFI_ERROR (DbStatus) && IsFound) {
//
// Image Hash is in allowed database (DB).
//
return EFI_SUCCESS;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,6 @@
## SOMETIMES_PRODUCES ## GUID # Unique ID for the type of the signature.
gEfiCertSha256Guid

## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the signature.
## SOMETIMES_PRODUCES ## GUID # Unique ID for the type of the signature.
gEfiCertSha384Guid

## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the signature.
## SOMETIMES_PRODUCES ## GUID # Unique ID for the type of the signature.
gEfiCertSha512Guid

## SOMETIMES_CONSUMES ## Variable:L"db"
## SOMETIMES_PRODUCES ## Variable:L"db"
## SOMETIMES_CONSUMES ## Variable:L"dbx"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1859,7 +1859,7 @@ HashPeImage (
SectionHeader = NULL;
Status = FALSE;

if ((HashAlg >= HASHALG_MAX)) {
if ((HashAlg != HASHALG_SHA256)) {
return FALSE;
}

Expand All @@ -1868,25 +1868,8 @@ HashPeImage (
//
ZeroMem (mImageDigest, MAX_DIGEST_SIZE);

switch (HashAlg) {
case HASHALG_SHA256:
mImageDigestSize = SHA256_DIGEST_SIZE;
mCertType = gEfiCertSha256Guid;
break;

case HASHALG_SHA384:
mImageDigestSize = SHA384_DIGEST_SIZE;
mCertType = gEfiCertSha384Guid;
break;

case HASHALG_SHA512:
mImageDigestSize = SHA512_DIGEST_SIZE;
mCertType = gEfiCertSha512Guid;
break;

default:
return FALSE;
}
mImageDigestSize = SHA256_DIGEST_SIZE;
mCertType = gEfiCertSha256Guid;

CtxSize = mHash[HashAlg].GetContextSize ();

Expand Down Expand Up @@ -2286,7 +2269,6 @@ EnrollImageSignatureToSigDB (
UINT32 Attr;
WIN_CERTIFICATE_UEFI_GUID *GuidCertData;
EFI_TIME Time;
UINT32 HashAlg;

Data = NULL;
GuidCertData = NULL;
Expand Down Expand Up @@ -2325,22 +2307,8 @@ EnrollImageSignatureToSigDB (
}

if (mSecDataDir->SizeOfCert == 0) {
Status = EFI_SECURITY_VIOLATION;
HashAlg = sizeof (mHash) / sizeof (HASH_TABLE);
while (HashAlg > 0) {
HashAlg--;
if ((mHash[HashAlg].GetContextSize == NULL) || (mHash[HashAlg].HashInit == NULL) || (mHash[HashAlg].HashUpdate == NULL) || (mHash[HashAlg].HashFinal == NULL)) {
continue;
}

if (HashPeImage (HashAlg)) {
Status = EFI_SUCCESS;
break;
}
}

if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "Fail to get hash digest: %r", Status));
if (!HashPeImage (HASHALG_SHA256)) {
Status = EFI_SECURITY_VIOLATION;
goto ON_EXIT;
}
} else {
Expand Down Expand Up @@ -3836,10 +3804,6 @@ LoadSignatureList (
ListType = STRING_TOKEN (STR_LIST_TYPE_SHA1);
} else if (CompareGuid (&ListWalker->SignatureType, &gEfiCertSha256Guid)) {
ListType = STRING_TOKEN (STR_LIST_TYPE_SHA256);
} else if (CompareGuid (&ListWalker->SignatureType, &gEfiCertSha384Guid)) {
ListType = STRING_TOKEN (STR_LIST_TYPE_SHA384);
} else if (CompareGuid (&ListWalker->SignatureType, &gEfiCertSha512Guid)) {
ListType = STRING_TOKEN (STR_LIST_TYPE_SHA512);
} else if (CompareGuid (&ListWalker->SignatureType, &gEfiCertX509Sha256Guid)) {
ListType = STRING_TOKEN (STR_LIST_TYPE_X509_SHA256);
} else if (CompareGuid (&ListWalker->SignatureType, &gEfiCertX509Sha384Guid)) {
Expand Down Expand Up @@ -4087,12 +4051,6 @@ FormatHelpInfo (
} else if (CompareGuid (&ListEntry->SignatureType, &gEfiCertSha256Guid)) {
ListTypeId = STRING_TOKEN (STR_LIST_TYPE_SHA256);
DataSize = 32;
} else if (CompareGuid (&ListEntry->SignatureType, &gEfiCertSha384Guid)) {
ListTypeId = STRING_TOKEN (STR_LIST_TYPE_SHA384);
DataSize = 48;
} else if (CompareGuid (&ListEntry->SignatureType, &gEfiCertSha512Guid)) {
ListTypeId = STRING_TOKEN (STR_LIST_TYPE_SHA512);
DataSize = 64;
} else if (CompareGuid (&ListEntry->SignatureType, &gEfiCertX509Sha256Guid)) {
ListTypeId = STRING_TOKEN (STR_LIST_TYPE_X509_SHA256);
DataSize = 32;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,6 @@ extern EFI_IFR_GUID_LABEL *mEndLabel;
#define MAX_DIGEST_SIZE SHA512_DIGEST_SIZE

#define WIN_CERT_UEFI_RSA2048_SIZE 256
#define WIN_CERT_UEFI_RSA3072_SIZE 384
#define WIN_CERT_UEFI_RSA4096_SIZE 512

//
// Support hash types
Expand All @@ -100,11 +98,6 @@ extern EFI_IFR_GUID_LABEL *mEndLabel;
//
#define CER_PUBKEY_MIN_SIZE 256

//
// Define KeyType for public key storing file
//
#define KEY_TYPE_RSASSA 0

//
// Types of errors may occur during certificate enrollment.
//
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#string STR_LIST_TYPE_X509 #language en-US "X509"
#string STR_LIST_TYPE_SHA1 #language en-US "SHA1"
#string STR_LIST_TYPE_SHA256 #language en-US "SHA256"
#string STR_LIST_TYPE_SHA384 #language en-US "SHA384"
#string STR_LIST_TYPE_SHA512 #language en-US "SHA512"
#string STR_LIST_TYPE_X509_SHA256 #language en-US "X509_SHA256"
#string STR_LIST_TYPE_X509_SHA384 #language en-US "X509_SHA384"
#string STR_LIST_TYPE_X509_SHA512 #language en-US "X509_SHA512"
Expand Down

0 comments on commit 36b848b

Please sign in to comment.