From bfb1a45eec911757e39b4ccc993604d9b1ca500d Mon Sep 17 00:00:00 2001 From: Doug Flick Date: Wed, 9 Apr 2025 11:33:15 -0700 Subject: [PATCH] SecurityPkg: SecureBootVariableLib: Prevent Invalid DBX This commit adds the ability to skip the setting the Dbx variable if the Default being provided is less than the size of the EFI_SIGNATURE_LIST structure. This is to prevent the setting of an invalid DBX which would cause the system to fail to boot. Additionally, this can be used to signal that setting the DBX should leave DBX undefined for Platforms that want to let the OS be the sole servicer of the DBX. Breakdown of the math is as follows: 1. **`sizeof(EFI_SIGNATURE_LIST)`**: - This is the size of the `EFI_SIGNATURE_LIST` structure itself, which includes: - `EFI_GUID SignatureType` (16 bytes) - `UINT32 SignatureListSize` (4 bytes) - `UINT32 SignatureHeaderSize` (4 bytes) - `UINT32 SignatureSize` (4 bytes) - Total: `16 + 4 + 4 + 4 = 28 bytes` 2. **`SignatureHeaderSize`**: - This is the size of the optional signature header. If no header is provided, this value is `0`. 3. **`SignatureSize`**: - This is the size of each `EFI_SIGNATURE_DATA` entry. For an empty list, this value is `0`. The total size of an empty `EFI_SIGNATURE_LIST` is: ```c sizeof(EFI_SIGNATURE_LIST) + SignatureHeaderSize ``` 1. **No Signature Header**: - If `SignatureHeaderSize = 0`, the size is: ```c 28 + 0 = 28 bytes ``` 2. **With a Signature Header**: - If `SignatureHeaderSize = 16` (example size for a header), the size is: ```c 28 + 16 = 44 bytes ``` - **Minimum Size**: `28 bytes` (if `SignatureHeaderSize = 0`). - **Additional Size**: Add the value of `SignatureHeaderSize` if a header is included. Signed-off-by: Doug Flick --- .../SecureBootVariableLib.c | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c b/SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c index 24599c7a0b..acb3c2af03 100644 --- a/SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c +++ b/SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c @@ -23,6 +23,21 @@ #include #include +// +// This is the minimum size of a EFI_SIGNATURE_LIST and data and still be valid. +// +// The UEFI specification defines this as: +// Each signature list is a list of signatures of one type, identified by SignatureType. +// The signature list contains a header and then an array of zero or more signatures in +// the format specified by the header. The size of each signature in the signature list +// is specified by SignatureSize. +// +// This is the size of a signature list with some data in it. Effectively 44 bytes are +// required to set the DBX. However in reality, the size of a real payload will be larger +// since this miniumum size does not consider the SIGNATURE_LIST +// +#define MINIMUM_VALID_SIGNATURE_LIST (sizeof(EFI_SIGNATURE_LIST) + sizeof(EFI_SIGNATURE_DATA) - 1) + // This time can be used when deleting variables, as it should be greater than any variable time. EFI_TIME mMaxTimestamp = { 0xFFFF, // Year @@ -816,12 +831,27 @@ SetSecureBootVariablesToDefault ( // dbx is a good place to start. Data = (UINT8 *)SecureBootPayload->DbxPtr; DataSize = SecureBootPayload->DbxSize; - Status = EnrollFromInput ( + if (DataSize > MINIMUM_VALID_SIGNATURE_LIST) { + // + // Ensure that that the DataSize meets a minimum allowed size before being set. + // + DEBUG ((DEBUG_INFO, "%a - Setting Dbx\n", __func__)); + Status = EnrollFromInput ( EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageSecurityDatabaseGuid, DataSize, Data ); + } else if ((DataSize == 0) || (DataSize == sizeof (EFI_SIGNATURE_LIST))) { + // + // The DBX is allowed to be empty by default + // + DEBUG ((DEBUG_INFO, "%a - Skipping Dbx - DataSize(%u)\n", __func__, DataSize)); + Status = EFI_SUCCESS; + } else { + DEBUG ((DEBUG_ERROR, "%a - Invalid Dbx size %u\n", __func__, DataSize)); + Status = EFI_INVALID_PARAMETER; + } // If that went well, try the db (make sure to pick the right one!). if (!EFI_ERROR (Status)) {