Re: SecurityPkg: untrusted OptionRom is loaded despite DENY_EXECUTE_ON_SECURITY_VIOLATION set.

[Laszlo Ersek <lersek@redhat.com>]

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 <lersek@redhat.com>
> Sent: Monday, October 16, 2017 11:11:01 AM
> To: Zmuda, Wojciech
> Cc: edk2-devel@lists.01.org
> 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
>