search

rhboot / shim

UEFI shim loader
Other
819 stars 289 forks source link

Add validation function for Microsoft signing(supersede #531) #567

Open dennis-tseng99 opened 1 year ago

dennis-tseng99 commented 1 year ago

To align line 45 of post-process-pe.c with upstream, the initial value of set_nx_compat variable in PR#531 is also updated to true. To understand why some validation functions were added onto post-process-pe.c, please refer the following link for more previous comments:

Signed-off-by: Dennis Tseng dennis.tseng@suse.com

aronowski commented 1 year ago

I have a few suggestions.

Instead of the help message

fprintf(out, "       -m    Microsoft validation\n");

let's use something that sheds some more light on the matter. Something like

fprintf(out, "       -m    Test for Microsoft's signing requirements\n");

With static bool set_ms_validation = true; the process is already set up to be ran. Why have -m to set it to true again if it's already set and have no -M that sets it to false?

The code block for checking sections characteristics looks improvable. Maybe something like this would be cleaner

for (i = 0, Section = ctx->FirstSection; i < ctx->NumberOfSections; i++, Section++) {
  if ((Section->Characteristics & EFI_IMAGE_SCN_MEM_WRITE) &&
      (Section->Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE)) {
    debug(INFO, "%14s: %s\n","Section-Wr-Exe", "FAIL");
  } else {
    debug(INFO, "%14s: %s\n","Section-Wr-Exe", "PASS");
  }
}

(This is a suggestion I made up on-the-fly, this is not guaranteed to compile or work properly in runtime)

While people that are in the environment of shim development, reviewing shim reviews and whatnot may know about Microsoft requirements, these may change in the future. I'd add comments that clarify what happens, why the function was added and what the requirements are as of today, as well as a link to Microsoft's official posts.

If you want, I can add my way of doing these things. Maybe another comment in this pull request with a diff to your current tree. Or as a .patch file. Or a pull request to your fork. Or something else you prefer.

But for that much code in exchange give me a token of appreciation and co-author me with

Co-authored-by: Kamil Aronowski <kamil.aronowski@yahoo.com>

in a new commit. ;)

dennis-tseng99 commented 1 year ago

@aronowski Thanks for your comments. Actually, I'd like to be a co-author with you. It is my pleasure. I'll put your co-author name on this PR in the later version.

>for (i = 0, Section = ctx->FirstSection; i < ctx->NumberOfSections; i++, Section++) {
  if ((Section->Characteristics & EFI_IMAGE_SCN_MEM_WRITE) &&
      (Section->Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE)) {
    debug(INFO, "%14s: %s\n","Section-Wr-Exe", "FAIL");
  } else {
    debug(INFO, "%14s: %s\n","Section-Wr-Exe", "PASS");
  }
}

Sorry to let you know I will still suggest we can keep "break test-loop even only one section is FAIL", rather than testing through all sections. Actually, I referred a Python tool codes provided by Microsoft to implement our codes when I sent PR#531 last year. Here is the Python codes named image_validation.py:

def execute(self, pe, config_data):
       .....
       for section in pe.sections:
            if (has_characteristic(section.Characteristics, 
SECTION_CHARACTERISTICS["IMAGE_SCN_MEM_EXECUTE"])
               and has_characteristic(section.Characteristics, SECTION_CHARACTERISTICS["IMAGE_SCN_MEM_WRITE"])):
                logging.error(f'[{Result.FAIL}]: Section [{section.Name.decode().strip()}] \
                              should not be both Write and Execute')
                return Result.FAIL
        return Result.PASS

I'd add comments that clarify what happens, why the function was added and what the requirements are as of today, as well as a link to Microsoft's official posts.

I like your idea. But before doing that, please also refer my Conversation 1 in https://github.com/rhboot/shim/pull/531 If you think that is not clear enough, please just put your comments on it.

aronowski commented 1 year ago

You're right about that function. I guess I forgot a return; once and shouldn't code after 8 hours of intellectual work from now on. ;)

