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

[Laszlo Ersek <lersek@redhat.com>]

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