From ff8b6134756fca6b0c55fedc76aeb5000f783875 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Fri, 31 Jan 2020 12:42:48 +0100 Subject: [PATCH 12/12] SecurityPkg/DxeImageVerificationHandler: fix "defer" vs. "deny" policies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RH-Author: Laszlo Ersek Message-id: <20200131124248.22369-13-lersek@redhat.com> Patchwork-id: 93620 O-Subject: [RHEL-8.2.0 edk2 PATCH 12/12] SecurityPkg/DxeImageVerificationHandler: fix "defer" vs. "deny" policies Bugzilla: 1751993 RH-Acked-by: Philippe Mathieu-Daudé RH-Acked-by: Vitaly Kuznetsov In DxeImageVerificationHandler(), we should return EFI_SECURITY_VIOLATION for a rejected image only if the platform sets DEFER_EXECUTE_ON_SECURITY_VIOLATION as the policy for the image's source. Otherwise, EFI_ACCESS_DENIED must be returned. Right now, EFI_SECURITY_VIOLATION is returned for all rejected images, which is wrong -- it causes LoadImage() to hold on to rejected images (in untrusted state), for further platform actions. However, if a platform already set DENY_EXECUTE_ON_SECURITY_VIOLATION, the platform will not expect the rejected image to stick around in memory (regardless of its untrusted state). Therefore, adhere to the platform policy in the return value of the DxeImageVerificationHandler() function. Furthermore, according to "32.4.2 Image Execution Information Table" in the UEFI v2.8 spec, and considering that edk2 only supports (AuditMode==0) at the moment: > When AuditMode==0, if the image's signature is not found in the > authorized database, or is found in the forbidden database, the image > will not be started and instead, information about it will be placed in > this table. we have to store an EFI_IMAGE_EXECUTION_INFO record in both the "defer" case and the "deny" case. Thus, the AddImageExeInfo() call is not being made conditional on (Policy == DEFER_EXECUTE_ON_SECURITY_VIOLATION); the documentation is updated instead. Cc: Chao Zhang Cc: Jian J Wang Cc: Jiewen Yao Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129 Fixes: 5db28a6753d307cdfb1cfdeb2f63739a9f959837 Signed-off-by: Laszlo Ersek Message-Id: <20200116190705.18816-12-lersek@redhat.com> Reviewed-by: Michael D Kinney [lersek@redhat.com: push with Mike's R-b due to Chinese New Year Holiday: ; msgid ] (cherry picked from commit 8b0932c19f31cbf9da26d3b8d4e8d954bdbb5269) Signed-off-by: Miroslav Rezanina --- .../Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c index 015a5b6..dbfbfcb 100644 --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c @@ -1548,7 +1548,8 @@ Done: execution table. @retval EFI_ACCESS_DENIED The file specified by File and FileBuffer did not authenticate, and the platform policy dictates that the DXE - Foundation many not use File. + Foundation may not use File. The image has + been added to the file execution table. **/ EFI_STATUS @@ -1872,7 +1873,8 @@ DxeImageVerificationHandler ( Failed: // - // Policy decides to defer or reject the image; add its information in image executable information table. + // Policy decides to defer or reject the image; add its information in image + // executable information table in either case. // NameStr = ConvertDevicePathToText (File, FALSE, TRUE); AddImageExeInfo (Action, NameStr, File, SignatureList, SignatureListSize); @@ -1885,7 +1887,10 @@ Failed: FreePool (SignatureList); } - return EFI_SECURITY_VIOLATION; + if (Policy == DEFER_EXECUTE_ON_SECURITY_VIOLATION) { + return EFI_SECURITY_VIOLATION; + } + return EFI_ACCESS_DENIED; } /** -- 1.8.3.1