On 10/17/17 21:07, Zmuda, Wojciech wrote: > Hi Laszlo, > > > Thanks for this detailed explanation. I repeated the experiment with EBC > interpreter included, this time stepping much further in the code, and I > actually can see that with Secure Boot enabled the EFI_SECURITY_VIOLATION > status is propagated, which does not let the image to load. With Secure Boot > disabled, I get early EFI_SUCCESS and the image is loaded, as far as I can > see in the debugger. Everything seems to work as designed, then. > > > I have one more question in this matter, if I may ask: why doesn't this path > terminate immediately after finding that the OptionROM is untrusted? It > wouldn't be executed anyway, with or without EBC interpreter, so why bother > with further checking?
"It wouldn't be executed anyway" is not correct -- please search my previous email (also visible below, in the context) for the following string: Trust() DXE Service You can read about that service in the PI spec, version 1.6, Volume 2, 7.3 Dispatcher Services. (It might not apply to files loaded from option ROMs -- but in general, considering the EFI_SECURITY_FILE_AUTHENTICATION_STATE member function's contract, promotion of the security status seems to be the point, after which gBS->StartImage() is expected to succeed. I guess!) I can't answer your question any better, so please talk to SecurityPkg maintainers if you'd like to see further details. Thanks! Laszlo > > > Thank you very much, > > Wojciech > > ________________________________ > From: Laszlo Ersek <[email protected]> > Sent: Monday, October 16, 2017 11:11:01 AM > To: Zmuda, Wojciech > Cc: [email protected] > Subject: Re: [edk2] SecurityPkg: untrusted OptionRom is loaded despite > DENY_EXECUTE_ON_SECURITY_VIOLATION set. > > 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 > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
