diff --git a/.github/skills/edk2-c-code-security/SKILL.md b/.github/skills/edk2-c-code-security/SKILL.md new file mode 100644 index 00000000000..14c94ff7db2 --- /dev/null +++ b/.github/skills/edk2-c-code-security/SKILL.md @@ -0,0 +1,470 @@ +--- +name: EDK2 C Code Security Guidelines +description: Comprehensive security guidelines for EDK2 firmware C code, including secure coding practices, secure code review principles, and firmware-specific security requirements. Follow these instructions to prevent vulnerabilities and ensure robust security in EDK2 firmware development. +applyTo: '**/*.{c,h,inf,dsc,dec,fdf}' +--- +MANDATORY: Completely understand and follow this instructions file. Failure to do so will cost significant time and +effort. + +# EDK2 Security Instructions + +This file provides security-specific guidelines for EDK2 firmware C code. For foundational +EDK2 concepts (file formats, build system, module types, coding standards, and common APIs), +refer to [edk2-project-general.instructions.md](../edk2-project-general/SKILL.md). + +## Secure Coding Guidelines for EDK2 Firmware + +> **Reference**: [EDK II Secure Coding Guide](https://github.com/tianocore-docs/EDK_II_Secure_Coding_Guide) + +These secure coding guidelines provide specific requirements for writing secure firmware code in EDK2. Follow these mandatory rules to prevent security vulnerabilities during development. + +### General Secure Coding Principles + +#### Input Validation +- **#GENERAL.1**: Code in trusted region MUST check any data from untrusted regions (PE images, capsules, SMM comm buffers, MMIO BARs) +- **#GENERAL.2**: MUST avoid buffer overflows - never access beyond buffer size fields +- **#GENERAL.3**: MUST avoid integer overflow - use addition vs subtraction, division vs multiplication for validation +- **#GENERAL.4**: MUST avoid untrusted data overlap with trusted regions +- **#GENERAL.5**: MUST check untrusted data in ALL possible code paths + +#### Input Processing Rules +- **#GENERAL.INPUT.1**: Check for valid input and reject everything else +- **#GENERAL.INPUT.2**: Perform sanity checks: type, length, range, format validation +- **#GENERAL.INPUT.3**: Use canonical representation - fully qualified pathnames +- **#GENERAL.INPUT.4**: Beware character encoding and escape characters +- **#GENERAL.INPUT.5**: Validate as deep as possible to prevent errors from code changes +- **#GENERAL.INPUT.6**: Careful of boundary conditions (off-by-one) and conditionals + +#### Buffer Management +- **#GENERAL.BUFFER.1**: Check buffer sizes, copies, and indices (especially sizeof) +- **#GENERAL.BUFFER.2**: Check appropriate buffer size with globally defined maximums +- **#GENERAL.BUFFER.3**: Check for NULL pointer dereferences +- **#GENERAL.BUFFER.4**: Check NULL for NIL-terminated strings + +#### Arithmetic Safety +- **#GENERAL.ARITH.1**: Check data type limitations (integer underflow/overflow) +- **#GENERAL.ARITH.2**: Properly cast numeric variables in string manipulation +- **#GENERAL.ARITH.3**: SHOULD use SafeInt library for external input integers + +#### Failure Handling +- **#GENERAL.FAIL.1**: Fail secure - fail closed when checks fail +- **#GENERAL.FAIL.2**: Don't provide hints to attackers in error messages + +### ASSERT Usage Guidelines + +**Critical Rules for ASSERT**: +- **#ASSERT.1**: ASSERT MUST be used for things that NEVER occur - use error handling for things that MIGHT occur +- **#ASSERT.Variable.1**: GetVariable with NV+RT without AU/RO MUST NOT ASSERT +- **#ASSERT.Variable.2**: SetVariable with NV attribute MUST NOT ASSERT +- **#ASSERT.Resource.1**: Memory allocation MUST NOT ASSERT after EndOfDxe +- **#ASSERT.SMM.1**: SMI handlers MUST NOT use ASSERT for external input after EndOfDxe +- **#ASSERT.NETWORK.1**: Network drivers MUST NOT use ASSERT for remote packets +- **#ASSERT.SHELL.1**: Shell MUST NOT use ASSERT for resource requests or user input + +### Deprecated API Replacement + +**#DEPRECATEDAPI.1**: MUST NOT use deprecated APIs - use secure replacements: + +| **Deprecated API** | **Secure Replacement** | +|-------------------|------------------------| +| `StrCpy` | `StrCpyS` | +| `StrnCpy` | `StrnCpyS` | +| `StrCat` | `StrCatS` | +| `StrnCat` | `StrnCatS` | +| `AsciiStrCpy` | `AsciiStrCpyS` | +| `AsciiStrnCpy` | `AsciiStrnCpyS` | +| `AsciiStrCat` | `AsciiStrCatS` | +| `AsciiStrnCat` | `AsciiStrnCatS` | +| `UnicodeStrToAsciiStr` | `UnicodeStrToAsciiStrS` | +| `AsciiStrToUnicodeStr` | `AsciiStrToUnicodeStrS` | +| `GetVariable` | `GetVariable2` | +| `GetEfiGlobalVariable` | `GetEfiGlobalVariable2` | + +### Race Condition Prevention + +- **#RACECONDITION.1**: Careful of Time-of-Check/Time-of-Use attacks - copy untrusted data to trusted region before processing +- **#RACECONDITION.2**: Careful of BSP/AP race conditions - keep security critical sections short and simple + +### Environment Security + +#### Memory Protection +- **#ENVIRONMENT.RUNTIME.1**: Runtime modules MUST be built with 4K alignment for OS protection +- **#ENVIRONMENT.NX.1**: Code regions SHOULD be ReadOnly, Data regions SHOULD be NonExecutable +- **#ENVIRONMENT.NX.2**: Unallocated memory SHOULD be non-present or NonExecutable +- **#ENVIRONMENT.STACK.1**: Stack SHOULD be set to NX +- **#ENVIRONMENT.STACK.2**: Stack Guard SHOULD be enabled +- **#ENVIRONMENT.HEAP.1**: Heap SHOULD be NX for data +- **#ENVIRONMENT.HEAP.2**: MAY use heap guard for debugging + +#### Advanced Protections +- **#ENVIRONMENT.ASLR.1-3**: Address Space Layout Randomization guidelines +- **#ENVIRONMENT.CONTROLFLOW.1**: Control Flow Guard for ROP/JOP protection + +### Cryptography Requirements + +#### Algorithm Selection +- **#CRYPTO.1**: SHOULD NOT use deprecated crypto algorithms +- **#CRYPTO.2**: SHOULD NOT implement custom crypto algorithms +- **#CRYPTO.3**: MUST follow cryptographic standards exactly +- **#CRYPTO.HASH.1**: SHOULD use SHA256 or stronger (NOT SHA1, MD4, MD5) +- **#CRYPTO.SYM.1**: SHOULD use AES or stronger +- **#CRYPTO.ASYM.1**: SHOULD use RSA or ECC equivalent or stronger + +#### Key Management +- **#CRYPTO.SYM.2**: Symmetric keys MUST NOT be saved in flash as plain text +- **#CRYPTO.ASYM.2**: Private keys MUST NOT be saved in flash as plain text +- **#CRYPTO.RANDOM.1**: SHOULD use approved random number generator + +### Password and Secret Handling + +#### Password Security +- **#PASSWORD.1**: Password plaintext MUST NOT be saved to variables (use SALT+HASH) +- **#PASSWORD.2**: Password updates MUST be in secure environment (SMM or before EndOfDxe) +- **#PASSWORD.3**: MUST meet password criteria (strength, update, algorithm, retry, etc.) +- **#PASSWORD.5**: MUST clear passwords from memory after use +- **#PASSWORD.6**: MUST NOT hardcode passwords in code +- **#PASSWORD.7**: MUST compare all characters of password strings (constant-time) +- **#PASSWORD.8**: MUST add salt to resist rainbow table attacks +- **#PASSWORD.9**: MUST add sufficient iterations to slow hash calculation + +#### Secret Management +- **#SECRET.1**: Secrets MUST NOT be saved as plain text in variables/disk +- **#SECRET.2**: MUST clear secrets from memory after use (global, stack, heap) +- **#SECRET.3**: MUST NOT hardcode secrets in code +- **#SECRET.4**: MUST compare entire secret data before completion +- **#SECRET.5**: Secret length MUST resist brute force attacks + +### Firmware-Specific Security Guidelines + +#### Flash Protection +- **#FLASH.1**: Platform MUST lock flash no later than EndOfDxe +- **#FLASH.2**: Flash lock MUST happen in ALL boot modes (normal, S3, capsule, recovery) + +#### Flash Updates +- **#FLASH.UPDATE.1-7**: Comprehensive flash update security requirements including integrity checking, version validation, trusted execution environment + +#### Variable Security +- **#VARIABLE.1**: MUST lock critical variables before EndOfDxe +- **#VARIABLE.2**: MUST use same lock policy in normal boot and S4 +- **#VARIABLE.SET.1-2**: Error handling for variable operations +- **#VARIABLE.GET.1-2**: Error handling and validation for variable access +- **#VARIABLE.ATTRIB.1-2**: Conservative RT and NV attribute usage +- **#VARIABLE.CHECK.1-3**: Enable HII, PCD, and UEFI variable checks +- **#VARIABLE.CONFIDENTIALITY.1-2**: Encryption requirements for confidential variables + +#### S3 Resume Security +- **#S3.1**: S3 BootScript MUST be saved in secure place (lockbox) +- **#S3.2**: S3 image dispatch MUST be saved securely +- **#S3.3**: S3 CPU data MUST be saved in SMM +- **#S3.4**: S3 configuration MUST be saved securely + +#### Secure Boot Requirements +- **#SECUREBOOT.1**: MUST NOT disable secure boot without authentication +- **#SECUREBOOT.2**: MUST verify ALL images in secure boot path +- **#SECUREBOOT.3-4**: Firmware update/recovery images must be signed +- **#SECUREBOOT.5**: Verification in ALL boot paths +- **#SECUREBOOT.UEFI.1-3**: UEFI secure boot implementation requirements +- **#SECUREBOOT.Key.1**: Public keys MUST be in hardware/boot block/auth variables + +#### Hardware and DMA Security +- **#HARDWARE.1**: MUST verify untrusted hardware inputs (SPD, USB descriptors, etc.) +- **#DMA.1**: Device DMA MUST be disabled by default +- **#DMA.2**: Enable DMA only when needed, disable after transaction +- **#DMA.3-5**: IOMMU configuration and DMA region isolation. DMA must not occur to the stack. +- **#NETWORK.1-2**: Network packet validation at all layers +- **#NETWORK.TLS.1-2**: Certificate storage requirements +- **#NETWORK.WIFI.1**: WiFi password protection + +#### Silicon Register Security +- **#SILICON.1**: Lockable registers MUST be locked before EndOfDxe +- **#SILICON.2**: Register locks MUST work in all boot paths + +### Side-Channel Attack Mitigation + +- **#SIDECHANNEL.1**: Use `SpeculationBarrier()` after untrusted data validation in SMM +- **#SIDECHANNEL.2**: Use `StuffRsb` before RSM to mitigate Branch Target Injection +- **#MDS.1**: Rendezvous all logical processors in SMM for MDS mitigation + +### Example: Secure Input Validation + +```c +// SECURE: Proper input validation with overflow checks +EFI_STATUS +SecureProcessBuffer ( + IN VOID *InputBuffer, + IN UINTN BufferSize, + IN UINTN ElementCount, + IN UINTN ElementSize + ) +{ + UINTN RequiredSize; + + // Validate input parameters + if (InputBuffer == NULL || BufferSize == 0) { + return EFI_INVALID_PARAMETER; + } + + // Check for integer overflow in multiplication + if (ElementCount > MAX_UINTN / ElementSize) { + return EFI_INVALID_PARAMETER; + } + + RequiredSize = ElementCount * ElementSize; + + // Validate buffer size + if (BufferSize < RequiredSize) { + return EFI_BUFFER_TOO_SMALL; + } + + // Copy to trusted region before processing + VOID *TrustedBuffer = AllocatePool (RequiredSize); + if (TrustedBuffer == NULL) { + return EFI_OUT_OF_RESOURCES; + } + + CopyMem (TrustedBuffer, InputBuffer, RequiredSize); + + // Process data from trusted region only + ProcessTrustedData (TrustedBuffer, RequiredSize); + + // Clear sensitive data + ZeroMem (TrustedBuffer, RequiredSize); + FreePool (TrustedBuffer); + + return EFI_SUCCESS; +} +``` + +### Example: Secure Password Handling + +```c +// SECURE: Proper password comparison and cleanup +EFI_STATUS +SecurePasswordVerify ( + IN CHAR8 *InputPassword, + IN UINTN InputPasswordSize + ) +{ + CHAR8 StoredPasswordHash[32]; + CHAR8 ComputedHash[32]; + CHAR8 Salt[16]; + CHAR8 *LocalPassword; + UINTN Index; + UINT8 Result; + EFI_STATUS Status; + + if ((InputPassword == NULL) || (InputPasswordSize == 0)) { + return EFI_INVALID_PARAMETER; + } + + // Allocate buffer for local copy of password + LocalPassword = AllocateZeroPool (InputPasswordSize); + if (LocalPassword == NULL) { + return EFI_OUT_OF_RESOURCES; + } + + Result = 0; + + // Copy input password to local buffer + CopyMem (LocalPassword, InputPassword, InputPasswordSize); + + // Get stored hash and salt + GetStoredPasswordHash (StoredPasswordHash, Salt); + + // Compute hash of local password with salt + ComputePasswordHash (LocalPassword, Salt, ComputedHash); + + // Constant-time comparison + for (Index = 0; Index < sizeof (StoredPasswordHash); Index++) { + Result |= (StoredPasswordHash[Index] ^ ComputedHash[Index]); + } + + Status = (Result == 0) ? EFI_SUCCESS : EFI_ACCESS_DENIED; + + // Clear sensitive data + ZeroMem (ComputedHash, sizeof (ComputedHash)); + ZeroMem (LocalPassword, InputPasswordSize); + FreePool (LocalPassword); + + return Status; +} +``` + +These secure coding guidelines provide the foundation for writing secure EDK2 firmware code that resists common attack vectors and follows industry best practices. + +## Security Guidelines for Firmware Code + +> **Reference**: [EDK II Secure Code Review Guide](https://github.com/tianocore-docs/EDK_II_Secure_Code_Review_Guide) + +Secure code review is critical for firmware development due to the privileged execution context and attack surface exposure. Unlike typical software reviews focused on quality, secure code reviews focus on confidentiality, integrity, and availability (C.I.A.) aspects. + +### General Security Review Principles + +#### 1. Prerequisites for Reviewers +- **Understand Threat Model**: Know the security architecture, assets, objectives, adversaries, and mitigations +- **Know Secure Design Principles**: Understand EDK II secure coding best practices +- **Review Priority**: Focus on high-risk code areas first + +#### 2. Code Review Priority (High to Low) +1. **Old legacy code** - Higher vulnerability likelihood +2. **Code running by default** - Broader attack surface +3. **Code in elevated context** - SMM, PEI, early DXE +4. **C/C++/Assembly code** - Memory safety vulnerabilities +5. **Code with vulnerability history** - Known problem areas +6. **Code handling sensitive data** - Cryptographic material, secrets +7. **Complex code** - Higher error probability +8. **Frequently changing code** - Introduction of new vulnerabilities + +#### 3. Common Vulnerability Patterns to Review +- **Integer arithmetic vulnerabilities** - Overflow, underflow, wraparound +- **Buffer overrun vulnerabilities** - Stack/heap buffer overflows +- **Cryptographic vulnerabilities** - Weak algorithms, improper key handling +- **Logic errors** - Off-by-one, incorrect operators (>, >=, ||, &&) +- **Input validation failures** - Insufficient boundary checks +- **Pointer validation issues** - NULL, dangling, out-of-bounds pointers + +### Firmware-Specific Security Categories + +Based on analysis of firmware vulnerabilities, focus reviews on these 8 critical categories: + +#### 1. External Input Validation +**Risk**: Attacker-controlled data causing overflow, injection, or privilege escalation + +**Common External Inputs**: +- UEFI capsule images +- Boot logo files (BMP, JPEG) +- File system partition contents +- Read/write UEFI variables +- SMM communication buffers +- Network packets (AMT, PXE) +- ACPI tables + +**Review Checklist**: +- [ ] What external inputs does the code process? +- [ ] How is input size/bounds validation performed? +- [ ] Are validation checks performed on ALL possible code paths? +- [ ] What happens when validation fails? (proper error handling) +- [ ] For SMM: Is `SmmIsBufferOutsideSmmValid()` used for communication buffers? +- [ ] For variables: How are read/write variables consumed and validated? +- [ ] Are ASSERT macros used for validation? (Should use proper error returns) + +**Example Vulnerability Pattern - Integer Overflow**: +```c +// VULNERABLE: Multiplication can overflow +BltBufferSize = BmpHeader->PixelWidth * BmpHeader->PixelHeight * sizeof(EFI_GRAPHICS_OUTPUT_BLT_PIXEL); +*GopBlt = AllocatePool(BltBufferSize); // May allocate tiny buffer due to overflow + +// SECURE: Check for overflow before multiplication +if (BmpHeader->PixelWidth > MAX_UINT32 / sizeof(EFI_GRAPHICS_OUTPUT_BLT_PIXEL) / BmpHeader->PixelHeight) { + return EFI_INVALID_PARAMETER; +} +``` + +#### 2. Race Conditions +**Risk**: Time-of-check-time-of-use (TOCTOU) vulnerabilities, resource corruption + +**Review Checklist**: +- [ ] What critical resources are accessed? +- [ ] Can BSP and AP cores access the same resource simultaneously? +- [ ] Does trusted code access untrusted region resources? +- [ ] Are proper synchronization mechanisms used (locks, atomics)? + +#### 3. Hardware Input Validation +**Risk**: Malicious or corrupted hardware providing unexpected data + +**Review Checklist**: +- [ ] What hardware inputs are processed (MMIO, registers, DMA)? +- [ ] How are hardware input values validated? +- [ ] Are MMIO BAR addresses properly validated before access? +- [ ] Are hardware timeouts and error conditions handled? + +#### 4. Secret Handling +**Risk**: Information disclosure, credential compromise + +**Review Checklist**: +- [ ] Where are secrets stored (keys, passwords, tokens)? +- [ ] How are secrets cleared after use (stack, heap, global data)? +- [ ] Are secrets stored in variables or configuration? +- [ ] Are default/hardcoded passwords used? +- [ ] Does password comparison use constant-time algorithms? +- [ ] Are side-channel attack mitigations implemented? + +**Example Secure Secret Clearing**: +```c +// Clear sensitive data from all locations +ZeroMem(Password, PasswordSize); // Clear variable +ZeroMem(PasswordBuffer, BufferSize); // Clear communication buffer +ZeroMem(&LocalPassword, sizeof(LocalPassword)); // Clear stack +``` + +#### 5. Register Lock Security +**Risk**: Security register bypass, configuration tampering + +**Review Checklist**: +- [ ] Which security registers need locking? +- [ ] When are registers locked (boot phase, timing)? +- [ ] Are register locks controlled by policy or variables? +- [ ] Can register locks be bypassed? +- [ ] Are registers locked in all boot paths (normal, S3, recovery, capsule)? +- [ ] Are registers locked in manufacturing/debug modes? + +#### 6. Secure Configuration +**Risk**: Security policy bypass, insecure defaults + +**Review Checklist**: +- [ ] Are security policies controlled by variables or PCDs? +- [ ] What are the default security configurations? +- [ ] How does security behave in different modes (S3, recovery, debug, manufacturing)? +- [ ] Can configuration be tampered with by unauthorized parties? + +#### 7. Replay/Rollback Protection +**Risk**: Downgrade attacks, replay of old vulnerable firmware + +**Review Checklist**: +- [ ] Are LSV (Lowest Supported Version) or SVN (Security Version Number) used? +- [ ] Where are version numbers stored and protected? +- [ ] How are timestamps, nonces, or monotonic counters implemented? +- [ ] Can version checks be bypassed? + +#### 8. Cryptography Implementation +**Risk**: Weak encryption, algorithm vulnerabilities, key management issues + +**Review Checklist**: +- [ ] Are signature verification algorithms properly implemented? +- [ ] Are deprecated/weak algorithms avoided (MD5, SHA1, RC4)? +- [ ] Is CRC/checksum used where cryptographic hash is needed? +- [ ] Are appropriate algorithms chosen (hash vs HMAC, symmetric vs asymmetric)? +- [ ] How are cryptographic keys deployed, stored, and destroyed? +- [ ] Are root keys vs session keys used appropriately? +- [ ] Is key material properly protected from disclosure? + +### Security Review Summary Checklist + +Use this comprehensive checklist during security-focused code reviews: + +| **Category** | **Key Review Questions** | +|--------------|--------------------------| +| **External Input** | What external inputs exist? How are they validated? Are checks on all paths? What happens on validation failure? Is SMM communication buffer validated? How are variables consumed? | +| **Race Conditions** | What critical resources exist? Can BSP/AP access same resource? Does trusted code access untrusted regions? | +| **Hardware Input** | What hardware inputs exist? How are they validated? Are MMIO BARs validated? | +| **Secret Handling** | Where are secrets stored? How are they cleared? Are they in variables? Default/hardcoded credentials? Constant-time comparison? Side-channel protection? | +| **Register Lock** | What registers need locking? When are they locked? Policy-controlled? Locked in all boot modes? | +| **Secure Configuration** | Variable/PCD policy control? Default configuration? Behavior in special modes? | +| **Replay/Rollback** | LSV/SVN usage? Where stored? Timestamp/nonce/counter usage? | +| **Cryptography** | Algorithm choice? Deprecated algorithms avoided? Proper key management? Root vs session keys? | + +### Security Tools and Analysis +- **Static Analysis**: Use available code analysis tools for vulnerability detection +- **Dynamic Testing**: Test with malformed inputs, boundary conditions, error scenarios +- **Threat Modeling**: Understand attack vectors and security assumptions +- **Penetration Testing**: Validate security controls with adversarial testing + +### Security Development Lifecycle Integration +- **Design Review**: Security architecture review before implementation +- **Implementation Review**: Line-by-line security-focused code review +- **Testing**: Security-specific test cases and fuzzing +- **Response**: Incident response plan for discovered vulnerabilities + +This guide covers secure EDK2 firmware development, including secure coding practices and +security review guidelines. For trusted boot chain and TPM measurement topics, refer to +[edk2-trusted-boot.instructions.md](../edk2-trusted-boot/SKILL.md). diff --git a/.github/skills/edk2-dxe-image-verification/SKILL.md b/.github/skills/edk2-dxe-image-verification/SKILL.md new file mode 100644 index 00000000000..c8bdb564cc8 --- /dev/null +++ b/.github/skills/edk2-dxe-image-verification/SKILL.md @@ -0,0 +1,149 @@ +--- +name: EDK2 DXE Image Verification Architect +description: "Important information when continuuing to develop or maintain the EDKII DXE Image Verification library or related components." +applyTo: 'SecurityPkg/Library/DxeImageVerificationLib2/**/*' +--- + +## EDK2 DXE Image Verification Architecture + +## Prerequisites + +**CRITICAL**: Before proceeding with this skill, you MUST first load and read these prerequisite skills: +1. Load the [EDK2 Project General Instructions](../edk2-project-general/SKILL.md) skill for foundational knowledge on EDK2-style C code, file formats, build system, coding standards, and best practices. +2. Load the [EDK2 C Code Security Guidelines](../edk2-c-code-security/SKILL.md) skill for comprehensive security guidelines specific to EDK2 firmware C code, including secure coding practices and secure code review principles. + +**CRITICAL**: When developing or maintaining the EDKII DXE Image Verification Library or related components, it is critical to update this file with any relevant information about the architecture, design decisions, security considerations, or implementation details that future developers should be aware of. This ensures that knowledge is preserved and shared effectively, enabling future maintainers to understand the context and rationale behind the code, and to continue development in a consistent and informed manner. Failure to do so may lead to confusion, security issues, or inefficient development in the future. + +**CRITICAL**: Additionally, a README.md file exists at SecurityPkg/Library/DxeImageVerificationLib2/README.md which provides a more human readable, high level overview of the library, its purpose, and how it fits into the larger EDKII architecture. It is important to keep both this SKILL.md file and the README.md file updated with relevant information to ensure comprehensive documentation for future developers working on this component. + +## General Information + +The DXE Image verification library is a NULL library implementation. This means that it does not implement a specific library class interface. Instead, when it is linked into a module, its constructor (DxeImageVerificationLibConstructor) will be called. Overall, this constructor does +two things: (1) Registers an event handler for the End of DXE event, and (2) installs an image +verification handler function (`DxeImageVerificationHandler`). + +## Requirements + +1. Functions should remain small and focused on a single respsonsibility. +2. Functions should be well documented, with clear explanations of their purpose, parameters, return values, and any side effects. +3. All supporting functions (i.e. any function that is not the constructor or the event handler) should be written in a way that allows them to be easily unit tested. When a function is created, unit tests should also be created to validate the functionality of that function. +4. No global variables should be used within any supporting functions and effort should be made to avoid global variables where possible. The only use of global variables allowed should be within the top level `OnReadyToBoot` event handler function and the `DxeImageVerificationHandler` +function. In those cases, the global variable should be returned as a pointer from a getter function that can be mocked during testing. +5. All files should have `\r\n` Line endings. + +## Building the library + +The below command can be used to build the library, and only the library. This is useful for development and testing purposes, as it allows for faster build times when only working on the library code. + +This command will build the DxeImageVerificationLib.inf module, using the CLANGPDB toolchain, for both DEBUG and RELEASE configurations and for IA32, X64, AARCH64 architectures. You can view the build log at `Build/CI_BUILDLOG.txt` and you can also review the actual build artifacts, which will be located in the `Build/SecurityPkg/` directory. There are subfolders that are dependent on the configuration and toolchain, so ensure you are looking in the correct location for the generated files. + +```bash +stuart_ci_build -c .pytool/CISettings.py -d CompilerPlugin=run -p SecurityPkg TOOL_CHAIN_TAG=CLANGPDB BUILDMODULE=SecurityPkg/Library/DxeImageVerificationLib2/DxeImageVerificationLib.inf +``` + +## Unit Testing + +Developing unit tests is done via the `GoogleTest` framework. Documentation related to how to write unit tests using this framework can be found in at [UnitTestFrameworkPkg's README](../../../UnitTestFrameworkPkg/ReadMe.md). All unit tests are currently placed in a single unit test INF located at `SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/*`. Tests should be grouped into separate files based on functionality being tested, but all tests are consolidated to this single INF to make it easier to build and run. + +To build and run the unit tests, you can use the below command, which will compile and run ONLY the test executable I specified below. This will both build and run unit tests and you can review the test results in the terminal, in the build output at `Build/SecurityPkg/`, and specifically the test results at `Build/TestSuites.xml`. + +```bash +stuart_ci_build -c .pytool/CISettings.py -d HostUnitTestCompilerPlugin=run -p SecurityPkg TOOL_CHAIN_TAG=GCC5 BUILDMODULE=SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/DxeImageVerificationLibGoogleTest.inf +``` + +### Code Coverage + +Building and running unit tests to validate pass/fail criteria is only the first step for full validation. Eventually we need to be concerned with ensuring the written unit tests are providing sufficient code coverage. To do this, we can utilize the following command to not only compile and run the test, but also generate code coverage results that can be reviewed. The following command generates a coverage xml file at `Build/SecurityPkg/HostTest//SecurityPkg_coverage.xml` that can be reviewed to determine which lines of code are being covered by the unit tests and which are not. This coverage file will contain a large amount of coverage results, most of which we do not care about. The main focus should be on the coverage results for the actual library code (i.e. code in `SecurityPkg/Library/DxeImageVerificationLib2`). You should use this information to write more / better unit tests to ensure that all paths are covered. If possible, you should have 100% coverage. + +```bash +stuart_ci_build -c .pytool/CISettings.py -d HostUnitTestCompilerPlugin=run -p SecurityPkg TOOL_CHAIN_TAG=GCC5 BUILDMODULE=SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/DxeImageVerificationLibGoogleTest.inf CODE_COVERAGE=TRUE CC_REORGANIZE=TRUE CC_FULL=TRUE CC_FLATTEN=TRUE +``` + +## Code Formatting + +Code formatting must be done for every commit. Formatting can be performed automatically using the following command. Do not waste time running it in the middle of active changes. Just run it once you believe the feature is finished. + +```bash +stuart_ci_build -c .pytool/CISettings.py -d -p SecurityPkg UncrustifyCheck=run UNCRUSTIFY_IN_PLACE=TRUE +``` + +## Policy Resolution (`Policy.c` / `Policy.h`) + +The first thing the verification handler must do for any image is decide +*how strict* it should be. That decision is driven by where the image +came from, encoded as one of the `IMAGE_*` image source constants, and +mapped to one of the `*_EXECUTE` / `*_ON_SECURITY_VIOLATION` policy +values defined in `Policy.h`. + +The classification logic lives in `Policy.c` and is intentionally split +into small, single-responsibility helpers so each branch is unit +testable in isolation. All four classifier helpers share the same +signature and contract: + +```c +EFI_STATUS +GetXxxImageType ( + IN CONST EFI_DEVICE_PATH_PROTOCOL *File, + OUT UINT32 *ImageType + ); +``` + +- `EFI_INVALID_PARAMETER` if either argument is `NULL`. +- `EFI_SUCCESS` only when the device path matches that helper's + category; `*ImageType` is then populated. +- `EFI_NOT_FOUND` otherwise (without modifying `*ImageType`). + +Helpers: + +| Helper | Probe | On match `*ImageType` becomes | +| --- | --- | --- | +| `GetFirmwareVolumeImageType` | `LocateDevicePath` + `OpenProtocol(EFI_FIRMWARE_VOLUME2_PROTOCOL)` | `IMAGE_FROM_FV` | +| `GetBlockIoImageType` | `LocateDevicePath` + `OpenProtocol(EFI_BLOCK_IO_PROTOCOL)`; inspects `Media->RemovableMedia` | `IMAGE_FROM_REMOVABLE_MEDIA` or `IMAGE_FROM_FIXED_MEDIA` | +| `GetSimpleFileSystemImageType` | `LocateDevicePath(EFI_SIMPLE_FILE_SYSTEM_PROTOCOL)` (used only when Block I/O didn't match) | `IMAGE_FROM_FIXED_MEDIA` | +| `GetDevicePathImageType` | Walks the device path looking for `MEDIA_RELATIVE_OFFSET_RANGE_DP` (option ROM) or `MSG_MAC_ADDR_DP` (network) nodes | `IMAGE_FROM_OPTION_ROM` or `IMAGE_FROM_REMOVABLE_MEDIA` | + +`GetImageType` is the orchestrator. It validates inputs, then probes +the helpers in the order above. The first helper that returns +`EFI_SUCCESS` wins. If all four return `EFI_NOT_FOUND`, `GetImageType` +sets `*ImageType = IMAGE_UNKNOWN` and still returns `EFI_SUCCESS` — the +caller distinguishes "classified" vs. "unclassified" by inspecting +`*ImageType`, not the status. The only error `GetImageType` returns is +`EFI_INVALID_PARAMETER`. + +`GetPolicyForImageType` is a pure function over the `IMAGE_*` value: + +- `IMAGE_FROM_FV` -> `ALWAYS_EXECUTE` (firmware volumes are trusted by + construction; this is hardcoded, not PCD-driven). +- `IMAGE_FROM_OPTION_ROM` -> `PcdOptionRomImageVerificationPolicy`. +- `IMAGE_FROM_REMOVABLE_MEDIA` -> `PcdRemovableMediaImageVerificationPolicy`. +- `IMAGE_FROM_FIXED_MEDIA` -> `PcdFixedMediaImageVerificationPolicy`. +- Anything else (notably `IMAGE_UNKNOWN`) -> `DENY_EXECUTE_ON_SECURITY_VIOLATION`. + This is the **fail-closed** default and must be preserved by future + maintainers. + +`GetExecutionPolicy` is the public entry point used by the verification +handler. It composes the two: validates inputs, calls `GetImageType`, +bubbles up `EFI_INVALID_PARAMETER` if it fails, then writes +`*Policy = GetPolicyForImageType (ImageType)` and returns +`EFI_SUCCESS`. Because `GetImageType` cannot otherwise fail, callers +that get `EFI_SUCCESS` always have a defined `*Policy` value, including +the fail-closed value for unclassified sources. + +### Design notes for future maintainers + +- Keep the helpers' signatures uniform. The matrix-style design is + what lets the unit tests exhaustively cover each branch with a + single mock pattern. +- The classification order in `GetImageType` matters and mirrors the + legacy `DxeImageVerificationLib`. Do not reorder without auditing + the security implications (e.g., a malicious device path that + matches multiple categories should still resolve to the most + trustworthy category first). +- Never broaden the `default` arm of `GetPolicyForImageType` to + something more permissive than `DENY_EXECUTE_ON_SECURITY_VIOLATION`. + The verification handler relies on this fail-closed behavior for + unrecognized image sources. +- The legacy `QUERY_USER_ON_SECURITY_VIOLATION` and + `ALLOW_EXECUTE_ON_SECURITY_VIOLATION` constants are kept for ABI + reasons but the verification handler explicitly rejects them — see + the `ASSERT` in `DxeImageVerificationHandler`. diff --git a/.github/skills/edk2-project-general/SKILL.md b/.github/skills/edk2-project-general/SKILL.md new file mode 100644 index 00000000000..9aa1d728708 --- /dev/null +++ b/.github/skills/edk2-project-general/SKILL.md @@ -0,0 +1,694 @@ +--- +name: EDK2-Style Workspace Instructions +description: Comprehensive instructions for working with EDK2-style C code, including file formats, build system, coding standards, and best practices. +applyTo: '**/*.{c,h,inf,dsc,dec,fdf}' +--- +MANDATORY: Completely understand and follow this instructions file. Failure to do so will cost significant time and +effort. + +# Repository Detection + +These instructions apply to repositories containing: +- `.dec` files (Package Declaration files) +- `.dsc` files (Platform Description files) +- `.inf` files (Module Information files) +- `.fdf` files (Flash Description files) +- Standard edk2 package structure + +# EDK2-Style C Code Instructions + +This file provides comprehensive instructions for working with EDK2-style C code in repositories containing `.dec` files. These instructions cover edk2 build systems, file formats, coding standards, and best practices. + +## EDK2 File Formats and Build System + +### Core File Types + +#### 1. DEC Files (Package Declaration) +- **Purpose**: Define package-level declarations including GUIDs, protocols, PPIs, library classes, and PCDs +- **Location**: Root of each package directory (e.g., `MdePkg/MdePkg.dec`) +- **Key Sections**: + - `[Defines]`: Package metadata and version information + - `[Includes]`: Include directory paths for header files + - `[LibraryClasses]`: Library class declarations + - `[Guids]`: GUID definitions with C-format GUID values + - `[Protocols]`: Protocol GUID definitions + - `[Ppis]`: PPI (PEIM-to-PEIM Interface) GUID definitions + - `[PcdsFixedAtBuild]`, `[PcdsPatchableInModule]`, etc.: PCD declarations +- **Documentation**: https://github.com/tianocore-docs/edk2-DecSpecification + +#### 2. DSC Files (Platform Description) +- **Purpose**: Define how to build a specific platform including component selection, library mappings, and PCD values +- **Location**: Platform-specific directories (e.g., `OvmfPkg/OvmfPkgX64.dsc`) +- **Key Sections**: + - `[Defines]`: Platform metadata, output directories, supported architectures + - `[LibraryClasses]`: Library class to implementation mappings + - `[Components]`: List of modules to build for the platform + - `[PcdsFixedAtBuild]`, `[PcdsPatchableInModule]`, etc.: PCD value assignments + - `[BuildOptions]`: Compiler and linker flags +- **Documentation**: https://github.com/tianocore-docs/edk2-DscSpecification + +#### 3. INF Files (Module Information) +- **Purpose**: Describe individual modules (drivers, libraries, applications) including sources, dependencies, and build requirements +- **Location**: Each module directory (e.g., `MdeModulePkg/Core/Dxe/DxeMain.inf`) +- **Key Sections**: + - `[Defines]`: Module metadata, type, version, entry point + - `[Sources]`: Source files to compile + - `[Packages]`: Package dependencies (references to .dec files) + - `[LibraryClasses]`: Required library classes + - `[Protocols]`, `[Ppis]`, `[Guids]`: Interface dependencies + - `[Pcd*]`: PCD usage declarations + - `[Depex]`: Dependency expressions for load order +- **Documentation**: https://github.com/tianocore-docs/edk2-InfSpecification + +#### 4. FDF Files (Flash Description) +- **Purpose**: Define flash device layout and firmware volume organization +- **Location**: Platform directories (e.g., `OvmfPkg/OvmfPkgX64.fdf`) +- **Key Sections**: + - `[FD.*]`: Flash device definitions with layout and size + - `[FV.*]`: Firmware volume definitions with file listings + - `[Rule.*]`: Rules for processing different file types + - `[Capsule.*]`: Capsule update definitions +- **Documentation**: https://github.com/tianocore-docs/edk2-FdfSpecification + +### Build System Architecture + +#### Build Process Overview +1. **Setup Phase**: Environment setup with `edksetup.bat` (Windows) or `edksetup.sh` (Linux) +2. **Parse Phase**: Parse DSC, FDF, INF, and DEC files to understand platform configuration +3. **AutoGen Phase**: Generate makefiles, dependency files, and AutoGen.c/AutoGen.h files +4. **Make Phase**: Compile source files and link binaries +5. **Flash Image Generation**: Create firmware volumes and flash images based on FDF + +#### Key Build Tools +- **build.exe**: Main build command (Python-based) +- **GenFds**: Flash device image generation +- **GenFw**: Firmware file generation +- **VfrCompile**: Visual Forms Representation compiler +- **GenC**: C code generation for PCDs + +#### Environment Variables +- `WORKSPACE`: Root directory of the edk2 source tree +- `PACKAGES_PATH`: Additional package search paths (colon/semicolon separated) +- `EDK_TOOLS_PATH`: Path to BaseTools directory +- `CONF_PATH`: Configuration file directory (defaults to Conf/) + +### Platform Configuration Database (PCD) System + +PCDs provide a standardized way to configure platform and module behavior without code changes. + +#### PCD Types +- **FixedAtBuild**: Compile-time constants, no runtime overhead +- **PatchableInModule**: Can be modified by tools after compilation +- **Dynamic**: Runtime configurable via PCD database +- **DynamicEx**: Dynamic PCDs with extended token space support +- **FeatureFlag**: Boolean compile-time flags for conditional compilation + +#### PCD Usage Patterns +- Declare in DEC files with default values and help text +- Override values in DSC files for platform-specific configuration +- Reference in INF files to indicate usage +- Access in C code using `PcdGet*()` and `PcdSet*()` macros + +**Documentation**: https://github.com/tianocore-docs/edk2-PcdSpecification + +## EDK2 Module Types and Architecture + +### Module Types +- **SEC**: Security Phase, earliest boot code +- **PEI_CORE**: PEI Core module +- **PEIM**: PEI Module +- **DXE_CORE**: DXE Core module +- **DXE_DRIVER**: Standard DXE driver +- **DXE_RUNTIME_DRIVER**: DXE driver that remains resident during OS runtime +- **DXE_SMM_DRIVER**: System Management Mode driver +- **UEFI_DRIVER**: UEFI-compliant driver +- **UEFI_APPLICATION**: UEFI application +- **BASE**: Library module with no architectural dependencies + +### Boot Phases +1. **SEC (Security)**: Initial CPU setup, temporary memory initialization +2. **PEI (Pre-EFI Initialization)**: Memory initialization, device discovery +3. **DXE (Driver Execution Environment)**: Full driver model, protocol installation +4. **BDS (Boot Device Selection)**: Boot manager, device enumeration +5. **TSL (Transient System Load)**: OS loader execution +6. **RT (Runtime)**: OS runtime services + +## Coding Standards and Best Practices + +### File Organization +- Use package-based organization (MdePkg, MdeModulePkg, etc.) +- Each package contains Library/, Universal/, Pci/, etc. subdirectories +- Include files in Include/ subdirectories with proper hierarchy +- Test files in separate test directories + +### Header File Structure +```c +/** @file + Brief description of the file. + + Detailed description if needed. + + Copyright (c) Year, Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent + +**/ + +#pragma once + +#include +// Other includes... + +// Definitions, structures, function prototypes... +``` + +### Function Documentation +Use Doxygen-style comments: +```c +/** + Brief function description. + + Detailed description if needed. + + @param[in] Parameter1 Description of input parameter. + @param[in, out] Parameter2 Description of input/output parameter. + @param[out] Parameter3 Description of output parameter. + + @retval EFI_SUCCESS The operation completed successfully. + @retval EFI_INVALID_PARAMETER Invalid parameter passed. + +**/ +EFI_STATUS +EFIAPI +FunctionName ( + IN UINT32 Parameter1, + IN OUT VOID *Parameter2, + OUT UINT32 *Parameter3 + ); +``` + +## EDK2 C Coding Standards and Conventions + +> **Reference**: https://github.com/tianocore-docs/edk2-CCodingStandardsSpecification + +All EDK2-style C code must follow the official TianoCore C Coding Standards Specification. This section provides essential conventions and examples for consistent, readable, and maintainable firmware code. + +### Naming Conventions + +#### Identifiers (Variables and Functions) +- **Format**: PascalCase - each word capitalized, no underscores +- **Examples**: `MyVariable`, `GetSystemConfiguration` +- **Length**: No limit, but 10-30 characters recommended +- **Readability**: Names must clearly represent purpose + +**Correct Examples**: +```c +UINTN BufferSize; +EFI_STATUS Status; +EFI_SYSTEM_TABLE *SystemTable; +MEMORY_DESCRIPTOR *MemoryMap; +``` + +**Incorrect Examples**: +```c +UINTN buf_size; // Underscores not allowed +int buffer_sz; // Standard C types not allowed +UINTN bs; // Unclear abbreviation +``` + +#### Function and Data Names +- **Functions**: PascalCase (`AllocateMemory`, `InitializeDriver`) +- **Structure Types**: PascalCase (`DRIVER_INSTANCE`, `PROTOCOL_INTERFACE`) +- **No Hungarian notation** (except 'g' for global, 'm' for module, 'p' for pointer) +- **Acronyms**: Only first letter capitalized (`PciDevice`, not `PCIDevice`) + +#### Macros and Typedefs +- **Format**: ALL_CAPS with underscores +- **Examples**: `MAX_BUFFER_SIZE`, `ALIGN_POINTER`, `EFI_SIGNATURE_32` + +```c +#define MAX_DEVICE_COUNT 16 +#define SIGNATURE_32(A, B, C, D) ((UINT32)(A) | ((UINT32)(B) << 8) | ((UINT32)(C) << 16) | ((UINT32)(D) << 24)) +typedef UINT32 DEVICE_TYPE; +typedef enum { + DeviceTypeUnknown, + DeviceTypePci, + DeviceTypeUsb +} DEVICE_CATEGORY; +``` + +#### Global and Module Variables +- **Global variables**: Prefix with 'g' (`gSystemTable`, `gBootServices`) +- **Module variables**: Prefix with 'm' (`mDriverInstance`, `mPrivateData`) +- **Pointer variables**: Optionally prefix with 'p' (`pBuffer`, `pDeviceList`) + +```c +extern EFI_SYSTEM_TABLE *gST; // Global +extern EFI_BOOT_SERVICES *gBS; // Global +STATIC DRIVER_INSTANCE mDriverInstance; // Module scope +STATIC EFI_EVENT mExitBootServicesEvent; // Module scope +``` + +### File Header Requirements + +Every file must begin with a Doxygen file header: + +```c +/** @file + Brief description of the file's purpose. + + Detailed description of the file's contents and other useful + information for a person viewing the file for the first time. + + Copyright (C) 2020 - 2024, Acme Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent + + @par Revision Reference: + - UEFI Version 2.8 + - PI Version 1.7 + + @par Glossary: + - API - Application Programming Interface + - GUID - Globally Unique Identifier +**/ + +#pragma once + +#include +#include +// Other includes... + +// Definitions, structures, function prototypes... +``` + +### Include File Guards +- Use pragma once + +### Function Documentation and Declaration + +Use Doxygen-style comments for all functions: + +```c +/** + Allocates and initializes a memory buffer for device communication. + + This function allocates a buffer of the specified size and initializes + it with default values for device communication protocols. + + @param[in] DeviceType Type of device requiring buffer allocation. + @param[in] BufferSize Size in bytes of buffer to allocate. + @param[out] Buffer Pointer to allocated and initialized buffer. + @param[in, out] ActualSize On input, requested size. On output, actual allocated size. + + @retval EFI_SUCCESS Buffer allocated and initialized successfully. + @retval EFI_INVALID_PARAMETER Invalid DeviceType or NULL pointer provided. + @retval EFI_OUT_OF_RESOURCES Insufficient memory to allocate buffer. + @retval EFI_UNSUPPORTED DeviceType not supported. + +**/ +EFI_STATUS +EFIAPI +AllocateDeviceBuffer ( + IN DEVICE_TYPE DeviceType, + IN UINTN BufferSize, + OUT VOID **Buffer, + IN OUT UINTN *ActualSize OPTIONAL + ); +``` + +#### Function Declaration Formatting +- Opening brace in column 1 on its own line +- Parameters aligned with proper `IN`, `OUT`, `OPTIONAL` modifiers +- Return type on separate line +- `EFIAPI` calling convention when required + +### Formatting and Spacing Rules + +#### General Rules +- **No tabs**: Use spaces only +- **Indentation**: 2 spaces per level +- **Line length**: Maximum 120 characters +- **Line endings**: CRLF (0x0D 0x0A) for all source files (tool-specific files may differ if required) +- **File ending**: All source files must end with CRLF (tool-specific files may differ if required) + +#### Vertical Spacing +- **One statement per line** - no exceptions +- **Blank lines**: Group related code blocks +- **Braces**: Opening brace on same line for simple predicates, own line for complex + +```c +// Simple predicate - brace on same line +if (Status == EFI_SUCCESS) { + ProcessSuccess (); +} + +// Complex predicate - brace on own line +if ((DeviceType == PciDevice) && + (BufferSize > MIN_BUFFER_SIZE) && + (Buffer != NULL)) +{ + AllocateBuffer (); +} +``` + +#### Horizontal Spacing +- **Binary operators**: Space before and after (`A + B`, `X == Y`) +- **Unary operators**: No space (`!Found`, `++Index`) +- **Commas/semicolons**: Space after if code follows +- **Parentheses**: Space before opening except for function calls +- **Function calls**: No space before opening parenthesis + +```c +// Correct spacing +if (Index < MaxCount) { + Result = (Value1 + Value2) * Multiplier; + FunctionCall (Parameter1, Parameter2); +} + +// Multi-line function calls +Status = gBS->AllocatePool ( + EfiBootServicesData, + sizeof (DRIVER_INSTANCE), + (VOID **)&PrivateData + ); +``` + +### Data Types and Declarations + +#### UEFI Data Types Only +Use only UEFI-defined data types, never standard C types: + +```c +// Correct UEFI types +BOOLEAN Found; +UINT8 ByteValue; +UINT16 WordValue; +UINT32 DwordValue; +UINT64 QwordValue; +UINTN NativeSize; +CHAR16 *UnicodeString; +VOID *GenericPointer; +EFI_STATUS Status; + +// WRONG - Standard C types not allowed +bool found; // Use BOOLEAN +int value; // Use UINT32 or appropriate size +char *string; // Use CHAR8* or CHAR16* +``` + +#### Structure Definitions +- Always use typedef format +- Document each member +- Use alignment for readability + +```c +/// Brief description of this structure. +/// Detailed description if needed. +typedef struct { + UINT32 Signature; ///< Unique structure identifier. + EFI_HANDLE Handle; ///< Associated device handle. + LIST_ENTRY Link; ///< Link to next instance. + DEVICE_TYPE Type; ///< Type of device represented. + BOOLEAN Initialized; ///< TRUE if instance is initialized. +} DRIVER_INSTANCE; + +#define DRIVER_INSTANCE_SIGNATURE SIGNATURE_32('D','R','V','I') +``` + +#### Enumerated Types +- Must end with maximum element +- Should begin with minimum element +- Document purpose of each member + +```c +/// Device power states for power management. +typedef enum { + DevicePowerStateMinimum = 0, ///< Minimum valid power state. + DevicePowerD0, ///< Fully powered and operational. + DevicePowerD1, ///< Reduced power, context preserved. + DevicePowerD2, ///< Lower power, some context lost. + DevicePowerD3, ///< Lowest power, most context lost. + DevicePowerStateMaximum ///< Maximum power state value. +} DEVICE_POWER_STATE; +``` + +### Control Flow and Statements + +#### Conditional Statements +```c +// Simple conditions +if (Found) { + ProcessItem (); +} + +if (Buffer != NULL) { + FreePool (Buffer); + Buffer = NULL; +} + +// Complex conditions with explicit comparisons +if ((Index < ArraySize) && + (Array[Index] != NULL) && + (Status == EFI_SUCCESS)) +{ + ProcessArrayElement (Array[Index]); +} + +// Switch statements +switch (DeviceType) { +case PciDevice: + InitializePciDevice (); + break; + +case UsbDevice: + InitializeUsbDevice (); + break; + +default: + return EFI_UNSUPPORTED; +} +``` + +#### Loop Constructs +```c +// For loops +for (Index = 0; Index < DeviceCount; Index++) { + if (DeviceArray[Index] == NULL) { + continue; + } + ProcessDevice (DeviceArray[Index]); +} + +// While loops +while (MoreDataAvailable ()) { + Data = GetNextData (); + ProcessData (Data); +} + +// Do-while loops +do { + Status = AttemptOperation (); + RetryCount++; +} while (EFI_ERROR (Status) && (RetryCount < MAX_RETRIES)); +``` + +### Error Handling +- Always use EFI_STATUS return values for functions that can fail +- Use ASSERT() for debug-time validation of programmer errors +- Use proper EFI error codes +- Clean up resources on all error paths + +```c +EFI_STATUS +EFIAPI +InitializeDevice ( + IN EFI_HANDLE DeviceHandle + ) +{ + EFI_STATUS Status; + DEVICE_CONTEXT *Context; + + if (DeviceHandle == NULL) { + return EFI_INVALID_PARAMETER; + } + + Context = AllocateZeroPool (sizeof (DEVICE_CONTEXT)); + if (Context == NULL) { + return EFI_OUT_OF_RESOURCES; + } + + Status = InitializeDeviceHardware (DeviceHandle, Context); + if (EFI_ERROR (Status)) { + FreePool (Context); + return Status; + } + + Status = RegisterDeviceProtocol (DeviceHandle, Context); + if (EFI_ERROR (Status)) { + CleanupDeviceHardware (Context); + FreePool (Context); + return Status; + } + + return EFI_SUCCESS; +} +``` + +### Memory Management +- Use `AllocatePool()`, `AllocateZeroPool()` for dynamic allocation +- Always check for NULL returns from allocation functions +- Use `FreePool()` to release allocated memory +- Prefer stack allocation for small, temporary buffers +- Set pointers to NULL after freeing + +```c +// Dynamic allocation +Buffer = AllocateZeroPool (BufferSize); +if (Buffer == NULL) { + return EFI_OUT_OF_RESOURCES; +} + +// Always free and nullify +if (Buffer != NULL) { + FreePool (Buffer); + Buffer = NULL; +} +``` + +### Comments and Documentation + +#### Internal Comments +- Use C++ style comments (`//`) for local comments +- Blank line before comment blocks +- Comments explain why, not what +- Match indentation of code + +```c +// +// Initialize the device list to prepare for enumeration. +// This must be done before any device detection operations. +// +Status = InitializeDeviceList (); + +// Check if the device is already initialized to avoid duplicate work +if (Device->Initialized) { + return EFI_ALREADY_STARTED; +} +``` + +#### What NOT to Comment +- No comment markers like `BUGBUG`, `FIX_THIS`, `TODO` in code +- No personal names or initials +- Use bug tracking systems instead of code markers + +This comprehensive coding standard ensures consistent, readable, and maintainable EDK2-style C code that follows official TianoCore specifications and best practices. + +## Common EDK2 Libraries and APIs + +### Essential Libraries +- **BaseLib**: Basic CPU and string manipulation functions +- **BaseMemoryLib**: Memory operation functions +- **UefiLib**: UEFI-specific utility functions +- **UefiBootServicesTableLib**: Access to UEFI Boot Services +- **UefiRuntimeServicesTableLib**: Access to UEFI Runtime Services +- **DebugLib**: Debug printing and assertion functions +- **PrintLib**: Formatted string printing functions +- **DevicePathLib**: Device path manipulation +- **HiiLib**: Human Interface Infrastructure functions + +### Common APIs +```c +// Debug printing +DEBUG ((DEBUG_INFO, "Message: %d\n", Value)); + +// Memory operations +CopyMem (Dest, Src, Size); +SetMem (Buffer, Size, Value); +ZeroMem (Buffer, Size); + +// String operations (use safe variants - see edk2-c-code-security.instructions.md) +StrCpyS (Destination, DestMax, Source); +StrLen (String); +AsciiStrCmp (String1, String2); + +// UEFI services access +Status = gBS->AllocatePool (EfiBootServicesData, Size, &Buffer); +Status = gRT->SetVariable (Name, Guid, Attributes, Size, Data); +``` +## References and Documentation + +### Official Specifications +- **Build Specification**: https://github.com/tianocore-docs/edk2-BuildSpecification +- **DEC Specification**: https://github.com/tianocore-docs/edk2-DecSpecification +- **DSC Specification**: https://github.com/tianocore-docs/edk2-DscSpecification +- **INF Specification**: https://github.com/tianocore-docs/edk2-InfSpecification +- **FDF Specification**: https://github.com/tianocore-docs/edk2-FdfSpecification +- **PCD Specification**: https://github.com/tianocore-docs/edk2-PcdSpecification +- **UNI Specification**: https://github.com/tianocore-docs/edk2-UniSpecification +- **VFR Specification**: https://github.com/tianocore-docs/edk2-VfrSpecification +- **Module Write Guide**: https://github.com/tianocore-docs/edk2-ModuleWriteGuide +- **UEFI Driver Writers Guide**: https://github.com/tianocore-docs/edk2-UefiDriverWritersGuide + +### Key Resources +- **TianoCore.org**: Main EDK2 project website +- **EDK2 GitHub**: https://github.com/tianocore/edk2 +- **EDK2 Documentation**: https://github.com/tianocore/tianocore.github.io/wiki +- **UEFI Specification**: Official UEFI Forum specifications +- **PI Specification**: Platform Initialization specifications + +## Common Patterns and Examples + +### Driver Entry Point +```c +EFI_STATUS +EFIAPI +DriverEntry ( + IN EFI_HANDLE ImageHandle, + IN EFI_SYSTEM_TABLE *SystemTable + ) +{ + EFI_STATUS Status; + + // Initialize driver + Status = InitializeDriver (); + if (EFI_ERROR (Status)) { + return Status; + } + + // Install protocols + Status = gBS->InstallProtocolInterface ( + &ImageHandle, + &gMyProtocolGuid, + EFI_NATIVE_INTERFACE, + &mMyProtocol + ); + + return Status; +} +``` + +### Library Constructor +```c +EFI_STATUS +EFIAPI +LibraryConstructor ( + IN EFI_HANDLE ImageHandle, + IN EFI_SYSTEM_TABLE *SystemTable + ) +{ + // Initialize library state + return EFI_SUCCESS; +} +``` + +### Protocol Implementation +```c +typedef struct { + UINT64 Signature; + MY_PROTOCOL Protocol; + EFI_HANDLE Handle; + // Private data members +} MY_PROTOCOL_INSTANCE; + +#define MY_PROTOCOL_INSTANCE_FROM_PROTOCOL(a) \ + CR (a, MY_PROTOCOL_INSTANCE, Protocol, MY_PROTOCOL_SIGNATURE) +``` diff --git a/.github/skills/edk2-trusted-boot/SKILL.md b/.github/skills/edk2-trusted-boot/SKILL.md new file mode 100644 index 00000000000..a197977a71e --- /dev/null +++ b/.github/skills/edk2-trusted-boot/SKILL.md @@ -0,0 +1,239 @@ +--- +name: EDK2 Trusted Boot Chain and Firmware Security Architecture +description: "Use when working on TPM measurement, PCR allocation, TCG event logs, trusted boot chain, attestation, SecurityPkg/Tcg modules, Boot Guard integration, MOR variables, or OPAL storage security." +--- + +# Trusted Boot Chain and Firmware Security Architecture + +## Overview of Firmware Security Concepts + +Understanding trusted boot chains is essential for implementing secure firmware that integrates with platform security architecture. The TCG (Trusted Computing Group) specifications define how firmware components are measured, stored, and verified to establish a chain of trust from hardware to operating system. + +### Key Security Specifications +- **NIST SP800-155**: Guidelines for platform firmware resiliency and measurement +- **TCG Platform Firmware Profile (PFP)**: Defines firmware measurement requirements +- **TCG Reference Integrity Manifest (RIM-IM)**: Framework for firmware integrity manifests +- **TCG PC Client RIM**: Reference implementation for PC platforms +- **TCG Firmware Integrity Measurement (FIM)**: Standards for firmware measurement + +### Root of Trust for Measurement (RTM) +The platform firmware acts as a Static Root of Trust for Measurement (SRTM), which: +- Measures firmware components into TPM Platform Configuration Registers (PCR) +- Records measurement actions in an event log +- Establishes a chain of trust through cryptographic hashing +- Enables remote attestation for platform verification + +## TPM Platform Configuration Registers (PCR) + +### PCR Allocation and Usage + +**PCR extend equation**: `PCR(new) = HASH(PCR(old) || HASH(Data))` + +| **PCR Index** | **Purpose** | **Content** | +|---------------|-------------|-------------| +| **PCR[0]** | SRTM, BIOS, Host Platform Extensions | Platform firmware (PEI, DXE, SMM), embedded option ROMs, PI drivers, ACPI tables, non-host components | +| **PCR[1]** | Host Platform Configuration | Microcode, SMBIOS tables, setup variables, policy configuration, device lists | +| **PCR[2]** | UEFI Driver and Application Code | Third-party UEFI drivers, option ROMs, non-host updatable components, SPDM device firmware | +| **PCR[3]** | UEFI Driver and Application Configuration | Third-party configuration data, non-host updatable config, SPDM device configuration | +| **PCR[4]** | UEFI Boot Manager Code | OS loader, boot attempts, pre-OS applications | +| **PCR[5]** | Boot Manager Configuration and Data | GPT/partition tables, ExitBootServices events | +| **PCR[6]** | Host Platform Manufacturer Specific | Platform-specific OEM data | +| **PCR[7]** | Secure Boot Policy and Authority | Secure boot variables (PK, KEK, db, dbx), debug mode, DMA protection status | + +### PCR Usage Rules + +**Code vs Configuration Pattern**: +- Even-numbered PCRs (0,2,4,6): Code components +- Odd-numbered PCRs (1,3,5,7): Configuration and data + +**Ownership Pattern**: +- PCR[0-1]: OEM platform firmware +- PCR[2-3]: Third-party components +- PCR[4-5]: OS boot related +- PCR[7]: Security policy + +## Measurement Implementation in EDK2 + +### Key Modules and Libraries + +**PEI Phase Measurement**: +- `SecurityPkg/Tcg/Tcg2Pei` - Main measurement module for PEI +- `SecurityPkg/Library/PeiTpmMeasurementLib` - PEI measurement services +- Measures firmware volumes (FV) at granularity level +- Records measurements in `EFI_TCG_EVENT2_HOB` + +**DXE Phase Measurement**: +- `SecurityPkg/Tcg/Tcg2Dxe` - Main measurement module for DXE +- `SecurityPkg/Library/DxeTpm2MeasureBootLib` - PE image measurement +- Measures individual PE images through `EFI_SECURITY2_ARCH_PROTOCOL` +- Handles TCG event log management + +**Common Libraries**: +- `SecurityPkg/Library/TcgEventLogRecordLib` - Event log recording services +- `MdeModulePkg/Library/TpmMeasurementLib` - Generic measurement interface +- `SecurityPkg/Library/HashLibBaseCryptoRouter` - Crypto-agile hash support + +### Measurement Event Types + +| **Event Type** | **Usage** | **PCR** | +|----------------|-----------|---------| +| `EV_S_CRTM_VERSION` | SRTM version string | PCR[0] | +| `EV_EFI_PLATFORM_FIRMWARE_BLOB` | Firmware volumes | PCR[0] | +| `EV_EFI_BOOT_SERVICES_DRIVER` | UEFI drivers | PCR[2] | +| `EV_EFI_BOOT_SERVICES_APPLICATION` | UEFI applications | PCR[4] | +| `EV_EFI_VARIABLE_DRIVER_CONFIG` | Secure boot variables | PCR[7] | +| `EV_EFI_VARIABLE_BOOT` | Boot variables | PCR[1] | +| `EV_SEPARATOR` | Boot phase separator | PCR[0-7] | +| `EV_EFI_ACTION` | Boot actions and events | Various | + +### Measurement Exclusion and Pre-hashing + +**Measurement Exclusion PPI**: +```c +// Exclude already-measured FV (e.g., by hardware root of trust) +EFI_PEI_FIRMWARE_VOLUME_INFO_MEASUREMENT_EXCLUDED_PPI +``` + +**Pre-hashed FV Support**: +```c +// Use hardware-provided hash instead of re-calculating +EDKII_PEI_FIRMWARE_VOLUME_INFO_PREHASHED_FV_PPI +``` + +**Stored Hash Verification**: +```c +// Verify FV against stored hash, then use for measurement +EDKII_PEI_FIRMWARE_VOLUME_INFO_STORED_HASH_FV_PPI +``` + +## Platform Security Implementation + +### Hardware Root of Trust Integration + +**Intel Boot Guard Integration**: +- ACM (Authenticated Code Module) measures initial FV +- Platform reports `EV_S_CRTM_CONTENTS` with ACM, Key Manifest, Boot Policy Manifest +- `TCG_EfiStartupLocalityEvent` for Locality 3 startup +- Pre-hashed FV support for Boot Guard verified components + +**Error Handling and Recovery**: +- TPM error detection creates `EFI_TPM_ERROR` HOB +- Status code reporting via `REPORT_STATUS_CODE()` +- Error separator events (`EV_SEPARATOR` with 0x00000001) cap PCRs +- Platform-specific error handling callbacks + +### TPM Device Management + +**Device Selection and Interface**: +- Support TPM 2.0 only (no TPM 1.2) +- FIFO and CRB interface detection +- Dynamic interface type configuration via `PcdActiveTpmInterfaceType` +- Device startup coordination via PPIs + +**Bank Selection and Crypto Agility**: +- Multiple hash algorithm support (SHA256, SHA384, SHA512, SM3) +- Runtime bank reconfiguration via Physical Presence +- Hash algorithm capability synchronization +- Crypto-agile event log support + +### Physical Presence and Management + +**Physical Presence Operations**: +- TPM Clear, Enable/Disable hierarchies +- PCR bank selection and reconfiguration +- Platform hierarchy authentication +- Vendor-specific extensions + +**Management Flow**: +1. OS submits PP request via ACPI `_DSM` method +2. Request stored in `Tcg2PhysicalPresence` variable +3. Platform BDS processes request before EndOfDxe +4. User confirmation for destructive operations +5. TPM configuration and system reset + +## Security Policy Implementation + +### Memory Overwrite Request (MOR) + +**MOR Variable Management**: +- `MemoryOverwriteRequestControl` - Controls memory clearing +- `MemoryOverwriteRequestControlLock` - Protects MOR variable +- SMM-based variable protection against tampering +- Secret key unlock mechanism (MORLock v2) + +**Platform Integration**: +- Memory initialization checks MOR variable +- Clear all system memory if MOR requested +- TPer reset for TCG storage devices +- Trusted storage connection before EndOfDxe + +### TCG Storage Security + +**OPAL Password Management**: +- User password prompt and validation +- S3 auto-unlock via secure LockBox storage +- Password update and device management +- PSID revert and secure erase capabilities + +**BlockSid Protection**: +- Prevent unauthorized SID access +- Physical Presence controlled policy +- Applied during normal boot and S3 resume +- Blocks malicious device locking attacks + +## Attestation and Verification + +### Remote Attestation Flow + +**Device Verification**: +1. TPM Endorsement Key (EK) verification against vendor CA +2. Attestation Key (AK) challenge-response protocol +3. TPM device authenticity confirmation + +**Event Log Verification**: +1. Quote generation with AK-signed PCR values +2. Event log replay to reproduce PCR values +3. Comparison with Reference Integrity Manifests (RIM) +4. Policy-based validation of measured components + +### Event Log Management + +**Event Log Interfaces**: +- `EFI_TCG2_PROTOCOL.GetEventLog()` - Runtime access +- `EFI_TCG2_FINAL_EVENTS_TABLE` - Post-GetEventLog events +- TPM2 ACPI table event log exposure +- Event log format compliance with TCG specifications + +## Platform Developer Checklist + +### Essential Implementation Requirements + +**TPM Measurement**: +- [ ] Configure `PcdTcgPfpMeasurementRevision` for compliance +- [ ] Set `PcdFirmwareVersionString` for version measurement +- [ ] Report all FV information in PEI phase +- [ ] Link `DxeTpm2MeasureBootLib` as LAST instance for SecurityStubDxe +- [ ] Measure platform-specific components (microcode, ACPI, SMBIOS) + +**Device Configuration**: +- [ ] Use TPM 2.0 only (no TPM 1.2 support) +- [ ] Link appropriate `Tpm2DeviceLib` instance +- [ ] Configure TPM interface detection and selection +- [ ] Set `PcdTpm2InitializationPolicy` appropriately + +**Physical Presence Integration**: +- [ ] Call PP processing in platform BDS before EndOfDxe +- [ ] Connect trusted consoles for user confirmation +- [ ] Support crypto-agile hash libraries +- [ ] Implement TPM hierarchy management + +**Security Policy**: +- [ ] Check and process MOR variable in memory initialization +- [ ] Connect trusted storages before EndOfDxe +- [ ] Enable BlockSid by default +- [ ] Configure ACPI tables with proper PCDs + +**Error Handling**: +- [ ] Register ReportStatusCode callback for TPM errors +- [ ] Implement platform-specific error responses +- [ ] Handle TPM startup failures in S3 resume diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h b/CryptoPkg/Include/Library/BaseCryptLib.h index a9ddcc0162f..ef5bc018f51 100644 --- a/CryptoPkg/Include/Library/BaseCryptLib.h +++ b/CryptoPkg/Include/Library/BaseCryptLib.h @@ -2588,6 +2588,46 @@ AuthenticodeVerify ( IN UINTN HashSize ); +/** + Compute the PE/COFF Authenticode-style image hash of an image as described + in "Windows Authenticode Portable Executable Signature Format". + + The caller selects the digest algorithm by HashType (e.g. + gEfiCertSha256Guid, gEfiCertSha384Guid). + + If the Data buffer is too small to hold the contents of the digest, + the error EFI_BUFFER_TOO_SMALL is returned and DigestSize is set to + the required buffer size to obtain the data. + + @param[in] FileBuffer Pointer to the in-memory PE/COFF image. + @param[in] FileSize Size of FileBuffer in bytes. + @param[in] HashType Signature-type GUID identifying the hash + algorithm to use. + @param[out] Digest Caller-provided buffer that receives the + computed digest. Must be at least + SHA512_DIGEST_SIZE bytes. + @param[out] DigestSize On success, receives the digest length in + bytes. + + @retval EFI_SUCCESS Digest was computed successfully. + @retval EFI_INVALID_PARAMETER A required pointer is NULL. + @retval EFI_BUFFER_TOO_SMALL DigestSize is too small for the + requested hash algorithm. + @retval EFI_UNSUPPORTED HashType is not a recognized image + hash algorithm, or this interface is + not supported by the underlying + library instance. +**/ +EFI_STATUS +EFIAPI +GetAuthenticodeHash ( + IN VOID *FileBuffer, + IN UINTN FileSize, + IN CONST EFI_GUID *HashType, + OUT UINT8 *Digest, + OUT UINTN *DigestSize + ); + /** Verifies the validity of a RFC3161 Timestamp CounterSignature embedded in PE/COFF Authenticode signature. diff --git a/CryptoPkg/Library/BaseCryptLibNull/Pk/CryptAuthenticodeNull.c b/CryptoPkg/Library/BaseCryptLibNull/Pk/CryptAuthenticodeNull.c index 62e8400c4cc..dc17f6dbae6 100644 --- a/CryptoPkg/Library/BaseCryptLibNull/Pk/CryptAuthenticodeNull.c +++ b/CryptoPkg/Library/BaseCryptLibNull/Pk/CryptAuthenticodeNull.c @@ -43,3 +43,47 @@ AuthenticodeVerify ( ASSERT (FALSE); return FALSE; } + +/** + Compute the PE/COFF Authenticode-style image hash of an image as described + in "Windows Authenticode Portable Executable Signature Format". + + The caller selects the digest algorithm by HashType (e.g. + gEfiCertSha256Guid, gEfiCertSha384Guid). + + If the Data buffer is too small to hold the contents of the digest, + the error EFI_BUFFER_TOO_SMALL is returned and DigestSize is set to + the required buffer size to obtain the data. + + @param[in] FileBuffer Pointer to the in-memory PE/COFF image. + @param[in] FileSize Size of FileBuffer in bytes. + @param[in] HashType Signature-type GUID identifying the hash + algorithm to use. + @param[out] Digest Caller-provided buffer that receives the + computed digest. Must be at least + SHA512_DIGEST_SIZE bytes. + @param[out] DigestSize On success, receives the digest length in + bytes. + + @retval EFI_SUCCESS Digest was computed successfully. + @retval EFI_INVALID_PARAMETER A required pointer is NULL. + @retval EFI_BUFFER_TOO_SMALL DigestSize is too small for the + requested hash algorithm. + @retval EFI_UNSUPPORTED HashType is not a recognized image + hash algorithm, or this interface is + not supported by the underlying + library instance. +**/ +EFI_STATUS +EFIAPI +GetAuthenticodeHash ( + IN VOID *FileBuffer, + IN UINTN FileSize, + IN CONST EFI_GUID *HashType, + OUT UINT8 *Digest, + OUT UINTN *DigestSize + ) +{ + ASSERT (FALSE); + return EFI_UNSUPPORTED; +} diff --git a/CryptoPkg/Test/Mock/Include/GoogleTest/Library/MockBaseCryptLib.h b/CryptoPkg/Test/Mock/Include/GoogleTest/Library/MockBaseCryptLib.h index d8a47d458f0..146d90fd037 100644 --- a/CryptoPkg/Test/Mock/Include/GoogleTest/Library/MockBaseCryptLib.h +++ b/CryptoPkg/Test/Mock/Include/GoogleTest/Library/MockBaseCryptLib.h @@ -869,9 +869,7 @@ struct MockBaseCryptLib { IN CONST UINT8 *KeyPassword, IN UINT8 *InData, IN UINTN InDataSize, - // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken IN CONST UINT8 *SignCert, - // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken IN UINTN SignCertSize, IN UINT8 *OtherCerts OPTIONAL, OUT UINT8 **SignedData, @@ -942,6 +940,18 @@ struct MockBaseCryptLib { ) ); + MOCK_FUNCTION_DECLARATION ( + EFI_STATUS, + GetAuthenticodeHash, + ( + IN VOID *FileBuffer, + IN UINTN FileSize, + IN CONST EFI_GUID *HashType, + OUT UINT8 *Digest, + OUT UINTN *DigestSize + ) + ); + MOCK_FUNCTION_DECLARATION ( BOOLEAN, ImageTimestampVerify, diff --git a/CryptoPkg/Test/Mock/Library/GoogleTest/MockBaseCryptLib/MockBaseCryptLib.cpp b/CryptoPkg/Test/Mock/Library/GoogleTest/MockBaseCryptLib/MockBaseCryptLib.cpp index f5144911963..6a1c92b9c48 100644 --- a/CryptoPkg/Test/Mock/Library/GoogleTest/MockBaseCryptLib/MockBaseCryptLib.cpp +++ b/CryptoPkg/Test/Mock/Library/GoogleTest/MockBaseCryptLib/MockBaseCryptLib.cpp @@ -92,14 +92,13 @@ MOCK_FUNCTION_DEFINITION (MockBaseCryptLib, RsaOaepDecrypt, 6, EFIAPI); MOCK_FUNCTION_DEFINITION (MockBaseCryptLib, Pkcs7GetSigners, 6, EFIAPI); MOCK_FUNCTION_DEFINITION (MockBaseCryptLib, Pkcs7FreeSigners, 1, EFIAPI); MOCK_FUNCTION_DEFINITION (MockBaseCryptLib, Pkcs7GetCertificatesList, 6, EFIAPI); -// MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken MOCK_FUNCTION_DEFINITION (MockBaseCryptLib, Pkcs7Sign, 10, EFIAPI); -// MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken MOCK_FUNCTION_DEFINITION (MockBaseCryptLib, Pkcs7Verify, 6, EFIAPI); MOCK_FUNCTION_DEFINITION (MockBaseCryptLib, Pkcs7Encrypt, 7, EFIAPI); MOCK_FUNCTION_DEFINITION (MockBaseCryptLib, VerifyEKUsInPkcs7Signature, 5, EFIAPI); MOCK_FUNCTION_DEFINITION (MockBaseCryptLib, Pkcs7GetAttachedContent, 4, EFIAPI); MOCK_FUNCTION_DEFINITION (MockBaseCryptLib, AuthenticodeVerify, 6, EFIAPI); +MOCK_FUNCTION_DEFINITION (MockBaseCryptLib, GetAuthenticodeHash, 5, EFIAPI); MOCK_FUNCTION_DEFINITION (MockBaseCryptLib, ImageTimestampVerify, 5, EFIAPI); MOCK_FUNCTION_DEFINITION (MockBaseCryptLib, X509GetVersion, 3, EFIAPI); MOCK_FUNCTION_DEFINITION (MockBaseCryptLib, X509GetSerialNumber, 4, EFIAPI); diff --git a/DxeImageVerificationLib_RewriteArchitecture.md b/DxeImageVerificationLib_RewriteArchitecture.md new file mode 100644 index 00000000000..d355f38febb --- /dev/null +++ b/DxeImageVerificationLib_RewriteArchitecture.md @@ -0,0 +1,1069 @@ + +# DxeImageVerificationLib Rearchitecture + +> **Companion document**: `DxeImageVerificationLib_CurrentArchitecture.md` contains the +> full function inventory, data-flow diagrams, and `SignatureType` consumption table for +> the current implementation. This document focuses on *why* and *how* to rearchitect; +> it references the baseline doc rather than duplicating it. + +--- + +## 1. Description + +`DxeImageVerificationLib` is the UEFI Secure Boot image verification policy engine in +`SecurityPkg`. It is a library instance of the `NULL` library class that self-registers +as a Security2 handler during its constructor. + +**Dispatch chain:** + +```mermaid +flowchart TD + loadImage["DXE Core LoadImage()"]:::phase + loadImage -->|calls| fileAuth + + fileAuth["EFI_SECURITY2_ARCH_PROTOCOL.FileAuthentication()"]:::protocol + fileAuth -->|dispatches via| execHandlers + + execHandlers["ExecuteSecurity2Handlers()"]:::module + execHandlers -->|invokes registered handler| handler + + handler["DxeImageVerificationHandler()"]:::security + + loadImage -.- srcCore(("DXE Core")):::phase + fileAuth -.- srcStub(("SecurityStubDxe")):::driver + execHandlers -.- srcMgmt(("DxeSecurityManagementLib")):::library + handler -.- srcLib(("this library")):::security + + classDef phase fill:#6B9E78,stroke:#4D7D5A,stroke-width:2px,color:#fff + classDef protocol fill:#E07A5F,stroke:#C45A3F,stroke-width:2px,color:#fff + classDef driver fill:#4A9B8E,stroke:#357A6F,stroke-width:2px,color:#fff + classDef library fill:#5886A5,stroke:#3D6B86,stroke-width:2px,color:#fff + classDef module fill:#E8A849,stroke:#C8882D,stroke-width:2px,color:#1a1a1a + classDef security fill:#8E6C8A,stroke:#6D4E69,stroke-width:2px,color:#fff +``` + +REF: [SecurityStubDxe::ExecuteSecurity2Handlers](https://github.com/tianocore/edk2/blob/0dddd6549d8d13d9bc5440d696fc702c3319172f/MdeModulePkg/Universal/SecurityStubDxe/SecurityStub.c#L147) + +REF: [DxeImageVerificationLib::DxeImageVerificationHandler](https://github.com/tianocore/edk2/blob/0dddd6549d8d13d9bc5440d696fc702c3319172f/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c#L1659) + +The handler receives the image buffer, device path, and boot policy flag. It decides +whether to allow (`EFI_SUCCESS`), defer (`EFI_SECURITY_VIOLATION`), or deny +(`EFI_ACCESS_DENIED`) execution based on: + +- The image source type (firmware volume, option ROM, removable media, fixed media) +- Platform policy PCDs (`PcdOptionRomImageVerificationPolicy`, etc.) +- The Secure Boot enable state (`SecureBoot` variable — **SetupMode or UserMode + only**; AuditMode and DeployedMode are removed per the revised UEFI Specification) +- Signature verification against the `db` (allow), `dbx` (deny), and `dbt` (timestamp + authority) security databases + +The handler also logs verification outcomes to the `EFI_IMAGE_EXECUTION_INFO_TABLE` +system configuration table and triggers authority measurement via `SecureBootHook()`. + +### 1.1 Relationship with Measured Boot + +Two distinct measurement paths interact with image verification: + +1. Image measurement -- Separate Security2 handlers (`DxeTpmMeasureBootLib`, + `DxeTpm2MeasureBootLib`) measure the *image itself* into TPM PCRs. These handlers + are registered alongside `DxeImageVerificationHandler` and dispatched independently + by `ExecuteSecurity2Handlers()`. They have no code dependency on this library. + +2. Authority measurement -- `SecureBootHook()` in `Measurement.c` (part of *this* + library) measures the *db/dbx entry that matched* during signature lookup. It is + called from two locations: + - `IsSignatureFoundInDatabase()` — when a hash match is found in `db` + - `IsAllowedByDb()` — when an X.509 certificate-based trust match succeeds + + This records *which trust anchor authorized* the image, not the image content. + + The authority measurement path uses `TpmMeasurementLib` to extend PCR 7 and + maintains a de-duplication cache so each unique authority entry is measured at most + once per boot. + +**Rearchitecture impact**: The coupling between `IsSignatureFoundInDatabase()` and +`SecureBootHook()` means database lookup has a measurement side-effect. The new +architecture must either preserve this call site or move authority measurement to an +explicit reporting phase (see Section 6.5). + +--- + +## 2. Responsibilities + +The current implementation mixes the following discrete responsibilities in a single +~2,500 line control path: + +| # | Responsibility | Current location | +|---|---------------|-----------------| +| R1 | **Image source classification** — Determine origin (FV, option ROM, removable, fixed media) from device path and protocol probing | `GetImageType()` | +| R2 | **Policy resolution** — Map image type to verification policy via PCDs | `DxeImageVerificationHandler` (inline) | +| R3 | **Secure Boot mode check** — Read `SecureBoot` variable, short-circuit if disabled. Current code also checks variable attributes to detect AuditMode (to be removed — see FR-8) | `DxeImageVerificationHandler` (inline) | +| R4 | **PE/COFF parsing** — Validate image structure, extract NT headers, locate certificate data directory | `DxeImageVerificationHandler` (inline) + `DxeImageVerificationLibImageRead()` | +| R5 | **Authenticode digest computation** — Hash image excluding checksum and security directory per PE/COFF Appendix A. **Duplicated** in measured boot libraries (`DxeTpmMeasureBootLib`/`DxeTpm2MeasureBootLib`). To be moved to `BaseCryptLib` — see FR-10 | `HashPeImage()` | +| R6 | **Hash algorithm detection** — Parse PKCS#7 ASN.1 to determine digest algorithm OID. To be replaced by `BaseCryptLib` `GetAuthenticodeHash()` — see FR-10 | `HashPeImageByType()` | +| R7 | **DBX revocation check (hash)** — Look up image hash in `dbx` | `IsSignatureFoundInDatabase()` called with `dbx` | +| R8 | **DBX revocation check (X.509 signer)** — Verify image Authenticode signature against X.509 certs in `dbx` | `IsForbiddenByDbx()` | +| R9 | **DB trust verification (X.509)** — Verify image Authenticode signature against X.509 trust anchors in `db` | `IsAllowedByDb()` | +| R9a | **DB trust verification (TBS cert-hash)** — Walk image certificate chain, match TBS hash against `gEfiCertX509Sha*` entries in `db` to establish trust anchor (new — see FR-9) | *Not yet implemented* | +| R10 | **DB trust verification (hash)** — Look up image hash in `db` | `IsSignatureFoundInDatabase()` called with `db` | +| R11 | **TBS cert-hash revocation + timestamp exception** — Check certificate chain hashes against `dbx` X509Sha* entries, validate signing time against `dbt` timestamp authorities. Note: TBS cert-hash is also used in `db` for trust anchor matching (FR-9) | `IsCertHashFoundInDbx()` + `PassTimestampCheck()` | +| R12 | **Execution info table logging** — Record verification outcome in `EFI_IMAGE_EXECUTION_INFO_TABLE` | `AddImageExeInfo()` + `GetImageExeInfoTableSize()` | +| R13 | **Authority measurement** — Measure matching db/dbx entry into TPM PCR 7 | `SecureBootHook()` (Measurement.c) | +| R14 | **ReadyToBoot table publication** — Ensure execution info table exists at ReadyToBoot | `OnReadyToBoot()` | + +--- + +## 3. Requirements + +Any rearchitecture of `DxeImageVerificationLib` **MUST** satisfy the following: + +### 3.1 Functional Requirements + +- **FR-1**: Produce identical allow/deny/defer decisions for all image types and + Secure Boot states as the current implementation, with the exceptions of: + AuditMode/DeployedMode removal (FR-8), multi-certificate revocation semantics + (FR-4), and TBS cert-hash trust anchor support in db (FR-9). The UEFI + Specification (Section 32.4.2 "Image Validation" and Section 32.5 "User + Identification") defines the normative behavior. + +- **FR-2**: Preserve and extend `SignatureType` consumption patterns documented in + the baseline architecture doc Section 2.1. The updated patterns are: + - `db`: image hash lookup (`gEfiCertSha*Guid`), X.509 trust anchor + (`gEfiCertX509Guid`), **TBS cert-hash trust anchor + (`gEfiCertX509Sha256Guid`/`384`/`512`)** — new, see FR-9 + - `dbx`: image hash deny-list (`gEfiCertSha*Guid`), X.509 forbidden signer + (`gEfiCertX509Guid`), TBS cert-hash revocation + (`gEfiCertX509Sha256Guid`/`384`/`512`) + - `dbt`: X.509 timestamp authority (`gEfiCertX509Guid`) + +- **FR-3**: DBX **must** override DB — a hash or signer found in both databases must + be denied. This is the security-critical ordering invariant. + +- **FR-4**: Change multi-certificate signed image revocation semantics. The + current implementation denies an image if **any** certificate in the PE/COFF + certificate data directory has a revoked signer. The rearchitected library must + change this to: an image is denied only if **all** certificates in the data + directory are revoked. If at least one certificate has a valid, non-revoked + signer that is trusted by `db`, the image is allowed. This aligns with the + intent that multi-signing provides redundancy — a single revoked signer should + not invalidate an image that has an alternative valid signer. + - Iteration order: process each `WIN_CERTIFICATE` entry in the certificate + data directory sequentially. + - For each certificate: run the full signer verification (dbx check, db trust + check, timestamp exception) as defined by `VerifySignedImage()`. + - **Short-circuit on trust**: If any certificate's signer is trusted (not + revoked, found in db), the image is **allowed** immediately — remaining + certificates need not be checked. + - **Deny only if exhausted**: If all certificates are checked and none + produced a trusted signer, the image is denied. + - The image hash check against dbx (FR-3) still takes precedence: if the + image hash itself is in dbx, the image is denied regardless of signers. + +- **FR-5**: Support PE32 and PE32+ image formats including correct Authenticode digest + computation per the Microsoft PE/COFF specification Appendix A. + +- **FR-6**: Publish `EFI_IMAGE_EXECUTION_INFO_TABLE` entries for failed/deferred + images with correct `EFI_IMAGE_EXECUTION_ACTION` values. + +- **FR-7**: Publish an empty `EFI_IMAGE_EXECUTION_INFO_TABLE` at ReadyToBoot if no + entries were recorded. + +- **FR-8**: **Drop AuditMode and DeployedMode support.** The revised UEFI + Specification defines only two Secure Boot modes: **SetupMode** and **UserMode**. + The rearchitected library must: + - Treat the `SecureBoot` variable as a simple enabled/disabled flag — no attribute + inspection to detect AuditMode (the current code checks that the variable + attributes equal `EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS` + only — i.e., the *absence* of `EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS` + — to distinguish a genuine disabled state from AuditMode). + - Remove any code path that continues verification when `SecureBoot` is disabled + (the current "but not AuditMode" exception). + - Not reference `AuditMode`, `DeployedMode`, or their associated variable + attributes anywhere in the implementation. + - Simplify `IsSecureBootEnabled()` (Layer 1) to: if `SecureBoot` variable is + absent or `SECURE_BOOT_MODE_DISABLE` → skip verification, period. + +- **FR-9**: **Support TBS certificate hash matching in `db` (allow-list).** The + current implementation only consumes `gEfiCertX509Sha256Guid`/`384`/`512` + entries from `dbx` (revocation). The rearchitected library must also support + these `SignatureType` values in `db` as trust anchors. The behavior is: + - When evaluating a signed image against `db`, extract the **full certificate + chain** from the image's Authenticode/PKCS#7 signature. + - For each `gEfiCertX509Sha*` entry in `db`, compute the TBS (To-Be-Signed) + hash of each certificate in the chain. + - If any certificate's TBS hash matches a `db` entry, that certificate becomes + the **trust anchor** for the image — the image is allowed. + - This enables enrolling a trust anchor by cert hash rather than the full X.509 + blob, reducing `db` variable size and avoiding exposure of the full certificate. + - The chain walk order should be: leaf (signer) certificate first, then + intermediate CAs, then root CA. The first match wins. + - **DBX override (FR-3) applies per-certificate across the entire chain.** Before + accepting a TBS cert-hash match in `db`, the implementation must check whether + *any* certificate in the chain matches a `dbx` cert-hash revocation entry. If + any certificate's TBS hash matches a `dbx` entry, the image is **denied** unless + the timestamp exception (`dbt`) applies to that specific certificate. This + ensures FR-3 ("dbx overrides db") holds at the image level across all chain + certificates, not just the matched trust anchor. + - If the matching certificate is also found in `dbx` (via cert-hash revocation + with timestamp), the existing timestamp exception logic (FR-2, `dbt`) applies. + +- **FR-10**: **Move Authenticode hash computation to `BaseCryptLib`.** The PE/COFF + Authenticode digest algorithm (Microsoft PE/COFF Specification, Appendix A) is + currently implemented in both `DxeImageVerificationLib` (`HashPeImage()` + + `HashPeImageByType()`) and the measured boot libraries + (`DxeTpmMeasureBootLib`/`DxeTpm2MeasureBootLib`). This is duplicated, + error-prone code. The rearchitecture must: + - Add a new `BaseCryptLib` API — `GetAuthenticodeHash()` — that accepts a PE/COFF + image buffer and either: + - (a) a PKCS#7 `AuthData` blob from which it detects the digest algorithm + internally (replacing the brittle ASN.1 offset parsing in + `HashPeImageByType()`), or + - (b) an explicit algorithm selector (for unsigned images where there is no + PKCS#7 to inspect). + - The API computes the Authenticode image digest per Appendix A (excluding + checksum field, excluding security data directory, hashing sections in order) + and returns the digest bytes, digest size, and algorithm identifier. + - Remove `HashPeImage()` and `HashPeImageByType()` from this library entirely. + Layer 2 calls `GetAuthenticodeHash()` instead. + - The same `GetAuthenticodeHash()` API becomes available to measured boot + libraries, eliminating their duplicate PE/COFF hash implementations. + + Conceptual API: + + ```c + EFI_STATUS + EFIAPI + GetAuthenticodeHash ( + IN CONST UINT8 *ImageBuffer, + IN UINTN ImageSize, + IN CONST UINT8 *AuthData OPTIONAL, // PKCS#7 blob for algorithm auto-detection + IN UINTN AuthDataSize OPTIONAL, // 0 if no AuthData + IN UINT16 HashAlgId OPTIONAL, // Explicit algorithm (used when AuthData is NULL) + OUT UINT8 *DigestBuffer, // Caller-provided, MAX_DIGEST_SIZE + OUT UINTN *DigestSize, + OUT EFI_GUID *CertType // e.g., gEfiCertSha256Guid + ); + ``` + +- **FR-11**: **Demand-driven hash computation for unsigned images.** The current + implementation computes Authenticode digests for all supported hash algorithms + (SHA512, SHA384, SHA256, SHA1) when verifying an unsigned image, regardless of + which `SignatureType` GUIDs are present in `db`/`dbx` (see Section 4.9). The + rearchitected library must: + - After loading security databases (Layer 3), scan `db` and `dbx` to determine + which hash `SignatureType` GUIDs (`gEfiCertSha256Guid`, `gEfiCertSha384Guid`, + `gEfiCertSha512Guid`) are actually present. + - For unsigned images, compute Authenticode digests **only** for algorithms that + have at least one entry in `db` or `dbx`. + - For signed images, this optimization does not apply — the digest algorithm is + determined by the PKCS#7 `AuthData` (FR-10). + - This is a performance optimization only — it must not change any allow/deny + decision. An algorithm with no entries in either database cannot produce a + match, so skipping its digest is semantically equivalent. + +### 3.2 Integration Requirements + +- **IR-1**: Continue to register via `RegisterSecurity2Handler()` with + `EFI_AUTH_OPERATION_VERIFY_IMAGE | EFI_AUTH_OPERATION_IMAGE_REQUIRED`. + +- **IR-2**: Authority measurement (`SecureBootHook` → `TpmMeasurementLib`) must + continue to record the specific db/dbx entry that authorized or denied an image. + The measurement may be relocated within the new architecture but must not be + dropped. + +- **IR-3**: Policy PCDs (`PcdOptionRomImageVerificationPolicy`, + `PcdRemovableMediaImageVerificationPolicy`, `PcdFixedMediaImageVerificationPolicy`) + must retain their current semantics and default values. + +- **IR-4**: The `.inf` `MODULE_TYPE` must remain `DXE_DRIVER`. The `LIBRARY_CLASS` + consumer list (`NULL|DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION + UEFI_DRIVER`) must be preserved to avoid breaking downstream platform consumers. + External interface (`RegisterSecurity2Handler` callback signature) must not change. + +### 3.3 Quality Requirements + +- **QR-1**: Individual verification decisions (hash lookup, cert verification, + timestamp check) must be unit-testable in a host-based environment without UEFI + runtime services. + +- **QR-2**: No module-scope mutable state shared between verification calls. All + per-image state must be scoped to a verification context structure. + **Exception**: The authority measurement de-duplication cache in `Measurement.c` + (`mMeasuredAuthorityCount`, `mMeasuredAuthorityCountMax`, `mMeasuredAuthorityList`) + is exempt from this requirement. This cache must persist across verification calls + by design — measuring the same authority entry twice would violate TCG measurement + semantics. The cache is managed by the reporting layer (Layer 5) and does not + participate in verification decisions. + +- **QR-3**: Each security database (db, dbx, dbt) should be read at most once per + verification call. Database reads must be centralized, not scattered across + helper functions. + +--- + +## 4. Problems with Current Architecture + +### 4.1 Global Mutable State + +The current implementation uses 8 module-scope mutable variables to carry per-image +state between functions: + +```c +EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION mNtHeader; +UINT32 mPeCoffHeaderOffset; +EFI_GUID mCertType; +UINTN mImageSize; +UINT8 *mImageBase; +UINT8 mImageDigest[MAX_DIGEST_SIZE]; +UINTN mImageDigestSize; +EFI_STRING mHashTypeStr; +``` + +Additionally, `Measurement.c` maintains 3 cross-call mutable globals for the authority +measurement de-duplication cache (`mMeasuredAuthorityCount`, +`mMeasuredAuthorityCountMax`, `mMeasuredAuthorityList`). These persist across +verification calls by design — see QR-2 for the exemption rationale. + +These globals are written by `HashPeImage()` and read by `IsForbiddenByDbx()`, +`IsAllowedByDb()`, and `IsSignatureFoundInDatabase()`. This creates: + +- **Implicit coupling**: Functions depend on call ordering to find valid state. + `IsForbiddenByDbx()` silently produces wrong results if `HashPeImage()` was not + called first, with no compile-time or runtime enforcement. +- **No reentrancy**: If a nested image load triggered verification during an existing + verification call, globals would be corrupted. +- **No testability**: Test harnesses cannot inject specific digest/cert combinations + without reproducing the exact global mutation sequence. + +### 4.2 Monolithic Handler + +`DxeImageVerificationHandler()` is approximately 500 lines and contains: + +- Policy lookup (PCD reads) +- Secure Boot variable reads +- PE/COFF header parsing and validation +- Certificate table iteration +- Calls to hash, forbid, allow, and lookup functions +- Execution info table construction +- Error path cleanup and result mapping + +The function has multiple `goto` targets and deeply nested conditionals that make the +verification decision tree difficult to follow, review, or modify. + +### 4.3 Duplicated Database Logic + +The timestamp revocation path (`IsCertHashFoundInDbx()` + `PassTimestampCheck()`) is +invoked from *both*: + +- `IsForbiddenByDbx()` — to check if a forbidden signer has a timestamp exception +- `IsAllowedByDb()` — to check if an allowed signer's cert has been revoked with a + timestamp grace period + +Each path independently reads `dbx` and `dbt` via `gRT->GetVariable()`, duplicating +allocation and parsing work. This also means `dbx` may be read up to **three times** +in a single verification call (once in `IsForbiddenByDbx`, once in `IsAllowedByDb`, +once in `IsSignatureFoundInDatabase`). + +### 4.4 Mixed Concerns + +The handler mixes five conceptually distinct concerns in one control path: + +1. **Policy** — Should this image type be verified at all? +2. **Parsing** — Is this a valid PE/COFF image? What are its signatures? +3. **Database access** — What do db/dbx/dbt contain right now? +4. **Verification logic** — Given the image digest and databases, is it allowed? +5. **Reporting** — Log the outcome, measure the authority. + +These concerns cannot be independently tested, replaced, or extended. + +### 4.5 Untestable + +There are **no unit tests** for this library. The current architecture makes host-based +testing impractical because: + +- Hash computation requires global state setup (`mImageBase`, `mNtHeader`, etc.) +- Database checks call `gRT->GetVariable()` directly — no indirection layer to mock +- Signature verification calls `AuthenticodeVerify()` from `BaseCryptLib` with no + abstraction boundary +- `AddImageExeInfo()` calls `gBS->InstallConfigurationTable()` — requires boot + services + +### 4.6 Fragile Algorithm Dispatch + +`HashPeImageByType()` determines the digest algorithm by parsing raw ASN.1 at a fixed +offset (+32 bytes into the PKCS#7 `ContentInfo` structure). This is: + +- **Brittle**: Assumes a specific PKCS#7 encoding layout that may not hold for all + valid encodings +- **Not extensible**: Adding a new hash algorithm requires understanding the ASN.1 + byte pattern and adding a new OID match +- **Duplicative**: `BaseCryptLib` already has ASN.1 parsing capability that could be + leveraged instead + +### 4.7 Duplicated Authenticode Hash Logic + +The PE/COFF Authenticode digest computation (hash image contents excluding checksum +field and security data directory, per Microsoft PE/COFF Appendix A) is implemented +in **two separate libraries**: + +1. `DxeImageVerificationLib` — `HashPeImage()` for signature verification +2. `DxeTpmMeasureBootLib` / `DxeTpm2MeasureBootLib` — for TPM image measurement + +Both implementations must handle PE32 vs PE32+ format differences, section ordering, +and the exact exclusion regions. Any bug fix or algorithm addition must be applied to +both codebases. This should be a single shared primitive in `BaseCryptLib` (see +FR-10). + +### 4.8 Scattered Database Reads + +Security database variables are read independently by multiple functions: + +| Function | Variables read | +|----------|---------------| +| `DxeImageVerificationHandler` | `SecureBoot` | +| `IsSignatureFoundInDatabase` | `db` or `dbx` (parameterized) | +| `IsForbiddenByDbx` | `dbx` | +| `IsAllowedByDb` | `db`, `dbx` | +| `PassTimestampCheck` | `dbt` | + +A single verification call may invoke `gRT->GetVariable()` 5–7 times, repeatedly +allocating and freeing buffers for the same variable content. + +### 4.9 Redundant Hash Computation for Unsigned Images + +When verifying an unsigned image, the current code iterates **all supported hash +algorithms** (SHA512, SHA384, SHA256, SHA1) and computes a full PE/COFF Authenticode +digest for each — regardless of whether `db` or `dbx` actually contain entries of +that `SignatureType`. For example, if `db` contains only `gEfiCertSha256Guid` entries +and `dbx` contains only `gEfiCertSha256Guid` entries, the code still computes SHA512, +SHA384, and SHA1 digests that can never match anything. Each unnecessary hash requires +a full pass over the image contents. + +With the new architecture's "read once, query many" principle (databases loaded before +verification), the loaded `db`/`dbx` buffers can be scanned to discover which hash +`SignatureType` GUIDs are actually present, and only those algorithms need to be +computed. + +--- + +## 5. Proposed Architecture + +### 5.1 Design Principles + +1. **Explicit state** — All per-verification state lives in a context structure passed + by pointer. No module-scope mutable variables. +2. **Read once, query many** — Security databases are loaded once at the start of + verification and exposed through a query interface. +3. **Layered separation** — Each concern (policy, parsing, database, verification, + reporting) is a distinct group of functions with defined inputs and outputs. +4. **Testable boundaries** — Database reads and crypto operations are accessed through + function pointers or thin wrappers that can be substituted in test harnesses. + +### 5.2 Verification Context + +Replace all module-scope globals with a per-call context structure: + +```c +typedef struct { + // + // Image identity + // + UINT8 *ImageBase; + UINTN ImageSize; + EFI_DEVICE_PATH_PROTOCOL *DevicePath; + + // + // PE/COFF parsed state + // + UINT32 PeCoffHeaderOffset; + EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION NtHeader; + BOOLEAN IsPe32Plus; + + // + // Computed digest (set by image hashing layer) + // + UINT8 ImageDigest[MAX_DIGEST_SIZE]; + UINTN ImageDigestSize; + EFI_GUID CertType; + + // + // Loaded security databases (set by database layer) + // + UINT8 *DbData; + UINTN DbSize; + UINT8 *DbxData; + UINTN DbxSize; + UINT8 *DbtData; + UINTN DbtSize; + + // + // Verification result (set by verification engine) + // + BOOLEAN IsVerified; + EFI_IMAGE_EXECUTION_ACTION Action; + EFI_SIGNATURE_LIST *MatchedSignature; + // NOTE: MatchedSignature is a borrowed pointer into DbData or DbxData. + // It is valid only until FreeSecurityDatabases() is called. The handler + // flow guarantees that all consumers (MeasureMatchedAuthority, + // RecordImageExecutionResult) execute before FreeSecurityDatabases(). +} IMAGE_VERIFICATION_CONTEXT; +``` + +This structure is stack-allocated by the handler entry point and passed by pointer to +all subordinate functions. It is freed (database buffers) before the handler returns. + +### 5.3 Layered Responsibilities + +```mermaid +block-beta + columns 5 + + block:orchestrator:5 + handler["DxeImageVerificationHandler (orchestrator only)"] + end + + block:l1:1 + columns 1 + l1h["Layer 1
Policy Resolution"] + l1a["GetImageType()"] + l1b["ResolveVerificationPolicy()"] + l1c["IsSecureBootEnabled()"] + space + space + space + space + end + + block:l2:1 + columns 1 + l2h["Layer 2
Image Parsing"] + l2a["ParsePeCoffImage()"] + l2b["ComputeImageDigest()"] + l2c["GetNextCertificate()"] + space + space + space + space + end + + block:l3:1 + columns 1 + l3h["Layer 3
Database Operations"] + l3a["LoadSecurityDatabases()"] + l3b["FindHashInDatabase()"] + l3c["FindX509InDatabase()"] + l3d["FindCertHashInDatabase()"] + l3e["GetHashAlgorithmsInDatabase()"] + l3f["ExtractCertificateChain()"] + l3g["FreeSecurityDatabases()"] + end + + block:l4:1 + columns 1 + l4h["Layer 4
Verification Engine"] + l4a["VerifyUnsignedImage()"] + l4b["VerifySignedImage()"] + l4c["IsSignerForbidden()"] + l4d["IsSignerTrusted()"] + l4e["ValidateTimestamp()"] + space + space + end + + block:l5:1 + columns 1 + l5h["Layer 5
Reporting"] + l5a["RecordImageExecutionResult()"] + l5b["MeasureMatchedAuthority()"] + space + space + space + space + space + end + + style handler fill:#4A9B8E,stroke:#357A6F,stroke-width:2px,color:#fff + style orchestrator fill:transparent,stroke:#357A6F,stroke-width:2px + + style l1h fill:#E8A849,stroke:#C8882D,stroke-width:2px,color:#1a1a1a + style l1a fill:#FFF3E0,stroke:#E8A849,color:#1a1a1a + style l1b fill:#FFF3E0,stroke:#E8A849,color:#1a1a1a + style l1c fill:#FFF3E0,stroke:#E8A849,color:#1a1a1a + style l1 fill:transparent,stroke:#C8882D,stroke-width:2px + + style l2h fill:#E8A849,stroke:#C8882D,stroke-width:2px,color:#1a1a1a + style l2a fill:#FFF3E0,stroke:#E8A849,color:#1a1a1a + style l2b fill:#FFF3E0,stroke:#E8A849,color:#1a1a1a + style l2c fill:#FFF3E0,stroke:#E8A849,color:#1a1a1a + style l2 fill:transparent,stroke:#C8882D,stroke-width:2px + + style l3h fill:#E8A849,stroke:#C8882D,stroke-width:2px,color:#1a1a1a + style l3a fill:#FFF3E0,stroke:#E8A849,color:#1a1a1a + style l3b fill:#FFF3E0,stroke:#E8A849,color:#1a1a1a + style l3c fill:#FFF3E0,stroke:#E8A849,color:#1a1a1a + style l3d fill:#FFF3E0,stroke:#E8A849,color:#1a1a1a + style l3e fill:#FFF3E0,stroke:#E8A849,color:#1a1a1a + style l3f fill:#FFF3E0,stroke:#E8A849,color:#1a1a1a + style l3g fill:#FFF3E0,stroke:#E8A849,color:#1a1a1a + style l3 fill:transparent,stroke:#C8882D,stroke-width:2px + + style l4h fill:#8E6C8A,stroke:#6D4E69,stroke-width:2px,color:#fff + style l4a fill:#F3E5F5,stroke:#8E6C8A,color:#1a1a1a + style l4b fill:#F3E5F5,stroke:#8E6C8A,color:#1a1a1a + style l4c fill:#F3E5F5,stroke:#8E6C8A,color:#1a1a1a + style l4d fill:#F3E5F5,stroke:#8E6C8A,color:#1a1a1a + style l4e fill:#F3E5F5,stroke:#8E6C8A,color:#1a1a1a + style l4 fill:transparent,stroke:#6D4E69,stroke-width:2px + + style l5h fill:#8E6C8A,stroke:#6D4E69,stroke-width:2px,color:#fff + style l5a fill:#F3E5F5,stroke:#8E6C8A,color:#1a1a1a + style l5b fill:#F3E5F5,stroke:#8E6C8A,color:#1a1a1a + style l5 fill:transparent,stroke:#6D4E69,stroke-width:2px +``` + +#### Layer 1: Policy Resolution + +- **Input**: Device path, boot policy flag +- **Output**: `IMAGE_VERIFICATION_POLICY` (always execute, never execute, verify + + action-on-failure) +- **Functions**: + - `GetImageType()` — Classify image source (unchanged logic) + - `ResolveVerificationPolicy()` — Map image type to policy via PCDs + - `IsSecureBootEnabled()` — Read `SecureBoot` variable; return TRUE only if + present and value is `SECURE_BOOT_MODE_ENABLE`. No AuditMode/DeployedMode + attribute inspection (FR-8). + +#### Layer 2: Image Parsing + +- **Input**: Image buffer, image size, context pointer +- **Output**: Populated PE/COFF fields in context, certificate iterator +- **Functions**: + - `ParsePeCoffImage()` — Validate PE/COFF structure, populate context header fields, + locate certificate data directory + - `ComputeImageDigest()` — Thin wrapper that calls `BaseCryptLib` + `GetAuthenticodeHash()` (FR-10). For signed images, passes the PKCS#7 `AuthData` + for algorithm auto-detection. For unsigned images, called once per algorithm + that is actually present in `db`/`dbx` (determined by + `GetHashAlgorithmsInDatabase()` — see FR-11). Stores result in + `Ctx->ImageDigest`, `Ctx->ImageDigestSize`, `Ctx->CertType`. + - `GetNextCertificate()` — Iterator over certificate data directory entries + +#### Layer 3: Database Operations + +- **Input**: Context pointer (for buffer storage) +- **Output**: Populated db/dbx/dbt buffers in context +- **Functions**: + - `LoadSecurityDatabases()` — Read db, dbx, dbt once into context buffers + - `FindHashInDatabase()` — Search a loaded database for a hash match + - `FindX509InDatabase()` — Iterate X.509 certs in a loaded database + - `FindCertHashInDatabase()` — Search for TBS cert-hash entries + (`gEfiCertX509Sha256Guid`/`384`/`512`) in a loaded database. Generalized + from the current `IsCertHashFoundInDbx()` to work against **any** database + (db or dbx). When searching `dbx`, returns the revocation time. When + searching `db`, returns a match indicating trust anchor (FR-9). + - `GetHashAlgorithmsInDatabase()` — Scan loaded `db` and `dbx` buffers and + return a bitmask (or list) of hash `SignatureType` GUIDs that are present + (e.g., `gEfiCertSha256Guid`, `gEfiCertSha384Guid`, `gEfiCertSha512Guid`). + Used by `VerifyUnsignedImage()` to determine which digests to compute + (FR-11). + - `FreeSecurityDatabases()` — Release context database buffers + +#### Layer 4: Verification Engine + +- **Input**: Context with populated digest and database buffers, certificate data +- **Output**: `IsVerified`, `Action`, `MatchedSignature` in context +- **Functions**: + - `VerifyUnsignedImage()` — Check unsigned image hash against dbx then db. + Calls `GetHashAlgorithmsInDatabase()` to determine which hash algorithms + have entries in `db`/`dbx`, then iterates only those algorithms: for each, + calls `ComputeImageDigest()` and checks against dbx then db (FR-11). + A dbx hash match **MUST** short-circuit immediately without checking db + (FR-3 security invariant: dbx overrides db). + - `VerifySignedImage()` — Iterate certificates in the PE/COFF certificate + data directory. For each certificate, check the signer against dbx then db. + **Short-circuit on trust**: if any certificate's signer is trusted and not + revoked, the image is allowed immediately (FR-4). Deny only if all + certificates are exhausted with no trusted signer found. + - `IsSignerForbidden()` — Check signer cert against dbx X.509 entries + (replaces `IsForbiddenByDbx`, operates on loaded data) + - `IsSignerTrusted()` — Check signer against db trust anchors via + `AuthenticodeVerify2()`. This function handles both X.509 and TBS cert-hash + matching internally: + 1. **X.509 match**: Verify signature against raw X.509 certs in db + 2. **TBS cert-hash match** (FR-9): Extract certificate chain from PKCS#7, + compute TBS hash of each cert, match against `gEfiCertX509Sha*` entries + in db. First chain cert whose TBS hash matches establishes trust. + Returns the matched certificate (if any). If the matched certificate is also + found in dbx via cert-hash revocation, apply timestamp exception logic via + `ValidateTimestamp()`. + - `ValidateTimestamp()` — Timestamp verification against dbt + (replaces `PassTimestampCheck`, operates on loaded data) + +#### Layer 5: Reporting + +- **Input**: Context with verification result +- **Output**: Side effects (execution info table update, authority measurement) +- **Functions**: + - `RecordImageExecutionResult()` — Build and append `EFI_IMAGE_EXECUTION_INFO` + entry (replaces `AddImageExeInfo`) + - `MeasureMatchedAuthority()` — Call `SecureBootHook()` with the matched db/dbx + entry. Moved here from `IsSignatureFoundInDatabase()` so measurement is a + post-verification step rather than a side-effect of lookup. + +### 5.4 Proposed Handler Flow + +```mermaid +flowchart TD + entry["DxeImageVerificationHandler
(File, FileBuffer, FileSize, BootPolicy)"]:::driver + + resolve["[Layer 1] ResolveVerificationPolicy
(File, BootPolicy)"]:::module + entry --> resolve + + policyCheck{{"ALWAYS_EXECUTE
or NEVER_EXECUTE?"}}:::module + resolve --> policyCheck + policyCheck -->|Yes| earlyReturn1["return immediately
(allow or deny per policy)"]:::driver + + sbCheck{{"IsSecureBootEnabled()?"}}:::module + policyCheck -->|No — needs verification| sbCheck + sbCheck -->|No| earlyReturn2["return EFI_SUCCESS
(skip verification)"]:::driver + + initCtx["Initialize IMAGE_VERIFICATION_CONTEXT
on stack"]:::driver + sbCheck -->|Yes| initCtx + + parse["[Layer 2] ParsePeCoffImage(&Ctx)"]:::module + initCtx --> parse + + parseCheck{{"Parse succeeded?"}}:::module + parse --> parseCheck + parseCheck -->|No| earlyReturn3["return EFI_ACCESS_DENIED"]:::driver + + loadDb["[Layer 3] LoadSecurityDatabases(&Ctx)
(db, dbx, dbt — read once)"]:::module + parseCheck -->|Yes| loadDb + + signedCheck{{"Image signed?"}}:::security + loadDb --> signedCheck + + unsigned["[Layer 4] VerifyUnsignedImage(&Ctx)
hash against dbx → db"]:::security + signed["[Layer 4] VerifySignedImage(&Ctx)
iterate certs, check signers"]:::security + signedCheck -->|Unsigned| unsigned + signedCheck -->|Signed| signed + + verifiedCheck{{"Ctx.IsVerified?"}}:::security + unsigned --> verifiedCheck + signed --> verifiedCheck + + record["[Layer 5] RecordImageExecutionResult(&Ctx)
append to EFI_IMAGE_EXECUTION_INFO_TABLE"]:::security + measure["[Layer 5] MeasureMatchedAuthority(&Ctx)
PCR 7 via SecureBootHook"]:::security + verifiedCheck -->|"No (denied/deferred)"| record + verifiedCheck -->|"Yes (allowed)"| measure + + freeDb["[Layer 3] FreeSecurityDatabases(&Ctx)"]:::module + record --> freeDb + measure --> freeDb + + result["return MapResultToStatus
(Ctx.IsVerified, policy)"]:::driver + freeDb --> result + + classDef driver fill:#4A9B8E,stroke:#357A6F,stroke-width:2px,color:#fff + classDef module fill:#E8A849,stroke:#C8882D,stroke-width:2px,color:#1a1a1a + classDef security fill:#8E6C8A,stroke:#6D4E69,stroke-width:2px,color:#fff +``` + +### 5.5 Proposed Interaction Diagram + +```mermaid +--- +config: + layout: elk +--- +classDiagram + +namespace Orchestrator { + class DxeImageVerificationHandler:::driver { + <> + +DxeImageVerificationHandler(File, Buffer, Size, BootPolicy) EFI_STATUS + } +} + +namespace Layer1_Policy { + class PolicyResolution:::module { + <> + +GetImageType(DevicePath) IMAGE_TYPE + +ResolveVerificationPolicy(ImageType) POLICY + +IsSecureBootEnabled() BOOLEAN + } +} + +namespace Layer2_Parsing { + class ImageParsing:::module { + <> + +ParsePeCoffImage(Ctx) EFI_STATUS + +ComputeImageDigest(Ctx, AuthData) EFI_STATUS + +GetNextCertificate(Ctx, Iterator) CERT_DATA + } +} + +namespace Layer3_Database { + class DatabaseOps:::module { + <> + +LoadSecurityDatabases(Ctx) EFI_STATUS + +FindHashInDatabase(DbData, Hash, CertType) BOOLEAN + +FindX509InDatabase(DbData) ITERATOR + +FindCertHashInDatabase(Cert, DbData, RevTime) BOOLEAN + +GetHashAlgorithmsInDatabase(Ctx) BITMASK + +FreeSecurityDatabases(Ctx) VOID + } +} + +namespace Layer4_Verification { + class VerificationEngine:::security { + <> + +VerifyUnsignedImage(Ctx) VOID + +VerifySignedImage(Ctx) VOID + -IsSignerForbidden(Ctx, CertData) BOOLEAN + -IsSignerTrusted(Ctx, CertData) BOOLEAN + -ValidateTimestamp(Ctx, CertData, RevTime) BOOLEAN + } +} + +namespace Layer5_Reporting { + class Reporting:::security { + <> + +RecordImageExecutionResult(Ctx) VOID + +MeasureMatchedAuthority(Ctx) VOID + } +} + +namespace ExternalDeps { + class BaseCryptLib:::library { + <> + +GetAuthenticodeHash(..) EFI_STATUS + +AuthenticodeVerify2(ImageBuffer, TbsHashList[]) (MatchedCertIndex, EFI_STATUS) + +ImageTimestampVerify(..) BOOLEAN + +Pkcs7GetSigners(..) BOOLEAN + } + class PeCoffLib:::library { + <> + } + class TpmMeasurementLib:::library { + <> + } + class RuntimeServices:::protocol { + <> + +gRT→GetVariable() + } + class BootServices:::protocol { + <> + +gBS→InstallConfigurationTable() + } +} + +DxeImageVerificationHandler --> PolicyResolution : 1. resolve policy +DxeImageVerificationHandler --> ImageParsing : 2. parse + hash +DxeImageVerificationHandler --> DatabaseOps : 3. load databases +DxeImageVerificationHandler --> VerificationEngine : 4. verify +DxeImageVerificationHandler --> Reporting : 5. report + +ImageParsing --> PeCoffLib : validate PE structure +ImageParsing --> BaseCryptLib : GetAuthenticodeHash (FR‑10) +DatabaseOps --> RuntimeServices : gRT→GetVariable (db/dbx/dbt) +VerificationEngine --> BaseCryptLib : AuthenticodeVerify2, ImageTimestampVerify +VerificationEngine --> DatabaseOps : query loaded databases +Reporting --> BootServices : InstallConfigurationTable +Reporting --> TpmMeasurementLib : PCR 7 authority measurement + +classDef driver fill:#4A9B8E,stroke:#357A6F,stroke-width:2px,color:#fff +classDef protocol fill:#E07A5F,stroke:#C45A3F,stroke-width:2px,color:#fff +classDef library fill:#5886A5,stroke:#3D6B86,stroke-width:2px,color:#fff +classDef library_instance fill:#89BDD3,stroke:#5886A5,stroke-width:2px,color:#1a1a1a +classDef module fill:#E8A849,stroke:#C8882D,stroke-width:2px,color:#1a1a1a +classDef security fill:#8E6C8A,stroke:#6D4E69,stroke-width:2px,color:#fff +classDef phase fill:#6B9E78,stroke:#4D7D5A,stroke-width:2px,color:#fff +``` + +--- + +## 6. Testing Strategy + +### 6.1 Testability by Layer + +| Layer | Host-testable? | Mocking required | Test focus | +|-------|---------------|-----------------|-----------| +| L1 Policy | Yes | PCD values, `SecureBoot` variable | Correct policy mapping for all image types and PCD combinations; SetupMode/UserMode only (no AuditMode/DeployedMode) | +| L2 Parsing | Yes | None (pure computation) | PE32/PE32+ digest correctness against known-good Authenticode hashes | +| L3 Database | Partially | `gRT->GetVariable` | Correct parsing of `EFI_SIGNATURE_LIST` structures, hash/cert matching | +| L4 Verification | Yes | Database query functions, `BaseCryptLib` | Decision matrix: unsigned/signed × db/dbx/dbt combinations × timestamp scenarios × TBS cert-hash in db (FR-9) | +| L5 Reporting | Partially | `gBS->InstallConfigurationTable`, `TpmMeasurementLib` | Correct `EFI_IMAGE_EXECUTION_INFO` construction | + +### 6.2 Test Approach + +- **Unit tests** using `UnitTestFrameworkPkg` `HOST_APPLICATION` test modules. +- **Database query layer**: Provide pre-built `EFI_SIGNATURE_LIST` byte arrays as test + fixtures. Functions operate on loaded buffers, so `gRT` is not needed at test time. +- **Verification engine**: Inject `AuthenticodeVerify2()` return values and mock + database query results to exercise all decision branches without real crypto. + Must include: + - TBS cert-hash trust anchor matching in db (FR-9) — verify that + `AuthenticodeVerify2()` correctly matches TBS hashes against db entries and + returns the matched certificate index; verify interaction with dbx cert-hash + revocation + timestamp exception. + - Multi-certificate signed images (FR-4) — test that a single trusted signer + allows the image even when other signers are revoked; test that the image is + denied only when all signers are revoked; test that image hash in dbx still + overrides regardless of signers. +- **Integration tests**: Full handler flow with mock `gRT`/`gBS` to validate + end-to-end behavior matches baseline flowchart (Section 4 of baseline doc), + extended with FR-9 scenarios. + +### 6.3 Regression Oracle + +The baseline architecture doc's verification flow diagram (Section 4) serves as the +regression specification. Each decision branch in that flowchart should map to at least +one test case covering the allow and deny paths. + +--- + +## 7. Migration Strategy + +### Phase 1: Introduce Context Structure + +- Define `IMAGE_VERIFICATION_CONTEXT` structure +- Modify `DxeImageVerificationHandler` to allocate context on stack and populate + `ImageBase`, `ImageSize`, `DevicePath` +- Thread context pointer through hash and verification functions — write digest into + context instead of globals +- Remove corresponding globals (`mImageBase`, `mImageSize`, `mImageDigest`, etc.) +- **Checkpoint**: Functional equivalence — behavior is identical, state is now explicit + +### Phase 1a: Implement `GetAuthenticodeHash()` in `BaseCryptLib` + +- Implement `GetAuthenticodeHash()` API in `BaseCryptLib` (FR-10) +- Migrate PE/COFF Authenticode digest logic from `HashPeImage()` + + `HashPeImageByType()` into the new API +- Replace `HashPeImage()` / `HashPeImageByType()` calls with `ComputeImageDigest()` + → `GetAuthenticodeHash()` +- Delete `HashPeImage()` and `HashPeImageByType()` from this library +- Optionally: migrate `DxeTpmMeasureBootLib` / `DxeTpm2MeasureBootLib` to use the + same API (can be done independently) +- **Checkpoint**: Authenticode hash is computed by `BaseCryptLib`; no PE/COFF hash + logic remains in this library or is duplicated across libraries + +### Phase 2: Centralize Database Reads + +- Implement `LoadSecurityDatabases()` / `FreeSecurityDatabases()` +- Modify `IsForbiddenByDbx()`, `IsAllowedByDb()`, `IsSignatureFoundInDatabase()` to + accept pre-loaded database buffers from context instead of calling + `gRT->GetVariable()` internally +- Remove per-function `GetVariable` calls for db/dbx/dbt +- **Checkpoint**: Each database read once per verification call + +### Phase 3: Extract Verification Engine and Enhance BaseCryptLib + +- Implement `AuthenticodeVerify2(ImageBuffer, TbsHashList[])` in BaseCryptLib. + This new API internalizes the certificate chain extraction and TBS hash + computation for FR-9 TBS cert-hash matching. It returns the index of the + matched certificate (or -1 if no match). This consolidates X.509 and + TBS cert-hash verification into a single BaseCryptLib call. +- Refactor `IsForbiddenByDbx()` → `IsSignerForbidden()` operating on context +- Refactor `IsAllowedByDb()` → `IsSignerTrusted()` operating on context, + calling `AuthenticodeVerify2()` to check both X.509 and TBS cert-hash + trust anchors +- Consolidate `IsCertHashFoundInDbx()` + `PassTimestampCheck()` into unified + `ValidateTimestamp()` called from one location +- Implement `VerifyUnsignedImage()` and `VerifySignedImage()` as top-level + verification entry points +- Implement FR-4 multi-certificate semantics in `VerifySignedImage()`: iterate + certificates, short-circuit on first trusted signer, deny only when all signers + are revoked +- **Checkpoint**: Verification engine is a pure function of context — testable. + BaseCryptLib owns certificate chain extraction and TBS computation for FR-9. + **Note**: Multi-certificate images with mixed revoked/trusted signers will now + produce different results than the current implementation (FR-4 behavior change). + +### Phase 4: Extract Reporting + +- Move `SecureBootHook()` call from `IsSignatureFoundInDatabase()` to + `MeasureMatchedAuthority()` called after verification completes +- Refactor `AddImageExeInfo()` → `RecordImageExecutionResult()` operating on context +- **Checkpoint**: Reporting is post-verification, no side effects during lookup + +### Phase 5: Add Tests + +- Write unit tests for Layer 4 (verification engine) first — highest value +- Add Layer 2 (image parsing / digest) tests with known-good PE images +- Add Layer 3 (database query) tests with constructed `EFI_SIGNATURE_LIST` fixtures +- Add integration test exercising full handler flow +- **Checkpoint**: Test coverage for all decision branches in baseline flowchart + +### Migration Invariant + +At the end of **every phase**, the library must produce identical results for all +inputs, **except** for the intentional behavior changes documented in FR-1: + +- FR-4: Multi-certificate images with mixed revoked/trusted signers are now + allowed (previously denied). Introduced in Phase 3. +- FR-8: AuditMode/DeployedMode code paths removed. Introduced in Phase 1. +- FR-9: TBS cert-hash entries in `db` now establish trust anchors. Introduced + in Phase 3. + +The baseline architecture doc's flowchart (Section 4) and `SignatureType` +table (Section 2.1) define the expected behavior for unchanged paths. + +--- + +## 8. Open Questions + +These items require discussion and decisions before implementation begins: + +1. **SecureBootHook relocation** — Moving authority measurement from the database + lookup layer to the reporting layer changes *when* measurement occurs relative to + the verification decision. Is there a spec or platform requirement that measurement + must happen during lookup (before the allow/deny decision is finalized)? + +2. **Database caching across calls** — Should loaded db/dbx/dbt be cached across + multiple `DxeImageVerificationHandler` invocations (e.g., for the duration of a + boot phase), or must they be re-read every call to reflect potential runtime + updates? Current behavior re-reads every call. + +3. **Separate library class for verification engine** — Should Layer 4 be a distinct + `ImageVerificationEngineLib` library class (separate `.inf`, mockable in platform + DSC), or should all layers remain internal to a single library instance? A separate + class improves testability but adds build complexity. + +4. **`GetAuthenticodeHash()` API design** — **Resolved: move to `BaseCryptLib`** + (FR-10). The new API handles both PKCS#7 algorithm auto-detection (replacing + `HashPeImageByType`) and PE/COFF Appendix A digest computation (replacing + `HashPeImage`). Open sub-questions: + - Should the API accept a raw image buffer and parse PE/COFF headers internally, + or should it accept pre-parsed section table data? Internal parsing is simpler + for callers but couples `BaseCryptLib` to PE/COFF format knowledge. + - Should the unsigned-image path (no PKCS#7 blob) try all algorithms and return + multiple digests, or should the caller iterate and call once per algorithm? + - Should `DxeTpmMeasureBootLib` migration be done in the same change series or + as a follow-up? + +5. **Certificate chain extraction for FR-9** — `BaseCryptLib` currently has + `Pkcs7GetSigners()` which returns signer certs. Does it also expose the full + chain (intermediates + root)? If not, a new `BaseCryptLib` API may be needed + (e.g., `Pkcs7GetCertificateChain()`) to walk the PKCS#7 certificate set in + order. Alternatively, could the chain extraction be done using existing + `X509GetTBSCert()` iteratively on raw ASN.1 parsing of the PKCS#7 certificates + field? + +6. **Multi-file vs. single-file** — Should the layers be implemented in separate `.c` + files within the library directory (e.g., `PolicyResolution.c`, `ImageParsing.c`, + `DatabaseOps.c`, `VerificationEngine.c`, `Reporting.c`), or remain in a single + file with clear section markers? + +7. **EFI_IMAGE_EXECUTION_INFO_TABLE lifetime** — The current `AddImageExeInfo()` + allocates a new runtime pool buffer, copies the existing table, installs the new + pointer via `InstallConfigurationTable()`, and frees the old buffer. There is no + memory leak — the previous allocation is properly freed on each reallocation. The + final table persists as a system configuration table until platform reset, which + is by-design behavior for configuration tables. No changes needed. + +8. **Scope of rearchitecture** — Beyond the AuditMode/DeployedMode removal (FR-8) + and TBS cert-hash in db (FR-9), + is this otherwise a refactor-only effort (same `.inf`, same external interface, + same behavior), or is there appetite to also fix other known bugs and spec + deviations discovered during the analysis? diff --git a/MdePkg/Include/Library/PeCoffLib.h b/MdePkg/Include/Library/PeCoffLib.h index 7be587f2fca..385a49f2f11 100644 --- a/MdePkg/Include/Library/PeCoffLib.h +++ b/MdePkg/Include/Library/PeCoffLib.h @@ -69,6 +69,30 @@ RETURN_STATUS OUT VOID *Buffer ); +/** + Optional abstraction to read each entry of the DataDirectory[] array in the PE/COFF optional header. + + Returning a non-RETURN_SUCCESS status from the callback aborts + + @param Index EFI_IMAGE_DIRECTORY_ENTRY_* slot of the + entry being delivered. + @param DataDirectory Pointer to the DataDirectory entry. Lives + inside the caller's image buffer; do not + retain past callback return. + @param Context Caller-owned opaque pointer. + + @retval RETURN_SUCCESS Continue iterating remaining entries. + @retval Other Abort iteration; status is propagated to + the caller of PeCoffLoaderGetImageInfo(). +**/ +typedef +RETURN_STATUS +(EFIAPI *PE_COFF_LOADER_READ_DATA_DIRECTORY)( + IN UINT32 Index, + IN CONST EFI_IMAGE_DATA_DIRECTORY *DataDirectory, + IN VOID *Context OPTIONAL + ); + /// /// The context structure used while PE/COFF image is being loaded and relocated. /// @@ -76,134 +100,143 @@ typedef struct { /// /// Set by PeCoffLoaderGetImageInfo() to the ImageBase in the PE/COFF header. /// - PHYSICAL_ADDRESS ImageAddress; + PHYSICAL_ADDRESS ImageAddress; /// /// Set by PeCoffLoaderGetImageInfo() to the SizeOfImage in the PE/COFF header. /// Image size includes the size of Debug Entry if it is present. /// - UINT64 ImageSize; + UINT64 ImageSize; /// /// Is set to zero by PeCoffLoaderGetImageInfo(). If DestinationAddress is non-zero, /// PeCoffLoaderRelocateImage() will relocate the image using this base address. /// If the DestinationAddress is zero, the ImageAddress will be used as the base /// address of relocation. /// - PHYSICAL_ADDRESS DestinationAddress; + PHYSICAL_ADDRESS DestinationAddress; /// /// PeCoffLoaderLoadImage() sets EntryPoint to to the entry point of the PE/COFF image. /// - PHYSICAL_ADDRESS EntryPoint; + PHYSICAL_ADDRESS EntryPoint; /// /// Passed in by the caller to PeCoffLoaderGetImageInfo() and PeCoffLoaderLoadImage() /// to abstract accessing the image from the library. /// - PE_COFF_LOADER_READ_FILE ImageRead; + PE_COFF_LOADER_READ_FILE ImageRead; /// /// Used as the FileHandle passed into the ImageRead function when it's called. /// - VOID *Handle; + VOID *Handle; /// /// Caller allocated buffer of size FixupDataSize that can be optionally allocated /// prior to calling PeCoffLoaderRelocateImage(). /// This buffer is filled with the information used to fix up the image. /// The fixups have been applied to the image and this entry is just for information. /// - VOID *FixupData; + VOID *FixupData; /// /// Set by PeCoffLoaderGetImageInfo() to the Section Alignment in the PE/COFF header. /// If the image is a TE image, then this field is set to 0. /// - UINT32 SectionAlignment; + UINT32 SectionAlignment; /// /// Set by PeCoffLoaderGetImageInfo() to offset to the PE/COFF header. /// If the PE/COFF image does not start with a DOS header, this value is zero. /// Otherwise, it's the offset to the PE/COFF header. /// - UINT32 PeCoffHeaderOffset; + UINT32 PeCoffHeaderOffset; /// /// Set by PeCoffLoaderGetImageInfo() to the Relative Virtual Address of the debug directory, /// if it exists in the image /// - UINT32 DebugDirectoryEntryRva; + UINT32 DebugDirectoryEntryRva; /// /// Set by PeCoffLoaderLoadImage() to CodeView area of the PE/COFF Debug directory. /// - VOID *CodeView; + VOID *CodeView; /// /// Set by PeCoffLoaderLoadImage() to point to the PDB entry contained in the CodeView area. /// The PdbPointer points to the filename of the PDB file used for source-level debug of /// the image by a debugger. /// - CHAR8 *PdbPointer; + CHAR8 *PdbPointer; /// /// Is set by PeCoffLoaderGetImageInfo() to the Section Alignment in the PE/COFF header. /// - UINTN SizeOfHeaders; + UINTN SizeOfHeaders; /// /// Not used by this library class. Other library classes that layer on top of this library /// class fill in this value as part of their GetImageInfo call. /// This allows the caller of the library to know what type of memory needs to be allocated /// to load and relocate the image. /// - UINT32 ImageCodeMemoryType; + UINT32 ImageCodeMemoryType; /// /// Not used by this library class. Other library classes that layer on top of this library /// class fill in this value as part of their GetImageInfo call. /// This allows the caller of the library to know what type of memory needs to be allocated /// to load and relocate the image. /// - UINT32 ImageDataMemoryType; + UINT32 ImageDataMemoryType; /// /// Set by any of the library functions if they encounter an error. /// - UINT32 ImageError; + UINT32 ImageError; /// /// Set by PeCoffLoaderLoadImage() to indicate the size of FixupData that the caller must /// allocate before calling PeCoffLoaderRelocateImage(). /// - UINTN FixupDataSize; + UINTN FixupDataSize; /// /// Set by PeCoffLoaderGetImageInfo() to the machine type stored in the PE/COFF header. /// - UINT16 Machine; + UINT16 Machine; /// /// Set by PeCoffLoaderGetImageInfo() to the subsystem type stored in the PE/COFF header. /// - UINT16 ImageType; + UINT16 ImageType; /// /// Set by PeCoffLoaderGetImageInfo() to the DLL flags stored in the PE/COFF header and /// in the DllCharacteristicsEx debug table. /// - UINT16 DllCharacteristics; - UINT32 DllCharacteristicsEx; + UINT16 DllCharacteristics; + UINT32 DllCharacteristicsEx; /// /// Set by PeCoffLoaderGetImageInfo() to TRUE if the PE/COFF image does not contain /// relocation information. /// - BOOLEAN RelocationsStripped; + BOOLEAN RelocationsStripped; /// /// Set by PeCoffLoaderGetImageInfo() to TRUE if the image is a TE image. /// For a definition of the TE Image format, see the Platform Initialization Pre-EFI /// Initialization Core Interface Specification. /// - BOOLEAN IsTeImage; + BOOLEAN IsTeImage; // MU_CHANGE START /// /// Set by PeCoffLoaderGetImageInfo() to TRUE if the image has the IMAGE_DLLCHARACTERISTICS_NX_COMPAT /// flag set. /// - BOOLEAN SupportsNx; + BOOLEAN SupportsNx; // MU_CHANGE END /// /// Set by PeCoffLoaderLoadImage() to the HII resource offset /// if the image contains a custom PE/COFF resource with the type 'HII'. /// Otherwise, the entry remains to be 0. /// - PHYSICAL_ADDRESS HiiResourceData; + PHYSICAL_ADDRESS HiiResourceData; /// /// Private storage for implementation specific data. /// - UINT64 Context; + UINT64 Context; + /// + /// Passed in by the caller to PeCoffLoaderGetImageInfo() and PeCoffLoaderLoadImage() + /// to abstract reading data directories from the library. + /// + PE_COFF_LOADER_READ_DATA_DIRECTORY DataDirectoryRead; + /// + /// Caller-owned opaque pointer provided to DataDirectoryRead. + /// + VOID *DataDirectoryReadContext; } PE_COFF_LOADER_IMAGE_CONTEXT; /** diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c index 6dae14352f8..03e28566c4d 100644 --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c @@ -306,6 +306,25 @@ PeCoffLoaderGetPeHeader ( } } + // + // Execute the data directory callback for each data directory, if the callback is provided. + // + if (ImageContext->DataDirectoryRead != NULL) { + UINT32 DirIndex; + + for (DirIndex = 0; DirIndex < Hdr.Pe32->OptionalHeader.NumberOfRvaAndSizes; DirIndex++) { + Status = ImageContext->DataDirectoryRead ( + DirIndex, + &Hdr.Pe32->OptionalHeader.DataDirectory[DirIndex], + ImageContext->DataDirectoryReadContext + ); + if (RETURN_ERROR (Status)) { + ImageContext->ImageError = Status; + return RETURN_UNSUPPORTED; + } + } + } + // // Use PE32 offset // @@ -428,6 +447,25 @@ PeCoffLoaderGetPeHeader ( } } + // + // Execute the data directory callback for each data directory, if the callback is provided. + // + if (ImageContext->DataDirectoryRead != NULL) { + UINT32 DirIndex; + + for (DirIndex = 0; DirIndex < Hdr.Pe32Plus->OptionalHeader.NumberOfRvaAndSizes; DirIndex++) { + Status = ImageContext->DataDirectoryRead ( + DirIndex, + &Hdr.Pe32Plus->OptionalHeader.DataDirectory[DirIndex], + ImageContext->DataDirectoryReadContext + ); + if (RETURN_ERROR (Status)) { + ImageContext->ImageError = Status; + return RETURN_UNSUPPORTED; + } + } + } + // // Use PE32+ offset // diff --git a/SecurityPkg/Library/DxeImageVerificationLib2/Database.c b/SecurityPkg/Library/DxeImageVerificationLib2/Database.c new file mode 100644 index 00000000000..1e16ab8609e --- /dev/null +++ b/SecurityPkg/Library/DxeImageVerificationLib2/Database.c @@ -0,0 +1,416 @@ +/** @file + Secureboot DB/DBX/DBT (EFI_SIGNATURE_LIST) helpers for the DXE Image Verification Library. + + Copyright (C) Microsoft Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#include "Database.h" + +/** + Load a Secure Boot Signature Database into a pool-allocated buffer. + + When *Buffer is non-NULL the caller takes ownership of the allocation and + must release it with FreePool. + + @param[in] DatabaseName Variable name (e.g. EFI_IMAGE_SECURITY_DATABASE, + EFI_IMAGE_SECURITY_DATABASE1). + @param[out] Buffer Pool-allocated copy of the variable contents, + or NULL if the variable does not exist. + Caller frees with FreePool when non-NULL. + @param[out] BufferSize Size of *Buffer in bytes, or 0 if the + variable does not exist. + + @retval EFI_SUCCESS The variable was loaded successfully, or it was absent. + @retval EFI_INVALID_PARAMETER A required pointer is NULL. + @retval Other Status from gRT->GetVariable. +**/ +EFI_STATUS +LoadSignatureDatabase ( + IN CONST CHAR16 *DatabaseName, + OUT VOID **Buffer, + OUT UINTN *BufferSize + ) +{ + EFI_STATUS Status; + + if ((DatabaseName == NULL) || (Buffer == NULL) || (BufferSize == NULL)) { + return EFI_INVALID_PARAMETER; + } + + *Buffer = NULL; + *BufferSize = 0; + + Status = GetVariable2 (DatabaseName, &gEfiImageSecurityDatabaseGuid, Buffer, BufferSize); + if (Status == EFI_NOT_FOUND) { + return EFI_SUCCESS; + } + + return Status; +} + +/** + Append a GUID to the set if it is not already present. + + @param[in,out] Set The set to update. + @param[in] Guid The GUID to insert. +**/ +STATIC +VOID +AppendUnique ( + IN OUT HASH_ALGORITHM_SET *Set, + IN CONST EFI_GUID *Guid + ) +{ + UINTN Index; + + for (Index = 0; Index < Set->Count; Index++) { + if (CompareGuid (&Set->Guids[Index], Guid)) { + return; + } + } + + ASSERT (Set->Count < ARRAY_SIZE (Set->Guids)); + if (Set->Count < ARRAY_SIZE (Set->Guids)) { + CopyGuid (&Set->Guids[Set->Count], Guid); + Set->Count++; + } +} + +/** + Determines if the given GUID is a supported image hash signature type. + + @param[in] Guid Pointer to an EFI_SIGNATURE_LIST::SignatureType + value, or any candidate signature-type GUID. + + @retval TRUE The image hash signature type is supported. + @retval FALSE The image hash signature type is not supported. +**/ +BOOLEAN +IsKnownImageHashGuid ( + IN CONST EFI_GUID *Guid + ) +{ + UINTN Index; + + if (Guid == NULL) { + return FALSE; + } + + for (Index = 0; Index < ARRAY_SIZE (mKnownImageHashGuids); Index++) { + if (CompareGuid (Guid, mKnownImageHashGuids[Index])) { + return TRUE; + } + } + + return FALSE; +} + +/** + Walk a signature-database buffer, invoking Callback for every + well-formed EFI_SIGNATURE_LIST it contains. + + @param[in] Buffer The raw database contents. + @param[in] BufferSize Size of Buffer in bytes. + @param[in] Callback Invoked once per EFI_SIGNATURE_LIST. + @param[in] Context Opaque pointer passed unmodified to Callback. + + @retval EFI_SUCCESS Buffer was fully consumed and Callback + returned EFI_SUCCESS for every list. + @retval EFI_INVALID_PARAMETER Buffer or Callback is NULL. + @retval EFI_VOLUME_CORRUPTED Buffer is structurally invalid. + @retval Other First non-EFI_SUCCESS status returned + by Callback. Iteration stops immediately. +**/ +EFI_STATUS +WalkSignatureDatabase ( + IN CONST VOID *Buffer, + IN UINTN BufferSize, + IN SIGNATURE_LIST_CALLBACK Callback, + IN VOID *Context OPTIONAL + ) +{ + CONST UINT8 *Cursor; + UINTN Remaining; + EFI_SIGNATURE_LIST *List; + UINTN PayloadSize; + EFI_STATUS CallbackStatus; + + if ((Buffer == NULL) || (Callback == NULL)) { + return EFI_INVALID_PARAMETER; + } + + Cursor = (CONST UINT8 *)Buffer; + Remaining = BufferSize; + + while (Remaining >= sizeof (EFI_SIGNATURE_LIST)) { + List = (EFI_SIGNATURE_LIST *)(VOID *)Cursor; + + // + // Outer header bounds: the list must declare at least the header + // size, and must fit inside what remains of the buffer. + // + if ((List->SignatureListSize < sizeof (EFI_SIGNATURE_LIST)) || + (List->SignatureListSize > Remaining)) + { + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: malformed signature list size.\n")); + return EFI_VOLUME_CORRUPTED; + } + + // + // Header + per-list header must fit within the declared list size, + // and individual signatures must each be at least an EFI_GUID. The + // remaining payload must divide evenly into SignatureSize chunks. + // + if ((List->SignatureSize < sizeof (EFI_GUID)) || + (List->SignatureHeaderSize > List->SignatureListSize - sizeof (EFI_SIGNATURE_LIST))) + { + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: malformed signature list fields.\n")); + return EFI_VOLUME_CORRUPTED; + } + + PayloadSize = List->SignatureListSize + - sizeof (EFI_SIGNATURE_LIST) + - List->SignatureHeaderSize; + if ((PayloadSize % List->SignatureSize) != 0) { + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: signature payload not a multiple of SignatureSize.\n")); + return EFI_VOLUME_CORRUPTED; + } + + CallbackStatus = Callback (List, Context); + if (EFI_ERROR (CallbackStatus)) { + return CallbackStatus; + } + + Cursor += List->SignatureListSize; + Remaining -= List->SignatureListSize; + } + + if (Remaining != 0) { + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: trailing bytes in signature database.\n")); + return EFI_VOLUME_CORRUPTED; + } + + return EFI_SUCCESS; +} + +/** + Walker callback for GetDatabaseHashAlgorithms: appends the list's + SignatureType to the caller's set if it is a recognized image hash + GUID. +**/ +STATIC +EFI_STATUS +EFIAPI +CollectHashAlgorithmCallback ( + IN CONST EFI_SIGNATURE_LIST *List, + IN VOID *Context OPTIONAL + ) +{ + HASH_ALGORITHM_SET *Set; + + if (Context == NULL) { + return EFI_INVALID_PARAMETER; + } + + Set = (HASH_ALGORITHM_SET *)Context; + + if (IsKnownImageHashGuid (&List->SignatureType)) { + AppendUnique (Set, &List->SignatureType); + } + + return EFI_SUCCESS; +} + +/** + Gathers all image hash signature types present in the Db and Dbx. + + @param[in] Db The raw `db` variable contents. + @param[in] DbSize Size of Db in bytes. + @param[in] Dbx The raw `dbx` variable contents. + @param[in] DbxSize Size of Dbx in bytes. + @param[out] HashAlgorithms On success, populated with the set of hash + signature types. + + @retval EFI_SUCCESS Db / Dbx were walked and HashAlgorithms was populated. + @retval EFI_INVALID_PARAMETER HashAlgorithms is NULL. + @retval EFI_VOLUME_CORRUPTED One of the buffers' signature lists is malformed. +**/ +EFI_STATUS +GetDatabaseHashAlgorithms ( + IN CONST VOID *Db OPTIONAL, + IN UINTN DbSize, + IN CONST VOID *Dbx OPTIONAL, + IN UINTN DbxSize, + OUT HASH_ALGORITHM_SET *HashAlgorithms + ) +{ + EFI_STATUS Status; + + if (HashAlgorithms == NULL) { + return EFI_INVALID_PARAMETER; + } + + ZeroMem (HashAlgorithms, sizeof (*HashAlgorithms)); + + if (Db != NULL) { + Status = WalkSignatureDatabase (Db, DbSize, CollectHashAlgorithmCallback, HashAlgorithms); + if (EFI_ERROR (Status)) { + return Status; + } + } + + if (Dbx != NULL) { + Status = WalkSignatureDatabase (Dbx, DbxSize, CollectHashAlgorithmCallback, HashAlgorithms); + if (EFI_ERROR (Status)) { + return Status; + } + } + + return EFI_SUCCESS; +} + +// +// Per-walk context for IsSignatureFoundInDatabase. The caller +// populates the three input fields and clears Found; the walker +// callback updates Found if a matching entry is encountered. +// +typedef struct { + CONST UINT8 *Signature; + CONST EFI_GUID *SignatureType; + UINTN SignatureSize; + BOOLEAN Found; +} SIGNATURE_SEARCH_CTX; + +/** + Walker callback for IsSignatureFoundInDatabase. + + Records a match by setting Search->Found to TRUE and returning EFI_ABORTED + so the walk terminates early. +**/ +STATIC +EFI_STATUS +EFIAPI +SignatureSearchCallback ( + IN CONST EFI_SIGNATURE_LIST *List, + IN VOID *Context OPTIONAL + ) +{ + SIGNATURE_SEARCH_CTX *Search; + UINTN EntryCount; + CONST UINT8 *EntryCursor; + CONST EFI_SIGNATURE_DATA *Entry; + UINTN Index; + + if (Context == NULL) { + return EFI_INVALID_PARAMETER; + } + + Search = (SIGNATURE_SEARCH_CTX *)Context; + + if ((Search->Signature == NULL) || + (Search->SignatureType == NULL) || + (Search->SignatureSize == 0)) + { + return EFI_INVALID_PARAMETER; + } + + // + // A list belongs to a different algorithm if either the type GUID or + // the per-entry size disagrees. EFI_SIGNATURE_DATA already carries an + // owner EFI_GUID, so the payload size is SignatureSize - sizeof(GUID). + // + if (!CompareGuid (&List->SignatureType, Search->SignatureType)) { + return EFI_SUCCESS; + } + + if (List->SignatureSize != sizeof (EFI_GUID) + Search->SignatureSize) { + return EFI_SUCCESS; + } + + EntryCount = (List->SignatureListSize + - sizeof (EFI_SIGNATURE_LIST) + - List->SignatureHeaderSize) / List->SignatureSize; + EntryCursor = (CONST UINT8 *)List + + sizeof (EFI_SIGNATURE_LIST) + + List->SignatureHeaderSize; + + for (Index = 0; Index < EntryCount; Index++) { + Entry = (CONST EFI_SIGNATURE_DATA *)EntryCursor; + if (CompareMem (Entry->SignatureData, Search->Signature, Search->SignatureSize) == 0) { + Search->Found = TRUE; + return EFI_ABORTED; + } + + EntryCursor += List->SignatureSize; + } + + return EFI_SUCCESS; +} + +/** + Search a image signature database buffer for a signature match. + + @param[in] Database The raw database contents. + @param[in] DatabaseSize Size of Database in bytes. + @param[in] Signature The signature digest to search for. + @param[in] SignatureType GUID identifying the digest algorithm. + @param[in] SignatureSize Size of Signature digest in bytes. + @param[out] IsFound TRUE if the signature was located. + + @retval EFI_SUCCESS Search completed; IsFound is valid. + @retval EFI_INVALID_PARAMETER A required pointer is NULL or SignatureSize is 0. + @retval EFI_VOLUME_CORRUPTED Database is structurally malformed. +**/ +EFI_STATUS +IsSignatureFoundInDatabase ( + IN CONST VOID *Database OPTIONAL, + IN UINTN DatabaseSize, + IN CONST UINT8 *Signature, + IN CONST EFI_GUID *SignatureType, + IN UINTN SignatureSize, + OUT BOOLEAN *IsFound + ) +{ + EFI_STATUS Status; + SIGNATURE_SEARCH_CTX Search; + + if ((Signature == NULL) || (SignatureType == NULL) || + (IsFound == NULL) || (SignatureSize == 0)) + { + return EFI_INVALID_PARAMETER; + } + + *IsFound = FALSE; + + if (Database == NULL) { + return EFI_SUCCESS; + } + + Search = (SIGNATURE_SEARCH_CTX) { + .Signature = Signature, + .SignatureType = SignatureType, + .SignatureSize = SignatureSize, + .Found = FALSE + }; + + Status = WalkSignatureDatabase ( + Database, + DatabaseSize, + SignatureSearchCallback, + &Search + ); + // + // SignatureSearchCallback returns EFI_ABORTED to stop the walk + // once a match is recorded in Search.Found; translate it back here. + // + if (Status == EFI_ABORTED) { + Status = EFI_SUCCESS; + } + + if (!EFI_ERROR (Status)) { + *IsFound = Search.Found; + } + + return Status; +} diff --git a/SecurityPkg/Library/DxeImageVerificationLib2/Database.h b/SecurityPkg/Library/DxeImageVerificationLib2/Database.h new file mode 100644 index 00000000000..4ad11f05335 --- /dev/null +++ b/SecurityPkg/Library/DxeImageVerificationLib2/Database.h @@ -0,0 +1,163 @@ +/** @file + Secureboot DB/DBX/DBT (EFI_SIGNATURE_LIST) helpers for the DXE Image Verification Library. + + Copyright (C) Microsoft Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#ifndef DXE_IMAGE_VERIFICATION_LIB_DATABASE_H_ +#define DXE_IMAGE_VERIFICATION_LIB_DATABASE_H_ + +#include "DxeImageVerificationLib.h" +#include + +// +// Supported image hash algorithms defined via their GUID. +// +STATIC CONST EFI_GUID *CONST mKnownImageHashGuids[] = { + &gEfiCertSha1Guid, + &gEfiCertSha256Guid, + &gEfiCertSha384Guid, + &gEfiCertSha512Guid +}; + +// +// A Set of image hash signature-type GUIDs +// +typedef struct { + UINTN Count; + EFI_GUID Guids[ARRAY_SIZE (mKnownImageHashGuids)]; +} HASH_ALGORITHM_SET; + +/** + Determines if the given GUID is a supported image hash signature type. + + @param[in] Guid Pointer to an EFI_SIGNATURE_LIST::SignatureType + value, or any candidate signature-type GUID. + + @retval TRUE The image hash signature type is supported. + @retval FALSE The image hash signature type is not supported. +**/ +BOOLEAN +IsKnownImageHashGuid ( + IN CONST EFI_GUID *Guid + ); + +/** + A callback that is executed by WalkSignatureDatabase once per + EFI_SIGNATURE_LIST in a signature database buffer. + + Returning EFI_SUCCESS continues iteration. Returning any other status + stops the walk and the error is propagated to the caller. + + @param[in] List The signature list currently being iterated. + @param[in] Context Caller-owned opaque pointer passed unmodified + through WalkSignatureDatabase. + + @retval EFI_SUCCESS The list was processed successfully. + @retval other Callback-specific error. +**/ +typedef +EFI_STATUS +(EFIAPI *SIGNATURE_LIST_CALLBACK)( + IN CONST EFI_SIGNATURE_LIST *List, + IN VOID *Context OPTIONAL + ); + +/** + Load a Secure Boot Signature Database into a pool-allocated buffer. + + When *Buffer is non-NULL the caller takes ownership of the allocation and + must release it with FreePool. + + @param[in] DatabaseName Variable name (e.g. EFI_IMAGE_SECURITY_DATABASE, + EFI_IMAGE_SECURITY_DATABASE1). + @param[out] Buffer Pool-allocated copy of the variable contents, + or NULL if the variable does not exist. + Caller frees with FreePool when non-NULL. + @param[out] BufferSize Size of *Buffer in bytes, or 0 if the + variable does not exist. + + @retval EFI_SUCCESS The variable was loaded successfully, or it was absent. + @retval EFI_INVALID_PARAMETER A required pointer is NULL. + @retval Other Status from gRT->GetVariable. +**/ +EFI_STATUS +LoadSignatureDatabase ( + IN CONST CHAR16 *DatabaseName, + OUT VOID **Buffer, + OUT UINTN *BufferSize + ); + +/** + Walk a signature-database buffer, invoking Callback for every + well-formed EFI_SIGNATURE_LIST it contains. + + @param[in] Buffer The raw database contents. + @param[in] BufferSize Size of Buffer in bytes. + @param[in] Callback Invoked once per EFI_SIGNATURE_LIST. + @param[in] Context Opaque pointer passed unmodified to Callback. + + @retval EFI_SUCCESS Buffer was fully consumed and Callback + returned EFI_SUCCESS for every list. + @retval EFI_INVALID_PARAMETER Buffer or Callback is NULL. + @retval EFI_VOLUME_CORRUPTED Buffer is structurally invalid. + @retval Other First non-EFI_SUCCESS status returned + by Callback. Iteration stops immediately. +**/ +EFI_STATUS +WalkSignatureDatabase ( + IN CONST VOID *Buffer, + IN UINTN BufferSize, + IN SIGNATURE_LIST_CALLBACK Callback, + IN VOID *Context OPTIONAL + ); + +/** + Gathers all image hash signature types present in the Db and Dbx. + + @param[in] Db The raw `db` variable contents. + @param[in] DbSize Size of Db in bytes. + @param[in] Dbx The raw `dbx` variable contents. + @param[in] DbxSize Size of Dbx in bytes. + @param[out] HashAlgorithms On success, populated with the set of hash + signature types. + + @retval EFI_SUCCESS Db / Dbx were walked and HashAlgorithms was populated. + @retval EFI_INVALID_PARAMETER HashAlgorithms is NULL. + @retval EFI_VOLUME_CORRUPTED One of the buffers' signature lists is malformed. +**/ +EFI_STATUS +GetDatabaseHashAlgorithms ( + IN CONST VOID *Db OPTIONAL, + IN UINTN DbSize, + IN CONST VOID *Dbx OPTIONAL, + IN UINTN DbxSize, + OUT HASH_ALGORITHM_SET *HashAlgorithms + ); + +/** + Search a image signature database buffer for a signature match. + + @param[in] Database The raw database contents. + @param[in] DatabaseSize Size of Database in bytes. + @param[in] Signature The signature digest to search for. + @param[in] SignatureType GUID identifying the digest algorithm. + @param[in] SignatureSize Size of Signature digest in bytes. + @param[out] IsFound TRUE if the signature was located. + + @retval EFI_SUCCESS Search completed; IsFound is valid. + @retval EFI_INVALID_PARAMETER A required pointer is NULL or SignatureSize is 0. + @retval EFI_VOLUME_CORRUPTED Database is structurally malformed. +**/ +EFI_STATUS +IsSignatureFoundInDatabase ( + IN CONST VOID *Database OPTIONAL, + IN UINTN DatabaseSize, + IN CONST UINT8 *Signature, + IN CONST EFI_GUID *SignatureType, + IN UINTN SignatureSize, + OUT BOOLEAN *IsFound + ); + +#endif // DXE_IMAGE_VERIFICATION_LIB_DATABASE_H_ diff --git a/SecurityPkg/Library/DxeImageVerificationLib2/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib2/DxeImageVerificationLib.c new file mode 100644 index 00000000000..9cf68b9c92a --- /dev/null +++ b/SecurityPkg/Library/DxeImageVerificationLib2/DxeImageVerificationLib.c @@ -0,0 +1,325 @@ +/** @file + Implement image verification services for secure boot service + + Caution: This file requires additional review when modified. + This library will have external input - PE/COFF image. + This external input must be validated carefully to avoid security issue like + buffer overflow, integer overflow. + + DxeImageVerificationLibImageRead() function will make sure the PE/COFF image content + read is within the image buffer. + + DxeImageVerificationHandler(), HashPeImageByType(), HashPeImage() function will accept + untrusted PE/COFF image and validate its data structure within this image buffer before use. + +Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
+(C) Copyright 2016 Hewlett Packard Enterprise Development LP
+Copyright (c) Microsoft Corporation. +SPDX-License-Identifier: BSD-2-Clause-Patent + +**/ + +#include "DxeImageVerificationLib.h" +#include "Database.h" +#include "Support.h" +#include "Policy.h" + +/** + Provide verification service for signed images, which include both signature validation + and platform policy control. For signature types, both UEFI WIN_CERTIFICATE_UEFI_GUID and + MSFT Authenticode type signatures are supported. + + In this implementation, only verify external executables when in USER MODE. + Executables from FV is bypass, so pass in AuthenticationStatus is ignored. + + The image verification policy is: + If the image is signed, + At least one valid signature or at least one hash value of the image must match a record + 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 + not be reflected in the security data base "dbx". + + Caution: This function may receive untrusted input. + PE/COFF image is external input, so this function will validate its data structure + within this image buffer before use. + + @param[in] AuthenticationStatus + This is the authentication status returned from the security + measurement services for the input file. + @param[in] File This is a pointer to the device path of the file that is + being dispatched. This will optionally be used for logging. + @param[in] FileBuffer File buffer matches the input file device path. + @param[in] FileSize Size of File buffer matches the input file device path. + @param[in] BootPolicy A boot policy that was used to call LoadImage() UEFI service. + + @retval EFI_SUCCESS The file specified by DevicePath and non-NULL + FileBuffer did authenticate, and the platform policy dictates + that the DXE Foundation may use the file. + @retval EFI_SUCCESS The device path specified by NULL device path DevicePath + and non-NULL FileBuffer did authenticate, and the platform + policy dictates that the DXE Foundation may execute the image in + FileBuffer. + @retval EFI_ACCESS_DENIED The file specified by File and FileBuffer did not + authenticate, and the DXE Foundation may not use File. The + image has been added to the file execution table. + +**/ +EFI_STATUS +EFIAPI +DxeImageVerificationHandler ( + IN UINT32 AuthenticationStatus, + IN CONST EFI_DEVICE_PATH_PROTOCOL *File OPTIONAL, + IN VOID *FileBuffer, + IN UINTN FileSize, + IN BOOLEAN BootPolicy + ) +{ + EFI_STATUS Status; + UINT32 Policy; + EFI_IMAGE_DATA_DIRECTORY SecDataDir; + EFI_IMAGE_EXECUTION_ACTION Action; + + Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED; + + // + // Sanity check. + // + if (File == NULL) { + return EFI_INVALID_PARAMETER; + } + + // + // Resolve the platform authorization policy from the image's origin. + // This runs before the Secure Boot variable check because it is much + // cheaper, and the common case (FV-dispatched drivers) short-circuits + // if the policy is ALWAYS_EXECUTE. + // + Status = GetExecutionPolicy (File, &Policy); + if (EFI_ERROR (Status)) { + return Status; + } + + // + // Policy unconditionally permits execution; no further checks needed. + // + if (Policy == ALWAYS_EXECUTE) { + return EFI_SUCCESS; + } + + // + // This handler only enforces UEFI Secure Boot. If Secure Boot is not + // enabled there is nothing for us to verify. + // + if (!IsSecureBootEnabled ()) { + return EFI_SUCCESS; + } + + // + // Inspect the image to locate its security data directory. Any failure + // to parse the PE/COFF headers is treated as a verification failure. + // + Status = GetImageSecurityDataDirectory (FileBuffer, FileSize, &SecDataDir); + if (EFI_ERROR (Status)) { + goto Exit; + } + + // + // Dispatch to the appropriate verification path based on whether the + // image carries an embedded signature. + // + if (SecDataDir.Size == 0) { + Status = ValidateUnsignedImage (FileBuffer, FileSize); + } else { + Status = ValidateSignedImage (FileBuffer, FileSize, &SecDataDir, &Action); + } + +Exit: + return Status; +} + +/** + Register security measurement handler. + + @param ImageHandle ImageHandle of the loaded driver. + @param SystemTable Pointer to the EFI System Table. + + @retval EFI_SUCCESS The handlers were registered successfully. +**/ +EFI_STATUS +EFIAPI +DxeImageVerificationLibConstructor ( + IN EFI_HANDLE ImageHandle, + IN EFI_SYSTEM_TABLE *SystemTable + ) +{ + return RegisterSecurity2Handler ( + DxeImageVerificationHandler, + EFI_AUTH_OPERATION_VERIFY_IMAGE | EFI_AUTH_OPERATION_IMAGE_REQUIRED + ); +} + +/** + Validate an unsigned PE/COFF image against the platform signature + databases. + + For each image-hash algorithm enrolled in db or dbx, computes the + image's Authenticode digest and checks the dbx then db for the hash. + A dbx hit denies the image. The image is authorized only if it is + found in db and never in dbx. + + @param[in] FileBuffer Pointer to the in-memory PE/COFF image. + @param[in] FileSize Size of FileBuffer in bytes. + + @retval EFI_SUCCESS The image's hash was found in db (and not + in dbx) under at least one enrolled + algorithm. + @retval EFI_ACCESS_DENIED The image was rejected: either no hash + algorithm is enrolled, the digest is + present in dbx, the digest is not present + in db, or a database lookup failed. +**/ +EFI_STATUS +ValidateUnsignedImage ( + IN VOID *FileBuffer, + IN UINTN FileSize + ) +{ + EFI_STATUS Status; + HASH_ALGORITHM_SET HashAlgorithms; + UINTN Index; + CONST EFI_GUID *HashType; + UINTN DigestSize; + UINT8 ImageDigest[SHA512_DIGEST_SIZE]; + BOOLEAN IsFound; + BOOLEAN IsFoundInDb; + VOID *Db; + UINTN DbSize; + VOID *Dbx; + UINTN DbxSize; + + Db = NULL; + Dbx = NULL; + + // + // Load the authorized (db) and forbidden (dbx) signature databases. + // + Status = LoadSignatureDatabase (EFI_IMAGE_SECURITY_DATABASE, &Db, &DbSize); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: failed to load db - %r\n", Status)); + Status = EFI_ACCESS_DENIED; + goto Exit; + } + + Status = LoadSignatureDatabase (EFI_IMAGE_SECURITY_DATABASE1, &Dbx, &DbxSize); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: failed to load dbx - %r\n", Status)); + Status = EFI_ACCESS_DENIED; + goto Exit; + } + + // + // Determine which algorithms are currently in use across db and dbx. + // If neither database enrolls any recognized hash type, there is no algorithm + // with which to authorize an unsigned image, so refuse to dispatch it. + // + Status = GetDatabaseHashAlgorithms (Db, DbSize, Dbx, DbxSize, &HashAlgorithms); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: GetDatabaseHashAlgorithms failed - %r\n", Status)); + Status = EFI_ACCESS_DENIED; + goto Exit; + } + + if (HashAlgorithms.Count == 0) { + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: no hash algorithms enrolled in db/dbx; rejecting unsigned image.\n")); + Status = EFI_ACCESS_DENIED; + goto Exit; + } + + // + // For each algorithm in use, compute the image's Authenticode digest and query the dbx / db. + // A hit in the dbx immediately denies the image. If no dbx hit occurs across all algorithms, + // the image is authorized or denied based on whether at least one db hit was recorded. + // + IsFoundInDb = FALSE; + for (Index = 0; Index < HashAlgorithms.Count; Index++) { + HashType = &HashAlgorithms.Guids[Index]; + + Status = GetAuthenticodeHash (FileBuffer, FileSize, HashType, ImageDigest, &DigestSize); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: GetAuthenticodeHash failed - %r\n", Status)); + Status = EFI_ACCESS_DENIED; + goto Exit; + } + + Status = IsSignatureFoundInDatabase ( + Dbx, + DbxSize, + ImageDigest, + HashType, + DigestSize, + &IsFound + ); + if (EFI_ERROR (Status) || IsFound) { + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Image is not signed and image is forbidden by DBX.\n")); + Status = EFI_ACCESS_DENIED; + goto Exit; + } + + if (IsFoundInDb) { + continue; + } + + Status = IsSignatureFoundInDatabase ( + Db, + DbSize, + ImageDigest, + HashType, + DigestSize, + &IsFound + ); + if (!EFI_ERROR (Status) && IsFound) { + IsFoundInDb = TRUE; + } + } + + Status = IsFoundInDb ? EFI_SUCCESS : EFI_ACCESS_DENIED; + +Exit: + if (Db != NULL) { + FreePool (Db); + } + + if (Dbx != NULL) { + FreePool (Dbx); + } + + return Status; +} + +/** + Validate a signed PE/COFF image's embedded Authenticode/UEFI signatures + against the platform signature databases. + + @param[in] FileBuffer Pointer to the in-memory PE/COFF image. + @param[in] FileSize Size of FileBuffer in bytes. + @param[in] SecDataDir Security data directory describing the + embedded WIN_CERTIFICATE table. + @param[out] Action Set to the EFI_IMAGE_EXECUTION_ACTION value + that best describes the outcome. + + @retval EFI_UNSUPPORTED The signed-image verification path is not yet + implemented. +**/ +EFI_STATUS +ValidateSignedImage ( + IN VOID *FileBuffer, + IN UINTN FileSize, + IN CONST EFI_IMAGE_DATA_DIRECTORY *SecDataDir, + OUT EFI_IMAGE_EXECUTION_ACTION *Action + ) +{ + *Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED; + return EFI_UNSUPPORTED; +} diff --git a/SecurityPkg/Library/DxeImageVerificationLib2/DxeImageVerificationLib.h b/SecurityPkg/Library/DxeImageVerificationLib2/DxeImageVerificationLib.h new file mode 100644 index 00000000000..ddfb47e4964 --- /dev/null +++ b/SecurityPkg/Library/DxeImageVerificationLib2/DxeImageVerificationLib.h @@ -0,0 +1,144 @@ +/** @file + Internal declarations for the DXE Image Verification Library. + + This header is consumed by the library's source files (and its unit + tests) to share prototypes for the constructor and the Security2 + verification handler. + + Copyright (C) Microsoft Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#ifndef DXE_IMAGE_VERIFICATION_LIB_H_ +#define DXE_IMAGE_VERIFICATION_LIB_H_ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/** + Provide verification service for signed images, which include both signature validation + and platform policy control. For signature types, both UEFI WIN_CERTIFICATE_UEFI_GUID and + MSFT Authenticode type signatures are supported. + + In this implementation, only verify external executables when in USER MODE. + Executables from FV is bypass, so pass in AuthenticationStatus is ignored. + + The image verification policy is: + If the image is signed, + At least one valid signature or at least one hash value of the image must match a record + 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 + not be reflected in the security data base "dbx". + + Caution: This function may receive untrusted input. + PE/COFF image is external input, so this function will validate its data structure + within this image buffer before use. + + @param[in] AuthenticationStatus + This is the authentication status returned from the security + measurement services for the input file. + @param[in] File This is a pointer to the device path of the file that is + being dispatched. This will optionally be used for logging. + @param[in] FileBuffer File buffer matches the input file device path. + @param[in] FileSize Size of File buffer matches the input file device path. + @param[in] BootPolicy A boot policy that was used to call LoadImage() UEFI service. + + @retval EFI_SUCCESS The file specified by DevicePath and non-NULL + FileBuffer did authenticate, and the platform policy dictates + that the DXE Foundation may use the file. + @retval EFI_SUCCESS The device path specified by NULL device path DevicePath + and non-NULL FileBuffer did authenticate, and the platform + policy dictates that the DXE Foundation may execute the image in + FileBuffer. + @retval EFI_ACCESS_DENIED The file specified by File and FileBuffer did not + authenticate, and the DXE Foundation may not use File. The + image has been added to the file execution table. + +**/ +EFI_STATUS +EFIAPI +DxeImageVerificationHandler ( + IN UINT32 AuthenticationStatus, + IN CONST EFI_DEVICE_PATH_PROTOCOL *File OPTIONAL, + IN VOID *FileBuffer, + IN UINTN FileSize, + IN BOOLEAN BootPolicy + ); + +/** + Register security measurement handler. + + @param ImageHandle ImageHandle of the loaded driver. + @param SystemTable Pointer to the EFI System Table. + + @retval EFI_SUCCESS The handlers were registered successfully. +**/ +EFI_STATUS +EFIAPI +DxeImageVerificationLibConstructor ( + IN EFI_HANDLE ImageHandle, + IN EFI_SYSTEM_TABLE *SystemTable + ); + +/** + Validate an unsigned PE/COFF image against the platform signature + databases. + + For each image-hash algorithm enrolled in db or dbx, computes the + image's Authenticode digest and checks the dbx then db for the hash. + A dbx hit denies the image. The image is authorized only if it is + found in db and never in dbx. + + @param[in] FileBuffer Pointer to the in-memory PE/COFF image. + @param[in] FileSize Size of FileBuffer in bytes. + + @retval EFI_SUCCESS The image's hash was found in db (and not + in dbx) under at least one enrolled + algorithm. + @retval EFI_ACCESS_DENIED The image was rejected: either no hash + algorithm is enrolled, the digest is + present in dbx, the digest is not present + in db, or a database lookup failed. +**/ +EFI_STATUS +ValidateUnsignedImage ( + IN VOID *FileBuffer, + IN UINTN FileSize + ); + +/** +Validate a signed PE/COFF image's embedded Authenticode/UEFI signatures +against the platform signature databases. + +@param[in] FileBuffer Pointer to the in-memory PE/COFF image. +@param[in] FileSize Size of FileBuffer in bytes. +@param[in] SecDataDir Security data directory describing the + embedded WIN_CERTIFICATE table. +@param[out] Action Set to the EFI_IMAGE_EXECUTION_ACTION value + that best describes the outcome. + +@retval EFI_UNSUPPORTED The signed-image verification path is not yet + implemented. +**/ +EFI_STATUS +ValidateSignedImage ( + IN VOID *FileBuffer, + IN UINTN FileSize, + IN CONST EFI_IMAGE_DATA_DIRECTORY *SecDataDir, + OUT EFI_IMAGE_EXECUTION_ACTION *Action + ); + +#endif // DXE_IMAGE_VERIFICATION_LIB_H_ diff --git a/SecurityPkg/Library/DxeImageVerificationLib2/DxeImageVerificationLib.inf b/SecurityPkg/Library/DxeImageVerificationLib2/DxeImageVerificationLib.inf new file mode 100644 index 00000000000..1f9f679fde4 --- /dev/null +++ b/SecurityPkg/Library/DxeImageVerificationLib2/DxeImageVerificationLib.inf @@ -0,0 +1,94 @@ +## @file +# Provides security service of image verification +# +# This library hooks LoadImage() API to verify every image by the verification policy. +# +# Caution: This module requires additional review when modified. +# This library will have external input - PE/COFF image. +# This external input must be validated carefully to avoid security issues such as +# buffer overflow or integer overflow. +# +# Copyright (C) Microsoft Corporation. +# SPDX-License-Identifier: BSD-2-Clause-Patent +# +## + +[Defines] + INF_VERSION = 0x00010005 + BASE_NAME = DxeImageVerificationLib + MODULE_UNI_FILE = DxeImageVerificationLib.uni + FILE_GUID = CFEF751D-B633-489D-8DB4-BE39262D1599 + MODULE_TYPE = DXE_DRIVER + VERSION_STRING = 1.0 + LIBRARY_CLASS = NULL|DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER + CONSTRUCTOR = DxeImageVerificationLibConstructor + +# +# The following information is for reference only and not required by the build tools. +# +# VALID_ARCHITECTURES = IA32 X64 EBC +# + +[Sources] + Database.c + Database.h + DxeImageVerificationLib.c + DxeImageVerificationLib.h + Policy.c + Policy.h + Support.c + Support.h + +[Packages] + MdePkg/MdePkg.dec + MdeModulePkg/MdeModulePkg.dec + CryptoPkg/CryptoPkg.dec + SecurityPkg/SecurityPkg.dec + +[LibraryClasses] + MemoryAllocationLib + BaseLib + UefiLib + UefiBootServicesTableLib + UefiRuntimeServicesTableLib + BaseMemoryLib + DebugLib + PeCoffLib + SecurityManagementLib + +[Protocols] + +[Guids] + ## SOMETIMES_CONSUMES ## Variable:L"DB" + ## SOMETIMES_CONSUMES ## Variable:L"DBX" + ## SOMETIMES_CONSUMES ## Variable:L"DBT" + ## PRODUCES ## SystemTable + ## CONSUMES ## SystemTable + gEfiImageSecurityDatabaseGuid + + ## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the signature. + ## SOMETIMES_PRODUCES ## GUID # Unique ID for the type of the signature. + gEfiCertSha1Guid + + ## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the signature. + ## 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 + + gEfiCertX509Guid ## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the signature. + gEfiCertX509Sha256Guid ## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the signature. + gEfiCertX509Sha384Guid ## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the signature. + gEfiCertX509Sha512Guid ## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the signature. + gEfiCertPkcs7Guid ## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the certificate. + +[Pcd] + gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy ## SOMETIMES_CONSUMES + gEfiSecurityPkgTokenSpaceGuid.PcdRemovableMediaImageVerificationPolicy ## SOMETIMES_CONSUMES + gEfiSecurityPkgTokenSpaceGuid.PcdFixedMediaImageVerificationPolicy ## SOMETIMES_CONSUMES diff --git a/SecurityPkg/Library/DxeImageVerificationLib2/DxeImageVerificationLib.uni b/SecurityPkg/Library/DxeImageVerificationLib2/DxeImageVerificationLib.uni new file mode 100644 index 00000000000..b2dbca0cda7 --- /dev/null +++ b/SecurityPkg/Library/DxeImageVerificationLib2/DxeImageVerificationLib.uni @@ -0,0 +1,20 @@ +// /** @file +// Provides security service of image verification +// +// This library hooks LoadImage() API to verify every image by the verification policy. +// +// Caution: This module requires additional review when modified. +// This library will have external input - PE/COFF image. +// This external input must be validated carefully to avoid security issues such as +// buffer overflow or integer overflow. +// +// Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
+// +// SPDX-License-Identifier: BSD-2-Clause-Patent +// +// **/ + + +#string STR_MODULE_ABSTRACT #language en-US "Provides security service of image verification" + +#string STR_MODULE_DESCRIPTION #language en-US "This library hooks LoadImage() API to verify every image by the verification policy. Caution: This module requires additional review when modified. This library will have external input - PE/COFF image. This external input must be validated carefully to avoid security issues such as buffer overflow or integer overflow." diff --git a/SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/DatabaseGoogleTest.cpp b/SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/DatabaseGoogleTest.cpp new file mode 100644 index 00000000000..5e5ed55398d --- /dev/null +++ b/SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/DatabaseGoogleTest.cpp @@ -0,0 +1,862 @@ +/** @file + Unit tests for the signature-database helpers in + DxeImageVerificationLib (Database.c): IsKnownImageHashGuid, + WalkSignatureDatabase, GetDatabaseHashAlgorithms, + IsSignatureFoundInDatabase, and LoadSignatureDatabase. + WalkSignatureDatabase, GetDatabaseHashAlgorithms, and + IsSignatureFoundInDatabase are exercised against synthetic in-memory + EFI_SIGNATURE_LIST buffers built by helpers in this file. + LoadSignatureDatabase is exercised against a mocked GetVariable2. + Copyright (C) Microsoft Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#include +#include + +#include +#include + +extern "C" { + #include + #include + #include + #include + #include "../Database.h" +} + +using ::testing::_; +using ::testing::Invoke; +using ::testing::Return; + +// --------------------------------------------------------------------------- +// Helpers for constructing signature-list buffers. +// --------------------------------------------------------------------------- + +// +// Append one EFI_SIGNATURE_LIST containing SignatureCount entries of +// EntrySize bytes each (entry payloads are zero-initialized) to Buffer. +// Returns the offset of the new list within Buffer. +// +static size_t +AppendSignatureList ( + std::vector &Buffer, + const EFI_GUID &SignatureType, + UINT32 SignatureHeaderSize, + UINT32 EntrySize, + UINT32 SignatureCount + ) +{ + const size_t PayloadBytes = (size_t)EntrySize * (size_t)SignatureCount; + const size_t ListBytes = sizeof (EFI_SIGNATURE_LIST) + SignatureHeaderSize + PayloadBytes; + const size_t Offset = Buffer.size (); + + Buffer.resize (Offset + ListBytes, 0); + + EFI_SIGNATURE_LIST *List = (EFI_SIGNATURE_LIST *)(Buffer.data () + Offset); + + CopyMem (&List->SignatureType, &SignatureType, sizeof (EFI_GUID)); + List->SignatureListSize = (UINT32)ListBytes; + List->SignatureHeaderSize = SignatureHeaderSize; + List->SignatureSize = EntrySize; + + return Offset; +} + +// +// Walk callback that simply counts invocations and records the +// per-list SignatureType GUIDs in visit order. +// +struct CallbackRecorder { + UINTN Count; + std::vector SeenTypes; + RETURN_STATUS ReturnAt; // RETURN_SUCCESS to keep going + UINTN AbortAfter; // applies when ReturnAt != SUCCESS +}; + +extern "C" EFI_STATUS EFIAPI +RecorderCallback ( + IN CONST EFI_SIGNATURE_LIST *List, + IN VOID *Context OPTIONAL + ) +{ + CallbackRecorder *Rec = (CallbackRecorder *)Context; + + Rec->Count++; + Rec->SeenTypes.push_back (List->SignatureType); + + if ((Rec->ReturnAt != EFI_SUCCESS) && (Rec->Count >= Rec->AbortAfter)) { + return Rec->ReturnAt; + } + + return EFI_SUCCESS; +} + +// SHA-256 entry size: 16-byte owner GUID + 32-byte digest. +static constexpr UINT32 kSha256EntrySize = sizeof (EFI_GUID) + 32; +static constexpr UINT32 kSha384EntrySize = sizeof (EFI_GUID) + 48; + +// --------------------------------------------------------------------------- +// IsKnownImageHashGuid +// --------------------------------------------------------------------------- + +TEST (IsKnownImageHashGuidTest, NullGuid_ReturnsFalse) { + EXPECT_FALSE (IsKnownImageHashGuid (NULL)); +} + +TEST (IsKnownImageHashGuidTest, KnownHashGuids_ReturnTrue) { + EXPECT_TRUE (IsKnownImageHashGuid (&gEfiCertSha1Guid)); + EXPECT_TRUE (IsKnownImageHashGuid (&gEfiCertSha256Guid)); + EXPECT_TRUE (IsKnownImageHashGuid (&gEfiCertSha384Guid)); + EXPECT_TRUE (IsKnownImageHashGuid (&gEfiCertSha512Guid)); +} + +TEST (IsKnownImageHashGuidTest, X509Guids_ReturnFalse) { + // X509-with-hash variants are intentionally excluded. + EXPECT_FALSE (IsKnownImageHashGuid (&gEfiCertX509Guid)); + EXPECT_FALSE (IsKnownImageHashGuid (&gEfiCertX509Sha256Guid)); + EXPECT_FALSE (IsKnownImageHashGuid (&gEfiCertX509Sha384Guid)); + EXPECT_FALSE (IsKnownImageHashGuid (&gEfiCertX509Sha512Guid)); +} + +TEST (IsKnownImageHashGuidTest, ArbitraryGuid_ReturnsFalse) { + EFI_GUID Junk = { 0x12345678, 0x1234, 0x5678, + { 0x9a, 0xbc, 0xde, 0xf0,0x12, 0x34, 0x56, 0x78 } + }; + + EXPECT_FALSE (IsKnownImageHashGuid (&Junk)); +} + +// --------------------------------------------------------------------------- +// WalkSignatureDatabase +// --------------------------------------------------------------------------- + +class WalkSignatureDatabaseTest : public ::testing::Test { +protected: + CallbackRecorder Rec{ }; + + void + SetUp ( + ) override + { + Rec.Count = 0; + Rec.ReturnAt = EFI_SUCCESS; + Rec.AbortAfter = 0; + Rec.SeenTypes.clear (); + } +}; + +TEST_F (WalkSignatureDatabaseTest, NullBuffer_ReturnsInvalidParameter) { + EXPECT_EQ ( + WalkSignatureDatabase (NULL, 0, RecorderCallback, &Rec), + EFI_INVALID_PARAMETER + ); + EXPECT_EQ (Rec.Count, 0u); +} + +TEST_F (WalkSignatureDatabaseTest, NullCallback_ReturnsInvalidParameter) { + UINT8 Dummy = 0; + + EXPECT_EQ ( + WalkSignatureDatabase (&Dummy, 1, NULL, &Rec), + EFI_INVALID_PARAMETER + ); +} + +TEST_F (WalkSignatureDatabaseTest, EmptyBuffer_ReturnsSuccessNoInvocations) { + UINT8 Dummy = 0; + + EXPECT_EQ ( + WalkSignatureDatabase (&Dummy, 0, RecorderCallback, &Rec), + EFI_SUCCESS + ); + EXPECT_EQ (Rec.Count, 0u); +} + +TEST_F (WalkSignatureDatabaseTest, SingleList_InvokesCallbackOnce) { + std::vector Buf; + + AppendSignatureList (Buf, gEfiCertSha256Guid, 0, kSha256EntrySize, 2); + + EXPECT_EQ ( + WalkSignatureDatabase (Buf.data (), Buf.size (), RecorderCallback, &Rec), + EFI_SUCCESS + ); + EXPECT_EQ (Rec.Count, 1u); + ASSERT_EQ (Rec.SeenTypes.size (), 1u); + EXPECT_EQ (CompareGuid (&Rec.SeenTypes[0], &gEfiCertSha256Guid), TRUE); +} + +TEST_F (WalkSignatureDatabaseTest, MultipleLists_InvokedInOrder) { + std::vector Buf; + + AppendSignatureList (Buf, gEfiCertSha256Guid, 0, kSha256EntrySize, 1); + AppendSignatureList (Buf, gEfiCertSha384Guid, 0, kSha384EntrySize, 3); + AppendSignatureList (Buf, gEfiCertX509Guid, 0, sizeof (EFI_GUID) + 8, 1); + + EXPECT_EQ ( + WalkSignatureDatabase (Buf.data (), Buf.size (), RecorderCallback, &Rec), + EFI_SUCCESS + ); + ASSERT_EQ (Rec.SeenTypes.size (), 3u); + EXPECT_EQ (CompareGuid (&Rec.SeenTypes[0], &gEfiCertSha256Guid), TRUE); + EXPECT_EQ (CompareGuid (&Rec.SeenTypes[1], &gEfiCertSha384Guid), TRUE); + EXPECT_EQ (CompareGuid (&Rec.SeenTypes[2], &gEfiCertX509Guid), TRUE); +} + +TEST_F (WalkSignatureDatabaseTest, ListSizeBelowHeader_ReturnsCorrupted) { + std::vector Buf (sizeof (EFI_SIGNATURE_LIST), 0); + EFI_SIGNATURE_LIST *List = (EFI_SIGNATURE_LIST *)Buf.data (); + + CopyGuid (&List->SignatureType, &gEfiCertSha256Guid); + List->SignatureListSize = sizeof (EFI_SIGNATURE_LIST) - 1; // bad + List->SignatureHeaderSize = 0; + List->SignatureSize = kSha256EntrySize; + + EXPECT_EQ ( + WalkSignatureDatabase (Buf.data (), Buf.size (), RecorderCallback, &Rec), + EFI_VOLUME_CORRUPTED + ); + EXPECT_EQ (Rec.Count, 0u); +} + +TEST_F (WalkSignatureDatabaseTest, ListSizeExceedsBuffer_ReturnsCorrupted) { + std::vector Buf; + + AppendSignatureList (Buf, gEfiCertSha256Guid, 0, kSha256EntrySize, 1); + + // Inflate the declared size past Buf.size(). + ((EFI_SIGNATURE_LIST *)Buf.data ())->SignatureListSize = (UINT32)(Buf.size () + 1); + + EXPECT_EQ ( + WalkSignatureDatabase (Buf.data (), Buf.size (), RecorderCallback, &Rec), + EFI_VOLUME_CORRUPTED + ); + EXPECT_EQ (Rec.Count, 0u); +} + +TEST_F (WalkSignatureDatabaseTest, SignatureSizeBelowGuidSize_ReturnsCorrupted) { + std::vector Buf; + + AppendSignatureList (Buf, gEfiCertSha256Guid, 0, kSha256EntrySize, 1); + + ((EFI_SIGNATURE_LIST *)Buf.data ())->SignatureSize = sizeof (EFI_GUID) - 1; + + EXPECT_EQ ( + WalkSignatureDatabase (Buf.data (), Buf.size (), RecorderCallback, &Rec), + EFI_VOLUME_CORRUPTED + ); +} + +TEST_F (WalkSignatureDatabaseTest, HeaderSizeOverflowsList_ReturnsCorrupted) { + std::vector Buf; + + AppendSignatureList (Buf, gEfiCertSha256Guid, 0, kSha256EntrySize, 1); + + // SignatureHeaderSize larger than the per-list payload region. + ((EFI_SIGNATURE_LIST *)Buf.data ())->SignatureHeaderSize = + ((EFI_SIGNATURE_LIST *)Buf.data ())->SignatureListSize; + + EXPECT_EQ ( + WalkSignatureDatabase (Buf.data (), Buf.size (), RecorderCallback, &Rec), + EFI_VOLUME_CORRUPTED + ); +} + +TEST_F (WalkSignatureDatabaseTest, PayloadNotMultipleOfEntry_ReturnsCorrupted) { + std::vector Buf; + + AppendSignatureList (Buf, gEfiCertSha256Guid, 0, kSha256EntrySize, 2); + + // Bump the entry size so payload no longer divides evenly. + ((EFI_SIGNATURE_LIST *)Buf.data ())->SignatureSize = kSha256EntrySize + 1; + + EXPECT_EQ ( + WalkSignatureDatabase (Buf.data (), Buf.size (), RecorderCallback, &Rec), + EFI_VOLUME_CORRUPTED + ); +} + +TEST_F (WalkSignatureDatabaseTest, TrailingBytes_ReturnsCorrupted) { + std::vector Buf; + + AppendSignatureList (Buf, gEfiCertSha256Guid, 0, kSha256EntrySize, 1); + // Append a partial header that's smaller than EFI_SIGNATURE_LIST. + Buf.push_back (0xAA); + + EXPECT_EQ ( + WalkSignatureDatabase (Buf.data (), Buf.size (), RecorderCallback, &Rec), + EFI_VOLUME_CORRUPTED + ); + EXPECT_EQ (Rec.Count, 1u); // first list was visited before trailing-byte check +} + +TEST_F (WalkSignatureDatabaseTest, CallbackAborts_PropagatesStatus) { + std::vector Buf; + + AppendSignatureList (Buf, gEfiCertSha256Guid, 0, kSha256EntrySize, 1); + AppendSignatureList (Buf, gEfiCertSha384Guid, 0, kSha384EntrySize, 1); + AppendSignatureList (Buf, gEfiCertSha512Guid, 0, sizeof (EFI_GUID) + 64, 1); + + Rec.ReturnAt = EFI_ABORTED; + Rec.AbortAfter = 2; + + EXPECT_EQ ( + WalkSignatureDatabase (Buf.data (), Buf.size (), RecorderCallback, &Rec), + EFI_ABORTED + ); + EXPECT_EQ (Rec.Count, 2u); // third list never visited +} + +// --------------------------------------------------------------------------- +// GetDatabaseHashAlgorithms +// --------------------------------------------------------------------------- + +TEST (GetDatabaseHashAlgorithmsTest, NullArg_ReturnsInvalidParameter) { + EXPECT_EQ ( + GetDatabaseHashAlgorithms (NULL, 0, NULL, 0, NULL), + EFI_INVALID_PARAMETER + ); +} + +TEST (GetDatabaseHashAlgorithmsTest, BothBuffersNull_EmptySet) { + HASH_ALGORITHM_SET Set; + + EXPECT_EQ ( + GetDatabaseHashAlgorithms (NULL, 0, NULL, 0, &Set), + EFI_SUCCESS + ); + EXPECT_EQ (Set.Count, 0u); +} + +TEST (GetDatabaseHashAlgorithmsTest, OnlyDb_HashType_Reported) { + std::vector Db; + + AppendSignatureList (Db, gEfiCertSha256Guid, 0, kSha256EntrySize, 1); + + HASH_ALGORITHM_SET Set; + + EXPECT_EQ ( + GetDatabaseHashAlgorithms (Db.data (), Db.size (), NULL, 0, &Set), + EFI_SUCCESS + ); + ASSERT_EQ (Set.Count, 1u); + EXPECT_EQ (CompareGuid (&Set.Guids[0], &gEfiCertSha256Guid), TRUE); +} + +TEST (GetDatabaseHashAlgorithmsTest, OnlyDbx_HashType_Reported) { + std::vector Dbx; + + AppendSignatureList (Dbx, gEfiCertSha384Guid, 0, kSha384EntrySize, 2); + + HASH_ALGORITHM_SET Set; + + EXPECT_EQ ( + GetDatabaseHashAlgorithms (NULL, 0, Dbx.data (), Dbx.size (), &Set), + EFI_SUCCESS + ); + ASSERT_EQ (Set.Count, 1u); + EXPECT_EQ (CompareGuid (&Set.Guids[0], &gEfiCertSha384Guid), TRUE); +} + +TEST (GetDatabaseHashAlgorithmsTest, BothPresent_Union) { + std::vector Db; + + AppendSignatureList (Db, gEfiCertSha256Guid, 0, kSha256EntrySize, 1); + + std::vector Dbx; + + AppendSignatureList (Dbx, gEfiCertSha384Guid, 0, kSha384EntrySize, 1); + + HASH_ALGORITHM_SET Set; + + EXPECT_EQ ( + GetDatabaseHashAlgorithms (Db.data (), Db.size (), Dbx.data (), Dbx.size (), &Set), + EFI_SUCCESS + ); + EXPECT_EQ (Set.Count, 2u); +} + +TEST (GetDatabaseHashAlgorithmsTest, DuplicateAcrossDbAndDbx_Deduplicated) { + std::vector Db; + + AppendSignatureList (Db, gEfiCertSha256Guid, 0, kSha256EntrySize, 1); + + std::vector Dbx; + + AppendSignatureList (Dbx, gEfiCertSha256Guid, 0, kSha256EntrySize, 1); + + HASH_ALGORITHM_SET Set; + + EXPECT_EQ ( + GetDatabaseHashAlgorithms (Db.data (), Db.size (), Dbx.data (), Dbx.size (), &Set), + EFI_SUCCESS + ); + ASSERT_EQ (Set.Count, 1u); + EXPECT_EQ (CompareGuid (&Set.Guids[0], &gEfiCertSha256Guid), TRUE); +} + +TEST (GetDatabaseHashAlgorithmsTest, NonHashSignatureTypes_Ignored) { + std::vector Db; + + // X509 cert list is not a plain image hash; should not contribute. + AppendSignatureList (Db, gEfiCertX509Guid, 0, sizeof (EFI_GUID) + 16, 1); + + HASH_ALGORITHM_SET Set; + + EXPECT_EQ ( + GetDatabaseHashAlgorithms (Db.data (), Db.size (), NULL, 0, &Set), + EFI_SUCCESS + ); + EXPECT_EQ (Set.Count, 0u); +} + +TEST (GetDatabaseHashAlgorithmsTest, MalformedDb_ReturnsCorrupted) { + std::vector Db; + + AppendSignatureList (Db, gEfiCertSha256Guid, 0, kSha256EntrySize, 1); + // Corrupt the list size. + ((EFI_SIGNATURE_LIST *)Db.data ())->SignatureListSize = (UINT32)(Db.size () + 1); + + HASH_ALGORITHM_SET Set; + + EXPECT_EQ ( + GetDatabaseHashAlgorithms (Db.data (), Db.size (), NULL, 0, &Set), + EFI_VOLUME_CORRUPTED + ); +} + +TEST (GetDatabaseHashAlgorithmsTest, MalformedDbx_ReturnsCorrupted) { + // Db is well-formed; Dbx is corrupt. The walk over Dbx must surface + // the error rather than silently returning the partial Db result. + std::vector Db; + + AppendSignatureList (Db, gEfiCertSha256Guid, 0, kSha256EntrySize, 1); + + std::vector Dbx; + + AppendSignatureList (Dbx, gEfiCertSha384Guid, 0, kSha384EntrySize, 1); + ((EFI_SIGNATURE_LIST *)Dbx.data ())->SignatureListSize = (UINT32)(Dbx.size () + 1); + + HASH_ALGORITHM_SET Set; + + EXPECT_EQ ( + GetDatabaseHashAlgorithms (Db.data (), Db.size (), Dbx.data (), Dbx.size (), &Set), + EFI_VOLUME_CORRUPTED + ); +} + +TEST (GetDatabaseHashAlgorithmsTest, MultipleAlgorithmsInSingleBuffer_AllReported) { + // All four known image-hash algorithms concatenated into a single + // buffer must all be reported. + std::vector Db; + + AppendSignatureList (Db, gEfiCertSha1Guid, 0, sizeof (EFI_GUID) + 20, 1); + AppendSignatureList (Db, gEfiCertSha256Guid, 0, kSha256EntrySize, 1); + AppendSignatureList (Db, gEfiCertSha384Guid, 0, kSha384EntrySize, 1); + AppendSignatureList (Db, gEfiCertSha512Guid, 0, sizeof (EFI_GUID) + 64, 1); + + HASH_ALGORITHM_SET Set; + + EXPECT_EQ ( + GetDatabaseHashAlgorithms (Db.data (), Db.size (), NULL, 0, &Set), + EFI_SUCCESS + ); + ASSERT_EQ (Set.Count, 4u); + EXPECT_EQ (CompareGuid (&Set.Guids[0], &gEfiCertSha1Guid), TRUE); + EXPECT_EQ (CompareGuid (&Set.Guids[1], &gEfiCertSha256Guid), TRUE); + EXPECT_EQ (CompareGuid (&Set.Guids[2], &gEfiCertSha384Guid), TRUE); + EXPECT_EQ (CompareGuid (&Set.Guids[3], &gEfiCertSha512Guid), TRUE); +} + +// --------------------------------------------------------------------------- +// IsSignatureFoundInDatabase +// --------------------------------------------------------------------------- + +// +// Write Bytes into the entry payload (the part after the owner GUID) of +// signature index EntryIndex inside the EFI_SIGNATURE_LIST that begins +// at ListOffset within Buffer. +// +static void +SetEntryPayload ( + std::vector &Buffer, + size_t ListOffset, + UINTN EntryIndex, + const std::vector &Bytes + ) +{ + EFI_SIGNATURE_LIST *List = (EFI_SIGNATURE_LIST *)(Buffer.data () + ListOffset); + const size_t FirstEntry = ListOffset + sizeof (EFI_SIGNATURE_LIST) + List->SignatureHeaderSize; + const size_t EntryStart = FirstEntry + (size_t)EntryIndex * (size_t)List->SignatureSize; + const size_t PayloadOff = EntryStart + sizeof (EFI_GUID); + + ASSERT_LE (PayloadOff + Bytes.size (), Buffer.size ()); + std::memcpy (Buffer.data () + PayloadOff, Bytes.data (), Bytes.size ()); +} + +// SHA-256 digest payload size (no owner GUID). +static constexpr UINTN kSha256DigestSize = 32; +static constexpr UINTN kSha384DigestSize = 48; + +TEST (IsSignatureFoundInDatabaseTest, NullSignature_ReturnsInvalidParameter) { + BOOLEAN Found = FALSE; + + EXPECT_EQ ( + IsSignatureFoundInDatabase (NULL, 0, NULL, &gEfiCertSha256Guid, kSha256DigestSize, &Found), + EFI_INVALID_PARAMETER + ); +} + +TEST (IsSignatureFoundInDatabaseTest, NullSignatureType_ReturnsInvalidParameter) { + UINT8 Sig[kSha256DigestSize] = { 0 }; + BOOLEAN Found = FALSE; + + EXPECT_EQ ( + IsSignatureFoundInDatabase (NULL, 0, Sig, NULL, sizeof (Sig), &Found), + EFI_INVALID_PARAMETER + ); +} + +TEST (IsSignatureFoundInDatabaseTest, NullIsFound_ReturnsInvalidParameter) { + UINT8 Sig[kSha256DigestSize] = { 0 }; + + EXPECT_EQ ( + IsSignatureFoundInDatabase (NULL, 0, Sig, &gEfiCertSha256Guid, sizeof (Sig), NULL), + EFI_INVALID_PARAMETER + ); +} + +TEST (IsSignatureFoundInDatabaseTest, ZeroSignatureSize_ReturnsInvalidParameter) { + UINT8 Sig = 0; + BOOLEAN Found = FALSE; + + EXPECT_EQ ( + IsSignatureFoundInDatabase (NULL, 0, &Sig, &gEfiCertSha256Guid, 0, &Found), + EFI_INVALID_PARAMETER + ); +} + +TEST (IsSignatureFoundInDatabaseTest, NullDatabase_NotFoundSuccess) { + UINT8 Sig[kSha256DigestSize] = { 0xAB }; + BOOLEAN Found = TRUE; // pre-set to verify it gets cleared + + EXPECT_EQ ( + IsSignatureFoundInDatabase (NULL, 0, Sig, &gEfiCertSha256Guid, sizeof (Sig), &Found), + EFI_SUCCESS + ); + EXPECT_FALSE (Found); +} + +TEST (IsSignatureFoundInDatabaseTest, ExactMatch_Found) { + std::vector Db; + size_t Off = AppendSignatureList (Db, gEfiCertSha256Guid, 0, kSha256EntrySize, 2); + + std::vector Target (kSha256DigestSize, 0xAA); + + SetEntryPayload (Db, Off, 1, Target); + + BOOLEAN Found = FALSE; + + EXPECT_EQ ( + IsSignatureFoundInDatabase ( + Db.data (), + Db.size (), + Target.data (), + &gEfiCertSha256Guid, + Target.size (), + &Found + ), + EFI_SUCCESS + ); + EXPECT_TRUE (Found); +} + +TEST (IsSignatureFoundInDatabaseTest, NoMatchingEntry_NotFound) { + std::vector Db; + size_t Off = AppendSignatureList (Db, gEfiCertSha256Guid, 0, kSha256EntrySize, 1); + + std::vector Stored (kSha256DigestSize, 0xAA); + + SetEntryPayload (Db, Off, 0, Stored); + + std::vector Wanted (kSha256DigestSize, 0xBB); + BOOLEAN Found = FALSE; + + EXPECT_EQ ( + IsSignatureFoundInDatabase ( + Db.data (), + Db.size (), + Wanted.data (), + &gEfiCertSha256Guid, + Wanted.size (), + &Found + ), + EFI_SUCCESS + ); + EXPECT_FALSE (Found); +} + +TEST (IsSignatureFoundInDatabaseTest, MismatchedSignatureType_Skipped) { + // Database contains a SHA-384 entry whose payload bytes happen to + // match the search target; lookup with a SHA-256 type GUID must skip + // the SHA-384 list and report not-found. + std::vector Db; + size_t Off = AppendSignatureList (Db, gEfiCertSha384Guid, 0, kSha384EntrySize, 1); + + std::vector Stored (kSha384DigestSize, 0xAA); + + SetEntryPayload (Db, Off, 0, Stored); + + std::vector Wanted (kSha256DigestSize, 0xAA); + BOOLEAN Found = TRUE; + + EXPECT_EQ ( + IsSignatureFoundInDatabase ( + Db.data (), + Db.size (), + Wanted.data (), + &gEfiCertSha256Guid, + Wanted.size (), + &Found + ), + EFI_SUCCESS + ); + EXPECT_FALSE (Found); +} + +TEST (IsSignatureFoundInDatabaseTest, MismatchedSignatureSize_Skipped) { + // List type matches but per-entry size doesn't, so the list describes + // a different algorithm and must be skipped. + std::vector Db; + size_t Off = AppendSignatureList (Db, gEfiCertSha256Guid, 0, kSha384EntrySize, 1); + + std::vector Stored (kSha384DigestSize, 0xCC); + + SetEntryPayload (Db, Off, 0, Stored); + + std::vector Wanted (kSha256DigestSize, 0xCC); + BOOLEAN Found = TRUE; + + EXPECT_EQ ( + IsSignatureFoundInDatabase ( + Db.data (), + Db.size (), + Wanted.data (), + &gEfiCertSha256Guid, + Wanted.size (), + &Found + ), + EFI_SUCCESS + ); + EXPECT_FALSE (Found); +} + +TEST (IsSignatureFoundInDatabaseTest, MatchInSecondList_Found) { + // First list is the wrong algorithm, second list contains the target. + std::vector Db; + + AppendSignatureList (Db, gEfiCertSha384Guid, 0, kSha384EntrySize, 1); + size_t SecondOff = AppendSignatureList (Db, gEfiCertSha256Guid, 0, kSha256EntrySize, 2); + + std::vector Target (kSha256DigestSize, 0x77); + + SetEntryPayload (Db, SecondOff, 1, Target); + + BOOLEAN Found = FALSE; + + EXPECT_EQ ( + IsSignatureFoundInDatabase ( + Db.data (), + Db.size (), + Target.data (), + &gEfiCertSha256Guid, + Target.size (), + &Found + ), + EFI_SUCCESS + ); + EXPECT_TRUE (Found); +} + +TEST (IsSignatureFoundInDatabaseTest, NonZeroSignatureHeaderSize_EntryMathCorrect) { + // SignatureHeaderSize is non-zero: the per-list header occupies + // additional bytes between EFI_SIGNATURE_LIST and the first entry. + // A naive cursor that forgets to skip it would either miss the + // payload entirely or read the header bytes as a fake entry. + constexpr UINT32 kHeader = 8; + std::vector Db; + size_t Off = AppendSignatureList (Db, gEfiCertSha256Guid, kHeader, kSha256EntrySize, 2); + + // Fill the per-list header with a recognizable pattern so a math + // bug that read it as an entry would compare against this, not the + // real digest. The search target intentionally differs from it. + std::memset (Db.data () + Off + sizeof (EFI_SIGNATURE_LIST), 0xEE, kHeader); + + std::vector Target (kSha256DigestSize, 0x55); + + SetEntryPayload (Db, Off, 1, Target); + + BOOLEAN Found = FALSE; + + EXPECT_EQ ( + IsSignatureFoundInDatabase ( + Db.data (), + Db.size (), + Target.data (), + &gEfiCertSha256Guid, + Target.size (), + &Found + ), + EFI_SUCCESS + ); + EXPECT_TRUE (Found); +} + +TEST (IsSignatureFoundInDatabaseTest, ZeroEntryList_NotFound) { + // A well-formed list with zero entries must be skipped without a + // false positive (EntryCount == 0 means the inner loop never runs). + std::vector Db; + + AppendSignatureList (Db, gEfiCertSha256Guid, 0, kSha256EntrySize, 0); + + UINT8 Sig[kSha256DigestSize] = { 0 }; + BOOLEAN Found = TRUE; + + EXPECT_EQ ( + IsSignatureFoundInDatabase ( + Db.data (), + Db.size (), + Sig, + &gEfiCertSha256Guid, + sizeof (Sig), + &Found + ), + EFI_SUCCESS + ); + EXPECT_FALSE (Found); +} + +TEST (IsSignatureFoundInDatabaseTest, MalformedDb_ReturnsCorrupted) { + std::vector Db; + + AppendSignatureList (Db, gEfiCertSha256Guid, 0, kSha256EntrySize, 1); + ((EFI_SIGNATURE_LIST *)Db.data ())->SignatureListSize = (UINT32)(Db.size () + 1); + + UINT8 Sig[kSha256DigestSize] = { 0 }; + BOOLEAN Found = FALSE; + + EXPECT_EQ ( + IsSignatureFoundInDatabase ( + Db.data (), + Db.size (), + Sig, + &gEfiCertSha256Guid, + sizeof (Sig), + &Found + ), + EFI_VOLUME_CORRUPTED + ); +} + +// --------------------------------------------------------------------------- +// LoadSignatureDatabase (uses MockUefiLib::GetVariable2) +// --------------------------------------------------------------------------- + +class LoadSignatureDatabaseTest : public ::testing::Test { +protected: + MockUefiLib UefiLibMock; +}; + +TEST_F (LoadSignatureDatabaseTest, NullDatabaseName_ReturnsInvalidParameter) { + VOID *Buffer = NULL; + UINTN Size = 0; + + EXPECT_EQ ( + LoadSignatureDatabase (NULL, &Buffer, &Size), + EFI_INVALID_PARAMETER + ); +} + +TEST_F (LoadSignatureDatabaseTest, NullBuffer_ReturnsInvalidParameter) { + UINTN Size = 0; + + EXPECT_EQ ( + LoadSignatureDatabase ((const CHAR16 *)u"db", NULL, &Size), + EFI_INVALID_PARAMETER + ); +} + +TEST_F (LoadSignatureDatabaseTest, NullSize_ReturnsInvalidParameter) { + VOID *Buffer = NULL; + + EXPECT_EQ ( + LoadSignatureDatabase ((const CHAR16 *)u"db", &Buffer, NULL), + EFI_INVALID_PARAMETER + ); +} + +TEST_F (LoadSignatureDatabaseTest, VariableMissing_SuccessWithNullBuffer) { + // EFI_NOT_FOUND is normalized to EFI_SUCCESS with *Buffer == NULL. + EXPECT_CALL (UefiLibMock, GetVariable2 (_, _, _, _)) + .WillOnce (Return (EFI_NOT_FOUND)); + + VOID *Buffer = (VOID *)(UINTN)0xDEADBEEF; // pre-set: must be cleared + UINTN Size = 0xAA; + + EXPECT_EQ ( + LoadSignatureDatabase ((const CHAR16 *)u"db", &Buffer, &Size), + EFI_SUCCESS + ); + EXPECT_EQ (Buffer, (VOID *)NULL); + EXPECT_EQ (Size, 0u); +} + +TEST_F (LoadSignatureDatabaseTest, VariablePresent_BufferAndSizePopulated) { + static const UINT8 kPayload[] = { 0xAA, 0xBB, 0xCC, 0xDD }; + + EXPECT_CALL (UefiLibMock, GetVariable2 (_, _, _, _)) + .WillOnce ( + Invoke ( + [] ( + IN CONST CHAR16 *Name, + IN CONST EFI_GUID *Guid, + OUT VOID **Value, + OUT UINTN *Size + ) -> EFI_STATUS { + (VOID)Name; + (VOID)Guid; + *Value = AllocateCopyPool (sizeof (kPayload), kPayload); + *Size = sizeof (kPayload); + return EFI_SUCCESS; + } + ) + ); + + VOID *Buffer = NULL; + UINTN Size = 0; + + EXPECT_EQ ( + LoadSignatureDatabase ((const CHAR16 *)u"db", &Buffer, &Size), + EFI_SUCCESS + ); + ASSERT_NE (Buffer, (VOID *)NULL); + EXPECT_EQ (Size, sizeof (kPayload)); + EXPECT_EQ (CompareMem (Buffer, kPayload, sizeof (kPayload)), 0); + + FreePool (Buffer); +} + +TEST_F (LoadSignatureDatabaseTest, GetVariableUnexpectedError_PropagatedVerbatim) { + // Errors other than EFI_NOT_FOUND must be reported unchanged. + EXPECT_CALL (UefiLibMock, GetVariable2 (_, _, _, _)) + .WillOnce (Return (EFI_DEVICE_ERROR)); + + VOID *Buffer = NULL; + UINTN Size = 0; + + EXPECT_EQ ( + LoadSignatureDatabase ((const CHAR16 *)u"db", &Buffer, &Size), + EFI_DEVICE_ERROR + ); +} diff --git a/SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/DxeImageVerificationLibGoogleTest.cpp b/SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/DxeImageVerificationLibGoogleTest.cpp new file mode 100644 index 00000000000..1aca1d17fbb --- /dev/null +++ b/SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/DxeImageVerificationLibGoogleTest.cpp @@ -0,0 +1,28 @@ +/** @file + Unit tests for the implementation of DxeImageVerificationLib. + + Copyright (c) 2025, Yandex. All rights reserved. + SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#include +#include +#include +#include +#include + +extern "C" { + #include + #include + #include +} + +int +main ( + int argc, + char *argv[] + ) +{ + testing::InitGoogleTest (&argc, argv); + return RUN_ALL_TESTS (); +} diff --git a/SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/DxeImageVerificationLibGoogleTest.inf b/SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/DxeImageVerificationLibGoogleTest.inf new file mode 100644 index 00000000000..cc1c6f89996 --- /dev/null +++ b/SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/DxeImageVerificationLibGoogleTest.inf @@ -0,0 +1,82 @@ +## @file +# Unit test suite for the DxeImageVerificationLib using Google Test +# +# Copyright (c) Microsoft Corporation. +# SPDX-License-Identifier: BSD-2-Clause-Patent +## + +[Defines] + INF_VERSION = 0x00010005 + BASE_NAME = DxeImageVerificationLibGoogleTest + FILE_GUID = 18723239-55AA-4814-9B7A-874BAF719A65 + MODULE_TYPE = HOST_APPLICATION + VERSION_STRING = 1.0 + +# +# The following information is for reference only and not required by the build tools. +# +# VALID_ARCHITECTURES = IA32 X64 +# + +[Sources] + DatabaseGoogleTest.cpp + DxeImageVerificationLibGoogleTest.cpp + PolicyGoogleTest.cpp + SupportGoogleTest.cpp + +[Packages] + MdePkg/MdePkg.dec + MdeModulePkg/MdeModulePkg.dec + SecurityPkg/SecurityPkg.dec + UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec + CryptoPkg/CryptoPkg.dec + +[LibraryClasses] + DxeImageVerificationLib + GoogleTestLib + BaseCryptLib + DebugLib + SecureBootVariableLib + +[Guids] + ## SOMETIMES_CONSUMES ## Variable:L"SecureBoot" + gEfiGlobalVariableGuid + + ## SOMETIMES_CONSUMES ## Variable:L"DB" + ## SOMETIMES_CONSUMES ## Variable:L"DBX" + ## SOMETIMES_CONSUMES ## Variable:L"DBT" + ## PRODUCES ## SystemTable + ## CONSUMES ## SystemTable + gEfiImageSecurityDatabaseGuid + + ## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the signature. + ## SOMETIMES_PRODUCES ## GUID # Unique ID for the type of the signature. + gEfiCertSha1Guid + + ## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the signature. + ## 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 + + gEfiCertX509Guid ## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the signature. + gEfiCertX509Sha256Guid ## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the signature. + gEfiCertX509Sha384Guid ## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the signature. + gEfiCertX509Sha512Guid ## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the signature. + gEfiCertPkcs7Guid ## SOMETIMES_CONSUMES ## GUID # Unique ID for the type of the certificate. + +[Protocols] + gEfiFirmwareVolume2ProtocolGuid ## SOMETIMES_CONSUMES + gEfiBlockIoProtocolGuid ## SOMETIMES_CONSUMES + gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES + +[Pcd] + gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy ## SOMETIMES_CONSUMES + gEfiSecurityPkgTokenSpaceGuid.PcdRemovableMediaImageVerificationPolicy ## SOMETIMES_CONSUMES + gEfiSecurityPkgTokenSpaceGuid.PcdFixedMediaImageVerificationPolicy ## SOMETIMES_CONSUMES diff --git a/SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/PolicyGoogleTest.cpp b/SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/PolicyGoogleTest.cpp new file mode 100644 index 00000000000..ef220e2ffdc --- /dev/null +++ b/SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/PolicyGoogleTest.cpp @@ -0,0 +1,183 @@ +/** @file + Unit tests for the Image Verification Library policy resolution helpers. + Copyright (C) Microsoft Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#include +#include + +extern "C" { + #include + #include + #include + #include + #include + + #include "../Policy.h" +} + +using ::testing::_; +using ::testing::Return; + +// +// A minimal stand-in for an EFI_DEVICE_PATH_PROTOCOL. The address is the +// only thing the helpers care about because LocateDevicePath / +// OpenProtocol are fully mocked. +// +static EFI_DEVICE_PATH_PROTOCOL mDevicePath; + +class PolicyTest : public ::testing::Test { +protected: + MockUefiBootServicesTableLib BsMock; +}; + +// --------------------------------------------------------------------------- +// IsFromFv +// --------------------------------------------------------------------------- + +TEST_F (PolicyTest, IsFromFv_NullArg_ReturnsInvalidParameter) { + EXPECT_EQ (IsFromFv (NULL), EFI_INVALID_PARAMETER); +} + +TEST_F (PolicyTest, IsFromFv_LocateFails_ReturnsNotFound) { + EXPECT_CALL (BsMock, gBS_LocateDevicePath) + .WillOnce (Return (EFI_NOT_FOUND)); + + EXPECT_EQ (IsFromFv (&mDevicePath), EFI_NOT_FOUND); +} + +TEST_F (PolicyTest, IsFromFv_OpenFails_ReturnsNotFound) { + EXPECT_CALL (BsMock, gBS_LocateDevicePath) + .WillOnce (Return (EFI_SUCCESS)); + EXPECT_CALL (BsMock, gBS_OpenProtocol) + .WillOnce (Return (EFI_UNSUPPORTED)); + + EXPECT_EQ (IsFromFv (&mDevicePath), EFI_NOT_FOUND); +} + +TEST_F (PolicyTest, IsFromFv_Success_ReturnsSuccess) { + EXPECT_CALL (BsMock, gBS_LocateDevicePath) + .WillOnce (Return (EFI_SUCCESS)); + EXPECT_CALL (BsMock, gBS_OpenProtocol) + .WillOnce (Return (EFI_SUCCESS)); + + EXPECT_EQ (IsFromFv (&mDevicePath), EFI_SUCCESS); +} + +// --------------------------------------------------------------------------- +// GetImageType +// --------------------------------------------------------------------------- + +TEST_F (PolicyTest, GetImageType_NullArgs_ReturnsInvalidParameter) { + UINT32 ImageType = IMAGE_UNKNOWN; + + EXPECT_EQ (GetImageType (NULL, &ImageType), EFI_INVALID_PARAMETER); + EXPECT_EQ (GetImageType (&mDevicePath, NULL), EFI_INVALID_PARAMETER); +} + +TEST_F (PolicyTest, GetImageType_FirmwareVolume_ReturnsFv) { + UINT32 ImageType = IMAGE_UNKNOWN; + + EXPECT_CALL (BsMock, gBS_LocateDevicePath) + .WillOnce (Return (EFI_SUCCESS)); + EXPECT_CALL (BsMock, gBS_OpenProtocol) + .WillOnce (Return (EFI_SUCCESS)); + + EXPECT_EQ (GetImageType (&mDevicePath, &ImageType), EFI_SUCCESS); + EXPECT_EQ (ImageType, (UINT32)IMAGE_FROM_FV); +} + +TEST_F (PolicyTest, GetImageType_NotFv_ReturnsUnknown) { + UINT32 ImageType = IMAGE_FROM_FV; // Pre-populate to confirm reset. + + EXPECT_CALL (BsMock, gBS_LocateDevicePath) + .WillOnce (Return (EFI_NOT_FOUND)); + + EXPECT_EQ (GetImageType (&mDevicePath, &ImageType), EFI_SUCCESS); + EXPECT_EQ (ImageType, (UINT32)IMAGE_UNKNOWN); +} + +TEST_F (PolicyTest, GetImageType_LocateSucceedsOpenFails_ReturnsUnknown) { + // + // Exercises the OpenProtocol-failure branch of IsFromFv via the + // GetImageType orchestrator: LocateDevicePath resolves a handle but + // the protocol is not actually present. + // + UINT32 ImageType = IMAGE_FROM_FV; + + EXPECT_CALL (BsMock, gBS_LocateDevicePath) + .WillOnce (Return (EFI_SUCCESS)); + EXPECT_CALL (BsMock, gBS_OpenProtocol) + .WillOnce (Return (EFI_UNSUPPORTED)); + + EXPECT_EQ (GetImageType (&mDevicePath, &ImageType), EFI_SUCCESS); + EXPECT_EQ (ImageType, (UINT32)IMAGE_UNKNOWN); +} + +// --------------------------------------------------------------------------- +// GetPolicyForImageType +// --------------------------------------------------------------------------- + +TEST_F (PolicyTest, GetPolicyForImageType_Fv_ReturnsAlwaysExecute) { + EXPECT_EQ (GetPolicyForImageType (IMAGE_FROM_FV), (UINT32)ALWAYS_EXECUTE); +} + +TEST_F (PolicyTest, GetPolicyForImageType_Unknown_FailsClosed) { + EXPECT_EQ ( + GetPolicyForImageType (IMAGE_UNKNOWN), + (UINT32)DENY_EXECUTE_ON_SECURITY_VIOLATION + ); +} + +TEST_F (PolicyTest, GetPolicyForImageType_OtherValue_FailsClosed) { + EXPECT_EQ ( + GetPolicyForImageType (0xDEADBEEF), + (UINT32)DENY_EXECUTE_ON_SECURITY_VIOLATION + ); +} + +// --------------------------------------------------------------------------- +// GetExecutionPolicy +// --------------------------------------------------------------------------- + +TEST_F (PolicyTest, GetExecutionPolicy_NullArgs_ReturnsInvalidParameter) { + UINT32 Policy = ALWAYS_EXECUTE; + + EXPECT_EQ (GetExecutionPolicy (NULL, &Policy), EFI_INVALID_PARAMETER); + EXPECT_EQ (GetExecutionPolicy (&mDevicePath, NULL), EFI_INVALID_PARAMETER); +} + +TEST_F (PolicyTest, GetExecutionPolicy_FirmwareVolume_ReturnsAlwaysExecute) { + UINT32 Policy = DENY_EXECUTE_ON_SECURITY_VIOLATION; + + EXPECT_CALL (BsMock, gBS_LocateDevicePath) + .WillOnce (Return (EFI_SUCCESS)); + EXPECT_CALL (BsMock, gBS_OpenProtocol) + .WillOnce (Return (EFI_SUCCESS)); + + EXPECT_EQ (GetExecutionPolicy (&mDevicePath, &Policy), EFI_SUCCESS); + EXPECT_EQ (Policy, (UINT32)ALWAYS_EXECUTE); +} + +TEST_F (PolicyTest, GetExecutionPolicy_NonFv_FailsClosed) { + UINT32 Policy = ALWAYS_EXECUTE; + + EXPECT_CALL (BsMock, gBS_LocateDevicePath) + .WillOnce (Return (EFI_NOT_FOUND)); + + EXPECT_EQ (GetExecutionPolicy (&mDevicePath, &Policy), EFI_SUCCESS); + EXPECT_EQ (Policy, (UINT32)DENY_EXECUTE_ON_SECURITY_VIOLATION); +} + +TEST_F (PolicyTest, GetExecutionPolicy_LocateSucceedsOpenFails_FailsClosed) { + UINT32 Policy = ALWAYS_EXECUTE; + + EXPECT_CALL (BsMock, gBS_LocateDevicePath) + .WillOnce (Return (EFI_SUCCESS)); + EXPECT_CALL (BsMock, gBS_OpenProtocol) + .WillOnce (Return (EFI_ACCESS_DENIED)); + + EXPECT_EQ (GetExecutionPolicy (&mDevicePath, &Policy), EFI_SUCCESS); + EXPECT_EQ (Policy, (UINT32)DENY_EXECUTE_ON_SECURITY_VIOLATION); +} diff --git a/SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/SupportGoogleTest.cpp b/SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/SupportGoogleTest.cpp new file mode 100644 index 00000000000..79ce2d30ee3 --- /dev/null +++ b/SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/SupportGoogleTest.cpp @@ -0,0 +1,271 @@ +/** @file + Unit tests for GetImageSecurityDataDirectory in the + DxeImageVerificationLib. + These tests construct minimal-but-valid PE/COFF images in memory and + exercise the real PeCoffLib through GetImageSecurityDataDirectory so + that the data-directory callback path is covered end-to-end. + Copyright (C) Microsoft Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#include + +#include +#include + +extern "C" { + #include + #include + #include + #include "../Support.h" +} + +// +// Layout constants for the synthetic PE32+ image. +// +// The buffer is a single contiguous blob laid out as: +// [DOS hdr][NT64 hdrs][1 section hdr][padding to SizeOfHeaders][image body] +// +// All sizes are picked to satisfy PeCoffLoaderGetImageInfo's +// consistency checks (NumberOfRvaAndSizes, SizeOfOptionalHeader, +// SectionHeaderOffset, SizeOfHeaders < SizeOfImage, last-byte reads of +// SizeOfHeaders and the security directory). +// +static constexpr UINT32 kDosHdrSize = sizeof (EFI_IMAGE_DOS_HEADER); +static constexpr UINT32 kNt64Size = sizeof (EFI_IMAGE_NT_HEADERS64); +static constexpr UINT32 kPeCoffOffset = kDosHdrSize; +static constexpr UINT32 kSizeOfHeaders = 0x200; +static constexpr UINT32 kSizeOfImage = 0x400; +static constexpr UINT32 kSecDirVa = 0x300; +static constexpr UINT32 kSecDirSize = 0x100; + +/** + Build a minimal valid PE32+ image in heap-backed storage. + When IncludeSecurityDir is true, the returned image's + EFI_IMAGE_DIRECTORY_ENTRY_SECURITY entry points at [kSecDirVa, + kSecDirVa + kSecDirSize). Otherwise that entry is left zero. +**/ +static std::vector +BuildPe32PlusImage ( + bool IncludeSecurityDir + ) +{ + std::vector Image (kSizeOfImage, 0); + + EFI_IMAGE_DOS_HEADER *Dos = (EFI_IMAGE_DOS_HEADER *)Image.data (); + EFI_IMAGE_NT_HEADERS64 *Nt = (EFI_IMAGE_NT_HEADERS64 *)(Image.data () + kPeCoffOffset); + + Dos->e_magic = EFI_IMAGE_DOS_SIGNATURE; + Dos->e_lfanew = kPeCoffOffset; + + Nt->Signature = EFI_IMAGE_NT_SIGNATURE; + Nt->FileHeader.Machine = IMAGE_FILE_MACHINE_X64; + Nt->FileHeader.NumberOfSections = 1; + Nt->FileHeader.SizeOfOptionalHeader = sizeof (EFI_IMAGE_OPTIONAL_HEADER64); + Nt->OptionalHeader.Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC; + Nt->OptionalHeader.SizeOfImage = kSizeOfImage; + Nt->OptionalHeader.SizeOfHeaders = kSizeOfHeaders; + Nt->OptionalHeader.SectionAlignment = 0x200; + Nt->OptionalHeader.NumberOfRvaAndSizes = EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES; + + if (IncludeSecurityDir) { + Nt->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY].VirtualAddress = kSecDirVa; + Nt->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY].Size = kSecDirSize; + } + + return Image; +} + +class GetImageSecurityDataDirectoryTest : public ::testing::Test { +}; + +// --------------------------------------------------------------------------- +// Argument validation +// --------------------------------------------------------------------------- + +TEST_F (GetImageSecurityDataDirectoryTest, NullFileBuffer_ReturnsInvalidParameter) { + EFI_IMAGE_DATA_DIRECTORY SecDataDir; + + EXPECT_EQ ( + GetImageSecurityDataDirectory (NULL, 0x100, &SecDataDir), + EFI_INVALID_PARAMETER + ); +} + +TEST_F (GetImageSecurityDataDirectoryTest, NullSecDataDir_ReturnsInvalidParameter) { + std::vector Image = BuildPe32PlusImage (false); + + EXPECT_EQ ( + GetImageSecurityDataDirectory (Image.data (), Image.size (), NULL), + EFI_INVALID_PARAMETER + ); +} + +TEST_F (GetImageSecurityDataDirectoryTest, NullFileBuffer_DoesNotTouchSecDataDir) { + // + // Caller-supplied SecDataDir must be untouched on the early + // invalid-parameter return; only the success path is allowed to + // overwrite it. + // + EFI_IMAGE_DATA_DIRECTORY SecDataDir; + + SetMem (&SecDataDir, sizeof (SecDataDir), 0xAA); + + EXPECT_EQ ( + GetImageSecurityDataDirectory (NULL, 0x100, &SecDataDir), + EFI_INVALID_PARAMETER + ); + EXPECT_EQ (SecDataDir.VirtualAddress, 0xAAAAAAAAu); + EXPECT_EQ (SecDataDir.Size, 0xAAAAAAAAu); +} + +// --------------------------------------------------------------------------- +// Malformed images +// --------------------------------------------------------------------------- + +TEST_F (GetImageSecurityDataDirectoryTest, GarbageBuffer_ReturnsLoadError) { + // + // A buffer that is large enough to attempt header parsing but does + // not actually carry valid DOS/NT signatures must be rejected. + // + std::vector Buffer (kSizeOfImage, 0xAB); + EFI_IMAGE_DATA_DIRECTORY SecDataDir; + + EXPECT_EQ ( + GetImageSecurityDataDirectory (Buffer.data (), Buffer.size (), &SecDataDir), + EFI_LOAD_ERROR + ); +} + +TEST_F (GetImageSecurityDataDirectoryTest, TinyBuffer_ReturnsLoadError) { + // + // A buffer too small to contain a DOS header must be rejected + // without dereferencing any of the bytes. + // + UINT8 TinyBuffer[4] = { 0 }; + EFI_IMAGE_DATA_DIRECTORY SecDataDir; + + EXPECT_EQ ( + GetImageSecurityDataDirectory (TinyBuffer, sizeof (TinyBuffer), &SecDataDir), + EFI_LOAD_ERROR + ); +} + +TEST_F (GetImageSecurityDataDirectoryTest, ZeroFileSize_ReturnsLoadError) { + UINT8 Byte = 0; + EFI_IMAGE_DATA_DIRECTORY SecDataDir; + + EXPECT_EQ ( + GetImageSecurityDataDirectory (&Byte, 0, &SecDataDir), + EFI_LOAD_ERROR + ); +} + +TEST_F (GetImageSecurityDataDirectoryTest, ElfanewPastEof_ReturnsLoadError) { + // + // Valid DOS signature but e_lfanew points well past the end of the + // buffer. The bounded reader must refuse to fetch the NT headers. + // + std::vector Image = BuildPe32PlusImage (false); + EFI_IMAGE_DOS_HEADER *Dos = (EFI_IMAGE_DOS_HEADER *)Image.data (); + + Dos->e_lfanew = (UINT32)(Image.size () + 0x1000); + + EFI_IMAGE_DATA_DIRECTORY SecDataDir; + + EXPECT_EQ ( + GetImageSecurityDataDirectory (Image.data (), Image.size (), &SecDataDir), + EFI_LOAD_ERROR + ); +} + +TEST_F (GetImageSecurityDataDirectoryTest, TruncatedAtNtHeaders_ReturnsLoadError) { + // + // Buffer is truncated to the DOS header only, so the read of the NT + // headers at e_lfanew straddles EOF. + // + std::vector Image = BuildPe32PlusImage (false); + + Image.resize (sizeof (EFI_IMAGE_DOS_HEADER)); + + EFI_IMAGE_DATA_DIRECTORY SecDataDir; + + EXPECT_EQ ( + GetImageSecurityDataDirectory (Image.data (), Image.size (), &SecDataDir), + EFI_LOAD_ERROR + ); +} + +TEST_F (GetImageSecurityDataDirectoryTest, ValidDosBadNtSignature_ReturnsLoadError) { + // + // Valid DOS header but corrupted NT signature -- exercises the NT + // signature validation path inside PeCoffLib. + // + std::vector Image = BuildPe32PlusImage (false); + EFI_IMAGE_NT_HEADERS64 *Nt = (EFI_IMAGE_NT_HEADERS64 *)(Image.data () + kPeCoffOffset); + + Nt->Signature = 0xDEADBEEF; + + EFI_IMAGE_DATA_DIRECTORY SecDataDir; + + EXPECT_EQ ( + GetImageSecurityDataDirectory (Image.data (), Image.size (), &SecDataDir), + EFI_LOAD_ERROR + ); +} + +// --------------------------------------------------------------------------- +// Happy paths exercised through real PeCoffLib + the data-directory callback +// --------------------------------------------------------------------------- + +TEST_F (GetImageSecurityDataDirectoryTest, ValidImageWithoutSecurityDir_ReturnsZeroedDir) { + std::vector Image = BuildPe32PlusImage (false); + EFI_IMAGE_DATA_DIRECTORY SecDataDir; + + // + // Pre-poison SecDataDir so we can prove the function actually wrote + // back zeros (rather than leaving stale caller-supplied bytes). + // + SetMem (&SecDataDir, sizeof (SecDataDir), 0xCD); + + EXPECT_EQ ( + GetImageSecurityDataDirectory (Image.data (), Image.size (), &SecDataDir), + EFI_SUCCESS + ); + EXPECT_EQ (SecDataDir.VirtualAddress, 0u); + EXPECT_EQ (SecDataDir.Size, 0u); +} + +TEST_F (GetImageSecurityDataDirectoryTest, ValidImageWithSecurityDir_ReturnsCapturedDir) { + std::vector Image = BuildPe32PlusImage (true); + EFI_IMAGE_DATA_DIRECTORY SecDataDir; + + EXPECT_EQ ( + GetImageSecurityDataDirectory (Image.data (), Image.size (), &SecDataDir), + EFI_SUCCESS + ); + EXPECT_EQ (SecDataDir.VirtualAddress, kSecDirVa); + EXPECT_EQ (SecDataDir.Size, kSecDirSize); +} + +TEST_F (GetImageSecurityDataDirectoryTest, OtherDataDirectoriesDoNotLeak) { + // + // Populate a non-SECURITY data directory entry (export table) with + // a recognizable value to prove the callback only captures the + // SECURITY index and ignores every other directory PeCoffLib walks. + // + std::vector Image = BuildPe32PlusImage (true); + EFI_IMAGE_NT_HEADERS64 *Nt = (EFI_IMAGE_NT_HEADERS64 *)(Image.data () + kPeCoffOffset); + + Nt->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_EXPORT].VirtualAddress = 0xCAFEBABE; + Nt->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_EXPORT].Size = 0xBEEFu; + + EFI_IMAGE_DATA_DIRECTORY SecDataDir; + + EXPECT_EQ ( + GetImageSecurityDataDirectory (Image.data (), Image.size (), &SecDataDir), + EFI_SUCCESS + ); + EXPECT_EQ (SecDataDir.VirtualAddress, kSecDirVa); + EXPECT_EQ (SecDataDir.Size, kSecDirSize); +} diff --git a/SecurityPkg/Library/DxeImageVerificationLib2/Policy.c b/SecurityPkg/Library/DxeImageVerificationLib2/Policy.c new file mode 100644 index 00000000000..b38a938b77a --- /dev/null +++ b/SecurityPkg/Library/DxeImageVerificationLib2/Policy.c @@ -0,0 +1,148 @@ +/** @file + Image verification policy helpers for the DXE Image Verification Library. + + Data types, image source classifications, and APIs that resolve an + EFI_DEVICE_PATH_PROTOCOL into an authorization policy that the verification + handler should apply. The current policy is intentionally minimal: images + loaded from a Firmware Volume are always allowed to execute; everything else is + denied if validation fails. + + Copyright (C) Microsoft Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#include "Policy.h" + +/** + Determine whether the given device path resolves to a Firmware Volume. + + @param[in] File Device path describing the image origin. + @param[out] ImageType On success, set to IMAGE_FROM_FV. + + @retval EFI_SUCCESS The image is from a Firmware Volume. + @retval EFI_NOT_FOUND The image is not from a Firmware Volume. + @retval EFI_INVALID_PARAMETER File is NULL. +**/ +EFI_STATUS +IsFromFv ( + IN CONST EFI_DEVICE_PATH_PROTOCOL *File + ) +{ + EFI_STATUS Status; + EFI_HANDLE DeviceHandle; + EFI_DEVICE_PATH_PROTOCOL *TempDevicePath; + + if (File == NULL) { + return EFI_INVALID_PARAMETER; + } + + DeviceHandle = NULL; + TempDevicePath = (EFI_DEVICE_PATH_PROTOCOL *)File; + Status = gBS->LocateDevicePath ( + &gEfiFirmwareVolume2ProtocolGuid, + &TempDevicePath, + &DeviceHandle + ); + if (EFI_ERROR (Status)) { + return EFI_NOT_FOUND; + } + + // + // Confirm the protocol is actually present on the resolved handle. + // + Status = gBS->OpenProtocol ( + DeviceHandle, + &gEfiFirmwareVolume2ProtocolGuid, + NULL, + NULL, + NULL, + EFI_OPEN_PROTOCOL_TEST_PROTOCOL + ); + if (EFI_ERROR (Status)) { + return EFI_NOT_FOUND; + } + + return EFI_SUCCESS; +} + +/** + Determines the Image type classification. + + @param[in] File Device path describing the image origin. + @param[out] ImageType The classification of the image source. + + @retval EFI_SUCCESS ImageType contains a valid value. + @retval EFI_INVALID_PARAMETER File or ImageType is NULL. +**/ +EFI_STATUS +GetImageType ( + IN CONST EFI_DEVICE_PATH_PROTOCOL *File, + OUT UINT32 *ImageType + ) +{ + if ((File == NULL) || (ImageType == NULL)) { + return EFI_INVALID_PARAMETER; + } + + if (!EFI_ERROR (IsFromFv (File))) { + *ImageType = IMAGE_FROM_FV; + return EFI_SUCCESS; + } + + *ImageType = IMAGE_UNKNOWN; + return EFI_SUCCESS; +} + +/** + Look up the configured authorization policy for the given image source. + + IMAGE_FROM_FV is mapped to ALWAYS_EXECUTE; all other image types map to + DENY_EXECUTE_ON_SECURITY_VIOLATION. + + @param[in] ImageType An IMAGE_* image source classification value. + + @return ALWAYS_EXECUTE or DENY_EXECUTE_ON_SECURITY_VIOLATION. +**/ +UINT32 +GetPolicyForImageType ( + IN UINT32 ImageType + ) +{ + if (ImageType == IMAGE_FROM_FV) { + return ALWAYS_EXECUTE; + } + + return DENY_EXECUTE_ON_SECURITY_VIOLATION; +} + +/** + Resolve an image's authorization policy. + + @param[in] File Device path describing the image origin. + @param[out] Policy On success, filled with the resolved policy value. + + @retval EFI_SUCCESS Policy contains a valid policy value. + @retval EFI_INVALID_PARAMETER File or Policy is NULL. +**/ +EFI_STATUS +GetExecutionPolicy ( + IN CONST EFI_DEVICE_PATH_PROTOCOL *File, + OUT UINT32 *Policy + ) +{ + EFI_STATUS Status; + UINT32 ImageType; + + if ((File == NULL) || (Policy == NULL)) { + return EFI_INVALID_PARAMETER; + } + + Status = GetImageType (File, &ImageType); + if (EFI_ERROR (Status)) { + return Status; + } + + *Policy = GetPolicyForImageType (ImageType); + + return EFI_SUCCESS; +} diff --git a/SecurityPkg/Library/DxeImageVerificationLib2/Policy.h b/SecurityPkg/Library/DxeImageVerificationLib2/Policy.h new file mode 100644 index 00000000000..b3b629e4605 --- /dev/null +++ b/SecurityPkg/Library/DxeImageVerificationLib2/Policy.h @@ -0,0 +1,94 @@ +/** @file + Image verification policy helpers for the DXE Image Verification Library. + + Data types, image source classifications, and APIs that resolve an + EFI_DEVICE_PATH_PROTOCOL into an authorization policy that the verification + handler should apply. The current policy is intentionally minimal: images + loaded from a Firmware Volume are always allowed to execute; everything else is + denied if validation fails. + + Copyright (C) Microsoft Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#ifndef DXE_IMAGE_VERIFICATION_LIB_POLICY_H_ +#define DXE_IMAGE_VERIFICATION_LIB_POLICY_H_ + +#include "DxeImageVerificationLib.h" +#include +#include +#include + +// +// Authorization policy bit definition +// +#define ALWAYS_EXECUTE 0x00000000 +#define DENY_EXECUTE_ON_SECURITY_VIOLATION 0x00000001 + +// +// Image type definitions +// +#define IMAGE_UNKNOWN 0x00000000 +#define IMAGE_FROM_FV 0x00000001 + +/** + Determine whether the given device path resolves to a Firmware Volume. + + @param[in] File Device path describing the image origin. + @param[out] ImageType On success, set to IMAGE_FROM_FV. + + @retval EFI_SUCCESS The image is from a Firmware Volume. + @retval EFI_NOT_FOUND The image is not from a Firmware Volume. + @retval EFI_INVALID_PARAMETER File is NULL. +**/ +EFI_STATUS +IsFromFv ( + IN CONST EFI_DEVICE_PATH_PROTOCOL *File + ); + +/** + Determines the Image type classification. + + @param[in] File Device path describing the image origin. + @param[out] ImageType The classification of the image source. + + @retval EFI_SUCCESS ImageType contains a valid value. + @retval EFI_INVALID_PARAMETER File or ImageType is NULL. +**/ +EFI_STATUS +GetImageType ( + IN CONST EFI_DEVICE_PATH_PROTOCOL *File, + OUT UINT32 *ImageType + ); + +/** + Look up the configured authorization policy for the given image source. + + IMAGE_FROM_FV is mapped to ALWAYS_EXECUTE; all other image types map to + DENY_EXECUTE_ON_SECURITY_VIOLATION. + + @param[in] ImageType An IMAGE_* image source classification value. + + @return ALWAYS_EXECUTE or DENY_EXECUTE_ON_SECURITY_VIOLATION. +**/ +UINT32 +GetPolicyForImageType ( + IN UINT32 ImageType + ); + +/** + Resolve an image's authorization policy. + + @param[in] File Device path describing the image origin. + @param[out] Policy On success, filled with the resolved policy value. + + @retval EFI_SUCCESS Policy contains a valid policy value. + @retval EFI_INVALID_PARAMETER File or Policy is NULL. +**/ +EFI_STATUS +GetExecutionPolicy ( + IN CONST EFI_DEVICE_PATH_PROTOCOL *File, + OUT UINT32 *Policy + ); + +#endif // DXE_IMAGE_VERIFICATION_LIB_POLICY_H_ diff --git a/SecurityPkg/Library/DxeImageVerificationLib2/Support.c b/SecurityPkg/Library/DxeImageVerificationLib2/Support.c new file mode 100644 index 00000000000..06c3a5190d0 --- /dev/null +++ b/SecurityPkg/Library/DxeImageVerificationLib2/Support.c @@ -0,0 +1,167 @@ +/** @file + Utility functions for DxeImageVerificationLib. + + Copyright (C) Microsoft Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#include "Support.h" + +// +// Caller-owned state passed through PeCoffLoaderGetImageInfo() to +// SecurityDirectoryCallback() and back. +// +typedef struct { + EFI_IMAGE_DATA_DIRECTORY SecDataDir; +} SECURITY_DIR_CALLBACK_CTX; + +// +// Image Handle that contains the image bytes and size. +// +typedef struct { + CONST UINT8 *Base; + UINTN Size; +} PE_COFF_IMAGE_HANDLE; + +/** + Buffer size aware implementation of PE_COFF_LOADER_READ_FILE. + + @param[in] FileHandle Pointer to a PE_COFF_IMAGE_HANDLE describing + the image bytes available to PeCoffLib. + @param[in] FileOffset Byte offset within the image to read from. + @param[in,out] ReadSize On input, bytes requested. On output, bytes + actually copied (may be 0 or less than + requested if the range extends past EOF). + @param[out] Buffer Destination buffer of at least the input + *ReadSize bytes. + + @retval RETURN_SUCCESS Read succeeded; *ReadSize is the + number of bytes copied. + @retval RETURN_INVALID_PARAMETER FileHandle, ReadSize, or Buffer was + NULL. +**/ +STATIC +RETURN_STATUS +EFIAPI +BoundedImageRead ( + IN VOID *FileHandle, + IN UINTN FileOffset, + IN OUT UINTN *ReadSize, + OUT VOID *Buffer + ) +{ + CONST PE_COFF_IMAGE_HANDLE *Handle; + UINTN Available; + + if ((FileHandle == NULL) || (ReadSize == NULL) || (Buffer == NULL)) { + return RETURN_INVALID_PARAMETER; + } + + Handle = (CONST PE_COFF_IMAGE_HANDLE *)FileHandle; + + if (FileOffset >= Handle->Size) { + *ReadSize = 0; + return RETURN_SUCCESS; + } + + Available = Handle->Size - FileOffset; + if (*ReadSize > Available) { + *ReadSize = Available; + } + + CopyMem (Buffer, Handle->Base + FileOffset, *ReadSize); + return RETURN_SUCCESS; +} + +/** + PE_COFF_LOADER_DATA_DIRECTORY_CALLBACK that captures the EFI_IMAGE_DATA_DIRECTORY + for the EFI_IMAGE_DIRECTORY_ENTRY_SECURITY data directory. + + @param[in] Index Data directory. + @param[in] DataDirectory The directory entry. + @param[out] CallbackContext Pointer to a SECURITY_DIR_CALLBACK_CTX. + + @retval RETURN_SUCCESS Always. +**/ +STATIC +RETURN_STATUS +EFIAPI +SecurityDirectoryCallback ( + IN UINT32 Index, + IN CONST EFI_IMAGE_DATA_DIRECTORY *DataDirectory, + OUT VOID *CallbackContext OPTIONAL + ) +{ + SECURITY_DIR_CALLBACK_CTX *Ctx; + + if (Index != EFI_IMAGE_DIRECTORY_ENTRY_SECURITY) { + return RETURN_SUCCESS; + } + + Ctx = (SECURITY_DIR_CALLBACK_CTX *)CallbackContext; + Ctx->SecDataDir = *DataDirectory; + return RETURN_SUCCESS; +} + +/** + Locate the EFI_IMAGE_DIRECTORY_ENTRY_SECURITY data directory in the + PE/COFF image contained in FileBuffer. + + Caution: This function may receive untrusted input. The PE/COFF image + is external input and is bounds-checked by PeCoffLib before any field + is dereferenced. + + @param[in] FileBuffer Pointer to the in-memory PE/COFF image. + @param[in] FileSize Size of FileBuffer in bytes. + @param[out] SecDataDir On success, filled with a copy of the image's + security data directory entry. Zeroed when + the image declares no security directory. + + @retval EFI_SUCCESS SecDataDir has been populated. + @retval EFI_INVALID_PARAMETER FileBuffer or SecDataDir is NULL. + @retval EFI_LOAD_ERROR FileBuffer does not contain a valid + PE/COFF image, or PeCoffLib otherwise + rejected the headers. +**/ +EFI_STATUS +GetImageSecurityDataDirectory ( + IN VOID *FileBuffer, + IN UINTN FileSize, + OUT EFI_IMAGE_DATA_DIRECTORY *SecDataDir + ) +{ + RETURN_STATUS PeCoffStatus; + PE_COFF_LOADER_IMAGE_CONTEXT ImageContext; + SECURITY_DIR_CALLBACK_CTX CallbackCtx; + PE_COFF_IMAGE_HANDLE Handle; + + if ((FileBuffer == NULL) || (SecDataDir == NULL)) { + return EFI_INVALID_PARAMETER; + } + + ZeroMem (SecDataDir, sizeof (*SecDataDir)); + ZeroMem (&CallbackCtx, sizeof (CallbackCtx)); + + Handle.Base = (CONST UINT8 *)FileBuffer; + Handle.Size = FileSize; + + // + // Delegate header parsing, signature validation, optional-header + // magic checks, and security-directory bounds checking to PeCoffLib. + // The callback receives the validated security data directory entry. + // + ZeroMem (&ImageContext, sizeof (ImageContext)); + ImageContext.Handle = &Handle; + ImageContext.ImageRead = BoundedImageRead; + ImageContext.DataDirectoryRead = SecurityDirectoryCallback; + ImageContext.DataDirectoryReadContext = &CallbackCtx; + + PeCoffStatus = PeCoffLoaderGetImageInfo (&ImageContext); + if (RETURN_ERROR (PeCoffStatus)) { + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: PeImage invalid (0x%lx).\n", (UINT64)PeCoffStatus)); + return EFI_LOAD_ERROR; + } + + *SecDataDir = CallbackCtx.SecDataDir; + return EFI_SUCCESS; +} diff --git a/SecurityPkg/Library/DxeImageVerificationLib2/Support.h b/SecurityPkg/Library/DxeImageVerificationLib2/Support.h new file mode 100644 index 00000000000..b87faf0f476 --- /dev/null +++ b/SecurityPkg/Library/DxeImageVerificationLib2/Support.h @@ -0,0 +1,41 @@ +/** @file + Utility functions for DxeImageVerificationLib. + + Copyright (C) Microsoft Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#ifndef DXE_IMAGE_VERIFICATION_LIB_SUPPORT_H_ +#define DXE_IMAGE_VERIFICATION_LIB_SUPPORT_H_ + +#include "DxeImageVerificationLib.h" +#include + +/** + Locate the EFI_IMAGE_DIRECTORY_ENTRY_SECURITY data directory in the + PE/COFF image contained in FileBuffer. + + Caution: This function may receive untrusted input. The PE/COFF image + is external input and is bounds-checked by PeCoffLib before any field + is dereferenced. + + @param[in] FileBuffer Pointer to the in-memory PE/COFF image. + @param[in] FileSize Size of FileBuffer in bytes. + @param[out] SecDataDir On success, filled with a copy of the image's + security data directory entry. Zeroed when + the image declares no security directory. + + @retval EFI_SUCCESS SecDataDir has been populated. + @retval EFI_INVALID_PARAMETER FileBuffer or SecDataDir is NULL. + @retval EFI_LOAD_ERROR FileBuffer does not contain a valid + PE/COFF image, or PeCoffLib otherwise + rejected the headers. +**/ +EFI_STATUS +GetImageSecurityDataDirectory ( + IN VOID *FileBuffer, + IN UINTN FileSize, + OUT EFI_IMAGE_DATA_DIRECTORY *SecDataDir + ); + +#endif // DXE_IMAGE_VERIFICATION_LIB_SUPPORT_H_ diff --git a/SecurityPkg/SecurityPkg.dsc b/SecurityPkg/SecurityPkg.dsc index 82dd0be714c..c44b19112cf 100644 --- a/SecurityPkg/SecurityPkg.dsc +++ b/SecurityPkg/SecurityPkg.dsc @@ -206,6 +206,7 @@ [Components] SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf + SecurityPkg/Library/DxeImageVerificationLib2/DxeImageVerificationLib.inf SecurityPkg/Library/DxeImageAuthenticationStatusLib/DxeImageAuthenticationStatusLib.inf # diff --git a/SecurityPkg/Test/Mock/Include/GoogleTest/Library/MockSecureBootVariableLib.h b/SecurityPkg/Test/Mock/Include/GoogleTest/Library/MockSecureBootVariableLib.h new file mode 100644 index 00000000000..07c05bdecc8 --- /dev/null +++ b/SecurityPkg/Test/Mock/Include/GoogleTest/Library/MockSecureBootVariableLib.h @@ -0,0 +1,30 @@ +/** @file + Google Test mocks for SecureBootVariableLib + + Copyright (C) Microsoft Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#ifndef MOCK_SECURE_BOOT_VARIABLE_LIB_H_ +#define MOCK_SECURE_BOOT_VARIABLE_LIB_H_ + +#include +#include +extern "C" { + #include + #include + #include + #include +} + +struct MockSecureBootVariableLib { + MOCK_INTERFACE_DECLARATION (MockSecureBootVariableLib); + + MOCK_FUNCTION_DECLARATION ( + BOOLEAN, + IsSecureBootEnabled, + () + ); +}; + +#endif diff --git a/SecurityPkg/Test/Mock/Library/GoogleTest/MockSecureBootVariableLib/MockSecureBootVariableLib.cpp b/SecurityPkg/Test/Mock/Library/GoogleTest/MockSecureBootVariableLib/MockSecureBootVariableLib.cpp new file mode 100644 index 00000000000..7cecd3eea8a --- /dev/null +++ b/SecurityPkg/Test/Mock/Library/GoogleTest/MockSecureBootVariableLib/MockSecureBootVariableLib.cpp @@ -0,0 +1,11 @@ +/** @file + Google Test mocks for SecureBootVariableLib + + Copyright (C) Microsoft Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent +**/ +#include + +MOCK_INTERFACE_DEFINITION (MockSecureBootVariableLib); + +MOCK_FUNCTION_DEFINITION (MockSecureBootVariableLib, IsSecureBootEnabled, 0, EFIAPI); diff --git a/SecurityPkg/Test/Mock/Library/GoogleTest/MockSecureBootVariableLib/MockSecureBootVariableLib.inf b/SecurityPkg/Test/Mock/Library/GoogleTest/MockSecureBootVariableLib/MockSecureBootVariableLib.inf new file mode 100644 index 00000000000..a01ec938f10 --- /dev/null +++ b/SecurityPkg/Test/Mock/Library/GoogleTest/MockSecureBootVariableLib/MockSecureBootVariableLib.inf @@ -0,0 +1,34 @@ +## @file +# Google Test mocks for SecureBootVariableLib +# +# Copyright (C) Microsoft Corporation. All rights reserved.
+# SPDX-License-Identifier: BSD-2-Clause-Patent +## + +[Defines] + INF_VERSION = 0x00010005 + BASE_NAME = MockSecureBootVariableLib + FILE_GUID = 4F5C2D34-2E9F-4F1E-9D6E-1F1F8C9D7C3A + MODULE_TYPE = HOST_APPLICATION + VERSION_STRING = 1.0 + LIBRARY_CLASS = SecureBootVariableLib + +# +# The following information is for reference only and not required by the build tools. +# +# VALID_ARCHITECTURES = IA32 X64 +# + +[Sources] + MockSecureBootVariableLib.cpp + +[Packages] + MdePkg/MdePkg.dec + SecurityPkg/SecurityPkg.dec + UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec + +[LibraryClasses] + GoogleTestLib + +[BuildOptions] + MSFT:*_*_*_CC_FLAGS = /EHsc diff --git a/SecurityPkg/Test/SecurityPkgHostTest.dsc b/SecurityPkg/Test/SecurityPkgHostTest.dsc index bbf0f7fc631..3cfabb48c0a 100644 --- a/SecurityPkg/Test/SecurityPkgHostTest.dsc +++ b/SecurityPkg/Test/SecurityPkgHostTest.dsc @@ -26,6 +26,7 @@ SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.inf SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.inf SecurityPkg/Test/Mock/Library/GoogleTest/MockPlatformPKProtectionLib/MockPlatformPKProtectionLib.inf + SecurityPkg/Test/Mock/Library/GoogleTest/MockSecureBootVariableLib/MockSecureBootVariableLib.inf SecurityPkg/Library/DxeTpm2MeasureBootLib/InternalUnitTest/DxeTpm2MeasureBootLibSanitizationTestHost.inf SecurityPkg/Library/DxeTpmMeasureBootLib/InternalUnitTest/DxeTpmMeasureBootLibSanitizationTestHost.inf # @@ -63,6 +64,19 @@ # } # MU_CHANGE - Problems with Consuming OpensslLibFull and UnitTestHostBaseCryptLib from CryptoPkg +SecurityPkg/Library/DxeImageVerificationLib2/GoogleTest/DxeImageVerificationLibGoogleTest.inf { + + UefiRuntimeServicesTableLib|MdePkg/Test/Mock/Library/GoogleTest/MockUefiRuntimeServicesTableLib/MockUefiRuntimeServicesTableLib.inf + UefiBootServicesTableLib|MdePkg/Test/Mock/Library/GoogleTest/MockUefiBootServicesTableLib/MockUefiBootServicesTableLib.inf + DevicePathLib|MdePkg/Test/Mock/Library/GoogleTest/MockDevicePathLib/MockDevicePathLib.inf + DxeImageVerificationLib|SecurityPkg/Library/DxeImageVerificationLib2/DxeImageVerificationLib.inf + BaseCryptLib|CryptoPkg/Test/Mock/Library/GoogleTest/MockBaseCryptLib/MockBaseCryptLib.inf + UefiLib|MdePkg/Test/Mock/Library/GoogleTest/MockUefiLib/MockUefiLib.inf + SecureBootVariableLib|SecurityPkg/Test/Mock/Library/GoogleTest/MockSecureBootVariableLib/MockSecureBootVariableLib.inf + SecurityManagementLib|MdeModulePkg/Test/Mock/Library/GoogleTest/MockSecurityManagementLib/MockSecurityManagementLib.inf + PeCoffLib|MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf + PeCoffExtraActionLib|MdePkg/Library/BasePeCoffExtraActionLibNull/BasePeCoffExtraActionLibNull.inf + } [PcdsPatchableInModule] gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x04