Anyway, I implemented the tweaks discussed and fixed some formatting issues. You can use my patch for inspiration - apply it on top of your commit 1b4f664:

From 8bbd6dd52ddc8f46f0128be40ff4a554fac85354 Mon Sep 17 00:00:00 2001
From: Kamil Aronowski <kamil.aronowski@yahoo.com>
Date: Tue, 9 May 2023 10:30:59 +0200
Subject: [PATCH] Some tweaks as discussed in PR 567

Signed-off-by: Kamil Aronowski <kamil.aronowski@yahoo.com>
---
 post-process-pe.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/post-process-pe.c b/post-process-pe.c
index 7083d3e..f39b623 100644
--- a/post-process-pe.c
+++ b/post-process-pe.c
@@ -367,6 +367,22 @@ set_dll_characteristics(PE_COFF_LOADER_IMAGE_CONTEXT *ctx)
 static void
 ms_validation(PE_COFF_LOADER_IMAGE_CONTEXT *ctx)
 {
+   /*
+    * Since November 30th, 2022 Microsoft requires that shim satisifes
+    * certain memory mitigations for it to be signed. Reference:
+    * https://techcommunity.microsoft.com/t5/hardware-dev-center/new-uefi-ca-memory-mitigation-requirements-for-signing/ba-p/3608714
+    * Currently they are implemented here with the codenames:
+    * 4K-Alignment: Section Alignment of the submitted PE file must be
+    * aligned with page size.  This must be 4kb, or a larger power of 2 (ex
+    * 64kb)
+    * Section-Wr-Exe: Section Flags must not combine IMAGE_SCN_MEM_WRITE and
+    * IMAGE_SCN_MEM_EXECUTE for any given section.
+    * NX-Compat-Flag: DLL Characteristics must include
+    * IMAGE_DLLCHARACTERISTICS_NX_COMPAT
+    *
+    * Keep an eye out for new incoming requirements and implement them
+    * here once they have been made public.
+    */
    EFI_IMAGE_SECTION_HEADER *Section;
    int i;

@@ -379,14 +395,11 @@ ms_validation(PE_COFF_LOADER_IMAGE_CONTEXT *ctx)
        PAGE_SIZE == ctx->PEHdr->Pe32Plus.OptionalHeader.SectionAlignment ?
        "PASS":"FAIL");

-   Section = ctx->FirstSection;
-   //printf("NumberOfSections=%d\n",ctx->NumberOfSections);
    for (i=0, Section = ctx->FirstSection; i < ctx->NumberOfSections; i++, Section++) {
-       //printf("name=%s, Charact=0x%04x\n",Section->Name,Section->Characteristics);
        if ((Section->Characteristics & EFI_IMAGE_SCN_MEM_WRITE) &&
-           (Section->Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE)) {
-           debug(INFO, "%14s: %s\n","Section-Wr-Exe", "FAIL");
-               return;
+           (Section->Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE)) {
+           (debug(INFO, "%14s: %s\n","Section-Wr-Exe", "FAIL");
+           return;
        }
    }
    debug(INFO, "%14s: %s\n","Section-Wr-Exe", "PASS");
@@ -541,7 +554,10 @@ int main(int argc, char **argv)
        {.name = "enable-nx-compat",
         .val = 'n',
        },
-       {.name = "ms-validation",
+       {.name = "disable-ms-validation",
+        .val = 'M',
+       },
+       {.name = "enable-ms-validation",
         .val = 'm',
        },
        {.name = "quiet",
@@ -567,11 +583,11 @@ int main(int argc, char **argv)
            set_nx_compat = true;
            break;
        case 'M':
-               set_ms_validation = false;  
+           set_ms_validation = false;
            break;
        case 'm':
            set_ms_validation = true;
-           break;  
+           break;
        case 'q':
            verbosity = MAX(verbosity - 1, MIN_VERBOSITY);
            break;
-- 
2.40.0

and once that's done, recompile shim and check if everything works as intended. Thanks a lot!