From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 901A0202E616D for ; Mon, 16 Oct 2017 11:07:28 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1A034C04B31B; Mon, 16 Oct 2017 18:11:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 1A034C04B31B Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-250.rdu2.redhat.com [10.10.120.250]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4385460178; Mon, 16 Oct 2017 18:11:02 +0000 (UTC) To: "Zmuda, Wojciech" References: From: Laszlo Ersek Cc: "edk2-devel@lists.01.org" Message-ID: <205fa9c0-5717-c20d-a5c1-260be05ee8c6@redhat.com> Date: Mon, 16 Oct 2017 20:11:01 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Mon, 16 Oct 2017 18:11:03 +0000 (UTC) Subject: Re: SecurityPkg: untrusted OptionRom is loaded despite DENY_EXECUTE_ON_SECURITY_VIOLATION set. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Oct 2017 18:07:28 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/16/17 18:57, Zmuda, Wojciech wrote: > Hello, > > I'd like to ask you for help with understanding Secure Boot policy > mechanism, specifically DENY_EXECUTE_ON_SECURITY_VIOLATION for > PcdOptionRomImageVerificationPolicy. The OptionROM in my setup is > loaded while, in my opinion, it should be rejected. > > I'm testing the following scenario: Secure Boot is enabled with my own > PK and KEK enrolled, but with no db, to make sure nothing unsigned or > signed by somebody else but me can be executed. A PCIe card with > OptionROM (some EBC code) is installed. > gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy is > set to 0x04 in my platform's package. What I expect, is the OptionROM > execution being denied, as it is not signed by my certificate. What I > observe, on the other hand, is a message on the console, that no EBC > interpreter is found, which suggest, that the OptionROM is loaded, > just fails at the execution of EBC. The same message is printed when > Secure Boot is disabled. > > I tried to understand the code by stepping through it in the DS-5. The > following part of > SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > seems suspicious to me: > > SecurityStatus = gSecurity->FileAuthenticationState ( > gSecurity, > AuthenticationStatus, > OriginalFilePath > ); > } This code is not from "DxeImageVerificationLib.c"; it is from CoreLoadImageCommon() in "MdeModulePkg/Core/Dxe/Image/Image.c". With more context: } else if ((gSecurity != NULL) && (OriginalFilePath != NULL)) { // // Verify the Authentication Status through the Security Architectural Protocol // SecurityStatus = gSecurity->FileAuthenticationState ( gSecurity, AuthenticationStatus, OriginalFilePath ); } > > // > // Check Security Status. > // > if (EFI_ERROR (SecurityStatus) && SecurityStatus != EFI_SECURITY_VIOLATION) { > if (SecurityStatus == EFI_ACCESS_DENIED) { > // > // Image was not loaded because the platform policy prohibits the image from being loaded. > // It's the only place we could meet EFI_ACCESS_DENIED. > // > *ImageHandle = NULL; > } > Status = SecurityStatus; > Image = NULL; > goto Done; > } > > The return code of gSecurity->FileAuthenticationState () (which is > implemented in > MdeModulePkg/Core/Dxe/Image/Image.c:DxeImageVerificationHandler ()), > is EFI_SECURITY_VIOLATION. Such return code skips this if-statement, > that prevents the image to be loaded. According to the comment in the > if-statement: for the policy to be respected, > gSecurity->FileAuthenticationState () should return EFI_ACCESS_DENIED. No, that's a different kind of "platform policy". The PCD that you mention is indeed platform policy, but only for DxeImageVerificationLib. The platform policy being referred-to in this comment is on the level of the gSecurity->FileAuthenticationState() function call; and for that, we have to review the type definition of the EFI_SECURITY_ARCH_PROTOCOL.FileAuthenticationState member function. The type name for this member function is EFI_SECURITY_FILE_AUTHENTICATION_STATE, and it is defined in "MdePkg/Include/Protocol/Security.h". I will not quote the entire description from said header file, just the part that explains the difference: > @retval EFI_SECURITY_VIOLATION The file specified by File did not authenticate, and > the platform policy dictates that File should be placed > in the untrusted state. A file may be promoted from > the untrusted to the trusted state at a future time > with a call to the Trust() DXE Service. > @retval EFI_ACCESS_DENIED The file specified by File did not authenticate, and > the platform policy dictates that File should not be > used for any purpose. Back to your email: On 10/16/17 18:57, Zmuda, Wojciech wrote: > That being said, I stepped through DxeImageVerificationHandler (). The > PCD with OptionROM policy is checked correctly. The function handles > ALWAYS_EXECUTE and NEVER_EXECUTE policies properly and hangs on > QUERY_USER_ and ALLOW_EXECUTE_ON_SECURITY_VIOLATION. This seems fine, > however there is no code handling the > DENY_EXECUTE_ON_SECURITY_VIOLATION (0x04) case. Stepping through this > function shows that the image to be loaded cannot be found in the db > (correct, as there's no db). Then, the function jumps to its very end > and returns EFI_SECURITY_VIOLATION, which skips the aforementioned > if-statement: > > Done: > if (Status != EFI_SUCCESS) { > // > // Policy decides to defer or reject the image; add its information in image executable information table. > // > NameStr = ConvertDevicePathToText (File, FALSE, TRUE); > AddImageExeInfo (Action, NameStr, File, SignatureList, SignatureListSize); > if (NameStr != NULL) { > DEBUG((EFI_D_INFO, "The image doesn't pass verification: %s\n", NameStr)); > FreePool(NameStr); > } > Status = EFI_SECURITY_VIOLATION; > } > > if (SignatureList != NULL) { > FreePool (SignatureList); > } > > return Status; > } This quote is indeed from "SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c", and indeed the EFI_SECURITY_VIOLATION value returned by it causes the "if" statement you quoted from "MdeModulePkg/Core/Dxe/Image/Image.c" to be skipped. However, that's not the whole story. In "MdeModulePkg/Core/Dxe/Image/Image.c", in the CoreLoadImageCommon() function, track the "SecurityStatus" variable from the point where the EFI_ACCESS_DENIED branch is *not* taken, to the end of the function. Near the end of the function, "Status" is EFI_SUCCESS, and thus "SecurityStatus" will be assigned to it (and the function will ultimately return EFI_SECURITY_VIOLATION): Done: // // All done accessing the source file // If we allocated the Source buffer, free it // if (FHand.FreeBuffer) { CoreFreePool (FHand.Source); } if (OriginalFilePath != InputFilePath) { CoreFreePool (OriginalFilePath); } // // There was an error. If there's an Image structure, free it // if (EFI_ERROR (Status)) { if (Image != NULL) { CoreUnloadAndCloseImage (Image, (BOOLEAN)(DstBuffer == 0)); Image = NULL; } } else if (EFI_ERROR (SecurityStatus)) { Status = SecurityStatus; } // // Track the return status from LoadImage. // if (Image != NULL) { Image->LoadImageStatus = Status; } return Status; } Back to your email: On 10/16/17 18:57, Zmuda, Wojciech wrote: > Is there anything I'm doing wrong trying to prevent untrusted > OptionROM execution? If my understanding is correct, my case should > make DxeImageVerificationHandler () return EFI_ACCESS_DENIED here. For > example, in the snippet above, Status should be set to > EFI_ACCESS_DENIED if Policy == DENY_EXECUTE_ON_SECURITY_VIOLATION. To me it seems like everything is working by design. The image is loaded and the security violation is recorded. Please see the documentation of the EFI_SECURITY_VIOLATION return code in the UEFI-2.7 spec, under EFI_BOOT_SERVICES.LoadImage(): Image was loaded and an ImageHandle was created with a valid EFI_LOADED_IMAGE_PROTOCOL.. However, the current platform policy specifies that the image should not be started. (This section also has a pointer to "35.1.5 Deferred Execution".) I believe the misunderstanding comes from the facts that (a) the message CoreLoadPeImage: There is no EBC interpreter for an EBC image. is printed by the CoreLoadPeImage() function, and (b) that CoreLoadImageCommon() calls CoreLoadPeImage() *between* the EFI_ACCESS_DENIED check that you thought should have been taken -- which is not the case -- and the final setting of Status to EFI_SECURITY_VIOLATION (from "SecurityStatus"), which actually *would* occur, if you had an EBC driver included in your platform firmware. In other words, CoreLoadImageCommon() identifies (and prepares to return) the EFI_SECURITY_VIOLATION status code, based on PcdOptionRomImageVerificationPolicy. However, before the function could finish processing (and return this error code), it comes across a more severe error -- no EBC interpreter -- which takes priority both with and without the Secure Boot operational mode. If you want to be completely sure, I suggest including the EBC interpreter in your platform, and retesting. (Based on commit 81d9f86f8a71 ("ArmVirtPkg: enable EBC interpreter for AArch64", 2016-07-29), adding the EBC interpreter should be easy.) Again, to me it looks like everything is working by design. Thanks Laszlo